Repository navigation
Revert Expense API test re-enablement and consolidated fixes - #12514
Conversation
…se fixes" This reverts commit 3f357d6 following the integration test failures in workflow run 37598783939. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThis rolls back clean-codeunit execution, restores Expense API exclusions and authentication support, and removes temporary general API exclusions to mitigate unstable HTTP 500 failures. Those test-infrastructure changes fit the mitigation, but the rollback also restores a production approver filter that can include users who were not assigned to that approver. Problem-solution fitFit: Partial The rollback reduces the failing test coverage as intended, but it also reverses an independent authorization fix that is not needed for that mitigation. SuggestionsS1 (🔴 High): Keep exact approver membership filtering Risk assessment and necessityRisk: An approver can see pending travel requests assigned to another approver when an unassigned user number falls between two assigned values. The BaseApp Necessity: The CI mitigation is necessary, but reverting this separate production authorization fix is not required. Keep that fix and its focused regression tests while reverting the unstable test enablement.
|
97c2f03
into
main
Picks up the revert of #12325, which broke the Expense Agent API IntegrationTests for all PRs targeting main. AB#647832 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…soft#11733) <!-- Thanks for contributing to BCApps! A few things before you hit "Create pull request": - Your PR must link to an approved issue. New here? See CONTRIBUTING.md. - You must have built and run your change yourself. CI is a safety net, not a substitute. - If you used AI or an agent to write this PR, you are still the author. Read the diff, build it, and try it before requesting review. Contributing guide: https://github.com/microsoft/BCApps/blob/main/CONTRIBUTING.md Local dev environment: https://github.com/microsoft/BCApps/blob/main/LOCAL_DEV_ENV.md --> ## What & why <!-- A few sentences: what does this change do, and what problem does it solve? --> Attachment filenames such as `Invoice Nov'25.pdf` fail migration because generated external paths violate storage validation. Storage Sync previously hid individual reasons behind an aggregate failure count. - Sanitize generated storage/media-import filenames without changing displayed File Name or File Extension. Preserve GUID uniqueness, folder layout, literal hash/percent characters, and saved External File Path readback without regeneration or decoding. - Isolate each synchronization step through Boolean worker.Run. Successful writes commit; runtime failures roll back that step and processing continues. Register every failed attachment locally; interactive runs also show Error Messages. Background runs register failures without intentionally failing the job queue entry. - Retain uncertain uploaded files after failed attachment persistence. Record their created logical paths for recovery; no compensation/by-path deletion API remains. - Eliminate all DA-app physical remote deletion calls, including the baseline helper/subscriber and both public compatibility overloads. The worker explicitly invokes RetireExternalReference, not a deletion step. - Adopt the canonical local-retirement guard from [microsoft#12432](microsoft#12432) at `5e1de1c93383b0bdc9df5b905ba47040b5cc53c0`: fresh permanent row and media under update locks, unchanged requested identity/path/upload/source/media snapshot, Stored Internally, and actual nonempty Tenant Media Content. True means LOCAL reference retired only; remote bytes remain. Temporary, missing, stale and external-only rows are blocked. - Copy preserves both references. To Internal Move first retires already-valid internal content without HTTP; otherwise it restores nonempty bytes and then attempts guarded local retirement. Summaries distinguish retained remote files, retired local references and blocked retirements. The page/setup text no longer promises remote deletion. - Preserve per-record diagnostics and sanitized failure telemetry (`0000VL5`); retention uses canonical `0000W5Q`/`0000W5R` with non-content dimensions. Add restored/external-only/stale worker and compatibility-overload regressions. This PR is standalone against main and does not require [microsoft#12438](microsoft#12438). That proposal concerns internal release, not remote ownership or deletion authority. Remote retention and guarded LOCAL external-reference retirement are intentional independently of whether microsoft#12438 ships; no future physical remote deletion is promised. ## Linked work <!-- Required: link an approved GitHub issue using "Fixes #<number>". Microsoft contributors: also link the ADO work item with "AB#<number>" if you have one. --> Fixes [AB#647832](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/647832) GitHub issue: N/A; this fix is tracked by the approved Azure DevOps bug. ## How I validated this - [x] 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. - [x] I added or updated tests for the new behavior, or explained below why none are needed. **What I tested and the outcome** *(required - be specific: scenarios, commands, screenshots for UI changes)* <!-- Example: - Ran the new "Post and Send" action on a sales invoice in a fresh container; document posted and email queued (see screenshot). - New unit tests in MyFeatureTest.Codeunit.al pass locally; full module test suite green. - No tests added because change is comment-only / refactor with existing coverage. --> Current head: `234464d4f3990fd1a7b22463bd80dd40ae62f3ce` (documentation-only follow-up, 2026-10-08). Code head inspected: `103e034068185e61930c946fbbadd16dc48ac6f2`. The author merged main in `8c139c3a29` to include the upstream Expense API revert [microsoft#12514](microsoft#12514), then corrected telemetry dimensions, permissions, and a translator comment. This follow-up changes documentation only; no runtime redesign, PR-base change, force push, or sibling source import. - `git diff --check`: passed for the documentation-only follow-up. No new AL code or tests are introduced by this follow-up. - `gh run view 37742245234 --repo microsoft/BCApps --json headSha,status,conclusion,jobs`: one-shot inspection showed the code-head run still in progress, 49 jobs completed, 113 in progress, and no failed completed jobs. This is not a passing run or current-head test evidence; no monitoring loop or automatic retry was started. - At preceding head `ea863079ededfb678e4d4359aeb883b8d9cc29f4`, `gh run download 37517120439 --repo microsoft/BCApps --pattern '*-TestResults-*' --dir <session-artifact-directory>` succeeded. Parsing the actual XML proved all 56 declared attachment tests executed and passed in each of 23 country/default artifacts (1,288 passes, no skips or missing attachment cases), on platform `30.0.55814.0`. W1/System Default and Clean builds passed. The run still failed in 20 Expense API integration jobs; matched server event logs show a SQL-connection/metadata OData null reference. This is historical evidence, not proof for `103e034068` or this documentation commit. [Detailed evidence](microsoft#11733 (comment)). - Earlier own-head `python verify_combined_retention.py .` source assertions passed at `063c259a0a`; that is source-only historical evidence, not current-head compilation or runtime proof. - The old disposable three-head source integration is superseded and must not be applied. It predates current heads and the changed internal-release contract; its queued-cleanup assertions are obsolete. It is not a prerequisite or acceptance gate for this standalone PR. - No local BC30 compilation or `Run-TestsInBcContainer` execution for this documentation follow-up: no approved local BC30 compiler/runtime is available and the local helper previously reported missing Docker permissions. Repository setup is documented in `LOCAL_DEV_ENV.md`; no container was created or deployed. Current code-head runtime results remain unverified. Historical evidence only: preceding head `da749476` compiled the app/tests in W1 Default/Clean with compiler `30.0.43.2701`, with no diagnostics in their compiler blocks. Earlier author-reported validation reproduced the apostrophe failure, compiled in leased BC artifact `30.0.54812.0`, ran `Run-TestsInBcContainer` for `136820` with 48/48 passing tests, and captured six interactive telemetry events plus a non-GUI invocation. That earlier invocation raised the first failure and used a temporary event tag. Those older results also do not validate the current head. ## Risk & compatibility <!-- Anything reviewers should watch for: breaking changes, upgrade/data impact, permissions, telemetry, feature flags, follow-up work. Write "None" if there's nothing to call out. --> No schema changes or existing external-file renames. Display names stay unchanged. The user-approved retention policy intentionally changes deletion semantics: both DeleteFromExternalStorage overloads return true only for verified LOCAL metadata retirement, never remote deletion. Remote files may remain after record deletion, successful restoration or failed uploads; a logged logical path is diagnostic, not ownership/deletion authority. **Remaining standalone review and validation gates; no merge-ready claim:** - Dedicated privacy-notice approval remains a product/privacy-owner decision. Setup disclaimer and manifest privacyStatement are not runtime approval. Uploads, downloads and outbound metadata require consistent policy; no interactive consent prompt was inserted into a write transaction. - Missing Discovered uptake is pre-existing in setup/discovery paths. The existing Used event now follows successful persistence, but discovery lifecycle remains unresolved; no fake Discovered event was added at upload time. - The public `DeleteFromExternalStorage` compatibility/deprecation decision remains open in two review threads. Both aliases retain their signatures but return true only for guarded LOCAL retirement. Restoring physical remote deletion would contradict the user-approved retention policy; preserving signatures does not erase the semantic change. No deprecation decision has been made on the owner's behalf. - The report `AddLoadFields` performance review remains open and needs its own scoped disposition; it is unrelated to microsoft#12438. - Internal release remains this PR's separate synchronous implementation against main, with fresh/stale checks. It is not an asynchronous queue and does not establish globally owner-safe internal-media release. No microsoft#12438 manager, provenance table, queue/finalizer, or cancellation hook is required or imported here. - Any future combination with [microsoft#12438](microsoft#12438) is optional work requiring a new, explicit integration decision. Its latest proposed internal-release contract blocks release and retains all internal references/bytes; it is not a remote-deletion foundation. Historical queued-success/source integration is superseded, not something this PR must restore or wait for. - Zero reachable DA-app remote deletion remains this PR's own acceptance requirement. Local external-reference retirement remains allowed only after the locked/nonempty-content guard, independently of future internal-release work. - [microsoft#12064](microsoft#12064 field14 URL/Decoded Path support has merged into main and is included through the author's main merge. Literal filename percent/hash values and persisted readback paths must remain untouched. [microsoft#12012](microsoft#12012) trailing-dot folders and [microsoft#11143](microsoft#11143) broader colon/apostrophe restrictions remain separate gaps. - Current-head BC30 compilation, analyzer output, and regression execution remain required. Historical standalone shared-media tests do not prove every ownership/concurrency scenario. No unsupported test or reviewer result is described as a pass; optional cross-PR validation is not a prerequisite for landing this PR. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Review focus: the Expense test changes and the auth-helper deletion; the ``enableCleanTestCodeunitExecution`` line. The large JSON diffs are temporary disables, undone in C. Re-enables the Expense Agent API tests on top of the default web service key auth. No Key Vault password and no auth subscriber. - Deletes `Expense API Test Auth Helper` and its usages. - Re-applies the Expense fixes from microsoft#12325 (reverted in microsoft#12514): `TravelRequestApproval`, Expense permission and spend-request tests, the `ExpDetailTest` permission set, and `SpendRequestTests`. - Turns on `enableCleanTestCodeunitExecution`, as microsoft#12325 did, so `RequiredTestIsolation = Disabled` codeunits run in clean-tenant lanes. - Expense disabled tests: keeps only the entries NAV master also disables (12). - Temporarily disables the unrelated APIV1/APIV2 tests that the clean lanes start executing, the same entries microsoft#12325 added. They are re-enabled in the next layers. Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Fixes AB#653393
What & why
Revert commit 3f357d6, merged through #12325, as requested following the failures in workflow run 37598783939.
This is the full 21-file revert, not just a test-exclusion change. It removes
enableCleanTestCodeunitExecution: true, restores the prior Expense API authentication helper and 51 Expense API exclusions, removes the 193 temporary general API exclusions introduced by that commit, and rolls back its consolidated production/test changes. Subsequent AL-Go settings updates are preserved.The run has 19 integration-test jobs with Expense API HTTP 500 failures, two runner-disconnection failures, and one downstream status-check failure. Italy also has ten Avalara E2E test failures caused by shipment number-series date validation. The API errors report
Object reference not set to an instance of an object; the available test call stacks do not identify the server-side null reference. This rollback mitigates the related enablement change, but does not prove that the underlying runtime defect is fixed.Failure breakdown for all 22 jobs
EmployeesAPICanFilterExpenseUsersinExpense Users API Test(148315).CapabilitiesProjectsEnabledViaAPIinExpense Capabilities API Test(148318).ActivityLogEntryIsExposedThroughReadOnlyAPIinExpense Activity Log API Test(148343).Each failed because its GET returned HTTP 500 with a null-reference error instead of HTTP 200. Verified against complete test-job logs, not just the generic
There are test failures!wrapper or truncated annotations.You cannot assign new numbers from the number series S-SHPT on a date before 10/07/26.Italy's additional failures are in
End-to-End Tests(133625), appE-Document Connector - Avalara Tests:E2E_OutboundInvoice_Error_ManualResend_Complete,E2E_MultipleDocuments_IndependentLifecycles,E2E_ServiceDown_ThenRecover,E2E_ActivationThenSubmit_Complete,E2E_ConnectionSetup_SelectCompany_ThenSubmit,E2E_SendModeTest_SubmitSucceeds,E2E_BlockedMandate_Unblock_Resend_Complete,E2E_StatusCheck_HttpError_RetrySucceeds,E2E_PageVerification_AllStatusTransitions, andE2E_MessageResponseHeader_PopulatedDuringGetResponse. Their call stacks reachLibrary - E-Document.PostInvoice->Sales-Post.UpdateShippingNo->No. Series.GetNextNo, before the intended connector lifecycle is exercised.The runner annotations establish lost communication, but do not distinguish machine termination, resource exhaustion, or network failure.
Linked work
Tracking bug: AB#653393 - intermittent Expense API HTTP 500 / SQL-pool null-reference failures after clean-tenant resets. Related to AB#646383. This is a non-closing link: the rollback mitigates CI blocking by reducing coverage, but does not establish that the underlying runtime defect is fixed.
Reverts #12325. Related failure run: https://github.com/microsoft/BCApps/actions/runs/37598783939.
No approved GitHub issue was supplied for this rollback; the original PR has no closing GitHub issue reference. An approved issue link remains to be added before this draft is ready for review. No issue or work item is claimed fixed.
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Invoke-Pester -Path '.\build\scripts\tests\ParallelTestExecution.Test.ps1' -PassThru— passed: 87 tests, 0 failed, 0 skipped. Covers existing clean-codeunit selection, ordinary execution, and rerun infrastructure; not an AL integration run.git ls-tree 3f357d6c93edab81eab2437909d8dab17637ba0d^ -- <path>withgit ls-files --stage -- <path>for every affected non-settings file — passed: all 20 exactly match the original parent blobs, including the restored helper and removed permission set. Verified exactly the original 21 changed paths. Reviewed the AL source diff and verified the settings diff only removes the enablement switch.Get-Content -LiteralPath <path> -Raw | ConvertFrom-Jsonfor each changed JSON file — passed.git diff --cached --check— failed only on the restored helper's original blank line at EOF. Preserved that line for an exact revert.git -c core.whitespace=-blank-at-eof diff --cached --check— passed.LOCAL_DEV_ENV.mdandbuild/scripts/tests/runTests.ps1. AL compilation and Business Central runtime tests were not run locally; no dedicated BC development container was provisioned for this rollback. Human full-diff review and runtime checkboxes remain open. CI results for this revert are pending.Risk & compatibility
This full rollback includes production travel-request approver filtering and the associated permission/filter/date/test-fixture fixes. Restoring
SelectionFilterManagement.GetSelectionFilterremoves the exact-user filter fix and can reintroduce the range-gap behavior it addressed. Coverage is reduced by restoring Expense API exclusions and removing the clean-codeunit opt-in. Reintroduce the intended fixes and API coverage in a follow-up once their failures are understood.This change does not repair the two disconnected runners or Italy's independent Avalara number-series date failure. It also does not establish that the server-side HTTP 500 null reference is resolved.