[E-Document Formats] Migrate NAV PRs 247170 and 247176 into BCApps

[Groenbech96]

Migrate formats to new import processing.

AB#580191

[github-actions]

Copilot PR Review

Iteration 6 · Outcome: completed

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	1	0	1	1	0
Error Handling	1	1	0	0	0
Interfaces	1	1	0	0	0
Performance	1	1	0	1	0
Style	1	1	0	1	0

Totals: 4 knowledge-backed · 1 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.}

[Groenbech96]

Follow-up on the work-item check: I could not infer a valid ADO work item from the mirrored source PRs, so this needs a human-provided work item ID. Once an ID is provided, add AB# (or Fixes AB#) to the PR description and the check should pass.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Other} \quad \color{gray}{\texttt{\small Iteration\ 3}}$

This test helper declares EventSubscriberInstance = Manual, but there is no BindSubscription for this codeunit anywhere in the Factura-E test app, so OnAfterCreateEDocument stays inactive and never enqueues the created E-Document.

Either bind the subscriber for the test scope or make the subscriber static if it is meant to be always on in these tests.

Knowledge:

• microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md

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

[github-actions]

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

Item1NoTok and Item2NoTok are declared inside the local var block of AssertPurchaseDocument, and the same pattern is introduced in src/Apps/ES/EDocumentFormats/FacturaE/test/src/FacturaEStructValidations.Codeunit.al.

Procedure-local Labels are fragile in the XLIFF/localization pipeline; move these Labels to the codeunit's top-level var block so translation tooling sees stable object-scoped declarations.

Knowledge:

• microsoft/knowledge/style/labels-declared-at-object-scope.md

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

[github-actions]

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

EDocument XML Helper is introduced as an Access = Public helper codeunit, but its public procedures (SetStringValueInField, SetDateValueInField, SetNumberValueInField, SetCurrencyValueInField, and GetNodeValue) have no XML documentation comments.

On a public library surface this leaves callers guessing about path expectations, truncation behavior, and return semantics; add meaningful /// <summary>, /// <param>, and /// <returns> docs for each public procedure.

Knowledge:

• microsoft/knowledge/style/xmldoc-for-public-library-procedures.md

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 3}}$

This test app depends on "E-Document Format for PINT A-NZ", which adds value 28005 to "E-Document Format" in the same installation graph.

Adding "Mock" at ordinal 6160 inserts a new enum member before that existing value, so persisted rows can resolve to a different member after install or upgrade. Per the referenced guidance, assign a new ordinal greater than every existing value on the enum.

Suggested change

	value(6160; "Mock")
	value(148003; "Mock")

Knowledge:

• microsoft/knowledge/upgrade/enum-values-additive-at-end.md

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 3}}$

This test app depends on "E-Document Format for Factura-E", which adds value 10772 to "E-Document Format" in the same installation graph.

Adding "Mock" at ordinal 6160 inserts a new enum member before that existing value, so persisted rows can resolve to a different member after install or upgrade. Per the referenced guidance, assign a new ordinal greater than every existing value on the enum.

Suggested change

	value(6160; "Mock")
	value(148001; "Mock")

Knowledge:

• microsoft/knowledge/upgrade/enum-values-additive-at-end.md

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Other} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

XmlDocument.ReadFrom is invoked as a statement, ignoring its Boolean success return.

If the received e-document blob is not well-formed XML, ReadFrom returns false and PINTANZXml stays as the default (empty) document; the handler then continues, calls GetRoot on the empty document, runs the case with no LocalName match, and silently produces an empty E-Document Purchase Header. The error surfaces later as a confusing downstream failure or a blank draft.

Recommendation:

• capture the return value and raise a controlled error labeled with the E-Document number when parsing fails.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if not XmlDocument.ReadFrom(TempBlob.CreateInStream(TextEncoding::UTF8), PINTANZXml) then
            Error(XmlParseErr, EDocument."Entry No");

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Other} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

XmlDocument.ReadFrom is invoked as a statement, ignoring its Boolean success return.

Malformed Factura-E XML leaves FacturaEXML as an empty XmlDocument; the handler then calls GetRoot and runs the population routines, silently producing an empty E-Document Purchase Header instead of surfacing a parse failure to the operator.

Recommendation:

• check the return value and raise a controlled error identifying the E-Document when the payload cannot be parsed.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        if not XmlDocument.ReadFrom(TempBlob.CreateInStream(TextEncoding::UTF8), FacturaEXML) then
            Error(XmlParseErr, EDocument."Entry No");

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Other} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

