[DRAFT] Playwright tests - #858
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughExpanded Playwright coverage for building block management, document editing, process management, and user dashboards. Added page objects, API helpers, shared schema, overflow-menu, and toggle utilities. Added stable frontend test identifiers and dashboard endpoint definitions. Improved E2E reliability through serial execution, scoped dropdown selection, state polling, guarded cleanup, and failure-only CI video retention. Updated documented coverage totals and scenarios. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
sofiaIvarsRitense
changed the title
|
Draft pull request until this pull request is merged: #798 |
sofiaIvarsRitense
changed the title
# Conflicts: # e2e/tests/access-control/access-control.spec.ts # e2e/tests/access-control/page.ts # e2e/tests/case-details-management-form-flows/page.ts # e2e/tests/case-details-management-tabs/page.ts # e2e/tests/case-details-management-tasks/page.ts # e2e/tests/case-details-management-widgets/case-details-management-widgets.ts # e2e/tests/case-details-management-widgets/page.ts # e2e/tests/case-details-management/case-details-management.spec.ts # e2e/tests/iko/iko-search-action/page.ts # e2e/tests/iko/iko-search-field/page.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/tests/case-details-management-tasks/case-details-management-tasks.spec.ts (1)
59-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard teardown against incomplete
beforeAllsetup.Setup failures run
afterAll, but the cleanup still reads values assigned by setup. Do not mask the setup failure with a second teardown error: guardtasksPagein the tasks spec, and guardwidgetsPage,draftVersion, andwidgetTabKeyin the widgets spec before calling the API cleanup methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: efb59e9f-88c4-4050-9e23-a86645820623
⛔ Files ignored due to path filters (3)
e2e/assets/building-block-archives/e2e-building-block-artwork.pngis excluded by!**/*.pnge2e/assets/building-block-archives/e2e-building-block-import-invalid-file.zipis excluded by!**/*.zipe2e/assets/building-block-archives/e2e-building-block-import-success_1.0.0.zipis excluded by!**/*.zip
📒 Files selected for processing (56)
e2e/TEST_COVERAGE.mde2e/constants/building-block-management-test-ids.tse2e/constants/components-test-ids.tse2e/constants/index.tse2e/playwright.config.tse2e/shared/overflow-menu/overflow-menu.utils.tse2e/shared/schema-editor/schema-editor.utils.tse2e/tests/access-control/access-control.spec.tse2e/tests/access-control/page.tse2e/tests/building-block-details-management-document/building-block-document-config.tse2e/tests/building-block-details-management-document/building-block-document.spec.tse2e/tests/building-block-details-management-document/page.tse2e/tests/building-block-details-management/building-block-details-config.tse2e/tests/building-block-details-management/building-block-details-management.spec.tse2e/tests/building-block-details-management/page.tse2e/tests/building-block-management/building-block-config.tse2e/tests/building-block-management/building-block-management.spec.tse2e/tests/building-block-management/page.tse2e/tests/case-details-management-form-flows/case-details-management-form-flows.spec.tse2e/tests/case-details-management-form-flows/page.tse2e/tests/case-details-management-search-fields/page.tse2e/tests/case-details-management-tabs/case-details-management-tabs.spec.tse2e/tests/case-details-management-tabs/page.tse2e/tests/case-details-management-tasks/case-details-management-tasks.spec.tse2e/tests/case-details-management-tasks/page.tse2e/tests/case-details-management-widgets/case-details-management-widgets.spec.tse2e/tests/case-details-management-widgets/case-details-management-widgets.tse2e/tests/case-details-management-widgets/page.tse2e/tests/case-details-management/case-details-management.spec.tse2e/tests/iko/iko-search-action/page.tse2e/tests/iko/iko-search-field/page.tse2e/tests/task-list/page.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-artwork/building-block-management-artwork.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-artwork/building-block-management-artwork.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-create-modal/building-block-management-create-modal.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-create-modal/building-block-management-create-modal.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-detail-actions/building-block-management-detail-actions.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-detail-actions/building-block-management-detail-actions.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-detail/building-block-management-detail.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-detail/building-block-management-detail.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-list/building-block-management-list.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-list/building-block-management-list.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-metadata/building-block-management-metadata.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-metadata/building-block-management-metadata.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-plugins/building-block-management-plugins.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-plugins/building-block-management-plugins.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-upload-modal/building-block-management-upload-modal.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-upload-modal/building-block-management-upload-modal.component.tsfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-version-selector/building-block-management-version-selector.component.htmlfrontend/projects/valtimo/building-block-management/src/lib/components/building-block-management-version-selector/building-block-management-version-selector.component.tsfrontend/projects/valtimo/building-block-management/src/lib/constants/building-block-management.test-ids.tsfrontend/projects/valtimo/building-block-management/src/lib/constants/index.tsfrontend/projects/valtimo/building-block-management/src/public-api.tsfrontend/projects/valtimo/components/src/lib/components/schema-editor/schema-editor.component.htmlfrontend/projects/valtimo/components/src/lib/components/schema-editor/schema-editor.component.tsfrontend/projects/valtimo/components/src/lib/constants/components.test-ids.ts
| export { | ||
| BUILDING_BLOCK_MANAGEMENT_LIST_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_CREATE_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_UPLOAD_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_DETAIL_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_METADATA_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_ARTWORK_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_PLUGINS_TEST_IDS, | ||
| BUILDING_BLOCK_MANAGEMENT_DETAIL_ACTIONS_TEST_IDS, | ||
| BUILDING_BLOCK_VERSION_OPTION_TEST_ID_PREFIX, | ||
| } from '../../frontend/projects/valtimo/building-block-management/src/lib/constants/building-block-management.test-ids'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required license header.
This TypeScript file starts with export on Line 1. Add the standard EUPL header before the first export.
Proposed header
+/*
+ * Copyright 2015-2026 Ritense BV, the Netherlands.
+ *
+ * Licensed under EUPL, Version 1.2 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
export {As per path instructions, TypeScript files must start with the standard EUPL license header.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { | |
| BUILDING_BLOCK_MANAGEMENT_LIST_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_CREATE_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_UPLOAD_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_DETAIL_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_METADATA_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_ARTWORK_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_PLUGINS_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_DETAIL_ACTIONS_TEST_IDS, | |
| BUILDING_BLOCK_VERSION_OPTION_TEST_ID_PREFIX, | |
| } from '../../frontend/projects/valtimo/building-block-management/src/lib/constants/building-block-management.test-ids'; | |
| /* | |
| * Copyright 2015-2026 Ritense BV, the Netherlands. | |
| * | |
| * Licensed under EUPL, Version 1.2 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ | |
| export { | |
| BUILDING_BLOCK_MANAGEMENT_LIST_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_CREATE_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_UPLOAD_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_DETAIL_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_METADATA_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_ARTWORK_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_PLUGINS_TEST_IDS, | |
| BUILDING_BLOCK_MANAGEMENT_DETAIL_ACTIONS_TEST_IDS, | |
| BUILDING_BLOCK_VERSION_OPTION_TEST_ID_PREFIX, | |
| } from '../../frontend/projects/valtimo/building-block-management/src/lib/constants/building-block-management.test-ids'; |
Source: Path instructions
| @@ -1,3 +1,4 @@ | |||
| export * from './building-block-management-test-ids'; | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required license header.
Line 1 starts with code. Prepend the standard EUPL license header before this export.
As per path instructions, each *.ts file must start with the standard license header.
Source: Path instructions
| async deleteBuildingBlockViaApi(key: string, versionTag: string) { | ||
| try { | ||
| await apiDelete(`${BUILDING_BLOCK_API_URL}/${key}/version/${versionTag}`); | ||
| } catch { | ||
| // No DELETE endpoint yet — nothing to do. | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the API helper error contract before narrowing the catch block.
ast-grep outline e2e/utils/api.utils.ts --items all
rg -n -C 8 '\b(apiDelete|apiPost|apiPut)\b' e2e/utils/api.utils.ts || true
# Find a supported delete endpoint or an explicit E2E database-reset mechanism.
rg -n -C 8 --glob '*.{kt,java,ts}' \
'\bBuildingBlockManagementResource\b|`@DeleteMapping`|\bDELETE\b' . || true
rg -n -i -C 4 \
'((e2e|playwright).*(database|db).*(reset|clean|cleanup)|(reset|clean|cleanup).*(database|db))' . || trueRepository: valtimo-platform/valtimo
Length of output: 50381
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "tracked e2e files of interest:"
git ls-files 'e2e/tests/building-block-details-management-document/*' 'e2e/tests/building-blocks*' 'e2e/**/*.ts' | sed -n '1,200p'
echo
echo "page.ts relevant section:"
sed -n '1,220p' e2e/tests/building-block-details-management-document/page.ts
echo
echo "spec.ts relevant sections:"
sed -n '1,220p' e2e/tests/building-block-details-management-document/building-block-document.spec.ts 2>/dev/null || true
fd -a 'building-block-document\.spec\.ts$' . | xargs -r sed -n '1,220p'
echo
echo "api.utils concise:"
sed -n '1,220p' e2e/utils/api.utils.ts 2>/dev/null || true
echo
echo "focused resource/deletion searches:"
rg -n --glob '*.{kt,java,ts}' --glob '!e2e/**/*common*' \
'BuildingBlockManagementResource|BuildingBlockResource|BuildingBlock|`@DeleteMapping`|deleteBuildingBlock|versionTag|ManagementDocument' . | sed -n '1,240p'
echo
echo "focused cleanup/reset phrases in e2e:"
rg -n -i --glob 'e2e/**/*.{ts,kt,java,yml,yaml,properties,json}' \
'reset|restore|cleanup|clean|flush|database|db\.' e2e | sed -n '1,240p' || trueRepository: valtimo-platform/valtimo
Length of output: 50380
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Focused building block management resource:"
rg -n --glob '*.{kt,java}' 'class BuildingBlockManagementResource|`@RestController`|`@RequestMapping`|`@GetMapping`|`@PostMapping`|`@PutMapping`|`@DeleteMapping`' backend e2e | sed -n '1,240p'
echo
echo "Focused building block deletion references in backend:"
rg -n --glob '*.{kt,java}' 'BuildingBlockManagementResource|BuildingBlockDefinitionRepository|deleteBy.*Key|delete.*Definition|versionTag' backend/building-block backend e2e/tests/building-block* e2e/utils/api.utils.ts | sed -n '1,260p'
echo
echo "Focused e2e teardown/reset/cleanup:"
rg -n --glob 'e2e/**/*.{ts,yml,yaml,properties,json}' -i \
'globalTeardown|globalSetup|afterAll|database|db\.|reset|restore|cleanup|clean|h2|flyway|liquibase' e2e | sed -n '1,260p'Repository: valtimo-platform/valtimo
Length of output: 50381
Remove the generated fixture after the suite.
This suite creates a unique building block in beforeAll. deleteBuildingBlockViaApi() calls /version/{versionTag}, which is unsupported by BuildingBlockManagementResource GET/POST/PUT, and ignores all cleanup failures. That leaves a persistent E2E record until an external cleanup occurs. Add a supported fixture deletion path, or make the E2E reset policy an explicit suite contract. Surface cleanup failures except for the known unsupported DELETE status.
| test.afterAll(async () => { | ||
| // Artwork can be removed; the building block itself cannot — there is no | ||
| // DELETE endpoint for building block definitions (see | ||
| // `deleteBuildingBlockViaApi`). Both versions created here stay behind until | ||
| // that endpoint exists. | ||
| for (const versionTag of [initialVersion, draftVersion]) { | ||
| await detailsPage.deleteArtworkViaApi(buildingBlockKey, versionTag); | ||
| } | ||
|
|
||
| for (const versionTag of [draftVersion, initialVersion]) { | ||
| await detailsPage.deleteBuildingBlockViaApi(buildingBlockKey, versionTag); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect source only. Do not execute repository code.
rg -n -C 3 --glob '*.{ts,js,mjs,cjs,yml,yaml,json,sh}' \
'globalSetup|globalTeardown|reset.*(database|data|environment)|truncate|drop.*(database|schema)|cleanup' \
. || true
rg -n -C 4 --glob '*.ts' \
'\b(deleteBuildingBlockViaApi|deleteArtworkViaApi|apiDelete)\b' \
e2eRepository: valtimo-platform/valtimo
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Candidate e2e config files:\n'
git ls-files | rg '(^|/)(playwright|package|buildkite|gha|ci|docker|k8s|helm|application).*\.(ya?ml|json|ts|js|mjs|cjs)$|(^|/)e2e/.*\.(ya?ml|json|ts|js|mjs|cjs)$' || true
printf '\nE2E-related mentions in config/docs/scripts:\n'
rg -n --hidden --glob '!node_modules/**' --glob '!target/**' --glob '!dist/**' --glob '!build/**' \
'e2e|playwright|globalSetup|globalTeardown|afterAll|deleteBuildingBlockViaApi|deleteArtworkViaApi' \
package.json .github e2e scripts || true
printf '\nRelevant page helper snippets:\n'
sed -n '430,475p' e2e/tests/building-block-details-management/page.ts
sed -n '332,366p' e2e/tests/building-block-management/page.ts
printf '\nRelevant spec teardown snippets:\n'
sed -n '55,85p' e2e/tests/building-block-details-management/building-block-details-management.spec.ts
sed -n '45,70p' e2e/tests/building-block-management/building-block-management.spec.tsRepository: valtimo-platform/valtimo
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Playwright config:\n'
cat -n e2e/playwright.config.ts
printf '\nTeardown utility:\n'
cat -n e2e/utils/teardown.ts
printf '\nGlobal setup utility, summary:\n'
wc -l e2e/utils/globalSetup.ts
sed -n '1,180p' e2e/utils/globalSetup.ts
printf '\nAPI DELETE helper:\n'
sed -n '58,90p' e2e/utils/api.utils.ts
printf '\nBuilding block definitions:\n'
rg -n --glob '*.java' 'class BuildingBlockManagementResource|`@DeleteMapping`|DELETE.*building|version.*building|building-block' backend e2e || trueRepository: valtimo-platform/valtimo
Length of output: 10347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Exact building-block API resource files:\n'
rg -n --glob '*.java' 'BuildingBlockManagementResource|`@RestController`|`@RequestMapping`|/building-block|building/block|processVersionTag|artifact' backend | head -200
printf '\nBuilding-block management page helper complete relevant area:\n'
sed -n '38,55p;340,362p' e2e/tests/building-block-management/page.ts
printf '\nBuilding-block details management page helper complete relevant area:\n'
sed -n '38,48p;435,465p' e2e/tests/building-block-details-management/page.ts
printf '\nBuilding-block specs setup/creation/teardown:\n'
sed -n '50,80p' e2e/tests/building-block-management/building-block-management.spec.ts
sed -n '60,80p' e2e/tests/building-block-details-management/building-block-details-management.spec.ts
printf '\nAPI helper full read-only body:\n'
wc -l e2e/utils/api.utils.ts
sed -n '1,180p' e2e/utils/api.utils.tsRepository: valtimo-platform/valtimo
Length of output: 17246
Handle missing DELETE support in the teardown helpers.
apiDelete() rejects unexpected responses, so deleteBuildingBlockViaApi() can still leave teardown failures unlogged while swallowing 404/405 when the endpoint is missing. Keep deleting 404/405 for this helper, but rethrow other failures. Add a comment noting these E2E suites currently leave built building-block versions behind until a DELETE endpoint exists.
📍 Affects 4 files
e2e/tests/building-block-details-management/building-block-details-management.spec.ts#L68-L79(this comment)e2e/tests/building-block-details-management/page.ts#L446-L468e2e/tests/building-block-management/building-block-management.spec.ts#L53-L63e2e/tests/building-block-management/page.ts#L344-L359
| /** | ||
| * These tests share one building block and run in declaration order: the draft | ||
| * has to be finalized before a new draft can be created, and both versions have | ||
| * to exist before they can be switched between. | ||
| */ | ||
| test.describe('13.18–13.22 — Version lifecycle', () => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect configuration only. Do not execute tests.
fd -0 -t f -a 'playwright\.config\.(ts|js|mjs|cjs)$' . |
xargs -0 -r rg -n -C 3 'fullyParallel|workers|mode\s*:'
rg -n -C 3 \
'test\.describe(\.serial|\.parallel)?|test\.describe\.configure|mode\s*:' \
e2e/tests/building-block-details-management/building-block-details-management.spec.tsRepository: valtimo-platform/valtimo
Length of output: 3350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the version lifecycle tests and top-level test declarations without running repository code.
sed -n '240,335p' e2e/tests/building-block-details-management/building-block-details-management.spec.ts
printf '\n--- all test.describe/define/config mode references ---\n'
rg -n -C 2 'test\.define|test\.describe(\.serial|\.configure|\.parallel)?|mode\s*:\s*["'\'']\w+["'\'']|fullyParallel|workers|CI' e2e/tests e2e -g '*.ts' -g '*.config.*'Repository: valtimo-platform/valtimo
Length of output: 50381
Declare the version lifecycle group as serial.
This group has order-dependent prerequisities within tests that share buildingBlockKey. With the project fullyParallel setting, tests can run in separate workers unless they run in one process via .describe.serial / test.describe.configure({mode: 'serial'}).
Proposed change
-test.describe('13.18–13.22 — Version lifecycle', () => {
+test.describe.serial('13.18–13.22 — Version lifecycle', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * These tests share one building block and run in declaration order: the draft | |
| * has to be finalized before a new draft can be created, and both versions have | |
| * to exist before they can be switched between. | |
| */ | |
| test.describe('13.18–13.22 — Version lifecycle', () => { | |
| /** | |
| * These tests share one building block and run in declaration order: the draft | |
| * has to be finalized before a new draft can be created, and both versions have | |
| * to exist before they can be switched between. | |
| */ | |
| test.describe.serial('13.18–13.22 — Version lifecycle', () => { |
Source: MCP tools
| async getArtworkViaApi(key: string, versionTag: string): Promise<{imageBase64: string} | null> { | ||
| try { | ||
| return await apiGet<{imageBase64: string}>( | ||
| `${BUILDING_BLOCK_API_URL}/${key}/version/${versionTag}/artwork` | ||
| ); | ||
| } catch { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate unexpected artwork read failures.
getArtworkViaApi converts every apiGet exception to null. The deletion test in e2e/tests/building-block-details-management/building-block-details-management.spec.ts, Line 223 treats null as proof that deletion succeeded. A server, authorization, or transport failure can therefore pass the deletion check. Return null only for the expected no-artwork response. Rethrow all other errors.
| test.afterAll(async () => { | ||
| await formFlowsPage.deleteFormFlowViaApi(formFlowTestData.key); | ||
| await context.close(); | ||
| if (context) await context.close(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files 'e2e/tests/*spec.ts' | sed -n '1,80p'
echo
for f in \
e2e/tests/case-details-management-form-flows/case-details-management-form-flows.spec.ts \
e2e/tests/case-details-management-tabs/case-details-management-tabs.spec.ts
do
echo "===== $f ====="
wc -l "$f"
sed -n '1,120p' "$f"
doneRepository: valtimo-platform/valtimo
Length of output: 11642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Page object declarations and teardown patterns:"
rg -n "let .*Page|beforeAll|afterAll|close\\(\\)|deleteFormFlowViaApi|deleteTabViaApi" e2e/tests/case-details-management-form-flows/case-details-management-form-flows.spec.ts e2e/tests/case-details-management-tabs/case-details-management-tabs.spec.ts -C 3Repository: valtimo-platform/valtimo
Length of output: 9179
Guard teardown cleanup against failed test setup.
If test.beforeAll fails before formFlowsPage/tabsPage is assigned, test.afterAll runs and calls the undefined page object directly, hiding the original setup failure. Run the API deletions only when the page object exists first; then close context when available.
📍 Affects 2 files
e2e/tests/case-details-management-form-flows/case-details-management-form-flows.spec.ts#L52-L54(this comment)e2e/tests/case-details-management-tabs/case-details-management-tabs.spec.ts#L59-L70
| <ng-template #buildingBlockDefinitionActions> | ||
| <div class="valtimo-building-block-management-list__actions"> | ||
| <button cdsButton="ghost" (click)="showUploadModal()"> | ||
| <button | ||
| [attr.data-test-id]="testIds.uploadButton" | ||
| cdsButton="ghost" | ||
| (click)="showUploadModal()" | ||
| > | ||
| {{ 'Upload' | translate }} | ||
|
|
||
| <svg cdsIcon="upload" size="16" class="cds--btn__icon"></svg> | ||
| </button> | ||
|
|
||
| <button cdsButton="primary" (click)="showCreateModal()"> | ||
| <button | ||
| [attr.data-test-id]="testIds.createButton" | ||
| cdsButton="primary" | ||
| (click)="showCreateModal()" | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the list action locators to one rendered action set.
buildingBlockDefinitionActions is rendered in both the toolbar and valtimo-no-results. When the list is empty, both new test IDs match two buttons. e2e/tests/building-block-management/page.ts, Line 70, uses page.getByTestId(...) globally, so Playwright strict mode will fail before the first building block can be created or uploaded. Scope the page-object locators to the toolbar, or assign distinct IDs to the empty-state actions.
Proposed locator fix
- return this.page.getByTestId(BUILDING_BLOCK_MANAGEMENT_LIST_TEST_IDS.uploadButton);
+ return this.page
+ .getByLabel('Table action bar')
+ .getByTestId(BUILDING_BLOCK_MANAGEMENT_LIST_TEST_IDS.uploadButton);Apply the same scope to createButton.
| /** | ||
| * Test id for a required-field checkbox, keyed on the property's full path so | ||
| * that a property name repeated at another object level stays unique. | ||
| */ | ||
| protected requiredPropertyTestId(path: string[], property: string): string { | ||
| return `${SCHEMA_EDITOR_REQUIRED_PROPERTY_TEST_ID_PREFIX}${[...(path ?? []), property].join('.')}`; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an injective required-property test ID.
A dot-joined path is ambiguous. A root address.street property and nested address → street property produce the same test ID. This creates duplicate data-test-id values and strict Playwright locator failures.
frontend/projects/valtimo/components/src/lib/components/schema-editor/schema-editor.component.ts#L232-L238: encode each path segment before joining it.e2e/shared/schema-editor/schema-editor.utils.ts#L184-L187: use the identical segment encoding when creating the locator.frontend/projects/valtimo/components/src/lib/constants/components.test-ids.ts#L31-L37: document the encoded path format.
Proposed fix
- return `${SCHEMA_EDITOR_REQUIRED_PROPERTY_TEST_ID_PREFIX}${[...(path ?? []), property].join('.')}`;
+ return `${SCHEMA_EDITOR_REQUIRED_PROPERTY_TEST_ID_PREFIX}${[...(path ?? []), property]
+ .map(segment => encodeURIComponent(segment).replace(/\./g, '%2E'))
+ .join('.')}`;- `${SCHEMA_EDITOR_REQUIRED_PROPERTY_TEST_ID_PREFIX}${path.join('.')}`
+ `${SCHEMA_EDITOR_REQUIRED_PROPERTY_TEST_ID_PREFIX}${path
+ .map(segment => encodeURIComponent(segment).replace(/\./g, '%2E'))
+ .join('.')}`📍 Affects 3 files
frontend/projects/valtimo/components/src/lib/components/schema-editor/schema-editor.component.ts#L232-L238(this comment)e2e/shared/schema-editor/schema-editor.utils.ts#L184-L187frontend/projects/valtimo/components/src/lib/constants/components.test-ids.ts#L31-L37
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: efdb49d3-08c7-4ef0-bc4b-517d684f9023
📒 Files selected for processing (13)
e2e/TEST_COVERAGE.mde2e/api/endpoints.tse2e/constants/dashboard-test-ids.tse2e/constants/index.tse2e/tests/user-dashboard/page.tse2e/tests/user-dashboard/user-dashboard-config.tse2e/tests/user-dashboard/user-dashboard.spec.tsfrontend/projects/valtimo/dashboard/src/lib/components/widget-dashboard-content/widget-dashboard-content.component.htmlfrontend/projects/valtimo/dashboard/src/lib/components/widget-dashboard-content/widget-dashboard-content.component.tsfrontend/projects/valtimo/dashboard/src/lib/components/widget-dashboard/widget-dashboard.component.htmlfrontend/projects/valtimo/dashboard/src/lib/components/widget-dashboard/widget-dashboard.component.tsfrontend/projects/valtimo/dashboard/src/lib/constants/dashboard.test-ids.tsfrontend/projects/valtimo/dashboard/src/lib/constants/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/constants/index.ts
| /** | ||
| * User-facing dashboard endpoints (as consumed by the dashboard shown on `/`). | ||
| * These are filtered by the caller's role via access-control permissions, so they | ||
| * only return dashboards the current user is allowed to view. | ||
| */ | ||
| userDashboard: { | ||
| getAll: '/api/v1/dashboard', | ||
| data: (dashboardKey: string) => `/api/v1/dashboard/${dashboardKey}/data`, | ||
| }, | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the standard EUPL license header at the start of this file.
Line 14 starts code before the required TypeScript header can end. Add the repository standard header before all code. As per path instructions, each .ts file must start with the standard EUPL header.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/tests/process-management/page.ts (2)
465-473: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe catch-all hides real API failures.
getProcessVersionsViaApireturns[]for every thrown error. A 500 response, an auth failure, or a network error then looks identical to "no deployed versions".The spec relies on this result for assertions. At
process-management.spec.tsline 338 the test assertstoHaveLength(0)after a delete, and at lines 290 and 159 it compares counts. A transient failure makes those assertions pass incorrectly.Narrow the catch to the 404 (or whichever status the API returns for an unknown key) and rethrow everything else.
59-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
APIRequestContextplumbing.ProcessManagementPagestores and receives the context, but its API methods call the sharedapiGet/apiDeletehelpers instead.api.utils.tscreates and disposes a module-level context independently, so thisrequestparameter is dead code. Also remove the unusedAPIRequestContextimport frome2e/tests/process-management/page.ts.e2e/tests/process-management/process-management.spec.ts (1)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the shared fixtures and drop the unused
requestvariable.
context,page, andrequestare declared without types, so they are implicitlyanyand lose all editor and compiler checking.requestis also only read once, at line 56, to construct the page object.♻️ Proposed refactor
-import {expect, test} from '`@playwright/test`'; +import {type BrowserContext, type Page, expect, test} from '`@playwright/test`';- let context; - let page; - let request; + let context: BrowserContext; + let page: Page; let processPage: ProcessManagementPage;context = await browser.newContext({baseURL}); page = await context.newPage(); - request = context.request; - processPage = new ProcessManagementPage(page, request); + processPage = new ProcessManagementPage(page, context.request);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df5f8858-d41d-45b9-81e4-35902c3f5356
📒 Files selected for processing (10)
e2e/TEST_COVERAGE.mde2e/constants/process-management-test-ids.tse2e/shared/carbon-toggle/carbon-toggle.utils.tse2e/tests/process-management/page.tse2e/tests/process-management/process-management-config.tse2e/tests/process-management/process-management.spec.tsfrontend/projects/valtimo/process-management/src/lib/components/process-management-builder/process-management-builder.component.htmlfrontend/projects/valtimo/process-management/src/lib/components/process-management-upload/process-management-upload.component.htmlfrontend/projects/valtimo/process-management/src/lib/components/process-management-upload/process-management-upload.component.tsfrontend/projects/valtimo/process-management/src/lib/constants/process-management.test-ids.ts
| test.describe('Process management — standalone processes', () => { | ||
| let context; | ||
| let page; | ||
| let request; | ||
| let processPage: ProcessManagementPage; | ||
|
|
||
| /** Every process key this suite may create, deleted in `afterAll`. */ | ||
| const createdProcessKeys: string[] = [UPLOADED_PROCESS.key, CREATED_PROCESS.key]; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Declare this suite as serial.
The tests in this file depend on each other. The comment at lines 38-40 states the order: the upload test seeds version 1, the edit test saves version 2, and the version tests assert against both. The delete test then removes the key.
All tests also share one page and one context created in beforeAll. Without serial mode, a failing test does not stop the ones that follow, and a retry re-runs a single test against mutated state. Both produce cascading and misleading failures.
Other specs in this PR were changed to serial execution. Apply the same configuration here.
🛠️ Proposed fix
test.describe('Process management — standalone processes', () => {
+ test.describe.configure({mode: 'serial'});
+
let context;
let page;
let request;
let processPage: ProcessManagementPage;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.describe('Process management — standalone processes', () => { | |
| let context; | |
| let page; | |
| let request; | |
| let processPage: ProcessManagementPage; | |
| /** Every process key this suite may create, deleted in `afterAll`. */ | |
| const createdProcessKeys: string[] = [UPLOADED_PROCESS.key, CREATED_PROCESS.key]; | |
| test.describe('Process management — standalone processes', () => { | |
| test.describe.configure({mode: 'serial'}); | |
| let context; | |
| let page; | |
| let request; | |
| let processPage: ProcessManagementPage; | |
| /** Every process key this suite may create, deleted in `afterAll`. */ | |
| const createdProcessKeys: string[] = [UPLOADED_PROCESS.key, CREATED_PROCESS.key]; |
| test('7.1 — Process list shows every unlinked process returned by the API', async () => { | ||
| const processes = await processPage.getProcessesViaApi(); | ||
|
|
||
| await processPage.goToProcessManagement(); | ||
|
|
||
| await expect(processPage.carbonList.rows).toHaveCount(processes.length); | ||
|
|
||
| for (const process of processes) { | ||
| await processPage.assertProcessVisible(process.processDefinition.key); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the backend controller for the unlinked process-definition collection endpoint.
rg -nP -C6 'management/v1/process-definition' --type=kt --type=java
# Check how the frontend list component maps that response into rows.
fd -t f 'process-management-list.component.ts' frontend | xargs -r rg -nP -C6 'processDefinition|map\('Repository: valtimo-platform/valtimo
Length of output: 3315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files matching process-management.spec.ts / process-management-list.component.ts / process-definition APIs:"
git ls-files | rg '(^|/)(process-management\.spec\.ts|process-management-list\.component\.ts|.*ProcessDefinition.*Processor|.*Process.*Repository|.*ProcessDefinition.*Controller|.*ProcessDefinition.*Service).*\.(ts|kt|java)$' || true
echo
echo "Search endpoint strings in all tracked source files:"
rg -n -C4 'management/v1/process-definition|process-definition|ProcessDefinition|ProcessDefinitions|deployed version|unlinked' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' | head -n 300
echo
echo "Inspect e2e spec and page helper candidates:"
fd -t f 'process-management\.spec\.ts|page\.ts|process-management-list\.component\.ts' . | sed -n '1,80p'Repository: valtimo-platform/valtimo
Length of output: 35484
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "process-management.spec.ts relevant lines:"
sed -n '80,105p' e2e/tests/process-management/process-management.spec.ts
echo
echo "process-management page.ts API/result interface lines:"
sed -n '40,75p' e2e/tests/process-management/page.ts
sed -n '458,473p' e2e/tests/process-management/page.ts
echo
echo "backend process-definition RestController candidates:"
rg -n -C5 '@(GetMapping|PostMapping|PutMapping|DeleteMapping).*process-definition|"/process-definition"|GetMapping.*/api/management/v1/process-definition|PostMapping.*/api/management/v1/process-definition' backend --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' || true
echo
echo "backend repository/service terms around unlinked process definitions:"
rg -n -C5 'unlinked|process-definition|ProcessDefinitionRepository|findAll|processDefinition.*key|find.*ProcessDefinition|processDefinition' backend --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' --glob '!**/src/test/**' | head -n 350
echo
echo "frontend list component relevant lines:"
sed -n '1,115p' frontend/projects/valtimo/process-management/src/lib/components/process-management-list/process-management-list.component.ts
sed -n '115,175p' frontend/projects/valtimo/process-management/src/lib/components/process-management-list/process-management-list.component.ts
echo
echo "frontend API/service endpoint definitions:"
rg -n -C4 'process-definition|processManagementService|processes\$\(' frontend --glob '*.ts' | head -n 300Repository: valtimo-platform/valtimo
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ProcessLinkResource unlinked endpoint implementation:"
sed -n '170,205p' backend/process-link/src/main/kotlin/com/ritense/processlink/web/rest/ProcessLinkResource.kt
echo
echo "Operaton process service methods:"
fd -t f 'OperatonProcessService.kt|OperatonProcessService.java' backend | xargs -r rg -n -C6 'fun getUnlinkedDeployedDefinitions|getUnlinkedDeployedDefinitions|getUnlinkedDeployed|getDeployedDefinitions|deployedDefinition' || true
echo
echo "Frontend list component service mapping:"
rg -n -C8 'processManagementService\.processes\$|processes\$|processManagementService|processManagementList|processDefinitions\$|processDefinition\.key' frontend/projects/valtimo/process-management/src/frontend/projects/valtimo/process-management/src/lib/components/process-management-list/process-management-list.component.ts frontend/projects/valtimo --glob '*.ts' | head -n 220
echo
echo "Locate ProcessManagementService definition:"
fd -t f 'process-management.*service.*\.(ts|js)$|process-management.*\.(ts|js)$' frontend/projects/valtimo | xargs -r rg -n -C4 'processes\$|processMgmt|process-definition|processDefinitions' || trueRepository: valtimo-platform/valtimo
Length of output: 39163
Compare with the key count, not the API row count.
getProcessesViaApi() returns one entry per deployed unlinked version, but the management list renders rows from processManagementService.processes$, not the collection endpoint. If any key has multiple versions, processes.length will compare against versions and can fail even when the key column is shown once. Deduplicate processes by processDefinition.key before comparing and asserting.
sofiaIvarsRitense
changed the title
…ion and Features 12 and 4 — Object management and Objects (user)
1 participant
Describe the changes
Link to the related Github issue:
Specify the code branch location:
Relevant comments:
Breaking changes
Documentation
New features or changes that have been introduced have been documented.
Tests
Unit tests have been added that cover these changes
Integration tests have been added that cover these changes
Describe the testing steps
Security
The Secure by Design principle has been applied to these changes
Added or changed REST API endpoints have authentication and authorization in place
Valtimo access control checks have been implemented
Dependencies
Newly added dependencies do not introduce known vulnerabilities/CVE's and are in line with the Valtimo license