[Expense tests] Correct restricted permission assertions - #11451
Open
Prangshuman Das (t-prda) wants to merge 25 commits into
Open
Prangshuman Das (t-prda) wants to merge 25 commits into
Prangshuman Das (t-prda) wants to merge 25 commits into
Conversation
…able Dedicated follow-up to API auth PR #10085 after current-main run34867798875 confirmed these methods fail. Preserve the tested business/permission contracts; re-enable only the methods repaired by this layer. Static review passed; AL runtime validation remains pending CI. AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
github-actions
Bot
added
AL: Apps (W1)
Prangshuman Das (t-prda)
added this pull request to stack #11232
September 14, 2026 21:46
This was referenced Sep 14, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Anders (AndersLarsenMicrosoft)
added
SCM
Anders (AndersLarsenMicrosoft)
deployed
to
triage
September 17, 2026 09:09 — with 
GitHub Actions
Active
GitHub Actions
Active
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Prangshuman Das (t-prda)
removed this pull request from stack #11232
September 24, 2026 15:27
Prangshuman Das (t-prda)
added this pull request to stack #11863
September 24, 2026 15:27
…astructure Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Prangshuman Das (t-prda)
removed this pull request from stack #11863
September 25, 2026 08:46
Prangshuman Das (t-prda)
added this pull request to stack #11893
September 25, 2026 08:47
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
This was referenced Sep 25, 2026
Anders (AndersLarsenMicrosoft)
deployed
to
triage
September 29, 2026 13:50 — with
GitHub Actions
Active
GitHub Actions
Active
pull Bot
pushed a commit
to CarstenMertes/BCApps
that referenced
this pull request
Sep 29, 2026
## Purpose [AB#646383](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/646383) Add opt-in authentication to `Library - Graph Mgt.` without API re-enablement or pipeline changes. ## Design - Default `None` preserves existing behavior. The public enum/interface and public `API Test Auth Context` retain external provider extensibility. - The Microsoft provider stays `Internal`; context `Apply` stays internal. Internal visibility is API encapsulation, not authorization. - Credential precedence remains container file, then existing Key Vault only if the file is absent. Invalid configured credentials fail; successful passwords remain instance-scoped `SecretText`. - File credentials now use a private file-mapping provider directly, without replacing or clearing the session-wide Azure Key Vault provider/cache. No extra AL plaintext copy is introduced. - `OnAfterInitializeWebRequestWithURL` remains last. The pre-existing empty Graph `OnRun` is restored. The trust boundary is admitted OnPrem test code and environment credential access, not `Internal` visibility or a destination-URL restriction. ## Tests and scope All six AL contracts remain: default None, event ordering, provider reuse, instance scoping, same-provider reselection and deselection. The dedicated recorder is removed. The non-SingleInstance internal mock publishes the **test-only** `OnAfterConfigureAuthentication` event; the same manually bound test-codeunit instance owns `Library - Variable Storage` for both recording and verification. Initialization clears that instance's queue after prior failures; each contract drains it and calls `AssertEmpty`. The six source-pattern PowerShell checks are removed, not replaced by other AL-text assertions. The real 401/200/401 HTTP scenario is owned by uptake microsoft#11860 after credential provisioning; core has no dependency on that future workflow. This is not a claim of a complete provider-branch matrix. No caller migration, URL repair, credential provisioning, scheduling, exclusion or work-date rollout belongs to this core. Native stack #11893: **microsoft#10085 auth core -> microsoft#11862 URL/fixture prerequisites -> microsoft#11891 workflow infrastructure -> microsoft#11860 AL uptake -> microsoft#11224–microsoft#11230 -> microsoft#11322 -> microsoft#11451–microsoft#11454**. ## Current checkpoint Head `4a76bb71851c158083dbe4d22e84bf40a0577842`, tree `12739039e502a3feeea9e98694fb360ea959c606`. Merged main baseline remains `0a602a2481c93ebfe7b1d27d6016fb9926c42111` (permission cleanup from PR 11561). No additional main merge or code changes were made during finalization. Old `b195c7a`/`4eef8ac` tree-equality claims remain obsolete after the intentional baseline/auth changes. [Verified AL compile/publish/test run 36131334134](https://github.com/microsoft/BCApps/actions/runs/36131334134) **succeeded, attempt 2**, at exact validation head `4a76bb71851c158083dbe4d22e84bf40a0577842`. **113/113 test jobs and 113/113 cleanup steps succeeded.** Core has no credential-file provisioning/removal step (expected). W1 artifacts verify all **6 auth contracts**. Local Pester at the applicable checkpoint: **28 passed, zero failed/skipped**. Core local Pester: **28 passed**; no unsupported claim about a separate root PowerShell workflow. ## Validation limits For uptake/full, CU139496 `MicrosoftAuthenticationRespectsServerAuthMode` is verified in the actual UserPassword fixture (**401/200/401**). The Windows **200/200/200** expectations are implemented but **Windows runtime remains unverified**; no foreign local NST was used. Successful GitHub runs do not establish universal native NAV coverage. Excluded PDF cases, country-specific absent/excluded cases and tolerated-native distinctions are not claimed passing. No PR has been merged or auto-merged; validation drafts remain Do Not Merge outside stack #11893. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
… expectations Preserve all eleven upstream permission checks, including Expense Team and Expense Approval Setup, by observing under restricted permissions and asserting only after cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
This was referenced Sep 30, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Contributor
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThis change re-enables restricted Expense Agent permission tests. The latest delta preserves the upstream permission model by separating setup-maintenance expectations from document-edit expectations and adds checks for Expense Team and Expense Approval Setup. Status of previous suggestionsNone in round 1. New observationsNone. The observations are still captured while the restricted role is active and asserted only after full permissions are restored. Risk assessment and necessityRisk: Low. This is test-only and does not broaden production permissions. Necessity: The tests must verify that edit roles can maintain documents without receiving administrator-only setup access.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
…exclusion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // [SCENARIO] An employee-only caller cannot approve requests without access to Expense User data. | ||
| Initialize(); | ||
| CreateTravelRequestApprovalScenario(SpendRequest, ExpenseUser, Approver); | ||
| // Preserve the fixture for the post-denial checks when asserterror rolls back. |
Contributor
There was a problem hiding this comment.
The test adds an explicit Commit() but has no TransactionModel attribute, so it uses the test framework's AutoRollback default and can fail at the commit instead of validating the permission-denial behavior. Assign an appropriate transaction model for this committed fixture setup and ensure the test runner provides isolation for the committed data.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.47.6
Contributor
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThis change re-enables four restricted Expense Agent permission tests by capturing permission results and the expected denial while the restricted role is active, then asserting them after full permissions are restored. The current net diff still matches the bug: it avoids requiring Assert execution under the restricted role and keeps the denied approval fixture available for post-error checks. Status of previous suggestionsNone in round 2. New observations (commits since round 2)None. The two commits since round 2 only forward-merge parent changes. The two files in this PR are unchanged, and no new PR-specific hunk appears in the current net diff. Risk assessment and necessityRisk: Low. The change is test-only and does not alter production permission sets. Fresh runtime validation for the refreshed head remains pending, so successful CI is still needed before merge. Necessity: The change restores coverage for the permission-enforcement regression while ensuring all permission observations happen under the exact restricted role and all assertions run after permission cleanup.
|
Andres Salazar (asalazargeeky)
pushed a commit
to geekyllc/BCApps
that referenced
this pull request
Oct 1, 2026
microsoft#11654) ## What and why Consolidate the Expense Agent test repairs on current `main`, following merged microsoft#11333. - Fix the three Expense Mgmt. Read/Edit/Admin tests by collecting all nine permission observations under the exact restricted role, restoring test permissions, and only then calling Assert. No production permissions change. - Replace the invalid ordinary positive/negative posted-history fixture with a normally posted zero-amount line. This preserves genuine posted header/line history and zero recorded spend for all eight deletion, report-recreation, and reapproval cases. Keep identity, ownership, audit, duplicate-report and zero-spend assertions. - Re-enable Approve/Submit/Reject Travel Request page-action tests. Their WebServiceActionResultCode assertion compilation fix is already on main. - Classify codeunit 148339 as UnitTest, as requested; it exercises AL in-process, including direct page procedures. The separate HTTP API codeunit remains IntegrationTest. - Repair the foreign-currency fixture in six [AB#650247](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650247) API tests (the normal rate, not the adjustment rate, must be non-unit); refresh cross-session reads and verify persisted identities, dates and ownership without removing negative operations. - Remove exactly 14 role/history/action exclusions from the BCApps Expense Agent disabled-test manifest. No exclusions remain for codeunits 148338 or 148339 in this branch. - Preserve ALL HTTP API exclusions in BCApps, including the six [AB#650247](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650247) cases: BCApps still needs the separate API-authentication prerequisite. The fixture corrections are retained, but these tests must not be counted as executed in BCApps. NAV has working API authentication and enables those six tests separately. Related: [AB#650245](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650245), [AB#650277](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650277), [AB#625894](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/625894). NAV test-enablement/uptake also tracks [AB#650246](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650246) and [AB#650247](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650247). This supersedes the overlapping repair work in drafts microsoft#11451 and microsoft#11452. Their other already-upstream changes are not replayed. The older drafts remain open until their owners reconcile/close them after this work is merged. ## Validation - Static JSON/exclusion checks and `git diff --check` passed; read-only review found no significant issue. - Latest BCApps main was fetched and verified included: `0c994435e3ee038dbaf66822f8a2791b1aa3f43a`. - Expected complete BCApps Default suites on this source: 20 tests in codeunit 148338 and 74 in codeunit 148339, including all 14 re-enabled methods. HTTP API tests remain excluded in BCApps by explicit user decision; their runtime evidence must come from the manually queued NAV buddy build instead. - Local W1 tenant2-1 resolution initially timed out because MSSQLSERVER was stopped. After the user requested local repair, that stopped dependency was started and both relevant databases are ONLINE. Tenant2-1 remains Failed; its Base/System/Application/Expense/Test apps are uninstalled and Cleaned. Normal tenant synchronization reports the tenant is not mounted. Shared NST is still 29.0.54137.0, incompatible with a current full BaseApp30 build. - No local publication or tests have run. No running service was restarted, no tenant1-1 data/configuration was changed, and no version metadata, reset, or reprovisioning was performed. Further local repair awaits explicit approval for an isolated BC30 environment or a shared-environment upgrade. - API authentication uses the existing helper unchanged. PR microsoft#10085 contains a separate shared authentication-provider change; it is not silently imported here. Until that dependency lands, BCApps API exclusions remain identical to main. NAV buddy results must establish whether the historical request-disappearance symptom is resolved. - CI compile, publication, and actual per-method XML/log execution verification are pending. A green aggregate check alone is not sufficient: earlier CI could report success after a codeunit runtime-compilation failure. - Private NAV uptake draft: [NAV #254455](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_git/NAV/pullrequest/254455). It removes 21 NAV method exclusions, including the six API cases, and restores the Expense Agent Tests app to `Get-AppsToInstall` by removing the blanket runner filter introduced in `f09e1491f2adacca7a851a12587966daf91d993d`. - The user will queue its buddy build manually. NAV remains draft until BCApps merges, the pointer is updated to an actual main commit, and buddy logs prove app installation and per-method execution. Static runner selection is not runtime installation evidence. Whole-app NAV test exclusion and installation/execution validation: [AB#650370](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/650370). The scoped bugs 650245, 650246, 650247, 650277 and 650370 are Active while verification is pending. ## CI iteration: 79ef58 (superseded by RU fixture correction) Actual W1 Default JUnit artifact 10646077590 from run35596801127 records 148338 **20/20 passed** and 148339 **74/74 passed**, zero errors/skips, including all14 re-enabled methods. DE Default JUnit also records20/20+74/74. RU Default job106353051317 failed: 14833820/20passed, but all74 Spend Request tests failed during Initialize because PrepareNormalGenPostingSetup could not find a fully populated General Posting Setup in the UnitTest database. This is fixture initialization, not74 different product failures. Commit0ed0d44f92 creates a complete posting setup using existing Library - ERM helpers before the country-specific setup. New-head validation is pending; the prior-head W1/DE passes are not a claim that this new commit has passed. No tests were disabled for this failure. Local and NAV buddy execution caveats above remain. ## CI iteration: 0ed0d44 (superseded by CH VAT fixture correction) Actual JUnit from run35616107333 attempt2 verifies **W1 and RU each passed all20 permission tests and74 Spend Request tests**, zero failures/errors/skips. W1 artifact10658609682; RU artifact10658544297. This confirms the RU posting-setup correction. CH Default job106447478820 failed: permissions20/20passed; all74 Spend Request cases stopped in country VAT initialization because the selected Normal VAT template referenced a nonexistent Sales VAT Account. Artifact10659053532 and the job stack identify Library - ERM Country Data.CreateVATPostingSetup. Commit bd80ea3 initializes the VAT template before country setup, creating replacement Sales/Purchase VAT accounts only when the referenced account is missing. Valid account references and all assertions remain unchanged; no test is disabled. Fresh validation of W1, RU and CH on this new head is pending. Older-head pass evidence is not a claim that the new head passed. ## Prior-head test execution: bd80ea3 Downloaded actual JUnit artifacts from run35644720549 attempt2 and verified every required re-enabled method, not just the job conclusion: | Country | CU148338 Expense Permissions | CU148339 Spend Request | Artifact | | --- | --- | --- | --- | | W1 | 20/20 passed | 74/74 passed | 10668715386 | | RU | 20/20 passed | 74/74 passed | 10667802737 | | CH | 20/20 passed | 74/74 passed | 10668516148 | All three have zero failures, errors and skipped cases in these suites. All14 re-enabled role/posted-history/page-action cases executed. This verifies both country fixture corrections and placement in the Default/UnitTest lane. It does not establish fully test-owned VAT setup: the current helper repairs the selected existing VAT template when necessary. Run35644720549 attempt2 completed with160 successful jobs,2 skipped and2 failed (BE Uncategorized plus the aggregate status check). The status-check log names ONLY the BE platform/indexes/platform.json download/setup failure; no Expense test failure remains in the verified W1/RU/CH suites. The overall CI gate is therefore still red. No rerun was initiated by this agent. HTTP API tests remain intentionally excluded in BCApps; current NAV buddy and local runtime validation are not claimed. The user-supplied older NAV buddy3641608 did install Expense Agent and its test app and pass20 permission tests, but all74 Spend Request cases hit the pre-fix RU initialization failure; its queue timestamp preceded the RU correction. ## Current fixture repair: 2d45e6f The supplied NAV buddy3641753 AU log passed all20 permission tests but failed all74 Spend Request tests in UpdateZeroVATPercentInVATPostingSetup: the demo-data helper assumed an existing blank-business-group VAT row. Earlier BCApps AU also passed94/94 on bd80ea3; that baseline difference does not prove the fixtures are independent. Replace broad demo-data normalization rather than adding another country-specific fallback: - Create fresh Expense Posting Groups and their G/L accounts, fresh Employee Posting Groups with valid expense accounts, and fresh payment methods. Bind the report fixtures to these records instead of reusing arbitrary posting masters. - Remove the unrelated country VAT/general/purchase setup mutations and the earlier RU/CH template repairs. These Spend Request lifecycle fixtures use no-VAT expense accounts; retain the explicit country helper for Journal Templ. Name Mandatory=false. - Restore General Ledger, Human Resources, Source Code and Expense Agent singleton setup between tests with Library - Setup Storage. Retain existing scoped Expense cleanup; do not delete unrelated VAT or ledger masters. - Preserve all74 test methods, assertions, scenario/feature tags and handlers. No exclusions or production behavior changed. Validation: static call-site checks and git diff --check passed. Fresh local resolution now reports tenant2-1 Operational, but a dedicated tenant2-1 AL MCP download returned404 for System30.0.0.0 (No published package matches the provided arguments). Application symbols downloaded to the worktree/tenant cache; required symbols remain incomplete. No local compile/publication/test was performed, and no shared symbols, metadata changes, reset, service restart or tenant1-1 modification was used. Fresh CI and manually queued NAV buddy execution are pending; all bd80ea3 pass evidence above is prior-head only. Pipeline audit: both NAV and BCApps AU UnitTest setup use an empty company, not Contoso generation. BCApps can retain the Disabled runner across ordinary app passes; the exact origin of the differing AU VAT data is unproven. Open PR microsoft#10085 has overlapping clean-tenant/split-isolation remediation. No shared-runner changes are imported into this fixture PR. ## Warning-gate correction: 67d0e3c Run35714820873 on2d45e6f finished failed. RU Default and SE/IT Clean logs identify two new AA0198 warnings: the new global LibraryHumanResource shadowed two existing local declarations in SpendRequestTest. RU stopped at the compilation/new-warning gate before publication and tests; this was introduced by the fixture change, not infrastructure. Commit67d0e3c removes exactly those two redundant local declarations and reuses the global instance. All74 test bodies/assertions are unchanged; exact two-line diff and diff checks passed. Fresh CI compile/publication and actual W1/RU/CH/AU94-test execution remain pending. Monitoring resumes every30minutes; no buddy build or CI rerun is automatically queued, and local tenant-specific System30 symbols remain unavailable. ## Payment-method fixture correction: 87357b7 The67d0e3c app builds passed for W1/RU/CH/AU, clearing AA0198. Actual AT Default JUnit artifact10705006253 from run35730199203 records CU14833820/20passed and CU14833955/74passed,19failed. All19 failures are the same fixture defect: Expense Payment Method enforces uniqueness by nonblank Reimbursement Type, so creating a new Employee Paid method for each report collides with the prior method. This is not a product assertion failure. Commit87357b7 clears only Employee Paid/Company Paid payment-method fixtures in Initialize, after Expense transaction cleanup and before the initialized guard. The six factories use the existing find-or-create library helper: the first call creates this test's method, and further calls reuse it without violating uniqueness. Posting-account ownership and all74 scenarios/assertions/tags/handlers remain unchanged. Static checks verify every-test cleanup ordering, all six callers, preserved assertions and no HR-library shadowing; runtime verification on this new head is pending. The earlier run also has an independent LegacyTestsBucket1 container-setup failure downloading platform/indexes/platform.json (HTTPS connection timeout). No automatic retry or shared-runner/environment change is included. Local tenant-specific System30 symbols remain unavailable; latest NAV buddy execution remains unverified. ## Verified current-head runtime: 87357b7 Downloaded actual JUnit from run35751572706 attempt1 and checked complete suite counts, zero failures/errors/skips and every required re-enabled method: | Country | CU148338 Permissions | CU148339 Spend Request | Artifact | | --- | --- | --- | --- | | W1 | 20/20 passed | 74/74 passed | 10714234115 | | RU | 20/20 passed | 74/74 passed | 10714194977 | | CH | 20/20 passed | 74/74 passed | 10715030599 | | AU | 20/20 passed | 74/74 passed | 10714392965 | | AT | 20/20 passed | 74/74 passed | 10713924969 | All14 re-enabled methods executed in each country. This verifies the latest payment-method cleanup/reuse correction, including the AT regression. All five country app builds passed. W1 job106867710487 additionally records publication, synchronization and installation of both Expense Agent (Preview) and Expense Agent Tests30.0.2147483647.85851, followed by successful execution of both codeunits. The overall workflow is not green: it is still running, with US LegacyTestsBucket2 job106867713372 failed because its self-hosted runner lost communication with GitHub. That infrastructure failure is separate from the verified target suites; no automatic rerun was requested. NAV draft254455 remote commit e939957a13d896b18ae1bf053a14ab6949a052fd already consumes this exact87357b7 commit. Fresh NAV buddy verification remains necessary, especially Codeunit isolation and the six NAV-enabled HTTP API tests that remain excluded in BCApps. The supplied older NAV3642277 W1 log references67d0e3c and reproduces the now-corrected19 payment-method failures; it does not test87357b7. No current NAV/local runtime pass is claimed. ## Final BCApps CI gate Run35751572706 subsequently completed SUCCESS on attempt2 for the same87357b7 head:162 successful jobs,2 skipped,0 failed. The earlier US legacy-runner communication failure is cleared. The agent did not request this retry. Five-country actual20+74 execution evidence above remains valid for this exact source commit. NAV validation remains separate. ## NAV HTTP API follow-up: af10e62 (validation pending) The supplied NAV3642786 NL IntegrationTests log (build30.0.54998.2786) executes all15 CU148347 cases:7pass,8fail. These HTTP API cases remain intentionally excluded in BCApps, so the prior87357b7 unit-test/green-CI evidence does not validate them. Library - Graph Mgt. appends successful responses to the supplied ResponseText and raises failed requests before populating that output. The failures expose two test-harness defects: stale concatenated responses explain the missing traveler expansion and two foreign-currency assertions; parsing empty response JSON explains four negative-case failures. This test-only commit clears all32 HTTP response buffers and checks failed requests through the helper's raised error, preserving expected HTTP400/ALDialog, domain/owner/request-number details, the server BadRequest read-only-status guard, and persistence checks. All15 methods, existing tags and conditional compilation remain unchanged. The eighth failure is still under investigation: the dates case fails the first local GetBySystemId immediately after successful POST, before PATCH. The test now explicitly compares the returned id with the client-supplied id before the existing persistence check, so a future failure can distinguish rewritten/ignored identity from record disappearance. The supplied-GUID requirement is NOT removed and no production workaround is applied. Static diff/buffer/case checks pass. A fresh dedicated tenant2-1 symbol download retrieved5of6 requested dependencies but again failed404 for System30.0.0.0; no local compile, publish or tests ran. Fresh CI compilation is pending and cannot substitute for NAV API execution while BCApps exclusions remain. Another NAV buddy build has not been queued automatically; the prior queue authorization was used for2773217. Both drafts remain in-progress, not fully validated. Current follow-up head: ff267e3. The returned-identity diagnostic uses the existing AL Evaluate(Guid, JsonValue.AsText()) pattern. This supersedes af10e62; its identity investigation and runtime-validation caveats still apply. No additional NAV build has been queued. ## ff267 CI gate completed Run35869253473 attempt1 completed SUCCESS on ff267e3:162 successful jobs,2 skipped,0 failed. Actual W1 and RU build logs confirm Expense Agent Tests compiled, with unchanged warning baselines (40304 W1;42672 RU). CU148347 HTTP API cases remain excluded in BCApps: this verifies compilation and the CI gate, NOT the eight NAV API failures or the unresolved date POST identity. Local runtime validation remains pending; no NAV rerun was initiated by this monitor. ## Locally verified API persistence fix: 29d7bb5 The remaining two API failures were reproduced outside the test runner: user-scoped owner/date-only POST returned201 but no stored request (blank number; subsequent GET returned0). Adding purpose caused insertion, but the tests were NOT weakened with such a payload workaround. Fix page7134 instead: copy supplied date values onto Rec to register a real record change while retaining deferred paired validation; clear the route-prefilled owner on new records and default it at insertion only when requestedBy was omitted. Explicit supplied owners still undergo scope validation. Requested identity and all existing date/ownership/persistence assertions are retained. Local validation after the explicitly authorized BC30 W1 reset (agents enabled, two tenants), exclusively on tenant2-1: - Downloaded fresh tenant-specific symbols; built and published both Expense Agent (Preview) and Expense Agent Tests30.0.0.0 through a dedicated AL MCP host. - CU148347: all15 HTTP API tests PASSED,0failures/0skips, runner130451 (required isolation Disabled), CRONUS W1, password-only test administrator. Includes complete date-only PATCH sequence and both original missing-record cases. Evidence:api-local-insert-fix.xml. - Empty Company regression: CU14833820/20 andCU14833974/74 PASSED,0failures/0skips, runner130450 Codeunit isolation. Evidence:local-unit-regression-20260924.xml. The helper's 'Disabled' setup log describes code-coverage tracking, not isolation. - Additional direct HTTP checks passed: supplied-id/date-only POST persists; omitted owner defaults from route and persists; explicit blank owner and unscoped missing owner return400 without inserting. Local runtime builds use CodeCop/UICop with default severities; partner AppSourceCop defaults incorrectly reject first-party namespaces/IDs and the full repository ruleset promotes pre-existing obsolete/style warnings. No source warnings were suppressed or manifests altered; CI remains the full first-party compilation gate. Fresh current-head CI and NAV buddy results are still pending. BCApps HTTP API exclusions remain unchanged pending the separate authentication prerequisite; NAV keeps these APIs enabled. The Windows-linked local account was unsuitable for the existing Basic-auth helper, so a password-only test account was used on tenant2-1. No production authentication behavior was changed. Canonical reset-tool compatibility fixes remain local and are not included in this PR. ## Final merged-source local verification: aaa5a76 Merged current BCApps main (a18b1b3) normally to resolve the sole namespace-import conflict, retaining both imports and all upstream changes. Main contributes six additional Spend Request tests, bringing CU148339 to80; no scenarios were removed. Rebuilt and published Expense Agent and its tests from the merged source. The seed Base Application lacked the newly inherited Purpose-validation behavior, so the matching unmodified Base Application30.0.0.0 was built from the same source and published only to tenant2-1 (verified tenant scope). No BaseApp source workaround or version/metadata override was used. Final persisted local results on tenant2-1: **15/15 HTTP API +20/20 Expense Permissions +80/80 Spend Request =115/115 passed**, zero failures/skips. HTTP runner130451 in CRONUS W1; unit runner130450 in Empty Company. Reports:api-local-final-main.xml andlocal-unit-final-main.xml. The original date/owner POST payloads and persistence assertions pass without adding purpose. Source worktree is clean; compiler-generated report-layout edits were not committed. This supersedes the earlier unresolved-identity caveat. New-head CI and NAV buddy results remain pending; BCApps API exclusions and NAV API enablement policy are unchanged. ## W1 Bucket5 fixture follow-up: 40a64b5 The supplied NAV3644685 log reports4,814passed/2failed: CU139194.CDSConnectionWizardCheckModifyCDSConnectionURL rejects a positive fixture outside dynamics.com; CU139148.NotExpiredToken fails its expiry assertion. These are separate from the repaired Expense APIs. Test-only corrections: use valid dynamics.com hosts for positive HTTPS normalization cases, preserve explicit-port/path expectations and HTTP rejection, and exercise SaaS validation with environment restoration. JWT fixtures now use the public Unix Timestamp helper rather than manual timezone/epoch arithmetic, with a one-hour past/future margin instead of one second. Production URL/security and JWT-expiry implementations are unchanged; no further tests are disabled. Both test apps compile locally through AL MCP with tenant2-1 symbols. The corrected CDS case passes locally. Full local validation is incomplete: the CRM suite has pre-existing local AutoRollback/EmailLogging and on-premises-mode setup failures; the latter reproduces independently of the modified case. Tests-Misc server publication rejects its existing MockAzureKeyVaultSecretProvider reference despite successful local compilation. The missing standard MockTest add-in was restored, but no service restart or validation bypass was performed. Token runtime and full-suite verification must come from the buddy build or follow-up local runtime maintenance. No full-pass claim is made for these two codeunits. The prior115 local Expense API/permission/Spend passes remain evidence for unchanged Expense code, not proof of these new fixture repairs. Existing NAV retention/VIES quarantines and differing BCApps/NAV API policies remain unchanged. ## Assisted-setup fixture follow-up: c52631a After ADO2787333 failed, the matching WorkIQ email for NAV3644810 identified two RU Bucket5 failures in CU139196: RunAssistedSetupFromNormalSetupRecordMissing and RunAssistedSetupFromNormalSetupRecordExists. Both used TEST as a positive Server Address and hit the dynamics.com host guard. Only those two fixture URLs become https://test.dynamics.com, with corresponding comments updated. The existing wizard-propagation assertions, handlers, test list and production validation remain unchanged; no additional exclusions were added. The CRM test app compiled and published on tenant2-1. Local runtime verification is blocked before these assertions: unchanged Initialize calls DisableEncryption, which is unsupported in this agent-enabled environment (47 reported,4 passed,43 failed in setup). No encryption/credential/service changes or assertion bypass were performed. NAV buddy execution is the remaining validation route; these two cases are not yet claimed passing. ## Scope correction: 5de4cb7 At the user's explicit request, this BCApps PR is limited to Expense Agent and Spend Request changes. Our earlier changes to CDSConnectionWizardTests, CDSConnectionSetupTest and UTREST have been restored exactly to their pre-repair aaa5 contents. The final PR diff contains only four Expense Agent AL files and its disabled-test manifest; the historical non-Expense fixture proposals above are superseded and NOT included. NAV handles unrelated failures through exact-method temporary exclusions instead: the four confirmed Bucket5 methods (one CDS wizard URL case, two CDS assisted-setup cases and UT REST.NotExpiredToken), alongside the previously authorized four Retention Policy and ten VIES methods. These are quarantines, not fixes or passing tests; bug filing remains deferred. All other existing exclusions are preserved. BCApps keeps the Expense non-API re-enables and its HTTP API exclusions. NAV retains the Expense app re-enable and its21 method re-enables, including the six HTTP API cases. The prior local15API+20permissions+80Spend passes apply to unchanged Expense code; fresh CI/buddy results remain pending. Future repairs in this BCApps PR must not modify unrelated tests. --------- Copilot-Session: 988fb581-008c-4e0d-99c1-285cd45d49e7
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Contributor
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThis change re-enables the restricted-permission approval test by preserving its fixture, capturing the permission denial while the employee-only role is active, and checking the business state after full permissions are restored. The sequence is sound: the fixture is committed before Status of previous suggestionsNone in round 3. New observations (commits since round 3)None. The four non-merge commits since round 3 do not overlap the current net PR diff. Changes inherited from the stacked base are not attributed to this PR. Risk assessment and necessityRisk: Low. The net change is limited to test setup, assertions, and disabled-test configuration; it does not change production behavior or public interfaces. Fresh exact-head CI is still pending, but that does not expose a concrete defect in the diff. Necessity: The change restores regression coverage for the employee-only approval denial and verifies that a failed approval leaves the released request unchanged and creates no expense report.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Contributor
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThis change re-enables the restricted-permission approval test by running the denial under the exact employee-only role, capturing the error before permission cleanup, and checking the database state after full permissions are restored. The current net change correctly verifies the denial reason, restricted read access, unchanged request status, and absence of a created expense report. Status of previous suggestionsNone in round 4. New observations (commits since round 4)None. The new non-merge work since round 4 does not overlap the current net PR diff, so no new PR-specific code requires review. Risk assessment and necessityRisk: Low. The net change is limited to test behavior and disabled-test configuration; it does not change production permissions or public interfaces. Exact-head CI is still running, but no concrete failure is present. Necessity: The test must preserve its fixture across the expected rollback while proving that the restricted role cannot approve the request or create an expense report. The change provides those checks without broadening permissions.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
AB#646383
Permission-test layer above #11322 in native stack #11893. Authentication is isolated in #10085; shared prerequisites, workflow infrastructure and AL uptake are #11862, #11891 and #11860.
Re-enable exactly four CU 148338 methods: the three
ExpenseMgmt*RetainsAppPermissionscases andTravelRequestApprovalFailsWithoutExpensePermissions. The first three also correspond to upstream AB#650245.Collect all nine permission observations under the exact restricted role, restore test permissions, then assert the captured values. For denied approval, retain the committed fixture and restricted action, capture error code/text immediately, and verify
DB:ClientReadDenied, the Expense User table, request survival/status and no-report postconditions after cleanup.Reconciled with the upstream Entra permission changes: all added imports, labels, Super activation/deactivation tests and helpers remain. No production role permissions are broadened.
Validation
Exact parent ancestry, file scope, whitespace and exclusion ownership checks pass. This layer now preserves upstream CanMaintainSetup role semantics and its Expense Team/Approval Setup checks: all eleven observations are captured while restricted and asserted after permission cleanup. Other owned changes remain unchanged. Upstream changes are inherited through the main merge, not reverted. Earlier exact-head cumulative artifact evidence is historical after this parent reconciliation; fresh new-head runtime validation is pending. Existing excluded PDF, IN/RU gaps and tolerated-native distinctions remain; no absent/excluded case is claimed passing.
Validation limits
The previous UserPassword HTTP 401/200/401 result is historical after this reconciliation. CU139496
MicrosoftAuthenticationRespectsServerAuthModestill executes all three requests; Windows200/200/200 runtime remains unverified. The workflow clean-codeunit gate stays default-off and uptake stays explicitly enabled. No provider, authentication contract, new public event, NAV selector or foreign NST change was introduced. Excluded PDF cases and absent/excluded country/native cases remain unverified; prior tolerated-native results are not universal passes. The 59 owned re-enabled methods cover the reviewed fixes, not59 distinct product defects. Validation drafts remain Do Not Merge, outside native stack #11893.Current checkpoint
Head
460383d69ec0dbf780a9501bb2ba6cf696d2c76c, tree73fe1439d8c798280085d622def809160fdbf821; parentb1973cf1bd2f40f58038b95adfae8ff45fd51565.Forward-integrated captured main
bb7111877ff786951b86a1a0f80d8b39b8f5dacd, including upstream CLEAN27 removal82b11d26c073de93df3aab17434069f72feab640(PR12066), to align the direct stacked-PR warning gate. The prior direct uptake checkout retained obsolete source while warning-reference run37020063048 used main73d5794e; RU and CH therefore each reported51 additional warnings (36 AA0244,15 AA0218). Main-targeted RU validations already passed with cleaned source. No warning suppression, parameter rename, partial cherry-pick, or comparator change was made.Local Pester:117 passed, zero failed/skipped at exact workflow, uptake and full heads. Every layer retains its exact owned patch; all changed baseline blobs equal captured main, and all remaining blobs—including exclusions—are unchanged. The23 committed wrappers/shared success-only finalizer, generator removal, current-process credential identity, ACLs, buffer clearing and narrowed platform classifier remain intact; ordinary configured reruns are unchanged.
Accepted cleanup limitation: failed/cancelled runs rely on normal container teardown; no hard-runner-loss guarantee. Normal CI uses per-run disposable credentials, but supplied credentials may differ. Hook logs prove invocation, not necessarily explicit deletion if teardown already removed the file.
Uptake retains the five query-safe URL compositions in CU148343
StandardSubmissionExposesPolicySnapshot, all assertions,8 methods, setup restoration and the single unlimited-approval fixture. Workflow remains default-off; uptake enables clean execution. Auth visibility/provider contracts and HTTP scenario are preserved. Upstream shared Spend Request zero-amount UnitTest semantics and permission cleanup from PR11561 remain unchanged. CU139806TestGetCompanyAndEnvironmentDescriptionsstays excluded pending its original rationale/current NAV verification (PR11741).Fresh exact-head GitHub CI is pending, not passed. Previous runs are historical for these new commits. Verify all8 activity methods across22 countries if present; Windows runtime and excluded PDF/native coverage remain unverified. Only this captured main was integrated—no repeated baseline chasing. Merged prefix/native stack #11893 and draft/ready states remain unchanged; validation drafts remain Do Not Merge.