[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 skips flagged rows so the BC sales invoice excludes the exchange item. (ApplyGlobalDiscounts deliberately keeps summing the discount over all order lines, including the exchange line, because OrderHeader."Discount Amount" is Shopify's order-level total and already includes any discount on that line.)
• 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 x 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.
• Because this negative-qty line is the only BC line that decrements inventory for the exchange item, CreateSalesLinesFromRefundLines resolves its Location Id from the linked Shpfy Order Line (the fulfillment location) at credit-memo build time -- the exchange refund line is created during order import before order lines exist, so the location cannot be resolved earlier.

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 the Shopify Connector (App) project. (The Shopify Connector Test project cannot be compiled on this local enlistment due to a known environmental symbol-cache issue; the test suite runs in CI.)
• al_run_tests against Shpfy Order Refund Test (139611) in earlier iterations: 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.
• 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]

⚠️ 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]

Copilot PR Review

Iteration 5 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

Findings by domain

Findings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).

Domain	Findings	Knowledge-backed	Agent	Inline	Fallback
Agent	1	0	1	1	0

Totals: 0 knowledge-backed · 1 agent findings.

Orchestrator pre-filter (13 file(s) excluded)

• layer-disabled (knowledge) : 13 file(s)

_{Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.}

[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{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 5}}$

In ShpfyRefundsAPI.GetRefundExchangeLines, ExchangeQuantity is read once per ExchangeLineItem ('quantity' field) but FillInExchangeRefundLine is invoked once per entry in that same ExchangeLineItem's 'lineItems' array, passing the full ExchangeLineItem-level ExchangeQuantity to every entry.

MarkExchangeItemOrderLines (same PR) explicitly loops over multiple 'lineItems' per ExchangeLineItem to collect distinct order-line ids, which shows the author anticipated an ExchangeLineItem can be fulfilled by more than one order line (e.g. split fulfillment) -- yet FillInExchangeRefundLine has no way to know each split's individual share of the quantity, because the GraphQL query fetches 'quantity' only at the ExchangeLineItem level, not per lineItem. When an ExchangeLineItem has more than one lineItems entry, each resulting synthetic 'Shpfy Refund Line' will carry the FULL ExchangeQuantity (and the derived Subtotal/Tax amounts computed from it) instead of a proportional share, double- (or N-times-) counting the exchange offset on the credit memo. The one new test (UnitTestCreateCrMemoFromRefundWithExchangeItem) only exercises a single lineItem per exchange, so this path is unexercised. If the impact were confirmed to be a common real-world scenario this would warrant a major/blocker-level fix (and a corresponding BCQuality knowledge rule); recommend either fetching a per-lineItem quantity from the Shopify API if available, or asserting/handling the single-lineItem case explicitly and logging/erroring on the multi-lineItem case until it is properly split.

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

[gggdttt]

This one looks make sense, should we consider to improve it?