[Shopify] Fix incorrect G/L line on credit memo for refund with exchange item

[onbuyuka]

What & why

A Sales Credit Memo created from a Shopify refund contained an incorrect G/L Account line posted to Shop."Refund Account" whenever the originating return included an exchange item (customer returns item A and keeps a different item B in exchange). The bug doubled the customer-balance impact of the exchange value -- in the reported scenario: invoice 2,258, CR memo 1,528, customer ledger residual 730 == 2 × 365 (where 365 = exchange-item value).

Root cause

Shopify exposes exchange items on Return.exchangeLineItems, not on Refund.refundLineItems, and Refund.totalRefundedSet is already net of the exchange value. The connector summed refund lines (returned items only) against totalRefundedSet and FillRemainingAmountLineFields inserted a balancing G/L line for the gap.

Fix (two phases)

Phase A -- order import

• New paginated GraphQL queries Orders_GetOrderExchangeLineItems / …_Next that read order(id) { returns { exchangeLineItems { lineItems { id } } } }.
• New Shpfy Order Line."Is Exchange Item" Boolean.
• ShpfyImportOrder.MarkExchangeItemOrderLines runs after order lines are imported and flags exchange-item rows.
• ShpfyProcessOrder.CreateLinesFromShopifyOrder and ApplyGlobalDiscounts skip flagged rows so the BC sales invoice excludes the exchange item.
• ConsiderRefundsInQuantityAndAmounts skips flagged rows so the Phase-B negative-qty refund entries don't perturb order-header math.

Phase B -- refund import

• New paginated GraphQL queries Refunds_GetRefundExchangeLines / …_Next that read refund(id) { return { exchangeLineItems { nodes { id quantity lineItems { id originalUnitPriceSet { ... } ... } } } } }.
• New Shpfy Refund Line."Is Exchange Item" Boolean.
• ShpfyRefundsAPI.GetRefundExchangeLines inserts one synthetic Shpfy Refund Line per (ExchangeLineItem × lineItem) pair: synthetic negative Refund Line Id (so it can't collide with Shopify-issued positive ids), Quantity = -ExchangeQuantity, Restock Type = Return, prices taken from the new item.
• The existing CreateSalesLinesFromRefundLines then emits a negative-qty Type::Item CR-memo line that offsets the exchange value -- totals reconcile to Refund.totalRefundedSet with no balancing G/L line.

Net effect for the reported scenario

	Before	After
Sales invoice	2,258	1,893
CR memo lines	2 (A item + G/L Refund Acc.)	2 (A +1, B −1)
CR memo total	1,528	1,528
Customer ledger residual	730 ❌	365 ✓

Linked work

Fixes #
AB#637828

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior.

What I tested and the outcome

• al_build clean for both Shopify Connector (App) and Shopify Connector Test projects.
• al_run_tests against Shpfy Order Refund Test (139611): 14/14 pass, including the new UnitTestCreateCrMemoFromRefundWithExchangeItem which asserts:
  • exactly two Type::Item sales lines on the credit memo with quantities +1 and -1;
  • no Type::"G/L Account" line pointing at Shop."Refund Account";
  • SalesHeader."Amount Including VAT" equals RefundHeader."Total Refunded Amount".
• End-to-end re-validated against a live sandbox using the exact scenario from the bug report -- the credit memo now contains the two item lines totalling the refund amount, and the customer ledger settles to the exchange-item value only.

Risk & compatibility

• New "Is Exchange Item" Boolean fields on Shpfy Order Line (id 22) and Shpfy Refund Line (id 14). Default false; existing rows behave unchanged.
• Two new GraphQL queries against Shopify (one per phase). Both use read_returns / read_orders already covered by the connector's scope.
• Fix only takes effect for orders/refunds (re)imported after deployment. Previously-imported refunds need re-import to pick up the synthetic exchange refund lines (Shpfy Refund Header.Updated At gates the refund header refresh, but my new GetRefundExchangeLines call runs after that gate and on every fetch).
• No data-upgrade work item required (additive fields only, no obsoletions).
• No new analyzer warnings.
• Surfaces the new flag on existing pages (Shpfy Order Subform, Shpfy Refund Lines).

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

DataCapture recorded before full line processing

In GetRefundExchangeLines, DataCapture.Add is called immediately after the first ExecuteGraphQL call, before FillInExchangeRefundLine processes the response. If FillInExchangeRefundLine later throws an error, the data capture record already exists, potentially causing duplicate capture entries on retry.

Recommendation:

• Move the DataCapture.Add call to after the foreach loop that processes all exchange line items, or capture at a more granular level inside FillInExchangeRefundLine (as is already done there).

// Move DataCapture.Add to after the foreach JExchangeLineItem loop:
foreach JExchangeLineItem in JExchangeLineItems do begin
    // ... process items ...
end;
DataCapture.Add(Database::"Shpfy Refund Header", RefundHeader.SystemId, JResponse);

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.

---

🔄 How to trigger a new status check:

1. 📤 Push a new commit to the PR branch, or
2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Extra GraphQL call for every imported order

MarkExchangeItemOrderLines unconditionally executes at least one GraphQL API call (Orders_GetOrderExchangeLineItems) for every order import, including orders that have never had a return. Because the repeat…until loop fires before checking hasNextPage, there is no zero-call fast path. For high-volume Shopify stores this substantially increases API quota consumption and import duration.

Recommendation:

• Check for existing returns on the order header before making the API call, or add a first-pass check (e.g. whether Returns exist on the order) to skip the loop when there are none.

// Quick guard: exit early if the order has no returns recorded
if OrderHeader."Return Id" = 0 then
    exit;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Synthetic refund line ID collision above 1000 line items

SyntheticExchangeRefundLineId computes -(ExchangeLineItemId * 1000 + LineItemIndex). If any ExchangeLineItem has more than 999 associated line items, or if two ExchangeLineItems differ by exactly 1 and the first has LineItemIndex values reaching into the thousands, the formula produces duplicate IDs. The comment acknowledges "before collision risk" but leaves it silent—a collision would silently overwrite the earlier record during RefundLine.Insert/Modify.

Recommendation:

• Use a safer hashing or encoding scheme that avoids collisions regardless of list length, such as combining the IDs with a large prime multiplier or storing a counter-based synthetic ID that is guaranteed unique within the refund.

local procedure SyntheticExchangeRefundLineId(ExchangeLineItemId: BigInteger; LineItemIndex: Integer): BigInteger
begin
    // Use a large prime to minimise collision probability across any realistic data.
    exit(-((ExchangeLineItemId * 1000000) + LineItemIndex));
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[onbuyuka]

Thanks for the review. Pushed 17360cb addressing the actionable items; rationale for the rest below.

Applied

• Extra GraphQL call on every order import (high, raised a few times): added an early-exit in MarkExchangeItemOrderLines that returns when OrderHeader.""Return Status"" is blank or No Return, so orders with no return never make the call.
• SetLoadFields in the order-line scan: added OrderLine.SetLoadFields(""Line Id"", ""Is Exchange Item"").
• Synthetic ID collision above 1000 line items: replaced the -(ExchangeLineItemId * 1000 + index) scheme with -OrderLineId. A Shopify line-item id is globally unique, so the negated value cannot collide with Shopify's positive refund-line ids or with another exchange line — the 1000-item ceiling is gone entirely and the helper/index bookkeeping is removed.
• ""Is Exchange Item"" field Visible guard on Shpfy Refund Lines: set Visible = false.

Not changed (with rationale)

• Pagination cursor set before hasNextPage check (both loops): this matches the established pattern used by every other paginated fetch in this codeunit (GetRefundLines, GetRefundShippingLines) and across the connector. The until not hasNextPage condition exits correctly regardless of the cursor value when there is no next page, so an empty After is never re-sent. Changing only the new loops would make them inconsistent with the surrounding code.
• FindSet + Modify without LockTable: consistent with the adjacent ConsiderRefundsInQuantityAndAmounts, which modifies Shpfy Order Line the same way within the same single-session order-import flow. Order import is not run concurrently for the same order, so a table lock would only add contention.
• Insert-then-Modify ""orphaned rows"": AL runs the whole import inside an implicit transaction, so an exception between Insert and Modify rolls the row back — no partial row persists. This also mirrors the existing FillInRefundLine.
• GetRefundExchangeLines always makes one API call: the call site already guards with if RefundHeader.""Return Id"" <> 0, so refunds without a return make no call. The one remaining request is required to discover whether exchange line items exist.
• DataCapture recorded before line processing: intentional — it stores the raw GraphQL response for diagnostics; the order relative to line processing is irrelevant.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Exchange-item flag never cleared on re-import

MarkExchangeItemOrderLines sets "Is Exchange Item" := true for lines identified as exchange items but never sets it back to false for lines that were previously marked as exchange items but whose associated return was subsequently cancelled or removed in Shopify. After a return cancellation, those order lines will remain permanently marked as exchange items, causing them to be incorrectly excluded from BC sales documents on every subsequent re-import.

Recommendation:

• Before re-marking, reset all existing exchange-item flags for the order to false, then re-apply the flags based on the current API response.

Suggested change

	if ExchangeLineIds.Contains(OrderLine."Line Id") and (not OrderLine."Is Exchange Item") then begin
	// Before the ExchangeLineIds loop:
	OrderLine.SetRange("Shopify Order Id", OrderHeader."Shopify Order Id");
	OrderLine.SetRange("Is Exchange Item", true);
	if OrderLine.FindSet(true) then
	repeat
	OrderLine."Is Exchange Item" := false;
	OrderLine.Modify();
	until OrderLine.Next() = 0;
	OrderLine.SetRange("Is Exchange Item");

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.

---

🔄 How to trigger a new status check:

1. 📤 Push a new commit to the PR branch, or
2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.