feat(accept-blue): implemented update subscription #557
Conversation
WalkthroughThis pull request introduces enhancements to the Vendure AcceptBlue plugin, focusing on subscription management and transaction processing. Key changes include the ability to update subscriptions through the Admin API, implementing permission-based controls for refunds, and restructuring API extensions to support distinct admin and shop contexts. Changes
Assessment against linked issues
Suggested reviewersPossibly related PRs
Finishing Touches
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 (
|
|
yeah, admin api only |
martijnvdbrug
requested review from
is0utfitters and
mschipperheyn
as code owners
![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
🧹 Nitpick comments (1)
packages/vendure-plugin-accept-blue/src/api/accept-blue-admin-resolver.ts (1)
24-25: Remove redundant null coalescingThe
?? undefinedis unnecessary as it's the default behavior of the null coalescing operator.- amount ?? undefined, - cvv2 ?? undefined + amount, + cvv2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/vendure-plugin-accept-blue/package.jsonis excluded by!**/*.json
📒 Files selected for processing (13)
.github/CODEOWNERS(1 hunks)packages/vendure-plugin-accept-blue/CHANGELOG.md(1 hunks)packages/vendure-plugin-accept-blue/src/accept-blue-plugin.ts(2 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-admin-resolver.ts(1 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-client.ts(3 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-common-resolvers.ts(1 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts(4 hunks)packages/vendure-plugin-accept-blue/src/api/api-extensions.ts(3 hunks)packages/vendure-plugin-accept-blue/src/events/accept-blue-subscription-event.ts(1 hunks)packages/vendure-plugin-accept-blue/src/index.ts(1 hunks)packages/vendure-plugin-accept-blue/src/types.ts(2 hunks)packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts(8 hunks)packages/vendure-plugin-accept-blue/test/helpers/graphql-helpers.ts(2 hunks)
🔇 Additional comments (15)
packages/vendure-plugin-accept-blue/src/events/accept-blue-subscription-event.ts (1)
10-19: LGTM! Clean event implementation.The event class is well-structured and properly typed.
packages/vendure-plugin-accept-blue/src/accept-blue-plugin.ts (1)
23-24: LGTM! Clean API separation.The split between admin and shop API extensions is well-implemented.
Also applies to: 27-28
packages/vendure-plugin-accept-blue/src/api/accept-blue-common-resolvers.ts (1)
1-1: LGTM! Clean removal of refund mutation.The refund functionality has been properly moved to the admin resolver.
packages/vendure-plugin-accept-blue/src/api/api-extensions.ts (3)
Line range hint
9-162: LGTM! Well-structured schema definitions.The input types and enums are well-documented and properly typed.
164-167: LGTM! Proper API separation.Good practice separating shop and admin APIs.
168-187: Verify permission checks for admin mutations.The mutations look good, but ensure they're properly protected with the UpdateOrder permission as mentioned in the PR objectives.
✅ Verification successful
Permission checks are properly implemented
Both admin mutations are correctly protected with
@Allow(Permission.UpdateOrder)as required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for permission checks in the resolver ast-grep --pattern 'class $_ { $$$ @$_("UpdateOrder") $$$ refundAcceptBlueTransaction($$$) { $$$ } $$$ }'Length of output: 123
Script:
#!/bin/bash # First find the resolver file fd resolver packages/vendure-plugin-accept-blue/src # Then look for the resolver implementation ast-grep --pattern 'class $_ { $$$ @$_($$$) $$$ refundAcceptBlueTransaction($$$) { $$$ } $$$ }' ast-grep --pattern 'class $_ { $$$ @$_($$$) $$$ updateAcceptBlueSubscription($$$) { $$$ } $$$ }'Length of output: 432
Script:
#!/bin/bash # Check the admin resolver content cat packages/vendure-plugin-accept-blue/src/api/accept-blue-admin-resolver.ts # Backup search for mutations and decorators rg -A 5 "refundAcceptBlueTransaction|updateAcceptBlueSubscription" packages/vendure-plugin-accept-blue/src/api/accept-blue-admin-resolver.tsLength of output: 2059
packages/vendure-plugin-accept-blue/src/api/accept-blue-client.ts (2)
136-154: LGTM! Proper amount and date handling.The implementation correctly handles:
- Amount conversion from cents
- Date formatting to yyyy-mm-dd
202-203: LGTM! Type safety improvement.Good update to use the more specific CreateInput type.
packages/vendure-plugin-accept-blue/src/types.ts (1)
Line range hint
265-291: LGTM! Well-defined types with proper documentation.Good separation between create and update types, with update type correctly making all fields optional.
packages/vendure-plugin-accept-blue/test/helpers/graphql-helpers.ts (2)
172-186: LGTM! Complete history query.The query includes all necessary fields for testing order history.
242-254: LGTM! Comprehensive subscription update mutation.The mutation includes all necessary fields to test subscription updates.
packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts (1)
67-68: LGTM!The constructor changes follow best practices by making dependencies explicit and using readonly where appropriate.
packages/vendure-plugin-accept-blue/CHANGELOG.md (1)
1-4: LGTM!The changelog entries accurately describe the implemented changes.
.github/CODEOWNERS (1)
5-5: LGTM!The CODEOWNERS entry is correctly formatted and properly adds the new owner.
packages/vendure-plugin-accept-blue/src/index.ts (1)
7-8: Verify file locations after restructuringThe event files have been moved to a new directory structure.
Let's verify the file locations:
✅ Verification successful
Files verified in new location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the event files exist in their new location fd --type f "accept-blue-(transaction|subscription)-event.ts" packages/vendure-plugin-accept-blue/src/eventsLength of output: 271
| const subcription = this.mapToGraphqlSubscription( | ||
| subscription, | ||
| orderLine.productVariant.id | ||
| ); |
There was a problem hiding this comment.
Fix typo in variable name
The variable name subcription is misspelled.
- const subcription = this.mapToGraphqlSubscription(
+ const subscription = this.mapToGraphqlSubscription(
subscription,
orderLine.productVariant.id
);📝 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 subcription = this.mapToGraphqlSubscription( | |
| subscription, | |
| orderLine.productVariant.id | |
| ); | |
| const subscription = this.mapToGraphqlSubscription( | |
| subscription, | |
| orderLine.productVariant.id | |
| ); |
packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts
Outdated
Show resolved
Hide resolved
packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vendure-plugin-accept-blue/README.md (1)
180-180: Fix typo in the textChange "de admin-api" to "the admin-api"
-You can update created subscriptions in Accept Blue as Admin via de admin-api with `UpdateOrder` permissions: +You can update created subscriptions in Accept Blue as Admin via the admin-api with `UpdateOrder` permissions:
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts (2)
328-331:⚠️ Potential issueFix typo in variable name
The variable name
subcriptionis misspelled.- const subcription = this.mapToGraphqlSubscription( + const subscription = this.mapToGraphqlSubscription( subscription, orderLine.productVariant.id );
339-343:⚠️ Potential issueFix propagated typo in event publishing
The misspelled variable is used in event publishing.
await this.eventBus.publish( - new AcceptBlueSubscriptionEvent(ctx, subcription, 'updated', input) + new AcceptBlueSubscriptionEvent(ctx, subscription, 'updated', input) ); - return subcription; + return subscription;
🧹 Nitpick comments (2)
packages/vendure-plugin-accept-blue/CHANGELOG.md (1)
8-8: Fix typo in changelogThere's a typo in "Piublicly".
-Piublicly expose ctx in AcceptBlueTransactionEvent +Publicly expose ctx in AcceptBlueTransactionEventpackages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts (1)
333-337: Consider adding more detailed update notesThe order note could be more specific about which fields were updated.
- note: `Subscription updated: ${JSON.stringify(input)}`, + note: `Subscription ${input.id} updated: ${Object.entries(input) + .filter(([key, value]) => value !== undefined && key !== 'id') + .map(([key, value]) => `${key}=${value}`) + .join(', ')}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vendure-plugin-accept-blue/CHANGELOG.md(1 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts(4 hunks)packages/vendure-plugin-accept-blue/src/types.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: packages/vendure-plugin-stripe-subscription/
- GitHub Check: packages/vendure-plugin-stock-monitoring/
- GitHub Check: packages/vendure-plugin-shipping-extensions/
- GitHub Check: packages/vendure-plugin-shipmate/
- GitHub Check: packages/vendure-plugin-sendcloud/
- GitHub Check: packages/vendure-plugin-public-customer-groups/
- GitHub Check: packages/vendure-plugin-primary-collection/
- GitHub Check: packages/vendure-plugin-popularity-scores/
- GitHub Check: packages/vendure-plugin-picqer/
- GitHub Check: packages/vendure-plugin-picklist/
- GitHub Check: packages/vendure-plugin-myparcel/
- GitHub Check: packages/vendure-plugin-multiserver-db-sessioncache/
- GitHub Check: packages/vendure-plugin-modify-customer-orders/
- GitHub Check: packages/vendure-plugin-coinbase/
- GitHub Check: packages/vendure-plugin-anonymized-order/
- GitHub Check: packages/vendure-plugin-admin-ui-helpers/
- GitHub Check: packages/vendure-plugin-admin-social-auth/
- GitHub Check: packages/util/
- GitHub Check: packages/test/
- GitHub Check: Prettier formatting
🔇 Additional comments (3)
packages/vendure-plugin-accept-blue/src/types.ts (2)
Line range hint
267-280: LGTM! Well-structured create interface.The interface correctly defines required and optional fields for creating recurring schedules.
281-293: LGTM! Well-structured update interface.The interface properly makes all fields optional to support partial updates of recurring schedules.
packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts (1)
319-327:⚠️ Potential issueFix boolean handling in updateRecurringSchedule
The
||operator will overridefalsevalues withundefined. Use nullish coalescing (??) instead.const subscription = await client.updateRecurringSchedule(scheduleId, { title: input.title || undefined, amount: input.amount || undefined, frequency: input.frequency ?? undefined, next_run_date: input.nextRunDate || undefined, num_left: input.numLeft || undefined, - active: input.active || undefined, + active: input.active ?? undefined, receipt_email: input.receiptEmail || undefined, });Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts (1)
625-627:⚠️ Potential issueAdd 'await' before expect
The assertion needs to be awaited for proper async error handling.
Apply this diff to fix the issue:
- expect(updateRequest).rejects.toThrowError( + await expect(updateRequest).rejects.toThrowError(
🧹 Nitpick comments (1)
packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts (1)
680-682: Add type safety to history entry filteringThe type assertion is unsafe and could lead to runtime errors.
Apply this diff to add type safety:
- const entry = order.history.items.find( - (entry: any) => entry.type === 'ORDER_NOTE' - ); + interface HistoryEntry { + type: string; + data: { + note: string; + }; + } + const entry = order.history.items.find( + (entry: HistoryEntry) => entry.type === 'ORDER_NOTE' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/vendure-plugin-accept-blue/CHANGELOG.md(1 hunks)packages/vendure-plugin-accept-blue/README.md(1 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts(4 hunks)packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/vendure-plugin-accept-blue/README.md
- packages/vendure-plugin-accept-blue/CHANGELOG.md
- packages/vendure-plugin-accept-blue/src/api/accept-blue-service.ts
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: packages/vendure-plugin-stripe-subscription/
- GitHub Check: packages/vendure-plugin-stock-monitoring/
- GitHub Check: packages/vendure-plugin-shipping-extensions/
- GitHub Check: packages/vendure-plugin-shipmate/
- GitHub Check: packages/vendure-plugin-sendcloud/
- GitHub Check: packages/vendure-plugin-public-customer-groups/
- GitHub Check: packages/vendure-plugin-primary-collection/
- GitHub Check: packages/vendure-plugin-popularity-scores/
- GitHub Check: packages/vendure-plugin-picqer/
- GitHub Check: packages/vendure-plugin-picklist/
- GitHub Check: packages/vendure-plugin-myparcel/
- GitHub Check: packages/vendure-plugin-multiserver-db-sessioncache/
- GitHub Check: packages/vendure-plugin-modify-customer-orders/
- GitHub Check: packages/vendure-plugin-coinbase/
- GitHub Check: packages/vendure-plugin-anonymized-order/
- GitHub Check: packages/vendure-plugin-admin-ui-helpers/
- GitHub Check: packages/vendure-plugin-admin-social-auth/
- GitHub Check: packages/util/
- GitHub Check: packages/test/
- GitHub Check: Prettier formatting
| const tenDaysFromNow = new Date(); | ||
| tenDaysFromNow.setDate(tenDaysFromNow.getDate() + 10); | ||
| await adminClient.query<any, MutationUpdateAcceptBlueSubscriptionArgs>( |
There was a problem hiding this comment.
Fix potential timezone issues with date handling
The date handling has two potential issues:
- Date creation doesn't consider timezone
- Date formatting using substring is fragile
Apply this diff to fix the issues:
- const tenDaysFromNow = new Date();
- tenDaysFromNow.setDate(tenDaysFromNow.getDate() + 10);
+ const tenDaysFromNow = new Date(Date.now() + 10 * 24 * 60 * 60 * 1000);- next_run_date: tenDaysFromNow.toISOString().substring(0, 10), // Take yyyy-mm-dd
+ next_run_date: tenDaysFromNow.toISOString().split('T')[0],Also applies to: 669-669
Description
UpdateOrderCloses #544 and #440
Breaking changes
refundAcceptBlueTransactiuonis now only available to users with permissionUpdateOrderChecklist
📌 Always:
👍 Most of the time:
📦 For publishable packages:
package.jsonCHANGELOG.md