[Subcontracting] Remove Subcontracting Purchase Provisioning Wizard#8167
Conversation
…contracting App. It will be added again if the necessary parts are moved to the BaseApp.
| UnitCostCalculationType: Enum "Unit Cost Calculation Type"; | ||
| begin | ||
| if not SubcManagementSetup.Get() then | ||
| if not ManufacturingSetup.Get() then |
There was a problem hiding this comment.
Double Get() on same record in CalcStandardCost
In CalcStandardCostOnAfterCalcRtngLineCost, the instance variable ManufacturingSetup is fetched with ManufacturingSetup.Get() at line 124 as an early-exit guard (loading all fields), and then fetched again with ManufacturingSetup.SetLoadFields("Cost Incl. Setup") + ManufacturingSetup.Get() at lines 158–159. The second fetch is redundant since the record is already loaded.
Recommendation:
- Remove the early-exit
ManufacturingSetup.Get()at line 124 and instead consolidate to a singleSetLoadFields+Get()call before the first field access, or check existence separately without a full load.
| if not ManufacturingSetup.Get() then | |
| begin | |
| ManufacturingSetup.SetLoadFields("Cost Incl. Setup"); | |
| if not ManufacturingSetup.Get() then | |
| exit; | |
| // ... rest of guard checks ... | |
| // Use ManufacturingSetup."Cost Incl. Setup" directly — no second Get() needed |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| } | ||
| } | ||
|
|
||
| internal procedure ItemChargeToRcptSubReferenceEnabled(): Boolean |
There was a problem hiding this comment.
ItemChargeToRcptSubReferenceEnabled ignores Rec
The method ItemChargeToRcptSubReferenceEnabled() is an instance method on the Manufacturing Setup table extension, yet it ignores Rec and instead declares a local ManufacturingSetup variable to perform a fresh Get(). Every caller (e.g. SubcItemChargeAssPurchExt, SubcPurchPostExt) that already has the record loaded will still trigger a database roundtrip unnecessarily.
Recommendation:
- Use
Recdirectly inside the method instead of declaring a local variable and callingGet(). If callers may invoke it on an unloaded instance, document the contract clearly.
| internal procedure ItemChargeToRcptSubReferenceEnabled(): Boolean | |
| internal procedure ItemChargeToRcptSubReferenceEnabled(): Boolean | |
| begin | |
| exit(Rec.RefItemChargeToRcptSubLines); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Trailing newline removed from Codeunit fileThis PR changes the final Recommendation:
Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
Trailing newline removed from test Codeunit fileThe final Recommendation:
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); |
There was a problem hiding this comment.
Manual Setup entry opens unscoped Manufacturing Setup
The "Subcontracting App" manual setup entry now navigates users to the generic Manufacturing Setup page instead of the former dedicated Subc. Management Setup page. Users searching for subcontracting configuration will land on a page that exposes all manufacturing settings, making it easy to accidentally modify unrelated fields.
Recommendation:
- If a subcontracting-specific setup page is not yet ready, consider either removing the manual setup registration entirely until a replacement page exists, or routing to a page that filters to only the subcontracting-relevant fields.
| sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, SubcontractingDescriptionLbl, 0, ObjectType::Page, Page::"Manufacturing Setup", ManualSetupCategory::Uncategorized, SubcontractingKeyWordsLbl); | |
| // Option A: Remove the manual setup registration until a dedicated page is available | |
| // (delete the OnRegisterManualSetup event subscriber) | |
| // Option B: Keep but update the description to clarify the page scope | |
| sender.InsertManualSetup(SubcontractingLbl, SubcontractingLbl, ManufacturingSetupDescriptionLbl, 0, ObjectType::Page, Page::"Manufacturing Setup", ManualSetupCategory::Manufacturing, SubcontractingKeyWordsLbl); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Item: Record Item; | ||
| RoutingLink: Record "Routing Link"; | ||
| SubManagementSetup: Record "Subc. Management Setup"; | ||
| ManufacturingSetup2: Record "Manufacturing Setup"; |
There was a problem hiding this comment.
Redundant ManufacturingSetup2 variable
The refactoring introduced a ManufacturingSetup2: Record "Manufacturing Setup" variable that duplicates exactly what ManufacturingSetup already does a few lines later (both attempt Get()/Init()/Insert() on the same singleton table). The ManufacturingSetup2 block at lines 33–36 sets no fields and its only effect is shadowed by the ManufacturingSetup block at lines 41–44.
Recommendation:
- Remove the
ManufacturingSetup2variable and the corresponding Get/Init/Insert block entirely, keeping only theManufacturingSetupblock.
| ManufacturingSetup2: Record "Manufacturing Setup"; | |
| procedure InitSetupFields() | |
| var | |
| Item: Record Item; | |
| RoutingLink: Record "Routing Link"; | |
| ManufacturingSetup: Record "Manufacturing Setup"; | |
| WorkCenter: Record "Work Center"; | |
| begin | |
| SubCreateProdOrdWizLibrary.CreateAndCalculateNeededWorkCenter(WorkCenter, true); | |
| LibraryManufacturing.CreateRoutingLink(RoutingLink); | |
| LibraryInventory.CreateItem(Item); | |
| if not ManufacturingSetup.Get() then begin | |
| ManufacturingSetup.Init(); | |
| ManufacturingSetup.Insert(); | |
| end; | |
| ManufacturingSetup."Rtng. Link Code Purch. Prov." := RoutingLink."Code"; | |
| ManufacturingSetup."Subc. Default Comp. Location" := ManufacturingSetup."Subc. Default Comp. Location"::Purchase; | |
| ManufacturingSetup.Modify(); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
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 Setuptable and related objects, consolidating setup logic onto the standardManufacturing Setuptable. 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
Subc. Management Setuptable and its data access have been removed from codeunits, replacing them with the standardManufacturing Setuptable. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Subc. Management Setuppage 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
Subcontract. - Objs,Subcontract. - Read,Subcontract. - Edit). [1] [2] [3] [4] [5] [6] [7] [8]Setup and initialization changes
Manufacturing Setuppage instead of the removedSubc. Management Setuppage.Dependency and namespace updates
using Microsoft.Manufacturing.Setup;statements to files now usingManufacturing Setup. [1] [2] [3]Linked work
Fixes AB#619329
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility
None