Skip to content

[28.x] Subscription Billing: O(N²) contract-renewal detection in Sales-Post subscribers slows posting (~28% of batch CPU) - #9567

Open
Andreas Hansen (AndreasHans) wants to merge 2 commits into
releases/28.xfrom
bugs/642309
Open

[28.x] Subscription Billing: O(N²) contract-renewal detection in Sales-Post subscribers slows posting (~28% of batch CPU)#9567
Andreas Hansen (AndreasHans) wants to merge 2 commits into
releases/28.xfrom
bugs/642309

Conversation

@AndreasHans

@AndreasHans Andreas Hansen (AndreasHans) commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Fixes AB#642309

@AndreasHans
Andreas Hansen (AndreasHans) requested a review from a team July 17, 2026 08:23
@AndreasHans
Andreas Hansen (AndreasHans) requested a review from a team as a code owner July 17, 2026 08:23
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 Finance GitHub request for Finance area labels Jul 17, 2026
@github-actions github-actions Bot modified the milestone: Version 28.4 Jul 17, 2026
@github-actions

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.

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR replaces repeated SalesLine.IsContractRenewal() and SalesHeader.HasOnlyContractRenewalLines() calls in Sales-Post subscribers with cached wrappers, and clears the caches on OnBeforePostSalesDoc and OnAfterPostSalesDoc. The change targets the AB#642309 performance issue and should remove repeated per-line full-document scans. I verified the new subscribers hook into existing BaseApp publisher events (SalesPost.Codeunit.al in src/Layers/*/BaseApp/Sales/Posting) and the null-SystemId fallback keeps behavior for temporary buffers.

Suggestions

S1 - Add regression test for cache reset boundaries
Please add a Contract Renewal test that posts multiple documents in one run and verifies normal lines are still posted correctly after a renewal document. This protects the new cache-reset logic and prevents stale cache values from changing posting behavior in future refactors.

Risk assessment and necessity

Risk: The touched path is Sales posting, so a cache mistake can silently skip invoice/shipment line insertion or header creation. The code is targeted to one codeunit, but the event subscribers are in a financially sensitive flow.

Necessity: The work item describes a real and costly performance problem (~25-28% of batch CPU in renewal detection), so optimization is justified. Without this change, large invoice batch posting remains unnecessarily slow and has no practical workaround.


[AI-PR-REVIEW] version=1 system=github pr=9567 round=1 by=alexei-dobriansky at=2026-08-04T06:05:13Z lastSha=56f824c75bf57caa9805db7f80429923118e9798 suggestions=S1

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

Labels

AL: Apps (W1) Add-on apps for W1 Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants