chore(deps): upgrade dj-stripe to 2.10 DEV-1918#6863
Conversation
… upgrade compatibility
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request migrates Stripe-related reads to use JSON payloads stored in Sequence DiagramsequenceDiagram
participant Client as Client/API
participant App as Application Code
participant ORM as Django ORM
participant DB as Database (JSONB)
participant djstripe as djstripe Models
Client->>App: Request billing/subscription data
App->>ORM: Query subscriptions (filter stripe_data__status__in=...)
ORM->>djstripe: Resolve Subscription model/query
djstripe->>DB: Read subscription.stripe_data JSONB fields
DB-->>djstripe: Return JSON values (status, period timestamps, unit_amount)
djstripe-->>ORM: Provide annotated fields (casted)
ORM-->>App: Queryset/results with extracted values
App->>App: Convert epoch ints -> timezone-aware datetimes
App-->>Client: Respond with billing/subscription info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
noliveleger
requested review from
Copilot and
jamesrkiger
and removed request for
jnm
noliveleger
changed the title
There was a problem hiding this comment.
Pull request overview
This PR upgrades dj-stripe to the 2.10.x series and updates the codebase to accommodate dj-stripe’s shift of many model fields into stripe_data JSON, including ORM lookups/annotations, raw SQL for the user reports materialized view, and test fixture creation patterns.
Changes:
- Upgrade
dj-stripeto2.10.3and adjust dependency inputs/lockfiles accordingly. - Refactor ORM queries/annotations to use
stripe_data__…JSON lookups and adapt code paths that relied on removed/changed relations (e.g., schedule, plan/product traversal). - Update user reports materialized view SQL + add migrations to safely rebuild it under the new dj-stripe schema; update tests/fixtures to write via
stripe_dataand handle Unix timestamps.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| kobo/apps/user_reports/utils/migrations.py | Updates MV SQL to read subscription/price/product fields from stripe_data JSON. |
| kobo/apps/user_reports/tests/test_user_reports.py | Adjusts bakery fixtures to set dj-stripe fields via stripe_data. |
| kobo/apps/user_reports/migrations/0007_djstripe_update.py | Adds migration to rebuild the MV/indexes for dj-stripe 2.10 schema. |
| kobo/apps/user_reports/migrations/0006_fix_org_subscriptions_missing_metadata.py | Converts prior heavy migration into a no-op placeholder in the chain. |
| kobo/apps/user_reports/migrations/0005_fix_infinite_usage_and_last_updated.py | Removes flake8 header comment. |
| kobo/apps/user_reports/migrations/0004_fix_social_accounts_aggregation.py | Removes flake8 header comment. |
| kobo/apps/user_reports/migrations/0003_fix_user_role_scopping.py | Removes flake8 header comment. |
| kobo/apps/user_reports/migrations/0002_create_user_reports_mv.py | Removes flake8 header comment. |
| kobo/apps/trackers/tests/test_utils.py | Updates dj-stripe model creation to use stripe_data. |
| kobo/apps/stripe/views.py | Updates subscription filtering + replaces price max aggregation with JSON extraction + cast. |
| kobo/apps/stripe/utils/subscription_limits.py | Updates active subscription filtering and replaces plan traversal with item/price/product traversal; casts JSON timestamps. |
| kobo/apps/stripe/utils/billing_dates.py | Updates org billing-period queries to filter/calculate based on stripe_data timestamps. |
| kobo/apps/stripe/tests/utils.py | Updates shared test helpers/fixtures to populate dj-stripe models via stripe_data and timestamps. |
| kobo/apps/stripe/tests/test_subscription_api.py | Updates fixtures to set billing_scheme via stripe_data. |
| kobo/apps/stripe/tests/test_stripe_utils.py | Updates tests for timestamp precision/type changes + item/product access changes. |
| kobo/apps/stripe/tests/test_product_api.py | Updates price fixtures to set billing_scheme via stripe_data. |
| kobo/apps/stripe/tests/test_organization_usage.py | Updates tests to compare ISO strings derived from Unix timestamps in dj-stripe properties. |
| kobo/apps/stripe/tests/test_one_time_addons_api.py | Updates subscription fixture status to be stored in stripe_data. |
| kobo/apps/stripe/tests/test_modify_subscription_api.py | Updates fixtures to set price/subscription item/subscription fields via stripe_data. |
| kobo/apps/stripe/tests/test_mfa_login.py | Updates fixtures to attach subscription items correctly and store status/unit_amount in stripe_data. |
| kobo/apps/stripe/tests/test_customer_portal_api.py | Adjusts subscription creation to work with new subscription-item patterns and stripe_data status. |
| kobo/apps/stripe/serializers.py | Updates queryset filter to use stripe_data__status. |
| kobo/apps/stripe/models.py | Updates runtime logic and queries referencing moved fields (status/refunded) to stripe_data. |
| kobo/apps/stripe/migrations/0005_djstripe_2_10.py | Adds a bridge migration depending on djstripe.0002_2_10 to avoid migration history issues. |
| kobo/apps/stripe/migrations/0001_initial.py | Updates djstripe dependency to 0001_initial for broader compatibility. |
| kobo/apps/organizations/models.py | Updates subscription-related queries to use stripe_data__… JSON lookups. |
| kobo/apps/accounts/utils.py | Updates paid-subscription checks to use stripe_data unit_amount/status lookups. |
| dependencies/pip/requirements.txt | Pins dj-stripe==2.10.3 and updates compiled dependency set. |
| dependencies/pip/requirements.in | Loosens input constraint to dj-stripe>=2.10. |
| dependencies/pip/dev_requirements.txt | Pins dev dependency set to include dj-stripe==2.10.3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| anchor=Max( | ||
| Cast( | ||
| F('djstripe_customers__subscriptions__stripe_data__ended_at'), | ||
| output_field=BigIntegerField(), | ||
| ) | ||
| ), |
There was a problem hiding this comment.
These annotations cast JSON subkeys (e.g., stripe_data__ended_at) to BigIntegerField. If the underlying JSON key transform resolves to a JSONB value (Postgres ->), casting JSONB directly to bigint will fail. Prefer extracting as text first (e.g., KeyTextTransform('ended_at', ...) / ->>), then casting, so Max() and comparisons work reliably.
| sub_start_date=Cast( | ||
| F('stripe_data__start_date'), | ||
| output_field=BigIntegerField(), | ||
| ), |
There was a problem hiding this comment.
sub_start_date is derived from stripe_data__start_date and cast to BigIntegerField(). If the JSON key transform yields JSONB (Postgres ->), the cast will fail at runtime. Extract the JSON key as text first (e.g., KeyTextTransform / ->>) and then cast, to guarantee numeric ordering in the window functions.
| unique_products = set() | ||
| for subscription in subscriptions: | ||
| unique_plans.add(subscription.plan) | ||
| for item in subscription.items.select_related('price__product').all(): | ||
| if item.price and item.price.product: | ||
| unique_products.add(item.price.product) |
There was a problem hiding this comment.
get_plan_name() now loops through subscriptions and, inside that loop, runs subscription.items.select_related(...).all(). This will issue an extra query per subscription (N+1) and can become expensive for orgs with many subscriptions. Consider prefetching items (with price__product) on the initial subscriptions queryset and then iterating subscription.items.all() without per-subscription queries.
| .annotate( | ||
| highest_unit_amount=Max( | ||
| Cast( | ||
| KeyTransform('unit_amount', 'prices__stripe_data'), | ||
| output_field=IntegerField(), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
KeyTransform('unit_amount', 'prices__stripe_data') returns a JSON/JSONB expression; casting that directly to IntegerField() is likely to error on Postgres (cannot cast type jsonb to integer). Use a text transform (e.g., KeyTextTransform) or extract the key as text (->>) before casting so Max() can operate on a numeric type.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
kobo/apps/stripe/models.py (1)
86-94:⚠️ Potential issue | 🟠 MajorUse
has_any_keyshere and filter empty tags.Blank
valid_tagsbecomes['']withsplit(','), making the condition at line 88 truthy even when no tags are specified. Additionally,__has_key__inis not a standard Django JSONField lookup; the correct lookup ishas_any_keys. This combination causes untagged add-ons to be incorrectly treated as tagged, and can reject organizations that should be eligible to purchase the add-on.Suggested fix
- tags = product.metadata.get('valid_tags', '').split(',') + tags = [ + tag for tag in product.metadata.get('valid_tags', '').split(',') + if tag + ] if ( tags and ('all' not in tags) and not Subscription.objects.filter( customer__subscriber=organization, - items__price__product__metadata__has_key__in=[tags], + items__price__product__metadata__has_any_keys=tags, stripe_data__status__in=ACTIVE_STRIPE_STATUSES, ).exists() ):kobo/apps/organizations/models.py (1)
116-145:⚠️ Potential issue | 🟠 MajorMake the selected billing row deterministic.
This queryset now expands one subscription into one row per subscription item, but
.first()is still only ordered bysubscriptions__stripe_data__start_date. If the newest subscription has multiple items, the returnedprice_idand metadata depend on join order instead of a defined contract.
🧹 Nitpick comments (5)
dependencies/pip/requirements.in (1)
30-30: Consider adding an upper bound constraint.The
>=2.10constraint allows any future dj-stripe version, including potential major releases (e.g., 3.x) that could introduce breaking changes. Consider constraining to>=2.10,<3to prevent unexpected upgrade issues while still allowing patch/minor updates.Suggested constraint
-dj-stripe>=2.10 +dj-stripe>=2.10,<3kobo/apps/accounts/utils.py (1)
11-19: Plan indexes for these JSON-backed auth checks.These helpers now run through
stripe_datakeys on the login/MFA path. Django documents that the default B-tree index isn't particularly helpful forJSONFieldqueries and that PostgreSQL should use JSON-appropriate indexing instead, so I'd plan dedicated indexes around these lookups if auth latency moves after this upgrade. (docs.djangoproject.com)Also applies to: 27-31
kobo/apps/stripe/utils/subscription_limits.py (2)
248-269: Add expression indexes for the new JSON-backed lookups.
statusfiltering andstart_dateranking used to ride normal columns; now this request path has to read and cast values out ofstripe_databefore the window functions run. Ifdjstripe_subscriptiondoes not already have matching expression indexes (for example on((stripe_data->>'status'))and((stripe_data->>'start_date')::bigint)), this will get noticeably slower as the table grows.
314-326: Prefetch subscription items and stabilize plan-name ordering.This loop does one
itemsquery per subscription, and thesetplus boolean-only sort makes the final"A and B"string unstable when there are multiple products of the same type. Prefetch once and sort with a deterministic secondary key.♻️ Possible refactor
- unique_products = set() + subscriptions = subscriptions.prefetch_related('items__price__product') + unique_products = {} for subscription in subscriptions: - for item in subscription.items.select_related('price__product').all(): - if item.price and item.price.product: - unique_products.add(item.price.product) + for item in subscription.items.all(): + product = getattr(getattr(item, 'price', None), 'product', None) + if product: + unique_products[product.pk] = product # Make sure plans come before addons product_list = sorted( - unique_products, - key=lambda product: product.metadata.get('product_type', '') == 'plan', - reverse=True, + unique_products.values(), + key=lambda product: ( + product.metadata.get('product_type') != 'plan', + product.name, + ), )kobo/apps/stripe/tests/utils.py (1)
94-115: Prefer building theSubscriptionItemlazily.
baker.make()writes theSubscriptionItembefore the intendedSubscriptionexists, so this helper either does an extra write or depends onmodel_bakery/djstripeFK defaults while the relation is still unset.baker.prepare()here would avoid that intermediate state.♻️ Suggested cleanup
- subscription_item = baker.make( - SubscriptionItem, - price=price, - livemode=False, - plan=plan, - stripe_data={'quantity': 1}, - ) subscription = baker.make( Subscription, customer=customer, livemode=False, metadata=subscription_metadata, stripe_data={ 'status': status, 'billing_cycle_anchor': int((created_date - period_offset).timestamp()), 'current_period_end': int((created_date + period_offset).timestamp()), 'current_period_start': int((created_date - period_offset).timestamp()), 'start_date': int(created_date.timestamp()), }, ) - subscription_item.subscription = subscription + subscription_item = baker.prepare( + SubscriptionItem, + subscription=subscription, + price=price, + livemode=False, + plan=plan, + stripe_data={'quantity': 1}, + ) subscription_item.save()
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecea96e0-1e1a-4f72-9375-f1fb62ca5b1e
📒 Files selected for processing (30)
dependencies/pip/dev_requirements.txtdependencies/pip/requirements.independencies/pip/requirements.txtkobo/apps/accounts/utils.pykobo/apps/organizations/models.pykobo/apps/stripe/migrations/0001_initial.pykobo/apps/stripe/migrations/0005_djstripe_2_10.pykobo/apps/stripe/models.pykobo/apps/stripe/serializers.pykobo/apps/stripe/tests/test_customer_portal_api.pykobo/apps/stripe/tests/test_mfa_login.pykobo/apps/stripe/tests/test_modify_subscription_api.pykobo/apps/stripe/tests/test_one_time_addons_api.pykobo/apps/stripe/tests/test_organization_usage.pykobo/apps/stripe/tests/test_product_api.pykobo/apps/stripe/tests/test_stripe_utils.pykobo/apps/stripe/tests/test_subscription_api.pykobo/apps/stripe/tests/utils.pykobo/apps/stripe/utils/billing_dates.pykobo/apps/stripe/utils/subscription_limits.pykobo/apps/stripe/views.pykobo/apps/trackers/tests/test_utils.pykobo/apps/user_reports/migrations/0002_create_user_reports_mv.pykobo/apps/user_reports/migrations/0003_fix_user_role_scopping.pykobo/apps/user_reports/migrations/0004_fix_social_accounts_aggregation.pykobo/apps/user_reports/migrations/0005_fix_infinite_usage_and_last_updated.pykobo/apps/user_reports/migrations/0006_fix_org_subscriptions_missing_metadata.pykobo/apps/user_reports/migrations/0007_djstripe_update.pykobo/apps/user_reports/tests/test_user_reports.pykobo/apps/user_reports/utils/migrations.py
💤 Files with no reviewable changes (4)
- kobo/apps/user_reports/migrations/0002_create_user_reports_mv.py
- kobo/apps/user_reports/migrations/0005_fix_infinite_usage_and_last_updated.py
- kobo/apps/user_reports/migrations/0004_fix_social_accounts_aggregation.py
- kobo/apps/user_reports/migrations/0003_fix_user_role_scopping.py
… djstripe 2.10 property-backed fields
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
static/openapi/schema_v2.yaml (1)
17945-17952:⚠️ Potential issue | 🟠 MajorModel subscription item
quantityas nullable.Stripe's subscription item schema marks
quantityas nullable, and the same docs note that metered prices aggregate usage instead of billing a fixed quantity. Keeping this as a required non-null integer means valid metered items won't be represented correctly in the OpenAPI contract.Suggested fix
quantity: type: integer + nullable: true
♻️ Duplicate comments (1)
kobo/apps/user_reports/migrations/0007_djstripe_update.py (1)
5-9:⚠️ Potential issue | 🟠 MajorFreeze SQL inside this migration to keep it reproducible.
Line 5-Line 9 and Line 31-Line 34 make this migration depend on whichever SQL the helper module contains at runtime. That breaks migration determinism on fresh installs/upgrades.
Suggested fix
-from kobo.apps.user_reports.utils.migrations import ( - CREATE_INDEXES_SQL, - CREATE_MV_SQL, - DROP_MV_SQL, -) +# Snapshot the SQL at migration creation time to keep this migration immutable. +DROP_MV_SQL = """ +-- paste exact DROP SQL snapshot here +""" + +CREATE_MV_SQL = """ +-- paste exact CREATE MATERIALIZED VIEW SQL snapshot here +""" + +CREATE_INDEXES_SQL = """ +-- paste exact CREATE INDEX SQL snapshot here +"""#!/bin/bash # Verify this migration imports live helper SQL and therefore is not self-contained. rg -n "from kobo.apps.user_reports.utils.migrations import|CREATE_MV_SQL|DROP_MV_SQL|CREATE_INDEXES_SQL" \ kobo/apps/user_reports/migrations/0007_djstripe_update.py # Show helper SQL definitions currently referenced at runtime. fd migrations.py kobo/apps/user_reports/utils --exec sed -n '1,260p' {} # Check whether other migrations have the same pattern. rg -n "from kobo.apps.user_reports.utils.migrations import" kobo/apps/user_reports/migrationsAlso applies to: 31-34
🧹 Nitpick comments (2)
kobo/apps/organizations/models.py (1)
157-169: Verifyended_attimestamp format is handled correctly downstream.Similar to other timestamp fields,
ended_atwill now be a Unix timestamp (integer) fromstripe_data. The return value at Line 169 (qs['anchor']) will be an integer, not a datetime. Confirm this is the expected behavior for callers ofcanceled_subscription_billing_cycle_anchor().static/openapi/schema_v2.json (1)
17521-17523: Keep the enum contract for Stripetypefields if the value set has not changed.Collapsing these to
string | nulldrops generated enums and client-side validation without changing the wire format. If the backend still only returns Stripe’s fixed values here, an explicitChoiceFieldor schema override would keep the OpenAPI contract precise.Also applies to: 23429-23431, 23494-23496, 23557-23560
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2eef314e-53ec-4952-964e-37f8c318453c
📒 Files selected for processing (36)
.github/workflows/pytest.ymldependencies/pip/requirements.injsapp/js/api/models/baseProduct.tsjsapp/js/api/models/blankEnum.tsjsapp/js/api/models/collectionMethodEnum.tsjsapp/js/api/models/price.tsjsapp/js/api/models/priceTransformQuantity.tsjsapp/js/api/models/priceWithProduct.tsjsapp/js/api/models/priceWithProductTransformQuantity.tsjsapp/js/api/models/product.tsjsapp/js/api/models/prorationBehaviorEnum.tsjsapp/js/api/models/stripePriceType.tsjsapp/js/api/models/stripeProductType.tsjsapp/js/api/models/subscription.tsjsapp/js/api/models/subscriptionBillingThresholds.tsjsapp/js/api/models/subscriptionDiscount.tsjsapp/js/api/models/subscriptionItem.tsjsapp/js/api/models/subscriptionPauseCollection.tsjsapp/js/api/models/subscriptionPendingInvoiceItemInterval.tsjsapp/js/api/models/subscriptionPendingUpdate.tsjsapp/js/api/models/subscriptionProrationBehavior.tsjsapp/js/api/models/subscriptionStatusEnum.tskobo/apps/organizations/models.pykobo/apps/stripe/migrations/0001_initial.pykobo/apps/stripe/models.pykobo/apps/stripe/serializers.pykobo/apps/stripe/tests/test_customer_portal_api.pykobo/apps/stripe/tests/test_modify_subscription_api.pykobo/apps/stripe/tests/test_stripe_utils.pykobo/apps/stripe/utils/billing_dates.pykobo/apps/stripe/views.pykobo/apps/trackers/tests/test_utils.pykobo/apps/user_reports/migrations/0007_djstripe_update.pykobo/apps/user_reports/utils/migrations.pystatic/openapi/schema_v2.jsonstatic/openapi/schema_v2.yaml
💤 Files with no reviewable changes (15)
- jsapp/js/api/models/priceTransformQuantity.ts
- jsapp/js/api/models/priceWithProductTransformQuantity.ts
- jsapp/js/api/models/subscriptionPauseCollection.ts
- jsapp/js/api/models/subscriptionPendingInvoiceItemInterval.ts
- jsapp/js/api/models/blankEnum.ts
- jsapp/js/api/models/subscriptionProrationBehavior.ts
- jsapp/js/api/models/subscriptionPendingUpdate.ts
- jsapp/js/api/models/stripePriceType.ts
- jsapp/js/api/models/prorationBehaviorEnum.ts
- jsapp/js/api/models/subscriptionBillingThresholds.ts
- jsapp/js/api/models/subscriptionDiscount.ts
- jsapp/js/api/models/stripeProductType.ts
- jsapp/js/api/models/subscriptionStatusEnum.ts
- jsapp/js/api/models/collectionMethodEnum.ts
- jsapp/js/api/models/subscription.ts
✅ Files skipped from review due to trivial changes (1)
- kobo/apps/stripe/tests/test_customer_portal_api.py
🚧 Files skipped from review as they are similar to previous changes (7)
- kobo/apps/stripe/tests/test_modify_subscription_api.py
- kobo/apps/stripe/tests/test_stripe_utils.py
- kobo/apps/stripe/views.py
- kobo/apps/stripe/migrations/0001_initial.py
- kobo/apps/stripe/utils/billing_dates.py
- kobo/apps/trackers/tests/test_utils.py
- dependencies/pip/requirements.in
| dependencies = [('user_reports', '0006_fix_org_subscriptions_missing_metadata')] | ||
|
|
||
| operations = [ | ||
| migrations.RunPython(apply_fix, reverse_code=migrations.RunPython.noop), | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify RUF012 applicability and local patterns.
for f in pyproject.toml .ruff.toml ruff.toml; do
[ -f "$f" ] && echo "== $f ==" && rg -n "RUF012|select|extend-select|ignore|extend-ignore" "$f";
done
rg -n "dependencies = \\[|operations = \\[" \
kobo/apps/user_reports/migrations/0007_djstripe_update.pyRepository: kobotoolbox/kpi
Length of output: 600
🏁 Script executed:
cat -n ruff.tomlRepository: kobotoolbox/kpi
Length of output: 1360
🏁 Script executed:
cat -n kobo/apps/user_reports/migrations/0007_djstripe_update.pyRepository: kobotoolbox/kpi
Length of output: 1547
Use tuples for dependencies and operations to satisfy RUF012.
Ruff flags these class attributes as mutable defaults. Converting lists to tuples removes the warning with no behavior change.
Suggested fix
- dependencies = [('user_reports', '0006_fix_org_subscriptions_missing_metadata')]
+ dependencies = (('user_reports', '0006_fix_org_subscriptions_missing_metadata'),)
- operations = [
- migrations.RunPython(apply_fix, reverse_code=migrations.RunPython.noop),
- ]
+ operations = (
+ migrations.RunPython(apply_fix, reverse_code=migrations.RunPython.noop),
+ )🧰 Tools
🪛 Ruff (0.15.6)
[warning] 39-39: Mutable default value for class attribute
(RUF012)
[warning] 41-43: Mutable default value for class attribute
(RUF012)
| "transform_quantity": { | ||
| "nullable": true, | ||
| "description": "Apply a transformation to the reported usage or set quantity before computing the amount billed. Cannot be combined with `tiers`." | ||
| "nullable": true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file size and structure
wc -l static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 94
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '23459,23465p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 321
🏁 Script executed:
# Also check the second location mentioned (23525-23527)
sed -n '23523,23529p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 304
🏁 Script executed:
# Check if there's git history to see what was removed
cd static/openapi && git log -1 --oneline -- schema_v2.json 2>/dev/null || echo "No git history available"Repository: kobotoolbox/kpi
Length of output: 156
🏁 Script executed:
# Look for any other definitions of transform_quantity in the file to understand the schema structure
rg "transform_quantity" static/openapi/schema_v2.json -B 2 -A 5Repository: kobotoolbox/kpi
Length of output: 3959
🏁 Script executed:
# Search for the complete transform_quantity schema definition
rg "transform_quantity" static/openapi/schema_v2.json -B 1 -A 8 | head -80Repository: kobotoolbox/kpi
Length of output: 4461
🏁 Script executed:
# Look for where the object schema with properties is defined
rg '"divide_by"' static/openapi/schema_v2.json -B 10 -A 3Repository: kobotoolbox/kpi
Length of output: 1121
🏁 Script executed:
# Check if there's a reusable schema component defined for transform_quantity
rg -A 15 '"TransformQuantity"' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Look at the complete context where the full transform_quantity schema is defined
sed -n '20000,20100p' static/openapi/schema_v2.json | rg -A 12 "transform_quantity"Repository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Get a better view of the schema structure around the full definition
ast-grep --pattern 'transform_quantity: {
type: "object",
$$$
}'Repository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Find all line numbers where transform_quantity appears
rg -n "transform_quantity" static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 2355
🏁 Script executed:
# Check the line numbers to understand the structure better
rg -n "transform_quantity" static/openapi/schema_v2.json | head -20Repository: kobotoolbox/kpi
Length of output: 2355
🏁 Script executed:
# Check the full schema definition starting at line 26398
sed -n '26395,26420p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 1671
🏁 Script executed:
# Let's also see more context around this to understand the schema structure
sed -n '26390,26430p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 2734
🏁 Script executed:
# Check if there's a components/schemas section with a TransformQuantity definition
rg -n "components.*schemas" static/openapi/schema_v2.json | head -5Repository: kobotoolbox/kpi
Length of output: 519
🏁 Script executed:
# Let's look for any schema definitions that could be reused
sed -n '1,100p' static/openapi/schema_v2.json | grep -i schemaRepository: kobotoolbox/kpi
Length of output: 476
🏁 Script executed:
# Check the structure of the file to find components section
rg -n '"components"' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 84
🏁 Script executed:
# Check the components section to see if there's a schema definition for TransformQuantity
sed -n '15941,16000p' static/openapi/schema_v2.json | head -50Repository: kobotoolbox/kpi
Length of output: 1901
🏁 Script executed:
# Look for all schema definitions that might contain transform_quantity as a reusable component
sed -n '15941,27012p' static/openapi/schema_v2.json | rg -B 5 -A 15 '"TransformQuantity"'Repository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Check what schemas are defined in the components section
sed -n '15941,20000p' static/openapi/schema_v2.json | rg '"([A-Z][a-zA-Z]+)":\s*\{' | head -30Repository: kobotoolbox/kpi
Length of output: 1205
🏁 Script executed:
# Get a list of all schemas to see if TransformQuantity exists
sed -n '15941,27012p' static/openapi/schema_v2.json | rg '^\s*"[A-Z][a-zA-Z]+":\s*\{' | grep -i transformRepository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Check the Price schema definition around line 23450-23470
sed -n '23440,23480p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 1655
🏁 Script executed:
# Check the PriceWithProduct schema around line 23510-23540
sed -n '23510,23550p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 1748
🏁 Script executed:
# Let's see what the git commit changed - check if we can see the previous state
git show HEAD:static/openapi/schema_v2.json | sed -n '23461,23463p' 2>/dev/null || echo "Cannot view previous version"Repository: kobotoolbox/kpi
Length of output: 165
🏁 Script executed:
# Check the context where transform_quantity is fully defined (line 26398) to understand the schema pattern
sed -n '26380,26435p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 3910
🏁 Script executed:
# Let's understand what schema this full definition belongs to
sed -n '26300,26400p' static/openapi/schema_v2.json | tail -60Repository: kobotoolbox/kpi
Length of output: 4611
🏁 Script executed:
# Check what schema contains the line 26398 definition by looking backwards
sed -n '26100,26400p' static/openapi/schema_v2.json | grep -n '"[A-Z][a-zA-Z]*":' | tail -20Repository: kobotoolbox/kpi
Length of output: 102
🏁 Script executed:
# Let's find the parent schema name for line 26398
sed -n '26000,26398p' static/openapi/schema_v2.json | grep '"[A-Z][a-zA-Z]*":\s*{' | tail -5Repository: kobotoolbox/kpi
Length of output: 207
🏁 Script executed:
# Check if there are any $ref references to a TransformQuantity component definition
rg '\$ref.*TransformQuantity' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Check the context before line 26398 to find the parent schema
sed -n '26200,26300p' static/openapi/schema_v2.json | grep -E '^\s*"[^"]+": \{$' | tail -10Repository: kobotoolbox/kpi
Length of output: 614
🏁 Script executed:
# Find the schema name that contains line 26398
awk 'NR <= 26398 { line=$0; nr=NR } NR > 26398 { if (line ~ /"[A-Z][a-zA-Z]*": \{/) { print nr": "line; exit } }' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Let's look at what's around line 26100 to find the schema header
sed -n '26100,26150p' static/openapi/schema_v2.json | grep -B 50 -A 5 '"[A-Z][a-zA-Z]*": {' | tail -20Repository: kobotoolbox/kpi
Length of output: 41
🏁 Script executed:
# Better approach: search backwards from line 26398 for a top-level schema definition
python3 << 'EOF'
with open('static/openapi/schema_v2.json', 'r') as f:
lines = f.readlines()
# Find the schema containing line 26398 (0-indexed: 26397)
depth = 0
for i in range(26397, -1, -1):
line = lines[i]
# Count braces
depth += line.count('{') - line.count('}')
if '"' in line and '": {' in line and depth == 1:
print(f"Line {i+1}: {line.strip()}")
break
EOFRepository: kobotoolbox/kpi
Length of output: 94
🏁 Script executed:
# Let's look at a broader range to find the schema name containing transform_quantity at line 26398
sed -n '26200,26250p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 2163
🏁 Script executed:
# Look further back for the schema definition
sed -n '26000,26100p' static/openapi/schema_v2.json | tail -50Repository: kobotoolbox/kpi
Length of output: 1687
🏁 Script executed:
# Let's look at lines around 26398 more carefully to understand the context
sed -n '26350,26450p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 7286
🏁 Script executed:
# Search for what schema names exist around that area
sed -n '26000,27000p' static/openapi/schema_v2.json | grep -E '^\s{12}"[A-Z][a-zA-Z]*":\s*\{' | head -20Repository: kobotoolbox/kpi
Length of output: 426
🏁 Script executed:
# Let me find the parent schema for line 26398 by looking at indentation pattern
python3 << 'EOF'
with open('static/openapi/schema_v2.json', 'r') as f:
lines = f.readlines()
# Look at indentation to find parent
for i in range(26398, 25900, -1):
line = lines[i]
# Count leading spaces
indent = len(line) - len(line.lstrip())
# Find schema definition at 12-space indent (parent level)
if indent == 12 and '": {' in line:
print(f"Line {i+1}: {line.rstrip()}")
break
EOFRepository: kobotoolbox/kpi
Length of output: 111
🏁 Script executed:
# Let's check if the simplified version at lines 23461-23463 uses a $ref instead
sed -n '23459,23465p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 321
🏁 Script executed:
# Same for the other location
sed -n '23523,23529p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 304
🏁 Script executed:
# Find which schema contains line 23461
python3 << 'EOF'
with open('static/openapi/schema_v2.json', 'r') as f:
lines = f.readlines()
for i in range(23460, 23000, -1):
line = lines[i]
indent = len(line) - len(line.lstrip())
if indent == 12 and '": {' in line:
print(f"Line {i+1}: {line.rstrip()}")
break
EOFRepository: kobotoolbox/kpi
Length of output: 93
🏁 Script executed:
# Find which schema contains line 23525
python3 << 'EOF'
with open('static/openapi/schema_v2.json', 'r') as f:
lines = f.readlines()
for i in range(23524, 23000, -1):
line = lines[i]
indent = len(line) - len(line.lstrip())
if indent == 12 and '": {' in line:
print(f"Line {i+1}: {line.rstrip()}")
break
EOFRepository: kobotoolbox/kpi
Length of output: 104
🏁 Script executed:
# Now let's verify the actual issue - check if there was a full schema before and it was removed
# Look at the Price schema completely
sed -n '23430,23475p' static/openapi/schema_v2.json | head -50Repository: kobotoolbox/kpi
Length of output: 1889
🏁 Script executed:
# Let's verify the complete Price schema definition
sed -n '23412,23475p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 2591
🏁 Script executed:
# And the complete PriceWithProduct schema
sed -n '23477,23540p' static/openapi/schema_v2.jsonRepository: kobotoolbox/kpi
Length of output: 2558
Don't replace transform_quantity with a bare nullable flag.
Lines 23461 and 23525 remove the field's schema definition entirely. This breaks code generation and validation for clients; they'll have no way to know transform_quantity should be an object with divide_by and round properties. The same field is properly defined elsewhere in the schema (line 26398) with type: "object" and properties. Keep that structure and mark it nullable instead.
Also applies to: 23525-23527
| description: | ||
| type: string | ||
| nullable: true | ||
| description: A description of this object. | ||
| readOnly: true | ||
| type: | ||
| allOf: | ||
| - $ref: '#/components/schemas/StripeProductType' | ||
| description: |- | ||
| The type of the product. The product is either of type `good`, which is eligible for use with Orders and SKUs, or `service`, which is eligible for use with Subscriptions and Plans. | ||
|
|
||
| * `good` - Good | ||
| * `service` - Service | ||
| type: string | ||
| nullable: true | ||
| metadata: | ||
| type: object | ||
| additionalProperties: | ||
| type: string | ||
| readOnly: true | ||
| required: | ||
| - description | ||
| - id | ||
| - metadata | ||
| - name | ||
| - type |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the specific lines
wc -l static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 94
🏁 Script executed:
# Examine the BaseProduct schema at lines 12584-12600
sed -n '12584,12600p' static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 402
🏁 Script executed:
# Examine the Product schema at lines 16850-16871
sed -n '16850,16871p' static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 519
🏁 Script executed:
# Let's also check the Price schema at lines 16754-16789
sed -n '16754,16789p' static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 1074
🏁 Script executed:
# Check the .editorconfig file for linting rules
cat .editorconfigRepository: kobotoolbox/kpi
Length of output: 386
🌐 Web query:
Stripe API Product object description field nullable
💡 Result:
Yes. In Stripe’s API, Product.description is a nullable string, so it can be null when unset. [1]
When updating a product, Stripe’s docs also show description being set to null (e.g., in the Node example), indicating you can clear it by updating with description: null. [2]
Sources:
[1] Stripe API Reference — Product object (description (string, nullable))
[2] Stripe API Reference — Update a product (Node example includes "description": null)
🌐 Web query:
Stripe API SubscriptionItem quantity field nullable
💡 Result:
Yes. In Stripe’s API, SubscriptionItem.quantity is defined as integer, nullable on the Subscription Item object. [1]
In practice, quantity is typically only meaningful/set for recurring[usage_type]=licensed (per-seat). For recurring[usage_type]=metered, you can’t specify a quantity (billing is based on usage records), so quantity may be absent/null depending on how you retrieve/deserialize the object. [2]
Sources:
[1] Stripe API Reference – “The Subscription Item object” (quantity (integer, nullable)) (docs.stripe.com)
[2] Stripe Billing Docs – “Set product or subscription quantities” (quantity only for usage_type=licensed) (docs.stripe.com)
Citations:
- 1: https://docs.stripe.com/api/subscription_items/object
- 2: https://docs.stripe.com/billing/subscriptions/multiple-products
🏁 Script executed:
# Check SubscriptionItem schema around lines 17945-17952
sed -n '17940,17960p' static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 504
🏁 Script executed:
# Let's also look at a broader context around SubscriptionItem to see the full schema
sed -n '17900,17970p' static/openapi/schema_v2.yamlRepository: kobotoolbox/kpi
Length of output: 2272
Add nullable: true to description on Stripe products.
Stripe's API documents product description as a nullable string, and the actual API can return "description": null. Without nullable: true, both BaseProduct and Product schemas are stricter than the API they describe, which breaks generated clients and tests for valid products without a description.
Suggested fix
description:
type: string
+ nullable: true
readOnly: trueAlso applies to: Product schema (16850–16871)
… casts and fix test fixture data
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
| start=F( | ||
| 'djstripe_customers__subscriptions__stripe_data__current_period_start' | ||
| ), | ||
| end=F('djstripe_customers__subscriptions__stripe_data__current_period_end'), | ||
| interval=F( |
There was a problem hiding this comment.
start/end JSON lookups not explicitly cast to integer — inconsistent with start_date
start_date uses Cast(KeyTextTransform(...), output_field=BigIntegerField()) to guarantee a typed integer for the Window-function comparison. However, start and end use plain F() expressions on the same JSONField:
start=F('djstripe_customers__subscriptions__stripe_data__current_period_start'),
end=F('djstripe_customers__subscriptions__stripe_data__current_period_end'),These values are later passed directly to datetime.fromtimestamp(res['start'], ...), which requires a numeric type. While psycopg2 will typically return a Python int for a JSON integer, the behaviour is implicitly relied upon rather than enforced. If the JSONB value were stored as a float or if psycopg2's JSON type-coercion behaved differently, fromtimestamp would receive an unexpected type at runtime.
For consistency and robustness, consider applying the same Cast(KeyTextTransform(...), output_field=BigIntegerField()) pattern used for start_date:
start=Cast(
KeyTextTransform(
'current_period_start',
'djstripe_customers__subscriptions__stripe_data',
),
output_field=BigIntegerField(),
),
end=Cast(
KeyTextTransform(
'current_period_end',
'djstripe_customers__subscriptions__stripe_data',
),
output_field=BigIntegerField(),
),| 'latest_invoice', sub.stripe_data->>'latest_invoice', | ||
| 'pending_setup_intent', sub.stripe_data->>'pending_setup_intent', | ||
| 'plan', sub.stripe_data->>'plan', | ||
| 'default_tax_rates', COALESCE(sub.stripe_data->'default_tax_rates', '[]'::jsonb) |
There was a problem hiding this comment.
default_tax_rates data shape has changed — downstream consumers may break
Previously, default_tax_rates was assembled by JOINing djstripe_djstripesubscriptiondefaulttaxrate and djstripe_taxrate, producing a full jsonb_agg of rich TaxRate objects (id, description, display_name, inclusive, percentage, etc.).
With this change it becomes:
'default_tax_rates', COALESCE(sub.stripe_data->'default_tax_rates', '[]'::jsonb)In Stripe's raw API response (which dj-stripe 2.10 stores verbatim in stripe_data), default_tax_rates is an array of expanded TaxRate objects only if Stripe was asked to expand it during sync. If the subscription was synced without expansion, default_tax_rates may contain just an array of ID strings, or may be absent entirely, resulting in '[]'::jsonb.
Any code (external reporting, admin dashboards, etc.) that previously expected full TaxRate objects (e.g. rate.percentage, rate.display_name) from the materialized-view subscriptions JSON will now receive a structurally different payload or an empty array. If this data is surfaced via the user-reports API, it's worth verifying that consumers are tolerant of both shapes.
noliveleger
force-pushed
the
django-5.2-update-stripe
branch
from
eebd5a1 to
c329d25
Compare
# Conflicts: # dependencies/pip/dev_requirements.txt # dependencies/pip/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
static/openapi/schema_v2.json (1)
23970-23972:⚠️ Potential issue | 🟠 MajorRestore the
transform_quantityobject schema.
{"nullable": true}removes the field’s shape entirely. In OpenAPI 3.0,nullableonly applies when the same schema also declares atype, so tooling will either ignore it or treattransform_quantityas effectively untyped. Keep the object schema here and mark that object nullable instead.OpenAPI 3.0 Schema Object: does `nullable: true` have any effect when `type` is omitted, and what is the correct way to represent a nullable object property?Also applies to: 24034-24036
🧹 Nitpick comments (3)
.github/workflows/openapi.yml (1)
46-49: Python 3.12 upgrade looks good; stale comment on line 73-75.The upgrade to Python 3.12 aligns with the CI modernization effort. However, the comment on lines 73-75 now references an outdated Python version:
# temporary 20260227: Let the CI pass with this warning: # > FutureWarning: You are using a Python version (3.10.19) which Google will stop supportingConsider updating or removing this comment since the workflow now runs on Python 3.12, making the warning context no longer accurate.
,
kobo/apps/stripe/utils/subscription_limits.py (1)
315-327: Potential N+1 query issue with subscription items iteration.The loop at lines 316-319 iterates over subscriptions and then over their items. While
select_related('price__product')is used on the items queryset, thesubscriptionsqueryset itself doesn't prefetch items, potentially causing N+1 queries.♻️ Suggested fix to prefetch items
`@requires_stripe` def get_plan_name(org_user: OrganizationUser, **kwargs) -> str | None: Subscription = kwargs['subscription_model'] subscriptions = Subscription.objects.filter( customer__subscriber_id=org_user.organization.id, stripe_data__status__in=ACTIVE_STRIPE_STATUSES, + ).prefetch_related( + 'items__price__product' ) unique_products = set() for subscription in subscriptions: - for item in subscription.items.select_related('price__product').all(): + for item in subscription.items.all(): if item.price and item.price.product: unique_products.add(item.price.product)kobo/apps/stripe/utils/billing_dates.py (1)
224-240: Inconsistent JSON field extraction patterns—consider standardizing for code clarity.Lines 224-230 use
F('stripe_data__billing_cycle_anchor')directly invalues(), while line 234-240 usesKeyTextTransform+Castforstart_date. While both approaches work correctly with Django 5.2 (F() properly extracts integer types from JSON), standardizing toKeyTextTransform+Castfor all timestamp fields would improve consistency and make the data type handling more explicit, especially since this pattern is already used for theMax()aggregation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99624b74-4b7e-40b4-879f-1203603abaff
📒 Files selected for processing (18)
.github/workflows/darker.yml.github/workflows/openapi.ymldependencies/pip/dev_requirements.independencies/pip/dev_requirements.txtdependencies/pip/requirements.independencies/pip/requirements.txtjsapp/js/api/models/subscriptionItem.tskobo/apps/accounts/utils.pykobo/apps/organizations/models.pykobo/apps/stripe/serializers.pykobo/apps/stripe/tests/test_stripe_utils.pykobo/apps/stripe/tests/utils.pykobo/apps/stripe/utils/billing_dates.pykobo/apps/stripe/utils/subscription_limits.pykobo/apps/stripe/views.pykobo/apps/user_reports/tests/test_user_reports.pystatic/openapi/schema_v2.jsonstatic/openapi/schema_v2.yaml
✅ Files skipped from review due to trivial changes (2)
- dependencies/pip/dev_requirements.in
- .github/workflows/darker.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- jsapp/js/api/models/subscriptionItem.ts
- kobo/apps/organizations/models.py
- kobo/apps/user_reports/tests/test_user_reports.py
- kobo/apps/accounts/utils.py
- static/openapi/schema_v2.yaml
| "type": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/StripeProductType" | ||
| } | ||
| ], | ||
| "description": "The type of the product. The product is either of type `good`, which is eligible for use with Orders and SKUs, or `service`, which is eligible for use with Subscriptions and Plans.\n\n* `good` - Good\n* `service` - Service" | ||
| "type": "string", | ||
| "nullable": true |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Official Stripe API docs: what values can Product.type and Price.type return, and does dj-stripe 2.10 change those enums or only the backing storage to stripe_data?
💡 Result:
- Stripe
Product.typecan begoodorservice. Stripe describestypeas required, withgoodfor products used with Orders/SKUs andservicefor products used with subscriptions/plans. [1] - Stripe
Price.typecan beone_timeorrecurring. [2]
dj-stripe 2.10 and enums: dj-stripe 2.10.0 (2025-08-11) is primarily a storage/ORM-compatibility step: it removes many concrete DB columns and accesses Stripe fields via the stripe_data JSONField (and encourages filtering via stripe_data__...). It does not redefine Stripe’s enum values for fields like product/price “type”; it’s about where/how the data is stored/accessed. [3] Additionally, dj-stripe notes “All enums are now set to 255 max length” (a DB/schema compatibility change, not a change to the allowed enum members). [4]
Sources:
[1] Stripe changelog entry describing Product.type as good or service.
[2] Stripe API reference for the Price object: type is one_time or recurring.
[3] dj-stripe 2.10.0 change notes explaining shift to stripe_data + property accessors.
[4] dj-stripe release notes mentioning enum fields max length set to 255.
Restore type enum references instead of using nullable string.
Stripe's Product.type is strictly good or service; Price.type is strictly one_time or recurring. The dj-stripe 2.10 migration to stripe_data is a storage and ORM change only—it does not alter Stripe's enum contract. Replacing the enum refs with nullable string unnecessarily weakens the generated public contract and removes useful type information for clients. Keep StripeProductType and StripePriceType enum references and add nullability around the refs if required.
Also applies to: 23938-23940, 24003-24005, 24066-24068
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
static/openapi/schema_v2.yaml (1)
12870-12883:⚠️ Potential issue | 🟠 Major
descriptionis still modeled too strictly for Stripe productsAt Line 12870 and Line 17144,
descriptionistype: stringand required (Line 12882, Line 17160), but not nullable. If Stripe returnsnull, generated clients will reject valid responses.Suggested fix
BaseProduct: type: object properties: description: type: string + nullable: true readOnly: true @@ Product: type: object properties: description: type: string + nullable: true readOnly: trueIn Stripe's current API reference, is Product.description nullable and can API responses include "description": null?Also applies to: 17144-17161
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b15b89d-88ed-4d92-806a-c1bf6dcfbcce
📒 Files selected for processing (7)
kobo/apps/user_reports/migrations/0001_squashed_old_migrations.pykobo/apps/user_reports/migrations/0002_reset_mv_for_djstripe_update.pykobo/apps/user_reports/migrations/0007_fix_mfa_is_active_new_table.pykobo/apps/user_reports/tests/test_user_reports.pykobo/apps/user_reports/utils/migrations.pystatic/openapi/schema_v2.jsonstatic/openapi/schema_v2.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- static/openapi/schema_v2.json
| max_length=32, | ||
| ), | ||
| ), | ||
| ('last_processed_org_id', models.CharField(blank=True, null=True)), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Django 5.2, does models.CharField require an explicit max_length in migration field definitions? What error or system check should we expect if it is omitted?
💡 Result:
Yes, in Django 5.2, models.CharField requires an explicit max_length in model field definitions. Omitting it triggers a system check error during commands like makemigrations or manage.py check. The expected error is a SystemCheckError with the message: "CharFields must define a 'max_length' attribute." and error code fields.E120. For example: SystemCheckError: System check identified some issues: ERRORS: app.Model.field_name: (fields.E120) CharFields must define a 'max_length' attribute.
Citations:
- 1: https://django.readthedocs.io/en/5.2.x/ref/models/fields.html
- 2: https://docs.djangoproject.com/en/6.0/ref/models/fields/
- 3: https://docs.djangoproject.com/en/5.2/ref/models/fields
- 4: https://code.djangoproject.com/ticket/25332
- 5: https://code.djangoproject.com/ticket/25628
- 6: https://stackoverflow.com/questions/73985380/where-in-django-charfield-is-max-length-defined-as-compulsory
- 7: https://docs.djangoproject.com/en/4.1/ref/checks
🏁 Script executed:
# Check if the file exists and examine its contents around line 55
if [ -f "kobo/apps/user_reports/migrations/0001_squashed_old_migrations.py" ]; then
echo "=== File found. Examining lines 50-60 ==="
sed -n '50,60p' "kobo/apps/user_reports/migrations/0001_squashed_old_migrations.py" | cat -n
echo ""
echo "=== Checking for CharField definitions without max_length in this file ==="
grep -n "CharField" "kobo/apps/user_reports/migrations/0001_squashed_old_migrations.py" | head -20
else
echo "File not found at expected path"
fiRepository: kobotoolbox/kpi
Length of output: 1514
Add explicit max_length to the CharField on line 55.
last_processed_org_id is defined as models.CharField(blank=True, null=True) without a max_length parameter. Django requires this parameter and will raise a SystemCheckError (fields.E120) during migration application. Other CharField definitions in this file specify max_length, so add it here as well (e.g., max_length=255 or an appropriate length for an org ID).
| dependencies = [('user_reports', '0001_squashed_old_migrations')] | ||
|
|
||
| operations = [ | ||
| migrations.RunSQL(DROP_MV_SQL, reverse_sql=''), | ||
| migrations.RunPython(reset_lrm_0019, reverse_code=migrations.RunPython.noop), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that this migration touches long_running_migrations.LongRunningMigration
# while its dependency list only mentions user_reports.
rg -n -C2 "dependencies =|long_running_migrations|RunPython" \
kobo/apps/user_reports/migrations/0002_reset_mv_for_djstripe_update.pyRepository: kobotoolbox/kpi
Length of output: 556
🏁 Script executed:
# Find the long_running_migrations app and its migrations
find . -type d -name "long_running_migrations" | head -20Repository: kobotoolbox/kpi
Length of output: 94
🏁 Script executed:
# List migrations in long_running_migrations if it exists
fd -type f "migrations/" . | grep long_running_migrations | head -20Repository: kobotoolbox/kpi
Length of output: 229
🏁 Script executed:
# Search for where LongRunningMigration model is defined
rg "class LongRunningMigration" --type pyRepository: kobotoolbox/kpi
Length of output: 759
🏁 Script executed:
# Check other migrations that reference long_running_migrations to see how they handle dependencies
rg "long_running_migrations" kobo/apps/*/migrations/ --type py -lRepository: kobotoolbox/kpi
Length of output: 1990
🏁 Script executed:
# List migrations in long_running_migrations ordered by name
ls -1 kobo/apps/long_running_migrations/migrations/ | grep -E "^[0-9]+" | sort -n | head -5Repository: kobotoolbox/kpi
Length of output: 279
🏁 Script executed:
# Check the initial migration file that likely creates the model
cat kobo/apps/long_running_migrations/migrations/0001_initial.py 2>/dev/null | head -40Repository: kobotoolbox/kpi
Length of output: 1178
🏁 Script executed:
# Search for other migrations that reference long_running_migrations model to see dependency patterns
rg -A5 "long_running_migrations.*LongRunningMigration" kobo/apps/long_running_migrations/migrations/ --type py | head -40Repository: kobotoolbox/kpi
Length of output: 3917
🏁 Script executed:
# Check the dependencies of user_reports.0001_squashed_old_migrations
rg -A10 "^class Migration" kobo/apps/user_reports/migrations/0001_squashed_old_migrations.py | head -20Repository: kobotoolbox/kpi
Length of output: 548
🏁 Script executed:
# Check the full dependencies declaration in 0001_squashed_old_migrations
rg "dependencies = " -A 5 kobo/apps/user_reports/migrations/0001_squashed_old_migrations.pyRepository: kobotoolbox/kpi
Length of output: 243
Add the migration dependency for long_running_migrations.0001_initial.
reset_lrm_0019() calls apps.get_model('long_running_migrations', 'LongRunningMigration') but the dependencies at line 17 only reference user_reports.0001_squashed_old_migrations, which does not depend on the long_running_migrations app. On a fresh database install, Django may attempt to run this migration before the LongRunningMigration model is created, causing apps.get_model() to fail.
Add ('long_running_migrations', '0001_initial') to the dependencies list.
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 17-17: Mutable default value for class attribute
(RUF012)
There was a problem hiding this comment.
♻️ Duplicate comments (1)
kobo/apps/user_reports/migrations/0002_reset_mv_for_djstripe_update.py (1)
18-25:⚠️ Potential issue | 🔴 CriticalMissing migration dependency for
long_running_migrationsapp.
reset_lrm_0019()callsapps.get_model('long_running_migrations', 'LongRunningMigration'), but the dependencies list at line 29 only includesuser_reports.0001_squashed_old_migrations. On a fresh database, Django may run this migration before theLongRunningMigrationmodel exists, causingapps.get_model()to raiseLookupError.Add the dependency:
class Migration(migrations.Migration): - dependencies = [('user_reports', '0001_squashed_old_migrations')] + dependencies = [ + ('user_reports', '0001_squashed_old_migrations'), + ('long_running_migrations', '0001_initial'), + ]Also applies to: 28-29
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d96c2505-1cc2-4ef4-8bff-2c9685657817
📒 Files selected for processing (1)
kobo/apps/user_reports/migrations/0002_reset_mv_for_djstripe_update.py
noliveleger
force-pushed
the
django-5.2-update-stripe
branch
from
aecf2b0 to
64c36f0
Compare
| 'djstripe_customers__subscriptions__stripe_data__billing_cycle_anchor' | ||
| ), | ||
| start=F( | ||
| 'djstripe_customers__subscriptions__stripe_data__current_period_start' |
There was a problem hiding this comment.
I'm pretty sure these current_period... lookups are wrong. As far as I can tell, current_period_end and current_period_start are nested (very unhelpfully) under stripe_data["items"]["data"][*]. That is an issue in itself. But I'm more concerned that it doesn't seem to have been caught by any of our tests, because we use a lot of mocked data for Stripe-side objects in our tests. And that makes me worry about anything else we might miss.
There was a problem hiding this comment.
Interloper comment, but I think we're okay with these ORM lookups based on dj-stripe's own use of the stripe_data dictionary: https://github.com/dj-stripe/dj-stripe/blob/stable/2.10/djstripe/models/billing.py#L1250-L1258, e.g. self.stripe_data.get("current_period_end")
we use a lot of mocked data for Stripe-side objects in our tests
This certainly is a weakness. I wonder if there's any way for us to use work that dj-stripe is presumably already doing to have up-to-date fixtures for their own tests? Could we compare the shape of those fixtures to some expectation as part of our unit tests?
There was a problem hiding this comment.
@jnm Good catch. I did not realize they still have that property there, but now I'm even more confused because as far as I can see it doesn't work:
In [9]: sub.current_period_start
In [10]: "current_period_start" in sub.stripe_data
Out[10]: False
The subscription-level billing period data was removed in a Stripe API release last year. So presumably these properties could be there for some backwards compatibility? But when I boot up KPI locally and sync Stripe data, djstripe uses a more recent API version that doesn't actually have those subscription-level keys anymore.
There was a problem hiding this comment.
Oo 👀 I'll butt out as I don't have time to fire up everything locally right now with this branch. If you and @noliveleger want to have a discussion about Stripe testing overall and think I'd be helpful from an architectural standpoint, LMK on Zulip
💭 Notes
Many breaking changes in
2.10. Please read https://github.com/dj-stripe/dj-stripe/releases/tag/2.10.0dj-stripe 2.10 consolidated nearly all Stripe model fields into a single
stripe_dataJSONField. Direct DB column access is replaced by@propertyaccessors reading fromstripe_data, so attribute access on model instances still works, but ORM.filter()/.values()/F()calls must usestripe_data__<field>JSON lookups.ORM filter/annotation changes:
status,start_date,billing_cycle_anchor,current_period_start,current_period_end,ended_at,unit_amount,recurring,refunded,quantitylookups now go throughstripe_data__<field>Max('prices__unit_amount')replaced withMax(Cast(KeyTransform('unit_amount', 'prices__stripe_data'), output_field=IntegerField()))F('start_date')replaced withCast(F('stripe_data__start_date'), output_field=BigIntegerField())since timestamps are now Unix intscharge.payment_intent.status→charge.payment_intent.stripe_data.get('status')charge.refunded→charge.stripe_data.get('refunded')subscription.plan.productno longer works; replaced with iteration viasubscription.items.select_related('price__product').select_related('schedule')removed (field moved tostripe_data)Raw SQL (materialized view):
user_reports/utils/migrations.py: all column references rewritten tostripe_data->>'field'orstripe_data->'field'JSON operatorsdefault_tax_ratessubquery joiningdjstripe_djstripesubscriptiondefaulttaxrateM2M table replaced withsub.stripe_data->'default_tax_rates'(M2M table dropped in 2.10)0006emptied; new0007_djstripe_updaterecreates the materialized view with updated SQLMigration strategy:
stripe.0001_initialdependency changed from('djstripe', '0011_2_7')→('djstripe', '0001_initial')(exists in all djstripe versions)stripe.0005_djstripe_2_10depends on('djstripe', '0002_2_10')as a bridge migration — this avoidsInconsistentMigrationHistoryon upgrade since djstripe 2.10 reset migrations without Django'sreplacesmechanismTest fixtures:
baker.make()calls updated: fields likestatus,unit_amount,recurring,billing_cycle_anchor, etc. are now@propertywith no setter — values must be passed viastripe_data={...}with Unix timestampsreplace(microsecond=0)since Unix timestamps lose microsecond precision