Skip to content

[Master] Exchange Rate Adjustment filter not working#9254

Open
Shikhverma wants to merge 3 commits into
microsoft:mainfrom
Shikhverma:bugs/Bug-640457-ExchangeRateAdjustmentFilterNotWorking
Open

[Master] Exchange Rate Adjustment filter not working#9254
Shikhverma wants to merge 3 commits into
microsoft:mainfrom
Shikhverma:bugs/Bug-640457-ExchangeRateAdjustmentFilterNotWorking

Conversation

@Shikhverma

@Shikhverma Shikhverma commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Bug 640457: [all-e]Exchange Rate Adjustment filter not working

AB#642009

@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 8, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 8, 2026
@Shikhverma Shikhverma closed this Jul 9, 2026
@Shikhverma Shikhverma reopened this Jul 9, 2026
@Shikhverma Shikhverma closed this Jul 9, 2026
@Shikhverma Shikhverma reopened this Jul 9, 2026
@Shikhverma Shikhverma added the Finance GitHub request for Finance area label Jul 10, 2026
@Shikhverma Shikhverma requested review from a team and v-ankitgoyal July 10, 2026 04:23
v-ankitgoyal
v-ankitgoyal previously approved these changes Jul 10, 2026
@Shikhverma Shikhverma self-assigned this Jul 10, 2026
@Shikhverma Shikhverma marked this pull request as ready for review July 10, 2026 07:39
@Shikhverma Shikhverma requested a review from a team July 10, 2026 07:39
if (CustLedgerEntry2."Posting Date" >= ExchRateAdjmtParameters."Start Date") and
(CustLedgerEntry2."Posting Date" <= ExchRateAdjmtParameters."End Date")
(CustLedgerEntry2."Posting Date" <= ExchRateAdjmtParameters."End Date") and
CustLedgEntryMatchesPostingGroupFilter(CustLedgerEntry2, CustPostingGroupFilter)

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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

CustLedgEntryMatchesPostingGroupFilter (new helper, lines 1354-1364) issues a fresh IsEmpty() query against Cust.

Ledger Entry for every Detailed Cust. Ledg. Entry iterated in PrepareTempCustLedgEntry's repeat/until loop, even though CustLedgerEntry2 was already fully loaded by the preceding Find('='). Cust. Ledger Entry is a production-scale table (~10M rows). The 'Customer Posting Group' value is already sitting in the CustLedgerEntry2 record passed in; re-querying the database to answer a question the caller can already answer in memory is the same N+1 pattern as calling Get/FindFirst inside a loop.

Recommendation:

  • move the CustPostingGroupFilter SetFilter onto CustLedgerEntry2 before the Find('=') call (it already filters by primary key there), and drop the separate existence-check procedure entirely.

Knowledge:

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

if (VendorLedgerEntry2."Posting Date" >= ExchRateAdjmtParameters."Start Date") and
(VendorLedgerEntry2."Posting Date" <= ExchRateAdjmtParameters."End Date")
(VendorLedgerEntry2."Posting Date" <= ExchRateAdjmtParameters."End Date") and
VendLedgEntryMatchesPostingGroupFilter(VendorLedgerEntry2, VendPostingGroupFilter)

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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

VendLedgEntryMatchesPostingGroupFilter (new helper, lines 1418-1428) issues a fresh IsEmpty() query against Vendor Ledger Entry for every Detailed Vendor Ledg.

Entry iterated in PrepareTempVendLedgEntry's repeat/until loop, even though VendorLedgerEntry2 was already fully loaded by the preceding Find('='). Vendor Ledger Entry is a production-scale table (~10M rows). The 'Vendor Posting Group' value is already present in the loaded record; re-querying the database is the same N+1 pattern as Get/FindFirst inside a loop.

Recommendation:

  • apply the VendPostingGroupFilter SetFilter onto VendorLedgerEntry2 before its Find('=') call and drop the separate existence-check procedure.

Knowledge:

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

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 2 · Outcome: completed

All sub-skills ran against the diff; performance and style leaves evaluated the changed logic, security/privacy leaves found no applicable concerns, upgrade and UI leaves ruled themselves not-applicable (no migration or page/UI files in the diff). Self-review pass found no additional cross-cutting defect clearing the precision bar.

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

Totals: 2 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.

@Shikhverma Shikhverma enabled auto-merge July 10, 2026 09:37
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This PR fixes Exchange Rate Adjustment so a customer or vendor posting group filter is applied to the ledger entries that are adjusted, not only to the Customer or Vendor master record. It captures the Customer/Vendor Posting Group filter, clears it from the master record selection, and then applies the same filter to Cust. Ledger Entry or Vendor Ledger Entry when building the temporary adjustment set.

The field choice is correct: the report receives filters from the Customer/Vendor dataitems, while the adjustment entries carry "Customer Posting Group" or "Vendor Posting Group" on the ledger entry. The open-entry path applies the filter directly, and the later-detailed-entry path checks the same ledger-entry field before inserting into the temporary set. That keeps posted FX gain/loss entries limited to the selected posting group and does not change the unfiltered general Adjust Exchange Rates run.

Suggestions

S1 - Cover alternate posting group filter
The new tests filter to the default posting group only. Add a case that filters to the alternate customer or vendor posting group, so the test proves the master record posting group filter is cleared and the ledger entry is still adjusted. This is useful because the RU process code is separate too.

Risk assessment and necessity

Risk: This is a financial posting area, so a wrong filter can post unrealized gains or losses to the wrong posting group. The change is narrow: it only moves the posting group filter from master selection to customer/vendor ledger entry selection in W1 and RU codeunit 699; public API and event signatures are unchanged. Existing runs without a posting group filter should keep the old behavior because an empty filter returns true and no ledger filter is added.

Necessity: Bug 640457 describes a reproduced scenario where one customer or vendor has multiple posting groups and the batch adjusts entries outside the selected group. The fix is needed because the old behavior can over-adjust entries and skew posted FX amounts. The scope is small and directly tied to the report filter.


[AI-PR-REVIEW] version=1 system=github pr=9254 round=1 by=alexei-dobriansky at=2026-07-10T11:06:56Z lastSha=4250b99d1d6662f0da55f954554ffe22a9ff38f3 suggestions=S1

CustLedgerEntry2."Entry No." := DtldCustLedgEntry2."Cust. Ledger Entry No.";
if CustLedgerEntry2.Find('=') then
if (CustLedgerEntry2."Posting Date" >= ExchRateAdjmtParameters."Start Date") and
(CustLedgerEntry2."Posting Date" <= ExchRateAdjmtParameters."End Date")

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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

CustLedgEntryMatchesPostingGroupFilter is invoked once per detailed customer ledger entry inside the DtldCustLedgEntry2 repeat-loop.

Each call copies the record and issues a fresh IsEmpty() query against Cust. Ledger Entry (a production-scale table) just to test whether the already-loaded record's 'Customer Posting Group' satisfies a filter string, producing an N+1 round-trip per iteration. The same filter is already applied via SetFilter before Find/FindSet a few lines below in the same procedure (the 'open entries' block), showing the filter can be folded into CustLedgerEntry2 before the loop (e.g. CustLedgerEntry2.SetFilter("Customer Posting Group", CustPostingGroupFilter) prior to the Find('-')/repeat), so Find('=') on the primary key would naturally exclude non-matching rows with zero extra queries.

Knowledge:

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

@github-actions

Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

VendLedgEntryMatchesPostingGroupFilter is invoked once per detailed vendor ledger entry inside the DtldVendLedgEntry2 repeat-loop, mirroring the customer-side issue: each call copies the record and runs a separate IsEmpty() query against Vendor Ledger Entry (a production-scale table) to test the already-loaded record's 'Vendor Posting Group' against a filter string.

The same filter is already applied via SetFilter a few lines below (the 'open entries' block), so it can be folded into VendorLedgerEntry2's filter set ahead of the loop instead of querying per row.

Knowledge:

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

Since round 1, this PR adds alternate posting group regression tests. The new tests run Adjust Exchange Rates with the Customer or Vendor master record filtered to an alternate posting group while the master record still has a different default posting group.

The assertions check that the unrealized detailed ledger entry is created for the alternate posting group and not for the default posting group. This proves the master-record posting group filter is cleared and the ledger-entry posting group filter still controls which entries are adjusted. RU coverage was also added for the separate RU customer process; W1 vendor coverage checks the vendor alternate-posting-group path.

Status of previous suggestions
ID Title Status Author response
S1 Cover alternate posting group filter Addressed No author reply. Fixed in aab084f2f9 with alternate posting group tests for W1, IT, RU customer coverage, and W1 vendor coverage.
New observations (commits since round 1)

None - the new commit only adds the requested regression coverage.

Risk assessment and necessity

Risk: The runtime change is still in a financial posting area, so a wrong filter could adjust entries in the wrong posting group. The round-2 commit only adds tests, so it does not add new runtime risk. The tests cover both the adjusted alternate posting group and the not-adjusted default posting group.

Necessity: The extra test is needed because the bug depends on a customer or vendor using more than one posting group. Without this coverage, a future change could again leave the posting group filter on the master record and skip the ledger entries that must be adjusted.


[AI-PR-REVIEW] version=1 system=github pr=9254 round=2 by=alexei-dobriansky at=2026-07-10T17:06:28Z lastSha=aab084f2f993ac4cb2ccc5e18cdb504e040a7edd suggestions=S1:addressed parentRound=1

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.

4 participants