Reinstate customer subscription webhook handling #22106
Conversation
|
Warning Rate limit exceeded@9larsons has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe changes address the handling of subscription attribution events across several modules. In the webhook tests, a function has been renamed to Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
allouis
deleted the
ONC-729-reinstate-customer-subscription-webhook-handling
branch
9larsons
restored the
ONC-729-reinstate-customer-subscription-webhook-handling
branch
|
In reviewing this, it looks like:
Unit tests are failing in multiple places, so I suspect that's what's left to handle. edit: this was much more obvious from the commit history than looking at the file changes 😓. Started in the wrong place. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/members/webhooks.test.js (1)
2163-2172: Consider adding more specific assertions for activity feed.While the activity feed test checks for subscription events and attributions, it could be more specific in its assertions.
Consider adding explicit checks for:
.matchBodySnapshot({ events: new Array(subscriptionAttributions.length).fill({ - data: anyObject + type: 'subscription_event', + data: { + type: anyString, + subscription: anyObject, + attribution: { + id: anyString, + url: anyString, + type: anyString, + title: anyString, + referrer_source: null, + referrer_medium: null, + referrer_url: null + } + } }) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members/__snapshots__/webhooks.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
ghost/core/test/e2e-api/members/webhooks.test.js(9 hunks)ghost/member-events/index.js(1 hunks)ghost/member-events/lib/SubscriptionAttributionEvent.js(1 hunks)ghost/members-api/lib/repositories/MemberRepository.js(2 hunks)ghost/members-events-service/lib/EventStorage.js(2 hunks)ghost/stripe/lib/services/webhook/CheckoutSessionEventService.js(2 hunks)ghost/stripe/lib/services/webhook/SubscriptionEventService.js(1 hunks)ghost/stripe/test/unit/lib/services/webhooks/CheckoutSessionEventService.test.js(2 hunks)ghost/stripe/test/unit/lib/services/webhooks/SubscriptionEventService.test.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 18.12.1, mysql8)
🔇 Additional comments (11)
ghost/member-events/lib/SubscriptionAttributionEvent.js (1)
1-7: Well-documented type definitions!The JSDoc type definitions are clear and properly documented, making the code more maintainable.
ghost/member-events/index.js (1)
12-12: LGTM!The new export follows the existing pattern and is properly aligned with other event exports.
ghost/stripe/lib/services/webhook/SubscriptionEventService.js (1)
21-33: Improved error handling for subscription linking!The changes improve error handling by:
- Directly attempting to link subscription if member exists.
- Properly handling database constraint violations.
- Converting specific database errors to a more meaningful
ConflictError.ghost/members-events-service/lib/EventStorage.js (1)
59-70: Well-structured event handling logic!The event subscription:
- Properly handles missing subscription events.
- Updates existing records with fallback to original values.
- Uses optional chaining for safe property access.
ghost/stripe/test/unit/lib/services/webhooks/SubscriptionEventService.test.js (1)
1-113: LGTM! Well-structured test suite with comprehensive error handling coverage.The test suite effectively covers various error scenarios including:
- Bad request handling for invalid subscription data
- Conflict error handling for duplicate entries
- Unexpected error handling
ghost/stripe/lib/services/webhook/CheckoutSessionEventService.js (1)
192-208: Good separation of concerns in subscription handling.The code now separates subscription linking from attribution updating, making the code more maintainable and following the Single Responsibility Principle.
ghost/stripe/test/unit/lib/services/webhooks/CheckoutSessionEventService.test.js (1)
640-648: Good test coverage for new attribution functionality.The test ensures that
updateSubscriptionAttributionis called correctly during subscription event handling.ghost/members-api/lib/repositories/MemberRepository.js (1)
888-899: Well-documented and clean implementation of subscription attribution.The new method follows the event-driven pattern consistently used in the codebase and includes proper JSDoc documentation.
ghost/core/test/e2e-api/members/webhooks.test.js (3)
1858-1858: LGTM! Function renamed for better clarity.The function name change from
testAttributionOnSignuptotestWithAttributionbetter reflects its purpose as it's used for testing various attribution scenarios, not just signup.
1903-1928: LGTM! Webhook payload construction is well-structured.The webhook payload construction properly includes attribution metadata when available, and the webhook signature generation is correctly implemented.
2008-2016: LGTM! Comprehensive test coverage for attribution scenarios.The test cases cover all important attribution scenarios:
- URL attribution
- Post attribution
- Deleted post attribution
- Page attribution
- Tag attribution
- Author attribution
- No attribution
- Empty attribution object
Each test case properly validates the attribution data structure and its properties.
Also applies to: 2031-2040, 2051-2060, 2074-2083, 2097-2106, 2120-2129, 2133-2142, 2147-2156
| await this.models.SubscriptionCreatedEvent.edit({ | ||
| attribution_id: attribution?.id ?? original.attribution_id, | ||
| attribution_url: attribution?.url ?? original.attribution_url, | ||
| attribution_type: attribution?.type ?? original.attribution_type, | ||
| referrer_source: attribution?.referrerSource ?? original.referrer_source, | ||
| referrer_medium: attribution?.referrerMedium ?? original.referrer_medium, | ||
| referrer_url: attribution?.referrerUrl ?? original.referrer_url | ||
| }, {id: subscriptionCreatedEvent.id}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The database edit operation lacks error handling. Consider wrapping it in a try-catch block to handle potential database errors gracefully.
Add error handling:
- await this.models.SubscriptionCreatedEvent.edit({
- attribution_id: attribution?.id ?? original.attribution_id,
- attribution_url: attribution?.url ?? original.attribution_url,
- attribution_type: attribution?.type ?? original.attribution_type,
- referrer_source: attribution?.referrerSource ?? original.referrer_source,
- referrer_medium: attribution?.referrerMedium ?? original.referrer_medium,
- referrer_url: attribution?.referrerUrl ?? original.referrer_url
- }, {id: subscriptionCreatedEvent.id});
+ try {
+ await this.models.SubscriptionCreatedEvent.edit({
+ attribution_id: attribution?.id ?? original.attribution_id,
+ attribution_url: attribution?.url ?? original.attribution_url,
+ attribution_type: attribution?.type ?? original.attribution_type,
+ referrer_source: attribution?.referrerSource ?? original.referrer_source,
+ referrer_medium: attribution?.referrerMedium ?? original.referrer_medium,
+ referrer_url: attribution?.referrerUrl ?? original.referrer_url
+ }, {id: subscriptionCreatedEvent.id});
+ } catch (err) {
+ // Log error for debugging
+ console.error('Failed to update subscription attribution:', err);
+ throw new Error('Failed to update subscription attribution');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.models.SubscriptionCreatedEvent.edit({ | |
| attribution_id: attribution?.id ?? original.attribution_id, | |
| attribution_url: attribution?.url ?? original.attribution_url, | |
| attribution_type: attribution?.type ?? original.attribution_type, | |
| referrer_source: attribution?.referrerSource ?? original.referrer_source, | |
| referrer_medium: attribution?.referrerMedium ?? original.referrer_medium, | |
| referrer_url: attribution?.referrerUrl ?? original.referrer_url | |
| }, {id: subscriptionCreatedEvent.id}); | |
| try { | |
| await this.models.SubscriptionCreatedEvent.edit({ | |
| attribution_id: attribution?.id ?? original.attribution_id, | |
| attribution_url: attribution?.url ?? original.attribution_url, | |
| attribution_type: attribution?.type ?? original.attribution_type, | |
| referrer_source: attribution?.referrerSource ?? original.referrer_source, | |
| referrer_medium: attribution?.referrerMedium ?? original.referrer_medium, | |
| referrer_url: attribution?.referrerUrl ?? original.referrer_url | |
| }, {id: subscriptionCreatedEvent.id}); | |
| } catch (err) { | |
| // Log error for debugging | |
| console.error('Failed to update subscription attribution:', err); | |
| throw new Error('Failed to update subscription attribution'); | |
| } |
…correct subscription id
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
ghost/members-events-service/lib/EventStorage.js (1)
60-84:⚠️ Potential issueAdd error handling for database operations.
The subscription attribution update lacks error handling. Add try-catch blocks to handle potential database errors gracefully.
Apply this diff to add error handling:
domainEvents.subscribe(SubscriptionAttributionEvent, async (event) => { - console.log('SubscriptionAttributionEvent', event.data); let attribution = event.data.attribution; - const subscriptionCreatedEvent = await this.models.SubscriptionCreatedEvent - .findOne({subscription_id: event.data.subscriptionId}, {require: false, withRelated: []}); + try { + const subscriptionCreatedEvent = await this.models.SubscriptionCreatedEvent + .findOne({subscription_id: event.data.subscriptionId}, {require: false, withRelated: []}); + if (!subscriptionCreatedEvent) { + return; + } + const original = subscriptionCreatedEvent.toJSON(); + await subscriptionCreatedEvent.save({ + attribution_id: attribution?.id ?? original.attribution_id, + attribution_url: attribution?.url ?? original.attribution_url, + attribution_type: attribution?.type ?? original.attribution_type, + referrer_source: attribution?.referrerSource ?? original.referrer_source, + referrer_medium: attribution?.referrerMedium ?? original.referrer_medium, + referrer_url: attribution?.referrerUrl ?? original.referrer_url + }, {patch: true}); + } catch (err) { + // Log error for debugging + logging.error('Failed to update subscription attribution:', err); + throw new Error('Failed to update subscription attribution'); + } - if (!subscriptionCreatedEvent) { - console.log('no subscriptionCreatedEvent'); - return; - } - const original = subscriptionCreatedEvent.toJSON(); - console.log('original', original); - console.log(`>>updating attribution for ${subscriptionCreatedEvent.id}`); - await subscriptionCreatedEvent.save({ - attribution_id: attribution?.id ?? original.attribution_id, - attribution_url: attribution?.url ?? original.attribution_url, - attribution_type: attribution?.type ?? original.attribution_type, - referrer_source: attribution?.referrerSource ?? original.referrer_source, - referrer_medium: attribution?.referrerMedium ?? original.referrer_medium, - referrer_url: attribution?.referrerUrl ?? original.referrer_url - }, {patch: true}); });🧰 Tools
🪛 ESLint
[error] 61-61: Unexpected console statement.
(no-console)
[error] 68-68: Unexpected console statement.
(no-console)
[error] 73-73: Unexpected console statement.
(no-console)
[error] 75-75: Unexpected console statement.
(no-console)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members/__snapshots__/webhooks.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
ghost/core/core/server/models/subscription-created-event.js(1 hunks)ghost/core/test/e2e-api/members/webhooks.test.js(11 hunks)ghost/members-events-service/lib/EventStorage.js(3 hunks)ghost/stripe/lib/services/webhook/CheckoutSessionEventService.js(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/models/subscription-created-event.js
🧰 Additional context used
🪛 ESLint
ghost/members-events-service/lib/EventStorage.js
[error] 43-43: Unexpected console statement.
(no-console)
[error] 61-61: Unexpected console statement.
(no-console)
[error] 68-68: Unexpected console statement.
(no-console)
[error] 73-73: Unexpected console statement.
(no-console)
[error] 75-75: Unexpected console statement.
(no-console)
ghost/core/test/e2e-api/members/webhooks.test.js
[error] 66-66: Unexpected console statement.
(no-console)
[error] 67-67: Unexpected console statement.
(no-console)
[error] 68-68: Unexpected console statement.
(no-console)
[error] 69-69: Unexpected console statement.
(no-console)
[error] 75-75: Missing space before function parentheses.
(space-before-function-paren)
[error] 95-95: Unexpected console statement.
(no-console)
[error] 179-179: Unexpected console statement.
(no-console)
[error] 180-180: Expected parentheses around arrow function argument having a body with curly braces.
(arrow-parens)
[error] 181-181: Unexpected console statement.
(no-console)
[error] 182-182: Unexpected console statement.
(no-console)
[error] 183-183: Unexpected console statement.
(no-console)
[error] 184-184: Unexpected console statement.
(no-console)
[error] 2200-2200: Unexpected exclusive mocha test.
(ghost/mocha/no-exclusive-tests)
[error] 2209-2209: Unexpected console statement.
(no-console)
[error] 2222-2222: Unexpected console statement.
(no-console)
ghost/stripe/lib/services/webhook/CheckoutSessionEventService.js
[error] 123-123: Unexpected console statement.
(no-console)
🪛 Gitleaks (8.21.2)
ghost/core/test/e2e-api/members/webhooks.test.js
172-172: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
ghost/core/test/e2e-api/members/webhooks.test.js
[error] 2200-2200: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🪛 GitHub Actions: CI
ghost/core/test/e2e-api/members/webhooks.test.js
[error] 66-69: Unexpected console statement
[error] 75-75: Missing space before function parentheses
[error] 95-95: Unexpected console statement
[error] 179-179: Unexpected console statement
[error] 180-180: Expected parentheses around arrow function argument having a body with curly braces
[error] 181-181: Unexpected console statement
[error] 182-182: Unexpected console statement
[error] 183-183: Unexpected console statement
[error] 184-184: Unexpected console statement
[error] 2200-2200: Unexpected exclusive mocha test
[error] 2209-2209: Unexpected console statement
[error] 2222-2222: Unexpected console statement
🔇 Additional comments (1)
ghost/members-events-service/lib/EventStorage.js (1)
1-1: LGTM!The import statement correctly includes the new
SubscriptionAttributionEventclass.
| @@ -40,6 +40,7 @@ class EventStorage { | |||
| }); | |||
|
|
|||
| domainEvents.subscribe(SubscriptionCreatedEvent, async (event) => { | |||
| console.log('SubscriptionCreatedEvent > subscriptionId', event.data.subscriptionId); | |||
There was a problem hiding this comment.
Remove debug console.log statements.
Debug logging statements should not be committed to production code. Consider using a proper logging system like the existing logging service.
Apply this diff to remove the debug statements:
- console.log('SubscriptionCreatedEvent > subscriptionId', event.data.subscriptionId);
...
- console.log('SubscriptionAttributionEvent', event.data);
...
- console.log('no subscriptionCreatedEvent');
...
- console.log('original', original);
...
- console.log(`>>updating attribution for ${subscriptionCreatedEvent.id}`);Also applies to: 61-61, 68-68, 73-73, 75-75
🧰 Tools
🪛 ESLint
[error] 43-43: Unexpected console statement.
(no-console)
| @@ -120,6 +120,7 @@ module.exports = class CheckoutSessionEventService { | |||
| } | |||
|
|
|||
| async handleSubscriptionEvent(session) { | |||
| console.log('handleSubscriptionEvent >> session.subscription', session.subscription); | |||
There was a problem hiding this comment.
Remove debug console.log statement.
Debug logging statements should not be committed to production code. Consider using a proper logging system.
Apply this diff to remove the debug statement:
- console.log('handleSubscriptionEvent >> session.subscription', session.subscription);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('handleSubscriptionEvent >> session.subscription', session.subscription); |
🧰 Tools
🪛 ESLint
[error] 123-123: Unexpected console statement.
(no-console)
| const subscriptionModel = await memberRepository.getSubscriptionByStripeID(session.subscription); | ||
| await memberRepository.updateSubscriptionAttribution(subscriptionModel.id, attribution); |
There was a problem hiding this comment.
Add error handling for subscription attribution update.
The subscription attribution update should be wrapped in a try-catch block to handle potential failures gracefully.
Apply this diff to add error handling:
- const subscriptionModel = await memberRepository.getSubscriptionByStripeID(session.subscription);
- await memberRepository.updateSubscriptionAttribution(subscriptionModel.id, attribution);
+ try {
+ const subscriptionModel = await memberRepository.getSubscriptionByStripeID(session.subscription);
+ await memberRepository.updateSubscriptionAttribution(subscriptionModel.id, attribution);
+ } catch (err) {
+ logging.error('Failed to update subscription attribution:', err);
+ // Don't throw as this is a non-critical operation
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const subscriptionModel = await memberRepository.getSubscriptionByStripeID(session.subscription); | |
| await memberRepository.updateSubscriptionAttribution(subscriptionModel.id, attribution); | |
| try { | |
| const subscriptionModel = await memberRepository.getSubscriptionByStripeID(session.subscription); | |
| await memberRepository.updateSubscriptionAttribution(subscriptionModel.id, attribution); | |
| } catch (err) { | |
| logging.error('Failed to update subscription attribution:', err); | |
| // Don't throw as this is a non-critical operation | |
| } |
| }; | ||
|
|
||
| await testAttributionOnSignup(attribution, attributionResource); | ||
| await testAttributionOnUpgrade(attribution, attributionResource); | ||
| console.log(`creating and linking subscription w/o attribution`); |
There was a problem hiding this comment.
Remove debug console.log statements from test.
Debug logging statements should be removed from tests unless they serve a specific testing purpose.
Apply this diff to remove the debug statements:
- console.log(`creating and linking subscription w/o attribution`);
...
- console.log(`updating attribution`);Also applies to: 2222-2222
🧰 Tools
🪛 ESLint
[error] 2209-2209: Unexpected console statement.
(no-console)
🪛 GitHub Actions: CI
[error] 2209-2209: Unexpected console statement
| }); | ||
| }); | ||
|
|
||
| it.only('Updates attribution when a member already exists', async function () { |
There was a problem hiding this comment.
Remove .only from test.
The focused test prevents other tests from running. This is typically used for debugging and should not be committed.
Apply this diff to remove the focus:
- it.only('Updates attribution when a member already exists', async function () {
+ it('Updates attribution when a member already exists', async function () {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.only('Updates attribution when a member already exists', async function () { | |
| it('Updates attribution when a member already exists', async function () { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 2200-2200: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🪛 ESLint
[error] 2200-2200: Unexpected exclusive mocha test.
(ghost/mocha/no-exclusive-tests)
🪛 GitHub Actions: CI
[error] 2200-2200: Unexpected exclusive mocha test
| nock.emitter.on('no match', (req) => { | ||
| console.log('\nNOCK REQUEST NOT MATCHED:'); | ||
| console.log('Method:', req.method); | ||
| console.log('Path:', req.path); | ||
| console.log('Headers:', JSON.stringify(req.headers, null, 2)); | ||
| }); |
There was a problem hiding this comment.
Replace console.log with proper test logging.
Debug logging statements should use proper test logging mechanisms.
Apply this diff to improve logging:
- console.log('\nNOCK REQUEST NOT MATCHED:');
- console.log('Method:', req.method);
- console.log('Path:', req.path);
- console.log('Headers:', JSON.stringify(req.headers, null, 2));
+ // Use a test logging utility or comment out if only needed for debugging
+ // testLogger.debug('NOCK REQUEST NOT MATCHED:', {
+ // method: req.method,
+ // path: req.path,
+ // headers: req.headers
+ // });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nock.emitter.on('no match', (req) => { | |
| console.log('\nNOCK REQUEST NOT MATCHED:'); | |
| console.log('Method:', req.method); | |
| console.log('Path:', req.path); | |
| console.log('Headers:', JSON.stringify(req.headers, null, 2)); | |
| }); | |
| nock.emitter.on('no match', (req) => { | |
| // Use a test logging utility or comment out if only needed for debugging | |
| // testLogger.debug('NOCK REQUEST NOT MATCHED:', { | |
| // method: req.method, | |
| // path: req.path, | |
| // headers: req.headers | |
| // }); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 66-66: Unexpected console statement.
(no-console)
[error] 67-67: Unexpected console statement.
(no-console)
[error] 68-68: Unexpected console statement.
(no-console)
[error] 69-69: Unexpected console statement.
(no-console)
🪛 GitHub Actions: CI
[error] 66-69: Unexpected console statement
Note: we previously had no test coverage with the `checkout.session.completed` handler in the case of an existing member. This had to modify a couple pieces to allow for that. Because we're editing the subscriptionCreatedEvent, the memberCreatedEvent info needs to be skipped as it may not match (which is expected).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@allouis Needs your review. Idk how to take over a PR short of opening a new one, which maybe is best here given the noisy commit history. |
|
Adding @cmraible for review. |
|
Closing this to open a new PR. I've pushed the revert commit separately to get it out with the next release. |
🍦