Skip to content

[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

Open
neeleshsinghal wants to merge 2 commits into
microsoft:mainfrom
neeleshsinghal:bugs/Bug-641379-Master-Take-Payment-Discounts-not-work-in-EB-Payment-Journal
Open

[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
neeleshsinghal wants to merge 2 commits into
microsoft:mainfrom
neeleshsinghal:bugs/Bug-641379-Master-Take-Payment-Discounts-not-work-in-EB-Payment-Journal

Conversation

@neeleshsinghal

@neeleshsinghal neeleshsinghal commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot added From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item labels Jul 4, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 4, 2026
@neeleshsinghal neeleshsinghal added the Finance GitHub request for Finance area label Jul 4, 2026
@neeleshsinghal neeleshsinghal marked this pull request as ready for review July 4, 2026 11:41
@neeleshsinghal neeleshsinghal requested a review from a team July 4, 2026 11:41
DueDate := WorkDate();
if PmtDiscDueDate = 0D then
PmtDiscDueDate := WorkDate();
if (IncPmtDiscount) and (PmtDiscDueDate = 0D) 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\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

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

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 3 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

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

Totals: 0 knowledge-backed · 1 agent findings.

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

  • layer-disabled (knowledge) : 13 file(s)

Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.


trigger OnValidate()
begin
if (IncPmtDiscount) and (PmtDiscDueDate = 0D) 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\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

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

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR makes the BE Suggest Vendor Payments EB request page clear Payment Discount Date when Take Payment Discounts is off, and adds regression tests for both toggle values. The direction matches the W1 intent: future invoices should only be selected for payment discount when the payment-discount option is enabled, and the discount date must drive the amount calculation. However, the new condition also clears a nonblank discount date when the option is enabled, so saved or custom dates can still make the report ignore the discount.

Suggestions

S1 - Keep discount date when enabled
The new if IncPmtDiscount and PmtDiscDueDate = 0D then ... else PmtDiscDueDate := 0D clears a nonblank PmtDiscDueDate even when IncPmtDiscount is true. Change it so the date is only cleared when IncPmtDiscount is false, and only defaulted to WorkDate() when the option is true and the date is blank.

S2 - Test discount-only selection and amount
The new false-case test uses a due date three months ahead, so the invoice is selected as a due invoice even without the payment discount path. Add a case where the invoice due date is after DueDate but the payment discount date is within the discount window, and assert no line when the option is false and the exact discounted amount when true.

Risk assessment and necessity

Risk: 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 Payment Discount Date even when Take Payment Discounts is off, which can create confusing or wrong vendor payment suggestions.


[AI-PR-REVIEW] version=1 system=github pr=9092 round=1 by=alexei-dobriansky at=2026-07-06 lastSha=86ec162a21758c0488da8ec2ad598f1c2fe026f8 suggestions=S1,S2

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Please review S1.

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

Labels

Finance GitHub request for Finance area From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants