Skip to content

[Subcontracting] Remove Subcontracting Purchase Provisioning Wizard#8167

Open
SPinkow wants to merge 3 commits into
microsoft:mainfrom
GOB-Software-Systeme-DevOps:w/pinkow/RemoveWizardFromSubcontractingApp
Open

[Subcontracting] Remove Subcontracting Purchase Provisioning Wizard#8167
SPinkow wants to merge 3 commits into
microsoft:mainfrom
GOB-Software-Systeme-DevOps:w/pinkow/RemoveWizardFromSubcontractingApp

Conversation

@SPinkow
Copy link
Copy Markdown
Contributor

@SPinkow SPinkow commented May 15, 2026

Remove Subcontracting Purchase Provisioning Wizard temporary from Subcontracting App.
It will be added again if the necessary parts are moved to the BaseApp.

What & why

These changes removes the Subcontracting Purchase Provizioning Wizard and the related tests. The Wizard will be moved to the BaseApp. Because this dependency on the base app does not exists for this build now we decided to first remove the wizard and add the subcontraction app related festures and tests later agian.

Details about the changes:
This pull request makes significant changes to the Subcontracting app by removing dependencies on the custom Subc. Management Setup table and related objects, consolidating setup logic onto the standard Manufacturing Setup table. It also removes several obsolete codeunits and pages, and updates permissions accordingly. The changes improve maintainability and align the app more closely with standard manufacturing features.

Key changes:

Removal of Subc. Management Setup and related objects

  • All references to the Subc. Management Setup table and its data access have been removed from codeunits, replacing them with the standard Manufacturing Setup table. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]
  • The obsolete Subc. Management Setup page and related codeunits and pages have been deleted (Subc. CrPurchSubcon(Yes/No), Subc. ItemJnlCheckExt, Subc. Create Prod. Rtng. Ext.). [1] [2] [3]

Permissions and security updates

  • Permissions for the removed table, pages, and codeunits have been deleted from all relevant permission sets (Subcontract. - Objs, Subcontract. - Read, Subcontract. - Edit). [1] [2] [3] [4] [5] [6] [7] [8]

Setup and initialization changes

  • The business setup extension now registers the standard Manufacturing Setup page instead of the removed Subc. Management Setup page.
  • The company initialization logic has been updated to initialize manufacturing setup defaults instead of custom subcontracting setup.

Dependency and namespace updates

  • Added necessary using Microsoft.Manufacturing.Setup; statements to files now using Manufacturing Setup. [1] [2] [3]

Linked work

Fixes AB#619329

How I validated this

  • I read the full diff and it contains only changes I intended.
  • I built the affected app(s) locally with no new analyzer warnings.
  • I ran the change in Business Central and confirmed it behaves as expected.
  • I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

None

…contracting App.

It will be added again if the necessary parts are moved to the BaseApp.
@SPinkow SPinkow requested a review from a team as a code owner May 15, 2026 12:21
@SPinkow SPinkow requested a review from TelesforoAleix May 15, 2026 12:21
@github-actions github-actions Bot added AL: Apps (W1) Add-on apps for W1 From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item labels May 15, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone May 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

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

Trailing newline removed from Codeunit file

This PR changes the final } of SubcPurchFactboxMgmt.Codeunit.al from a line with a trailing newline to one without (\ No newline at end of file). The original file was correct; the change is a regression that can cause AL compiler warnings and noisy Git diffs.

Recommendation:

  • Add a trailing newline at the end of the file to restore the original formatting.
    end;
}

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

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Trailing newline removed from test Codeunit file

The final } of SubcSubcontractingUITest.Codeunit.al was changed from having a trailing newline (correct) to missing one (\ No newline at end of file). This is a regression from the original, can produce AL compiler warnings, and will show up as a spurious change in future diffs.

Recommendation:

  • Restore the trailing newline at the end of the file.
        SubcontractingActionsNotEnabledErr: Label 'Subcontractor Prices action should be enabled for a subcontracting Work Center.';
}

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

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

@ChethanT ChethanT added the Subcontracting Subcontracting related activities label May 15, 2026
@ChethanT ChethanT removed the request for review from TelesforoAleix May 15, 2026 15:22
ChethanT
ChethanT previously approved these changes May 16, 2026
@JesperSchulz JesperSchulz added the SCM GitHub request for SCM area label May 18, 2026
@alexei-dobriansky
Copy link
Copy Markdown
Contributor

Analysis

Scenario Validity:
Valid. The linked ADO bug 619329 ("hide / make non-extensible/internal parts of subcontracting app related to Provisioning Wizard as we move parts of it to BaseApp") and the PR description both describe the same goal: remove the in-app Subcontracting Purchase Provisioning Wizard plus its dedicated setup table (Subc. Management Setup), consolidate the remaining settings onto Manufacturing Setup, and re-introduce the wizard later from BaseApp.

Correctness — consolidation onto Manufacturing Setup:

  • Every former SubcManagementSetup.Get() call site is mechanically replaced with ManufacturingSetup.Get() and the codeunit-level fields/cache flags are renamed in lock-step (SubcPriceManagement.Codeunit.al:24,27,70,124; SubcPurchaseOrderCreator.Codeunit.al:24-30,114,360-368GetManufacturingSetup only; SubcontractingManagement.Codeunit.alSubcManagementSetup/HasSubManagementSetup/GetSubmanagementSetup removed). Both tables are singletons with identical Get() semantics, so this is behavior-preserving.
  • The "is item-charge → receipt sub-reference enabled" check is moved onto the table extension as internal procedure ItemChargeToRcptSubReferenceEnabled() (SubcManufacturingSetup.TableExt.al:90-99) backed by the new field RefItemChargeToRcptSubLines (99001510). The procedure does its own SetLoadFields + Get, so callers (SubcItemChargeAssPurchExt.Codeunit.al:21, SubcPurchPostExt.Codeunit.al:46,62, SubcItemChargeAssPurch.PageExt.al:68) using their own un-Get-ed ManufacturingSetup variable are still correct.
  • Install path is consistently rewired: SubcBusinessSetupExt.Codeunit.al:22 now registers Page::"Manufacturing Setup" for Manual Setup, and SubcontractingCompInit.Codeunit.al:21-39 initializes the Manufacturing Setup record (with Init/Insert if absent) before stamping the subcontracting defaults. No code path remains that expects a separate Subc. Management Setup row.

Correctness — wizard removal and dead surface left behind:
I focused on what is not deleted but should be. Three orphan surfaces survive the cut and warrant cleanup in this PR (or an immediate follow-up):

  1. Report Subc. Create Prod. Routing (99001503) — still present at src/Apps/W1/Subcontracting/App/src/Process/Reports/SubcCreateProdRouting.Report.al and still listed in SubcontractObjs.PermissionSet.al:82. Its only invoker was the CreatePurchProvProdBOMRtng action on SubcItemCard.PageExt.al, which the PR removes (with its BindSubscription/UnbindSubscription of the also-deleted Subc. Create Prod. Rtng. Ext.). After the PR, no code in the Subcontracting app calls this report; partners can still discover and run it via Tell Me. If the goal is to "hide / make non-extensible" the wizard surface, this report should have gone too — or be hidden / marked UsageCategory = None until the BaseApp version lands.

  2. Codeunit Subcontracting Management Ext. (99001527) is declared as EventSubscriberInstance = Manual (subscribes to Subc. Purchase Order Creator.OnBeforeHandleProdOrderRtngWorkCenterWithSubcontractor). Across the post-PR repo nothing BindSubscriptions it — the only binder was inside the now-deleted Subc. Create Prod. Ord. Opt. wizard codeunit. As a result both the codeunit and the public procedure it depends on (SubcontractingManagement.GetKeyCreateProdOrderProcess() at SubcontractingManagement.Codeunit.al:135) are dead. Additionally, no remaining call in the app writes the corresponding session-state key via SubcSessionState.SetCode(...), so even if something did bind the subscriber, SubcSessionState.GetCode(GetKeyCreateProdOrderProcess()) returns empty unconditionally. The codeunit (and the now-unused GetKeyCreateProdOrderProcess helper) should be deleted as part of the same wizard removal.

  3. Field "Rtng. Link Code Purch. Prov." (99001506) on SubcManufacturingSetup.TableExt.al:54-58 is now read by zero product code paths — its only consumer was SubcontractingManagement.CreatePurchProvisionRoutingLine, which the PR deletes. It is still visible and editable on SubcManufacturingSetup.PageExt.al:56 and is still written by SubcSetupLibrary.Codeunit.al:46 (test library). The result is a user-visible setup field whose value nothing reads — confusing, and again contrary to the "hide wizard surface" intent. Either remove the field + its page control, or wrap them with a Visible = false / [Obsolete] marker noting that the field is reserved for the future BaseApp version.

Side Effects:

  • Behavioral functional regression that is intentional per the PR description: you can no longer create a Production Order from a Subcontracting Purchase Order via the page action — SubcPOSubform.PageExt.al loses the CreateProdOrder action plus its CreateProductionOrder/ShowCreatedProdOrderConfirmationMessage helpers, and the Commit()-before-Codeunit.Run pattern goes away with them. The intent is to re-introduce the BaseApp-hosted wizard later; the regression is acknowledged in the PR body and the linked work item.
  • No event subscribers are added in the diff; only removed. No new BaseApp-publisher dependencies introduced, so cross-repo coupling (Step 6b) is moot.

Risk Assessment:

  • Risk: Low for the changes that are in the diff, ignoring the explicit pre-GA breaking-change carve-out you specified. The setup consolidation is straightforward and the consumers fall out symmetrically.
  • The residual risk is purely the dead surface listed above: orphan fields/objects accumulating tech debt and partially defeating the stated scenario.
  • If we don't make this change: the Subcontracting app keeps shipping the duplicate setup table plus the partially-disconnected wizard, which is what the linked bug explicitly says we want to stop doing before the BaseApp move.

Recommendation: Accept with Suggestions

  1. Delete the orphan report Subc. Create Prod. Routingsrc/Apps/W1/Subcontracting/App/src/Process/Reports/SubcCreateProdRouting.Report.al and its entry in SubcontractObjs.PermissionSet.al:82. The PR already deletes its only invoker; leaving the report behind exposes wizard-flavored functionality on Tell Me that the work item explicitly wants hidden.
  2. Delete the orphan subscriber codeunit Subcontracting Management Ext. (99001527) and the public helper SubcontractingManagement.GetKeyCreateProdOrderProcess() it depends on. Nothing binds the codeunit post-PR, and nothing writes the underlying session-state key, so the subscriber is unreachable. Drop the corresponding permission entry as well.
  3. Drop or hide field "Rtng. Link Code Purch. Prov." (99001506) plus its SubcManufacturingSetup.PageExt.al:56 field control. The PR removes its only reader. Leaving an editable setup field whose value nothing consumes is a UX trap. If the field must survive for the future BaseApp version, mark it Visible = false / [Obsolete('Reserved for BaseApp wizard re-introduction')] and likewise remove the page control so users don't set it.

@alexei-dobriansky
Copy link
Copy Markdown
Contributor

Consider deleting orphaned objects.

{
var
SubcManagementSetup: Record "Subc. Management Setup";
ManufacturingSetup: Record "Manufacturing Setup";
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}}$

Module-level record variable never populated via Get()

The codeunit declares a module-level ManufacturingSetup: Record "Manufacturing Setup" variable but never calls Get() on it before invoking ManufacturingSetup.ItemChargeToRcptSubReferenceEnabled(). The intent to cache the setup record is not realized — each call to the method triggers its own fresh DB read internally. The same pattern exists in SubcPurchPostExt and SubcItemChargeAssPurch.PageExt.al.

Recommendation:

  • Either populate the module-level variable with ManufacturingSetup.Get() once (or lazily) and access ManufacturingSetup.RefItemChargeToRcptSubLines directly, or remove the module-level variable and call the helper method on a local variable. This avoids the misleading appearance of caching where none occurs.
Suggested change
ManufacturingSetup: Record "Manufacturing Setup";
var
ManufacturingSetup: Record "Manufacturing Setup";
ManufacturingSetupLoaded: Boolean;
local procedure GetManufacturingSetup()
begin
if ManufacturingSetupLoaded then
exit;
ManufacturingSetup.SetLoadFields(RefItemChargeToRcptSubLines);
ManufacturingSetupLoaded := ManufacturingSetup.Get();
end;

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Module-level ManufacturingSetup never populated, double DB reads

Like SubcItemChargeAssPurchExt, this codeunit declares ManufacturingSetup: Record "Manufacturing Setup" as a module-level variable but never calls Get() on it. The ItemChargeToRcptSubReferenceEnabled() method is called twice in this codeunit (lines ~49 and ~61 in the new version), each causing a separate DB read to the same single-record setup table.

Recommendation:

  • Cache the ManufacturingSetup record at module level with a lazy-load guard, or read RefItemChargeToRcptSubLines once and store it in a Boolean variable for reuse within the same codeunit session.
var
    ManufacturingSetup: Record "Manufacturing Setup";
    HasManufacturingSetup: Boolean;

local procedure GetManufacturingSetup()
begin
    if HasManufacturingSetup then
        exit;
    ManufacturingSetup.SetLoadFields(RefItemChargeToRcptSubLines);
    HasManufacturingSetup := ManufacturingSetup.Get();
end;

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

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Misleading manual setup description label

The SubcontractingDescriptionLbl label still reads 'Make manual Subcontracting Setup', but the manual setup entry now links to Page::"Manufacturing Setup" (the full Manufacturing Setup page), not a dedicated subcontracting page. This description is misleading to end users navigating via the manual setup checklist.

Recommendation:

  • Update SubcontractingDescriptionLbl to accurately describe that the user will be taken to the Manufacturing Setup page where the subcontracting settings are now located, or add an anchor/section hint if the page supports it.
SubcontractingDescriptionLbl: Label 'Configure Subcontracting settings in the Manufacturing Setup page';

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

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

ManualSetupCategory: Enum "Manual Setup Category";
begin
sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, SubcontractingDescriptionLbl, 0, ObjectType::Page, Page::"Subc. Management Setup", ManualSetupCategory::Uncategorized, SubcontractingKeyWordsLbl);
sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, SubcontractingDescriptionLbl, 0, ObjectType::Page, Page::"Manufacturing Setup", ManualSetupCategory::Uncategorized, SubcontractingKeyWordsLbl);
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\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Manual setup opens full Manufacturing Setup page

The Subcontracting app's manual setup entry now opens Page::"Manufacturing Setup", which is a general-purpose page with many unrelated fields. Users navigating from the Subcontracting App guided setup will see all Manufacturing Setup fields rather than only subcontracting-relevant ones, resulting in a poor user experience.

Recommendation:

  • Consider navigating to a dedicated subcontracting section within Manufacturing Setup by using a filtered or anchor-based navigation, or create a lightweight card page extension that shows only the relevant subcontracting fields and link to that instead.
Suggested change
sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, SubcontractingDescriptionLbl, 0, ObjectType::Page, Page::"Manufacturing Setup", ManualSetupCategory::Uncategorized, SubcontractingKeyWordsLbl);
sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, SubcontractingDescriptionLbl, 0, ObjectType::Page, Page::"Subc. Manufacturing Setup", ManualSetupCategory::Uncategorized, SubcontractingKeyWordsLbl);
// Note: Requires a dedicated page extension or page for subcontracting-specific setup fields.

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Inconsistent caching of ManufacturingSetup across procedures

The module-level ManufacturingSetup variable is fetched via Get() directly (without SetLoadFields) in ApplySubcontractorPricingToProdOrderRouting and GetSubcPriceForPurchLine, but CalcStandardCostOnAfterCalcRtngLineCost calls ManufacturingSetup.SetLoadFields("Cost Incl. Setup") followed by Get() — overwriting any previously loaded fields. This inconsistent usage can corrupt the cached state and cause unexpected behavior if procedures are called in sequence.

Recommendation:

  • Consolidate ManufacturingSetup access into a single cached getter procedure that loads all required fields at once (or use separate local variables when different field sets are needed). Avoid mixing module-level caching with SetLoadFields re-fetches that overwrite previously loaded data.
local procedure GetManufacturingSetup()
begin
    if HasManufacturingSetup then
        exit;
    ManufacturingSetup.SetLoadFields("Cost Incl. Setup"); // Add other needed fields
    HasManufacturingSetup := ManufacturingSetup.Get();
end;

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

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

@@ -1,100 +0,0 @@
// ------------------------------------------------------------------------------------------------
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\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

No upgrade/migration for deleted table data

The 'Subc. Management Setup' table (table 99001501) is deleted entirely with no upgrade codeunit to migrate existing customer data. Fields such as 'Common Work Center No.', 'Preset Component Item No.', 'Def. provision flushing method', and all wizard configuration fields will be silently lost on upgrade for any tenant that had configured these values.

Recommendation:

  • Add an upgrade codeunit that runs before the table is dropped, migrating any still-relevant fields to the ManufacturingSetup table extension or preserving the data for customers. At minimum, document that this data is intentionally abandoned and that it is safe to drop.
Suggested change
// ------------------------------------------------------------------------------------------------
// Add a codeunit with SubType = Upgrade:
[UpgradeTag]
procedure UpgradeSubcManagementSetupToManufacturingSetup()
var
SubcMgmtSetup: Record "Subc. Management Setup";
ManufacturingSetup: Record "Manufacturing Setup";
begin
if not SubcMgmtSetup.Get() then
exit;
if ManufacturingSetup.Get() then begin
// migrate relevant fields if needed
ManufacturingSetup.Modify();
end;
end;

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

@github-actions
Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Field removal without data migration

Field 99001506 'Rtng. Link Code Purch. Prov.' (Code[10]) is removed from the Manufacturing Setup table extension. Any existing tenant data in this field will be silently dropped on upgrade. This field was previously referenced in setup initialization code and tests.

Recommendation:

  • If this field is truly no longer needed, add an upgrade tag to document the intentional removal and ensure the schema migration is handled cleanly. If any customers were using this routing link code for production scenarios, migrate the value or provide a transition path.
// In an upgrade codeunit, document removal:
// ObsoleteState = Removed, ObsoleteReason = 'Field removed in version X. Routing link code is no longer required for purchase provision.';
// Add UpgradeTag to mark the schema change as handled.

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

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

@github-actions
Copy link
Copy Markdown
Contributor

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

Permission entries removed without ObsoleteState transition

Multiple objects were removed from the Subcontract. - Objs permission set (e.g., Subc. Management Setup table/page, wizard codeunits, Subc. Create Prod. Routing report) without any deprecation or transition period. Existing permission set extensions or customizations referencing these objects may fail silently or generate errors during upgrade.

Recommendation:

  • When removing objects from permission sets in a minor/patch version, verify that no ISV or customer permission set extensions reference the removed objects. Consider flagging the removed objects as Obsolete in a prior version before hard deletion.
// Before deleting, add to removed objects:
// [ObsoleteState(Removed)]
// [ObsoleteReason('Removed in version X. Wizard functionality has been removed.')]
// Then remove in the subsequent major version.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 From Fork Pull request is coming from a fork Linked Issue is linked to a Azure Boards work item SCM GitHub request for SCM area Subcontracting Subcontracting related activities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants