[Master]-Take Payment Discounts setting does not work as expected in the Suggest Vendor Payments EB routine from the EB Payment Journal in the Belgian version.#9092
Conversation
| DueDate := WorkDate(); | ||
| if PmtDiscDueDate = 0D then | ||
| PmtDiscDueDate := WorkDate(); | ||
| if (IncPmtDiscount) and (PmtDiscDueDate = 0D) then |
There was a problem hiding this comment.
Report 2000019 declares SaveValues = true, so IncPmtDiscount and PmtDiscDueDate are meant to persist across invocations.
The new OnOpenPage logic is if (IncPmtDiscount) and (PmtDiscDueDate = 0D) then PmtDiscDueDate := WorkDate() else PmtDiscDueDate := 0D;. When the checkbox was previously saved as true together with a non-zero discount date (the normal steady state after a first run), reopening the request page evaluates the second condition to false (PmtDiscDueDate <> 0D) and falls into the else branch, which wipes the persisted date back to 0D on every subsequent open. This defeats SaveValues for this field and also risks tripping the existing OnPreDataItem check if IncPmtDiscount and (PmtDiscDueDate < Today) then Confirm(...)/Error(...) on every run, since 0D < Today is always true. Actual impact is functionally significant (silent loss of a persisted user setting plus a spurious confirm/error each run), which would normally warrant major severity, but this is flagged as an unbacked agent finding and capped at minor per BCQuality's agent-finding rules; consider promoting this to a knowledge-backed rule. Recommended
Recommendation:
- only default the date when it is currently blank, independent of clearing on uncheck, e.g. keep
if PmtDiscDueDate = 0D then PmtDiscDueDate := WorkDate();in OnOpenPage (mirroring DueDate/PostingDate) and confine the clear-on-uncheck behavior to the checkbox's own OnValidate trigger.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Copilot PR ReviewIteration 11 · Outcome: completed
Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
|
|
||
| trigger OnValidate() | ||
| begin | ||
| if (IncPmtDiscount) and (PmtDiscDueDate = 0D) then |
There was a problem hiding this comment.
In the IncPmtDiscount field's OnValidate trigger, the new logic 'if (IncPmtDiscount) and (PmtDiscDueDate = 0D) then PmtDiscDueDate := WorkDate() else PmtDiscDueDate := 0D' resets PmtDiscDueDate to 0D whenever IncPmtDiscount is true but PmtDiscDueDate already holds a non-zero value (the else branch fires because the AND condition requires PmtDiscDueDate = 0D).
The prior code only ever initialized an empty date and never clobbered an already-set one. Recommend nesting the checks so a non-zero date is preserved when the checkbox is enabled, and only cleared when it is disabled: guard the WorkDate() default inside 'if IncPmtDiscount then' and only assign 0D in the 'else' for IncPmtDiscount = false.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if IncPmtDiscount then begin
if PmtDiscDueDate = 0D then
PmtDiscDueDate := WorkDate();
end else
PmtDiscDueDate := 0D;👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
…t-Discounts-not-work-in-EB-Payment-Journal
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR makes the BE SuggestionsS1 - Keep discount date when enabled S2 - Test discount-only selection and amount Risk assessment and necessityRisk: This is BE payment proposal logic, so the risk is financial: a wrong branch can suggest invoices at the wrong amount or with the wrong payment discount. No new BaseApp event publisher is involved, but the relevant BE unit-test check is currently failing, so the added coverage has not proved the change yet. Necessity: The work item is a Bug with clear repro steps. Without a fix, the EB routine can use
|
|
Please review S1. |
…t-Discounts-not-work-in-EB-Payment-Journal
| PaymentJournalLine.FindFirst(); | ||
|
|
||
| // [THEN] Verify that the "Pmt. Discount Date" is Blank, Pmt Discount is not taken, in Payment Journal Line. | ||
| Assert.AreEqual(0D, PaymentJournalLine."Pmt. Discount Date", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Discount Date")); |
There was a problem hiding this comment.
WrongPmtDiscErr declares Comment = '%1 = Field Caption', but the two call sites pass the raw field value (a Date/Decimal) instead of FieldCaption(...).
On assertion failure the message reads 'Wrong 01/20/24 on the payment journal line.' instead of naming the field, contradicting the label's own documented placeholder contract and defeating its purpose for debugging.
Recommendation:
- substitute PaymentJournalLine.FieldCaption("Pmt. Discount Date") / FieldCaption("Pmt. Disc. Possible") instead of the value.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
Assert.AreEqual(0D, PaymentJournalLine."Pmt. Discount Date", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine.FieldCaption("Pmt. Discount Date")));
Assert.AreEqual(0, PaymentJournalLine."Pmt. Disc. Possible", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine.FieldCaption("Pmt. Disc. Possible")));Knowledge:
- microsoft/knowledge/style/label-comment-explains-placeholders.md
- microsoft/knowledge/style/fieldcaption-not-fieldname-in-user-messages.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| LettersTxt: Label 'ABCDEFGHIJKLMNOPQRSTUVWXYZ', Locked = true; | ||
| WrongNumberOfLinesErr: Label 'Wrong number of Payment Journal Lines.'; | ||
| WrongStatusOfLineErr: Label 'Wrong status of Payment Journal Line.'; | ||
| WrongPmtDiscErr: Label 'Wrong %1 on the payment journal line.', Comment = '%1 = Field Caption'; |
There was a problem hiding this comment.
WrongPmtDiscErr is declared with Comment = '%1 = Field Caption', but none of its 4 call sites pass a FieldCaption — they all pass the field's actual value instead (e.g.
StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Discount Date")). On assertion failure this renders as a nonsensical message like 'Wrong 07/06/26 on the payment journal line.' instead of naming the field that was wrong. Pass PaymentJournalLine.FieldCaption("Pmt. Discount Date") / FieldCaption("Pmt. Disc. Possible") to match the documented Comment.
Knowledge:
- microsoft/knowledge/style/label-comment-explains-placeholders.md
- microsoft/knowledge/style/fieldcaption-not-fieldname-in-user-messages.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Agentic PR Review - Round 2Recommendation: Request ChangesWhat this PR doesSince round 1, the report logic was changed so Status of previous suggestions
New observations (commits since round 1)None - the new commit only addresses S1 and does not introduce a new issue. Risk assessment and necessityRisk: This is BE payment proposal logic. A wrong branch can suggest invoices at the wrong amount or with the wrong payment discount. The round-2 code change is narrow, but the remaining test gap is still in financial logic. Necessity: The bug is valid and the fix is needed. Without it, invoices eligible for payment discount can be suggested even when
|
|
|
||
| // [THEN] Verify that the "Pmt. Discount Date" and Pmt Discount is taken, in Payment Journal Line. | ||
| Assert.AreEqual(WorkDate(), PaymentJournalLine."Pmt. Discount Date", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Discount Date")); | ||
| Assert.AreEqual(PaymentJournalLine."Pmt. Disc. Possible", Abs(VendorLedgerEntry."Remaining Pmt. Disc. Possible"), StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Disc. Possible")); |
There was a problem hiding this comment.
Assert.AreEqual(PaymentJournalLine."Pmt.
Disc. Possible", Abs(VendorLedgerEntry."Remaining Pmt. Disc. Possible"), ...) at line 705 places the actual/recorded value first and the expected/computed value second, reversed from the (Expected, Actual) convention used by the other three Assert.AreEqual calls added in this same test (lines 669, 670, 704). Assert.AreEqual is symmetric for pass/fail, but the standard Assert library renders failures as 'Expected: Actual: ', so a failure here would report the recorded line value as 'Expected' and the freshly computed value as 'Actual' — backwards during triage. Recommend swapping the two arguments to match the surrounding convention.
| Assert.AreEqual(PaymentJournalLine."Pmt. Disc. Possible", Abs(VendorLedgerEntry."Remaining Pmt. Disc. Possible"), StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Disc. Possible")); | |
| Assert.AreEqual(Abs(VendorLedgerEntry."Remaining Pmt. Disc. Possible"), PaymentJournalLine."Pmt. Disc. Possible", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Disc. Possible")); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
…t-Discounts-not-work-in-EB-Payment-Journal
…in-EB-Payment-Journal' of https://github.com/neeleshsinghal/BCApps into bugs/Bug-641379-Master-Take-Payment-Discounts-not-work-in-EB-Payment-Journal
There was a problem hiding this comment.
Pull request overview
This PR fixes Belgian “Suggest Vendor Payments EB” request page behavior so that disabling Take Payment Discounts truly disables the payment-discount path (by clearing/locking the Payment Discount Date), and adds BE regression tests to validate the corrected behavior.
Changes:
- Updated the request page so Payment Discount Date is editable only when Take Payment Discounts is enabled, and is cleared when disabled.
- Adjusted request page initialization/validation so
PmtDiscDueDateis only defaulted when payment discounts are enabled. - Added BE test scenarios covering “take discount off/on” and the “invoice not yet due but discount window applies” selection logic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/Layers/BE/BaseApp/Local/BankMgt/Payment/SuggestVendorPaymentsEB.Report.al | Makes Payment Discount Date dependent on Take Payment Discounts (editable + cleared/defaulted consistently). |
| src/Layers/BE/Tests/Local/ERMPaymentJournal.Codeunit.al | Adds regression tests and request page handlers to validate discount-selection behavior for BE payment journals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Assert.AreEqual(0D, PaymentJournalLine."Pmt. Discount Date", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Discount Date")); | ||
| Assert.AreEqual(0, PaymentJournalLine."Pmt. Disc. Possible", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Disc. Possible")); |
| Assert.AreEqual(WorkDate(), PaymentJournalLine."Pmt. Discount Date", StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Discount Date")); | ||
| Assert.AreEqual(PaymentJournalLine."Pmt. Disc. Possible", Abs(VendorLedgerEntry."Remaining Pmt. Disc. Possible"), StrSubstNo(WrongPmtDiscErr, PaymentJournalLine."Pmt. Disc. Possible")); |
| [Test] | ||
| [HandlerFunctions('SuggestVendorPaymentsTakeDiscRPH')] | ||
| procedure SuggestVendorPaymentsWithoutTakingPaymentDiscount() |
| [Test] | ||
| [HandlerFunctions('SuggestVendorPaymentsTakeDiscRPH')] | ||
| procedure SuggestVendorPaymentsTakingPaymentDiscount() |
| [Test] | ||
| [HandlerFunctions('SuggestVendorPaymentsPmtDiscDateRPH')] | ||
| procedure SuggestVendorPaymentsSelectsDiscountOnlyEntryWithDiscountedAmount() |
| [RequestPageHandler] | ||
| procedure SuggestVendorPaymentsTakeDiscRPH(var SuggestVendorPaymentsEB: TestRequestPage "Suggest Vendor Payments EB") | ||
| begin | ||
| SuggestVendorPaymentsEB.DueDate.SetValue(CalcDate('<3M>', WorkDate())); | ||
| SuggestVendorPaymentsEB.IncPmtDiscount.SetValue(LibraryVariableStorage.DequeueBoolean()); | ||
| SuggestVendorPaymentsEB.OK().Invoke(); | ||
| end; |
| [RequestPageHandler] | ||
| procedure SuggestVendorPaymentsPmtDiscDateRPH(var SuggestVendorPaymentsEB: TestRequestPage "Suggest Vendor Payments EB") | ||
| begin | ||
| SuggestVendorPaymentsEB.DueDate.SetValue(LibraryVariableStorage.DequeueDate()); | ||
| SuggestVendorPaymentsEB.IncPmtDiscount.SetValue(LibraryVariableStorage.DequeueBoolean()); | ||
| SuggestVendorPaymentsEB.OK().Invoke(); | ||
| end; |
…t-Discounts-not-work-in-EB-Payment-Journal
Agentic PR Review - Round 3Recommendation: Request ChangesWhat this PR doesSince round 2, the only author-owned non-merge commit adds automation in the BE test app. The new test creates a vendor with payment-discount terms, posts a purchase invoice, runs Status of previous suggestions
New observations (commits since round 2)None - the remaining issue is the carry-forward S2 test gap, not a new S3 issue. Risk assessment and necessityRisk: This is BE vendor payment suggestion logic. The product-code change is narrow, but the scenario touches payment discounts, dates, and suggested payment amounts, so a weak regression test can let a financial regression return. Necessity: The bug is valid and the fix is needed. Without it, invoices eligible only through payment discount can be suggested even when
|
…in-EB-Payment-Journal' of https://github.com/neeleshsinghal/BCApps into bugs/Bug-641379-Master-Take-Payment-Discounts-not-work-in-EB-Payment-Journal
Agentic PR Review - Round 4Recommendation: AcceptWhat this PR doesSince round 3, the only author-owned non-merge commit updates the BE test app. The discount-only test still verifies that no payment line is suggested when Status of previous suggestions
New observations (commits since round 3)None - the changes only strengthen the BE regression test and no new issue was found. Risk assessment and necessityRisk: The round-4 change is test-only. The covered behavior is still financially sensitive because it controls BE vendor payment suggestions, payment discount dates, and suggested payment amounts. Necessity: The bug is valid and the fix is needed. Without it, invoices eligible through payment discount can be suggested when
|
|
Very good, thank you! |
@https://github.com/alexei-dobriansky @ Could you also please approve this PR? |
…t-Discounts-not-work-in-EB-Payment-Journal
…t-Discounts-not-work-in-EB-Payment-Journal
Workitem:- Bug 641379: [master] [All-e]Take Payment Discounts setting does not work as expected in the Suggest Vendor Payments EB routine from the EB Payment Journal in the Belgian version.
Fixes AB#641379