-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Shopify Developer App updates #18978
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdded input validation for the order ID in the get-order action via a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/shopify_developer_app/actions/get-order/get-order.mjs (1)
1-48: Incomplete fix: ApplyvalidateOrderId()method torefund-order.mjsas well.The 'Search for orders' action does not use orderId, so it's unaffected. However,
refund-order.mjspassesorderIdtoshopify.getOrder()at line 46 without validation, exactly like the originalget-order.mjsdid. Add the samevalidateOrderId()method torefund-order.mjsto handle both raw numeric IDs and GraphQL-formatted IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/shopify_developer_app/actions/get-order/get-order.mjs(2 hunks)components/shopify_developer_app/package.json(1 hunks)
🔇 Additional comments (3)
components/shopify_developer_app/package.json (1)
3-3: LGTM: Version bump is appropriate.The patch version increment correctly reflects a bug fix according to semantic versioning.
components/shopify_developer_app/actions/get-order/get-order.mjs (2)
3-3: LGTM: Appropriate imports, version bump, and annotation.The ConfigurationError import is correctly used for input validation, the patch version increment aligns with the bug fix, and the
readOnlyHint: trueannotation properly reflects that this is a read-only GET operation.Also applies to: 9-9, 13-13
38-47: Fix correctly validates and normalizes order IDs before API calls.The implementation confirms the Shopify API requires order IDs in
gid://shopify/Order/format. ThevalidateOrderIdmethod (lines 26-36) now handles both formats:
- Accepts pre-formatted
gid://shopify/Order/IDs- Automatically converts numeric IDs to the required format
- Immediately rejects invalid formats with a
ConfigurationErrorBy validating before the API call, improperly formatted IDs never reach the Shopify GraphQL endpoint, preventing the JSON parsing errors from malformed responses.
luancazarine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michelle0927, LGTM! Ready for QA!
|
Hi everyone, all test cases are passed! Ready for release! Test reports
|
Resolves #18971
Summary by CodeRabbit
New Features
Chores