[Subcontracting] Add feature mechanism for subcontracting app#8436
Conversation
- Implemented Subc. Feature Flag Handler to check if subcontracting is enabled. - Created Subc. Upgrade Tag Def. Ext. to manage upgrade tags for subcontracting. - Enhanced Subcontracting Install codeunit to set subcontracting feature on installation. - Introduced Subc. Application Area Handler to manage application area settings for subcontracting.
…e-handling-to-subcontracting-app
…e-handling-to-subcontracting-app
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Manufacturing.Subcontracting; |
There was a problem hiding this comment.
The field "Legacy Subcontracting" is marked as obsolete 29, this code should follow the same and all the references to this as well
|
|
||
| codeunit 99001569 "Subc. Feature Flag Handler" | ||
| { | ||
| procedure IsSubcontractingEnabled(): Boolean |
There was a problem hiding this comment.
Feature flag issues DB read on every call
IsSubcontractingEnabled() calls ManufacturingSetup.Get() every time it is invoked, and SubcFeatureFlagHandler is not SingleInstance. Because this guard is added to dozens of event subscribers (purchase posting, planning, WIP, BOM calculation, etc.) each invocation triggers a separate database round-trip, multiplying the cost on any operation that fires multiple subscribers.
Recommendation:
- Declare the codeunit as
SingleInstance = trueso the result is computed once per session. Alternatively, cache the boolean result in a local variable or store it inSubcSessionStateon first read, and clear it only whenManufacturingSetupis modified.
| procedure IsSubcontractingEnabled(): Boolean | |
| codeunit 99001569 "Subc. Feature Flag Handler" | |
| { | |
| SingleInstance = true; | |
| var | |
| IsEnabled: Boolean; | |
| Initialized: Boolean; | |
| procedure IsSubcontractingEnabled(): Boolean | |
| var | |
| ManufacturingSetup: Record "Manufacturing Setup"; | |
| begin | |
| if Initialized then | |
| exit(IsEnabled); | |
| ManufacturingSetup.SetLoadFields("Legacy Subcontracting"); | |
| if not ManufacturingSetup.Get() then | |
| exit(false); | |
| IsEnabled := not ManufacturingSetup."Legacy Subcontracting"; | |
| Initialized := true; | |
| exit(IsEnabled); | |
| end; | |
| procedure ResetCache() | |
| begin | |
| Initialized := false; | |
| end; | |
| } |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| procedure ClearAllDictionariesForKey(StoredKey: Text) | ||
| begin | ||
| if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then |
There was a problem hiding this comment.
SingleInstance cache defeated by per-call DB reads
SubcSessionState is a SingleInstance codeunit whose purpose is to cache values across calls within a session. Adding a non-cached IsSubcontractingEnabled() (which calls ManufacturingSetup.Get()) at the top of every Set, Get, and Clear method means every dictionary access now incurs a database read, negating the performance benefit of the singleton pattern entirely.
Recommendation:
- Cache the feature-enabled flag once inside
SubcSessionStateitself, or makeSubcFeatureFlagHandlerSingleInstanceso the result is only read once per session across all callers.
| if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then | |
| // In SubcSessionState, replace repeated IsSubcontractingEnabled() calls: | |
| var | |
| SubcFeatureFlagHandler: Codeunit "Subc. Feature Flag Handler"; | |
| FeatureFlagChecked: Boolean; | |
| FeatureEnabled: Boolean; | |
| local procedure GetFeatureEnabled(): Boolean | |
| begin | |
| if not FeatureFlagChecked then begin | |
| FeatureEnabled := SubcFeatureFlagHandler.IsSubcontractingEnabled(); | |
| FeatureFlagChecked := true; | |
| end; | |
| exit(FeatureEnabled); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
GetGuidXxx returns empty Guid when disabled
Recommendation:
Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
| BaseQtyToPurch: Decimal; | ||
| QtyToPurch: Decimal; | ||
| begin | ||
| if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then |
There was a problem hiding this comment.
Feature guard in setter/helper methods is redundant
Methods such as SetOperationNoForCreatedPurchaseOrder, ClearOperationNoForCreatedPurchaseOrder, SetRoutingReferenceNoForCreatedPurchaseOrder, and ClearRoutingReferenceNoForCreatedPurchaseOrder merely assign or clear a variable. Adding an IsSubcontractingEnabled() DB read to each of these trivial setters is disproportionate to the risk and multiplies the database load when the purchase order creator is active.
Recommendation:
- Guard only the entry-point public methods (e.g., the purchase order creation methods) and remove guards from internal state-management helpers whose sole effect is updating a local variable. The entry-point check is sufficient to prevent execution when the feature is disabled.
| if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then | |
| // Remove guards from setter/clear helpers: | |
| internal procedure SetOperationNoForCreatedPurchaseOrder(OperationNoToSet: Code[10]) | |
| begin | |
| OperationNo := OperationNoToSet; | |
| end; | |
| internal procedure ClearOperationNoForCreatedPurchaseOrder() | |
| begin | |
| Clear(OperationNo); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| codeunit 99001568 "Subc. Application Area Handler" | ||
| { | ||
| [EventSubscriber(ObjectType::Codeunit, Codeunit::"Application Area Mgmt. Facade", OnGetPremiumExperienceAppAreas, '', false, false)] |
There was a problem hiding this comment.
Application area subscriber reads DB on every call
The OnGetPremiumExperienceAppAreasSubscriber event fires whenever an application area is evaluated (e.g., page open, field visibility check). SetApplicationArea performs ManufacturingSetup.Get() on every invocation. If SubcFeatureFlagHandler were SingleInstance, this would be free; without caching it adds a database round-trip to every page-open event in a premium session.
Recommendation:
- This concern is resolved by making
SubcFeatureFlagHandlerSingleInstance(see finding Fix CICD #1). Alternatively, callSubcFeatureFlagHandler.IsSubcontractingEnabled()directly here so the cached value is used.
| [EventSubscriber(ObjectType::Codeunit, Codeunit::"Application Area Mgmt. Facade", OnGetPremiumExperienceAppAreas, '', false, false)] | |
| local procedure SetApplicationArea(var TempApplicationAreaSetup: Record "Application Area Setup") | |
| var | |
| SubcFeatureFlagHandler: Codeunit "Subc. Feature Flag Handler"; // SingleInstance after fix | |
| begin | |
| TempApplicationAreaSetup."Subcontracting" := SubcFeatureFlagHandler.IsSubcontractingEnabled(); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
| // ------------------------------------------------------------------------------------------------ | ||
| namespace Microsoft.Manufacturing.Subcontracting; |
There was a problem hiding this comment.
File uses CRLF line endings inconsistently
The diff for this file shows ^M (carriage-return) at the end of every line, indicating CRLF line endings. The rest of the changed AL files in this PR use LF only. Mixed line endings within the repository can cause diff noise and may violate the .gitattributes normalisation rules.
Recommendation:
- Convert the file to LF-only line endings to match the repository convention, or ensure the author's editor/IDE is configured to use LF for
.alfiles.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| begin | ||
| exit('MS-406123-Subcontracting-20260601'); | ||
| end; | ||
| } No newline at end of file |
There was a problem hiding this comment.
File missing newline at end
The new SubcUpgradeTagDefExt.al file is missing a newline at end of file (diff shows \ No newline at end of file). This is inconsistent with the rest of the codebase and can produce noisy diffs in future edits.
Recommendation:
- Add a trailing newline after the closing
}on the last line.
| } | |
| internal procedure GetSubcontractingUpgradeTag(): Code[250] | |
| begin | |
| exit('MS-406123-Subcontracting-20260601'); | |
| end; | |
| } |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
|
||
| if ManufacturingSetup.Get() then begin | ||
| ManufacturingSetupBackup.Copy(ManufacturingSetup); | ||
| ManufacturingSetup.Delete(); |
There was a problem hiding this comment.
Test deletes singleton table without proper isolation
GuardReturnsFalseWhenSetupNotExists calls ManufacturingSetup.Delete() on the company-wide singleton record. If the assertion at line 152 or the subsequent restore at lines 155-157 fails (e.g., due to a conflicting test or a runtime error), the ManufacturingSetup record is left permanently deleted in the test company, breaking all tests that follow in the same test suite.
Recommendation:
- Wrap the delete/restore logic in a try-finally pattern, or use a test isolation library (e.g., mock/stub the codeunit) instead of mutating the singleton table. At minimum, add a teardown step via
[TearDown]or move the restore into a protected block.
| ManufacturingSetup.Delete(); | |
| procedure GuardReturnsFalseWhenSetupNotExists() | |
| var | |
| ManufacturingSetup: Record "Manufacturing Setup"; | |
| ManufacturingSetupBackup: Record "Manufacturing Setup" temporary; | |
| SubcFeatureFlagHandler: Codeunit "Subc. Feature Flag Handler"; | |
| begin | |
| Initialize(); | |
| if ManufacturingSetup.Get() then begin | |
| ManufacturingSetupBackup.Copy(ManufacturingSetup); | |
| ManufacturingSetup.Delete(); | |
| end; | |
| // Guard check | |
| Assert.IsFalse(SubcFeatureFlagHandler.IsSubcontractingEnabled(), 'Guard should return false when ManufacturingSetup does not exist.'); | |
| finally | |
| // Always restore | |
| ManufacturingSetup.Init(); | |
| ManufacturingSetup.Copy(ManufacturingSetupBackup); | |
| if not ManufacturingSetup.Insert() then | |
| ManufacturingSetup.Modify(); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| begin | ||
| end; | ||
|
|
||
| local procedure SetSubcontractingFeatureOnInstall() |
There was a problem hiding this comment.
Upgrade path skips feature-flag initialisation
SetSubcontractingFeatureOnInstall() is called only from HandleFreshInstallPerCompany(). Existing customers who upgrade (which triggers HandleReinstallPerCompany) never receive the feature-flag setup, so the upgrade tag MS-406123-Subcontracting-20260601 is never recorded for them. Any future upgrade logic that checks HasUpgradeTag for this tag will incorrectly assume it has not run.
Recommendation:
- Call
SetSubcontractingFeatureOnInstall()fromHandleReinstallPerCompany()as well, or move the logic into a dedicated upgrade codeunit that fires during the upgrade path. The internal upgrade-tag guard (HasUpgradeTag) already prevents double execution.
| local procedure SetSubcontractingFeatureOnInstall() | |
| local procedure HandleReinstallPerCompany() | |
| var | |
| SubcontractingCompInit: Codeunit "Subcontracting Comp. Init."; | |
| begin | |
| SubcontractingCompInit.CreateBasicSubcontractingMgtSetup(); | |
| SetSubcontractingFeatureOnInstall(); // ensure tag is stamped on upgrades too | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| if UpgradeTag.HasUpgradeTag(SubcUpgradeTagDefExt.GetSubcontractingUpgradeTag()) then | ||
| exit; | ||
|
|
||
| SubcApplicationAreaHandler.UpdateApplicationArea(); |
There was a problem hiding this comment.
Install does not set Legacy Subcontracting default
SetSubcontractingFeatureOnInstall() only calls UpdateApplicationArea() (which refreshes the experience-tier cache) but never writes ManufacturingSetup."Legacy Subcontracting" := false. The feature-enabled state therefore depends entirely on the field's default value supplied by the table extension. If the field defaults to true, a fresh installation will have subcontracting silently disabled.
Recommendation:
- Explicitly set
ManufacturingSetup."Legacy Subcontracting" := falsebefore callingUpdateApplicationArea()to make the intended default explicit and self-documenting.
| SubcApplicationAreaHandler.UpdateApplicationArea(); | |
| local procedure SetSubcontractingFeatureOnInstall() | |
| var | |
| ManufacturingSetup: Record "Manufacturing Setup"; | |
| UpgradeTag: Codeunit "Upgrade Tag"; | |
| SubcApplicationAreaHandler: Codeunit "Subc. Application Area Handler"; | |
| SubcUpgradeTagDefExt: Codeunit "Subc. Upgrade Tag Def. Ext."; | |
| begin | |
| if UpgradeTag.HasUpgradeTag(SubcUpgradeTagDefExt.GetSubcontractingUpgradeTag()) then | |
| exit; | |
| if ManufacturingSetup.Get() then begin | |
| ManufacturingSetup."Legacy Subcontracting" := false; | |
| ManufacturingSetup.Modify(); | |
| end; | |
| SubcApplicationAreaHandler.UpdateApplicationArea(); | |
| UpgradeTag.SetUpgradeTag(SubcUpgradeTagDefExt.GetSubcontractingUpgradeTag()); | |
| end; |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
|
@DennisFranzmannGOB please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This pull request introduces a new feature flag mechanism for the Subcontracting module, allowing all related logic to be conditionally enabled or disabled based on a central setting. The main changes include the implementation of the feature flag handler, integration of feature checks throughout the session state and process extension codeunits, and the addition of upgrade tag management to support future upgrades and feature rollouts.
Fixes AB#406123