Skip to content

[Master]-Slice 425446: Payment Journal Batch Total - #10538

Open
ViditGupta1277 wants to merge 2 commits into
microsoft:mainfrom
ViditGupta1277:features/Slice-425446-Payment-Journal-Batch-Total
Open

[Master]-Slice 425446: Payment Journal Batch Total#10538
ViditGupta1277 wants to merge 2 commits into
microsoft:mainfrom
ViditGupta1277:features/Slice-425446-Payment-Journal-Batch-Total

Conversation

@ViditGupta1277

@ViditGupta1277 ViditGupta1277 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Fixes AB#425446

Requirement:
Would be nice to have a Batch Total for Payment Journal. Currently, have to post a payment journal before knowing how much was selected for payment.

Balance and Total Balance is only related to balancing the transaction for each row and not useful for batch totals.

@ViditGupta1277
ViditGupta1277 requested a review from a team August 23, 2026 06:28
@ViditGupta1277
ViditGupta1277 requested a review from a team as a code owner August 23, 2026 06:28
@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 Team: Finance GitHub request for Finance area labels Aug 23, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Aug 23, 2026
Comment thread src/Layers/W1/BaseApp/Finance/GeneralLedger/Journal/GenJnlManagement.Codeunit.al Outdated
Comment thread src/Layers/W1/BaseApp/Finance/GeneralLedger/Journal/PaymentJournal.Page.al Outdated
Comment thread src/Layers/W1/BaseApp/Finance/GeneralLedger/Journal/PaymentJournal.Page.al Outdated
Comment thread src/Layers/W1/Tests/General Journal/ERMGeneralJournalUT.Codeunit.al
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This Slice adds a Batch Total (LCY) field to Payment Journal pages across localized layers, backed by GenJnlManagement.CalcBatchTotal, and adds tests for totals, empty batches, batch changes, and amount edits. The feature goal is valid: users need a batch-level payment total before posting. The sum itself uses the current page filters and Amount (LCY), which matches the stated batch-total behavior. The main correctness risk is that the page recalculates the total during amount field validation by reading persisted lines, but the edited line has not been explicitly saved first.

Suggestions

S1 - Save before recalculating edited amounts
The new amount-field UpdateBalance() calls recalculate Batch Total from persisted Gen. Journal Line records. Save the current line first, or adjust the total with the Rec/xRec amount delta, so the edited amount is included before the user leaves the line.

S2 - Avoid extra full-batch sums per edit
Each amount edit now runs the existing balance refresh and another full-batch CalcSums("Amount (LCY)") across every localized Payment Journal page. Large payment batches can pay several aggregate queries per edit. Consider updating Batch Total from the edited-line delta, or refresh it once after the record is saved.

S3 - Document new public extension points
CalcBatchTotal and the new Batch Total events expose new integration points, but they do not have the XML summary and param comments used by nearby public procedures and events. Add those comments in W1, ES, NL, and the page copies so subscribers understand the filters, amount sign, and visibility contract.

Risk assessment and necessity

Risk: This is a financial UI surface, so a stale Batch Total can mislead users about the amount selected for payment before posting. The diff does not change posting or ledger-entry creation, but it adds a public helper and event publishers and it runs extra aggregate calculations on Payment Journal navigation and amount validation in all localized page copies.

Necessity: The change is justified by Slice 425446 and the Ideas-site request: Balance and Total Balance do not answer the batch-level payment-total question. The scope is mostly right because all Payment Journal page copies are covered and tests were added, but the edited-line refresh path should be fixed before merge.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10538 round=1 by=alexei-dobriansky at=2026-08-23T07:14:54Z lastSha=3ad85272d7b0e240ea0a4fb0419810ef39752787 reviewKey=29558c8ec93c737f8d43bebe7d56afb7dce690edd55dc6dc9caaecf91c7ebfc2 suggestions=S1@392321d0,S2@b721127e,S3@f9d4d535

@github-actions

Copy link
Copy Markdown
Contributor

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

UpdateBalance now unconditionally calls UpdateBatchTotal on every current-record refresh and OnNewRecord. That means each row navigation/new line adds another full filtered CalcSums over Gen. Journal Line on top of the existing CalcBalance work, and the same pattern is duplicated across all localized Payment Journal page copies in this PR. Compute Batch Total only when the batch/filter context changes, or fold it into the existing balance calculation instead of issuing a third aggregate query per refresh.

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 thread src/Layers/CH/Tests/General Journal/ERMGeneralJournalUT.Codeunit.al
Comment thread src/Layers/ES/Tests/General Journal/ERMGeneralJournalUT.Codeunit.al
Comment thread src/Layers/W1/Tests/General Journal/ERMGeneralJournalUT.Codeunit.al
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Accept with Suggestions

What this PR does

This round updates the Payment Journal Batch Total implementation after round 1. Amount validation now saves the current line and refreshes only Batch Total, so the edited amount is included without forcing the full balance refresh path. The new Batch Total filter hook events were also removed. The feature still matches the Slice goal: show the total selected for payment at batch level, with tests for totals, empty batches, batch switching, and amount edits.

Status of previous suggestions
ID Title Status Author response
S1 Save before recalculating edited amounts Addressed The amount validation path now calls CurrPage.SaveRecord() before recalculating Batch Total.
S2 Avoid extra full-batch sums per edit Addressed The amount validation path no longer calls the full UpdateBalance() flow; it refreshes only Batch Total after save.
S3 Document new public extension points Not addressed No author reply found. The new event publishers were removed, but CalcBatchTotal is still a new public procedure without XML summary and parameter comments.
New observations (commits since round 1)

None - the new commit only addresses prior review feedback.

Risk assessment and necessity

Risk: This is a financial UI surface, so a wrong or stale Batch Total can mislead users before posting. The round-2 change lowers the main correctness risk by saving the edited line before recalculating. It does not change posting or ledger entries. One non-blocking maintainability issue remains: the new public CalcBatchTotal procedure should document its filter and visibility contract.

Necessity: The Slice remains justified by the Ideas-site request. Balance and Total Balance do not show the batch-level amount selected for payment, and the scope covers the Payment Journal page copies plus relevant tests.


[AI-PR-REVIEW] version=1 promptVersion=2 system=github pr=10538 round=2 by=alexei-dobriansky at=2026-08-23T19:06:01Z lastSha=5101561d1f37efcc74a8e0040d6ba68dc92bde16 reviewKey=42c5d6b913f5616a33244e458d211df2a80cecb9755ca83e13e22dd133205fd4 suggestions=S1@392321d0:addressed,S2@b721127e:addressed,S3@f9d4d535:notaddressed parentRound=1

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants