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

[neeleshsinghal]

AB#641061

[github-actions]

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

In the rewritten BuildSourceFilters (PriceListManagement.Codeunit.al, local procedure at line 751), when every price source in PriceSourceList is empty (NonEmptyCount stays 0), MarkingIsUsed is left false and the trailing 'else ClearSourceFilters(PriceListLine)' branch (line 792-795) removes the Source Type/Parent Source No./Source No.

filters entirely. Because the caller (SetPriceListLineFilters, line 642-643) only calls PriceListLine.MarkedOnly(true) when MarkingIsUsed is true, no marking restriction is applied either. The net effect is that PriceListLine ends up filtered only by Status/Price Type/Amount Type, i.e. it returns every price line in the system for that status/type/amount combination instead of an empty result -- a real regression versus the pre-change behavior, where ClearSourceFilters was always paired with an unconditional MarkedOnly(true), so zero marked lines correctly produced zero results. The immediately adjacent, structurally identical BuildAssetFilters/CheckIfPriceListLineMarkingIsNeeded pair in the same codeunit (line 711-712) explicitly guards this exact case with 'if RecordSetsCounter = 0 then exit(true)' to force MarkedOnly(true) when no asset matched; BuildSourceFilters' new NonEmptyCount logic omits the equivalent guard for the 0-match case. Neither new test (T220, T221) exercises a customer/context where every price source is empty, so this path is untested. Recommend forcing MarkingIsUsed := true (or otherwise ensuring the result set is empty) when NonEmptyCount = 0, mirroring CheckIfPriceListLineMarkingIsNeeded. This would otherwise be rated major/blocker (functional correctness: unrelated customers' or items' price lines become visible), but is capped at minor here per agent-finding severity rules since no BCQuality knowledge article covers this exact case.

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Testing} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

In T221_SalesPriceLinesFromCustomerCardMultipleSourceTypesSharingEmptyParent, the fixture assigns Customer."Customer Price Group" and Customer."Customer Disc.

Group" directly and calls Modify(), bypassing the TableRelation and any OnValidate logic on those foreign-key fields. Use Customer.Validate on each field before Modify so the fixture exercises the same setup rules as production code.

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

Customer.Validate("Customer Price Group", CustomerPriceGroup.Code);
        Customer.Validate("Customer Disc. Group", CustomerDiscountGroup.Code);
        Customer.Modify(true);

Knowledge:

• microsoft/knowledge/testing/use-library-codeunits-for-test-fixtures.md

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

[github-actions]

Copilot PR Review

Iteration 2 · Outcome: completed

All 16 sub-skills completed; no knowledge-backed violations or cross-cutting defects found. The diff optimizes BuildSourceFilters to avoid unconditional Mark-based filtering (using IsEmpty existence checks and SetView for the 0/1-match cases), which was validated against BCQuality performance guidance and found compliant.

Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf

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

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

_{Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives.}

[qasimikram]

S1 (blocking): Please preserve an empty result when none of the price sources match. In BuildSourceFilters, NonEmptyCount = 0 currently leaves MarkingIsUsed false and then calls ClearSourceFilters. The caller consequently skips MarkedOnly(true), leaving only the status, price type, and amount type filters and potentially showing unrelated price lines. Please handle the zero-match case explicitly and add a regression test where every source is empty.

Question on performance coverage: The new fast path avoids marking only when exactly one source has matching rows. When both All Customers and a customer-specific or group source contain rows, NonEmptyCount reaches 2 and the implementation returns to marking every matching row, including a potentially very large All Customers set. Could you confirm one of the following?

1. The production reproduction has only one non-empty source, with before/after timing showing that this change meets the <5-second target.
2. The multi-source scenario has also been tested at scale and remains acceptably fast when both All Customers and customer/group rows exist.

[qasimikram]

The zero-match regression is addressed: NonEmptyCount = 0 now forces MarkedOnly(true), and T222 covers the case where unrelated price lines exist but none match the current customer's sources.

One performance-coverage question remains. The fast path avoids marking only when exactly one source has matching rows. If both All Customers and a customer-specific or group source contain rows, NonEmptyCount reaches 2 and the implementation still marks every matching row, including a potentially large All Customers set. Could you confirm one of the following?

1. The production reproduction has only one non-empty source, with before/after timing showing that this change meets the <5-second target.
2. The multi-source scenario has also been tested at scale and remains acceptably fast when both All Customers and customer/group rows exist.

Non-blocking test suggestion: use Validate for the customer group fields and Modify(true) in T221/T222 so the fixtures exercise the same validation path as production setup.

[qasimikram]

Could you confirm the performance coverage for this fallback path? When both All Customers and a customer-specific or group source contain rows, this branch still marks every matching row, including a potentially large All Customers set.

Please confirm either that the production reproduction has only one non-empty source with before/after timing meeting the <5-second target, or that this multi-source path was tested at scale and remains acceptably fast.

[qasimikram]

The blocking zero-match regression is fixed and covered by T222. I found no remaining correctness issue in the updated diff. Approving with the open performance-coverage thread left for confirmation before merge.