Skip to content

[Main]Customer Direct Debit Payment Slip – SEPA File Generation Fails and Credit Memo Is Processed Incorrectly When Invoice and Credit Memo Exist for the Same Customer in the French version. - #10895

Open
Shikhverma wants to merge 8 commits into
mainfrom
bugs/Bug-641369-main-Cust-Pmt-Slip-SEPA-File-Fails-Cr-Memo-Process-Incorrect
Open

[Main]Customer Direct Debit Payment Slip – SEPA File Generation Fails and Credit Memo Is Processed Incorrectly When Invoice and Credit Memo Exist for the Same Customer in the French version.#10895
Shikhverma wants to merge 8 commits into
mainfrom
bugs/Bug-641369-main-Cust-Pmt-Slip-SEPA-File-Fails-Cr-Memo-Process-Incorrect

Conversation

@Shikhverma

@Shikhverma Shikhverma commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Fixes: AB#644993

Issue

When a customer had an open invoice and an unapplied credit memo, the Customer Payment Slip (FR) handled them incorrectly:

SEPA Direct Debit export failed with errors such as "You cannot export a SEPA customer payment that is applied to multiple documents" and "unapplied lines not allowed" whenever a credit memo was collected alongside an invoice for the same customer.
Posting produced the wrong result — the credit memo was posted as a separate Refund entry instead of being netted against the invoice, so the customer was debited the gross invoice amount rather than the net balance, and entries were left open.

Root cause

Posting side (Payment Management FR): settlement amounts were always taken as Abs(...) regardless of the applied document type, so a credit memo was never subtracted. There was no logic to net a credit memo into the invoice it shared collection with, so each document posted on its own line and the credit memo became a refund.
SEPA export side (PaymentMgt Subscribers FR): CheckPaymentLine required every collected line to resolve to a single invoice ledger entry. Credit-memo lines, and invoice+credit-memo combinations under one Applies-to ID, tripped the "multiple documents" / "unapplied lines" validations. The transfer amount also used the raw Credit Amount without deducting the credit memo, so the exported amount would have been wrong even if validation passed.

Solutions

Added TryGetCustomerNettingContext on the Payment Line FR table as the single source of truth that locates the shared invoice line and aggregates the customer's credit-memo amount — reused by both the posting and export paths.
Posting (Payment Management FR):
GetSettlementAmount now signs the amount by document type (credit memo → negative).
NormalizeAccountLevelSign corrects the account-level debit/credit sign.
NetCustomerSettlement redirects the entry onto the shared invoice's Applies-to ID so invoice and credit memo settle as one net payment; a standalone credit memo (no shared invoice) falls back to a Refund.
ApplyCreditMemoToSharedInvoice applies the collected credit-memo/secondary document to the shared invoice application on the customer ledger.
SEPA export (PaymentMgt Subscribers FR):
Credit-memo lines are excluded from the file and their amount is netted into the shared invoice line via GetNetTransferAmount.
CheckPaymentLine now filters to invoice ledger entries and no longer errors on the invoice + credit-memo combination.
Tests: added coverage for the net posting flow (ERM Payment Management) and for SEPA export of an invoice + credit memo for the same customer (SEPA.02 DD Functional Test).

@Shikhverma Shikhverma added the Team: Finance GitHub request for Finance area label Sep 1, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 1, 2026
@Shikhverma
Shikhverma marked this pull request as ready for review September 1, 2026 13:48
@Shikhverma
Shikhverma requested a review from a team September 1, 2026 13:48
@Shikhverma
Shikhverma requested a review from a team as a code owner September 1, 2026 13:48
Comment thread src/Apps/FR/PaymentManagementFR/app/src/Tables/PaymentLineFR.Table.al Outdated
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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

The new posting test says it proves both the bank debit and the receivables credit, but VerifyGenLedgerEntry only filters rows where "Debit Amount" is non-zero and sums those debits. A regression that posts the right debit total while misposting or omitting the credit-side receivables entry would still pass. Add an assertion for the corresponding credit entry and, ideally, the expected receivables account.

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This change tries to let French customer payment slips net a credit memo against an invoice for the same customer, both when posting the slip and when creating the SEPA direct debit file. The main path is covered: the export now finds one invoice entry and subtracts a credit memo amount, and the posting path moves the credit memo application onto the shared invoice application. The remaining problem is that the new netting checks are wider than the reported scenario. They can skip or combine credit memo lines that are not proven to belong to the same invoice netting group.

Problem-solution fit

Fit: Partial

The reported scenario is important and the change addresses the simple invoice-plus-credit-memo case. It does not yet preserve the existing validation for standalone or unrelated credit memo lines, so the solution is not safe enough for this payment-file and posting path.

Suggestions

S1 (🔴 High): Credit memo lines can be skipped silently
Do not exit for every credit memo line here. First prove that the line is part of the invoice-and-credit-memo netting case; otherwise keep the payment file error. A standalone or unrelated credit memo can now be omitted from the SEPA file without an error.

S2 (🔴 High): Netting scans unrelated customer lines
TryGetCustomerNettingContext only filters by payment slip and customer. It can take a credit memo from another line for the same customer and subtract it from the first invoice line with an Applies-to ID. Limit the search to the same application group so unrelated documents are not netted together.

S3 (🟠 Moderate): Posting test misses the credit entry
The new posting test checks the bank ledger and only debit-side G/L entries. Add an assertion for the receivables credit entry and the expected account. That will catch a sign regression in this money-posting path.

Risk assessment and necessity

Risk: This is a high-risk payment and posting change. It affects SEPA direct debit export, customer ledger application, and G/L signs in PaymentManagementFR.Codeunit.al, PaymentMgtSubscribersFR.Codeunit.al, and PaymentLineFR.Table.al. The subscriber still hooks the existing temporary-entry creation event, but the new handled path now owns validation, amount calculation, and skipped lines.

Necessity: The change is needed because collecting the gross invoice while leaving the credit memo open is wrong. The scope is also in the right area, but it must keep the old error behavior unless a credit memo is safely netted into the same invoice collection.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=10895 round=1 by=alexei-dobriansky at=2026-09-01T19:22:51Z lastSha=2e107bb19ff7a0176b8afb2608d4341286fad4d6 reviewKey=a46960ec78ded19b8934a6a38d73d99074d506dd1f831e67e62f3e6842b47239 suggestions=S1@c0914e64,S2@5799e528,S3@413fb67d

…Bug-641369-main-Cust-Pmt-Slip-SEPA-File-Fails-Cr-Memo-Process-Incorrect
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept

What this PR does

The new commit tightens the customer netting check so credit memos are only skipped when they share the same Applies-to ID with an invoice. It also adds a receivables credit assertion to the posting test. With these changes, the reported invoice-plus-credit-memo collection path is narrower, and the previous sign-test gap is covered.

Status of previous suggestions
ID Title Status Author response
S1 Credit memo lines can be skipped silently Addressed Fixed in 4d28fdc: credit memo lines now fall back to the existing validation unless they are part of the same netting group.
S2 Netting scans unrelated customer lines Addressed Fixed in 4d28fdc: the netting lookup now requires and filters on the same Applies-to ID.
S3 Posting test misses the credit entry Addressed Fixed in 4d28fdc: the posting test now checks the receivables G/L credit entry and amount.
New observations (commits since round 1)

None - changes only addressed prior suggestions.

Risk assessment and necessity

Risk: This remains a sensitive payment and posting area because it affects SEPA direct debit export, customer ledger application, and G/L signs. The new commit reduces the main regression risk by keeping unrelated credit memo lines on the old error path and by checking the receivables credit entry in the posting test.

Necessity: The change is needed because collecting the gross invoice while leaving the credit memo open is wrong. The scope now matches the reported invoice-and-credit-memo netting scenario more closely.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=10895 round=2 by=alexei-dobriansky at=2026-09-02T13:18:18Z lastSha=1b00d7d051da4df15db6f312f0cb213de94d9b4d reviewKey=c44cc978221edb449076f456385ffc555c8af3dff02f63131d75d210d79fbf48 suggestions=S1@c0914e64:addressed,S2@5799e528:addressed,S3@413fb67d:addressed parentRound=1

@Shikhverma
Shikhverma marked this pull request as draft September 2, 2026 13:30
@Shikhverma Shikhverma closed this Sep 7, 2026
@Shikhverma Shikhverma reopened this Sep 7, 2026
…Bug-641369-main-Cust-Pmt-Slip-SEPA-File-Fails-Cr-Memo-Process-Incorrect
@Shikhverma
Shikhverma marked this pull request as ready for review September 8, 2026 09:51
CreditMemoAmount: Decimal;
begin
if InvPostingBuffer[1]."Account Type" <> InvPostingBuffer[1]."Account Type"::Customer 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{🟡\ Medium\ Severity\ —\ Agent}$

NetCustomerSettlement, TryGetCustomerNettingContext, GetSettlementAmount and ApplyCreditMemoToSharedInvoice implement a non-obvious cross-document netting algorithm (matching a credit memo against a shared invoice line, flipping sign/document type to Refund when no invoice is found, and re-pointing the ledger entry's Applies-to ID) with no explanatory comment. Because the logic silently changes posted amounts and document types based on implicit ordering assumptions (first Invoice line found via FindSet order), a short comment describing the netting contract and its ordering assumption would materially reduce the risk of future regressions during maintenance.

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

begin
DocumentCustLedgEntry.SetRange("Customer No.", PaymentLine."Account No.");
DocumentCustLedgEntry.SetRange("Document Type", DocumentType);
DocumentCustLedgEntry.SetRange("Document No.", DocumentNo);

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\ —\ Agent}$

ApplyCreditMemoToSharedInvoice (PaymentManagementFR.Codeunit.al) writes "Applies-to ID" directly onto a Cust. Ledger Entry and calls Modify(), bypassing the standard customer-application routines (e.g. CustEntrySetApplID / apply-and-post helpers) that also maintain detail ledger entries, currency application, and open-status bookkeeping consistently. Doing this ad hoc inside invoice-posting-buffer generation risks leaving the ledger entry's applies-to state inconsistent with the rest of the application machinery if another line targets the same invoice concurrently in the same posting run, since the field is overwritten rather than merged/validated through the normal application API. Recommend routing this through the standard apply-customer-entries logic (or at least validating no conflicting Applies-to ID is being clobbered) instead of a raw field write.

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

end;

#if not CLEAN28
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Payment Management Feature FR", OnAfterCheckFeatureEnabled, '', false, false)]

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\ —\ Events}$

The new subscriber in "Library - Localization FR" is a default static subscriber that forces "Payment Management Feature FR" on for every event raise in every test session. That behavior is scoped test setup, so keeping it static leaks the override across unrelated tests with no way to switch it off; move it behind a manual subscriber and bind it only for the tests that need the feature enabled.

Knowledge:

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

end;
end;

internal procedure TryGetCustomerNettingContext(var SharedInvoiceLine: Record "Payment Line FR"; var CreditMemoAmount: Decimal): Boolean

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

TryGetCustomerNettingContext opens a new Payment Line FR FindSet every time the caller needs netting data. The new posting and SEPA-export paths call this helper from per-line loops, so summarized slips rescan the same customer lines repeatedly instead of computing the netting context once per header/customer and reusing it.

Knowledge:

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

end;

[Test]
[HandlerFunctions('PaymentClassListModalPageHandler,SuggestCustomerPaymentsSummarizedRequestPageHandler,ConfirmHandlerTrue')]

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

The new test lists PaymentClassListModalPageHandler in [HandlerFunctions(...)], but its changed path creates a header, opens an existing payment slip, runs Suggest Customer Payments, and posts the slip directly; it never opens Payment Class List. Nonoptional listed handlers must execute, so this can fail on handler wiring instead of verifying the payment-netting behavior.

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

[HandlerFunctions('SuggestCustomerPaymentsSummarizedRequestPageHandler,ConfirmHandlerTrue')]

Knowledge:

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

PaymentClass.Get(SetupForPaymentSlipPost(PaymentStepLedger."Detail Level"::Account, PaymentClass.Suggestions::Customer));
CreatePaymentHeader(PaymentHeader);
Commit(); // Required for execute report.
SuggestCustomerPaymentLinesSummarized(CustomerNo, SummarizePer::" ", PaymentHeader."No.");

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 scenario in PostPaymentSlipNetsCreditMemoAgainstInvoice says it verifies "Summarize per Customer" and a single net payment line, but the test actually enqueues SummarizePer::" " and then asserts two payment lines. That means it is not covering the summarized path it claims; either drive SummarizePer::Customer and assert one line, or rename and reword the test to match the non-summarized flow.

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 3

Recommendation: Request Changes

What this PR does

The new commit changes the netting lookup so it can handle separate invoice and credit-memo payment lines for the same customer. That fixes the simple non-summarized case, but it removes the grouping guard that made the previous round safe. The current code again treats any credit memo for the customer on the slip as part of the first invoice netting context, so the fix is too broad for this payment-file and posting path.

Status of previous suggestions
ID Title Status Author response
S1 Credit memo lines can be skipped silently Not addressed No reply; the current code again skips a credit-memo line when another invoice line for the same customer exists.
S2 Netting scans unrelated customer lines Not addressed No reply; the Applies-to ID guard and filter were removed, so the lookup again scans all customer lines on the slip.
S3 Posting test misses the credit entry Addressed The posting test still checks the receivables G/L credit entry and amount.
New observations (commits since round 2)

S1 (🔴 High): Credit memo lines can be skipped silently
Do not skip every credit memo line just because another invoice line exists for the same customer. First prove that the credit memo belongs to the invoice netting group; otherwise keep the existing payment file error. An unrelated credit memo can now be omitted from the SEPA file without a clear error.

S2 (🔴 High): Netting scans unrelated customer lines
TryGetCustomerNettingContext now scans all payment lines for the same customer and sums every credit memo. With several invoices or credit memos for that customer, posting and export can subtract or apply a credit memo to the wrong invoice line. Use a safe grouping key, or reject combinations that cannot be netted safely.

Risk assessment and necessity

Risk: This is still a high-risk payment and posting change. It affects SEPA direct debit amounts, customer ledger application, and receivables G/L signs in PaymentManagementFR.Codeunit.al, PaymentMgtSubscribersFR.Codeunit.al, and PaymentLineFR.Table.al. The newest change widens the netting set again, so a customer with more than the simple one-invoice/one-credit-memo case can be collected or applied incorrectly.

Necessity: The fix is needed because collecting the gross invoice and leaving the credit memo open is wrong. The scope is right, but the implementation must keep unrelated documents on the old validation path unless they are safely tied to the same netting group.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=10895 round=3 by=alexei-dobriansky at=2026-09-08T14:58:00Z lastSha=95c0393311d4458e5dc4137ff03bd0a97d6995a0 reviewKey=80cf9ee34e4e2d51b8a95577aa08be8047e930f9a443e8c5053ceaddd73579b4 suggestions=S1@c0914e64:notaddressed,S2@5799e528:notaddressed,S3@413fb67d:addressed parentRound=2

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

Labels

Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants