Skip to content

Bug 638464: Remove Subc. Detailed Calculation report; extend BaseApp report via event #8673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChethanT wants to merge 3 commits into
base: main
Choose a base branch
from
+104 −6,460
Open

Bug 638464: Remove Subc. Detailed Calculation report; extend BaseApp report via event #8673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c24397
Bug 638464: Remove Subc. Detailed Calculation report; extend BaseApp …
ChethanT Jun 18, 2026
cf101af
Merge branch 'main' of https://github.com/microsoft/BCApps into bug/6…
ChethanT Jun 19, 2026
f11ad1c
Merge branch 'main' of https://github.com/microsoft/BCApps into bug/6…
ChethanT Jun 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/Apps/W1/Subcontracting/App/src/General/SubcReportingTriggersExt.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
// ------------------------------------------------------------------------------------------------
namespace Microsoft.Manufacturing.Subcontracting;

using Microsoft.Foundation.Enums;
using Microsoft.Manufacturing.Reports;
using System.Environment;
using Microsoft.Manufacturing.Routing;
using Microsoft.Manufacturing.WorkCenter;

codeunit 99001512 "Subc. Reporting Triggers Ext"
{
Expand All @@ -27,4 +29,43 @@ codeunit 99001512 "Subc. Reporting Triggers Ext"
if ReportId = Report::"Detailed Calculation" then
NewReportId := Report::"Subc. Detailed Calculation";
end;

[EventSubscriber(ObjectType::Report, Report::"Detailed Calculation", OnAfterGetRecordRoutingLineOnBeforeCalcRoutingCostPerUnit, '', false, false)]
local procedure OnAfterGetRecordRoutingLineOnBeforeCalcCost(var RoutingLine: Record "Routing Line"; ItemNo: Code[20]; BaseUnitOfMeasure: Code[10]; StandardTaskCode: Code[10]; CalculationDate: Date; var DirectUnitCost: Decimal; var IndirectCostPct: Decimal; var OverheadRate: Decimal; var ProdUnitCost: Decimal; var UnitCostCalculation: Enum "Unit Cost Calculation Type"; var IsHandled: Boolean)
var
SubcontractorPrice: Record "Subcontractor Price";
WorkCenter: Record "Work Center";
SubcPriceManagement: Codeunit "Subc. Price Management";
begin
if RoutingLine.Type <> RoutingLine.Type::"Work Center" then
exit;

if not WorkCenter.Get(RoutingLine."Work Center No.") then

@github-actions github-actions Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

exit;

if WorkCenter."Subcontractor No." = '' then
exit;

SubcontractorPrice."Vendor No." := WorkCenter."Subcontractor No.";
SubcontractorPrice."Item No." := ItemNo;
SubcontractorPrice."Standard Task Code" := StandardTaskCode;
SubcontractorPrice."Work Center No." := WorkCenter."No.";
SubcontractorPrice."Variant Code" := '';
SubcontractorPrice."Unit of Measure Code" := BaseUnitOfMeasure;
SubcontractorPrice."Starting Date" := CalculationDate;
SubcontractorPrice."Currency Code" := '';
SubcPriceManagement.SetRoutingPriceListCost(
SubcontractorPrice,
WorkCenter,
DirectUnitCost,
IndirectCostPct,
OverheadRate,
ProdUnitCost,
UnitCostCalculation,
1,

@github-actions github-actions Bot Jun 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

1,
1);

IsHandled := true;

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end;
}
1 change: 0 additions & 1 deletion src/Apps/W1/Subcontracting/App/src/Permissions/SubcontractObjs.PermissionSet.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,5 @@ permissionset 99001501 "Subcontract. - Objs"
report "Subc. Calculate Subcontracts" = X,
report "Subc. Create Transf. Order" = X,
report "Subc. Create SubCReturnOrder" = X,
report "Subc. Detailed Calculation" = X,

@github-actions github-actions Bot Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

report "Subc. Dispatching List" = X;
}
Loading
Loading