[Subcontracting] Add feature mechanism for subcontracting app

[DennisFranzmannGOB]

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

[DennisFranzmannGOB]

@ChethanT

[github-actions]

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

GetGuidXxx returns empty Guid when disabled

GetGuidProductionOrderCreatedNotification() and GetGuidSubcontractingPOCreatedNotification() use bare exit (no argument) when subcontracting is disabled, returning the default empty Guid {00000000-0000-0000-0000-000000000000}. Any caller that passes this value to MyNotifications.InsertDefault(), IsEnabled(), or notification lookup will silently operate on the zero-GUID, potentially matching unrelated notifications or causing data corruption.

Recommendation:

• Callers of these GetGuid* procedures should check IsSubcontractingEnabled() before calling them. Alternatively, keep the procedures unconditional — they return a constant and adding a guard here is unnecessary since the callers (InitializeSubcontractingNotifications, DisableNotification) already guard themselves.

procedure GetGuidProductionOrderCreatedNotification(): Guid
begin
    exit('{5d564aca-ce60-4345-ba68-e1e50976a346}');
end;

procedure GetGuidSubcontractingPOCreatedNotification(): Guid
begin
    exit('{f7b10c9e-071a-4455-a048-d17b29ef764c}');
end;

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

ClearAllDictionariesForKey skips clearing when disabled

ClearAllDictionariesForKey exits immediately when IsSubcontractingEnabled() returns false, so the requested key is never removed from the dictionaries. Since SubcSessionState is SingleInstance, if the feature was enabled earlier in the same session (data written) and is subsequently disabled, the stale entries persist for the duration of the session and will be visible if the flag is re-enabled.

Recommendation:

• Always execute the dictionary cleanup logic regardless of the feature flag state, or at minimum clear all three dictionaries on the session when the flag is found to be off, so stale entries are not silently retained.

procedure ClearAllDictionariesForKey(StoredKey: Text)
begin
    // Always clear to avoid stale state when feature flag changes
    if CodeDictionary.ContainsKey(StoredKey) then
        CodeDictionary.Remove(StoredKey);
    if DateDictionary.ContainsKey(StoredKey) then
        DateDictionary.Remove(StoredKey);
    if RecordIDDictionary.ContainsKey(StoredKey) then
        RecordIDDictionary.Remove(StoredKey);
end;

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

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

[github-actions]

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

Delete-guard runs on all purchase orders

The old subscriber checked Rec.CalcFields("Subc. Order") and exited early for non-subcontracting purchase orders before calling CheckTransferOrderExistsForPurchaseHeader. This guard was removed, so every purchase order deletion now triggers the subcontracting transfer-order lookup—a database query that is unnecessary for standard purchase orders.

Recommendation:

• Restore the early-exit check: call CalcFields("Subc. Order") and if not Rec."Subc. Order" then exit; before invoking CheckTransferOrderExistsForPurchaseHeader.

if Rec.IsTemporary() then
    exit;
if not RunTrigger then
    exit;

Rec.CalcFields("Subc. Order");
if not Rec."Subc. Order" then
    exit;

#if not CLEAN29
#pragma warning disable AL0432
if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then
#pragma warning restore AL0432
    exit;
#endif
SubcSynchronizeManagement.CheckTransferOrderExistsForPurchaseHeader(Rec);

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

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

[github-actions]

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

Stock-at-subcontractor check removed on delete

The CheckTransferOrderOnBeforeDeleteEvent subscriber previously called SubcTransferManagement.CheckStockAtSubcLocationForPurchHeader to block deletion when components or WIP items had already been physically transferred to the subcontractor. That call (and the underlying procedures CheckStockAtSubcLocationForPurchHeader, HasStockAtSubcLocation, and CalcConsumedQtyAtSubcLocation) have been removed entirely with no replacement, allowing purchase headers to be deleted even when inventory is still at the subcontractor's location.

Recommendation:

• Port CheckStockAtSubcLocationForPurchHeader and its helpers (HasStockAtSubcLocation, CalcConsumedQtyAtSubcLocation) into SubcontractingManagement and restore the call inside the OnBeforeDeleteEvent subscriber behind the feature-flag guard.

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

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

