refactor: address PR review feedback for new-shopify-order-to-slack integration - #79
Open
sdmdg wants to merge 1 commit into
Open
refactor: address PR review feedback for new-shopify-order-to-slack integration#79sdmdg wants to merge 1 commit into
sdmdg wants to merge 1 commit into
Conversation
Contributor
WalkthroughTwo Ballerina functions files updated to improve Shopify order-to-Slack integration: functions marked Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ballerina-integrator/new-shopify-order-to-slack/functions.bal`:
- Line 96: The buildSlackMessage function currently only replaces known tokens
(e.g., order id, totalPrice) and leaves unknown {…} tokens (from the
configurable template/customMessage) unvalidated; update buildSlackMessage to
perform a post-replacement scan of the resulting string for any remaining
`{...}` tokens and return an error if any are found (include the offending
token(s) in the error message) instead of returning the broken string—this
ensures typos like `{totlPrice}` fail fast; locate the token-replacement logic
in buildSlackMessage and add the validation step immediately after replacements,
returning string|error accordingly.
- Around line 61-67: The returned items block currently appends a trailing
newline to the joined itemLines expression; remove the "+ (itemLines.length() >
0 ? "\n" : "")" suffix in the return so itemsDetails is just string:'join("\n",
...itemLines) (this prevents the extra blank line before Subtotal while
preserving the joined lines built from itemLines and quantities/product names).
In `@ballerina-integrator/new-shopify-order-to-slack/main.bal`:
- Around line 24-25: The current clientMsgId uses uuid:createType1AsString()
when orderDetails.hasRealOrderId is false, which produces a new value on
retries; instead compute a stable fallback by hashing stable order fields (e.g.,
orderDetails.orderNumber, orderDetails.createdAt, orderDetails.customerEmail or
orderDetails.shopifyId) so the same event yields the same client_msg_id on
retry. Replace the uuid call in the clientMsgId assignment with a deterministic
hash of those stable fields (use the existing crypto/hash utility in the project
or add one) so clientMsgId remains stable across retries while preserving the
branch that uses orderDetails.orderNumber when hasRealOrderId is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 683d668d-262a-4e1d-89d4-33cd6d2f5f87
📒 Files selected for processing (2)
ballerina-integrator/new-shopify-order-to-slack/functions.balballerina-integrator/new-shopify-order-to-slack/main.bal
Comment on lines
+61
to
+67
| string[] itemLines = from shopify:LineItem item in lineItems | ||
| let int? quantity = item?.quantity | ||
| let string quantityStr = quantity is int ? quantity.toString() : "1" | ||
| let string productName = item?.name ?: "Unknown Product" | ||
| select " • " + quantityStr + "x " + productName; | ||
|
|
||
| return itemsDetails; | ||
| return string:'join("\n", ...itemLines) + (itemLines.length() > 0 ? "\n" : ""); |
Contributor
There was a problem hiding this comment.
Drop the extra newline from itemsDetails.
Line 67 appends "\n", and the default template in ballerina-integrator/new-shopify-order-to-slack/config.bal:8-9 already adds a <br> immediately after {items}. That leaves a blank line before Subtotal for every non-empty order.
♻️ Suggested fix
- return string:'join("\n", ...itemLines) + (itemLines.length() > 0 ? "\n" : "");
+ return string:'join("\n", ...itemLines);📝 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.
Suggested change
| string[] itemLines = from shopify:LineItem item in lineItems | |
| let int? quantity = item?.quantity | |
| let string quantityStr = quantity is int ? quantity.toString() : "1" | |
| let string productName = item?.name ?: "Unknown Product" | |
| select " • " + quantityStr + "x " + productName; | |
| return itemsDetails; | |
| return string:'join("\n", ...itemLines) + (itemLines.length() > 0 ? "\n" : ""); | |
| string[] itemLines = from shopify:LineItem item in lineItems | |
| let int? quantity = item?.quantity | |
| let string quantityStr = quantity is int ? quantity.toString() : "1" | |
| let string productName = item?.name ?: "Unknown Product" | |
| select " • " + quantityStr + "x " + productName; | |
| return string:'join("\n", ...itemLines); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/new-shopify-order-to-slack/functions.bal` around lines
61 - 67, The returned items block currently appends a trailing newline to the
joined itemLines expression; remove the "+ (itemLines.length() > 0 ? "\n" : "")"
suffix in the return so itemsDetails is just string:'join("\n", ...itemLines)
(this prevents the extra blank line before Subtotal while preserving the joined
lines built from itemLines and quantities/product names).
|
|
||
| // Builds the Slack message by replacing placeholders with actual values | ||
| function buildSlackMessage(OrderDetails details, string template) returns string { | ||
| isolated function buildSlackMessage(OrderDetails details, string template) returns string|error { |
Contributor
There was a problem hiding this comment.
Fail fast on unknown template tokens.
Lines 121-124 only replace known keys. Because customMessage is configurable, a typo like {totlPrice} will survive unchanged and get posted as broken notification text. Since this helper already returns string|error, validate after the replacement pass that no {...} tokens remain and surface that as an error or warning.
Also applies to: 121-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/new-shopify-order-to-slack/functions.bal` at line 96,
The buildSlackMessage function currently only replaces known tokens (e.g., order
id, totalPrice) and leaves unknown {…} tokens (from the configurable
template/customMessage) unvalidated; update buildSlackMessage to perform a
post-replacement scan of the resulting string for any remaining `{...}` tokens
and return an error if any are found (include the offending token(s) in the
error message) instead of returning the broken string—this ensures typos like
`{totlPrice}` fail fast; locate the token-replacement logic in buildSlackMessage
and add the validation step immediately after replacements, returning
string|error accordingly.
Comment on lines
+24
to
+25
| // Determine client_msg_id for deduplication | ||
| string clientMsgId = orderDetails.hasRealOrderId ? orderDetails.orderNumber : uuid:createType1AsString(); |
Contributor
There was a problem hiding this comment.
Use a stable fallback for the dedupe key.
Line 25 generates a fresh value whenever hasRealOrderId is false. If the same event is retried in that state, it will get a different client_msg_id each time and can be posted more than once. Derive the fallback from stable order fields instead of minting a new value here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ballerina-integrator/new-shopify-order-to-slack/main.bal` around lines 24 -
25, The current clientMsgId uses uuid:createType1AsString() when
orderDetails.hasRealOrderId is false, which produces a new value on retries;
instead compute a stable fallback by hashing stable order fields (e.g.,
orderDetails.orderNumber, orderDetails.createdAt, orderDetails.customerEmail or
orderDetails.shopifyId) so the same event yields the same client_msg_id on
retry. Replace the uuid call in the clientMsgId assignment with a deterministic
hash of those stable fields (use the existing crypto/hash utility in the project
or add one) so clientMsgId remains stable across retries while preserving the
branch that uses orderDetails.orderNumber when hasRealOrderId is true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Addressing code review feedback and refactoring the Shopify-to-Slack integration for better performance, maintainability and traceability.
Resolves PR suggestions from #62.
Goals
isolatedfunctions).Approach
onOrdersCreate) for better log filtering.escapeHtmltoescapeSlackTextto better reflect its domain-specific purpose.re:replaceAll(Regex) tostring:replaceAllto reduce overhead.foreachloops with Ballerina Query Expressions for list building.isolatedas they do not access mutable module-level state, ensuring thread safety.is shopify:LineItem[]) where the logic already handled the type or nullability.Security checks
Related PRs
This PR addresses the feedback provided in the previous PR for the
new-shopify-order-to-slack#62.Test environment
Tested on:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements