Added: Service and job data for returns batch processing#245
Added: Service and job data for returns batch processing#245parnikarangnekar wants to merge 1 commit intomainfrom
Conversation
Summary of ChangesHello @parnikarangnekar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new batch processing mechanism for handling Shopify returns and cancellations. It establishes a structured workflow by introducing a dedicated service job to queue relevant order IDs, defining specific system message types for feed generation and consumption, and creating a service to process these feeds and trigger the creation of Shopify refunds. This enhancement aims to streamline and automate the return and cancellation process. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds new services and job data for processing returns and cancellations in batches. The changes are generally well-structured, but I've identified a critical bug in the new service logic that would prevent it from working as intended. Additionally, there are opportunities to improve naming consistency and clarity in descriptions and warning messages. My review includes suggestions to address these points.
| <service-call name="co.hotwax.shopify.return.ShopifyReturnServices.create#ShopifyRefunds" in-map="[systemMessageRemoteId: systemMessage.systemMessageRemoteId,shopifyOrderId: orderId]" | ||
| ignore-error="true" transaction="force-new"/> |
There was a problem hiding this comment.
There's a bug in the in-map of the service call. The orderId variable is not defined within the iterate loop. You should use order.id to access the ID from the iterated order object. Without this change, the service call will fail to process any orders.
| <service-call name="co.hotwax.shopify.return.ShopifyReturnServices.create#ShopifyRefunds" in-map="[systemMessageRemoteId: systemMessage.systemMessageRemoteId,shopifyOrderId: orderId]" | |
| ignore-error="true" transaction="force-new"/> | |
| <service-call name="co.hotwax.shopify.return.ShopifyReturnServices.create#ShopifyRefunds" in-map="[systemMessageRemoteId: systemMessage.systemMessageRemoteId,shopifyOrderId: order.id]" | |
| ignore-error="true" transaction="force-new"/> |
data/ShopifyServiceJobData.xml
Outdated
| <moqui.service.job.ServiceJob jobName="queue_ReturnAndCancellationOrderIdsFeed" description="Queue returned orderIds feed" | ||
| serviceName="co.hotwax.shopify.system.ShopifySystemMessageServices.queue#FeedSystemMessage" cronExpression="0 0 * * * ?" paused="Y"> |
There was a problem hiding this comment.
The description 'Queue returned orderIds feed' is generic and appears to be copied from another job. To improve clarity and make this job's purpose distinct, please update the description to be more specific, such as 'Queue Order Ids Feed for Returns and Cancellations'.
| <moqui.service.job.ServiceJob jobName="queue_ReturnAndCancellationOrderIdsFeed" description="Queue returned orderIds feed" | |
| serviceName="co.hotwax.shopify.system.ShopifySystemMessageServices.queue#FeedSystemMessage" cronExpression="0 0 * * * ?" paused="Y"> | |
| <moqui.service.job.ServiceJob jobName="queue_ReturnAndCancellationOrderIdsFeed" description="Queue Order Ids Feed for Returns and Cancellations" | |
| serviceName="co.hotwax.shopify.system.ShopifySystemMessageServices.queue#FeedSystemMessage" cronExpression="0 0 * * * ?" paused="Y"> |
data/ShopifySetupSeedData.xml
Outdated
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateOrderIdsFeedForReturnAndCancellation" | ||
| description="Generate Order Ids Feed For Return and Cancel Order" | ||
| sendPath="component://shopify-connector/template/graphQL/ReturnedOrderIdsQuery.ftl" | ||
| sendServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnedOrderIdsFeed" | ||
| receivePath="${contentRoot}/shopify/OrderIdsFeedForReturnAndCancelOrder/ReturnedOrderIdsFeed-${dateTime}-${systemMessageId}.json"/> | ||
| <moqui.basic.Enumeration description="Generate Returned Order Ids Feed" enumId="GenerateOrderIdsFeedForReturnAndCancellation" enumTypeId="ShopifyMessageTypeEnum" relatedEnumId="GenerateReturnAndCancelFeed" relatedEnumTypeId="ShopifyMessageTypeEnum"/> | ||
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateReturnAndCancelFeed" | ||
| description="Generate Return and Cancel Feed" | ||
| parentTypeId="LocalFeedFile" | ||
| consumeServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnAndCancelFeed"/> |
There was a problem hiding this comment.
There are several naming inconsistencies across the newly added entities related to returns and cancellations. For better maintainability and clarity, I suggest standardizing on a single convention, for example, using ReturnAndCancellation.
Here are the suggested changes:
- SystemMessageType
GenerateOrderIdsFeedForReturnAndCancellation:description: 'Generate Order Ids Feed For Return and Cancel Order' -> 'Generate Order Ids Feed for Returns and Cancellations'receivePath: directoryOrderIdsFeedForReturnAndCancelOrder->OrderIdsFeedForReturnAndCancellation
- Enumeration
GenerateOrderIdsFeedForReturnAndCancellation:description: 'Generate Returned Order Ids Feed' -> 'Generate Order Ids Feed for Returns and Cancellations'relatedEnumId: 'GenerateReturnAndCancelFeed' -> 'GenerateReturnAndCancellationFeed'
- SystemMessageType
GenerateReturnAndCancelFeed:systemMessageTypeId: 'GenerateReturnAndCancelFeed' -> 'GenerateReturnAndCancellationFeed'description: 'Generate Return and Cancel Feed' -> 'Generate Return and Cancellation Feed'consumeServiceName: '...generate#ReturnAndCancelFeed' -> '...generate#ReturnAndCancellationFeed'
Applying these changes will make the configuration more consistent and easier to understand. This also affects service/co/hotwax/shopify/system/ShopifySystemMessageServices.xml and upgrade/UpcomingRelease/UpgradeData.xml.
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateOrderIdsFeedForReturnAndCancellation" | |
| description="Generate Order Ids Feed For Return and Cancel Order" | |
| sendPath="component://shopify-connector/template/graphQL/ReturnedOrderIdsQuery.ftl" | |
| sendServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnedOrderIdsFeed" | |
| receivePath="${contentRoot}/shopify/OrderIdsFeedForReturnAndCancelOrder/ReturnedOrderIdsFeed-${dateTime}-${systemMessageId}.json"/> | |
| <moqui.basic.Enumeration description="Generate Returned Order Ids Feed" enumId="GenerateOrderIdsFeedForReturnAndCancellation" enumTypeId="ShopifyMessageTypeEnum" relatedEnumId="GenerateReturnAndCancelFeed" relatedEnumTypeId="ShopifyMessageTypeEnum"/> | |
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateReturnAndCancelFeed" | |
| description="Generate Return and Cancel Feed" | |
| parentTypeId="LocalFeedFile" | |
| consumeServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnAndCancelFeed"/> | |
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateOrderIdsFeedForReturnAndCancellation" | |
| description="Generate Order Ids Feed for Returns and Cancellations" | |
| sendPath="component://shopify-connector/template/graphQL/ReturnedOrderIdsQuery.ftl" | |
| sendServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnedOrderIdsFeed" | |
| receivePath="${contentRoot}/shopify/OrderIdsFeedForReturnAndCancellation/ReturnedOrderIdsFeed-${dateTime}-${systemMessageId}.json"/> | |
| <moqui.basic.Enumeration description="Generate Order Ids Feed for Returns and Cancellations" enumId="GenerateOrderIdsFeedForReturnAndCancellation" enumTypeId="ShopifyMessageTypeEnum" relatedEnumId="GenerateReturnAndCancellationFeed" relatedEnumTypeId="ShopifyMessageTypeEnum"/> | |
| <moqui.service.message.SystemMessageType systemMessageTypeId="GenerateReturnAndCancellationFeed" | |
| description="Generate Return and Cancellation Feed" | |
| parentTypeId="LocalFeedFile" | |
| consumeServiceName="co.hotwax.shopify.system.ShopifySystemMessageServices.generate#ReturnAndCancellationFeed"/> |
| </if> | ||
| </actions> | ||
| </service> | ||
| <service verb="generate" noun="ReturnAndCancelFeed" authenticate="anonymous-all" transaction-timeout="1800"> |
There was a problem hiding this comment.
For consistency with other related entities, please rename the service noun from ReturnAndCancelFeed to ReturnAndCancellationFeed. This aligns with the naming convention suggested in the review of data/ShopifySetupSeedData.xml.
| <service verb="generate" noun="ReturnAndCancelFeed" authenticate="anonymous-all" transaction-timeout="1800"> | |
| <service verb="generate" noun="ReturnAndCancellationFeed" authenticate="anonymous-all" transaction-timeout="1800"> |
| <set field="orders" from="org.moqui.impl.context.ContextJavaUtil.jacksonMapper.readValue(fileText, List.class)"/> | ||
|
|
||
| <if condition="!orders"> | ||
| <return type="warning" message="System message [${systemMessageId}] for Type [${systemMessage?.systemMessageTypeId}] has messageText [${systemMessage.messageText}], with ReturnedOrderIdsFeed file having incorrect data and may contain null, not generating order returns feed file."/> |
There was a problem hiding this comment.
The warning message here is a bit misleading. This service consumes a feed to create refunds, but the message says 'not generating order returns feed file'. Please update the message to accurately reflect the service's action, for example, 'not processing refunds'.
| <return type="warning" message="System message [${systemMessageId}] for Type [${systemMessage?.systemMessageTypeId}] has messageText [${systemMessage.messageText}], with ReturnedOrderIdsFeed file having incorrect data and may contain null, not generating order returns feed file."/> | |
| <return type="warning" message="System message [${systemMessageId}] for Type [${systemMessage?.systemMessageTypeId}] has messageText [${systemMessage.messageText}], with ReturnedOrderIdsFeed file having incorrect data and may contain null, not processing refunds."/> |
ad2d9b3 to
300452a
Compare
Closes:#228