Test/use tenor data - #2361
Open
Nyeng wants to merge 37 commits into
Open
Conversation
…pakkedelegering - selectMainUnitBySearching: detect single-actor (auto-selected, lands on dashboard with reportee-name H1) vs multi-actor account menu; loop until the reportee H1 is visible instead of racing a generic dialog. - clickUser: user-list rows render as link OR collapsible expand-button; expand to reveal the self-link and navigate via its href (avoids the search-rerender detach that breaks a click). - dagligLederMedOrg requires a bosatt daglig leder (must be able to log in); drop the redundant NOT-doed from bosattMyndigKql (personstatus is current). - remove dead code (personMedFnr, clickUserListRow).
Replace hardcoded PIDs/orgnr with dynamic Tenor queries (bosatteMyndigePersoner, dagligLederMedOrg, hentTilfeldigVirksomhet) to avoid parallel-run collisions (Altinn/altinn-authentication#2086). Switch actor selection to the hardened login.selectMainUnitBySearching and drop redundant expandOrg (clickUser self-expands collapsible org rows).
Convert Hovedadministrator/Vanlig bruker/Standard bruker describes to Tenor (dagligLederMedOrg grants packages, bosattMyndigPerson receives them) and use login.selectMainUnitBySearching for actor selection. The 'Tilgangsstyrer ... de selv har' test stays on hardcoded data: it opens the user's OWN page expecting "Gi fullmakt", but a clean tilgangsstyrer sees "Be om fullmakt" (cannot self-delegate) — the old fixed user only passed via extra accumulated privileges. Needs a redesign (delegate to a separate recipient) before it can use Tenor data; documented inline. Leaves 'over- og underenheter' hardcoded (needs hovedenhet/underenhet orgs).
The test opened the acting user's OWN page and expected "Gi fullmakt", which only worked for a fixed user with accumulated privileges (a clean tilgangsstyrer sees "Be om fullmakt" — no self-delegation). Redesign to delegate to a SEPARATE recipient: daglig leder grants the tilgangsstyrer role+packages and connects a recipient; the tilgangsstyrer logs in, represents the org, opens the recipient and delegates. All data now from Tenor.
accessManagement.spec.ts and enkelttjenestedelegering.spec.ts were the only two spec files starting with an uppercase letter (17 others are lowercase-first).
…erenhet Add TenorTestData.hovedenhetMedUnderenhet (and TenorApiRequests.hentUnderenheter / hentVirksomhetMedForm): sources a hovedenhet that has an underenhet with a living daglig leder, resolving the relationship child->parent via the underenhet's `underenhet.hovedenhet` field (parents don't list children). Requires matching hoved/underenhet names so the nth(0)/nth(1) actor selectors are reliable. Convert all four 'over- og underenheter' tests to Tenor. Delegate from the HOVEDENHET (a hovedenhet delegation is visible under both units; an underenhet delegation is not visible under the hovedenhet). Rename the tests to match what they actually do and drop a broken step that checked the delegator in the recipient manager's actor list (the delegator is never in the recipient's account menu; the real check is under 'fullmakter hos andre'). Also make expectUserToHavePackage use an exact name match so e.g. 'Byggesøknad' doesn't also match 'Slett fullmakt for Byggesøknad'.
Org-A and Org-B are now dagligLederMedOrg (Org-B provides both the org to delegate to and its daglig leder = the key-role user that inherits the packages). Replace the title-keyed DelegationApiUtil cleanup (tied to hardcoded orgs) with EnduserConnection.deleteConnection on the dynamic orgs. verifyKeyRoleUserHasDelegatedPackages now filters the user list before clicking the org (a random avgiver-org has a full, paginated rightholder board, so the recipient is otherwise beyond page 1) and retries the expand until the key-role link is visible (survives the search re-render collapse).
Self-documents what each TenorTestData helper requires (in plain language + KQL), and lists the original hardcoded PIDs/orgs as known-good fallback values so a scenario can be run manually if Tenor is down or its model changes.
All 6 describes now use dagligLederMedOrg for the administering org (actor) and the client org, and bosattMyndigPerson for the added agent. The client->actor relationship is created via API in beforeEach, so the actor org need not be a registered facilitator (revisor/regnskapsfører). actor and client are guaranteed distinct. Actor selection via login.selectMainUnitBySearching.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR shifts Playwright e2e coverage to Tenor-generated test data, adds shared delegation and system-user cleanup helpers, and hardens page objects for retry-based navigation and filtering. It also updates docs and request utilities to support the new dynamic test setup. ChangesTenor test data foundation and E2E migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant PlaywrightSpec
participant TestDataProvider
participant TenorTestData
participant CleanupHelpers
PlaywrightSpec->>TestDataProvider: createTestData()
TestDataProvider->>TenorTestData: select Tenor-backed fixtures
TenorTestData-->>PlaywrightSpec: orgs, persons, facilitators
PlaywrightSpec->>CleanupHelpers: cleanupConnection / cleanupSystemUser
CleanupHelpers-->>PlaywrightSpec: best-effort teardown
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Nyeng
commented
Jul 2, 2026
Nyeng
commented
Jul 2, 2026
Nyeng
commented
Jul 2, 2026
Nyeng
commented
Jul 2, 2026
Nyeng
commented
Jul 2, 2026
selectSystemVendor, approveSystemUserRequest, approveSystemUserChangeRequest and deleteSystemUser now get the customer/owner org (and the person who logs in/approves) from tenor.dagligLederMedOrg. The system vendor + prebuilt system stay hardcoded — they are registered systemregister infrastructure, not Tenor data. The customer org is passed to postSystemuserRequest so a fresh Tenor org is used per run (no shared-actor collisions).
The escalation test needs a manager (approves) and a regular contact person (can only escalate). In Tenor an AS has either a daglig leder OR a kontaktperson (KONT), never both, so we take an org with a daglig leder (dagligLederMedOrg) and connect a separate bosattMyndigPerson as a plain rightholder (contact person) via EnduserConnection — a rightholder without access-manager rights can escalate but not approve. Vendor + system stay hardcoded (registered infrastructure). Also refactor hentDagligLeder to use a generic hentRolleFnr(role) helper.
…Tenor Add facilitator helpers: TenorTestData.facilitatorMedKlienter(rolle) and forretningsfoererMedEiendomsklient() (backed by TenorApiRequests hentFacilitatorMedBosattLeder / hentForretningsfoererMedEiendomsklient). Both guarantee a living daglig leder and use real brreg clients; forretningsfører picks a BRL/ESEK eiendom client (the only type forretningsforer-eiendom can be delegated for). Regnskapsfører and Forretningsfører describes now use Tenor (they add ONE client, so any facilitator with >=1 client works — abundant). The Ansvarlig revisor describe stays hardcoded on purpose: it delegates ALL clients in one click, and Tenor revisors have too many clients (none <=10 among candidates) to keep that fast. Vendor + system stay hardcoded (registered infrastructure).
Owner + clients now come from Tenor: a fresh owner has no clients, so we set up exactly ANTALL_KLIENTER (2) via API (each client delegates the access package to the owner) — keeping "add all clients" fast. Verified the API-delegated clients surface in addAllCustomers. afterEach now deletes the agent system user (on failure; the happy path deletes it in the UI step), the system from the register, and the client connections — so nothing leaks after the run. Vendor + system stay hardcoded.
Nyeng
marked this pull request as ready for review
July 2, 2026 20:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts (1)
59-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRedundant/noisy cleanup after a test that already deletes its own resource.
The test itself calls
klientAdministrasjonPage.slettBruker()to delete the agent via the UI, so on a passing run the subsequentapi.deleteClientDelegationAgent(...)inafterEachwill hit an already-deleted resource and log a "Cleanup: Failed to delete client delegation agent" error every time the suite passes.As per coding guidelines, "Skip cleanup in afterEach when test passed (status === 'passed') — API cleanup is only a fallback for failed runs to avoid log spam".🧹 Proposed fix using testInfo.status
- test.afterEach(async () => { + test.afterEach(async ({}, testInfo) => { + if (testInfo.status === 'passed') return; try { await api.deleteClientDelegationAgent(actor.dagligLeder.pid, actor.org.orgnr, agent.pid); } catch (error) { console.error('Cleanup: Failed to delete client delegation agent:', error); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts` around lines 59 - 99, The `test.afterEach` cleanup in `slett bruker` is redundant because `slettBruker()` already removes the agent during the test, so `api.deleteClientDelegationAgent(...)` should not run on successful executions. Update the `afterEach` in this spec to skip the API cleanup when `testInfo.status === 'passed'`, and keep it only as a fallback for failed runs; use the existing `actor`, `agent`, and `api.deleteClientDelegationAgent` symbols to locate the cleanup block.Source: Coding guidelines
🧹 Nitpick comments (13)
playwright/pages/AccessManagementFrontPage.ts (1)
68-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDedupe the expand-to-self-link logic.
clickUser(85-90) andensureRowExpanded(108-113) carry the same "if no link → click collapsible → wait for self-link" block. Two copies of the same fragile retry will drift apart over time;clickUsercan just reuseensureRowExpanded.♻️ Suggested consolidation
async clickUser(userName: string, num = 0) { await this.filterUserList(userName); ... const main = this.page.getByRole('main'); const link = main.getByRole('link', { name: userName }); - const collapsibleRow = main.getByRole('button', { name: userName }).first(); - await expect(async () => { - if ((await link.count()) === 0) { - await expect(collapsibleRow).toBeVisible({ timeout: 5000 }); - await collapsibleRow.click(); // utvid for å avdekke selv-lenken - await expect(link.first()).toBeVisible({ timeout: 3000 }); - } + await this.ensureRowExpanded(userName); const href = await link.nth(num).getAttribute('href'); if (!href) throw new Error(`Fant ingen href på lenka for "${userName}" (raden re-rendrer).`); await this.page.goto(new URL(href, this.page.url()).toString()); }).toPass({ timeout: 25000 }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/pages/AccessManagementFrontPage.ts` around lines 68 - 115, The expand-to-self-link retry logic is duplicated between clickUser and ensureRowExpanded, so consolidate it in ensureRowExpanded and have clickUser call that helper before reading the href. Keep the unique link/button lookup and retry behavior in one place using the existing main, link, and collapsibleRow queries, then let clickUser only handle href extraction and navigation.playwright/pages/profile/accessPackageDelegationPage.ts (1)
237-244: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
keyUserLink.click()sits outside the retry — same detach raceclickUserwas reworked to avoid.The
toPassonly asserts visibility; the actual click at Line 244 can still hit "element detached" if the debounced list re-renders after the block resolves. Consider mirroringAccessManagementFrontPage.clickUser(navigate via the linkhref), or pull the click into thetoPass. Please confirm this flow isn't flaky under the paginated Tenor lists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/pages/profile/accessPackageDelegationPage.ts` around lines 237 - 244, The click on keyUserLink is still outside the retry block, so it can fail with a detach race after the list re-renders. Update the key user navigation flow in accessPackageDelegationPage to match the safer pattern used by AccessManagementFrontPage.clickUser: either move the keyUserLink.click() inside the toPass retry or navigate using the link’s href instead of clicking a potentially stale element. Keep the existing visibility checks around orgButton and keyUserLink, but make the final navigation resilient to pagination-driven rerenders.playwright/tenor/TenorApiRequests.ts (2)
331-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
stokkduplicatesTenorTestData.shuffle.This Fisher–Yates implementation is copy-pasted verbatim as
shuffleinplaywright/tenor/TenorTestData.ts(lines 228-236). SinceTenorTestDataalready imports from this file, consider exporting one shuffle utility (or moving it toplaywright/util/helper.ts, which already hostspickRandom) and reusing it in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tenor/TenorApiRequests.ts` around lines 331 - 339, The Fisher–Yates logic in stokk is duplicated by TenorTestData.shuffle, so consolidate it into a single reusable utility instead of keeping two copy-pasted implementations. Move or export the shuffle helper from TenorApiRequests (or reuse an existing shared helper like pickRandom in helper.ts), then update both stokk and TenorTestData.shuffle to call the shared function so there is one source of truth.
295-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
FACILITATOR_ROLLERfor theforretningsfoererlookup
forretningsfoerereOrgnrandFFØRalready live inFACILITATOR_ROLLER['forretningsfoerer']; reading them from there keeps this helper consistent with the others and avoids drift if the mapping changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tenor/TenorApiRequests.ts` around lines 295 - 320, The forretningsfoerer lookup in hentForretningsfoererMedEiendomsklient is hardcoded instead of using the existing FACILITATOR_ROLLER mapping. Update this helper to read both the search field and role code from FACILITATOR_ROLLER['forretningsfoerer'] when calling sokBrreg and hentRolleVirksomhet, keeping it consistent with the other facilitator helpers and avoiding mapping drift.playwright/tenor/TenorTestData.ts (2)
131-157: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify
ORG_POOL = 100is enough for the triple-filteredhovedenhetMedUnderenhetsearch.This method requires all three of: hovedenhet organisasjonsform match, hovedenhet/underenhet same navn, and a bosatt daglig leder — chained over a single, non-refillable pool of 100 underenheter. If the intersection is rare in the Tenor dataset, this could throw intermittently in CI (the exact flakiness class this PR is trying to eliminate), rather than being a deterministic fallback.
Consider widening the pool (or falling back to a second
hentUnderenheterpage) when the first pass exhausts all candidates without a match, instead of failing immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tenor/TenorTestData.ts` around lines 131 - 157, The `hovedenhetMedUnderenhet` lookup can fail intermittently because it exhausts a single `hentUnderenheter(ORG_POOL)` page while applying three strict filters (`organisasjonsform`, same `navn`, and bosatt daglig leder). Update this method to avoid relying only on the initial 100-item pool in `TenorTestData`: either widen the candidate set or add a fallback that fetches additional underenheter pages and continues searching before throwing. Keep the existing matching flow in `hovedenhetMedUnderenhet`, but make the search resilient when the first page contains no valid match.
92-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline daglig-leder lookups duplicate the existing
dagligLederFraFnrhelper.
dagligLederMedOrg(Lines 104-108) andhovedenhetMedUnderenhet(Lines 143-147) both re-implement the exactidentifikator:${pid} AND bosattMyndigKql()→hentPersonerquery thatdagligLederFraFnr(Lines 194-203) already encapsulates. The only difference is failure handling (continueto try the next candidate vs.throw). Extracting a shared nullable lookup (e.g.private async tilBosattMyndigPerson(fnr): Promise<TenorPerson | null>) that both the loops anddagligLederFraFnr(wrap-and-throw) build on would remove three near-identical query blocks.♻️ Shared nullable lookup
+ private async tilBosattMyndigPerson(fnr: string): Promise<TenorPerson | null> { + const [person] = await this.tenor.hentPersoner( + `identifikator:${fnr} AND ${TenorApiRequests.bosattMyndigKql()}`, + 1, + ); + return person ? tilTenorPerson(person) : null; + } + private async dagligLederFraFnr(fnr: string | null): Promise<TenorPerson> { if (!fnr) throw new Error('Facilitator mangler daglig leder.'); - const [person] = await this.tenor.hentPersoner( - `identifikator:${fnr} AND ${TenorApiRequests.bosattMyndigKql()}`, - 1, - ); - if (!person) throw new Error(`Daglig leder ${fnr} er ikke en bosatt, myndig person.`); - return tilTenorPerson(person); + const person = await this.tilBosattMyndigPerson(fnr); + if (!person) throw new Error(`Daglig leder ${fnr} er ikke en bosatt, myndig person.`); + return person; }Also applies to: 131-157, 194-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tenor/TenorTestData.ts` around lines 92 - 117, The daily-leader lookup logic is duplicated across dagligLederMedOrg, hovedenhetMedUnderenhet, and dagligLederFraFnr. Extract the shared hentPersoner query into a reusable nullable helper (for example, a private method that returns a TenorPerson or null for a given fnr/pid and the bosattMyndigKql filter), then have dagligLederMedOrg and hovedenhetMedUnderenhet continue on null while dagligLederFraFnr wraps that helper and throws when no person is found. Keep the existing return shapes and failure behavior, but centralize the query construction and tilTenorPerson conversion in one place.playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts (2)
139-141: 📐 Maintainability & Code Quality | 🔵 TrivialafterEach placed after the test body.
As per coding guidelines, spec files under
playwright/**/{e2eTests,uuTests}/**/*.{ts,tsx}should follow the ordertest.describe → test.beforeEach → test.afterEach → test(). This newly-addedafterEachsits after the test() block instead of before it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts` around lines 139 - 141, The newly added cleanup hook is in the wrong place in the spec order; move the test.afterEach callback in addOwnOrg.spec.ts so it appears before the test() block, following the required test.describe → test.beforeEach → test.afterEach → test() structure and keeping the existing cleanup logic in place.Source: Coding guidelines
33-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded retry loop for unique client selection.
The
whileloop keeps callingtenor.dagligLederMedOrg()with no iteration cap. If the pool is exhausted or repeatedly yields duplicates, this loops indefinitely (mitigated only by the overall test timeout). Consider adding a max-attempts guard that throws a clear error.🔁 Suggested guard
+ const MAX_ATTEMPTS = 20; + let attempts = 0; while (clients.length < ANTALL_KLIENTER) { + if (++attempts > MAX_ATTEMPTS) { + throw new Error(`Klarte ikke finne ${ANTALL_KLIENTER} unike klienter etter ${MAX_ATTEMPTS} forsøk.`); + } const kandidat = await tenor.dagligLederMedOrg(); const brukt = [owner, ...clients].some((o) => o.org.orgnr === kandidat.org.orgnr); if (!brukt) clients.push(kandidat); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts` around lines 33 - 37, The unique-client selection loop in addOwnOrg.spec.ts can run forever because the `while (clients.length < ANTALL_KLIENTER)` block keeps retrying `tenor.dagligLederMedOrg()` without a cap. Update this logic to include a max-attempts guard inside the loop, and fail fast with a clear error if enough unique orgs cannot be collected. Use the existing `owner`, `clients`, and `ANTALL_KLIENTER` symbols to keep the change localized and easy to find.playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts (3)
101-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated setup/exclusion logic across four describe blocks.
The
do...while (client.org.orgnr === actor.org.orgnr)guard plus theaddConnectionAndPackagesToUser+addClientDelegationAgentsetup is repeated near-verbatim in this block and in169-235,237-301, and303-365. Extracting a sharedsetupClientAndAgent()/differentOrg()helper (local to this file, or upstream inTenorTestData) would reduce duplication and centralize the exclusion logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts` around lines 101 - 167, The setup in this describe block repeats the same client/actor exclusion and delegation initialization used in the other three describe blocks. Extract the shared `do...while (client.org.orgnr === actor.org.orgnr)` selection logic plus the `addConnectionAndPackagesToUser` and `addClientDelegationAgent` calls into a reusable helper such as `setupClientAndAgent()` or `differentOrg()` in this spec file (or `TenorTestData`), and have each describe block call it instead of duplicating the setup.
15-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHook declaration order deviates from repo convention.
This describe (and every nested describe in this file: 59-99, 101-167, 169-235, 237-301, 303-365) declares
beforeEach→test()→afterEach, but the sibling spec (accessPackageDirectdelegatation.spec.ts) and the stated convention order hooks asdescribe → beforeEach → afterEach → test(). Doesn't affect runtime behavior, but is inconsistent with the established file layout.As per coding guidelines, "Playwright spec files must follow order: test.describe → test.beforeEach → test.afterEach → test()".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts` around lines 15 - 57, The hook order in this spec file is inconsistent with the repo convention: the `test.describe` blocks in `klientadministrasjon.spec.ts` currently place `test()` before `test.afterEach`, while the established order is `test.describe` → `test.beforeEach` → `test.afterEach` → `test()`. Update this describe and the other nested describes in the file to move each `test.afterEach` block above the `test()` definitions, keeping the same behavior while matching the convention.Source: Coding guidelines
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
expect.soft()for the multiple assertions in this test step.Both assertions here (and similar multi-assert steps throughout this file, e.g. lines 213-215/217-220, 45-46 duplicated pattern) use hard
expect, so the test bails on the first failure instead of surfacing all mismatches.As per coding guidelines, "Use expect.soft() when verifying multiple assertions in the same test to see all failures at once".✅ Proposed fix
- await expect(klientAdministrasjonPage.slettBrukerKnapp).toBeVisible(); - await expect(klientAdministrasjonPage.brukerKnapp(agent.navn)).toBeVisible(); + await expect.soft(klientAdministrasjonPage.slettBrukerKnapp).toBeVisible(); + await expect.soft(klientAdministrasjonPage.brukerKnapp(agent.navn)).toBeVisible();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts` around lines 44 - 47, The test step in klientadministrasjon.spec.ts uses hard assertions for multiple checks, so the first failure stops the rest from running. Update the multi-assert blocks in this test file to use expect.soft() inside the relevant test.step callbacks, including the one that verifies slettBrukerKnapp and brukerKnapp(agent.navn), and apply the same pattern to the other similar assertion groups in the file so all mismatches are reported together.Source: Coding guidelines
playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts (2)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a reusable "distinct org" helper.
The
do...whileloop to avoidorgA/orgBcollisions is duplicated inklientadministrasjon.spec.ts(4x) with the same pattern. SinceTenorTestDataalready supports anekskluderparam onhentTilfeldigVirksomhet, adding a similar exclusion option todagligLederMedOrg()would remove this repeated boilerplate across specs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts` around lines 12 - 19, The `do...while` self-collision guard in `beforeEach` is duplicated across multiple specs, so refactor it into a reusable distinct-org helper. Update `TenorTestData.dagligLederMedOrg()` to accept an exclusion parameter like the existing `hentTilfeldigVirksomhet(..., ekskluder)` flow, then use that helper here to fetch `orgB` without manual looping. Keep the `orgA`/`orgB` setup in `accessPackageDirectdelegatation.spec.ts` and any other affected specs aligned with the new helper signature.
83-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSkipped test still references hardcoded/shared identifiers.
This block is
test.skip'd so it's not executed, but it still hardcodes a PID and org name the rest of the PR is moving away from. Since it's already flagged as not testing anything, consider removing it rather than keeping stale hardcoded fixtures around.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts` around lines 83 - 127, Remove the skipped `test.skip('Org-C revokes all delegated rights from Org-D'...)` block from `accessPackageDirectdelegatation.spec.ts` instead of keeping stale hardcoded fixtures in a non-executed test. The issue is that this dead test still hardcodes the PID and org names while the rest of the PR is moving to shared identifiers, so delete the entire skipped test and its related `test.step` blocks rather than updating them. Use the `test.skip` wrapper and the `DelegationApiUtil` calls as the anchors for locating the code to remove.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts`:
- Around line 139-174: The fallback cleanup in test.afterEach is deleting an
agent-created system user through the non-agent path, so switch the cleanup flow
to use the agent-specific API for this resource type. Locate the afterEach block
in addOwnOrg.spec.ts and replace the getSystemUserByQuery/cleanUpSystemUsers
combination for this case with the dedicated deleteAgentSystemUser request
exposed by SystemUserApiRequests.tsx, using the same identifiers already
available in the test. Keep the broader cleanup for system register and client
connections unchanged, and avoid swallowing a cleanup mismatch by routing agent
users through the correct endpoint.
In `@playwright/tenor/README.md`:
- Line 60: The markdown tables in the README are missing the required blank
lines before and after them, which triggers MD058. Update each flagged table
entry around the table blocks (including the ones referenced near the existing
table rows and the other tables in the same section) by adding empty lines
immediately surrounding the table markup so the formatting is consistent
throughout the document.
- Around line 69-77: The table under dagligLederMedOrg contains a malformed row
that does not match the dagligLeder pid, orgnr, orgnavn columns. Update the
README table entry in dagligLederMedOrg so the orgnr cell contains a real
organization number and the orgnavn cell contains the organization name, or
remove the row if no valid orgnr is available. Use the dagligLederMedOrg section
and the surrounding accessPackageDirect example as the reference points when
fixing the literal fallback data.
---
Outside diff comments:
In
`@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts`:
- Around line 59-99: The `test.afterEach` cleanup in `slett bruker` is redundant
because `slettBruker()` already removes the agent during the test, so
`api.deleteClientDelegationAgent(...)` should not run on successful executions.
Update the `afterEach` in this spec to skip the API cleanup when
`testInfo.status === 'passed'`, and keep it only as a fallback for failed runs;
use the existing `actor`, `agent`, and `api.deleteClientDelegationAgent` symbols
to locate the cleanup block.
---
Nitpick comments:
In
`@playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts`:
- Around line 12-19: The `do...while` self-collision guard in `beforeEach` is
duplicated across multiple specs, so refactor it into a reusable distinct-org
helper. Update `TenorTestData.dagligLederMedOrg()` to accept an exclusion
parameter like the existing `hentTilfeldigVirksomhet(..., ekskluder)` flow, then
use that helper here to fetch `orgB` without manual looping. Keep the
`orgA`/`orgB` setup in `accessPackageDirectdelegatation.spec.ts` and any other
affected specs aligned with the new helper signature.
- Around line 83-127: Remove the skipped `test.skip('Org-C revokes all delegated
rights from Org-D'...)` block from `accessPackageDirectdelegatation.spec.ts`
instead of keeping stale hardcoded fixtures in a non-executed test. The issue is
that this dead test still hardcodes the PID and org names while the rest of the
PR is moving to shared identifiers, so delete the entire skipped test and its
related `test.step` blocks rather than updating them. Use the `test.skip`
wrapper and the `DelegationApiUtil` calls as the anchors for locating the code
to remove.
In
`@playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts`:
- Around line 101-167: The setup in this describe block repeats the same
client/actor exclusion and delegation initialization used in the other three
describe blocks. Extract the shared `do...while (client.org.orgnr ===
actor.org.orgnr)` selection logic plus the `addConnectionAndPackagesToUser` and
`addClientDelegationAgent` calls into a reusable helper such as
`setupClientAndAgent()` or `differentOrg()` in this spec file (or
`TenorTestData`), and have each describe block call it instead of duplicating
the setup.
- Around line 15-57: The hook order in this spec file is inconsistent with the
repo convention: the `test.describe` blocks in `klientadministrasjon.spec.ts`
currently place `test()` before `test.afterEach`, while the established order is
`test.describe` → `test.beforeEach` → `test.afterEach` → `test()`. Update this
describe and the other nested describes in the file to move each
`test.afterEach` block above the `test()` definitions, keeping the same behavior
while matching the convention.
- Around line 44-47: The test step in klientadministrasjon.spec.ts uses hard
assertions for multiple checks, so the first failure stops the rest from
running. Update the multi-assert blocks in this test file to use expect.soft()
inside the relevant test.step callbacks, including the one that verifies
slettBrukerKnapp and brukerKnapp(agent.navn), and apply the same pattern to the
other similar assertion groups in the file so all mismatches are reported
together.
In `@playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts`:
- Around line 139-141: The newly added cleanup hook is in the wrong place in the
spec order; move the test.afterEach callback in addOwnOrg.spec.ts so it appears
before the test() block, following the required test.describe → test.beforeEach
→ test.afterEach → test() structure and keeping the existing cleanup logic in
place.
- Around line 33-37: The unique-client selection loop in addOwnOrg.spec.ts can
run forever because the `while (clients.length < ANTALL_KLIENTER)` block keeps
retrying `tenor.dagligLederMedOrg()` without a cap. Update this logic to include
a max-attempts guard inside the loop, and fail fast with a clear error if enough
unique orgs cannot be collected. Use the existing `owner`, `clients`, and
`ANTALL_KLIENTER` symbols to keep the change localized and easy to find.
In `@playwright/pages/AccessManagementFrontPage.ts`:
- Around line 68-115: The expand-to-self-link retry logic is duplicated between
clickUser and ensureRowExpanded, so consolidate it in ensureRowExpanded and have
clickUser call that helper before reading the href. Keep the unique link/button
lookup and retry behavior in one place using the existing main, link, and
collapsibleRow queries, then let clickUser only handle href extraction and
navigation.
In `@playwright/pages/profile/accessPackageDelegationPage.ts`:
- Around line 237-244: The click on keyUserLink is still outside the retry
block, so it can fail with a detach race after the list re-renders. Update the
key user navigation flow in accessPackageDelegationPage to match the safer
pattern used by AccessManagementFrontPage.clickUser: either move the
keyUserLink.click() inside the toPass retry or navigate using the link’s href
instead of clicking a potentially stale element. Keep the existing visibility
checks around orgButton and keyUserLink, but make the final navigation resilient
to pagination-driven rerenders.
In `@playwright/tenor/TenorApiRequests.ts`:
- Around line 331-339: The Fisher–Yates logic in stokk is duplicated by
TenorTestData.shuffle, so consolidate it into a single reusable utility instead
of keeping two copy-pasted implementations. Move or export the shuffle helper
from TenorApiRequests (or reuse an existing shared helper like pickRandom in
helper.ts), then update both stokk and TenorTestData.shuffle to call the shared
function so there is one source of truth.
- Around line 295-320: The forretningsfoerer lookup in
hentForretningsfoererMedEiendomsklient is hardcoded instead of using the
existing FACILITATOR_ROLLER mapping. Update this helper to read both the search
field and role code from FACILITATOR_ROLLER['forretningsfoerer'] when calling
sokBrreg and hentRolleVirksomhet, keeping it consistent with the other
facilitator helpers and avoiding mapping drift.
In `@playwright/tenor/TenorTestData.ts`:
- Around line 131-157: The `hovedenhetMedUnderenhet` lookup can fail
intermittently because it exhausts a single `hentUnderenheter(ORG_POOL)` page
while applying three strict filters (`organisasjonsform`, same `navn`, and
bosatt daglig leder). Update this method to avoid relying only on the initial
100-item pool in `TenorTestData`: either widen the candidate set or add a
fallback that fetches additional underenheter pages and continues searching
before throwing. Keep the existing matching flow in `hovedenhetMedUnderenhet`,
but make the search resilient when the first page contains no valid match.
- Around line 92-117: The daily-leader lookup logic is duplicated across
dagligLederMedOrg, hovedenhetMedUnderenhet, and dagligLederFraFnr. Extract the
shared hentPersoner query into a reusable nullable helper (for example, a
private method that returns a TenorPerson or null for a given fnr/pid and the
bosattMyndigKql filter), then have dagligLederMedOrg and hovedenhetMedUnderenhet
continue on null while dagligLederFraFnr wraps that helper and throws when no
person is found. Keep the existing return shapes and failure behavior, but
centralize the query construction and tilTenorPerson conversion in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25c68543-f4f5-4ff9-910e-4aaa3b430515
📒 Files selected for processing (18)
playwright/e2eTests/altinn3/Tilgangsstyring/accessManagement.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/enkelttjenestedelegering.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/tilgangspakkedelegering.spec.tsplaywright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.tsplaywright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.tsplaywright/e2eTests/altinn3/systemuser/addOwnOrg.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserChangeRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/clientDelegation.spec.tsplaywright/e2eTests/altinn3/systemuser/deleteSystemUser.spec.tsplaywright/e2eTests/altinn3/systemuser/escalateSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.tsplaywright/pages/AccessManagementFrontPage.tsplaywright/pages/LoginPage.tsplaywright/pages/profile/accessPackageDelegationPage.tsplaywright/tenor/README.mdplaywright/tenor/TenorApiRequests.tsplaywright/tenor/TenorTestData.ts
klikkKnapp now loads more pages via the 'Se mer' button until the target row is visible before clicking. With Tenor actors the klient/bruker lists are paginated, so the target (e.g. the client in 'deleger klient fra brukerfanen') can sit past the first page. Hardens all klikkKnapp usages.
Add playwright/util/systemUserCleanup.ts and call it from every systemuser spec's afterEach (except deleteSystemUser, which tests deletion). It deletes the system user via API and removes the created system, best-effort (swallows/quiets errors so cleanup never fails a test). - Regular system users (selectSystemVendor, approveSystemUserRequest, approveSystemUserChangeRequest, escalate): deleted via API (getSystemUserByQuery/known id -> cleanUpSystemUsers). - Agent system users (clientDelegation, addOwnOrg): deleted in the UI step (the API agent-delete 500s when customers are assigned), and the util removes the created system + connections. selectSystemVendor captures the system-user id from the UI link so it too can be deleted via API.
…stemUser The assertion was floating (missing await), so it didn't actually wait for the package button before clicking confirm — surfacing intermittently as a 'Forretningsforer eiendom' not-visible failure under parallel load. Awaiting it makes the step wait/retry properly.
Any org can act as a system vendor, so instead of always using one hardcoded vendor, the specs that create their own system (selectSystemVendor, deleteSystemUser, clientDelegation, addOwnOrg, escalate) now pick from a 20-vendor list (playwright/util/systemVendors.ts, pickVendorOrg). Spreads load so tests don't all share one vendor. The prebuilt-system specs (approve request / change request) keep the vendor their prebuilt system is registered on.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Address CodeRabbit: surround the fallback-example tables with blank lines, and fix the SUBJEKTIV ELASTISK TIGER AS row so its cells match the pid|orgnr|orgnavn columns.
Only the approve test creates a system user; the reject (Avvis) test doesn't, so running cleanup there logged a noisy 404 from get-system-user-by-query. Gate the afterEach cleanup on a flag set only after a successful approval.
Only the approve test creates a system user, so cleaning up inline there (no flag, no afterEach guard) is simpler and keeps the reject test from logging a 404.
Add playwright/util/delegationCleanup.ts (cleanupConnection, cleanupServiceDelegation, cleanupPackageDelegation, cleanupClientDelegationAgent) — best-effort, error-swallowing helpers so afterEach blocks are 1-2 lines instead of repeated try/catch. Applied to enkelttjeneste and tilgangspakke.
…ssPackageDirect, klientadministrasjon Replace repeated try/catch afterEach blocks with cleanupConnection / cleanupServiceDelegation / cleanupPackageDelegation / cleanupClientDelegationAgent.
…gation
- dagligLederMedOrg now takes { organisasjonsform?, ekskluder? }, replacing the
repeated do-while "pick a distinct org" loops (accessPackageDirect + the 4
klientadministrasjon describes) with a one-liner.
- Rename delegationCleanup.ts -> delegationHelpers.ts and add setupServiceDelegation
(addConnection + delegateSingleService) so the "slett"-tests' beforeEach is one
call. A single addConnection stays direct (already concise).
Trekk ut det gjentatte addConnectionAndPackagesToUser-oppsettet til en felles setupPackagesForUser(api, ref, packages) i delegationHelpers, og bruk den i accessManagement- og tilgangspakkedelegering-testene. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mness) pickRandom og Fisher–Yates-stokkene brukte Math.random. Verdiene flyter videre inn i API-kall (bl.a. valg av leverandør-org i systembruker- oppryddingen), så CodeQL flagget dem som js/insecure-randomness. Bytt til Node sin crypto.randomInt — samme variasjon i testdata, men scanneren blir ren. Ingen funksjonell endring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
playwright/pages/tilgangsstyring/KlientAdministrasjonPage.ts (1)
143-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the fixed
seMerlocator to the constructor.
seMerhas no parameters, so per this file's own convention (and the path guideline forplaywright/pages/**), it belongs as a readonly constructor property rather than being re-created on everysurfaceViaSeMercall.♻️ Proposed fix
readonly alleBrukereFane: Locator; + readonly seMerKnapp: Locator; constructor(page: Page, language: Language = Language.NB) { ... this.alleBrukereFane = this.page.getByRole('tab', { name: client.client_can_get_agents_tab }); + this.seMerKnapp = this.page.getByRole('button', { name: this.texts.common.show_more }); ... } ... private async surfaceViaSeMer(target: Locator) { - const seMer = this.page.getByRole('button', { name: this.texts.common.show_more }); await expect(async () => { if (await target.isVisible().catch(() => false)) return; - if (await seMer.isVisible().catch(() => false)) { - await seMer.click(); + if (await this.seMerKnapp.isVisible().catch(() => false)) { + await this.seMerKnapp.click(); } await expect(target).toBeVisible({ timeout: 2000 }); }).toPass({ timeout: 30000 }); }As per path instructions,
playwright/pages/**/*.{ts,tsx}: "constructor defines all fixed locators as readonly properties, dynamic locators are methods that return Locator."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/pages/tilgangsstyring/KlientAdministrasjonPage.ts` around lines 143 - 162, The fixed “Se mer” locator is being created inside surfaceViaSeMer even though it has no parameters and should follow this page-object convention. Move the getByRole locator for this.texts.common.show_more into a readonly constructor property on KlientAdministrasjonPage, then update surfaceViaSeMer to reuse that property instead of recreating the locator on each call.Source: Path instructions
playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts (1)
19-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHook ordering:
afterEachdeclared aftertest().Based on the closing braces,
test.afterEach(line 58) is registered after thetest()block rather than before it. As per coding guidelines, "Playwright spec files must follow order: test.describe → test.beforeEach → test.afterEach → test()". Consider movingafterEachabove thetest()call for consistency with the rest of the suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts` around lines 19 - 58, The hook order in the System Register spec is out of sequence because test.afterEach is declared after the test() block instead of alongside the other lifecycle hooks. Move the afterEach registration in test.describe('System Register') so it appears before the test('Create system user and verify landing page') definition, keeping the ordering consistent with test.beforeEach and the rest of the suite.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts`:
- Around line 58-64: The cleanup in the `test.afterEach` block is missing the
created system identifier, so `cleanupSystemUser` only removes the system user
and leaves the setup system behind. Update the `cleanupSystemUser` call in
`selectSystemVendor.spec.ts` to also pass the `systemName` value from `system`
so the system register entry is deleted after each test. Use the existing
`cleanupSystemUser` helper and the `system` setup variable to keep the teardown
aligned with the test setup.
---
Nitpick comments:
In `@playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts`:
- Around line 19-58: The hook order in the System Register spec is out of
sequence because test.afterEach is declared after the test() block instead of
alongside the other lifecycle hooks. Move the afterEach registration in
test.describe('System Register') so it appears before the test('Create system
user and verify landing page') definition, keeping the ordering consistent with
test.beforeEach and the rest of the suite.
In `@playwright/pages/tilgangsstyring/KlientAdministrasjonPage.ts`:
- Around line 143-162: The fixed “Se mer” locator is being created inside
surfaceViaSeMer even though it has no parameters and should follow this
page-object convention. Move the getByRole locator for
this.texts.common.show_more into a readonly constructor property on
KlientAdministrasjonPage, then update surfaceViaSeMer to reuse that property
instead of recreating the locator on each call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d172cc64-8a05-431d-b2f7-02101a92bce0
📒 Files selected for processing (21)
playwright/e2eTests/altinn3/Tilgangsstyring/accessManagement.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/enkelttjenestedelegering.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/tilgangspakkedelegering.spec.tsplaywright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.tsplaywright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.tsplaywright/e2eTests/altinn3/systemuser/addOwnOrg.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserChangeRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/clientDelegation.spec.tsplaywright/e2eTests/altinn3/systemuser/deleteSystemUser.spec.tsplaywright/e2eTests/altinn3/systemuser/escalateSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.tsplaywright/pages/systemuser/ClientDelegation.tsplaywright/pages/tilgangsstyring/KlientAdministrasjonPage.tsplaywright/tenor/README.mdplaywright/tenor/TenorApiRequests.tsplaywright/tenor/TenorTestData.tsplaywright/util/delegationHelpers.tsplaywright/util/helper.tsplaywright/util/systemUserCleanup.tsplaywright/util/systemVendors.ts
✅ Files skipped from review due to trivial changes (1)
- playwright/tenor/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- playwright/e2eTests/altinn3/systemuser/deleteSystemUser.spec.ts
- playwright/e2eTests/altinn3/systemuser/approveSystemUserRequest.spec.ts
- playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts
- playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts
- playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts
- playwright/e2eTests/altinn3/systemuser/approveSystemUserChangeRequest.spec.ts
- playwright/e2eTests/altinn3/Tilgangsstyring/accessManagement.spec.ts
- playwright/e2eTests/altinn3/systemuser/escalateSystemUserRequest.spec.ts
- playwright/e2eTests/altinn3/systemuser/clientDelegation.spec.ts
- playwright/tenor/TenorApiRequests.ts
- playwright/e2eTests/altinn3/Tilgangsstyring/tilgangspakkedelegering.spec.ts
Søkefeltet gjenbrukes mellom delegeringene, og et enkelt fill kunne bli forkastet når lista re-rendret rett etter forrige tømming/delegering — da dukket aldri pakkeknappen opp («element not found»). Fyll på nytt via expect.toPass til knappen er synlig, og skop knappen til delegeringsmodalen i stedet for hele siden. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…knapp
grantAccessPkgName lette etter en knapp med navn = pakkenavnet for å åpne en
detaljmodal, men i delegeringsmodalen er pakkeraden ikke en knapp (den bærer
den innebygde handlingsknappen), så selektoren traff aldri. Delegér i stedet
via den beviste «Gi fullmakt for {pakke}»-knappen (pakkenavnet ligger i
aria-label), med samme re-fill-retry mot søke-race. Slå sammen med den
identiske grantAccessPkgNameDirect og oppdater kallene.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
verifyDelegatedPackage antok at pakken alltid er en knapp med pakkenavnet,
men direkte delegerte pakker rendres som en div med egen «Slett fullmakt
for {pakke}»-knapp (kun arvede pakker er en ren navn-knapp). Godta begge, så
den funker både i avgivers egen liste og hos nøkkelrolle-brukeren.
Bytt også ut resten av Math.random som flyter inn i system-API-kall
(externalRef, clientId, systemnavn-suffiks) med crypto.randomUUID/randomInt,
så CodeQL-alertene (js/insecure-randomness) forsvinner ved neste skann.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Området må være utvidet for at pakken skal vises, men et ubetinget klikk kunne kollapse et allerede åpent område (eller komme før pakkeinnholdet var ferdig lastet), så pakken av og til aldri ble synlig. Klikk bare når området ikke allerede er åpent (via aria-expanded), og prøv på nytt til pakken vises. Verifisert stabil over 35 kjøringer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
afterEach ryddet systemet med en egen TestdataApi.removeSystem i stedet for å la den felles cleanupSystemUser gjøre det. Send systemName til helperen så opprydding skjer ett sted, og fjern den overflødige removeSystem-jobben. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pakkeknappen har på nyeste frontend navnet «{pakke} {n} tjenester», så et
exact-treff på pakkenavnet bommet på arvede pakker. Bytt til ett ikke-eksakt
navnetreff som dekker både arvet («{pakke} {n} tjenester») og direkte delegert
(«Slett fullmakt for {pakke}») — ingen .or-spesialtilfeller.
Naviger dessuten til nøkkelrolle-brukeren via lenkas href i stedet for klikk:
søke-re-renderet detacher lenka midt i klikket. Verifisert stabilt over ~45
kjøringer i at23 (gjenstående massefeil var en trunkert API-respons under
tung parallellkjøring – nettverk, ikke testlogikk).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/pages/profile/accessPackageDelegationPage.ts (1)
219-244: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSolid detach-safe navigation — one nit on the non-exact user link match.
The expand→read-
href→gotoretry neatly sidesteps the "element detached" flake.keyUserLinkuses a non-exact name match, so if a filtered org happens to surface two rettighetshavere whose names sharekeyRoleUserNameas a substring,.first()could resolve the wrong link. Worth confirming names are unique within the filtered list, or tightening toexact: true.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/pages/profile/accessPackageDelegationPage.ts` around lines 219 - 244, The retry-safe navigation in accessPackageDelegationPage is good, but keyUserLink currently uses a non-exact role-name match, which can select the wrong user when multiple links share the same substring. Tighten the locator in the code around keyUserLink/getByRole so it matches the intended user unambiguously, ideally by using an exact name match or by confirming uniqueness after the org filter/search step. Keep the existing expand→read href→goto flow, but make sure the selected link always corresponds to keyRoleUserName before calling goto.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@playwright/pages/profile/accessPackageDelegationPage.ts`:
- Around line 219-244: The retry-safe navigation in accessPackageDelegationPage
is good, but keyUserLink currently uses a non-exact role-name match, which can
select the wrong user when multiple links share the same substring. Tighten the
locator in the code around keyUserLink/getByRole so it matches the intended user
unambiguously, ideally by using an exact name match or by confirming uniqueness
after the org filter/search step. Keep the existing expand→read href→goto flow,
but make sure the selected link always corresponds to keyRoleUserName before
calling goto.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74e4c30c-6315-4cd9-936d-56ce2b6663f5
📒 Files selected for processing (4)
playwright/api-requests/SystemUserApiRequests.tsxplaywright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.tsplaywright/pages/profile/accessPackageDelegationPage.tsplaywright/util/TestdataApi.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
playwright/tenor/testData.ts (1)
32-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFailed calls skip annotation entirely.
test.info().annotations.push(...)only runs afterawait verdi.apply(...)resolves; if the provider method throws (e.g. "Fant ingen ... daglig leder"), the annotation is never recorded — precisely the case where knowing which testdata call failed would be most useful in the trace viewer.♻️ Always annotate, including failures
return async (...args: unknown[]) => { - const resultat = await verdi.apply(target, args); - const argTekst = args.length ? kort(args.length === 1 ? args[0] : args) : ''; - // testInfo finnes bare under Playwright (ikke i tsx-script) → guard. - try { - test.info().annotations.push({ - type: `testdata:${kilde}`, - description: `${String(prop)}(${argTekst}) → ${kort(resultat)}`, - }); - } catch { - // utenfor Playwright-kontekst - } - return resultat; + const argTekst = args.length ? kort(args.length === 1 ? args[0] : args) : ''; + let resultat: unknown; + let feilTekst: string | undefined; + try { + resultat = await verdi.apply(target, args); + return resultat; + } catch (e) { + feilTekst = e instanceof Error ? e.message : String(e); + throw e; + } finally { + // testInfo finnes bare under Playwright (ikke i tsx-script) → guard. + try { + test.info().annotations.push({ + type: `testdata:${kilde}`, + description: feilTekst + ? `${String(prop)}(${argTekst}) → FEIL: ${feilTekst}` + : `${String(prop)}(${argTekst}) → ${kort(resultat)}`, + }); + } catch { + // utenfor Playwright-kontekst + } + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@playwright/tenor/testData.ts` around lines 32 - 53, The medAnnotering wrapper currently adds annotations only after the provider call succeeds, so failures from the proxied method are never captured. Update medAnnotering in testData.ts so the annotation is recorded for every invocation of the wrapped method, including when verdi.apply throws; use the existing symbols test.info().annotations.push, kort, and the Proxy get wrapper to attach either success or failure details before rethrowing the original error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@playwright/tenor/testData.ts`:
- Around line 32-53: The medAnnotering wrapper currently adds annotations only
after the provider call succeeds, so failures from the proxied method are never
captured. Update medAnnotering in testData.ts so the annotation is recorded for
every invocation of the wrapped method, including when verdi.apply throws; use
the existing symbols test.info().annotations.push, kort, and the Proxy get
wrapper to attach either success or failure details before rethrowing the
original error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90c5143b-fe33-447f-9a08-02e33ec62fc9
📒 Files selected for processing (18)
playwright/README.mdplaywright/e2eTests/altinn3/Tilgangsstyring/accessManagement.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/enkelttjenestedelegering.spec.tsplaywright/e2eTests/altinn3/Tilgangsstyring/tilgangspakkedelegering.spec.tsplaywright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.tsplaywright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.tsplaywright/e2eTests/altinn3/systemuser/addOwnOrg.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserChangeRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/approveSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/clientDelegation.spec.tsplaywright/e2eTests/altinn3/systemuser/deleteSystemUser.spec.tsplaywright/e2eTests/altinn3/systemuser/escalateSystemUserRequest.spec.tsplaywright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.tsplaywright/fixture/pomFixture.tsplaywright/tenor/StaticTestData.tsplaywright/tenor/TenorTestData.tsplaywright/tenor/TestDataProvider.tsplaywright/tenor/testData.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- playwright/e2eTests/altinn3/accessPackageDelegation/accessPackageDirectdelegatation.spec.ts
- playwright/e2eTests/altinn3/systemuser/selectSystemVendor.spec.ts
- playwright/e2eTests/altinn3/systemuser/approveSystemUserChangeRequest.spec.ts
- playwright/e2eTests/altinn3/systemuser/escalateSystemUserRequest.spec.ts
- playwright/e2eTests/altinn3/systemuser/approveSystemUserRequest.spec.ts
- playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts
- playwright/e2eTests/altinn3/systemuser/clientDelegation.spec.ts
- playwright/e2eTests/altinn3/systemuser/addOwnOrg.spec.ts
- playwright/e2eTests/altinn3/Tilgangsstyring/enkelttjenestedelegering.spec.ts
- playwright/e2eTests/altinn3/Tilgangsstyring/tilgangspakkedelegering.spec.ts
- playwright/e2eTests/altinn3/Tilgangsstyring/accessManagement.spec.ts
I «fullmakter hos andre»/mottatte-visningen har pakkeknappen antall tjenester i navnet («Byggesøknad 4 tjenester»), mens den på brukere-siden heter bare «Byggesøknad». `exact: true` bommet derfor på hoved-/underenhet- testene (arvet pakke vises uten slett-knapp, så vi kan ikke bruke userCanDeletePackage). Delnavn er trygt her siden visningen ikke har noen «Slett fullmakt for …»-knapp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sneha-sirure
approved these changes
Jul 3, 2026
# Conflicts: # playwright/e2eTests/altinn3/klientadministrasjon/klientadministrasjon.spec.ts # playwright/pages/profile/accessPackageDelegationPage.ts # playwright/pages/tilgangsstyring/KlientAdministrasjonPage.ts
Tenor gir tilfeldige aktører per kjøring, så en feilende test feiler ikke nødvendigvis igjen. Nå tas aktørene opp per test (disk + attachment i rapporten), og TESTDATA_PIN spiller dem tilbake. Alt henger på Proxy-en som allerede annoterte kallene, så ingen tester eller providere er endret. Nøkkelen er metodenavn#ordinal (ikke argumentene, som inneholder verdier fra tidligere tilfeldige kall), og ordinalen tildeles synkront så Promise.all i beforeEach er deterministisk. Opptak er alltid på — det kan ikke skrus på etter at CI er blitt rød. Ved replay får alle forsøk samme data, som er det som skiller en flaky test fra en dårlig aktør.
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.
Mål
Bytt ut delte, hardkodede testdata (fødselsnummer/orgnr) med ferske data fra Tenor per kjøring, så parallelle tester ikke kolliderer (jf. #2086).
Hvordan
Testene henter data via en
testData-fixture i stedet for hardkodede verdier i hver test. Standard er Tenor: hver kjøring får nye, tilfeldige aktører, så to tester aldri deler samme aktør/koblinger. Konvertert:tilgangspakkedelegering,enkelttjenestedelegering,accessManagement,accessPackageDirectdelegatation,klientadministrasjonog systembruker-testene.Slå av Tenor
Sett
USE_TENOR=falsefor å bruke hardkodede reservedata (StaticTestData.ts) i stedet — nyttig hvis Tenor er nede eller for å debugge med faste verdier:Summary by CodeRabbit
Summary