Skip to content

[Master]-Opening price list from Customer Card is extremely slow on large datasets (~220s) due to AddAllSourceType call#9265

Open
neeleshsinghal wants to merge 2 commits into
mainfrom
bugs/Bug-641061-Opening-price-list-from-Customer-Card-is-fast1
Open

[Master]-Opening price list from Customer Card is extremely slow on large datasets (~220s) due to AddAllSourceType call#9265
neeleshsinghal wants to merge 2 commits into
mainfrom
bugs/Bug-641061-Opening-price-list-from-Customer-Card-is-fast1

Conversation

@neeleshsinghal

@neeleshsinghal neeleshsinghal commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@neeleshsinghal neeleshsinghal requested a review from a team July 9, 2026 06:17
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 9, 2026
@neeleshsinghal neeleshsinghal added the SCM GitHub request for SCM area label Jul 9, 2026
@neeleshsinghal neeleshsinghal requested a review from Copilot July 9, 2026 06:21

Copilot AI left a comment

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.

Pull request overview

This PR addresses a major performance regression when opening Sales Price Lines from the Customer Card on large datasets by reducing unnecessary record loading/iteration work in Price List source filtering, and it adds a regression UI test to validate that “All Customers” price lines are fully returned.

Changes:

  • Optimized source-line discovery/marking in Price List Management by using IsEmpty() for existence checks and limiting loaded fields during marking.
  • Added a UI test that creates many “All Customers” price lines and verifies they are all shown when opened from the Customer Card.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
src/Layers/W1/BaseApp/Pricing/PriceList/PriceListManagement.Codeunit.al Optimizes filtering/marking logic to reduce data loading and improve performance.
src/Layers/W1/Tests/ERM/PriceListsUI.Codeunit.al Adds a regression UI test to ensure “All Customers” lines are shown when opened from Customer Card.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Layers/W1/Tests/ERM/PriceListsUI.Codeunit.al
Comment thread src/Layers/W1/Tests/ERM/PriceListsUI.Codeunit.al
Comment thread src/Layers/W1/BaseApp/Pricing/PriceList/PriceListManagement.Codeunit.al Outdated
@github-actions

github-actions Bot commented Jul 9, 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 1 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.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR tries to speed up Price List Line Review opened from Customer Card by using IsEmpty() in search mode and SetLoadFields("Price List Code", "Line No.") before marking matching source lines. It also adds a UI test for All Customers price lines.

The change preserves the exact source tuple behavior: each (Source Type, Parent Source No., Source No.) is still filtered separately, and OnBuildSourceFiltersOnBeforeFindLines still runs after those filters are set. This avoids the independent OR-filter bug seen in PR 9219/9269. However, the main performance root cause remains: the non-search path still iterates and marks every matching All Customers line, then applies MarkedOnly(true).

Relationship to the other PRs: PR 9219 was closed after round-2 Request Changes. PR 9265 is a different replacement/alternative, not the same diff. PR 9269 is still open for the same work item and is closer to the OR-filter approach from PR 9219, so it is a competing duplicate alternative.

Suggestions

S1 - Still materializes all source lines
The non-search path still calls FindSet() and Mark(true) for every matching All Customers line, and SetPriceListLineFilters still applies MarkedOnly(true). SetLoadFields reduces loaded columns, but it does not remove the 61k-row marking work that caused the 220s load. Replace this with an approach that avoids materializing the broad source set while still preserving each exact source tuple.

S2 - Test does not prove the fix
T220_SalesPriceLinesFromCustomerCardManyAllCustomersLinesAreNotMarked only creates 10-25 All Customers lines and counts them. The old code already returns those lines, so the test can pass without this fix. Add a regression or performance test that fails on the old marking path and proves the large-source path is no longer materialized.

S3 - Close the duplicate alternative PR
PR 9219 is closed, and PR 9269 is still open for the same ADO bug with a different OR-filter approach. Keep one replacement PR and close the other alternative. This avoids merging a conflicting fix for the same pricing path.

Risk assessment and necessity

Risk: This is a pricing UI path, so incorrect filtering can show the wrong price list lines. This PR keeps the exact tuple filters and keeps the event contract, which is good. The remaining risk is that the performance bug is not actually fixed because the broad All Customers set is still fully marked.

Necessity: ADO bug 641061 is valid and important. Opening Sales Prices from Customer Card can take about 220 seconds on large datasets, so a fix is needed. The scope should stay in this filtering path, but it must remove the expensive materialization, not only reduce loaded fields.


[AI-PR-REVIEW] version=1 system=github pr=9265 round=1 by=alexei-dobriansky sha=a86bd0d73b22e13940929511ccb740037e673f49

Comment thread src/Layers/W1/Tests/ERM/PriceListsUI.Codeunit.al
@neeleshsinghal

Copy link
Copy Markdown
Contributor Author

s1

Addressed

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 2

Recommendation: Request Changes

What this PR does

The new commit adds a source pre-check so SetPriceListLineFilters only applies MarkedOnly(true) when more than one source tuple returns rows. This helps only when a single source tuple has data.

It does not fully fix the reported Customer Card scenario. If both customer-specific rows and All Customers rows exist, the pre-check sets MarkingIsUsed to true, and BuildSourceFilters still calls FindSet() and Mark(true) over the matching rows before applying MarkedOnly(true). The test and duplicate PR state were not updated.

Status of previous suggestions
ID Title Status Author response
S1 Still materializes all source lines Not addressed "s1 Addressed", but the new code still marks all rows when more than one source tuple has data.
S2 Test does not prove the fix Not addressed No test change since round 1.
S3 Close the duplicate alternative PR Not addressed PR 9269 is still open; PR 9219 is closed.
New observations (commits since round 1)

S4 - Source filter event now runs twice
The new source pre-check calls OnBuildSourceFiltersOnBeforeFindLines, and the real build pass calls it again. Subscribers can now run twice in the non-search path. Use a separate pre-check hook or avoid raising the existing event during the count pass.

Risk assessment and necessity

Risk: This is a pricing UI path, so incorrect filters can show wrong price list lines. The remaining performance risk is high because large All Customers rows can still be marked when another source tuple also has rows. The new pre-check also changes event behavior by calling the source filter event twice.

Necessity: ADO bug 641061 is valid and important. The reported Customer Card Sales Prices path can take about 220 seconds on large datasets, so a fix is needed. The fix must remove the broad materialization while keeping exact source tuple semantics, and one duplicate PR should be closed.


[AI-PR-REVIEW] version=1 system=github pr=9265 round=2 by=alexei-dobriansky sha=363e2e3947ad3d611676274b6e3b3e41f6d2bba4

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

Labels

SCM GitHub request for SCM area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants