Client #124: E2E tests and full app test coverage#127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe web client now uses real server APIs at runtime, removes mock-switch configuration, and adds Playwright E2E infrastructure with fixture-backed API and authentication stubs. Shared fixtures and broad unit/component tests cover core pages, roles, dialogs, validation, and profile editing. ChangesClient testing and runtime integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
web-client/e2e/events.spec.ts (1)
41-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile locator: duplicate event names make
.first()selection order-dependent.
eventSummaryFixtures[0]("Football Juniors Open Session", idaaaaaaaa-0001-...) shares its name withaaaaaaaa-0031-...in the fixtures (web-client/src/testing/fixtures/events.tslines 1059-1091). Both the edit test (line 45) and delete test (line 62) locate the row byEdit/Delete ${target.name}+.first(). This only targets the intended event if the rendered list order happens to match the fixture array order — ifSportEventsPagesorts by date/name differently, these tests could silently operate on the wrong row.Consider scoping the locator to a row containing the event's
id(e.g., adata-testidor a row locator filtered by id) instead of relying on accessible name +.first().🤖 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 `@web-client/e2e/events.spec.ts` around lines 41 - 68, Replace the order-dependent `.first()` locators in the “edits an event name from the list” and “deletes an event after confirmation” tests with locators scoped to the specific fixture event identified by its id. Use an existing row identifier or add a stable data-testid/id-based row locator, then find the corresponding Edit/Delete button within that row.web-client/src/features/organization/api/queries.ts (1)
44-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent path-parameter encoding across single-resource endpoints.
useSport(Line 44) encodes the id withencodeURIComponent, butuseUpdateSport(Line 62),useDeleteSport(Line 75),useTeam(Line 100),useUpdateTeam(Line 118), anduseDeleteTeam(Line 130) interpolate the rawid. ApplyencodeURIComponentuniformly so all resource lookups stay correct if an id ever contains a reserved URL character.♻️ Proposed fix for consistent encoding
- mutationFn: ({ id, ...data }) => organizationClient.patch<Sport>(`/sports/${id}`, data).then(r => r.data), + mutationFn: ({ id, ...data }) => organizationClient.patch<Sport>(`/sports/${encodeURIComponent(id)}`, data).then(r => r.data),- mutationFn: id => organizationClient.delete(`/sports/${id}`).then(() => undefined), + mutationFn: id => organizationClient.delete(`/sports/${encodeURIComponent(id)}`).then(() => undefined),- queryFn: () => organizationClient.get<Team>(`/teams/${id}`).then(r => r.data), + queryFn: () => organizationClient.get<Team>(`/teams/${encodeURIComponent(id)}`).then(r => r.data),- mutationFn: ({ id, ...data }) => organizationClient.patch<Team>(`/teams/${id}`, data).then(r => r.data), + mutationFn: ({ id, ...data }) => organizationClient.patch<Team>(`/teams/${encodeURIComponent(id)}`, data).then(r => r.data),- mutationFn: id => organizationClient.delete(`/teams/${id}`).then(() => undefined), + mutationFn: id => organizationClient.delete(`/teams/${encodeURIComponent(id)}`).then(() => undefined),Also applies to: 62-62, 75-75, 100-100, 118-118, 130-130
🤖 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 `@web-client/src/features/organization/api/queries.ts` at line 44, Single-resource endpoint hooks inconsistently encode IDs in URL paths. Update useUpdateSport, useDeleteSport, useTeam, useUpdateTeam, and useDeleteTeam to wrap each interpolated id with encodeURIComponent, matching useSport while preserving the existing request behavior.
🤖 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 `@web-client/e2e/support/server/events.ts`:
- Around line 30-32: Replace the plain Error-based eventError helper with
httpError throughout the event server module. Remove eventError, import or reuse
httpError, and update every eventError call in createEvent, updateEvent,
deleteEvent, validateEventTimes, memberRef, sportRef, and teamRef to pass the
appropriate 400, 403, or 404 status and existing message.
In `@web-client/e2e/support/server/helper.ts`:
- Around line 94-113: Add authorization enforcement to deleteReport by accepting
the requesting user, updating the api.ts call site to pass USER, and rejecting
non-admin users whose identity does not match the report creator, matching
deleteFeedback’s checks. Update related callers and tests to cover authorized,
unauthorized, and admin deletion.
In `@web-client/e2e/support/server/organization.ts`:
- Around line 186-194: Trim the incoming name, description, and address values
in updateTeam before assigning them, matching the normalization performed by
createTeam and updateSport. Preserve the existing team values when each field is
omitted, and leave sport and member reference handling unchanged.
In `@web-client/e2e/support/server/payments.ts`:
- Line 94: Missing scope authorization check when updateTransaction reassigns a
transaction to another member. In updateTransaction, when data.member is
provided and nextMember differs from the existing member, call
canCreateTransactionForMember for the new member before applying the update, and
reject unauthorized reassignment; preserve the existing member when data.member
is undefined.
In `@web-client/src/features/members/components/MemberEditorDialog.test.tsx`:
- Around line 170-185: Strengthen the test “maps bean-validation field errors
onto the form” by asserting the first-name field displays the mapped validation
message, not only the form-level “Validation failed” text. After submission,
locate the error associated with the `member-first-name` field and verify it
contains “must not be blank” (or the rendered equivalent), while retaining the
existing form-level assertion if desired.
In `@web-client/src/testing/fixtures/events.ts`:
- Around line 1064-1091: Remove the duplicate Lena Roth attendee from the
attendees array for event aaaaaaaa-0031-... in both eventSummaryFixtures and the
corresponding eventDetailsById entry, leaving only one entry with ID
11111111-1111-1111-1111-111111111111.
---
Nitpick comments:
In `@web-client/e2e/events.spec.ts`:
- Around line 41-68: Replace the order-dependent `.first()` locators in the
“edits an event name from the list” and “deletes an event after confirmation”
tests with locators scoped to the specific fixture event identified by its id.
Use an existing row identifier or add a stable data-testid/id-based row locator,
then find the corresponding Edit/Delete button within that row.
In `@web-client/src/features/organization/api/queries.ts`:
- Line 44: Single-resource endpoint hooks inconsistently encode IDs in URL
paths. Update useUpdateSport, useDeleteSport, useTeam, useUpdateTeam, and
useDeleteTeam to wrap each interpolated id with encodeURIComponent, matching
useSport while preserving the existing request behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55755765-d369-4c38-8df4-193dfabb9891
⛔ Files ignored due to path filters (1)
web-client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (77)
web-client/.env.development.exampleweb-client/.gitignoreweb-client/e2e/README.mdweb-client/e2e/app-shell.spec.tsweb-client/e2e/boot.spec.tsweb-client/e2e/events.spec.tsweb-client/e2e/feedback.spec.tsweb-client/e2e/helper.spec.tsweb-client/e2e/letters.spec.tsweb-client/e2e/members.spec.tsweb-client/e2e/organization.spec.tsweb-client/e2e/pages.spec.tsweb-client/e2e/payments.spec.tsweb-client/e2e/profile.spec.tsweb-client/e2e/support/api.tsweb-client/e2e/support/auth.tsweb-client/e2e/support/data.tsweb-client/e2e/support/fixtures.tsweb-client/e2e/support/server/dashboard.tsweb-client/e2e/support/server/events.tsweb-client/e2e/support/server/feedback.tsweb-client/e2e/support/server/helper.tsweb-client/e2e/support/server/letters.tsweb-client/e2e/support/server/members.tsweb-client/e2e/support/server/organization.tsweb-client/e2e/support/server/payments.tsweb-client/package.jsonweb-client/playwright.config.tsweb-client/src/__tests__/DashboardPage.test.tsxweb-client/src/app/layout/AppShell.test.tsxweb-client/src/app/pages/api/dashboardQueries.tsweb-client/src/app/router/RouteRoleGuard.test.tsxweb-client/src/app/theme/ThemeProvider.test.tsxweb-client/src/features/auth/currentUser.test.tsweb-client/src/features/auth/currentUser.tsweb-client/src/features/auth/useAuth.tsweb-client/src/features/feedback/api/queries.tsweb-client/src/features/feedback/components/FeedbackComposeDialog.test.tsxweb-client/src/features/feedback/components/FeedbackEditDialog.test.tsxweb-client/src/features/feedback/pages/FeedbackPage.test.tsxweb-client/src/features/helper/api/queries.tsweb-client/src/features/helper/pages/HelperPage.test.tsxweb-client/src/features/letters/api/queries.tsweb-client/src/features/letters/pages/LettersPage.test.tsxweb-client/src/features/members/api/queries.tsweb-client/src/features/members/components/MemberEditorDialog.test.tsxweb-client/src/features/members/model/useMembersViewModel.test.tsweb-client/src/features/members/pages/MembersPage.test.tsxweb-client/src/features/organization/api/queries.tsweb-client/src/features/organization/pages/OrganizationPage.test.tsxweb-client/src/features/payments/api/queries.tsweb-client/src/features/payments/pages/PaymentsPage.test.tsxweb-client/src/features/profile/pages/ProfilePage.test.tsxweb-client/src/features/profile/pages/ProfilePage.tsxweb-client/src/features/sport-events/api/queries.tsweb-client/src/features/sport-events/components/SportEventEditorDialog.test.tsxweb-client/src/features/sport-events/model/useEventsViewModel.test.tsweb-client/src/features/sport-events/pages/SportEventsPage.test.tsxweb-client/src/lib/keycloak.tsweb-client/src/mocks/mockSwitch.tsweb-client/src/testing/fixtures/dashboard.tsweb-client/src/testing/fixtures/events.tsweb-client/src/testing/fixtures/feedback.tsweb-client/src/testing/fixtures/finance.tsweb-client/src/testing/fixtures/index.tsweb-client/src/testing/fixtures/members.tsweb-client/src/testing/fixtures/organization.tsweb-client/src/testing/fixtures/report.tsweb-client/src/testing/httpError.tsweb-client/src/testing/personas.tsweb-client/src/testing/scope.test.tsweb-client/src/testing/scope.tsweb-client/src/types.test.tsweb-client/src/vite-env.d.tsweb-client/tsconfig.e2e.jsonweb-client/tsconfig.jsonweb-client/vite.config.ts
💤 Files with no reviewable changes (2)
- web-client/src/mocks/mockSwitch.ts
- web-client/src/vite-env.d.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (2)
web-client/e2e/events.spec.ts (1)
41-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragile locator: duplicate event names make
.first()selection order-dependent.
eventSummaryFixtures[0]("Football Juniors Open Session", idaaaaaaaa-0001-...) shares its name withaaaaaaaa-0031-...in the fixtures (web-client/src/testing/fixtures/events.tslines 1059-1091). Both the edit test (line 45) and delete test (line 62) locate the row byEdit/Delete ${target.name}+.first(). This only targets the intended event if the rendered list order happens to match the fixture array order — ifSportEventsPagesorts by date/name differently, these tests could silently operate on the wrong row.Consider scoping the locator to a row containing the event's
id(e.g., adata-testidor a row locator filtered by id) instead of relying on accessible name +.first().🤖 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 `@web-client/e2e/events.spec.ts` around lines 41 - 68, Replace the order-dependent `.first()` locators in the “edits an event name from the list” and “deletes an event after confirmation” tests with locators scoped to the specific fixture event identified by its id. Use an existing row identifier or add a stable data-testid/id-based row locator, then find the corresponding Edit/Delete button within that row.web-client/src/features/organization/api/queries.ts (1)
44-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent path-parameter encoding across single-resource endpoints.
useSport(Line 44) encodes the id withencodeURIComponent, butuseUpdateSport(Line 62),useDeleteSport(Line 75),useTeam(Line 100),useUpdateTeam(Line 118), anduseDeleteTeam(Line 130) interpolate the rawid. ApplyencodeURIComponentuniformly so all resource lookups stay correct if an id ever contains a reserved URL character.♻️ Proposed fix for consistent encoding
- mutationFn: ({ id, ...data }) => organizationClient.patch<Sport>(`/sports/${id}`, data).then(r => r.data), + mutationFn: ({ id, ...data }) => organizationClient.patch<Sport>(`/sports/${encodeURIComponent(id)}`, data).then(r => r.data),- mutationFn: id => organizationClient.delete(`/sports/${id}`).then(() => undefined), + mutationFn: id => organizationClient.delete(`/sports/${encodeURIComponent(id)}`).then(() => undefined),- queryFn: () => organizationClient.get<Team>(`/teams/${id}`).then(r => r.data), + queryFn: () => organizationClient.get<Team>(`/teams/${encodeURIComponent(id)}`).then(r => r.data),- mutationFn: ({ id, ...data }) => organizationClient.patch<Team>(`/teams/${id}`, data).then(r => r.data), + mutationFn: ({ id, ...data }) => organizationClient.patch<Team>(`/teams/${encodeURIComponent(id)}`, data).then(r => r.data),- mutationFn: id => organizationClient.delete(`/teams/${id}`).then(() => undefined), + mutationFn: id => organizationClient.delete(`/teams/${encodeURIComponent(id)}`).then(() => undefined),Also applies to: 62-62, 75-75, 100-100, 118-118, 130-130
🤖 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 `@web-client/src/features/organization/api/queries.ts` at line 44, Single-resource endpoint hooks inconsistently encode IDs in URL paths. Update useUpdateSport, useDeleteSport, useTeam, useUpdateTeam, and useDeleteTeam to wrap each interpolated id with encodeURIComponent, matching useSport while preserving the existing request behavior.
🤖 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 `@web-client/e2e/support/server/events.ts`:
- Around line 30-32: Replace the plain Error-based eventError helper with
httpError throughout the event server module. Remove eventError, import or reuse
httpError, and update every eventError call in createEvent, updateEvent,
deleteEvent, validateEventTimes, memberRef, sportRef, and teamRef to pass the
appropriate 400, 403, or 404 status and existing message.
In `@web-client/e2e/support/server/helper.ts`:
- Around line 94-113: Add authorization enforcement to deleteReport by accepting
the requesting user, updating the api.ts call site to pass USER, and rejecting
non-admin users whose identity does not match the report creator, matching
deleteFeedback’s checks. Update related callers and tests to cover authorized,
unauthorized, and admin deletion.
In `@web-client/e2e/support/server/organization.ts`:
- Around line 186-194: Trim the incoming name, description, and address values
in updateTeam before assigning them, matching the normalization performed by
createTeam and updateSport. Preserve the existing team values when each field is
omitted, and leave sport and member reference handling unchanged.
In `@web-client/e2e/support/server/payments.ts`:
- Line 94: Missing scope authorization check when updateTransaction reassigns a
transaction to another member. In updateTransaction, when data.member is
provided and nextMember differs from the existing member, call
canCreateTransactionForMember for the new member before applying the update, and
reject unauthorized reassignment; preserve the existing member when data.member
is undefined.
In `@web-client/src/features/members/components/MemberEditorDialog.test.tsx`:
- Around line 170-185: Strengthen the test “maps bean-validation field errors
onto the form” by asserting the first-name field displays the mapped validation
message, not only the form-level “Validation failed” text. After submission,
locate the error associated with the `member-first-name` field and verify it
contains “must not be blank” (or the rendered equivalent), while retaining the
existing form-level assertion if desired.
In `@web-client/src/testing/fixtures/events.ts`:
- Around line 1064-1091: Remove the duplicate Lena Roth attendee from the
attendees array for event aaaaaaaa-0031-... in both eventSummaryFixtures and the
corresponding eventDetailsById entry, leaving only one entry with ID
11111111-1111-1111-1111-111111111111.
---
Nitpick comments:
In `@web-client/e2e/events.spec.ts`:
- Around line 41-68: Replace the order-dependent `.first()` locators in the
“edits an event name from the list” and “deletes an event after confirmation”
tests with locators scoped to the specific fixture event identified by its id.
Use an existing row identifier or add a stable data-testid/id-based row locator,
then find the corresponding Edit/Delete button within that row.
In `@web-client/src/features/organization/api/queries.ts`:
- Line 44: Single-resource endpoint hooks inconsistently encode IDs in URL
paths. Update useUpdateSport, useDeleteSport, useTeam, useUpdateTeam, and
useDeleteTeam to wrap each interpolated id with encodeURIComponent, matching
useSport while preserving the existing request behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55755765-d369-4c38-8df4-193dfabb9891
⛔ Files ignored due to path filters (1)
web-client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (77)
web-client/.env.development.exampleweb-client/.gitignoreweb-client/e2e/README.mdweb-client/e2e/app-shell.spec.tsweb-client/e2e/boot.spec.tsweb-client/e2e/events.spec.tsweb-client/e2e/feedback.spec.tsweb-client/e2e/helper.spec.tsweb-client/e2e/letters.spec.tsweb-client/e2e/members.spec.tsweb-client/e2e/organization.spec.tsweb-client/e2e/pages.spec.tsweb-client/e2e/payments.spec.tsweb-client/e2e/profile.spec.tsweb-client/e2e/support/api.tsweb-client/e2e/support/auth.tsweb-client/e2e/support/data.tsweb-client/e2e/support/fixtures.tsweb-client/e2e/support/server/dashboard.tsweb-client/e2e/support/server/events.tsweb-client/e2e/support/server/feedback.tsweb-client/e2e/support/server/helper.tsweb-client/e2e/support/server/letters.tsweb-client/e2e/support/server/members.tsweb-client/e2e/support/server/organization.tsweb-client/e2e/support/server/payments.tsweb-client/package.jsonweb-client/playwright.config.tsweb-client/src/__tests__/DashboardPage.test.tsxweb-client/src/app/layout/AppShell.test.tsxweb-client/src/app/pages/api/dashboardQueries.tsweb-client/src/app/router/RouteRoleGuard.test.tsxweb-client/src/app/theme/ThemeProvider.test.tsxweb-client/src/features/auth/currentUser.test.tsweb-client/src/features/auth/currentUser.tsweb-client/src/features/auth/useAuth.tsweb-client/src/features/feedback/api/queries.tsweb-client/src/features/feedback/components/FeedbackComposeDialog.test.tsxweb-client/src/features/feedback/components/FeedbackEditDialog.test.tsxweb-client/src/features/feedback/pages/FeedbackPage.test.tsxweb-client/src/features/helper/api/queries.tsweb-client/src/features/helper/pages/HelperPage.test.tsxweb-client/src/features/letters/api/queries.tsweb-client/src/features/letters/pages/LettersPage.test.tsxweb-client/src/features/members/api/queries.tsweb-client/src/features/members/components/MemberEditorDialog.test.tsxweb-client/src/features/members/model/useMembersViewModel.test.tsweb-client/src/features/members/pages/MembersPage.test.tsxweb-client/src/features/organization/api/queries.tsweb-client/src/features/organization/pages/OrganizationPage.test.tsxweb-client/src/features/payments/api/queries.tsweb-client/src/features/payments/pages/PaymentsPage.test.tsxweb-client/src/features/profile/pages/ProfilePage.test.tsxweb-client/src/features/profile/pages/ProfilePage.tsxweb-client/src/features/sport-events/api/queries.tsweb-client/src/features/sport-events/components/SportEventEditorDialog.test.tsxweb-client/src/features/sport-events/model/useEventsViewModel.test.tsweb-client/src/features/sport-events/pages/SportEventsPage.test.tsxweb-client/src/lib/keycloak.tsweb-client/src/mocks/mockSwitch.tsweb-client/src/testing/fixtures/dashboard.tsweb-client/src/testing/fixtures/events.tsweb-client/src/testing/fixtures/feedback.tsweb-client/src/testing/fixtures/finance.tsweb-client/src/testing/fixtures/index.tsweb-client/src/testing/fixtures/members.tsweb-client/src/testing/fixtures/organization.tsweb-client/src/testing/fixtures/report.tsweb-client/src/testing/httpError.tsweb-client/src/testing/personas.tsweb-client/src/testing/scope.test.tsweb-client/src/testing/scope.tsweb-client/src/types.test.tsweb-client/src/vite-env.d.tsweb-client/tsconfig.e2e.jsonweb-client/tsconfig.jsonweb-client/vite.config.ts
💤 Files with no reviewable changes (2)
- web-client/src/mocks/mockSwitch.ts
- web-client/src/vite-env.d.ts
🛑 Comments failed to post (6)
web-client/e2e/support/server/events.ts (1)
30-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace
eventErrorwithhttpErrorfor correct HTTP status codes in E2E responses.
eventErrorcreates a plainErrorwith no HTTP status code, so the API layer cannot produce the correct response status. Every other server module (feedback.ts,helper.ts) useshttpError(status, message)consistently. This meansupdateEvent/deleteEvent404 and 403 cases,createEvent/validateEventTimes400 cases, andmemberRef/sportRef/teamRef404 cases will all surface as 500 errors in E2E tests.🐛 Proposed fix: remove `eventError` and use `httpError` throughout
function eventError(message: string): Error { return new Error(message) } function newEventId(): string {Remove
eventError(lines 30–32) and replace all call sites:function memberRef(id: string): Reference { const name = memberNamesById[id] - if (!name) throw eventError('Member not found') + if (!name) throw httpError(404, 'Member not found') return { id, name } } function sportRef(id: string): Reference { const sport = sportFixtures.find((item) => item.id === id) - if (!sport) throw eventError('Sport not found') + if (!sport) throw httpError(404, 'Sport not found') return { id: sport.id, name: sport.name } } function teamRef(id: string): Reference { const team = teamFixtures.find((item) => item.id === id) - if (!team) throw eventError('Team not found') + if (!team) throw httpError(404, 'Team not found') return { id: team.id, name: team.name } }function validateEventTimes(startTime: string, endTime: string): void { if (Number.isNaN(new Date(startTime).getTime()) || Number.isNaN(new Date(endTime).getTime())) { - throw eventError('Start and end time are required') + throw httpError(400, 'Start and end time are required') } if (new Date(endTime) <= new Date(startTime)) { - throw eventError('End time must be after start time') + throw httpError(400, 'End time must be after start time') } }const name = data.name.trim() - if (!name) throw eventError('Name is required') + if (!name) throw httpError(400, 'Name is required') validateEventTimes(data.start_time, data.end_time)const current = eventDetailsState[id] - if (!current) throw eventError('Event not found') + if (!current) throw httpError(404, 'Event not found') if (!canManageEvent(user, current)) { - throw eventError('You are not allowed to update this event') + throw httpError(403, 'You are not allowed to update this event') } const name = data.name === undefined ? current.name : data.name.trim() - if (!name) throw eventError('Name is required') + if (!name) throw httpError(400, 'Name is required')const event = eventDetailsState[id] - if (!event) throw eventError('Event not found') + if (!event) throw httpError(404, 'Event not found') if (!canManageEvent(user, event)) { - throw eventError('You are not allowed to delete this event') + throw httpError(403, 'You are not allowed to delete this event') }Also applies to: 44-44, 50-50, 56-56, 74-74, 78-78, 135-135, 164-164, 166-166, 170-170, 201-201, 203-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 `@web-client/e2e/support/server/events.ts` around lines 30 - 32, Replace the plain Error-based eventError helper with httpError throughout the event server module. Remove eventError, import or reuse httpError, and update every eventError call in createEvent, updateEvent, deleteEvent, validateEventTimes, memberRef, sportRef, and teamRef to pass the appropriate 400, 403, or 404 status and existing message.web-client/e2e/support/server/helper.ts (1)
94-113: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
deleteReportskips authorization checks.Unlike
deleteFeedbackinfeedback.tswhich verifiesuser.role !== 'admin' && existing.creator?.id !== user.id,deleteReportaccepts nouserparameter and allows any caller to delete any report. The API layer (api.ts) callsdeleteReport(kind)without passingUSER. This may be covered by the known follow-up for non-admin authorization branches, but flagging to ensure it's tracked.🤖 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 `@web-client/e2e/support/server/helper.ts` around lines 94 - 113, Add authorization enforcement to deleteReport by accepting the requesting user, updating the api.ts call site to pass USER, and rejecting non-admin users whose identity does not match the report creator, matching deleteFeedback’s checks. Update related callers and tests to cover authorized, unauthorized, and admin deletion.web-client/e2e/support/server/organization.ts (1)
186-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Inconsistent field trimming between
createTeamandupdateTeam.
createTeamtrimsname,description, andaddress(lines 128, 140–141), butupdateTeamuses rawdata.name ?? team.name,data.description ?? team.description, anddata.address ?? team.addresswithout trimming. This means updated team names could retain leading/trailing whitespace, diverging from create behavior.updateSportcorrectly trimsdata.name(line 95), making this inconsistency specific toupdateTeam.♻️ Proposed fix to trim fields in `updateTeam`
const updated: Team = { ...team, - name: data.name ?? team.name, - description: data.description ?? team.description, - address: data.address ?? team.address, + name: data.name !== undefined ? data.name.trim() : team.name, + description: data.description !== undefined ? data.description.trim() : team.description, + address: data.address !== undefined ? data.address.trim() : team.address, sport: nextSport ? { id: nextSport.id, name: nextSport.name } : team.sport, trainers: data.trainers !== undefined ? memberRefsFromIds(data.trainers) : team.trainers, trainees: data.trainees !== undefined ? memberRefsFromIds(data.trainees) : team.trainees, }📝 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.const updated: Team = { ...team, name: data.name !== undefined ? data.name.trim() : team.name, description: data.description !== undefined ? data.description.trim() : team.description, address: data.address !== undefined ? data.address.trim() : team.address, sport: nextSport ? { id: nextSport.id, name: nextSport.name } : team.sport, trainers: data.trainers !== undefined ? memberRefsFromIds(data.trainers) : team.trainers, trainees: data.trainees !== undefined ? memberRefsFromIds(data.trainees) : team.trainees, }🤖 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 `@web-client/e2e/support/server/organization.ts` around lines 186 - 194, Trim the incoming name, description, and address values in updateTeam before assigning them, matching the normalization performed by createTeam and updateSport. Preserve the existing team values when each field is omitted, and leave sport and member reference handling unchanged.web-client/e2e/support/server/payments.ts (1)
94-94: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing scope authorization check when reassigning transaction member.
createTransaction(line 60) correctly validatescanCreateTransactionForMemberbefore creating, butupdateTransactiondoes not re-check scope whendata.memberchanges. A trainer who created a transaction for member A can reassign it to member B outside their scope — bypassing the authorization thatcreateTransactionenforces. This means E2E tests exercising member reassignment would get false positives.🔒 Proposed fix to add scope check for the new member
// Resolve a new member only if the caller is moving the transaction; keep the ref otherwise. const nextMember = data.member !== undefined ? memberRef(data.member) : existing.member + if (data.member !== undefined && data.member !== existing.member.id) { + if (!canCreateTransactionForMember(user, data.member)) { + throw httpError(403, 'You are not allowed to create transactions for this member.') + } + } + const title = data.title !== undefined ? data.title.trim() : existing.titleAlso applies to: 111-114
🤖 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 `@web-client/e2e/support/server/payments.ts` at line 94, Missing scope authorization check when updateTransaction reassigns a transaction to another member. In updateTransaction, when data.member is provided and nextMember differs from the existing member, call canCreateTransactionForMember for the new member before applying the update, and reject unauthorized reassignment; preserve the existing member when data.member is undefined.web-client/src/features/members/components/MemberEditorDialog.test.tsx (1)
170-185: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test name claims field-level mapping but only asserts form-level error.
The test "maps bean-validation field errors onto the form" provides
httpError(400, 'Validation failed', [{ message: 'first_name: must not be blank' }])but only checksdocument.body.textContentcontains'Validation failed'. It doesn't verify that thefirst_namefield error is actually mapped and displayed next to the field.💚 Proposed fix to strengthen the assertion
expect(document.body.textContent).toContain('Validation failed') + expect(document.body.textContent).toContain('must not be blank')📝 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.it('maps bean-validation field errors onto the form', async () => { mutationMocks.createMember.mockRejectedValue( httpError(400, 'Validation failed', [{ message: 'first_name: must not be blank' }]), ) await renderOpenCreate() await fillField('member-first-name', 'Nina') await fillField('member-last-name', 'Neu') await fillField('member-email', 'nina.neu@club.de') await fillField('member-password', 'initial-secret') await submitForm() await submitForm() await submitForm() expect(document.body.textContent).toContain('Validation failed') expect(document.body.textContent).toContain('must not be blank') })🤖 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 `@web-client/src/features/members/components/MemberEditorDialog.test.tsx` around lines 170 - 185, Strengthen the test “maps bean-validation field errors onto the form” by asserting the first-name field displays the mapped validation message, not only the form-level “Validation failed” text. After submission, locate the error associated with the `member-first-name` field and verify it contains “must not be blank” (or the rendered equivalent), while retaining the existing form-level assertion if desired.web-client/src/testing/fixtures/events.ts (1)
1064-1091: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Duplicate attendee entry for event
aaaaaaaa-0031-...."Lena Roth" (
11111111-1111-1111-1111-111111111111) appears twice consecutively in the attendees array, in botheventSummaryFixtures(lines 1065-1066) and the correspondingeventDetailsByIdentry (lines 2395-2396). If any UI renders attendees keyed byattendee.id, this produces a duplicate-key warning, and any attendee-count logic would be inflated by one.🐛 Proposed fix
"attendees": [ { id: "11111111-1111-1111-1111-111111111111", name: "Lena Roth" }, - { id: "11111111-1111-1111-1111-111111111111", name: "Lena Roth" }, { id: "99999999-0013-0001-be1e-0000000bbe15", name: "Marie Wolf" },(apply the equivalent removal in the
eventDetailsByIdentry as well)Also applies to: 2394-2425
🤖 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 `@web-client/src/testing/fixtures/events.ts` around lines 1064 - 1091, Remove the duplicate Lena Roth attendee from the attendees array for event aaaaaaaa-0031-... in both eventSummaryFixtures and the corresponding eventDetailsById entry, leaving only one entry with ID 11111111-1111-1111-1111-111111111111.
Editable name and email both landed in the profile page during the e2e migration; email must stay account-managed while name stays editable.
The e2e suite only ran locally so far, leaving it unenforced and prone to silent rot; wire it into the same job as the rest of the web-client checks.
|
lgtm! |
2 participants
Why
The web client was built and tested against mocks while the backend was still incomplete. Now that every service exists, we need end-to-end tests over the core journeys and much better unit coverage so the whole app is guarded against regressions, and the client can drop its mock-switch crutch entirely.
What changed
e2e/support/server/*.ts) — Playwright intercepts/api/v1/*and Keycloak requests and serves them from a stateful in-memory fixture store, so e2e runs are hermetic and don't need a live backend.RouteRoleGuard,AppShell,ThemeProvider, and every feature page's dialogs/forms (feedback, helper, letters, members, organization, payments, sport-events, profile).mockOr/VITE_USE_MOCKS/VITE_MOCK_PERSONAandsrc/mocks/mockSwitch.tsare gone; the client now depends solely on the server.src/mocks/was renamed tosrc/testing/(fixtures, personas, scope helpers) since it's test-only support now, not a runtime code path.Notes
Ran
/code-reviewover the diff; three items are worth flagging as known follow-ups rather than blockers (all pre-existing gaps in test infra, not regressions — full suite is green):e2e/support/server/events.ts) return HTTP 500 instead of 400/403/404 for validation/permission failures — latent, no current test hits it.run()JSON-serializes theBlobgeneratePdf()returns) — latent, no current test hits it.Filed as follow-up polish, not addressed here to keep this diff scoped to landing the suite.
Testing
pnpm -C web-client lint,typecheck, andbuildall passpnpm -C web-client test— 203/203 passingpnpm -C web-client e2e— 41/41 passingCloses #124
Summary by CodeRabbit