[Main]-Payment XML file generated from EB Payment Journal upon selecting "FCY Symbol" includes currency text by neeleshsinghal · Pull Request #9356 · microsoft/BCApps

neeleshsinghal · 2026-07-13T11:30:28Z

Fixes AB#641821
Workitem :- Bug 641821: [master] [all-e]Payment XML file generated from EB Payment Journal upon selecting "FCY Symbol" includes currency text

Copilot

Pull request overview

This PR fixes the Belgian SEPA payment XML export so that the <InstdAmt> element contains a strictly numeric value (no currency symbol/text) even when General Ledger Setup “Show Currency” is configured to show FCY symbols.

Changes:

Update InstdAmt XML value formatting to use an explicit numeric format string (<Precision,2:2><Standard Format,9>) in the EUR SEPA export.
Apply the same explicit numeric formatting in the Non-Euro SEPA export.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Layers/BE/BaseApp/Local/BankMgt/Payment/FileSEPAPayments.Report.al	Forces numeric-only formatting for `<InstdAmt>` in the EUR SEPA payment XML.
src/Layers/BE/BaseApp/Local/BankMgt/Payment/FileNonEuroSEPAPayments.Report.al	Forces numeric-only formatting for `<InstdAmt>` in the non-EUR SEPA payment XML.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);
+        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>'), '', XMLNewChild);


alexei-dobriansky · 2026-07-13T17:07:07Z

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR changes the InstdAmt value in File SEPA Payments and File Non Euro SEPA Payments from Format(PmtJnlLine.Amount, 0, 9) to an explicit numeric format. For these two reports, this fixes the reported problem: the XML amount node stays numeric, while the currency remains in the Ccy attribute (EUR for SEPA, or the ISO currency code for non-euro SEPA). The normal non-FCY output should stay compatible because the new format matches the existing BE test helper expectation for InstdAmt. The fix is not complete because the exposed pain.001.001.09 SEPA reports still use the old amount format.

Suggestions

S1 - Update all SEPA XML exports
File FCY SEPA 001.001.09 Pmts and File SEPA 001.001.09 Pmts still write InstdAmt with Format(PmtJnlLine.Amount, 0, 9). Use the same numeric-only format there, because those export protocols can hit the same Show Currency setting and write currency text into the amount node.

S2 - Add an XML regression test
Add a test in EBPaymentJournalExport.Codeunit.al that sets General Ledger Setup.Show Currency to FCY Symbol Only and exports the affected payment file. Assert that InstdAmt contains only the two-decimal amount and that the Ccy attribute still contains the ISO currency code. Existing helpers such as VerifyXMLAmountCurrency make this practical.

Risk assessment and necessity

Risk: This is payment export XML, so a bad value can create a bank-rejected payment file or a malformed payment amount. The changed reports keep the currency in the correct XML attribute and do not change public APIs, but the same old formatting remains in the pain.001.001.09 BE SEPA reports.

Necessity: The bug is valid and has clear repro steps: selecting FCY Symbol Only in General Ledger Setup makes the exported amount include currency text. Without a fix, users must change the display setting before export, which is not a safe workaround for payment processing. The change is needed, but the scope and tests need to cover the equivalent exposed export reports.

[AI-PR-REVIEW] version=1 system=github pr=9356 round=1 by=alexei-dobriansky at=2026-07-13T17:06:43Z lastSha=840dfae526640c917048c51296520e3a79a9d49a suggestions=S1,S2

github-actions · 2026-07-14T09:14:05Z

Copilot PR Review

Iteration 6 · Outcome: completed

All sub-skills completed or returned not-applicable; no sub-skill failed or ran partially. The self-review pass surfaced one cross-cutting correctness concern not covered by any leaf domain.

Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf

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	2	0	2	2	0

Totals: 0 knowledge-backed · 2 agent findings.

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

layer-disabled (knowledge) : 2 file(s)

_{Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives.}

…Journal-upon-FCY-Symbol-include-currency-text

github-actions · 2026-07-14T10:26:42Z

+        GeneralLedgerSetup.Modify(true);
+    end;
+
+    local procedure SetShowCurrencyToLCYAndFCYSymbol()


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

SetShowCurrencyToLCYAndFCYSymbol, added in this PR, is never called anywhere in the codeunit or repository.

It configures General Ledger Setup for an 'LCY and FCY Symbol' scenario that the new test VerifyInstdAmtIsNumericWhenShowCurrencyIsFCYSymbolOnly does not exercise (it only calls SetShowCurrencyToFCYSymbolOnly).

Recommendation:

either delete the unused procedure, or add the missing test scenario that exercises the 'LCY and FCY Symbol' Show Currency option if that coverage was intended.

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

…ol-include-currency-text' of https://github.com/neeleshsinghal/BCApps into bugs/Bug-641821-Export-EB-Payment-Journal-upon-FCY-Symbol-include-currency-text

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

+    [Test]
+    [HandlerFunctions('SuggestVendorPaymentsReportHandler,FileSEPAPaymentsReportHandlerNonEuro')]
+    procedure VerifyInstdAmtIsNumericWhenShowCurrencyIsFCYSymbolOnly()


        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);
+        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>'), '', XMLNewChild);


        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);
+        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>'), '', XMLNewChild);


        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);
+        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>'), '', XMLNewChild);


        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);
+        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>'), '', XMLNewChild);


alexei-dobriansky · 2026-07-14T11:06:02Z

Agentic PR Review - Round 2

Recommendation: Accept

What this PR does

Since round 1, the PR updates the two pain.001.001.09 SEPA export reports to write InstdAmt with the same numeric-only format as the earlier reports. The final head now uses Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>') in all four affected BE SEPA payment reports, and the currency remains in the Ccy attribute. The added regression test sets General Ledger Setup Show Currency to FCY Symbol Only, exports non-euro SEPA payments, and checks both the numeric amount text and ISO currency attributes.

Status of previous suggestions

ID	Title	Status	Author response
S1	Update all SEPA XML exports	Addressed	Fixed in `c2dd6f0a8c`; the two pain.001.001.09 reports now use the numeric-only amount format.
S2	Add an XML regression test	Addressed	Fixed in `c2dd6f0a8c`; the new test covers `FCY Symbol Only`, `InstdAmt`, and the `Ccy` ISO code.

New observations (commits since round 1)

None - changes only addressed prior suggestions.

Risk assessment and necessity

Risk: This remains payment export XML, so incorrect formatting can create bank-rejected files. The round-2 changes are narrow: they only change amount formatting in the two remaining reports, add a focused regression test, and remove an unused test helper. No public API or event signature changes are involved.

Necessity: The linked bug is still valid and important. Without this fix, users can get currency text inside an XML amount node when Show Currency is FCY Symbol Only, and the workaround is to change a display setting before export. The final scope now matches the affected SEPA export variants.

[AI-PR-REVIEW] version=1 system=github pr=9356 round=2 by=alexei-dobriansky at=2026-07-14T11:05:01Z lastSha=6e7f922acc341433af2e300833fe074feb2ce8fc suggestions=S1:addressed,S2:addressed parentRound=1

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

+    local procedure SetShowCurrencyToFCYSymbolOnly()
+    var
+        GeneralLedgerSetup: Record "General Ledger Setup";
+    begin
+        GeneralLedgerSetup.Get();
+        GeneralLedgerSetup.Validate("Show Currency", GeneralLedgerSetup."Show Currency"::"FCY Symbol Only");
+        GeneralLedgerSetup.Modify(true);
+    end;


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

+    local procedure SetShowCurrencyToFCYSymbolOnly()
+    var
+        GeneralLedgerSetup: Record "General Ledger Setup";
+    begin
+        GeneralLedgerSetup.Get();
+        GeneralLedgerSetup.Validate("Show Currency", GeneralLedgerSetup."Show Currency"::"FCY Symbol Only");
+        GeneralLedgerSetup.Modify(true);
+    end;
+
+    local procedure SetCurrencySymbol(CurrencyCode: Code[10])


        // Preparation: create settings
+        SetShowCurrencyToFCYSymbolOnly();
        CountryCode := FindCountryRegion();


…Journal-upon-FCY-Symbol-include-currency-text

alexei-dobriansky · 2026-07-15T11:03:46Z

Agentic PR Review - Round 3

Recommendation: Accept

What this PR does

Since round 2, the new commit only changes the automation. It removes the separate VerifyInstdAmtIsNumericWhenShowCurrencyIsFCYSymbolOnly test and moves its setup into the existing multi-currency non-euro SEPA XML test. The product reports at the current head still write InstdAmt with the numeric-only format, and the XML currency is still written through the Ccy attribute.

Status of previous suggestions

ID	Title	Status	Author response
S1	Update all SEPA XML exports	Addressed	Still addressed at head; all four affected BE SEPA payment reports use `Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>')` for `InstdAmt`.
S2	Add an XML regression test	Addressed	Still addressed at head; the multi-currency non-euro test now sets `Show Currency` to `FCY Symbol Only`, sets currency symbols, and verifies both numeric `InstdAmt` text and ISO `Ccy` attributes.

New observations (commits since round 2)

None - the automation change keeps the regression coverage and does not weaken the product fix.

Risk assessment and necessity

Risk: This is still payment export XML, so wrong formatting can create bank-rejected payment files. The round-3 change is limited to test code. It keeps the same assertions for InstdAmt and Ccy, while the production reports remain unchanged from the accepted round-2 state.

Necessity: The linked bug remains a valid payment export defect. The automation change is reasonable because it avoids a duplicate test while preserving the FCY Symbol Only regression scenario in an existing multi-currency XML coverage path.

[AI-PR-REVIEW] version=1 system=github pr=9356 round=3 by=alexei-dobriansky at=2026-07-15T11:03:16Z lastSha=821dd61638702cccee8d60b108d8e0bb7ce86b0a suggestions=S1:addressed,S2:addressed parentRound=2

…Journal-upon-FCY-Symbol-include-currency-text

github-actions · 2026-07-15T14:45:46Z

        AddElement(XMLNodeCurr, 'Amt', '', '', XMLNewChild);
        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);


$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 6}}$

Same pattern as in FileNonEuroSEPAPayments.Report.al: Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>') hardcodes 2 decimal digits for the InstdAmt element.

This report (File FCY SEPA 001.001.09 Pmts) also uses a dynamic ISOCurrCode, so it exports payments in arbitrary foreign currencies whose configured 'Amount Decimal Places' may differ from 2, risking silent rounding/padding of the transferred amount in the generated ISO 20022 file. Capped at minor pending a BCQuality knowledge article on currency-aware amount formatting; consider deriving the precision from the currency's own decimal-places setting rather than a fixed '2:2'.

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

github-actions · 2026-07-15T14:45:47Z

        AddElement(XMLNodeCurr, 'Amt', '', '', XMLNewChild);
        XMLNodeCurr := XMLNewChild;

-        AddElement(XMLNodeCurr, 'InstdAmt', Format(PmtJnlLine.Amount, 0, 9), '', XMLNewChild);


$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 6}}$

The fix for the currency-symbol leak now formats PmtJnlLine.Amount with the hardcoded format string '<Precision,2:2><Standard Format,9>', forcing exactly 2 decimal digits.

The previous code (Format(PmtJnlLine.Amount, 0, 9)) rendered the value with whatever decimal precision the amount actually carried. This report (File Non Euro SEPA Payments) explicitly supports arbitrary foreign currencies via a dynamic ISOCurrCode/'Ccy' attribute, and Business Central currencies can be configured with an 'Amount Decimal Places' other than 2 (e.g. 0 or 3 decimals). Hardcoding 2 decimals means the InstdAmt element in the exported ISO 20022 payment file will silently round or pad amounts for any such currency instead of reflecting the currency's configured precision, which is a real-money correctness risk in a bank payment export. If the underlying impact materializes it would warrant major severity, but this is capped at minor here because there is no BCQuality knowledge article backing it yet; consider formatting with the currency's own decimal-places precision (e.g. via Currency."Amount Decimal Places") instead of a fixed '2:2', and this should be promoted to a knowledge-backed rule once confirmed.

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

FIx

840dfae

neeleshsinghal added the Finance GitHub request for Finance area label Jul 13, 2026

neeleshsinghal added this to the Version 29.0 milestone Jul 13, 2026

neeleshsinghal added the Linked Issue is linked to a Azure Boards work item label Jul 13, 2026

neeleshsinghal requested a review from Copilot July 13, 2026 11:33

github-actions Bot added the From Fork Pull request is coming from a fork label Jul 13, 2026

Copilot started reviewing on behalf of neeleshsinghal July 13, 2026 11:38 View session

Copilot AI reviewed Jul 13, 2026

View reviewed changes

neeleshsinghal marked this pull request as ready for review July 13, 2026 13:08

neeleshsinghal requested a review from a team July 13, 2026 13:08

neeleshsinghal enabled auto-merge July 13, 2026 13:48

Suggestion

c2dd6f0

Merge branch 'microsoft:main' into bugs/Bug-641821-Export-EB-Payment-…

6bca8af

…Journal-upon-FCY-Symbol-include-currency-text

github-actions Bot reviewed Jul 14, 2026

View reviewed changes

neeleshsinghal added 2 commits July 14, 2026 16:20

remove procedure

adc328e

Merge branch 'bugs/Bug-641821-Export-EB-Payment-Journal-upon-FCY-Symb…

6e7f922

…ol-include-currency-text' of https://github.com/neeleshsinghal/BCApps into bugs/Bug-641821-Export-EB-Payment-Journal-upon-FCY-Symbol-include-currency-text

neeleshsinghal requested a review from Copilot July 14, 2026 10:52

Copilot started reviewing on behalf of neeleshsinghal July 14, 2026 10:54 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

neeleshsinghal requested review from a team, AndersLarsenMicrosoft and v-ankitgoyal July 14, 2026 11:19

Correct the automation

811ed60

neeleshsinghal requested a review from Copilot July 15, 2026 06:38

Copilot started reviewing on behalf of neeleshsinghal July 15, 2026 06:38 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

neeleshsinghal requested a review from Copilot July 15, 2026 08:13

Copilot started reviewing on behalf of neeleshsinghal July 15, 2026 08:14 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Merge branch 'microsoft:main' into bugs/Bug-641821-Export-EB-Payment-…

821dd61

…Journal-upon-FCY-Symbol-include-currency-text

Merge branch 'microsoft:main' into bugs/Bug-641821-Export-EB-Payment-…

88a2a63

…Journal-upon-FCY-Symbol-include-currency-text

github-actions Bot reviewed Jul 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Main]-Payment XML file generated from EB Payment Journal upon selecting "FCY Symbol" includes currency text#9356

[Main]-Payment XML file generated from EB Payment Journal upon selecting "FCY Symbol" includes currency text#9356
neeleshsinghal wants to merge 8 commits into
microsoft:mainfrom
neeleshsinghal:bugs/Bug-641821-Export-EB-Payment-Journal-upon-FCY-Symbol-include-currency-text

neeleshsinghal commented Jul 13, 2026 •

edited by github-actions Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

alexei-dobriansky commented Jul 13, 2026

Uh oh!

github-actions Bot commented Jul 14, 2026 •

edited

Loading

Uh oh!

github-actions Bot Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

alexei-dobriansky commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

alexei-dobriansky commented Jul 15, 2026

Uh oh!

github-actions Bot Jul 15, 2026

Uh oh!

github-actions Bot Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

neeleshsinghal commented Jul 13, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

alexei-dobriansky commented Jul 13, 2026

Agentic PR Review - Round 1

Recommendation: Request Changes

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Copilot PR Review

Findings by domain

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

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

SetShowCurrencyToLCYAndFCYSymbol, added in this PR, is never called anywhere in the codeunit or repository.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

alexei-dobriansky commented Jul 14, 2026

Agentic PR Review - Round 2

Recommendation: Accept

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

alexei-dobriansky commented Jul 15, 2026

Agentic PR Review - Round 3

Recommendation: Accept

Uh oh!

github-actions Bot Jul 15, 2026

Choose a reason for hiding this comment

Same pattern as in FileNonEuroSEPAPayments.Report.al: Format(PmtJnlLine.Amount, 0, '<Precision,2:2><Standard Format,9>') hardcodes 2 decimal digits for the InstdAmt element.

Uh oh!

github-actions Bot Jul 15, 2026

Choose a reason for hiding this comment

The fix for the currency-symbol leak now formats PmtJnlLine.Amount with the hardcoded format string '<Precision,2:2><Standard Format,9>', forcing exactly 2 decimal digits.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neeleshsinghal commented Jul 13, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading