🐛 Fixed "labels is not defined" error when creating comped members via API#26837
🐛 Fixed "labels is not defined" error when creating comped members via API#26837troyciesco merged 4 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe member-bread service's 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.41.1)ghost/core/test/e2e-api/admin/members.test.js[] Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
62d8213 to
d0dd703
Compare
cmraible
left a comment
There was a problem hiding this comment.
LGTM! I just pushed a single e2e-api test to cover this, mostly just to prevent incidental regressions in the future 🚢
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/admin/members.test.js (1)
1283-1354: LGTM! Well-structured test that directly verifies the bug fix.The test correctly exercises the code path where a comped member is created with labels, which previously would fail with "labels is not defined on the model" error. The Stripe mocking pattern follows existing tests, and the assertions verify both the
compedstatus and label presence.One optional enhancement for consistency with other comped tests in this file:
💡 Optional: Add member event assertions for more comprehensive coverage
assert.ok(member.labels.find(l => l.name === 'Complimentary'), 'Member should have Complimentary label'); + // Verify member status events (optional - for parity with other comped tests) + await assertMemberEvents({ + eventType: 'MemberStatusEvent', + memberId: member.id, + asserts: [{ + from_status: null, + to_status: 'free' + }, { + from_status: 'free', + to_status: 'comped' + }] + }); });,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-api/admin/members.test.js` around lines 1283 - 1354, Add an optional assertion to verify member status events by calling assertMemberEvents after the existing label assertions in the "Can create a comped member with labels via API" test; specifically, invoke assertMemberEvents with eventType 'MemberStatusEvent', the created member's id (member.id), and asserts array checking transitions from null→'free' and 'free'→'comped' so the test matches parity with other comped tests and ensures status-change events are recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/e2e-api/admin/members.test.js`:
- Around line 1283-1354: Add an optional assertion to verify member status
events by calling assertMemberEvents after the existing label assertions in the
"Can create a comped member with labels via API" test; specifically, invoke
assertMemberEvents with eventType 'MemberStatusEvent', the created member's id
(member.id), and asserts array checking transitions from null→'free' and
'free'→'comped' so the test matches parity with other comped tests and ensures
status-change events are recorded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1d07c38-02cd-4315-a434-18963e1ed4ce
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/members.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
ghost/core/test/e2e-api/admin/members.test.js
refs ONC-1553 Verifies the fix for passing sharedOptions to setComplimentarySubscription
2ebcec4 to
da978ed
Compare
closes https://linear.app/tryghost/issue/ONC-1553
add()method inmemberBREADServicepassed the fulloptionsobject (includingwithRelated: ['labels', 'newsletters']) tosetComplimentarySubscription().sharedOptions(context + transacting only), matching the pattern already used forlinkStripeCustomer().Summary
When creating a comped member via the Admin API with a
stripe_customer_idpointing to a Stripe customer that has existing subscriptions,setComplimentarySubscription()received the full options object includingwithRelated: ['labels', 'newsletters']. Inside that method,member.related('stripeSubscriptions').fetch(options)attempted to eager-loadlabelsonStripeCustomerSubscriptionmodels, which don't have that relation — throwing "labels is not defined on the model".Impact
The bug's impact depends on the Stripe customer's existing subscriptions:
comped: true: The comp conversion never runs. The member ends up withpaidstatus instead ofcomped, and the API returns a 500. This is a real data issue — the member doesn't get their complimentary access.linkStripeCustomer()before the error occurs, so the member is properly set up. However, the API still returns a 500 to the caller.StripeCustomerSubscriptionmodels to eager-load on.Fix
Uses
sharedOptions(onlycontext+transacting) instead of the fulloptionswhen callingsetComplimentarySubscription()on line 406 ofmember-bread-service.js. This matches the pattern already established for thelinkStripeCustomer()call on line 381.