[Shopify] Fix missing shop currency mapping on order transactions

[nikolakukrika]

Problem

The amountSet.shopMoney.currencyCode was no longer mapped to the Currency field on Shpfy Order Transaction records. This regression was introduced in commit acd8280 (presentment currency support) where the existing shopMoney.currencyCode mapping was accidentally replaced by the new presentmentMoney lines instead of being kept alongside them.

This causes the Currency column on the Shopify Order Transactions page to show stale/incorrect values from prior sync runs or remain empty for new transactions - while TranslateCurrencyCode(OrderTransaction.Currency) is called expecting the field to be populated.

Root Cause

In commit acd8280, the shopMoney.currencyCode line was replaced by presentmentMoney lines instead of being kept alongside them.

Fix

Re-add the missing amountSet.shopMoney.currencyCode to Currency mapping, restoring parity with how amountRoundingSet correctly handles both shopMoney and presentmentMoney currency codes in the same block.

Verification

• The GraphQL query (ShpfyGQLOrderTransactions.Codeunit.al) already requests shopMoney { amount currencyCode } - the data was available but not being read.
• The TranslateCurrencyCode call on line 121 correctly handles the raw Shopify currency code once populated.
• The amountRoundingSet block (lines 99-102) was already correct (both shopMoney and presentmentMoney mapped).

Fixes AB#641570

Ref: ICM 51000001074581

[github-actions]

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

Currency lookup calls Count() twice on same set

The new currency resolution logic calls Currency.Count() twice on the same filtered record set — once to check if count equals 1, and again to check if count is greater than 1 — which results in two redundant database round-trips.

Recommendation:

• Capture the count in a local variable and reuse it: LocalCount := Currency.Count(); if LocalCount = 1 then ... else if LocalCount > 1 then ...

local CurrencyCount: Integer;
CurrencyCount := Currency.Count();
if CurrencyCount = 1 then begin
    Currency.FindFirst();
    CurrencyCode := Currency.Code;
end else begin
    if CurrencyCount > 1 then
        Session.LogMessage('0000S1A', StrSubstNo(DuplicateISOCodeTelemetryLbl, ShopifyCurrencyCode, CurrencyCount), Verbosity::Warning, DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, 'Category', CategoryTok);
    if Currency.Get(CopyStr(ShopifyCurrencyCode, 1, MaxStrLen(Currency.Code))) then
        CurrencyCode := Currency.Code;
end;

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

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

[github-actions]

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

Local procedure broadened to internal for testability

TranslateCurrencyCode was local and is now internal, exposing an implementation detail to the entire app extension solely so test codeunits can call it directly. Widening visibility to satisfy test needs couples tests to internal implementation and makes refactoring harder.

Recommendation:

• Keep the procedure local and test its behavior indirectly through the existing ImportOrder or ImportCreateAndUpdateOrderHeaderFromMock paths, as done in the integration tests. If direct unit testing is required, consider an explicit test-support hook or a wrapper in a dedicated test helper codeunit.

    local procedure TranslateCurrencyCode(ShopifyCurrencyCode: Text): Code[10]

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

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

[onbuyuka]

Edge case: TranslateCurrencyCode can return a non-blank code for local currency when LCY Code ≠ ISO Code

The rewritten TranslateCurrencyCode moved the LCY check to an early exit that compares the raw Shopify ISO code directly to G/L Setup."LCY Code":

if CopyStr(ShopifyCurrencyCode, 1, MaxStrLen(GeneralLedgerSetup."LCY Code")) = GeneralLedgerSetup."LCY Code" then
    exit('');

Shopify always sends ISO 4217 codes, and Currency."ISO Code" is ISO 4217 — but Currency.Code (the primary key) is free-form and is not required to equal the ISO code. The previous implementation compared the resolved BC Currency.Code to LCY Code; the new one compares the raw ISO code. These diverge whenever Code ≠ ISO Code.

Failing configuration — local currency record Code = 'DAN', ISO Code = 'DKK', and G/L Setup."LCY Code" = 'DAN'. Shopify sends DKK:

	Old logic	New logic
Resolve ISO→Code	DKK → DAN	DKK → DAN
LCY check	DAN = DAN → '' ✓	raw DKK ≠ DAN → returns DAN ✗

So a transaction that is actually in local currency gets stamped with a non-blank currency code instead of blank — a currency mismatch versus the LCY document.

Note the old logic had the mirror gap (when LCY Code is ISO-style but the currency record's Code differs, the raw check catches it but the resolved-code check does not). Neither approach alone is robust to Code ≠ ISO Code.

Suggested fix — keep the early exit as a fast path for the common ISO-convention case, and also re-check the resolved code against LCY Code at the end (union of old + new behavior). While here, cache Currency.Count() to address the redundant round-trips flagged elsewhere:

internal procedure TranslateCurrencyCode(ShopifyCurrencyCode: Text): Code[10]
var
    Currency: Record Currency;
    GeneralLedgerSetup: Record "General Ledger Setup";
    CurrencyCode: Code[10];
    CurrencyCount: Integer;
begin
    if ShopifyCurrencyCode = '' then
        exit('');

    GeneralLedgerSetup.Get();
    if CopyStr(ShopifyCurrencyCode, 1, MaxStrLen(GeneralLedgerSetup."LCY Code")) = GeneralLedgerSetup."LCY Code" then
        exit('');

    Currency.SetLoadFields(Code);
    Currency.SetRange("ISO Code", CopyStr(ShopifyCurrencyCode, 1, 3));
    CurrencyCount := Currency.Count();
    if CurrencyCount = 1 then begin
        Currency.FindFirst();
        CurrencyCode := Currency.Code;
    end else
        if CurrencyCount > 1 then begin
            Session.LogMessage('0000S1A', StrSubstNo(DuplicateISOCodeTelemetryLbl, ShopifyCurrencyCode, CurrencyCount), Verbosity::Warning, DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, 'Category', CategoryTok);
            Currency.FindFirst();
            CurrencyCode := Currency.Code;
        end else
            if Currency.Get(CopyStr(ShopifyCurrencyCode, 1, MaxStrLen(Currency.Code))) then
                CurrencyCode := Currency.Code
            else
                Session.LogMessage('0000S1B', StrSubstNo(CurrencyNotFoundTelemetryLbl, ShopifyCurrencyCode), Verbosity::Warning, DataClassification::SystemMetadata, TelemetryScope::ExtensionPublisher, 'Category', CategoryTok);

    // Edge case: LCY Code is configured as a non-ISO currency code, so the fast path
    // above did not catch it. Treat the resolved code as LCY as well.
    if CurrencyCode = GeneralLedgerSetup."LCY Code" then
        exit('');

    exit(CurrencyCode);
end;

I'll also add a test where Currency.Code ≠ "ISO Code" and the shop currency equals LCY-by-ISO, asserting the result is blank — this currently isn't exercised (the existing tests set ISO = Code).

[onbuyuka]

Pushed a commit (23dc6dc) applying the fix and folding in the actionable review feedback:

Applied

• LCY edge case — after resolving the ISO code, also compare the resolved BC currency code to LCY Code, so a shop currency in local currency returns blank even when LCY Code is configured as a non-ISO code (Code ≠ ISO Code). Kept the raw early-exit as a fast path.
• Currency.Count() called 3× (High-severity perf comment) — cached it in a CurrencyCount local and reused it in all three places.
• Invalid object ID — the new test codeunit was 139700, which is outside the Shopify Test app's idRanges (they end at 139699), so it fails to compile (AL0297). Reassigned to the free ID 139697. This wasn't in the bot comments but would have blocked CI.
• Added TranslateCurrencyCodeReturnsEmptyWhenResolvedCodeIsLcy to guard the edge case (non-committing unit test, so no cross-run currency accumulation).

Considered, not changed

• "Test currency ISO code equals Currency.Code; translation not exercised" — the ISO→Code translation is already unit-tested by TranslateCurrencyCodeWithISOCodeMatch (ISO 'ZZZ' ≠ generated Code). The integration tests intentionally set ISO = Code because they Commit(); a fixed non-Code ISO would accumulate duplicate currencies across local runs and break the Count() = 1 path. Left as-is.
• "local broadened to internal for testability" — kept internal; the unit tests call TranslateCurrencyCode directly and internal (not public) keeps it within the app.
• "No linked ADO work item" — the description links an ICM but no AB#. @nikolakukrika could you add Fixes AB#<id> for the backing work item?

Validated: App project builds clean (0 errors); both modified test files compile with 0 errors.

[github-actions]

Copilot PR Review

Iteration 5 · Outcome: completed

All sub-skills completed or correctly reported not-applicable; no findings met the citation or agent-finding bar after review of both changed source files and the new test codeunit.

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

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.}

[onbuyuka]

Pushed 98b0ab6 fixing the failing UncategorizedTests (the transaction tests were failing across every country). Two root causes:

1. "Transaction should be created" (5 tests) — the tests imported a full order via ImportCreateAndUpdateOrderHeaderFromMock, which runs SetAndCreateRelatedRecords → fulfillment, shipping, then transactions, each an ExecuteGraphQL call. GraphQL read queries bypass the Allow Outgoing Requests gate (CheckOutgoingRequests returns early for "query"), so all three hit the single queue-based mock handler. The fulfillment query dequeued the enqueued transaction data first, so by the time the transactions query ran the queue was empty and no Shpfy Order Transaction was created.
→ Fixed by importing transactions in isolation via Transactions.UpdateTransactionInfos(OrderHeader) — exactly one GraphQL call reaches the mock.

2. TranslateCurrencyCodeFallsBackToCodeMatch (Expected GU00000063, Actual GU00000007) — the tests set Currency."ISO Code" := CopyStr(Code, 1, MaxStrLen("ISO Code")). "ISO Code" is Code[3], so every generated currency got ISO 'GU0', and because the tests Commit(), these accumulated. SetRange("ISO Code", 'GU0') then matched multiple rows and FindFirst returned the wrong one.
→ Fixed by using distinct 3-char ISO codes (Z01, Z02, …) and dropping Commit() so per-test currency data is rolled back and can't pollute later tests.

Also: removed UnitTestTransactionCurrencyIsSetFromOrderImport from ShpfyShippingChargesTest — it's a transaction-currency test misplaced in the shipping codeunit and relied on the same fragile shared-handler flow. The equivalent coverage now lives, robustly and isolated, in ShpfyTransactionsTest.

Both edited test files compile clean (0 errors / 0 warnings) locally against the full v29 symbol set; CI will confirm the run.

[onbuyuka]

The re-run got past the infra outage and surfaced the real failure: two of the new tests hit The record in table "Shopify Order Header" already exists for fixed ids (691767, 323801 — identical across the BE and CA jobs). Root cause: Any.IntegerInRange is deterministic, so the generated order/transaction ids collided with records committed by other Shopify test codeunits in the shared UncategorizedTests database (uncommitted per-test data rolls back, but other codeunits' committed rows persist).

Notably, ImportTransactionSetsShopCurrencyFromJson and ImportTransactionSetsPresentmentCurrencyFromJson passed — so the transaction-import and currency-mapping logic itself is correct; only the id generation was fragile.

Fixed in a31ccb0: generate Shopify Order Id and Shopify Transaction Id as FindLast + 1 (always above any existing row) and skip ISO codes already in use. New run is building.