Skip to content

[FR E-Reporting] Add payment and invoice lifecycle messages - #10437

Open
Milica Đukić (djukicmilica) wants to merge 24 commits into
mainfrom
feature/fr-collected-refused-messages
Open

[FR E-Reporting] Add payment and invoice lifecycle messages#10437
Milica Đukić (djukicmilica) wants to merge 24 commits into
mainfrom
feature/fr-collected-refused-messages

Conversation

@djukicmilica

@djukicmilica Milica Đukić (djukicmilica) commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Why

French electronic invoicing requires lifecycle communication beyond the parent E-Document processing status. Outgoing invoice payments must produce collected or reversed-payment messages, buyers must be able to accept or refuse incoming invoices, and platform lifecycle messages must be correlated without overwriting the invoice's own state.

This change models these exchanges as normalized child E-Document messages and adds reusable transport, queuing, payment-occurrence, and external-reference infrastructure to E-Document Core.

Summary

  • Added a public E-Document message API, IMessageSender integration contract, payload persistence, message status handling, and queued background delivery.
  • Added generic applied and reversed E-Document payment occurrences with replay protection and original-occurrence linkage.
  • Added French Collected and Negative Collected messages for payment applications and unapplications, including CDAR payload generation.
  • Added buyer Accepted and Refused messages for incoming purchase invoices, with mandatory refusal reasons and one-response validation.
  • Added incoming lifecycle-message correlation by external document and message IDs, normalization of Submitted, Accepted, and technical rejection statuses, payload retention, validation, and deduplication.
  • Added a unified French E-Invoice lifecycle history page and focused integration tests for payment, buyer-response, transport, and incoming-message flows.
  • Fixed E-Document test object ID collisions that blocked BCApps validation against NAV.

Fixes AB#637593

@djukicmilica
Milica Đukić (djukicmilica) requested a review from a team as a code owner August 20, 2026 09:29
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Team: Other GitHub request for other area than SCM, Finance or Integration Ownership: Needs Review Ownership is Other, low confidence, or needs manual correction labels Aug 20, 2026
FREInvoiceMessage.SetRange("E-Document Entry No.", EDocument."Entry No");
FREInvoiceMessage.SetRange("Source Occurrence ID", SourceOccurrenceID);
FREInvoiceMessage.SetRange(Type, MessageType);
if FREInvoiceMessage.FindFirst() then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

This lookup uses FindFirst() only to test whether a matching FR E-Invoice Message already exists, but the record is discarded. IsEmpty() expresses the existence check directly and avoids materializing a row unnecessarily.

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

        if not FREInvoiceMessage.IsEmpty() then

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.34.4

if not TempBlob.HasValue() then
Error(MessagePayloadErr, MessageEntryNo);

EDocMessageContext.Initialize(EDocMessage, TempBlob);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Privacy}$

The new outbound E-Document message flow sends customer-content payloads (including FR lifecycle/refusal message content) through IMessageSender without a per-send Privacy Notice approval check in the changed code. Service Integration V2 only gathers generic setup-time consent, so this path can transfer data to an external service without the per-integration privacy-notice gate required for outbound data transfer.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.34.4

Access = Public;
InherentEntitlements = X;

procedure CreateMessage(EDocument: Record "E-Document"; MessageType: Enum "E-Document Message Type"; ResponseType: Enum "E-Doc. Response Type"; var TempBlob: Codeunit "Temp Blob"): Integer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Security}$

The new public "E-Document Message API" codeunit exposes CreateMessage/SendMessage to any caller via InherentEntitlements = X with no authorization check of its own. Note this matches an established convention already used by ~68 codeunits across this app (including the sibling "E-Doc. Message Mgt." codeunit it wraps), so it is consistent with existing design rather than a defect newly introduced by this PR; flagged for awareness only.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.34.4

Comment thread src/Apps/FR/EDocument_FR/EReportingFR/test/src/FREInvoiceMessageTests.Codeunit.al Outdated
Comment thread src/Apps/W1/EDocument/App/src/Processing/Message/EDocMessageMgt.Codeunit.al Outdated
Comment thread src/Apps/W1/EDocument/App/src/Processing/Message/EDocMessageSendJob.Codeunit.al Outdated
DetailedCustLedgEntry: Record "Detailed Cust. Ledg. Entry";
FREInvoiceMessageMgt: Codeunit "FR E-Invoice Message Mgt.";
begin
Initialize();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AL test agent? :)

@djukicmilica Milica Đukić (djukicmilica) changed the title Add French collected and refused E-Document messages [FR E-Reporting] Add payment and invoice lifecycle messages Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Data\ Modeling}$

The updated "E-Document Message" model stores each payload through "Data Storage Entry No.", but the table still has no OnDelete cleanup for the referenced "E-Doc. Data Storage" row. "E-Document" deletion already removes child messages with DeleteAll(true), so message deletion now leaves orphaned blob records behind. Add an OnDelete trigger that deletes the related data-storage entry before the message row is removed.

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

tabledata "E-Doc. Data Storage" = imd,
tabledata "E-Document Integration Log" = imd,
tabledata "E-Document Message" = imd,
tabledata "E-Doc. Payment Occurrence" = imd,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Security}$

The core user role adds direct IMD rights on "E-Doc. Payment Occurrence" and "E-Doc. External Reference". Those records represent message-correlation and payment-occurrence state that the new message-management flow creates through codeunits, not through editable user surfaces. Uppercase write/delete grants allow direct tampering with that state instead of keeping it code-mediated with indirect permissions.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

CreateDetailedLedgerEntry() fabricates a Detailed Cust. Ledg. Entry with Init/Insert and a hand-picked entry number to simulate an unapplication. That bypasses the posting/application helpers, so these reversal tests can pass against a ledger state production code would never create and are brittle if the table gains new required fields or validation. Build the unapplication through the test libraries or the real application flow instead of inserting the ledger row directly.

Knowledge:

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Extensible = true;
Access = Public;
DefaultImplementation = IConsentManager = "Consent Manager Default Impl.";
DefaultImplementation = IConsentManager = "Consent Manager Default Impl.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Web\ Services}$

The new child-message transport is wired only to the default throwing implementation. In this PR, production code now queues French lifecycle messages for background send, but the only IMessageSender/IMessageResponseHandler implementations added are test mocks; no non-test service-integration enum extension maps these interfaces. As a result, real E-Document services will resolve to "E-Doc. Msg. Transport Default" and fail with "does not support sending/polling E-Document messages" instead of reaching the external service. Add production connector implementations before enabling QueueMessage/PollMessageResponse for this pipeline.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

value(133501; "Mock")
{
Implementation = IDocumentSender = "E-Doc. Integration Mock V2", IDocumentReceiver = "E-Doc. Integration Mock V2", IConsentManager = "E-Doc. Integration Mock V2";
Implementation = IDocumentSender = "E-Doc. Integration Mock V2", IDocumentReceiver = "E-Doc. Integration Mock V2", IConsentManager = "E-Doc. Integration Mock V2", IMessageResponseHandler = "E-Doc. Integration Mock V2";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Interfaces}$

The new child-message pipeline always dispatches sends through Interface IMessageSender, but the Service Integration test value Mock still maps only IMessageResponseHandler. In EDocMessageMgt.SendMessage, MessageSender := EDocumentService."Service Integration V2"; MessageSender.SendMessage(...) now requires an IMessageSender implementation, so a service configured with Service Integration::Mock will fall back to the enum default sender and raise the generic unsupported-transport error instead of exercising the mock connector. Add IMessageSender support for the mock value (either by extending E-Doc. Integration Mock V2 to implement IMessageSender and mapping it, or by mapping a dedicated message-sender codeunit) so send and poll use a consistent interface contract.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Comment on lines +222 to +224
AppliesToDocumentNo := AppliesToDocumentNoFieldRef.Value();
if (AppliesToDocumentNo = '') or not SalesInvoiceHeader.Get(AppliesToDocumentNo) then
exit;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Performance}$

AddInvoiceReferencedDocument loads the full Sales Invoice Header row even though it only reads "Document Date". This is the partial-record anti-pattern on a wide posted-document table; call SetLoadFields before Get so the credit-memo reference lookup does not transfer unused columns.

Suggested change
AppliesToDocumentNo := AppliesToDocumentNoFieldRef.Value();
if (AppliesToDocumentNo = '') or not SalesInvoiceHeader.Get(AppliesToDocumentNo) then
exit;
AppliesToDocumentNo := AppliesToDocumentNoFieldRef.Value();
SalesInvoiceHeader.SetLoadFields("Document Date");
if (AppliesToDocumentNo = '') or not SalesInvoiceHeader.Get(AppliesToDocumentNo) then
exit;

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

CII XML generation rescans SourceDocumentLines multiple times in one export path (billing mode detection, line total calculation, invoice discount calculation, VAT aggregation, and final line serialization). On larger invoices this turns one document build into several passes over the same line set; cache the derived line facts or consolidate these helpers into a shared pass before emitting XML.

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

local procedure InsertTaxElement(var SettlementElement: XmlElement; CalculatedAmount: Text; BasisAmount: Text; CategoryCode: Text; RateApplicablePercent: Text; VATEXCode: Text)
var
TradeTaxElement: XmlElement;
ExemptionReasonLbl: Label 'Exempt from VAT', Locked = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

InsertTaxElement adds ExemptionReasonLbl inside the procedure var block. The style guidance for Labels is to keep them in the codeunit's top-level var block; in this file the code already uses the object-scoped VATExemptionReasonLbl, so the new procedure-local declaration should be removed instead of introducing a duplicate local Label.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

if TryPollMessageResponse(EDocumentMessage."Entry No.") then
exit;

LastErrorText := GetLastErrorText();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Telemetry}$

This new background response-poll failure path also only persists Last Error and rethrows, without emitting structured telemetry first. As a result, asynchronous response failures are harder to correlate and query than other e-document failures. Emit a non-PII error telemetry event before Error(...), ideally through the same shared helper/pattern used for other e-document error telemetry.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

EDocument, "E-Document Message Type"::"FR Invoice Lifecycle", "E-Doc. Response Type"::Refused, TempBlob);
MessageSenderMock.SetReportSuccess(false);

asserterror EDocumentMessageAPI.SendMessage(MessageEntryNo);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Testing}$

MessageSenderMustReportSuccess uses asserterror EDocumentMessageAPI.SendMessage(MessageEntryNo); without any Assert.ExpectedError(...), so any unrelated failure in SendMessage would satisfy the test and hide whether the missing-success-status path was the error that actually occurred.

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

asserterror EDocumentMessageAPI.SendMessage(MessageEntryNo);
Assert.ExpectedError('could not be sent');
Assert.AreEqual(1, MessageSenderMock.GetSendCount(), 'The connector must be invoked before its missing success result is rejected.');

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ AppSource}$

This pageextension relies on a two-level namespace, but the added control is still named "Clearance Date" with no FR affix. Namespaces only replace the affix for objects the app owns; members added to another publisher's page still need the registered affix, so this control can fail AppSourceCop AS0011.

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

            field("FR Clearance Date"; Rec."Clearance Date")

Knowledge:

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Comment on lines +28 to +31
codeunit "E-Document Message API" = X,
codeunit "E-Doc. Message Send Job" = X,
codeunit "E-Doc. Message Send Runner" = X,
codeunit "E-Doc. Payment Occurrence Mgt." = X,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ AppSource}$

The assignable "E-Doc. Core - User" permission set still omits execute permission for codeunit "E-Doc. Message Response Job", even though the normal pending-response and Retry flows schedule that job through the E-Document Message API. Users assigned only the app's permission sets can therefore hit response polling without SUPER, which violates the AppSource requirement that setup and normal usage work without elevated permissions.

Suggested change
codeunit "E-Document Message API" = X,
codeunit "E-Doc. Message Send Job" = X,
codeunit "E-Doc. Message Send Runner" = X,
codeunit "E-Doc. Payment Occurrence Mgt." = X,
codeunit "E-Document Message API" = X,
codeunit "E-Doc. Message Send Job" = X,
codeunit "E-Doc. Message Send Runner" = X,
codeunit "E-Doc. Message Response Job" = X,
codeunit "E-Doc. Payment Occurrence Mgt." = X,

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

PreviousMessageType: Enum "FR E-Invoice Message Type";
HasPreviousMessage: Boolean;
begin
FREInvoiceMessage.SetRange("E-Document Entry No.", EDocumentEntryNo);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

ValidateLifecycleTransition filters FR E-Invoice Message by E-Document Entry No. and Type, then calls FindLast(), but the new table has no key whose leading fields match that lookup. Once collected/payment messages accumulate for a document, this transition check has to read through unrelated message rows to find the last lifecycle status; add a key for the lifecycle lookup (for example starting with E-Document Entry No. and Type, plus the field used to identify the latest row) and use it here.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

UnsupportedStatusErr: Label 'French invoice lifecycle status %1 is not supported.', Comment = '%1 = lifecycle status';
InvoiceMismatchErr: Label 'The lifecycle message invoice ID %1 does not match E-Document invoice %2.', Comment = '%1 = message invoice identifier, %2 = E-Document invoice identifier';
InvalidLifecycleTransitionErr: Label 'French invoice lifecycle status cannot change from %1 to %2.', Comment = '%1 = previous lifecycle status, %2 = new lifecycle status';
NoPreviousStatusTok: Label 'no previous status';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

NoPreviousStatusTok is natural-language fallback text that is inserted into InvalidLifecycleTransitionErr, so it behaves like translatable UI text rather than a wire-level token. Rename it to a UI-text suffix such as NoPreviousStatusLbl to match its usage and avoid the misleading Tok naming convention.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

SchemeNode: XmlNode;
XmlNode: XmlNode;
PartyPath: Text;
PartyPathTok: Label '//*[local-name()="ExchangedDocument"]/*[local-name()="%1"]', Locked = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Style}$

PartyPathTok is declared in the local var block of AssertTradeParty(). BCQuality recommends moving Labels to the object's top-level var section because procedure-scoped Labels are fragile in XLIFF extraction and translation review.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

"E-Doc. Response Type"::None, TempBlob);

// [WHEN] The message is retried
asserterror EDocumentMessageAPI.RetryMessage(MessageEntryNo);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

The new negative tests in this file (RetryMessageRejectsMessageWithoutError, RetryMessageRejectsIncomingMessage, and PollMessageResponseRejectsUnsupportedConnector) stop at Assert.ExpectedError(...) after asserterror. That is better than a bare asserterror, but it still does not pin the error code, so a different dialog/runtime error with a matching substring can keep these tests green.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

GrossAmount := -(VATEntry."Source Currency VAT Base" + VATEntry."Source Currency VAT Amount");
VATEntry."Source Currency Code" = '':
GrossAmount := -(VATEntry.Base + VATEntry.Amount)
else

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Error\ Handling}$

In GetVATEntryGrossAmount, the case true of statement's else branch contains three statements (ErrorType, Message, Error) with no enclosing begin...end. AL only permits a single statement after a case branch label without a compound block, so this does not compile. The underlying impact is build-breaking (would otherwise be blocker/major) but is capped at minor per agent-finding policy since it has no dedicated knowledge-file citation. This is the branch responsible for raising the Internal ErrorInfo when the VAT entry currency doesn't match the lifecycle currency, so the whole error path is broken. Every other multi-statement branch in this same PR wraps its body in begin...end.

Recommendation:

  • wrap the three statements in begin...end;.

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

            else begin
                VATEntryCurrencyErrorInfo.ErrorType(ErrorType::Internal);
                VATEntryCurrencyErrorInfo.Message(StrSubstNo(VATEntryCurrencyErr, VATEntry."Entry No.", CurrencyCode));
                Error(VATEntryCurrencyErrorInfo);
            end;

Agent judgement — not directly backed by a BCQuality knowledge article.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

IsInitialized: Boolean;

[Test]
procedure QueueMessageSchedulesBackgroundSend()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Testing}$

QueueMessageSchedulesBackgroundSend calls EDocumentMessageAPI.QueueMessage, which reaches EDocMessageMgt.QueueMessage and its unconditional Commit() call (EDocMessageMgt.Codeunit.al line 216), while the test method runs under the default AutoRollback transaction model instead of [TransactionModel(TransactionModel::AutoCommit)]. A Commit() inside an AutoRollback test raises a runtime error, so this test fails for infrastructure reasons rather than verifying the intended behavior.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

end;

[Test]
procedure RetryMessageRequeuesExistingMessage()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Testing}$

RetryMessageRequeuesExistingMessage and RetryMessageReschedulesFailedResponsePoll call EDocumentMessageAPI.RetryMessage, which reaches EDocMessageMgt.RetryMessage and its unconditional Commit() call (EDocMessageMgt.Codeunit.al line 236), while running under the default AutoRollback model instead of declaring [TransactionModel(TransactionModel::AutoCommit)]. Commit() inside an AutoRollback test raises a runtime error.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

begin
RequireNodeText(XmlDoc, '//*[local-name()="BusinessProcessSpecifiedDocumentContextParameter"]/*[local-name()="ID"]', RegulatedBusinessProcessTok);
if XmlDoc.SelectSingleNode('//*[local-name()="SenderTradeParty" or local-name()="RecipientTradeParty"]', XmlNode) then
Error(UnexpectedProfileNodeErr, CDVInvoiceProfileTok);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Error\ Handling}$

FR E-Invoice Profile Validator validates XML that this extension has just built itself, so these failures are internal invariant breaches, not user-correctable validation errors. Raising raw XPath/profile diagnostics with plain Error(...) exposes developer-only detail directly to users instead of classifying it as internal telemetry detail via ErrorInfo.ErrorType := ErrorType::Internal.

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Comment on lines +284 to +292
EDocMessage.LockTable();
EDocMessage.SetRange(Service, ServiceCode);
EDocMessage.SetRange("External Message ID", ExternalMessageID);
if EDocMessage.FindFirst() then
exit(EDocMessage."Entry No.");

EDocExternalReference.SetRange(Service, ServiceCode);
EDocExternalReference.SetRange("External Document ID", ExternalDocumentID);
if not EDocExternalReference.FindFirst() then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Performance}$

CreateIncomingMessage performs the duplicate-message and external-document lookups under LockTable without selecting the existing (Service, "External Message ID") and (Service, "External Document ID") keys first, so both FindFirst() calls run on the primary-key order and widen the locked read path.

Suggested change
EDocMessage.LockTable();
EDocMessage.SetRange(Service, ServiceCode);
EDocMessage.SetRange("External Message ID", ExternalMessageID);
if EDocMessage.FindFirst() then
exit(EDocMessage."Entry No.");
EDocExternalReference.SetRange(Service, ServiceCode);
EDocExternalReference.SetRange("External Document ID", ExternalDocumentID);
if not EDocExternalReference.FindFirst() then
EDocMessage.LockTable();
EDocMessage.SetCurrentKey(Service, "External Message ID");
EDocMessage.SetRange(Service, ServiceCode);
EDocMessage.SetRange("External Message ID", ExternalMessageID);
if EDocMessage.FindFirst() then
exit(EDocMessage."Entry No.");
EDocExternalReference.SetCurrentKey(Service, "External Document ID");
EDocExternalReference.SetRange(Service, ServiceCode);
EDocExternalReference.SetRange("External Document ID", ExternalDocumentID);
if not EDocExternalReference.FindFirst() then

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Comment on lines +84 to +86
AppliedOccurrence.SetRange("Source Occurrence ID", OldDetailedCustLedgEntry.SystemId);
AppliedOccurrence.SetRange(Type, AppliedOccurrence.Type::Applied);
if not AppliedOccurrence.FindSet() then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Performance}$

ProcessUnapplication loops over "E-Doc. Payment Occurrence" but only reads "E-Document Entry No.", Amount, "Currency Code", and "Entry No." from each row; without SetLoadFields, every reversal loads the whole record on this posting path.

Suggested change
AppliedOccurrence.SetRange("Source Occurrence ID", OldDetailedCustLedgEntry.SystemId);
AppliedOccurrence.SetRange(Type, AppliedOccurrence.Type::Applied);
if not AppliedOccurrence.FindSet() then
AppliedOccurrence.SetRange("Source Occurrence ID", OldDetailedCustLedgEntry.SystemId);
AppliedOccurrence.SetRange(Type, AppliedOccurrence.Type::Applied);
AppliedOccurrence.SetLoadFields("E-Document Entry No.", Amount, "Currency Code", "Entry No.");
if not AppliedOccurrence.FindSet() then

Knowledge:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Ownership: Needs Review Ownership is Other, low confidence, or needs manual correction Team: Other GitHub request for other area than SCM, Finance or Integration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants