[IT] Precheck subcontracting migration locations

[Chethan Thopaiah (ChethanT)]

What

• Block IT subcontracting migration when legacy vendor or purchase-header locations use unsupported warehouse settings.
• Report every incompatible location and list each enabled setting.
• Keep migration data untouched when the precheck fails.

Why

The migration copied legacy location codes directly into the new subcontracting fields, bypassing the target validation and leaving migrated records in an unsupported state.

Tests

• Added coverage for multiple locations and all unsupported warehouse settings.
• Added integration coverage proving migration stops before vendor data is copied.

Review

• al-review (BCQuality, Branch mode, High): the ErrorBehavior::Collect precheck now handles its collected errors explicitly and raises a single deliberate blocking error instead of relying on the platform concatenated dialog.

ADO bug: 648577

AB#648577

[alexei-dobriansky]

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This change adds a precheck before the IT subcontracting migration copies legacy subcontracting location codes into the new fields. It gathers the vendor and purchase-header locations, reports each unsupported warehouse setting, and adds tests for multiple locations and for stopping before vendor data is copied.

The static validation path is mostly correct: the check runs before RunMigration(), uses the same vendor and purchase-header filters as the migration, and reports collected errors instead of stopping at the first bad location. The remaining problem is that the check runs before the migration locks the source data, so the data that is checked is not guaranteed to be the data that is migrated.

Problem-solution fit

Fit: Partial

The bug says migration must stop before unsupported subcontracting locations are copied into the new setup. The change covers the normal single-session path, but it still allows a change between the precheck and the locked migration section.

Suggestions

S1 (🔴 High): Run the location check under the migration locks
CheckSubcontractingLocations() runs before the confirm dialog and before LockTables(), and the Location table is not locked. Another session can change a legacy location code or enable unsupported warehouse settings after the check, so migration can still copy an invalid location. Move the check into the locked section and lock or reread the affected Location records before data is migrated.

Risk assessment and necessity

Risk: The touched path is the one-way IT subcontracting migration for Vendor and Purchase Header location fields. If the precheck misses changed data, migrated records can still contain locations that the new subcontracting app rejects, which leaves setup and open documents hard to repair. There is no event publisher dependency to verify for this change.

Necessity: The change is needed because copying these fields without validation can leave migrated data in an unsupported state. The scope is right for a bug fix, and the added tests cover the main failure path, but the locking gap should be closed before merge.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11001 round=1 by=alexei-dobriansky at=2026-09-03T12:11:56.7022407Z lastSha=25f58e212569974682d48807e903a6618259b447 reviewKey=2f30528ff1ec153bb32461e7f3d0bf5a14aafe0edb5c53dab809ec75cc6620f8 suggestions=S1@812421fe

[alexei-dobriansky]

Good Sense Reviewer - Round 2

Recommendation: Request Changes

What this PR does

The new commit changes the precheck so collected location errors are turned into one deliberate blocking error. That improves the error path for tests and for callers, but it does not change when the precheck runs.

The main correctness gap is still open. The precheck still runs before the migration locks Vendor and Purchase Header, and Location is not locked or reread under those locks. The checked data is therefore not guaranteed to be the same data that migration copies.

Status of previous suggestions

ID	Title	Status	Author response
S1	Run the location check under the migration locks	Not addressed	None.

New observations (commits since round 1)

None - the new commit only changes collected-error handling.

Risk assessment and necessity

Risk: The touched path is the one-way IT subcontracting migration for Vendor and Purchase Header location fields. If source records or location warehouse settings change after the precheck, migration can still persist unsupported subcontracting locations and leave setup or open documents hard to repair. There is no event publisher dependency to verify for this change.

Necessity: The change is needed because copying legacy subcontracting location codes without validation can leave migrated data in an unsupported state. The scope is still right for a bug fix, and the added tests cover the normal failure path, but the locking gap should be closed before merge.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11001 round=2 by=alexei-dobriansky at=2026-09-03T18:02:30Z lastSha=a1f4be2f23f927401e51c14821dbbba8bc200969 reviewKey=940e9e25fabb3931df297a4fd932d66b2f2cef7a0427cdd97317332d8c67ff43 suggestions=S1@812421fe:notaddressed parentRound=1

[Chethan Thopaiah (ChethanT)]

Addressed the locking race identified by Good Sense Reviewer rounds 1 and 2 (comments #5525621750 and #5530204298) in commit 4d949e9ad79a751fde5bb9d5289d8b032614f4a7. The early precheck remains before confirmation for prompt UX. After confirmation, migration now locks Vendor, Purchase Header, and Location, then reruns the authoritative location precheck immediately before RunMigration() in the same transaction. This prevents legacy source references or warehouse settings from changing between validation and migration without holding locks during user confirmation.

[alexei-dobriansky]

Good Sense Reviewer - Round 3

Recommendation: Accept

What this PR does

The new commits keep the early location precheck for prompt UX, then rerun the authoritative precheck after table locks are taken and before migration writes data. They also lock Location, report missing legacy location references, aggregate collected errors into one blocking client error, and update tests for the new behavior.

This addresses the previous data race. The checked Vendor, Purchase Header, and Location data is now read after the migration locks and immediately before RunMigration(), so the data being validated is the data that is about to be migrated.

Status of previous suggestions

ID	Title	Status	Author response
S1	Run the location check under the migration locks	Addressed	The early check stays for prompt UX; the authoritative check now reruns after confirmation, after Vendor, Purchase Header, and Location are locked, and before migration starts.

New observations (commits since round 2)

None - the new commits address the previous locking gap and tighten the error aggregation tests.

Risk assessment and necessity

Risk: The touched path is the one-way IT subcontracting migration for Vendor and Purchase Header location fields. An incorrect check could still leave invalid subcontracting locations in migrated data, but the authoritative precheck now runs under locks before any migration writes. There is no event publisher dependency to verify for this change.

Necessity: The change is needed because copying legacy subcontracting location codes without validation can leave migrated data in a state the new subcontracting setup rejects. The scope remains right for a bug fix, and the tests cover multiple invalid locations, missing locations, and the no-data-moved failure path.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11001 round=3 by=alexei-dobriansky at=2026-09-06T12:05:02Z lastSha=7153bde1a291ab209ef463af3e03edf9110fc400 reviewKey=f8162ee0fc763e1f5be9a8a1061a6b7d196226b7a0b66054e429bbbd1abc2b7b suggestions=S1@812421fe:addressed parentRound=2

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

The new negative tests duplicate the production error texts in test-local Labels and then assert those full formatted messages. The asserterror guidance prefers a shared Label/assert helper or at least a stable invariant fragment, because duplicated full messages drift easily and can make the test fail on wording churn instead of behavior.

Knowledge:

• microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6}