Fix(accept-blue): Numerous differen bug fixes, see description #538
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Warning Rate limit exceeded@martijnvdbrug has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 50 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. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/vendure-plugin-accept-blue/src/accept-blue-plugin.ts (1)
53-54: Breaking change: Database migration requiredThe changes correctly implement the nullable and readonly flags for
acceptBlueCustomerIdas requested in issues #531 and #519. However, making the field nullable requires a database migration.Please ensure you:
- Create a migration script
- Document the breaking change in the changelog
- Update the version number according to semver
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/vendure-plugin-accept-blue/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
packages/vendure-plugin-accept-blue/src/accept-blue-plugin.ts(3 hunks)packages/vendure-plugin-accept-blue/src/api/accept-blue-handler.ts(2 hunks)packages/vendure-plugin-accept-blue/src/index.ts(1 hunks)packages/vendure-plugin-accept-blue/src/util.spec.ts(1 hunks)packages/vendure-plugin-accept-blue/src/util.ts(2 hunks)packages/vendure-plugin-accept-blue/vitest.config.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/vendure-plugin-accept-blue/src/index.ts (1)
Learnt from: martijnvdbrug
PR: Pinelab-studio/pinelab-vendure-plugins#458
File: packages/vendure-plugin-accept-blue/src/index.ts:2-2
Timestamp: 2024-11-12T08:32:12.721Z
Learning: Exports in index files are always allowed because these are publishable packages.
🔇 Additional comments (8)
packages/vendure-plugin-accept-blue/vitest.config.js (1)
5-5: Verify test discovery after removing include pattern
The empty test configuration will now discover tests throughout the project. Let's verify all tests are still being picked up.
✅ Verification successful
Empty test config is fine, all test files are discoverable
The empty test configuration test: {} works correctly as it allows Vitest to discover all test files in the project. The search results confirm that both test files for the accept-blue plugin are discoverable:
./packages/vendure-plugin-accept-blue/src/util.spec.ts./packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test file locations and ensure they're discoverable
# Find all test files
echo "Test files in the project:"
fd -e ts -e js --full-path . -x echo {} | grep -E "spec\.(ts|js)$"
# Check if vitest can discover these files
vitest --list
Length of output: 2703
packages/vendure-plugin-accept-blue/src/index.ts (1)
10-10: LGTM! Export aligns with PR objectives.
The new export for custom field types matches the requirements for making customer.customerId nullable and readonly.
packages/vendure-plugin-accept-blue/src/accept-blue-plugin.ts (3)
1-1: LGTM!
The Type import is correctly added to support the init method's return type.
64-64: LGTM!
The return type change to Type<AcceptBluePlugin> is more accurate and type-safe.
48-49: Verify the necessity of OrderLine field changes
The changes to make acceptBlueSubscriptionIds nullable and readonly weren't mentioned in the PR objectives. Please confirm if these changes are intentional and necessary.
✅ Verification successful
Field changes are intentional and necessary
The changes to make acceptBlueSubscriptionIds nullable and readonly are justified based on the codebase analysis:
- The field is only written once in
accept-blue-service.tswhen setting subscription IDs - It's primarily used for reading subscription data as shown in multiple query operations
- The CHANGELOG indicates this field was recently refactored from
subscriptionIdsspecifically to avoid conflicts - The field can be null as evidenced by the null coalescing operator in the service:
orderLine.customFields.acceptBlueSubscriptionIds ?? []
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these fields are used elsewhere in the codebase
rg "acceptBlueSubscriptionIds" -B 2 -A 2
Length of output: 6588
packages/vendure-plugin-accept-blue/src/util.ts (3)
126-128: LGTM! This fixes the subscription counting issue.
The change from Math.floor to Math.ceil correctly addresses issue #532 by preventing subscriptions from being treated as ongoing when they should end.
134-135: LGTM! Daily frequency support added.
Simple and correct implementation for daily billing cycles.
151-151: LGTM! Added TypeScript safety comment.
The eslint comment helps prevent runtime errors while maintaining type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/vendure-plugin-accept-blue/test/accept-blue.spec.ts (1)
193-202: Consider adding test cleanupThe test correctly sets up the mock customerId, but it should clean up the test data after completion to prevent potential interference with other tests.
Add this cleanup step after the test:
it('Gets saved payment methods for logged in customer', async () => { // ... existing test code ... + // Clean up after test + afterEach(async () => { + await server.app + .get(DataSource) + .getRepository(Customer) + .update( + { id: 1 }, + { + customFields: { acceptBlueCustomerId: null }, + } + ); + }); });
Description
Closes Accept Blue can accidently end up in an unwanted auto renew scenario #532
Closes acceptBlueCustomerId should be nullable #531
Closes acceptBlueCustomerId should be a readonly hidden customField #519
Declinedwhen handling payment fails, instead of throwing.Closes Accept Blue payment handler should return Declined instead of throwing #493
Breaking changes
Checklist
📌 Always:
👍 Most of the time:
📦 For publishable packages:
package.jsonCHANGELOG.md