[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{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Extra GraphQL call on every order import

MarkExchangeItemOrderLines is called unconditionally for every order import, making an extra paginated GraphQL API call even for orders that have no returns or exchanges. This will increase API quota consumption and slow down bulk order imports.

Recommendation:

• Add an early-exit guard before making the first API call. For example, check whether any returns exist for the order (e.g. via a cheap metadata field or a header flag) before entering the repeat..until loop.

Suggested change

	internal procedure MarkExchangeItemOrderLines(var OrderHeader: Record "Shpfy Order Header")
	// Before Parameters.Add('OrderId', ...) in MarkExchangeItemOrderLines:
	if not OrderHeader."Has Returns" then
	exit;

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

[github-actions]

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

Pagination cursor set before first page check

In MarkExchangeItemOrderLines the 'After' cursor is unconditionally added from the first API response before checking hasNextPage. If endCursor is null/empty and hasNextPage is false, the empty cursor value persists in Parameters for no reason. More critically, if hasNextPage is unexpectedly true with a null cursor, the loop will make repeated identical API calls.

Recommendation:

• Move the cursor update inside the hasNextPage check, mirroring the pattern used in other pagination loops in this codebase.

Suggested change

	if Parameters.ContainsKey('After') then
	// After processing JReturns, only set the cursor if there are more pages:
	if JsonHelper.GetValueAsBoolean(JResponse, 'data.order.returns.pageInfo.hasNextPage') then begin
	if Parameters.ContainsKey('After') then
	Parameters.Set('After', JsonHelper.GetValueAsText(JResponse, 'data.order.returns.pageInfo.endCursor'))
	else
	Parameters.Add('After', JsonHelper.GetValueAsText(JResponse, 'data.order.returns.pageInfo.endCursor'));
	end;

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

[github-actions]

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

Missing lock on rows updated in FindSet loop

MarkExchangeItemOrderLines calls OrderLine.FindSet() (no lock) and then calls OrderLine.Modify() inside the loop. Concurrent processes that also modify OrderLine records could cause lost updates or non-repeatable reads.

Recommendation:

• Use OrderLine.FindSet(true) to acquire an optimistic lock, or call OrderLine.LockTable() before FindSet to prevent concurrent modification.

Suggested change

	exit;
	OrderLine.LockTable();
	if OrderLine.FindSet() then
	repeat
	if ExchangeLineIds.Contains(OrderLine."Line Id") and (not OrderLine."Is Exchange Item") then begin
	OrderLine."Is Exchange Item" := true;
	OrderLine.Modify();
	end;
	until OrderLine.Next() = 0;

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

[github-actions]

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

Pagination cursor set before first page check

In GetRefundExchangeLines the 'After' cursor is added/set from the response before confirming the first page returned any items. If the API returns an empty nodes array and a null endCursor, Parameters will contain an empty 'After' value that is sent in subsequent calls, which may cause API errors or an infinite loop if hasNextPage is incorrectly true.

Recommendation:

• Only update the 'After' cursor when hasNextPage is true, or guard against an empty endCursor value before setting the parameter.

Suggested change

	repeat
	if JsonHelper.GetValueAsBoolean(JResponse, 'data.refund.return.exchangeLineItems.pageInfo.hasNextPage') then begin
	if Parameters.ContainsKey('After') then
	Parameters.Set('After', JsonHelper.GetValueAsText(JResponse, 'data.refund.return.exchangeLineItems.pageInfo.endCursor'))
	else
	Parameters.Add('After', JsonHelper.GetValueAsText(JResponse, 'data.refund.return.exchangeLineItems.pageInfo.endCursor'));
	end;

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

[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]

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

Insert then Modify pattern risks orphaned rows

FillInExchangeRefundLine calls RefundLine.Insert() and then sets several fields followed by RefundLine.Modify(). If the process is interrupted between Insert and Modify (e.g. by an exception from JsonHelper calls), an incomplete RefundLine row with default/zero field values will be left in the database.

Recommendation:

• Populate all fields before calling Insert, or use a single Modify(true) with all fields set in one block.

Suggested change

	TotalTaxPresentment += JsonHelper.GetValueAsDecimal(JTaxLine, 'priceSet.presentmentMoney.amount');
	RefundLine.Init();
	RefundLine."Refund Line Id" := SyntheticRefundLineId;
	RefundLine."Refund Id" := RefundHeader."Refund Id";
	RefundLine."Order Line Id" := OrderLineId;
	RefundLine."Restock Type" := RefundLine."Restock Type"::Return;
	RefundLine.Quantity := -ExchangeQuantity;
	// ... set all other fields ...
	if not RefundLine.Get(RefundHeader."Refund Id", SyntheticRefundLineId) then
	RefundLine.Insert()
	else
	RefundLine.Modify();

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

[github-actions]

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

Synthetic ID collision risk at 1000+ line items

SyntheticExchangeRefundLineId computes -(ExchangeLineItemId * 1000 + LineItemIndex). The in-code comment acknowledges collision risk beyond 1000 line items per ExchangeLineItem. If Shopify ever returns more than 1000 entries in lineItems, two distinct exchange lines will map to the same synthetic ID, causing a duplicate primary-key error or silent data overwrite.

Recommendation:

• Use a larger multiplier (e.g. 10000) or adopt a hash/UUID-based synthetic ID generation scheme to reduce collision probability. At minimum add a guard that raises an error when LineItemIndex exceeds the safe limit.

Suggested change

	RefundLine."Can Create Credit Memo" := true;
	local procedure SyntheticExchangeRefundLineId(ExchangeLineItemId: BigInteger; LineItemIndex: Integer): BigInteger
	const
	MaxLineItemsPerExchange = 10000;
	begin
	if LineItemIndex > MaxLineItemsPerExchange then
	Error('Exchange line item index %1 exceeds supported limit %2.', LineItemIndex, MaxLineItemsPerExchange);
	exit(-((ExchangeLineItemId * MaxLineItemsPerExchange) + LineItemIndex));
	end;

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