Skip to content

[Main] [all-e]Unbalanced G/L Entries if you apply a Payment and a Posted Purchase Invoice having both different Posting Groups and using the "Applies-to Document No." on the line instead of using Apply Entries in the Spanish version.#8950

Closed
sanjmaurya wants to merge 10 commits into
microsoft:mainfrom
sanjmaurya:bugs/Bug-640410---master]UnbalancedG/LEntriesIfyouApply-PaymentPostedPurchase

Conversation

@sanjmaurya

@sanjmaurya sanjmaurya commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

I have create new PR

@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 1, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 1, 2026
@sanjmaurya sanjmaurya closed this Jul 1, 2026
@sanjmaurya sanjmaurya reopened this Jul 1, 2026
@sanjmaurya sanjmaurya marked this pull request as ready for review July 1, 2026 10:04
@sanjmaurya sanjmaurya requested a review from a team July 1, 2026 10:04
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 14 · 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
Performance 1 1 0 0 0

Totals: 1 knowledge-backed · 0 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.

@sanjmaurya sanjmaurya added the Finance GitHub request for Finance area label Jul 3, 2026
v-ankitgoyal
v-ankitgoyal previously approved these changes Jul 3, 2026
@sanjmaurya sanjmaurya requested a review from a team July 3, 2026 06:55
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This PR narrows the ES-specific vendor application account override when a payment is applied through Applies-to Doc. No.. The new check uses the existing vendor ledger entry to tell the payment-side detailed entry from the applied invoice-side entry, so the invoice side can use its own vendor posting group.

The change matches the root cause: the detailed buffer Document Type alone cannot identify which side of the application is being posted. If the payment ledger entry is not inserted yet, the code still keeps the payment posting group path. The added ES test covers the exact inline Applies-to Doc. No. scenario with different posting groups and asserts both payables accounts net to zero.

Suggestions

S1 - PR text overstates unapply coverage
The PR text says the change is mirrored in PostDtldVendLedgEntryUnapply, but the diff only changes PostDtldVendLedgEntry. Please either update the description, or add the same guard and a test if unapply must be covered.

Risk assessment and necessity

Risk: This is high-sensitivity posting code in the ES BaseApp vendor application path. The code change is narrow, adds no public API or event signature change, and does not depend on a new BaseApp publisher. W1 and other country BaseApp layers use the existing vendor ledger entry posting group path directly and do not have this ES Cartera special case, so the ES-only scope is appropriate.

Necessity: The ADO bug is clear and important: applying a posted purchase invoice through Applies-to Doc. No. can leave payables accounts unbalanced when the payment uses a different vendor posting group. The new regression test makes the sensitive path repeatable and checks the resulting G/L balance. Without the change, users must avoid the inline apply path and use Apply Entries instead.


[AI-PR-REVIEW] version=1 system=github pr=8950 round=1 by=alexei-dobriansky at=2026-07-03 lastSha=4f2da8f970e29a66f636ee13dc45fc74dc12493c suggestions=S1

dcenic
dcenic previously approved these changes Jul 3, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.


🔄 How to trigger a new status check:

  1. 📤 Push a new commit to the PR branch, or
  2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

@sanjmaurya sanjmaurya closed this Jul 6, 2026
@sanjmaurya sanjmaurya reopened this Jul 6, 2026
@sanjmaurya sanjmaurya closed this Jul 7, 2026
@sanjmaurya sanjmaurya reopened this Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 5}}$

PostDtldVendLedgEntry was fixed in this PR to skip the payment's own Payables Account when the applied document (via "Applies-to Doc.

No.") uses a different Vendor Posting Group, by adding IsApplnEntryForAppliedVendorDoc as an extra guard. PostDtldVendLedgEntryUnapply (lines 5645-5674) contains the exact same MultiplePostingGroups / Initial Document Type / Applies-to Doc. No. condition structure but was not updated with the same guard. If a payment applied this way is later unapplied, the unapply path will still resolve to the payment's own posting group's Payables Account instead of the applied invoice's, reintroducing the same G/L imbalance this PR fixes for the apply path — only during unapply. Given the underlying impact mirrors the bug this PR fixes (potentially major/blocker if it recurs), this concern should be verified and, if confirmed, promoted to a tracked fix (and ideally a knowledge-backed rule about keeping apply/unapply account-resolution logic symmetric) rather than left as an advisory-only agent finding.

Recommendation:

  • apply IsApplnEntryForAppliedVendorDoc (or equivalent) to the corresponding branch in PostDtldVendLedgEntryUnapply and add a regression test for unapplying a payment that was applied via "Applies-to Doc. No." across different Vendor Posting Groups.

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

        if MultiplePostingGroups and (DetailedCVLedgEntryBuffer."Entry Type" = DetailedCVLedgEntryBuffer."Entry Type"::Application) then
            if DetailedCVLedgEntryBuffer."Initial Document Type" = DetailedCVLedgEntryBuffer."Initial Document Type"::Bill then
                AccNo := VendPostingGr."Payables Account"
            else
                if (GenJournalLine."Document Type" = GenJournalLine."Document Type"::Payment) and (GenJournalLine."Applies-to Doc. No." <> '') and (DetailedCVLedgEntryBuffer."Document Type" = DetailedCVLedgEntryBuffer."Document Type"::Payment) and
                   (not IsApplnEntryForAppliedVendorDoc(GenJournalLine, DetailedCVLedgEntryBuffer))
                then
                    AccNo := VendPostingGr."Payables Account"
                else
                    AccNo := GetVendDtldCVLedgEntryBufferAccNo(GenJournalLine, DetailedCVLedgEntryBuffer)
        else

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 7}}$

Underlying impact would otherwise be major/blocker, capped to minor per agent-finding severity rules (no knowledge-file citation yet).

GenJnlPostLine.Codeunit.al adds IsApplnEntryForAppliedVendorDoc(...) to PostDtldVendLedgEntry (the apply path, line ~5634) so an application posted via 'Applies-to Doc. No.' against a differently-posting-grouped document routes to the applied document's own Payables Account instead of the payment's. PostDtldVendLedgEntryUnapply — the sibling reversal procedure a few lines below (~5666) — still has the exact pre-fix condition without the new guard, so reversing (unapplying) such a payment will route the reversing G/L entry to the payment's posting-group Payables Account instead of the invoice's, re-introducing the same balance defect this PR fixes for apply. The new test (ApplyPmtToPostedPurchInvWithDifferentPostingGroupsViaAppliesToDocNo) only exercises apply and does not cover unapply, so this gap is untested.

Recommendation:

  • apply the same IsApplnEntryForAppliedVendorDoc(...) guard to the equivalent condition in PostDtldVendLedgEntryUnapply, and add an unapply-scenario test alongside the new apply test. This concern should be promoted to a knowledge-backed rule once confirmed.

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

                if (GenJournalLine."Document Type" = GenJournalLine."Document Type"::Payment) and (GenJournalLine."Applies-to Doc. No." <> '') and (DetailedCVLedgEntryBuffer."Document Type" = DetailedCVLedgEntryBuffer."Document Type"::Payment) and
                   (not IsApplnEntryForAppliedVendorDoc(GenJournalLine, DetailedCVLedgEntryBuffer))
                then
                    AccNo := VendPostingGr."Payables Account"
                else
                    AccNo := GetVendDtldCVLedgEntryBufferAccNo(GenJournalLine, DetailedCVLedgEntryBuffer)

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 8}}$

PostDtldVendLedgEntryUnapply (line 5645) still contains the exact pre-fix condition that PostDtldVendLedgEntry (line 5630) just had corrected: 'if (GenJournalLine."Document Type" = ...Payment) and (GenJournalLine."Applies-to Doc.

No." <> '') and (DetailedCVLedgEntryBuffer."Document Type" = ...Payment) then AccNo := VendPostingGr."Payables Account"' with no call to the new IsApplnEntryForAppliedVendorDoc guard. Both procedures mirror each other line-for-line elsewhere in this codeunit (apply vs. unapply of the same detailed CV ledger entry buffer rows). Since the new test only exercises apply and not unapply, unapplying a payment that was applied via "Applies-to Doc. No." across two different vendor posting groups will still post the reversal to the payment's own Payables Account instead of the invoice's own posting group account, recreating the same G/L imbalance this PR fixes for the apply path. Impact would be major/blocker (unbalanced Payables Accounts after unapply) if confirmed; recommend applying the same IsApplnEntryForAppliedVendorDoc check inside PostDtldVendLedgEntryUnapply and adding a matching unapply test case, and until then this should be treated as a real defect rather than a minor one.

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

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Accept

What this PR does

The new commit adds the same IsApplnEntryForAppliedVendorDoc(...) guard to PostDtldVendLedgEntryUnapply that the apply path already uses. This removes the mismatch called out in round 1 between the PR description and the unapply code.

The added code is narrow and matches the apply-side account selection shape: the payment-side application entry can still use the payment posting group's payables account, while an applied document entry can fall back to GetVendDtldCVLedgEntryBufferAccNo(...) and use its own posting group. In the normal unapply entry point, the Gen. Journal Line is rebuilt for unapply and does not carry Applies-to Doc. No., so this change does not broaden standard unapply behavior.

Status of previous suggestions
ID Title Status Author response
S1 PR text overstates unapply coverage Addressed No written reply; commit 3c41da9c6d adds the unapply guard.
New observations (commits since round 1)

None - the only author-owned change since round 1 is the unapply guard, and I did not find a new issue in that change.

Risk assessment and necessity

Risk: This remains sensitive ES vendor posting code because it controls payables account G/L entries during apply and unapply with multiple vendor posting groups. The round-2 code change is only three added lines in the ES Gen. Jnl.-Post Line unapply path, adds no public API or event signature change, and does not affect W1 or other country layers. There is still no direct unapply regression test, but I am not treating that as blocking because the change is narrow and the normal unapply setup does not use the inline Applies-to Doc. No. field.

Necessity: The original bug is valid and important: users can get unbalanced payables accounts when applying a payment to a posted purchase invoice through the inline Applies-to Doc. No. path with different vendor posting groups. The round-2 change is justified because it keeps the ES apply and unapply account-selection logic aligned with the PR description and avoids leaving the unapply path as a future divergence point.


[AI-PR-REVIEW] version=1 system=github pr=8950 round=2 by=alexei-dobriansky at=2026-07-10T11:04:05.441Z lastSha=3c41da9c6da06abef4028dbc9ec4d8d8cc98c0a3 suggestions=S1:addressed parentRound=1

@sanjmaurya sanjmaurya closed this Jul 10, 2026
@sanjmaurya sanjmaurya reopened this Jul 10, 2026
@aholstrup1

Copy link
Copy Markdown
Contributor

Heads up, these build failures aren't from your code changes. The special character (a stray ]) in the branch name trips up our pipeline. Recreating the branch without the bracket and repointing the PR should fix it. Sticking to letters, numbers, /, -, and _ in branch names avoids this going forward.

@sanjmaurya sanjmaurya closed this Jul 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Could not find linked issues in the pull request description. Please make sure the pull request description contains a line that contains 'Fixes #' followed by the issue number being fixed. Use that pattern for every issue you want to link.

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.

6 participants