Bug 648962: Create transfer order shows generic error when existing transfers cover subcontracting demand

[Chethan Thopaiah (ChethanT)]

AB#648962

What & why

When all positive component or WIP demand for a subcontracting purchase order was already covered by open, in-transit, or posted transfer activity, Create Transfer Order to Subcontractor showed the generic "Nothing to create" error. The report's dry run returned the same Boolean result both for truly ineligible/zero demand and for eligible demand that was fully covered.

This change tracks covered positive eligible demand separately. The generic error remains for setup, routing, quantity, and supply-method cases with no positive eligible demand. Fully covered demand now produces the requested actionable error and offers Show open transfer orders only when a related unposted outbound transfer header exists. Navigation reuses the existing subcontracting factbox helper, which opens one order as a card or multiple orders as a filtered list while excluding returns and unrelated orders.

Linked work

AB#648962

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

• Re-established TDD proof from clean commit c6d92043e9 in an isolated worktree: the component regression failed because expected covered-demand guidance was replaced by the old generic error.
• Compiled both Subcontracting and Subcontracting Test with 0 errors and 0 warnings using the supported AL toolchain.
• Full Subc. Subcontracting Test codeunit: 62 passed, 0 failed.
• Full Subc. WIP Trans. Create Test codeunit: 26 passed, 0 failed.
• Existing partial-demand recreation test passed.
• Covered components; covered WIP with open, in-transit, and posted-only activity; zero demand; ineligible supply method; outbound card/list navigation; and exclusion of returns/unrelated orders are covered by focused regressions.
• Manual visual verification of the rendered actionable-error button in the Web client remains external; no shared environment deployment was authorized. The AL integration tests exercise the error text and the action callback/navigation behavior on the local BC service.

Risk & compatibility

Low risk. The change is limited to the report's no-lines-created error selection and a new actionable-error callback. It does not change transfer quantity calculation or creation when any uncovered demand exists, adds no schema or upgrade changes, and is backward compatible.

[alexei-dobriansky]

Good Sense Reviewer - Round 1

Recommendation: Accept

What this PR does

This change keeps the existing generic error for setup, routing, zero-quantity, and ineligible supply-method cases. It adds a separate covered-demand state so fully covered component or WIP demand gets a clearer actionable error instead.

The fix is targeted. HasCoveredDemand is reset for each dry run, set only when positive eligible demand is fully covered, and ignored when any uncovered demand still creates transfer lines. The new action reuses the existing subcontracting transfer-order navigation helper and filters to outbound transfer orders.

Problem-solution fit

Fit: Strong

The bug is about a fully covered subcontracting purchase order being reported as if there were no eligible transfer demand. The diff addresses that root cause directly and keeps the old error for the cases where there is truly nothing eligible to transfer.

Suggestions

None.

Risk assessment and necessity

Risk: Low. The regression surface is limited to report 20501 when no new subcontracting transfer lines are created. Transfer quantity calculation and insertion branches stay unchanged, and the tests cover covered components, covered/open/in-transit/posted WIP, zero demand, ineligible components, and outbound navigation filtering.

Necessity: The change is needed because users otherwise cannot tell whether demand is already covered or whether the purchase order has no eligible transfer demand. The scope is appropriate for a bug fix, with no schema, upgrade, or BaseApp publisher dependency.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11147 round=1 by=alexei-dobriansky at=2026-09-07T18:19:23Z lastSha=8f4b29cddd3289905ef180b59b22f339ebe2eec1 reviewKey=33216122a2d14a6264fb4f123671c238a00bc906f6df77e5e9d236ac64d3d1b7 suggestions=none

[github-actions]

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

The posted-only regression test NoWIPTransferCreatedWhenExpectedEqualsPostedQuantity covers the new covered-demand message after deleting the only transfer header, but it never proves that CreateCoveredTransferErrorInfo omitted the "Show open transfer orders" action in that no-open-transfer branch. A regression that always adds the action would still pass; extend this test to assert the absence of the navigation action as well as the message.

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

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

[alexei-dobriansky]

Good Sense Reviewer - Round 2

Recommendation: Accept

What this PR does

The change separates eligible subcontracting demand that is already covered by transfer activity from cases where there is no eligible demand. The added checks only set the covered-demand state after positive component or WIP demand is found and existing open, in-transit, or posted quantities cover it, so the generic error still remains for setup, routing, zero-quantity, and supply-method cases. The round 2 change strengthens the zero-demand regression test by validating quantity changes and updating amounts instead of using a bulk modify.

Status of previous suggestions

None from the previous round.

New observations (commits since round 1)

None - the new commit only improves test setup and no new issue was found.

Risk assessment and necessity

Risk: Low. The product change is limited to no-lines-created error selection and an action callback for related outbound transfer orders. It does not change transfer quantity calculation or creation when uncovered demand exists, and the new round 2 hunk is test-only.

Necessity: The change is needed because the prior generic error made fully covered subcontracting demand look the same as no eligible demand. The scope matches that problem and keeps the old message for unrelated no-demand cases.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11147 round=2 by=alexei-dobriansky at=2026-09-09T12:16:31Z lastSha=6c03409cef4b9d941bade4b6b5a5e4c7d17ec00b reviewKey=d834f9df88509b890a882adcc5df5b2f3f2ba292150cdb6eed5c2a93fa974f93 suggestions=none parentRound=1

[Chethan Thopaiah (ChethanT)]

PR Shepherd plan ps-40617e7286b4c7733730e114cc71cb7244d04fa2715903ae438fdfd1a1c885c1

• Provider: github
• Repository: microsoft/BCApps
• PR: 11147
• Generation: 1
• Source head: 6c03409
• Base: c4be4de
• Created: 2026-09-09T19:42:17.829525Z
• Expires: 2026-09-10T19:42:17.829525Z

Summary

{"summary":"The error-label suffix and brittle full-sentence test assertions are valid maintainability findings. Rename the error message label to an Err suffix and make the tests use Assert.ExpectedError with the product ErrorInfo message instead of duplicating the localized sentence. The separate request to assert that no ErrorInfo action exists is not implementable through the public AL ErrorInfo API: it exposes AddAction but no action enumeration/getter. Adding a product-only test seam would increase scope without validating user-visible behavior, so the plan explains and declines that request."}

Approved actions

• fix-maintainability fix -> refs/heads/bugs/648962: Rename the error label and replace brittle duplicated full-message assertions with stable ExpectedError checks.
  Payload:

  {"allowed_files":["src/Apps/W1/Subcontracting/App/src/Purchase/SubcPurchaseHeaderExt.Codeunit.al","src/Apps/W1/Subcontracting/Test/Tests/SubcWIPTransCreateTest.Codeunit.al","src/Apps/W1/Subcontracting/Test/Tests/SubcSubcontractingTest.Codeunit.al"],"behavior":"Use an error-oriented suffix for the covered-demand label. In the affected tests, derive the expected message from CreateCoveredTransferErrorInfo(PurchaseHeader) and use Assert.ExpectedError after asserterror, avoiding duplicated localized message text while preserving the same behavioral assertions.","push_destination":"microsoft/BCApps:bugs/648962","source_branch":"bugs/648962","validation_commands":["git diff --check","pwsh -File build\\scripts\\RunTestsInBcContainer.ps1 -TestType IntegrationTest -parameters <isolated Subcontracting test-app container parameters>; run the changed tests in codeunits 139987 \"Subc. WIP Trans. Create Test\" and the affected Subc. Subcontracting Test codeunit"]}

• reply-label reply -> PRRT_kwDOJh2Tgs6gnzO-: Reply that the label suffix was corrected.
  Payload:

  {"body":"Fixed in {{commit_url}}. The covered-demand message label now uses an error-oriented `Err` suffix, matching its use as `ErrorInfo.Message` and the surrounding naming convention."}

  Depends on: fix-maintainability
• react-label reaction -> PRRC_kwDOJh2Tgs7sfZZj: Mark the valid label naming finding as useful.
  Payload:

  {"reaction":"+1"}

  Depends on: fix-maintainability
• resolve-label resolve -> PRRT_kwDOJh2Tgs6gnzO-: Resolve the label naming thread after the fix and reply are delivered.
  Payload:

  {"resolution":"resolved"}

  Depends on: reply-label
• reply-tests reply -> PRRT_kwDOJh2Tgs6gnzRf: Reply that the brittle error assertions were replaced.
  Payload:

  {"body":"Fixed in {{commit_url}}. The covered-demand tests now use `Assert.ExpectedError` with the message produced by `CreateCoveredTransferErrorInfo(PurchaseHeader)`, so the localized sentence is no longer duplicated inline."}

  Depends on: fix-maintainability
• react-tests reaction -> PRRC_kwDOJh2Tgs7sfZdu: Mark the valid test maintainability finding as useful.
  Payload:

  {"reaction":"+1"}

  Depends on: fix-maintainability
• resolve-tests resolve -> PRRT_kwDOJh2Tgs6gnzRf: Resolve the brittle assertion thread after the fix and reply are delivered.
  Payload:

  {"resolution":"resolved"}

  Depends on: reply-tests
• reply-action-coverage reply -> 5600826983: Explain why the requested no-action assertion is not added.
  Payload:

  {"body":"I am not adding a product-only test seam for this suggestion. The public AL `ErrorInfo` API supports adding actions but does not expose an action enumeration/getter that a test can use to assert absence. This test deletes the only transfer header and exercises the no-open-transfer branch; the production guard adds the action only when an outbound transfer header exists. The message and no-new-transfer assertions remain the observable contract.","location":"conversation"}

• react-action-coverage reaction -> 5600826983: Mark the unsupported action-enumeration suggestion as not applicable.
  Payload:

  {"reaction":"-1"}

  Depends on: reply-action-coverage
• report-outcome report -> 11147: Post the receipt-based outcome after the approved feedback actions complete.
  Payload:

  {"body":"Report the verified commit, validation results, two fixed/resolved inline findings, the explained unsupported coverage request, pipeline state, and any remaining blockers.","location":"conversation"}

  Depends on: resolve-label, resolve-tests, react-action-coverage

approve ps-40617e7286b4c7733730e114cc71cb7244d04fa2715903ae438fdfd1a1c885c1
reject ps-40617e7286b4c7733730e114cc71cb7244d04fa2715903ae438fdfd1a1c885c1

[Chethan Thopaiah (ChethanT)]

approve ps-40617e7286b4c7733730e114cc71cb7244d04fa2715903ae438fdfd1a1c885c1

[alexei-dobriansky]

Good Sense Reviewer - Reply

Thanks for the update. Responses to your points:

• Good Sense review - Agree. The round 2 Good Sense review had no open suggestions, and the follow-up comments do not change that result.
• Label suffix and error assertion cleanup - Agree. In the current head, CoveredTransferDemandMsg is used as ErrorInfo.Message, and the covered-demand tests compare a repeated full sentence through GetLastErrorText(). Renaming the label to an error suffix and deriving the expected error from the product helper is a valid maintainability cleanup.
• No-action assertion - Agree with not adding a product-only seam. The code only calls AddAction when an outbound transfer header exists, and the public ErrorInfo API exposes AddAction but no action getter or enumerator for a direct absence assertion.
• Approval command - No technical response needed. It only approves the Shepherd plan.

---

[AI-PR-REPLY] version=1 promptVersion=4 system=github pr=11147 reviewRound=2 by=alexei-dobriansky at=2026-09-10T06:03:12Z respondsTo=2026-09-09T20:08:02Z reviewKey=d834f9df88509b890a882adcc5df5b2f3f2ba292150cdb6eed5c2a93fa974f93

[Predrag Maricic (PredragMaricic)]

No blocking findings. The covered-demand state is set only for positive eligible demand fully satisfied by existing transfer activity; uncovered demand still creates lines, while zero or ineligible demand retains the existing error. The outbound navigation action is present only when a related non-return transfer header exists, and the focused tests cover open, in-transit, posted-only, zero-demand, ineligible, card, and filtered-list scenarios.

[Chethan Thopaiah (ChethanT)]

Regarding the posted-only no-action coverage suggestion: I am not adding a product-only test seam. The public AL ErrorInfo API supports adding actions but does not expose an action enumeration/getter that a test can use to assert absence. The test deletes the only transfer header and exercises the no-open-transfer branch; the production guard adds the action only when an outbound transfer header exists. The message and no-new-transfer assertions remain the observable contract.

[alexei-dobriansky]

Good Sense Reviewer - Round 3

Recommendation: Accept with Suggestions

What this PR does

This change separates positive subcontracting component or WIP demand that is already covered by transfer activity from cases where there is no eligible demand. The report now raises a covered-demand ErrorInfo only after the dry run finds positive demand fully covered by open, in-transit, or posted quantities, so uncovered demand still creates transfer lines and setup, routing, zero-quantity, and ineligible supply-method cases keep the generic error.

The round 3 update is limited to test and label cleanup. It renames the covered-demand message label with an error suffix and makes the covered-demand tests get the expected message from the product helper.

Status of previous suggestions

None from the previous round.

New observations (commits since round 2)

S1 (🟠 Moderate): Keep one independent message assertion
The covered-demand tests now read the expected message from the same helper that production code uses. If the required user-facing message is accidentally changed, these tests will still pass. Keep at least one direct assertion for the required message text.

Risk assessment and necessity

Risk: The production change is targeted to the no-new-transfer-lines path in SubcCreateTransfOrder.Report.al and does not change quantity calculation or transfer creation when uncovered demand exists. The remaining risk is test robustness for the exact covered-demand message; the latest cleanup makes the tests less able to catch an unintended text change.

Necessity: The change is needed because the action otherwise gives the same generic error for two different states: no eligible demand and demand that is already covered. The scope is appropriate for a bug fix, and the tests cover component, WIP open, WIP in transit, posted-only WIP, zero-demand, ineligible-demand, and outbound navigation paths.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11147 round=3 by=alexei-dobriansky at=2026-09-10T12:26:27Z lastSha=8c706a108285efc4f4bec40cbcaa4e62c034c1af reviewKey=b8502a3e90226096a3044ae1fe2b336cf5d0059052e946291a81b35f25eb5673 suggestions=S1@8ee7dfed:new parentRound=2

[Chethan Thopaiah (ChethanT)]

Addressed S1 in 2c83e54fe5. The covered-demand tests retain Assert.ExpectedError using the production helper, and CoveredComponentsExplainWhyNoTransferIsCreated now independently asserts the exact approved ErrorInfo.Message. This preserves maintainability while detecting unintended user-facing wording changes.

[github-actions]

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

NoWIPTransferCreatedWhenExpectedEqualsPostedQuantity says the retry must not create a new WIP transfer, but after asserterror PurchaseHeaderPage.CreateTransfOrdToSubcontractor.Invoke(); it verifies only the error text. If the implementation creates a new transfer header or line before raising the covered-demand error, this test still passes and misses the regression. Requery the transfer tables after the failing invoke and assert that no new WIP transfer header or line was created.

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

[alexei-dobriansky]

Good Sense Reviewer - Round 4

Recommendation: Accept

What this PR does

This change separates subcontracting demand that is already covered by transfer activity from cases where there is no eligible demand. The report now raises a covered-demand ErrorInfo only when positive component or WIP demand is fully covered, keeps the generic error for setup, zero-quantity, and ineligible supply-method cases, and offers navigation only when related outbound transfer orders still exist.

The round 4 update addresses the previous test concern by adding one direct assertion for the approved covered-demand message. It also sets DataClassification on the actionable error, which is consistent with nearby ErrorInfo patterns and does not change transfer creation logic.

Status of previous suggestions

ID	Title	Status	Author response
S1	Keep one independent message assertion	Addressed	The latest commits added a direct assertion for the required message text.

New observations (commits since round 3)

None - the new commits address the previous suggestion and add required ErrorInfo classification.

Risk assessment and necessity

Risk: The production change remains limited to error selection and navigation for the no-new-transfer-lines path. It does not change transfer quantity calculation or creation when uncovered demand exists; the latest DataClassification assignment only completes the ErrorInfo metadata.

Necessity: The change is needed because the action otherwise uses the same generic error for no eligible demand and for demand that is already covered. The scope is appropriate for the bug, and the tests now cover component coverage, WIP open and in-transit coverage, posted-only WIP coverage, generic-error fallbacks, and outbound navigation.

---

[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11147 round=4 by=alexei-dobriansky at=2026-09-11T09:49:52Z lastSha=e274b8e17860f38ae79d9d4e64432e1fc165d3a0 reviewKey=d6b3615742989405b28f43363147b04224e076aed33abf0876f499a2319c49e0 suggestions=S1@8ee7dfed:addressed parentRound=3