[Bug][SubscriptionBilling]: Performance: Billing document creation does not scale to large datasets (4M+ subscription lines)

[miljance]

Introduces a suite of code-level and database-index optimizations to make billing-proposal creation, document creation, and usage-data processing scale to millions of subscription lines.

Progress & UX

• New "Progress Tracker" codeunit (8035): reusable ETA/throughput dialog, GuiAllowed-guarded, time-throttled, safe for Job Queue runs.
• Progress dialogs added to: CreateBillingDocuments, BillingProposal (create/clear/delete), GenericConnectorProcessing, ProcessUsageDataBilling.
• Recurring Billing page: cached per-row contract/partner name lookups (eliminates 10k+ uncached Gets per render); removed 9 full temp-table rebuilds from pure-navigation drilldowns; targeted group refresh for document drilldowns, Delete Billing Line, and Change Billing To Date actions.

Transaction boundaries

• BillingProposal: per-N-contract commit checkpoint (N=10) for large automated runs; test suite updated to match new commit semantics.
• CreateBillingDocuments: per-document commit checkpoint on the non-posting path.
• ProcessUsageDataBilling: removed per-commitment Commit() inside loop.

Code-level optimizations

• BillingPriceCalcSkip (new CU 8036): manual-binding subscriber that skips redundant unit-price/cost engine calls when inserting billing document lines, eliminating price-list lookups for every line.
• CreateBillingDocuments: Dictionary-based temp-billing-line aggregation replacing O(n) FindFirst per line; per-contract TrimTempBillingLines using shared-table copy to keep temp table in memory.
• BillingProposal: SetLoadFields on contract/subscription header Gets in the recursive UpdateBillingLineFromServiceCommitment path.
• FilterBillingLinesOnServiceCommitment: added Subscription Header No. filter so proposal loop seeks on existing SK1 index.
• GenericConnectorProcessing: hoisted loop-invariant GenericImportSettings Get out of the per-row loop.
• UsageBasedContrSubscribers: removed 3 event subscribers whose logic now lives directly in BillingLine.OnDelete, SalesDocuments, and PurchaseDocuments - eliminating event dispatch overhead on hot paths.
• CheckOnlyOneServicePartnerType: replaced full-scan loop with two IsEmpty() checks.

Database indexes

• BillingLine: SK7 ("Subscription Line Entry No."), TableRelation + DropDown fieldgroup added.
• BillingLineArchive: DropDown fieldgroup added.
• SubscriptionLine: Key4 ("Supplier Reference Entry No.", "Subscription Line Start Date").
• UsageDataBilling: key2-key4 reordered/extended; key5 ("Billing Line Entry No."); TableRelation for Document Type = None added so the field navigates correctly from the list.
• UsageDataGenericImport: Key2 ("Usage Data Import Entry No.", "Processing Status").

What & why

Linked work

Fixes #7690

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required ΓÇö be specific: scenarios, commands, screenshots for UI changes)

• Ran the full Subscription Billing test suite (RecurringBillingTest, UsageBasedBillingTest, contract and UBB suites); all pass.
• Verified progress dialogs (elapsed, progress, ETA, throughput) on all modified actions interactively: Create Billing Proposal, Create Documents, Clear Billing Proposal, Delete Billing Lines, Delete Documents, Process Usage Data (import and billing steps). ETA converges as expected; dialogs close cleanly on completion and do not open in headless/Job Queue sessions.
• Confirmed that drilldowns on Partner, Contract, Subscription, and Document fields in Recurring Billing refresh the affected groups without rebuilding the full proposal temp table. Opened a collective invoice document from the list, deleted it inside the card, and verified the billing lines showed cleared Document No. with correct group subtotals and no stale rows.

Risk & compatibility

