[Subcontracting] Bug 638464: Remove Subc. Detailed Calculation report; extend BaseApp report via event

[ChethanT]

Summary

• Report 99001500 "Subc. Detailed Calculation" was a full copy of BaseApp Report 99000756 "Detailed Calculation" with a no-op report substitution subscriber
• This caused duplicate Tell Me entries, lacked extensibility events, and the substitution never actually fired

Root Cause

\SubcReportingTriggersExt\ subscribed to \SubstituteReport\ but checked \if ReportId = Report::"Subc. Detailed Calculation"\ and set \NewReportId\ to itself (99001500 → 99001500). This was a no-op — it should have been substituting the BaseApp report (99000756).

Fix

1. BaseApp (separate NAV commit): Added integration event \OnAfterGetRecordRoutingLineOnBeforeCalcRoutingCostPerUnit\ to Report 99000756's Routing Line \OnAfterGetRecord\ trigger
2. Subcontracting app (this PR):
   • Removed Report 99001500 and its RDLC layout
   • Rewrote \SubcReportingTriggersExt\ to subscribe to the new BaseApp event and apply subcontractor pricing via \SubcPriceManagement.SetRoutingPriceListCost\
   • Removed the report from the permission set

Test

Added \DetailedCalculationReportUsesSubcontractorPricing\ in codeunit 139982 "Subc. Pricing Test" — [SCENARIO 638464].

Fixes AB#638464

🤖 Generated with GitHub Copilot

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Deleted report still referenced; build fails

The SubstituteDetailedCalculation event subscriber (line 29–30) sets NewReportId := Report::"Subc. Detailed Calculation", but that report object (99001500) is deleted in this very PR. The AL compiler will fail with an unresolved identifier error, blocking every build that includes the Subcontracting app.

Recommendation:

• Remove the SubstituteDetailedCalculation procedure entirely from this codeunit. The substitution is no longer needed because the custom report has been replaced by the new OnAfterGetRecordRoutingLineOnBeforeCalcRoutingCostPerUnit event subscriber that directly modifies the base report's cost calculation.

// Remove the entire SubstituteDetailedCalculation procedure:
// [EventSubscriber(ObjectType::Codeunit, Codeunit::"Reporting Triggers", SubstituteReport, '', false, false)]
// local procedure SubstituteDetailedCalculation(...)
// ...
// end;

// Also remove the now-unused 'using Microsoft.Manufacturing.Reports;' import if nothing else in the codeunit needs it.

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

Permission for deleted report removed but D365 READ not updated

The SubcontractObjs.PermissionSet.al correctly drops the report "Subc. Detailed Calculation" = X entry, but D365ReadSubcontracting.PermissionSetExt.al (and any other permission set extensions not checked here) may still carry a reference to this report. If any role-based permission set outside this diff references the report by name, it will fail to compile.

Recommendation:

• Audit all permission set files in the Subcontracting app for any remaining reference to report "Subc. Detailed Calculation" and remove them. Run grep -rn '"Subc. Detailed Calculation"' src/Apps/W1/Subcontracting/ to confirm no other sets need updating.

// Verify no other permission set contains:
// report "Subc. Detailed Calculation" = X,

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

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Dead reference to deleted report causes compile error

The SubstituteDetailedCalculation procedure still assigns NewReportId := Report::"Subc. Detailed Calculation", but that report object (99001500) is deleted in this same PR. AL resolves report names to IDs at compile time, so this will produce an unresolved-reference compilation error and prevent the app from building.

Recommendation:

• Remove the entire SubstituteDetailedCalculation event-subscriber procedure (and its associated #if not CLEAN29 feature-flag block) from the codeunit. The new OnAfterGetRecordRoutingLineOnBeforeCalcCost event subscriber on the base "Detailed Calculation" report replaces the old substitution pattern; the substitution procedure is no longer needed and must not be left behind.

// Remove the SubstituteDetailedCalculation procedure entirely:
// [EventSubscriber(ObjectType::Codeunit, Codeunit::"Reporting Triggers", SubstituteReport, '', false, false)]
// local procedure SubstituteDetailedCalculation(ReportId: Integer; var NewReportId: Integer)
// ... (delete this whole block)

// Keep only the new event subscriber:
[EventSubscriber(ObjectType::Report, Report::"Detailed Calculation", OnAfterGetRecordRoutingLineOnBeforeCalcRoutingCostPerUnit, '', false, false)]
local procedure OnAfterGetRecordRoutingLineOnBeforeCalcCost(...)
...

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

Test runs deleted report via still-active substitution

The test declares DetailedCalculation: Report "Detailed Calculation" and calls SaveAsXml, but while SubstituteDetailedCalculation remains in the codeunit, BC will redirect execution to the now-deleted report 99001500 at runtime, making the test fail before it can validate the new event-subscriber behavior.

Recommendation:

• Fixing the critical compilation issue (removing SubstituteDetailedCalculation) will also unblock this test. Once the substitution is removed, the test correctly exercises the base report with the new event subscriber. No changes needed in the test itself beyond that prerequisite fix.

// No code change needed in the test file.
// Prerequisite: remove SubstituteDetailedCalculation from SubcReportingTriggersExt.Codeunit.al
// so the base Report "Detailed Calculation" runs and fires the new event subscriber.

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

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Reference to deleted report causes compile error

The SubstituteDetailedCalculation subscriber still references Report::"Subc. Detailed Calculation" (99001500) on line 30, which is deleted in this PR. This reference is outside any #if not CLEAN29 guard and will cause a compilation error in all build configurations.

Recommendation:

• Remove the entire SubstituteDetailedCalculation event subscriber procedure. The redirect is no longer needed since the new OnAfterGetRecordRoutingLineOnBeforeCalcCost event subscriber on the base "Detailed Calculation" report provides the subcontracting pricing hook directly.

// Remove the SubstituteDetailedCalculation procedure entirely:
// [EventSubscriber(ObjectType::Codeunit, Codeunit::"Reporting Triggers", SubstituteReport, '', false, false)]
// local procedure SubstituteDetailedCalculation(...)
// ... (delete lines 14–31)

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

Redirect subscriber conflicts with new event approach

With the base report's OnAfterGetRecordRoutingLineOnBeforeCalcRoutingCostPerUnit event now handling subcontractor pricing, the SubstituteReport subscriber that redirects "Detailed Calculation" to the deleted "Subc. Detailed Calculation" is both broken and architecturally inconsistent. Even if the compile error were patched, users would be redirected to a non-existent report at runtime.

Recommendation:

• Delete the SubstituteDetailedCalculation procedure and its associated #if not CLEAN29 feature-flag guards. Also remove the using Microsoft.Manufacturing.Reports; import if it is no longer needed by the remaining subscriber (verify whether the Report::"Detailed Calculation" reference in the new event attribute still requires it).

// Entire SubstituteDetailedCalculation procedure should be deleted.
// The codeunit should only contain the new OnAfterGetRecordRoutingLineOnBeforeCalcCost subscriber.

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

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

[github-actions]

Copilot PR Review

Iteration 3 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

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
Agent	1	0	1	1	0

Totals: 0 knowledge-backed · 1 agent findings.

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

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

_{Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.}

[github-actions]

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

The new test procedure DetailedCalculationReportUsesSubcontractorPricing calls Commit() explicitly (line 730) but carries no [TransactionModel(TransactionModel::AutoCommit)] attribute.

Per the referenced guidance, when the test code itself issues Commit(), the test method must declare TransactionModel = AutoCommit; applying the default AutoRollback to a test that calls Commit() produces a runtime error on the first Commit, leaving no business-logic verdict. Add [TransactionModel(TransactionModel::AutoCommit)] immediately before [Test] on this procedure. Because this commits data to the database, pair the test codeunit with a TestIsolation-enabled test runner so committed changes are reverted at the suite level.

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

    [TransactionModel(TransactionModel::AutoCommit)]
    [Test]

Knowledge:

• microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md

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

[github-actions]

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

The event subscriber OnAfterGetRecordRoutingLineOnBeforeCalcCost sets IsHandled := true at the end but never checks if IsHandled then exit; at the start.

In the BC IsHandled pattern, a subscriber that does not guard on entry will overwrite the work of any earlier subscriber that already set IsHandled := true — for example, a partner extension or future Microsoft extension that also customises routing-cost calculation for subcontractor work centres. Add if IsHandled then exit; as the very first statement in the procedure body to respect the contract.

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

        if IsHandled then
            exit;

        if RoutingLine.Type <> RoutingLine.Type::"Work Center" then
            exit;

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