Repository navigation
[API tests] Return shipment PDF selection and pdfDocument test isolation - #12683
Prangshuman Das (t-prda) wants to merge 1 commit into
Conversation
github-actions
Bot
added
Build: Automation
github-actions
Bot
added
Ownership: Needs Review
7da98dd to
090cdd6
Compare
090cdd6 to
40b1db3
Compare
40b1db3 to
a22a1ff
Compare
|
The newly enabled PDF expansion test does not assert the Boolean result of GetPropertyValueFromJSON, so a nonempty response that omits the pdfDocument property can still pass. Assert that the property lookup succeeds. Suggested fix (apply manually — could not be anchored as a one-click suggestion): Assert.IsTrue(LibraryGraphMgt.GetPropertyValueFromJSON(ResponseText, 'pdfDocument', PdfDocumentValue), 'Could not find pdfDocument');Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
2 similar comments
|
The newly enabled PDF expansion test does not assert the Boolean result of GetPropertyValueFromJSON, so a nonempty response that omits the pdfDocument property can still pass. Assert that the property lookup succeeds. Suggested fix (apply manually — could not be anchored as a one-click suggestion): Assert.IsTrue(LibraryGraphMgt.GetPropertyValueFromJSON(ResponseText, 'pdfDocument', PdfDocumentValue), 'Could not find pdfDocument');Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
|
The newly enabled PDF expansion test does not assert the Boolean result of GetPropertyValueFromJSON, so a nonempty response that omits the pdfDocument property can still pass. Assert that the property lookup succeeds. Suggested fix (apply manually — could not be anchored as a one-click suggestion): Assert.IsTrue(LibraryGraphMgt.GetPropertyValueFromJSON(ResponseText, 'pdfDocument', PdfDocumentValue), 'Could not find pdfDocument');Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
a22a1ff to
717f320
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
717f320 to
a512e47
Compare
Good Sense Reviewer - Round 2Recommendation: Accept with SuggestionsWhat this PR doesThe rebased commit keeps the same functional change: posted return-shipment PDFs now use the posted return-shipment report selection, and three PDF expansion tests are re-enabled with test isolation disabled. The production mapping matches the posted purchase-document pattern and the Status of previous suggestionsRound 1 had no suggestions. New observations (commits since round 1)S1 (🟠 Moderate): Assert the expanded PDF property exists Risk assessment and necessityRisk: The production change is limited to one report-selection value and follows the matching posted-document pattern. The remaining risk is weaker regression detection because the re-enabled tests do not fail when the expanded property is missing. Necessity: Without this change, posted return shipments can use the purchase return-order report selection and the affected PDF scenarios remain excluded. The scope is narrow and appropriate for restoring this behavior and coverage.
|
a512e47 to
fb5522a
Compare
Good Sense Reviewer - Round 3Recommendation: Accept with SuggestionsWhat this PR doesPosted return-shipment PDFs now use the posted return-shipment report selection. Three PDF expansion tests are re-enabled, with test isolation disabled and printable lines added to the purchase quote and blanket order. The report selection matches the posted shipment record and the corresponding posted-document handlers. The rebased change keeps the same behavior as the previous round; the PDF property assertions have not changed. Problem-solution fitFit: Strong The change directly corrects the report-selection mismatch and restores the affected PDF tests. It does not itself fix the separate platform SQL connection-pool problem. Status of previous suggestions
S1 (🟠 Moderate): Assert that each expanded PDF property exists New observations (commits since round 2)None — the rebase preserves the same scoped changes. Risk assessment and necessityRisk: The production change affects report selection and PDF attachment naming, not posting, amounts, or public signatures. Disabling test isolation follows the existing HTTP API test pattern, but the missing property assertions still weaken regression detection. Necessity: Without the change, posted return shipments use the return-order report selection and the three PDF expansion tests remain excluded. The scope is appropriate for correcting PDF generation and restoring coverage.
|
fb5522a to
b0bfbfa
Compare
Select the purchase return shipment report for return shipment PDF documents, run the PDF document codeunits without test isolation and re-enable their TestExpandPdfDocument tests. AB#646383 AB#653393 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
b0bfbfa to
4fa9ecd
Compare
Review focus: ``MicrosoftTestAuthProvider`` (about 60 lines) and the enum default; the ``build/`` changes are deletions plus the anti-SSRF line in ``NewBcContainer.ps1``. Defaults API test authentication (`Library - Graph Mgt`) to the current user's web service key. API tests no longer need per-app auth uptake or a container password. - `API Test Authentication` enum: `DefaultImplementation` = `Microsoft Test Auth Provider`. Removes `No API Test Auth Provider` and the explicit `Microsoft Test Environment` value. - `Microsoft Test Auth Provider`: on-premises with NavUserPassword, sends Basic auth with `UserId()` and the web service key. When the key is missing or expired, a background session (`API Test Web Service Key`, CU 131025) creates it with a 24h expiry and commits it, because the API request runs in a separate server session. The caller's transaction is never committed; the provider waits up to 60 s for the session and then re-reads the key. - `Library - Graph Mgt`: adds the `SetLicenseSafeWorkDate` and `AddFieldToIgnoreIfExists` helpers used by later layers. - Build: removes the container password and Key Vault credential plumbing (`ApiTestCredential.psm1`, `Remove-ApiTestPassword.ps1`, `PipelineFinalize` hooks and their tests). Keeps the clean-tenant execution logic. Sets `NavHttpClientAntiSSRFEnabled=false` in the test container, as NAV `RunALTests.ps1` does: since microsoft#12323, API tests call the container's own endpoint through AL `HttpClient`, which anti-SSRF blocks. - CU 139494 `API Test Auth Provider Tests`: covers key creation and renewal. Declares `TestHttpRequestPolicy = AllowAllOutboundRequests` (NAV blocks outbound requests by default; microsoft#12694 covered the other API tests). 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
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
Review focus: mostly mechanical. ``Initialize`` sets a license-safe work date in each API test, and the JSON deletions re-enable tests. A few files have small fixture or isolation tweaks. Re-enables the APIV1, APIV2, IRS 1099 and E-Document API tests that pass without area-specific fixes. Aligned with NAV master: only tests that NAV master also runs are re-enabled. - API test `Initialize` sets a license-safe work date. Adds minimal fixture and isolation adjustments. - Removes the APIV1/APIV2 exclusions, keeping only the entries fixed in the following area PRs and the two `139806` methods NAV master disables. Removes E-Doc `135575`. 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
This branch was successfully deployed
2 participants
Review focus: the one-line BaseApp change in
ReturnShptPDFDocHandlerplus the isolation property in three codeunits.Area fix (previously #11230 + isolation): return-shipment PDFs use the posted return-shipment report selection.
RequiredTestIsolation = Disabledin CU 139908/139922/139928 avoids 409 conflicts while the HTTP session renders the PDF. Re-enablesTestExpandPdfDocumentin those codeunits.Related: AB#646383, AB#653393
Stack (each PR shows only its own layer; merge bottom first)
main)Experiments (on D8, do not merge): #12685 E single tenant, #12686 F no clean execution. Combined validation (do not merge): #12687, same tree as D8.
Platform:
mainalready pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix (AB#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 targetedmain, so their results are valid for the cumulative change.