• Commit boundary in CreateBillingProposal: a checkpoint is committed every 10 contracts. Any caller that holds an open page or record reference across a CreateBillingProposal call on a dataset larger than 10 contracts will see a stale-rowversion error on the next edit (BC's standard concurrency message). The two affected standard tests were updated. Third-party extensions keeping a subpage open across a proposal run would need the same treatment.
• BillingPriceCalcSkip (CU 8036): while bound during sales/purchase line initialisation, OnBeforeUpdateUnitPrice, OnBeforeGetUnitCost, OnBeforeUpdateQuantityFromUOMCode (Sales Line) and OnBeforeUpdateUnitCost (Purchase Line) are handled and skipped. A third-party subscriber using those events to apply a surcharge or override during billing document creation would be bypassed. The subscriber is bound/unbound surgically around field initialisation only; all other validation still runs.
• DeleteBillingProposal now uses per-line Delete(true) instead of DeleteAll(true): OnDelete triggers still fire identically; the only difference is that partial progress is durable on interruption. No behavioral change.
• BillingLine.OnDelete directly updates UsageDataBilling (previously via event subscriber with IsTemporary guard): AL OnDelete never fires for temporary records so the guard was redundant ΓÇö no behavioral change.
• Database index additions (BillingLine SK7, UsageDataBilling key5, others) are purely additive; no data migration or upgrade codeunit needed.

Fixes AB#641986

[github-actions]

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

Commit() removed after service object quantity update

The Commit() after UpdateServiceObjectQuantity was protecting atomicity: it ensured that each successfully processed subscription line was durable before the next one began. Without it, an error on a later subscription line will roll back all quantity and commitment changes in the batch, potentially losing partial progress on a long-running import.

Recommendation:

• Re-add Commit() after ServiceCommitment.Get(), or implement explicit error-handling (e.g. TryFunction) so successful lines are committed independently of later failures.

UpdateServiceObjectQuantity(ServiceCommitment."Subscription Header No.", NewServiceObjectQuantity);
ServiceCommitment.Get(ServiceCommitment."Entry No."); // re-read recalculated values
Commit(); // make each processed line durable before continuing the batch
UpdateServiceCommitment(...);

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

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

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

CalcSubscriptionLineEndDate ignores Extension Term guard

Removing if not IsExtensionTermEmpty() then exit; means Subscription Line End Date is now always set to Start Date + Initial Term even when an Extension Term is defined. Previously, the presence of an Extension Term signalled a perpetually-renewable subscription whose end date should not be pinned to the initial term.

Recommendation:

• Reintroduce the early-exit guard (or replace it with a deliberate comment explaining why auto-renewing subscriptions should now receive a fixed end date) to avoid unintentionally terminating ongoing subscriptions.

if IsInitialTermEmpty() then
    exit;
if not IsExtensionTermEmpty() then
    exit;
TestField("Subscription Line Start Date");
"Subscription Line End Date" := CalcDate("Initial Term", "Subscription Line Start Date");

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

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

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

CalcTermUntil drops Extension Term logic entirely

The removed code used Extension Term + Initial Term (or Extension Term + Notice Period) to determine Term Until when the subscription end date is zero. The new code only considers Notice Period, so subscriptions with an Extension Term but no Notice Period will no longer get a Term Until value, and subscriptions that previously derived their renewal term from the Initial Term will now derive it from the Notice Period — a silent behavioral change.

Recommendation:

• Audit the intended semantics of Term Until for each combination of Initial Term / Extension Term / Notice Period and reinstate the correct branch logic, or add explicit tests documenting the new behavior.

// Restore Extension Term handling or document why it is intentionally removed:
if not IsExtensionTermEmpty() then begin
    if not IsInitialTermEmpty() then begin
        "Term Until" := CalcDate("Initial Term", "Subscription Line Start Date");
        "Term Until" := CalcDate('<-1D>', "Term Until");
    end else
        if not IsNoticePeriodEmpty() then begin
            "Term Until" := CalcDate("Notice Period", "Subscription Line Start Date");
            "Term Until" := CalcDate('<-1D>', "Term Until");
        end;
end;

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

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

[miljance]

"Term Until" := CalcDate("Initial Term", "Subscription Li

resolved with rebase.

[miljance]

🟠 High Severity — Upgrade Iteration 1

CalcTermUntil drops Extension Term logic entirely

The removed code used Extension Term + Initial Term (or Extension Term + Notice Period) to determine Term Until when the subscription end date is zero. The new code only considers Notice Period, so subscriptions with an Extension Term but no Notice Period will no longer get a Term Until value, and subscriptions that previously derived their renewal term from the Initial Term will now derive it from the Notice Period — a silent behavioral change.

Recommendation:

• Audit the intended semantics of Term Until for each combination of Initial Term / Extension Term / Notice Period and reinstate the correct branch logic, or add explicit tests documenting the new behavior.

// Restore Extension Term handling or document why it is intentionally removed:
if not IsExtensionTermEmpty() then begin
    if not IsInitialTermEmpty() then begin
        "Term Until" := CalcDate("Initial Term", "Subscription Line Start Date");
        "Term Until" := CalcDate('<-1D>', "Term Until");
    end else
        if not IsNoticePeriodEmpty() then begin
            "Term Until" := CalcDate("Notice Period", "Subscription Line Start Date");
            "Term Until" := CalcDate('<-1D>', "Term Until");
        end;
end;

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

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

resolved with rebase.

[alexei-dobriansky]

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR adds performance changes for Subscription Billing proposal creation, billing document creation, and usage data processing. It adds progress tracking, new keys, temp-table aggregation, narrower field loading, bulk updates, and new commit checkpoints.

The direction matches the reported large-data problem in GitHub issue #7690, and the BaseApp publishers used by the new manual subscribers exist in the W1 BaseApp. However, the diff also changes functional behavior in sensitive paths: document line price/cost/UoM events are bypassed, usage-data links are moved in bulk, and commit boundaries change. Those changes need stronger proof that billing amounts, usage links, and extension-visible behavior stay correct.

Suggestions

S1 - Add functional regression tests
No test files changed, but this PR changes billing document creation, usage-data linking, and commit behavior. Add tests that create customer and vendor billing documents, including usage-based lines, and verify amounts, document links, and usage-data links. This area is financially sensitive and the existing Subscription Billing tests make this feasible.

S2 - Do not bypass pricing hooks unconditionally
Billing Price Calc. Skip sets handled on sales and purchase price, cost, and UoM events while lines are initialized. This can change results for subscribers that adjust billing document price, cost, quantity, or UoM. Add a safe opt-out or integration point, or add tests that prove supported billing results stay the same.

S3 - Link the internal bug work item
The PR body links GitHub issue #7690 but no AB# work item was found, and the internal work-item check is failing. Add the AB# link so reviewers can verify the approved bug and the detailed repro context. This matters because the change is broad for a bug fix.

Risk assessment and necessity

Risk: The regression surface is high because the PR touches billing proposal creation, sales and purchase billing document lines, usage-data billing links, and transaction boundaries. The new BaseApp event subscribers were checked against W1 BaseApp publishers, but setting handled on those events changes extension-visible behavior during line initialization. The PR also has no new functional tests and no linked ADO work item was available for [AI-REPRO] verification.

Necessity: The scenario is important: issue #7690 describes billing runs over about 4 million subscription lines with no progress feedback and poor recoverability. Performance and progress improvements are justified, but the scope is broad and should be backed by tests and the internal bug link before merge.

---

[AI-PR-REVIEW] version=1 system=github pr=8553 round=1 by=alexei-dobriansky at=2026-07-02 lastSha=db26bceeee8e950e9abe031645fe49aed279ef46 suggestions=S1,S2,S3

[github-actions]

Copilot PR Review

Iteration 2 · Outcome: completed

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

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
Accessibility	1	0	1	1	0
Error Handling	1	0	1	1	0
Events	1	1	0	1	0
Performance	2	2	0	1	1
Privacy	2	2	0	0	2
Security	1	0	1	1	0

Totals: 5 knowledge-backed · 3 agent findings.

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.}

[github-actions]

⚠️ 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.

[alexei-dobriansky]

Agentic PR Review - Round 2

Recommendation: Request Changes

What this PR does

The commit since round 1 addresses some cleanup feedback: it adds the AB# link, moves labels to object scope, restores several procedures to internal, and replaces the temporary Indent reuse with a dictionary for billing-line links.

Those changes are good, but they do not close the two main functional risks from round 1. The PR still changes billing document creation, usage-data linking, pricing/cost event handling, and commit boundaries without added functional regression tests. It also still sets handled on BaseApp price, cost, and UoM events while billing lines are initialized, so subscribers that depend on those events can still be bypassed.

Status of previous suggestions

ID	Title	Status	Author response
S1	Add functional regression tests	Not addressed	No author reply. The round-2 diff does not add test files, and the full PR still has no test-file changes.
S2	Do not bypass pricing hooks unconditionally	Not addressed	No author reply. Billing Price Calc. Skip still handles the sales and purchase price, cost, and UoM events during line initialization.
S3	Link the internal bug work item	Addressed	The PR body now links AB#641986.

New observations (commits since round 1)

None - the new commit mostly addresses cleanup feedback and does not introduce a separate new concern.

Risk assessment and necessity

Risk: The main risk is still high because this affects billing proposal creation, customer and vendor billing documents, usage-data billing links, document amounts, and transaction boundaries. The round-2 dictionary change is safer than reusing Indent, and restoring internal visibility reduces public API risk, but the open test and event-subscriber risks remain.

Necessity: The performance scenario is valid and important. GitHub issue #7690 describes billing runs with about 4 million subscription lines, long-running proposal and document creation, no progress feedback, and rollback after failures. The fix is worth doing, but this broad billing change still needs functional proof before merge.

---

[AI-PR-REVIEW] version=1 system=github pr=8553 round=2 by=alexei-dobriansky at=2026-07-13T17:06:07Z lastSha=b09468315ece3e17ba87c18943e8db768a0718d5 suggestions=S1:notaddressed,S2:notaddressed,S3:addressed parentRound=1

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Events} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

BillingPriceCalcSkip is a manually bound subscriber held in a codeunit-global variable, and both line-building paths do several Validate/TestField calls before UnbindSubscription.

If any of those calls errors, the explicit unbind is skipped and the manual subscriber can stay bound longer than the intended scope, so later unrelated Sales/Purchase Line validations in the same session could still bypass the standard price/cost logic. Move the subscriber instance into a local scope, or a helper that guarantees scope-based cleanup, so the binding cannot leak on error.

Knowledge:

• microsoft/knowledge/events/choose-static-vs-manual-subscribers-deliberately.md

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

[miljance]

Later unrelated Sales/Purchase Line validations in the same session could still bypass the standard price/cost logic - that sound like a danger. I need to investigate.

[github-actions]

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

CreateTempBillingLines iterates Billing Line with FindSet() and then does CustomerContract.Get(...)/VendorContract.Get(...) inside the loop, only reusing the immediately previous contract number.

Because the outer scan is not keyed/ordered by contract number, the same contract can be re-fetched whenever its rows are non-adjacent, leaving an N+1 lookup pattern in this hot aggregation path.

Knowledge:

• microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md
• microsoft/knowledge/performance/setcurrentkey-aligns-key-with-filters.md
• microsoft/knowledge/performance/use-setloadfields-for-partial-records.md

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

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

[github-actions]

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

The changed Normal table field "Subscription Line Entry No." still relies on the table-level DataClassification instead of declaring its own.

DataClassification should be declared explicitly on each Normal field so the field remains correctly classified independent of future table-level changes and passes AS0016 validation.

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

        field(31; "Subscription Line Entry No."; Integer)
        {
            Caption = 'Subscription Line Entry No.';
            DataClassification = CustomerContent;
            TableRelation = "Subscription Line"."Entry No.";
        }

Knowledge:

• microsoft/knowledge/privacy/table-level-data-classification-cascades.md

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

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

[github-actions]

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

The changed "Billing Line Entry No." field on Usage Data Billing still relies on the table-level DataClassification instead of declaring its own.

DataClassification should be declared explicitly on each Normal field so the field remains correctly classified and passes AS0016 validation.

Knowledge:

• microsoft/knowledge/privacy/table-level-data-classification-cascades.md

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

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

The new "Billing Price Calc.

Skip" helper is a public, manually bindable subscriber that unconditionally sets Handled/IsHandled := true on Sales Line and Purchase Line validation hooks (unit price, unit cost, UoM quantity conversion). Any dependent extension can instantiate this codeunit and bind it around its own line creation to suppress unit price, unit cost, and UoM validation outside the intended billing flow. Consider keeping this bypass internal to the Subscription Billing app and guarding it so only the intended document-creation path can activate it.

_{Agent judgement — not directly backed by a BCQuality knowledge article.}

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

[miljance]

🟠 High Severity — Performance Iteration 2

CreateTempBillingLines iterates Billing Line with FindSet() and then does CustomerContract.Get(...)/VendorContract.Get(...) inside the loop, only reusing the immediately previous contract number.

Because the outer scan is not keyed/ordered by contract number, the same contract can be re-fetched whenever its rows are non-adjacent, leaving an N+1 lookup pattern in this hot aggregation path.

Knowledge:

• microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md
• microsoft/knowledge/performance/setcurrentkey-aligns-key-with-filters.md
• microsoft/knowledge/performance/use-setloadfields-for-partial-records.md

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

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

I agree that adding a key makes sense, will do that.

[miljance]

🟠 High Severity — Privacy Iteration 2

The changed Normal table field "Subscription Line Entry No." still relies on the table-level DataClassification instead of declaring its own.

DataClassification should be declared explicitly on each Normal field so the field remains correctly classified independent of future table-level changes and passes AS0016 validation.

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

        field(31; "Subscription Line Entry No."; Integer)
        {
            Caption = 'Subscription Line Entry No.';
            DataClassification = CustomerContent;
            TableRelation = "Subscription Line"."Entry No.";
        }

Knowledge:

• microsoft/knowledge/privacy/table-level-data-classification-cascades.md

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

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

This is int the context of this change. All fields on the table do not have DataClassification

[miljance]

🟠 High Severity — Privacy Iteration 2

The changed "Billing Line Entry No." field on Usage Data Billing still relies on the table-level DataClassification instead of declaring its own.

DataClassification should be declared explicitly on each Normal field so the field remains correctly classified and passes AS0016 validation.

Knowledge:

• microsoft/knowledge/privacy/table-level-data-classification-cascades.md

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

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

irrelevant for the context of the changes.