[Bug 620643] Fix deletion of Subcontractor Prices when Work Center or Item is deleted

[ChethanT]

Summary

• SetCurrentKey("Work Center No.") in SubcWorkCenterExtension.Codeunit.al and SetCurrentKey("Item No.") in SubcItemExtension.Codeunit.al failed at runtime because neither field was the first field of any existing key on the SubcontractorPrice table.
• This caused a runtime error whenever a Work Center or Item with associated Subcontractor Prices was deleted, preventing the deletion from completing.
• Added secondary keys Key03 ("Work Center No.") and Key04 ("Item No.") to SubcontractorPrice.Table.al so the SetCurrentKey calls resolve correctly and cascaded deletion works as intended.

Root cause

The SubcVendorExtension correctly calls SetCurrentKey("Vendor No.") because "Vendor No." is the first field of the clustered PK. The other two extensions used fields that are not the first field of any key:

Codeunit	SetCurrentKey call	Was valid?
SubcVendorExtension	"Vendor No."	✅ First field of PK
SubcWorkCenterExtension	"Work Center No."	❌ No key starts with this field
SubcItemExtension	"Item No."	❌ No key starts with this field

Fix

Added two secondary keys to SubcontractorPrice.Table.al:

• key(Key03; "Work Center No.")
• key(Key04; "Item No.")

Tests added

• DeleteWorkCenterWithPricesDeletesRelatedPrices (codeunit 139989) — creates a work center with multiple subcontractor prices, deletes the work center, asserts all prices are deleted
• DeleteItemWithPricesDeletesRelatedPrices (codeunit 139989) — creates an item with multiple subcontractor prices, deletes the item, asserts all prices are deleted

Fixes AB#620643

🤖 Generated with Claude Code

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• Subcontracting-Test: 0% documentation coverage
• Subcontracting: 0% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[alexei-dobriansky]

Analysis

Correctness:

• The three new helper procedures (DeletePricesForVendor, DeletePricesForWorkCenter, DeletePricesForItem in SubcontractorPrice.Table.al:171/179/187) faithfully reproduce the prior inline logic — same SetCurrentKey → SetRange → IsEmpty short-circuit → DeleteAll(true) order. No semantic divergence from main.
• After the fix, the two new keys Key03 ("Work Center No.") and Key04 ("Item No.") make the SetCurrentKey calls in DeletePricesForWorkCenter and DeletePricesForItem valid. DeletePricesForVendor calls SetCurrentKey("Vendor No.") which was already valid via the PK (PK leads with "Vendor No."), so this call is redundant but harmless — kept for symmetry with the other two helpers.
• DeleteAll(true) runs the table's own OnDelete triggers (none on Subcontractor Price) and any subscribers' OnAfterDeleteEvent — same behavior as before.
• Rec.IsTemporary() and RunTrigger guards are preserved in all three subscribers.

Side Effects:

• No public API surface change. All three new procedures are internal, so they're callable only inside the Subcontracting module.
• Two new SQL indexes (Key03, Key04) ship to every tenant on every Subcontractor Price table. For a typically tiny pricing table this is negligible storage/write overhead, but it is a permanent addition. The simpler alternative — removing the SetCurrentKey(...) calls (or keeping them only where they target an existing key) — would have avoided adding indexes. Adding the keys is defensible (they document the intended access pattern and accelerate filtered reads beyond the cascade-delete path), but the PR description should justify it as a deliberate choice rather than a forced consequence.
• Order of deletion is unchanged. Vendor.Delete(true) → OnAfterDeleteEvent → DeletePricesForVendor works exactly as before, just routed through the helper.

Risk Assessment: Low.

• Single module (Subcontracting), no posting/financial logic touched.
• No event-publisher changes; no BaseApp coupling.
• No Sales/Purchase/Service analog to keep in sync — this is a Subcontracting-internal cleanup.
• What if we don't ship? If SetCurrentKey raises a runtime error on main (the PR description's claim), then Work Center.Delete(true) and Item.Delete(true) are broken whenever associated prices exist. Workaround: manually delete prices first, then the master record. If the runtime-error claim is not accurate (AL silently falls back), the PR still delivers the DRY refactor that the linked bug actually requested. Either way the change is net-positive.

Test Coverage:

• DeleteWorkCenterWithPricesDeletesRelatedPrices and DeleteItemWithPricesDeletesRelatedPrices exercise the two refactored event subscribers end-to-end. Both create two prices to verify the deletion is not accidentally narrowed to a single record. Assertions are at the right level — they query the table after deletion and assert empty, which directly tests the delete-cascade outcome.
• Gap: no test for Vendor.Delete(true). SubcVendorExtension.Codeunit.al was refactored to call the new DeletePricesForVendor helper, but no test exercises that path. By symmetry with the other two, a DeleteVendorWithPricesDeletesRelatedPrices test should exist. Without it, a regression in that subscriber would not be caught.
• Tests do not assert that the price records actually existed before the delete (i.e., that CreateSubContractingPrice succeeded and inserted two records). A bad test setup that silently produces zero records would pass these tests vacuously. A pre-condition Assert.AreEqual(2, SubcontractorPrice.Count(), '…') between creation and the delete would harden the tests.

Recommendation: Accept with Suggestions

The fix is correct, low-risk, and well-localized. It satisfies the DRY intent of the linked work item and additionally hardens two SetCurrentKey calls that are at minimum poorly-formed and at worst runtime-broken. Suggestions:

1. Add a test for the Vendor deletion path — DeleteVendorWithPricesDeletesRelatedPrices, symmetric to the two new tests. The Vendor codeunit was refactored in this PR; without a regression test, a future change to that subscriber could silently break price cleanup.
2. Harden the new tests with a pre-condition assertion — Assert.AreEqual(2, SubcontractorPrice.Count(), 'Setup must create 2 prices') between the CreateSubContractingPrice calls and the Delete(true), so the tests cannot pass vacuously.
3. Reconcile the PR description with the linked bug — AB#620643 is filed as a DRY-refactor task and the [AI-REPRO] comment explicitly notes "code quality/architecture issue, not a functional defect." The PR description introduces a runtime-error narrative that is not in the bug. Either update the bug with the runtime symptom (so the work item reflects what's being fixed), or rephrase the PR summary to lead with the DRY motivation and treat the SetCurrentKey remediation as a secondary cleanup.
4. (Minor) Drop or comment the redundant SetCurrentKey("Vendor No.") in DeletePricesForVendor — that field is already the PK's leading field, so the call is a no-op. Either remove it or add a one-line comment that it's kept for symmetry.
5. (Minor) Justify the two new keys in the PR description — they ship to every tenant. A sentence stating that Key03/Key04 are added to support the documented filter patterns (not just to silence a runtime error) makes the choice intentional. The alternative — dropping SetCurrentKey from the helpers — is also valid; calling out why keys were preferred would help future maintainers.