[github-actions]

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

Purchase line field guards silently removed

Six OnBeforeValidate event subscribers (Quantity, No., Location Code, Bin Code, Variant Code, Unit of Measure Code) that called CheckSubcPurchLineCanBeModified to block edits when transfer orders exist have been deleted entirely. The backing procedures CheckSubcPurchLineCanBeModified and CheckSubcPurchLineCanBeDeleted were only in the now-deleted SubcTransferManagement and are not present anywhere in the new code, so users can freely modify locked fields on active subcontracting purchase lines.

Recommendation:

• Restore the six OnBeforeValidate subscribers (and the OnBeforeDeleteEvent subscriber that called CheckSubcPurchLineCanBeDeleted) and move their underlying logic into SubcontractingManagement or a dedicated validation codeunit, protected by the #if not CLEAN29 / feature-flag guard pattern used elsewhere in this PR.

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

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

[github-actions]

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

Transfer line validation guards removed

Three OnBeforeValidate event subscribers on Transfer Line (Item No., Variant Code, Quantity) that called CheckSubcTransferLineCanBeModified have been deleted. The procedure CheckSubcTransferLineCanBeModified existed only in the now-deleted SubcTransferManagement codeunit and has no replacement, so users can alter the item, variant, or quantity on a subcontracting transfer line that is linked to a released production order without any guard.

Recommendation:

• Port CheckSubcTransferLineCanBeModified (and its helper HasSubcTransferForPurchLine) into SubcontractingManagement and restore the three OnBeforeValidate subscribers behind the feature-flag guard used for every other subscriber in this file.

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

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

[alexei-dobriansky]

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This adds an on/off switch for the Subcontracting app. The switch is the BaseApp field Manufacturing Setup."Legacy Subcontracting". When it is off, the app is on (IsSubcontractingEnabled = not "Legacy Subcontracting"). The default is off, so the app is on by default.

The whole mechanism sits under #if not CLEAN29, and the handler is marked ObsoleteState = Pending / 29.0. This matches the BaseApp field, which is Pending in 29.0 and Removed in 32.0. That is the correct way to phase out the old feature.

I checked three things and they hold up:

• Gating is complete. 42 of the 45 subscriber files check the switch and exit first, before doing anything. The 3 that do not are meant to always run (manual-setup registration, upgrade-tag registration, and the app-area handler, which reads the switch itself).
• The guard is in the right place. It is the first line in each subscriber, so nothing runs when the app is off. I confirmed this on the purchase, warehouse, and transfer posting paths.
• Permissions and UI are handled. New codeunits are granted in the permission set, and field/action visibility is driven by the Subcontracting application area, which follows the same switch.

The core logic is sound. My suggestions are about diff size, migration, and test depth - not the switch itself.

Suggestions

S1 - Split out the line-ending and BOM changes
The PR shows +13k / -10k, but most of that is not real change. About 10k lines are just a CRLF-to-LF conversion on ~20 files (mostly large test files) plus removing the UTF-8 BOM from many files. The real change is only +3062 / -280. The conversion matches the repo's * text=auto, so it is fine on its own - but please move it to a separate PR. Right now it buries the real change and rewrites git blame.

S2 - Confirm the migration plan with the BaseApp change
This PR never sets "Legacy Subcontracting", and that field is not user-editable (Editable = false). So every install turns the app on by default, and no one can opt back to legacy from the UI. I also could not find any BaseApp code that turns legacy off when the switch is set. Please confirm the matching BaseApp change does two things: (1) makes legacy subcontracting run only when the switch is on, and (2) sets the switch for environments already using legacy. Otherwise those environments silently move to the new app, and both could post at once.

S3 - Add a test for the "app is off" case
The new tests cover UI visibility and the switch value well, but none run a real action with the app turned off. Please add one end-to-end test: with "Legacy Subcontracting" = true, post or release a subcontracting document and check the app did nothing (no WIP ledger entries, no transfer created). This protects the 42 guards from future regressions.

S4 - Consider caching the switch value
Subc. Session State is SingleInstance, but it reads the switch (a Manufacturing Setup.Get()) on every Set/Get, and each subscriber re-reads it too. The setup is a cached singleton, so the cost is small, but reading it once into the single-instance codeunit would avoid repeated reads in posting loops. If you do this, note that a mid-session toggle would be stale until refresh (acceptable, since toggling already needs an experience-tier refresh).

S5 - Make the "not visible" test clearer
In VendorCardSubcFieldsNotVisibleWhenDisabled, the line asserterror Assert.IsFalse(VendorCard.<field>.Visible()) only passes because reading a hidden control throws an error. That is hard to read, and it would pass for the wrong reason if the call stopped throwing. Asserting the field is not accessible (or not present) would be clearer.

Risk assessment and necessity

Risk: The change touches a lot of code - the guard goes into ~42 posting, transfer, warehouse, and planning subscribers, plus many table OnValidate triggers. But each change is the same simple "exit first" line, and no functional subscriber was missed, so the regression risk is well contained. The main open risk is cross-repo: the switch only works if the BaseApp side gates legacy and sets the flag for existing users, and I cannot prove that from this repo (see S2). Public-API risk is low - the new handler and the BaseApp field both carry matching obsolete tags. The diff size is a readability problem, not a correctness one (see S1).

Necessity: Justified. A clean on/off switch with a defined obsolete lifecycle is the right way to ship a replacement for an old BaseApp feature and let environments fall back during the transition. The scope fits an infrastructure change, the gating is complete, and tests exist for the switch. The open items are confirming the migration plan and adding an "app off" test - not a redesign.

---

[AI-PR-REVIEW] version=1 system=github pr=8436 round=1 by=alexei-dobriansky at=2026-06-12T09:23:00Z lastSha=ec6e0ab011367b47bb4eba16530d2191ff0e8ca5 suggestions=S1,S2,S3,S4,S5

[github-actions]

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

ManufacturingSetup.Get() on every call without caching

IsSubcontractingEnabled() executes a ManufacturingSetup.Get() database read on every invocation. This method is now called at the top of every event subscriber across ~60 codeunits, including high-frequency paths such as OnAfterValidateEvent field triggers, posting routines, and transfer order operations, meaning each of those operations hits the database once more than before.

Recommendation:

• Cache the result of IsSubcontractingEnabled() in a SingleInstance codeunit (e.g., SubcSessionState, which is already SingleInstance = true) and invalidate the cache when ManufacturingSetup changes. Alternatively, since the value changes only when an admin modifies setup, a session-scoped static Boolean field in the codeunit is sufficient.

Suggested change

	#endif
	// In SubcSessionState (SingleInstance = true):
	var
	IsSubcontractingEnabledCached: Boolean;
	IsSubcontractingEnabledCacheInitialized: Boolean;
	procedure IsSubcontractingEnabled(): Boolean
	var
	ManufacturingSetup: Record "Manufacturing Setup";
	begin
	if not IsSubcontractingEnabledCacheInitialized then begin
	ManufacturingSetup.SetLoadFields("Legacy Subcontracting");
	IsSubcontractingEnabledCached := ManufacturingSetup.Get() and not ManufacturingSetup."Legacy Subcontracting";
	IsSubcontractingEnabledCacheInitialized := true;
	end;
	exit(IsSubcontractingEnabledCached);
	end;

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

[github-actions]

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

Default state mismatch between flag handler and area handler

When ManufacturingSetup does not exist, SubcApplicationAreaHandler.SetApplicationArea() defaults to TempApplicationAreaSetup."Subcontracting" := true (feature ON), while SubcFeatureFlagHandler.IsSubcontractingEnabled() returns false (feature OFF). This creates a broken state: the UI renders Subcontracting fields as visible, but every code path guarded by the feature flag silently exits, leaving users with an inaccessible, non-functional feature surface.

Recommendation:

• Align the defaults so that both components agree on the fallback when no setup record exists. Since the absence of setup is an edge case (typically a brand-new company), the safest default is to treat the feature as enabled (true) for both, consistent with the application area handler.

Suggested change

	var
	// In SubcFeatureFlagHandler.IsSubcontractingEnabled():
	procedure IsSubcontractingEnabled(): Boolean
	var
	ManufacturingSetup: Record "Manufacturing Setup";
	begin
	ManufacturingSetup.SetLoadFields("Legacy Subcontracting");
	if not ManufacturingSetup.Get() then
	exit(true); // align with SetApplicationArea default
	exit(not ManufacturingSetup."Legacy Subcontracting");
	end;

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

[github-actions]

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

asserterror Assert.IsFalse pattern always passes

In VendorCardSubcFieldsNotVisibleWhenDisabled, the pattern asserterror Assert.IsFalse(VendorCard."...".Visible(), '...') passes in both the correct and incorrect case: if the field is inaccessible, .Visible() throws and asserterror catches it; if the field is accessible, Assert.IsFalse(true,...) throws an assertion error which asserterror also catches. The test never actually fails, providing false confidence in the feature-flag gating.

Recommendation:

• Use a bare asserterror on the field access to verify inaccessibility. After the asserterror, assert the last error text to confirm the specific application-area error rather than any arbitrary error.

Suggested change

	// Replace asserterror Assert.IsFalse(VendorCard."Subcr. Location Code".Visible(), '...') with:
	assertError VendorCard."Subcr. Location Code".Visible();
	Assert.ExpectedError(''); // or check GetLastErrorText() for the ApplicationArea error text
	assertError VendorCard."Subc. Linked to Work Center".Visible();
	assertError VendorCard."Subc. Work Center No.".Visible();

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

[github-actions]

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

Test deletes ManufacturingSetup without TearDown

GuardReturnsFalseWhenSetupNotExists deletes the ManufacturingSetup record, asserts, then manually calls Insert() + Commit() to restore it. If the assertion at line 169 fails (or any error is thrown between delete and restore), subsequent tests run without a ManufacturingSetup record, potentially causing cascading, hard-to-diagnose failures in other test codeunits that rely on it.

Recommendation:

• Wrap the delete-and-restore logic in a pattern that guarantees cleanup: either use a try/finally-equivalent via AssertError-safe wrapper, or implement a dedicated TearDown / [TearDown] test isolation mechanism that restores state unconditionally.

Suggested change

	#pragma warning restore AL0432
	// Protect restoration from test failure:
	ManufacturingSetup.Delete();
	if GuardReturnsFalseAssert(SubcFeatureFlagHandler) then; // replace inline assertion
	ManufacturingSetup.Init();
	ManufacturingSetup.Copy(TempManufacturingSetupBackup);
	ManufacturingSetup.Insert();
	Commit();
	// Or move the deletion to a TearDown that always restores state

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

[github-actions]

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

Missing newline at end of file

After the modifications, SubcontractingInstall.Codeunit.al has no trailing newline. This is inconsistent with the repository coding standards and causes noisy diffs in future PRs.

Recommendation:

• Add a single trailing newline after the closing }.

        UpgradeTag.SetUpgradeTag(SubcUpgradeTagDefExt.GetSubcontractingUpgradeTag());
    end;
}

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

SetSubcontractingFeatureOnInstall no-op on reinstall

SetSubcontractingFeatureOnInstall() is called from both HandleFreshInstallPerCompany and HandleReinstallPerCompany, but the upgrade-tag guard (UpgradeTag.HasUpgradeTag(...)) causes it to exit immediately on any reinstall because the tag was set during the original install. This means that on reinstall the application area is never refreshed, so if the Subcontracting area flag has become stale (e.g., after a database restore or manual reset), a reinstall will not fix it.

Recommendation:

• Either remove the call from HandleReinstallPerCompany (accepting that reinstall won't refresh the area), or remove the upgrade-tag guard and call UpdateApplicationArea() unconditionally on both install and reinstall. The upgrade-tag pattern is primarily for data upgrades, not for idempotent configuration refreshes.

local procedure HandleReinstallPerCompany()
var
    SubcontractingCompInit: Codeunit "Subcontracting Comp. Init.";
    SubcApplicationAreaHandler: Codeunit "Subc. Application Area Handler";
begin
    SubcontractingCompInit.CreateBasicSubcontractingMgtSetup();
    SubcApplicationAreaHandler.UpdateApplicationArea(); // always refresh on reinstall
end;

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

ObsoleteReason is misleading for the feature flag codeunit

The ObsoleteReason says "Legacy Subcontracting will be discontinued, environments should move to the Subcontracting App so this codeunit will be removed in a future release." This is confusing because this codeunit IS the bridge enabling migration away from legacy subcontracting — the reason implies the codeunit exists to support the old system, when in fact it exists to support the transition to the new one.

Recommendation:

• Rewrite the ObsoleteReason to accurately describe the purpose and removal rationale: the flag handler is a transition mechanism that will be removed once all customers have migrated and the Legacy Subcontracting field is removed.

Suggested change

	ObsoleteReason = 'Legacy Subcontracting will be discontinued, environments should move to the Subcontracting App so this codeunit will be removed in a future release.';
	ObsoleteReason = 'This codeunit is a temporary transition guard for the migration from Legacy Subcontracting to the Subcontracting App. It will be removed in 29.0 once the Legacy Subcontracting field and all related conditional compilation blocks are cleaned up.';

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

[github-actions]

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

TryBindPostingPreviewHandler returns implicit false

TryBindPostingPreviewHandler() is declared as Boolean but uses a bare exit; (implicit false) when the feature flag is disabled. Callers that inspect the return value cannot distinguish between "feature disabled" and "BindSubscription failed", both of which silently return false. This can hide binding failures.

Recommendation:

• Use exit(false) explicitly to document intent and enable callers to differentiate correctly if needed.

Suggested change

	}
	#if not CLEAN29
	#pragma warning disable AL0432
	if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then
	#pragma warning restore AL0432
	exit(false);
	#endif

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

[github-actions]

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

TryUnbindPostingPreviewHandler returns implicit false

TryUnbindPostingPreviewHandler() is also declared as Boolean and uses a bare exit; when the feature is disabled, returning an implicit false. The caller in UnbindPostPrecEventHandlerOnAfterUnbindSubscription discards the return value, so there is no immediate bug, but the inconsistency makes the contract of the function unclear and fragile under future refactoring.

Recommendation:

• Use exit(false) to be explicit about the returned value when the guard fires.

Suggested change

	exit;
	#if not CLEAN29
	#pragma warning disable AL0432
	if not SubcFeatureFlagHandler.IsSubcontractingEnabled() then
	#pragma warning restore AL0432
	exit(false);
	#endif

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

[DennisFranzmannGOB]

Agentic PR Review - Round 1

Recommendation: Accept with Suggestions

What this PR does

This adds an on/off switch for the Subcontracting app. The switch is the BaseApp field Manufacturing Setup."Legacy Subcontracting". When it is off, the app is on (IsSubcontractingEnabled = not "Legacy Subcontracting"). The default is off, so the app is on by default.

The whole mechanism sits under #if not CLEAN29, and the handler is marked ObsoleteState = Pending / 29.0. This matches the BaseApp field, which is Pending in 29.0 and Removed in 32.0. That is the correct way to phase out the old feature.

I checked three things and they hold up:

* **Gating is complete.** 42 of the 45 subscriber files check the switch and exit first, before doing anything. The 3 that do not are meant to always run (manual-setup registration, upgrade-tag registration, and the app-area handler, which reads the switch itself).

* **The guard is in the right place.** It is the first line in each subscriber, so nothing runs when the app is off. I confirmed this on the purchase, warehouse, and transfer posting paths.

* **Permissions and UI are handled.** New codeunits are granted in the permission set, and field/action visibility is driven by the `Subcontracting` application area, which follows the same switch.

The core logic is sound. My suggestions are about diff size, migration, and test depth - not the switch itself.

Suggestions

S1 - Split out the line-ending and BOM changes The PR shows +13k / -10k, but most of that is not real change. About 10k lines are just a CRLF-to-LF conversion on ~20 files (mostly large test files) plus removing the UTF-8 BOM from many files. The real change is only +3062 / -280. The conversion matches the repo's * text=auto, so it is fine on its own - but please move it to a separate PR. Right now it buries the real change and rewrites git blame.

S2 - Confirm the migration plan with the BaseApp change This PR never sets "Legacy Subcontracting", and that field is not user-editable (Editable = false). So every install turns the app on by default, and no one can opt back to legacy from the UI. I also could not find any BaseApp code that turns legacy off when the switch is set. Please confirm the matching BaseApp change does two things: (1) makes legacy subcontracting run only when the switch is on, and (2) sets the switch for environments already using legacy. Otherwise those environments silently move to the new app, and both could post at once.

S3 - Add a test for the "app is off" case The new tests cover UI visibility and the switch value well, but none run a real action with the app turned off. Please add one end-to-end test: with "Legacy Subcontracting" = true, post or release a subcontracting document and check the app did nothing (no WIP ledger entries, no transfer created). This protects the 42 guards from future regressions.

S4 - Consider caching the switch value Subc. Session State is SingleInstance, but it reads the switch (a Manufacturing Setup.Get()) on every Set/Get, and each subscriber re-reads it too. The setup is a cached singleton, so the cost is small, but reading it once into the single-instance codeunit would avoid repeated reads in posting loops. If you do this, note that a mid-session toggle would be stale until refresh (acceptable, since toggling already needs an experience-tier refresh).

S5 - Make the "not visible" test clearer In VendorCardSubcFieldsNotVisibleWhenDisabled, the line asserterror Assert.IsFalse(VendorCard.<field>.Visible()) only passes because reading a hidden control throws an error. That is hard to read, and it would pass for the wrong reason if the call stopped throwing. Asserting the field is not accessible (or not present) would be clearer.

Risk assessment and necessity

Risk: The change touches a lot of code - the guard goes into ~42 posting, transfer, warehouse, and planning subscribers, plus many table OnValidate triggers. But each change is the same simple "exit first" line, and no functional subscriber was missed, so the regression risk is well contained. The main open risk is cross-repo: the switch only works if the BaseApp side gates legacy and sets the flag for existing users, and I cannot prove that from this repo (see S2). Public-API risk is low - the new handler and the BaseApp field both carry matching obsolete tags. The diff size is a readability problem, not a correctness one (see S1).

Necessity: Justified. A clean on/off switch with a defined obsolete lifecycle is the right way to ship a replacement for an old BaseApp feature and let environments fall back during the transition. The scope fits an infrastructure change, the gating is complete, and tests exist for the switch. The open items are confirming the migration plan and adding an "app off" test - not a redesign.

[AI-PR-REVIEW] version=1 system=github pr=8436 round=1 by=alexei-dobriansky at=2026-06-12T09:23:00Z lastSha=ec6e0ab011367b47bb4eba16530d2191ff0e8ca5 suggestions=S1,S2,S3,S4,S5

@alexei-dobriansky
My thoughts on these suggestions :

S1 - Split out the line-ending and BOM changes
Copilot mentioned this on some files i changed so i fixed it for all files in this app to avoid more of these comments. Reverting this will be a time consuming.

S2 - Confirm the migration plan with the BaseApp change
"Legacy Subcontracting" will only be touched by italian base app. In all other (country) versions this will default to false (init value) so subcontracting application area will default to true ( equals to Subcontracting App is enabled)

S3 - Add a test for the "app is off" case
This test VendorCardSubcFieldsNotVisibleWhenDisabled demonstrates the “app is off” behavior. In addition test AppAreaClearedWhenSubcontractingDisabled validates that subcontracting application area is set correctly and unit test GuardReturnsFalseWhenDisabled ensures the guard procedure works as intended. In conclusion i think we have tested this well, whats your opinion on this?

S4 - Consider caching the switch value
Discussed this with Chethan in the first comment. Record.Get will take advantage from Server Caching so we would produce a more risky behavior when add an additional cache level to the logic which will not significantly enhance performance. Chethan then closed the comment.

S5 - Make the "not visible" test clearer
This was answered somewhere in the AI review comments. If a field on a testpage is not visible due to a not set application area then Page.Field.Visible() will throw an error instead of true/false. So this is the only tested way to check if a field is disabled by application area. i think there is one more test in the base app that already does this.

To summarize the only real possible suggestion to discuss is in my opinion S3 but it think this is solid tested.