Skip to content

Comments

523 enhancement returns and refunds flow merge#164

Open
Prerakghatode wants to merge 7 commits intohotwax:mainfrom
Prerakghatode:523-enhancement-returns-and-refunds-flow-merge
Open

523 enhancement returns and refunds flow merge#164
Prerakghatode wants to merge 7 commits intohotwax:mainfrom
Prerakghatode:523-enhancement-returns-and-refunds-flow-merge

Conversation

@Prerakghatode
Copy link
Contributor

Added Refunds handling flow in generate ReturnsAndExchangeFeed based on orderId.

</#if>
<#if !filterQuery?has_content>
<#assign filterQuery = "return_status:'RETURNED'"/>
<#assign filterQuery = "return_status:'RETURNED' OR (return_status:'NO_RETURN' AND (financial_status:'PARTIALLY_REFUNDED' OR financial_status:'REFUNDED'))"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider separating the repeated status condition into a variable to reduce duplication and improve readability.

As the status check is always there in filterQuery, we should declare it at the top and then conditionally append updated_at checks.

node(id: "${shopifyOrderId}") {
id
... on Order {
refunds(first: 10) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will you make sure that we get all the refunds for a specific order?

... on Order {
refunds(first: 10) {
id
transactions(first: 10) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where will you be using the transactions list, and if using the same thing, how will you make sure to get all transactions?

Consider the GraphQL query cost.

<histories versionName="01" previousVersionName="01"/>
</file>
</moqui.resource.DbResource>
<moqui.resource.DbResource filename="RefundIdsByOrderIdQuery.ftl" isFile="Y" resourceId="RefundIdsByOrderIdQuery" parentResourceId="GraphQL">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the filename, you are querying to fetch the transactions' createdAt too with refund ids

<if condition="!refundResponse.response.node">
<return type="warning" message="No Shopify order found for id: ${shopifyOrderId}"/>
</if>
<set field="refundIdList" from="[]"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What arethe requirements for this handling

<set field="refundIdList" from="[]"/>
<if condition="refundResponse.response.node.refunds">
    <script>refundIdList.addAll(refundResponse.response.node.refunds)</script>
</if>


<service-call name="co.hotwax.shopify.return.ShopifyReturnServices.get#RefundsByOrderId"
in-map="[systemMessageRemoteId:systemMessage.systemMessageRemoteId, shopifyOrderId:order.id]"
out-map="refundsResponse" ignore-error="true" transaction="force-new"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use force-new

<service-call name="co.hotwax.shopify.return.ShopifyReturnServices.get#RefundsByOrderId"
in-map="[systemMessageRemoteId:systemMessage.systemMessageRemoteId, shopifyOrderId:order.id]"
out-map="refundsResponse" ignore-error="true" transaction="force-new"/>
<set field="orderMap.refunds" from="refundsResponse.orderRefunds.refunds"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential NPE

<iterate list="refundIdList" entry="refund">
<set field="refundIdString" from="refund.id"/>
<service-call name="co.hotwax.shopify.return.ShopifyReturnServices.get#RefundDetails" in-map="[systemMessageRemoteId:systemMessageRemoteId, refundId:refundIdString]" out-map="refundDetails"/>
<script>refunds.addAll(refundDetails.refundDetail)</script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried running this code once? I think the addAll method will throw an error as it accepts a List.

@Prerakghatode Prerakghatode force-pushed the 523-enhancement-returns-and-refunds-flow-merge branch from bc68a19 to 7cdddaa Compare July 10, 2025 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants