[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 ICM 51000001074581

[github-actions]

Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234'

[github-actions]

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

Currency filter excludes empty ISO code incorrectly

The filter Currency.SetFilter("ISO Code", '<>%1&%2', '', CopyStr(ShopifyCurrencyCode, 1, 3)) uses & (AND) between the two conditions, which will never match any record because no ISO Code can simultaneously be both not-empty AND match the given currency code. The intended logic is likely OR, or a separate SetRange call.

Recommendation:

• Use two separate filter calls or the correct operator. To find currencies matching the ISO code that are not blank: Currency.SetRange("ISO Code", CopyStr(ShopifyCurrencyCode, 1, 3));

Suggested change

	if ShopifyCurrencyCode = '' then
	Currency.SetRange("ISO Code", CopyStr(ShopifyCurrencyCode, 1, 3));

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

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

Currency.Count() called three times in one path

Currency.Count() is invoked on lines 666, 670, and again inside StrSubstNo on line 671 — each is a separate database round-trip. This function is called for every order imported from Shopify, so the redundant queries compound at scale.

Recommendation:

• Cache the result of Currency.Count() in a local CurrencyCount: Integer variable before the if-else block, then reference the variable in all three places.

Suggested change

	if Currency.Count() = 1 then begin
	CurrencyCount := Currency.Count();
	if CurrencyCount = 1 then begin
	Currency.FindFirst();
	CurrencyCode := Currency.Code;
	end else begin
	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);
	end;

_{👍 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}

[github-actions]

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

Test currency ISO code equals Currency.Code; translation not exercised

In ImportTransactionSetsShopCurrencyFromJson and similar tests, the mocked Shopify currency code is set equal to the BC Currency.Code. Because TranslateCurrencyCode returns the same value in this case, the test does not verify the translation path (ISO Code → BC Code) when they differ, leaving the core mapping logic untested.

Recommendation:

• Set the currency's ISO Code to a value that differs from Currency.Code (e.g., set Currency.Code := 'EURC' and Currency."ISO Code" := 'EUR'), then enqueue 'EUR' as the Shopify currency and assert OrderTransaction.Currency = 'EURC'. This exercises the actual ISO-to-BC-code translation.

Suggested change

	CurrencyCode := LibraryERM.CreateCurrencyWithRounding();
	Currency.Get(CurrencyCode);
	Currency."ISO Code" := 'ZYX'; // ISO code deliberately different from Currency.Code
	Currency.Modify();
	TransactionData.Enqueue('ZYX'); // shopMoney.currencyCode (ISO)
	// ... rest of test ...
	LibraryAssert.AreEqual(CurrencyCode, OrderTransaction.Currency, 'Currency should be translated from ISO code to BC currency code');

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