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{🟠\ High\ Severity\ —\ Security} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Base report not added to permission set

The custom Subc. Detailed Calculation report (99001500) was removed from the permission set without adding the base Detailed Calculation report (99000756) in its place. Users assigned only the Subcontracting permission set will be unable to run the Detailed Calculation report after this change.

Recommendation:

• Add report "Detailed Calculation" = X to the permission set so subcontracting users retain access to the replaced report.

Suggested change

	report "Subc. Detailed Calculation" = X,
	report "Subc. Create SubCReturnOrder" = X,
	report "Detailed Calculation" = X,
	report "Subc. Dispatching List" = X;

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

[github-actions]

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

IsHandled guard missing in event subscriber

The event subscriber does not check whether IsHandled is already true before executing. If another extension has already handled the event, this code will still run and override the earlier handler's result, silently suppressing its work.

Recommendation:

• Add an early exit at the start of the procedure when IsHandled is already true.

Suggested change

	IsHandled := true;
	begin
	if IsHandled then
	exit;
	if RoutingLine.Type <> RoutingLine.Type::"Work Center" then
	exit;

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Test missing negative/fallback coverage

The new test only verifies the happy path where a subcontractor price exists and is picked up. There is no test case verifying that when a work center has a Subcontractor No. but no matching subcontractor price record exists, the report correctly falls back to the work center's own direct unit cost rather than outputting zero or an uninitialized value.

Recommendation:

• Add a second test procedure that creates a work center with a subcontractor but no SubcontractorPrice record, and asserts that ProdUnitCost equals the work center's Direct Unit Cost.

Suggested change

	DetailedCalculation.SetTableView(Item);
	[Test]
	procedure DetailedCalculationReportFallsBackToWorkCenterCostWhenNoPriceExists()
	// ... arrange: work center with Subcontractor No., no SubcontractorPrice record
	// ... assert: ProdUnitCost = WorkCenterDirectCost
	begin
	end;

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

WorkCenter.Get called without prior type guard on routing line

The guard if RoutingLine.Type <> RoutingLine.Type::"Work Center" then exit; is correct, but WorkCenter.Get is then called unconditionally for every routing line in the report dataset. In a production routing with many work-center operations, each line triggers an extra database read even for work centers with no subcontractor configured.

Recommendation:

• Cache the last-retrieved work center number outside the procedure or use CalcFields selectively. At minimum ensure the existing guard at line 46 (WorkCenter."Subcontractor No." = '') short-circuits quickly, which it does — the current code is acceptable but should be noted for routing headers with many lines.

Suggested change

	if not WorkCenter.Get(RoutingLine."Work Center No.") then
	// Current pattern is acceptable. If performance is a concern for large routings,
	// consider caching WorkCenter lookups by "Work Center No." using a temporary record or dictionary.

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

[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{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Hardcoded QtyUoM/ProdQtyPerUom/QtyBase bypass UOM conversion

The call to SetRoutingPriceListCost passes 1, 1, 1 for QtyUoM, ProdQtyPerUom, and QtyBase. Inside that codeunit these values drive the unit-of-measure price conversion; hardcoding them to 1 means the price is never scaled for items whose base UOM differs from the subcontractor price list UOM, resulting in an incorrect ProdUnitCost in those cases.

Recommendation:

• Supply the actual lot-size quantity and UOM conversion factor from the event parameters (BaseUnitOfMeasure) when calling SetRoutingPriceListCost. The event already provides BaseUnitOfMeasure; derive the UOM factor using Unit of Measure Management or pass the item's lot size from the caller context if available.

Suggested change

	1,
	SubcPriceManagement.SetRoutingPriceListCost(
	SubcontractorPrice,
	WorkCenter,
	DirectUnitCost,
	IndirectCostPct,
	OverheadRate,
	ProdUnitCost,
	UnitCostCalculation,
	1, // QtyUoM – replace with actual quantity if obtainable from context
	1, // ProdQtyPerUom – replace with UOM conversion factor
	1); // QtyBase – replace with lot-size quantity

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Test verifies only ProdUnitCost, not DirectUnitCost/overhead

The new test DetailedCalculationReportUsesSubcontractorPricing asserts only ProdUnitCost = 200 but the event subscriber also sets DirectUnitCost, IndirectCostPct, and OverheadRate via SetRoutingPriceListCost. A regression in any of those output values would go undetected by this test.

Recommendation:

• Assert the additional cost components that the subscriber sets. If the subcontractor price record has Direct Unit Cost = 200 and zero overhead/indirect cost, assert DirectUnitCost = 200, IndirectCostPct = 0, and OverheadRate = 0 in the report dataset.

Suggested change

	LibraryReportDataset.AssertElementWithValueExists('ProdUnitCost', SubcPriceAmount);
	LibraryReportDataset.LoadDataSetFile();
	LibraryReportDataset.AssertElementWithValueExists('ProdUnitCost', SubcPriceAmount);
	// Also verify the subscriber correctly propagates the other cost components:
	LibraryReportDataset.AssertElementWithValueExists('DirectUnitCost_RtngLine', SubcPriceAmount);

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