NewLineXML is declared as a local XmlDocument but never instantiated with XmlDocument.Create() before NewLineXML.ReplaceNodes(LineXMLNode) is called inside the foreach.

Because the variable is reused across iterations without a Clear, the operation either fails or leaks state between lines. Compare the sibling PINT A-NZ handler, which correctly does Clear(NewLineXML); NewLineXML := XmlDocument.Create(); NewLineXML.Add(LineXMLNode.AsXmlElement()); before populating.

Recommendation:

• mirror the PINT A-NZ pattern so every iteration constructs a fresh, single-element document before PopulateFacturaEPurchaseLine walks it.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

        foreach LineXMLNode in LineXMLList do begin
            Clear(EDocumentPurchaseLine);
            EDocumentPurchaseLine.Validate("E-Document Entry No.", EDocumentEntryNo);
            EDocumentPurchaseLine."Line No." := EDocumentPurchaseLine.GetNextLineNo(EDocumentEntryNo);
            Clear(NewLineXML);
            NewLineXML := XmlDocument.Create();
            NewLineXML.Add(LineXMLNode.AsXmlElement());
            PopulateFacturaEPurchaseLine(NewLineXML, XmlNamespaces, EDocumentPurchaseLine);
            EDocumentPurchaseLine.Insert(false);
        end;

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Other} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

TryGetUOMCodeFromInternationalCode is named with the Try prefix and clearly intends best-effort semantics (the caller has no fallback path), but it is not decorated with [TryFunction].

Its body calls Evaluate(Code, ImportedUOMCode, 9) without checking the Boolean return, then indexes Enum::"Factura-E Units of Measure".Names().Get(Code) with the unvalidated integer. A malformed UnitOfMeasure element or an out-of-range value from a legitimate but new Factura-E code will raise an uncaught runtime error and abort the entire import instead of leaving "[BC] Unit of Measure" blank.

Recommendation:

• either mark the procedure [TryFunction] and let callers ignore the failure, or guard Evaluate and range-check Code before Get.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

    local procedure TryGetUOMCodeFromInternationalCode(ImportedUOMCode: Text) UOMCode: Code[10]
    var
        UnitOfMeasure: Record "Unit of Measure";
        Names: List of [Text];
        Code: Integer;
    begin
        if ImportedUOMCode = '' then
            exit;
        if not Evaluate(Code, ImportedUOMCode, 9) then
            exit;
        Names := Enum::"Factura-E Units of Measure".Names();
        if (Code < 1) or (Code > Names.Count()) then
            exit;
        UnitOfMeasure.SetRange("International Standard Code", Names.Get(Code));
        if UnitOfMeasure.FindFirst() then
            UOMCode := UnitOfMeasure.Code;
    end;

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

[github-actions]

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

This PR adds SetStringValueInField and SetCurrencyValueInField to the shared "E-Document PEPPOL Utility" codeunit (src/Apps/W1/EDocument/App/src/Processing/Import/StructureReceivedEDocument/EDocumentPEPPOLUtility.Codeunit.al), evidently intended for reuse by the new structured-format handlers.

However, neither the new PINT A-NZ handler nor the new Factura-E handler calls them: both instead define their own local SetTextValueFromNode/SetCurrencyFromNode wrappers that re-implement the identical TryGetStringValue + CopyStr (and SetCurrencyIfForeign) logic. No other caller in the repository invokes the new shared procedures either. The result is dead code added to a shared utility plus duplicated logic across two new format handlers in the same PR, increasing maintenance surface for what should be a single reused implementation.

Recommendation:

• have EDocumentPINTANZHandler and EDocumentFacturaEHandler call EDocumentPEPPOLUtility.SetStringValueInField/SetCurrencyValueInField directly and remove the local duplicate wrapper procedures, or drop the new shared procedures if they are not meant to be reused yet.

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

[github-actions]

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

Inside InsertFacturaEPurchaseInvoiceLines, each XML line calls EDocumentPurchaseLine.GetNextLineNo(EDocumentEntryNo).

That helper performs a FindLast() on the staging table, so importing N lines adds N extra reads against a growing table. Keep the next line number in a local counter (initialized once and incremented by 10000) instead of re-querying the table for every row.

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

[github-actions]

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

The new Library - E-Document codeunit exposes public helper procedures such as SetupStandardVAT() without any XML documentation.

On a library codeunit, the public procedure surface should carry ///

/ / comments so callers get intent and parameter guidance in IntelliSense.

Knowledge:

• microsoft/knowledge/style/xmldoc-for-public-library-procedures.md

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