[Bug][Subscription Billing] "Delete Invoiced Sales Orders" leaves orphaned Sales Subscription Lines behind - #10210
Conversation
…d-order deletion reports Reports 299 "Delete Invoiced Sales Orders" and 291 "Delete Invd Blnkt Sales Orders" remove sales lines with Delete() and DeleteAll(), which default to RunTrigger = false, so the OnDelete() trigger of tableextension 8054 "Sales Line" never fires. The Sales Subscription Lines of the deleted document survive as orphans pointing at a document that no longer exists, and nothing in the UI can reach them. Add an event subscriber for each report to codeunit 8069, following the per-path pattern the app already uses for Sales-Post and Sales-Quote to Order. Report 299 hooks OnAfterDeleteSalesLinesLoop rather than OnBeforeDeleteSalesHeader, so the subscription lines are cleaned up on the branch where an unassigned Charge (Item) line keeps the order alive after its item lines have been deleted. Consolidate the four trigger-less deletion paths on a new SalesHeader.DeleteSalesServiceCommitments() in tableextension 8053, which batch-deletes the subscription lines of a whole document. It deliberately does not filter on document type: Copy Document carries Sales Subscription Lines over to invoices and credit memos, and those still have to be removed when the document is posted. Both deletion procedures are public rather than internal. This is intentional - customers already need to call the deletion logic from outside the app, so they are exposed as a supported entry point and documented with XML comments. Tests cover both reports and posting an invoice that carries subscription lines copied from an order. The two report tests use TransactionModel::AutoCommit because both reports commit unconditionally. Fixes microsoft#10175 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
Agentic PR Review - Round 1Recommendation: AcceptWhat this PR doesThis PR fixes orphaned Sales Subscription Lines when report 299 deletes invoiced sales orders and report 291 deletes invoiced blanket sales orders without running Sales Line delete triggers. It also routes the existing Sales-Post and Sales-Quote-to-Order cleanup through a shared Sales Header procedure. The change matches the root cause: these paths delete lines with Delete() or DeleteAll(false), so the Sales Line tableextension OnDelete() cleanup does not run. I verified the BaseApp publishers and timing: report 299 raises OnAfterDeleteSalesLinesLoop after the line loop and before possible header deletion, report 291 raises OnBeforeDeleteSalesHeader after line DeleteAll, and Sales-Post passes the header before SalesLine.DeleteAll. The new tests cover the sales order report, the blanket order report, the copied invoice posting path, and that another document is not over-deleted. SuggestionsNone. Risk assessment and necessityRisk: This is a data cleanup path, so the main risk is deleting too many Sales Subscription Lines or missing one of the trigger-less delete flows. The shared procedure filters by document type and document number, and the tests include a second document guard, which keeps the cleanup scoped. The public procedure exposure is additive; there is no schema, upgrade, permission, or telemetry change. Necessity: The issue is valid and important because the current batch job can leave live subscription records that point to a deleted document and need a data fix. The scope is right: it uses existing BaseApp events, keeps the cleanup in the Subscription Billing app, and adds focused regression tests for the affected paths.
|
3 participants
What & why
Reports 299 "Delete Invoiced Sales Orders" and 291 "Delete Invd Blnkt Sales Orders" remove sales
lines with
Delete()andDeleteAll(), which default toRunTrigger = false. TheOnDelete()trigger of tableextension 8054 "Sales Line" therefore never fires, and the Sales Subscription Lines
of the deleted document survive as orphans pointing at a document number that no longer exists.
Users see the "Sales Subscription Lines" list keep growing with entries for orders nobody can open
any more, and nothing in the UI can clean them up — only a data fix can.
This hits Subscription Billing users especially often, because Subscription Items force
Qty. to Invoice = 0, soEverythingInvoicedis never true andSales-Postdoes not auto-deletethe order. The order is left fully shipped and invoiced — exactly report 299's scope.
The issue reports report 299. Report 291 is included because it is the identical defect on the
blanket-order path:
SalesBlanketOrderLine.DeleteAll()is equally trigger-less, and "Blanket Order"is one of the document types in
IsSalesDocumentTypeWithServiceCommitments(). It was confirmed by afailing test before the fix, not assumed.
Changes:
pattern the app already uses for
Sales-PostandSales-Quote to Order. No Base Applicationchange is needed; both reports already publish suitable integration events.
OnAfterDeleteSalesLinesLoop, notOnBeforeDeleteSalesHeader. The latter firesonly when the header is deleted, which would leak the subscription lines on the branch where an
unassigned
Charge (Item)line keeps the order alive after its item lines were already removed.Sales-Post,Sales-Quote to Order) now sharea new
SalesHeader.DeleteSalesServiceCommitments()on tableextension 8053, which batch-deletes awhole document's subscription lines. It lives on the header because
Document Type+No.is theheader's primary key and all four call sites already hold a
Sales Header.source line and stamps the target's document type unchecked, so subscription lines can legitimately
exist on invoices and credit memos and must still be cleaned up on posting.
SalesLine.DeleteSalesServiceCommitment()andSalesHeader.DeleteSalesServiceCommitments()arepublic rather than internal, and carry XML documentation. This is intentional: customers already
need to call the deletion logic from outside the app, so both are exposed as a supported entry point.
Linked work
Fixes #10175
How I validated this
What I tested and the outcome
Built with
alc.exe(CodeCop, UICop, PerTenantExtensionCop,src/rulesets/base.ruleset.json) andpublished to a BC 29 container. The only warning is pre-existing
AA0139in an untouched file.Three new tests in codeunit 139915 "Sales Service Commitment Test":
CheckDeleteSalesServiceCommitmentOnDeleteInvoicedSalesOrders— order with a Subscription Item,fully shipped and invoiced, still open; run report 299; order gone and no subscription line left.
CheckDeleteSalesServiceCommitmentOnDeleteInvdBlnktSalesOrders— blanket order with two lines,converted to an order that is then fully posted; run report 291; same assertions.
CheckDeleteSalesServiceCommitmentOnPostSalesInvoiceCopiedFromSalesOrder— invoice created from anorder by Copy Document; posting it removes the copied subscription lines. This path had no coverage
and is what proves the shared procedure must not filter on document type.
Both report tests carry
[TransactionModel(TransactionModel::AutoCommit)]because reports 299 and 291commit unconditionally.
Each test was confirmed red before the fix and green after. The fix was then mutation-tested by
rebuilding, republishing and re-running: removing either subscriber reddens only its own test;
weakening the shared filter to document-type-only reddens both new tests and the pre-existing
SalesServiceCommitmentMakeOrderFromQuote; dropping the second blanket-order line's cleanup reddensthe blanket test. Both new tests also assert that a second, untouched document keeps its subscription
lines, so an over-deleting filter cannot pass.
Suites run green on the container: codeunit 139915 (69/69), 139687 "Recurring Billing Docs Test"
(88/88, covering the invoice and credit-memo posting paths that share the refactored procedure), and
139916 "Service Comm. Archive Test" (5/5).
Risk & compatibility
contract, by design (see above).
DeleteSalesServiceCommitment()was previouslyinternal.Sales-PostandSales-Quote to Ordersubscribers now routethrough the shared procedure. Behaviour is unchanged apart from the
Sales-Postsubscriber no longerneeding
SalesLine.FindFirst()to recover the document key, since the publisher passes the header."Sales Subscription Line Archive" entries are written as before. Only the live table 8068 was leaking.
Locked = truetest assertion messages.