fix(ui): hide write/edit controls from READ_ALL (read-only) users (#696) #699

Merged

marcel merged 4 commits from feat/issue-696-hide-write-controls into main 2026-05-31 11:31:35 +02:00

Conversation 7 Commits 4 Files Changed 3 +34 -1

marcel commented 2026-05-31 11:14:08 +02:00

Owner

Copy Link

Closes #696.

What

Closes the one confirmed write-control leak in the global chrome: the header "Hochladen" (upload) button was gated only on {#if data?.user}, so a reader without WRITE_ALL saw it, clicked it, and got bounced by the server-side redirect in documents/new — confusing friction on the main read journey.

Changes

Commit	What
c3652f5b	fix(ui) — +layout.svelte gate changed to {#if data?.user && data.canWrite} (canWrite already on layout data). Red-first via a canWrite: false → no-upload-link test.
97274beb	test(layout) — breadth guard: an ANNOTATE_ALL-only user (canWrite: false) still sees no upload link, proving the gate keys on lack of WRITE_ALL, not on being READ_ALL.
5edefdd0	test(document) — explicit @WithMockUser(authorities = "READ_ALL") 403 boundary tests for POST /api/documents and POST /api/documents/quick-upload — the control that actually matters.

Verification

• layout.svelte.spec.ts (client project): 11 passed — new test confirmed red before the gate, green after.
• DocumentControllerTest: 104 passed (102 + 2 new READ_ALL boundary tests).
• svelte-check: no errors in the touched files. (32 repo-wide errors are pre-existing on main in users/[id] / admin/users, untouched here.)

Scope

No backend model change, no i18n change, no API regeneration (upload_action already exists), no doc updates (no new route/permission/ErrorCode). DropZone.svelte left without an internal guard per the issue's defensive note. canAnnotate rewiring and read-only transcription (#697) are out of scope.

🤖 Generated with Claude Code

Closes #696. ## What Closes the one confirmed write-control leak in the global chrome: the header **"Hochladen"** (upload) button was gated only on `{#if data?.user}`, so a reader without `WRITE_ALL` saw it, clicked it, and got bounced by the server-side redirect in `documents/new` — confusing friction on the main read journey. ## Changes | Commit | What | |---|---| | `c3652f5b` | **fix(ui)** — `+layout.svelte` gate changed to `{#if data?.user && data.canWrite}` (`canWrite` already on layout data). Red-first via a `canWrite: false` → no-upload-link test. | | `97274beb` | **test(layout)** — breadth guard: an `ANNOTATE_ALL`-only user (`canWrite: false`) still sees no upload link, proving the gate keys on lack of `WRITE_ALL`, not on being `READ_ALL`. | | `5edefdd0` | **test(document)** — explicit `@WithMockUser(authorities = "READ_ALL")` 403 boundary tests for `POST /api/documents` and `POST /api/documents/quick-upload` — the control that actually matters. | ## Verification - `layout.svelte.spec.ts` (client project): **11 passed** — new test confirmed red before the gate, green after. - `DocumentControllerTest`: **104 passed** (102 + 2 new READ_ALL boundary tests). - `svelte-check`: no errors in the touched files. (32 repo-wide errors are pre-existing on `main` in `users/[id]` / `admin/users`, untouched here.) ## Scope No backend model change, no i18n change, no API regeneration (`upload_action` already exists), no doc updates (no new route/permission/ErrorCode). `DropZone.svelte` left without an internal guard per the issue's defensive note. `canAnnotate` rewiring and read-only transcription (#697) are out of scope. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 3 commits 2026-05-31 11:14:09 +02:00

fix(ui): hide header upload button from non-writers (#696) ... c3652f5b57

The header "Hochladen" link was gated only on {#if data?.user}, so a
reader without WRITE_ALL saw it, clicked it, and got bounced by the
server-side redirect in documents/new — confusing friction on the main
read journey. Gate it on data.canWrite (already on the layout data).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(layout): lock upload-button gate against ANNOTATE_ALL-only users (#696) ... 97274beba0

Documents that the gate keys on lack of WRITE_ALL, not on being READ_ALL:
an ANNOTATE_ALL-only user (canWrite=false) must still not see the upload
link. The writer-sees-it contract is already covered by the existing
upload-link tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(document): document READ_ALL -> 403 on document write endpoints (#696) ... 5edefdd082

Hiding the header upload button is UI polish; the real control is endpoint
authz. Add explicit READ_ALL-only 403 boundary tests for POST /api/documents
and POST /api/documents/quick-upload, matching the reader-only convention
already used elsewhere in this suite.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-31 11:15:06 +02:00

Author

Owner

Copy Link

🛡️ Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

This is exactly the shape a UI-hardening fix should take: the visual gate is cosmetic, and the real control (server-side 403) is the part that gets the explicit regression tests. The PR understands that hiding a button is not a security boundary — denying the request is.

What I checked

• Defense in depth is intact. +layout.svelte now reads {#if data?.user && data.canWrite}, and canWrite is derived server-side in +layout.server.ts from the user's WRITE_ALL permission — not client-trusted state. Hiding the link removes UI friction; it does not become the access control.
• The authorization boundary is now tested at the right layer. The two new DocumentControllerTest cases assert @WithMockUser(authorities = "READ_ALL") → 403 for POST /api/documents and POST /api/documents/quick-upload. This is precisely the "test that the unauthorized user is blocked, not just that the authorized one succeeds" pattern. CWE-862 (Missing Authorization) regression coverage — good.
• No new attack surface. No new endpoint, no @RequestMapping without a method, no permission magic strings, no error-message leakage. The change is purely subtractive on the client and additive on the test side.

Notes (not blockers)

• The new tests assert 403 for an authenticated-but-unauthorized reader. The complementary 401-unauthenticated case for these endpoints is presumably already covered elsewhere in this file (the diff context shows a sibling isForbidden test above each insertion). If it is not, I'd want a POST with no auth → 401 alongside these — but that's a pre-existing-coverage question, not something this PR regressed.
• DropZone.svelte is intentionally left without an internal guard per the issue. I'm comfortable with that: the server rejects the upload regardless, so the component guard would be redundant belt-and-suspenders, not a missing control.

Solid, minimal, well-tested. No security objections.

## 🛡️ Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** This is exactly the shape a UI-hardening fix should take: the visual gate is cosmetic, and the *real* control (server-side 403) is the part that gets the explicit regression tests. The PR understands that hiding a button is not a security boundary — denying the request is. ### What I checked - **Defense in depth is intact.** `+layout.svelte` now reads `{#if data?.user && data.canWrite}`, and `canWrite` is derived server-side in `+layout.server.ts` from the user's `WRITE_ALL` permission — not client-trusted state. Hiding the link removes UI friction; it does not become the access control. - **The authorization boundary is now tested at the right layer.** The two new `DocumentControllerTest` cases assert `@WithMockUser(authorities = "READ_ALL")` → 403 for `POST /api/documents` and `POST /api/documents/quick-upload`. This is precisely the "test that the unauthorized user is *blocked*, not just that the authorized one succeeds" pattern. CWE-862 (Missing Authorization) regression coverage — good. - **No new attack surface.** No new endpoint, no `@RequestMapping` without a method, no permission magic strings, no error-message leakage. The change is purely subtractive on the client and additive on the test side. ### Notes (not blockers) - The new tests assert 403 for an *authenticated-but-unauthorized* reader. The complementary 401-unauthenticated case for these endpoints is presumably already covered elsewhere in this file (the diff context shows a sibling `isForbidden` test above each insertion). If it is not, I'd want a `POST` with no auth → 401 alongside these — but that's a pre-existing-coverage question, not something this PR regressed. - `DropZone.svelte` is intentionally left without an internal guard per the issue. I'm comfortable with that: the server rejects the upload regardless, so the component guard would be redundant belt-and-suspenders, not a missing control. Solid, minimal, well-tested. No security objections.

marcel commented 2026-05-31 11:15:18 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Clean, disciplined, TDD-shaped change. The fix is a one-token condition tweak, and the tests precede and justify it. Nothing here violates the project's clean-code or Svelte 5 rules.

TDD evidence — good

• The PR body documents red-first (canWrite: false → no-upload-link test asserted red before the gate). The two READ_ALL backend tests are pure boundary additions. This is the right red/green order.

Frontend (+layout.svelte)

• The gate moved from {#if data?.user} to {#if data?.user && data.canWrite}. The condition is short and reads as intent. Borderline on my own "no business logic in template markup" rule — but two flags with self-documenting names is fine; extracting a $derived const canUpload = data?.user && data.canWrite would be marginally cleaner and consistent with how isAdmin/isAuthPage are already $derived in this same file. Suggestion, not a blocker — if you touch this again, derive it.
• Props discipline is unchanged; the component already receives data, and canWrite is a real layout-data field, so no new coupling.

Tests (layout.svelte.spec.ts)

• Two new specs use the existing makeData({ canWrite: false }) factory with overrides — exactly the factory pattern I want. Names read as sentences. The second test ("ANNOTATE_ALL-only … gate is lack of WRITE_ALL, not READ_ALL") is a genuinely valuable breadth guard, not a duplicate — it pins the reason the gate fires.
• Assertions test user-visible behavior (getByRole('link') … .not.toBeInTheDocument()), not internals.

Backend (DocumentControllerTest.java)

• should-less naming (createDocument_returns403_forReaderOnly) matches the existing convention in this file, so consistency wins over my personal should_ preference. .with(csrf()) is correctly included so the request reaches the authorization layer rather than failing CSRF first. Good.

No blockers.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Clean, disciplined, TDD-shaped change. The fix is a one-token condition tweak, and the tests precede and justify it. Nothing here violates the project's clean-code or Svelte 5 rules. ### TDD evidence — good - The PR body documents red-first (`canWrite: false` → no-upload-link test asserted red before the gate). The two READ_ALL backend tests are pure boundary additions. This is the right red/green order. ### Frontend (`+layout.svelte`) - The gate moved from `{#if data?.user}` to `{#if data?.user && data.canWrite}`. The condition is short and reads as intent. Borderline on my own "no business logic in template markup" rule — but two flags with self-documenting names is fine; extracting a `$derived const canUpload = data?.user && data.canWrite` would be marginally cleaner and consistent with how `isAdmin`/`isAuthPage` are already `$derived` in this same file. **Suggestion, not a blocker** — if you touch this again, derive it. - Props discipline is unchanged; the component already receives `data`, and `canWrite` is a real layout-data field, so no new coupling. ### Tests (`layout.svelte.spec.ts`) - Two new specs use the existing `makeData({ canWrite: false })` factory with overrides — exactly the factory pattern I want. Names read as sentences. The second test ("ANNOTATE_ALL-only … gate is lack of WRITE_ALL, not READ_ALL") is a genuinely valuable breadth guard, not a duplicate — it pins the *reason* the gate fires. - Assertions test user-visible behavior (`getByRole('link')` … `.not.toBeInTheDocument()`), not internals. ### Backend (`DocumentControllerTest.java`) - `should`-less naming (`createDocument_returns403_forReaderOnly`) matches the existing convention in this file, so consistency wins over my personal `should_` preference. `.with(csrf())` is correctly included so the request reaches the authorization layer rather than failing CSRF first. Good. No blockers.

marcel commented 2026-05-31 11:15:30 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ✅ Approved

The test strategy here is textbook: the behavior is verified at the layer where it lives. UI visibility → component test; authorization → controller slice test. No test pushed up the pyramid unnecessarily, no E2E bloat for a one-line gate.

Coverage assessment

• Component layer: Two new vitest-browser-svelte specs cover the negative cases (canWrite: false, and ANNOTATE_ALL-only). Combined with the pre-existing positive specs ("navigates to /documents/new", "has aria-label"), the upload-link visibility matrix is now: write-user sees it / reader doesn't / annotator-only doesn't. That's the full truth table that matters.
• Controller layer: @WithMockUser(authorities = "READ_ALL") → isForbidden() for both upload endpoints. This is the permission-boundary test I always ask for — proving the endpoint rejects the unauthorized role, not just that it accepts the authorized one. Sits next to existing WRITE_ALL → 200 tests, so each endpoint now has matched accept/reject coverage.

Determinism / hygiene

• The new specs reuse the existing makeData factory and the established beforeEach/afterEach (fetch stub + cleanup() + unstubAllGlobals()), so no new flakiness vector is introduced. The fetch stub that prevents the /login redirect race is already in place and the new tests inherit it.
• The controller tests use .with(csrf()), so they exercise the authorization filter rather than short-circuiting on CSRF — correct and deterministic.

Minor notes (not blockers)

• These browser specs run client-project / CI-only per this project's norms; I'm trusting the PR's reported "11 passed" + "104 passed" rather than a local run. The numbers are consistent with 2+2 additions.
• If you ever want the belt-and-suspenders, an integration-layer test that a READ_ALL user loading the layout gets canWrite: false would pin the server derivation — but the controller 403 tests already cover the security-critical half, so this is optional polish.

Quality gate: pass.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ✅ Approved** The test strategy here is textbook: the behavior is verified at the layer where it lives. UI visibility → component test; authorization → controller slice test. No test pushed up the pyramid unnecessarily, no E2E bloat for a one-line gate. ### Coverage assessment - **Component layer:** Two new `vitest-browser-svelte` specs cover the negative cases (`canWrite: false`, and `ANNOTATE_ALL`-only). Combined with the pre-existing positive specs ("navigates to /documents/new", "has aria-label"), the upload-link visibility matrix is now: write-user sees it / reader doesn't / annotator-only doesn't. That's the full truth table that matters. - **Controller layer:** `@WithMockUser(authorities = "READ_ALL")` → `isForbidden()` for both upload endpoints. This is the permission-boundary test I always ask for — proving the endpoint *rejects* the unauthorized role, not just that it accepts the authorized one. Sits next to existing `WRITE_ALL → 200` tests, so each endpoint now has matched accept/reject coverage. ### Determinism / hygiene - The new specs reuse the existing `makeData` factory and the established `beforeEach`/`afterEach` (fetch stub + `cleanup()` + `unstubAllGlobals()`), so no new flakiness vector is introduced. The fetch stub that prevents the `/login` redirect race is already in place and the new tests inherit it. - The controller tests use `.with(csrf())`, so they exercise the authorization filter rather than short-circuiting on CSRF — correct and deterministic. ### Minor notes (not blockers) - These browser specs run client-project / CI-only per this project's norms; I'm trusting the PR's reported "11 passed" + "104 passed" rather than a local run. The numbers are consistent with 2+2 additions. - If you ever want the belt-and-suspenders, an integration-layer test that a READ_ALL user `load`ing the layout gets `canWrite: false` would pin the server derivation — but the controller 403 tests already cover the security-critical half, so this is optional polish. Quality gate: pass.

marcel commented 2026-05-31 11:15:39 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

No architectural concerns. This is a leaf-level UI gate plus test coverage — it touches no module boundary, no layer rule, no schema, and no transport choice.

Boundaries & layering

• The visibility decision keys on data.canWrite, which is already derived in +layout.server.ts from the session user's permissions. The component consumes a server-computed flag rather than re-deriving permission logic in the template — that's the correct direction of dependency. No business/authorization logic leaked into the view beyond a boolean read.
• The authoritative control remains server-side (@RequirePermission on the controller, exercised by the new 403 tests). UI gate and server gate are layered correctly: cosmetic in the client, enforced in the backend. This is exactly how application-layer access control should be split.

Documentation currency

I ran the PR against my doc-trigger checklist:

• No new Flyway migration, table, column, or FK → DB diagrams untouched. ✅
• No new package, controller, service, route, Docker service, external system. ✅
• No new ErrorCode or Permission value (WRITE_ALL and canWrite already exist). ✅
• Auth/upload-flow change? This is the one to scrutinize. The change does not alter the upload sequence or the auth flow — it hides an entry point that was always server-rejected anyway. The seq-document-upload.puml and seq-auth-flow.puml diagrams describe the request/response sequence, which is unchanged. No diagram update required.

No ADR warranted — this is a bug fix correcting an entry-point that leaked past a canWrite flag, not a lasting structural decision.

Boring, correct, minimal. Approved.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** No architectural concerns. This is a leaf-level UI gate plus test coverage — it touches no module boundary, no layer rule, no schema, and no transport choice. ### Boundaries & layering - The visibility decision keys on `data.canWrite`, which is already derived in `+layout.server.ts` from the session user's permissions. The component consumes a server-computed flag rather than re-deriving permission logic in the template — that's the correct direction of dependency. No business/authorization logic leaked into the view beyond a boolean read. - The authoritative control remains server-side (`@RequirePermission` on the controller, exercised by the new 403 tests). UI gate and server gate are layered correctly: cosmetic in the client, enforced in the backend. This is exactly how application-layer access control should be split. ### Documentation currency I ran the PR against my doc-trigger checklist: - No new Flyway migration, table, column, or FK → DB diagrams untouched. ✅ - No new package, controller, service, route, Docker service, external system. ✅ - No new `ErrorCode` or `Permission` value (`WRITE_ALL` and `canWrite` already exist). ✅ - **Auth/upload-flow change?** This is the one to scrutinize. The change does *not* alter the upload sequence or the auth flow — it hides an entry point that was always server-rejected anyway. The `seq-document-upload.puml` and `seq-auth-flow.puml` diagrams describe the request/response sequence, which is unchanged. No diagram update required. No ADR warranted — this is a bug fix correcting an entry-point that leaked past a `canWrite` flag, not a lasting structural decision. Boring, correct, minimal. Approved.

marcel commented 2026-05-31 11:15:50 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX & Accessibility Lead

Verdict: ✅ Approved

This is a real UX win, and it's the right fix from a user-journey standpoint. A reader without WRITE_ALL previously saw a "Hochladen" button, clicked it, and got bounced by a server redirect — that's a dead-end interaction (Nielsen #5, Error Prevention, and #1, Visibility of System Status). Removing an affordance the user can't act on is better than letting them hit a wall.

Accessibility / brand checks

• The upload link retains its aria-label={m.upload_action()}, aria-hidden="true" on the decorative SVG, and the i18n label span. The change only gates whether it renders — it doesn't degrade the control's accessibility when it does render. No regression there.
• The pre-existing spec "has aria-label for screen reader access" still guards the labeling, so screen-reader users with write access are unaffected.
• No color, contrast, touch-target, or typography surface was touched. The button is unchanged for write-users.

One thing I'd flag for the broader effort (not a blocker here)

The PR scopes itself to the header upload button only and explicitly defers the documents/[id]/edit controls and read-only transcription (#697). That's a reasonable slice. But from a UX-consistency view, a read-only user will now have a coherent header (no upload CTA) yet may still encounter edit/save affordances deeper in the app that also dead-end. That's a known follow-up per the issue, so I'm not holding this PR for it — just noting the journey isn't fully consistent until #697 lands. Worth keeping the follow-up visible so readers don't hit the same dead-end one level down.

For what it changes: clean, accessible, and it removes friction on the primary read path. Approved.

## 🎨 Leonie Voss — UX & Accessibility Lead **Verdict: ✅ Approved** This is a real UX win, and it's the *right* fix from a user-journey standpoint. A reader without `WRITE_ALL` previously saw a "Hochladen" button, clicked it, and got bounced by a server redirect — that's a dead-end interaction (Nielsen #5, Error Prevention, and #1, Visibility of System Status). Removing an affordance the user can't act on is better than letting them hit a wall. ### Accessibility / brand checks - The upload link retains its `aria-label={m.upload_action()}`, `aria-hidden="true"` on the decorative SVG, and the i18n label span. The change only gates *whether* it renders — it doesn't degrade the control's accessibility when it does render. No regression there. - The pre-existing spec "has aria-label for screen reader access" still guards the labeling, so screen-reader users with write access are unaffected. - No color, contrast, touch-target, or typography surface was touched. The button is unchanged for write-users. ### One thing I'd flag for the broader effort (not a blocker here) The PR scopes itself to the header upload button only and explicitly defers the `documents/[id]/edit` controls and read-only transcription (#697). That's a reasonable slice. But from a UX-consistency view, a read-only user will now have a *coherent* header (no upload CTA) yet may still encounter edit/save affordances deeper in the app that also dead-end. That's a known follow-up per the issue, so I'm not holding this PR for it — just noting the journey isn't fully consistent until #697 lands. Worth keeping the follow-up visible so readers don't hit the same dead-end one level down. For what it changes: clean, accessible, and it removes friction on the primary read path. Approved.

marcel commented 2026-05-31 11:15:57 +02:00

Author

Owner

Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved (LGTM)

Nothing in my domain to flag. I checked for the things I always check:

• No infrastructure surface touched — no Compose file, no Dockerfile, no CI workflow, no image tags, no env vars, no secrets. Three files: one Svelte component, one frontend spec, one Java test.
• No new env var or config that would need to be threaded through docker-compose*.yml or documented in DEPLOYMENT.md. canWrite is computed at runtime from session data — zero deployment config.
• CI impact is negligible — two more JUnit cases and two more vitest-browser specs. Both run in existing test jobs; no new job, no new runner requirement, no artifact change. Sub-second additions to suites that already exist.
• The backend tests are @WebMvcTest-style slices (per the surrounding file), so they don't spin up Testcontainers/MinIO — no added CI resource pressure.

No reproducibility, secrets, or pipeline concerns. Ship it.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved (LGTM)** Nothing in my domain to flag. I checked for the things I always check: - **No infrastructure surface touched** — no Compose file, no Dockerfile, no CI workflow, no image tags, no env vars, no secrets. Three files: one Svelte component, one frontend spec, one Java test. - **No new env var or config** that would need to be threaded through `docker-compose*.yml` or documented in `DEPLOYMENT.md`. `canWrite` is computed at runtime from session data — zero deployment config. - **CI impact is negligible** — two more JUnit cases and two more vitest-browser specs. Both run in existing test jobs; no new job, no new runner requirement, no artifact change. Sub-second additions to suites that already exist. - The backend tests are `@WebMvcTest`-style slices (per the surrounding file), so they don't spin up Testcontainers/MinIO — no added CI resource pressure. No reproducibility, secrets, or pipeline concerns. Ship it.

marcel commented 2026-05-31 11:16:10 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with one traceability concern

The change is well-scoped and the implementation matches the stated intent. My lens is requirements completeness and traceability, and there's a small gap between the issue title and what this PR delivers.

What's good

• Clear problem statement, measurable behavior. The PR articulates the JTBD precisely: "a reader without WRITE_ALL saw the upload button, clicked it, and got bounced" — a real friction point on the primary read journey. The fix is testable and tested (component visibility + 403 boundary). Definition of Done is effectively met for the header upload control.
• Scope is explicitly bounded. The PR names its non-goals (DropZone guard, canAnnotate rewiring, read-only transcription #697). Naming non-goals is exactly the scope-creep discipline I want to see.

The concern — title vs. delivered scope (traceability)

The PR title is "hide write/edit controls from READ_ALL (read-only) users (#696)" — plural "controls", and it names edit explicitly. What actually ships is a single control: the header upload button. Edit controls (documents/[id]/edit, save/delete affordances) are deferred.

This is an ambiguity/traceability flag, not a code problem:

• A reader of issue #696 would reasonably expect "write/edit controls" (plural) to be addressed. Shipping only the upload button under that title risks the issue being auto-closed (via "Closes #696") while edit-control leaks remain — a classic requirements-debt trap where the tracker says "done" but the user-facing acceptance criterion isn't fully met.

Recommendation (not a blocker for the code): Either (a) narrow this PR's framing to "header upload button" and keep #696 open for the remaining edit controls, or (b) confirm #696's acceptance criteria were explicitly scoped down to just this control and the rest tracked under #697. Make sure the "done" state of #696 is unambiguous — right now the title promises more than the diff delivers.

NFR checklist (quick pass)

• Security: covered (403 tests). Accessibility: unaffected (label retained). Usability: improved. i18n: no new strings. Performance/observability: N/A.

The implementation is sound; my only ask is that the backlog's "done" reflects reality. Resolve the title-vs-scope question and this is a clean close.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with one traceability concern** The change is well-scoped and the implementation matches the stated intent. My lens is requirements completeness and traceability, and there's a small gap between the issue *title* and what this PR *delivers*. ### What's good - **Clear problem statement, measurable behavior.** The PR articulates the JTBD precisely: "a reader without `WRITE_ALL` saw the upload button, clicked it, and got bounced" — a real friction point on the primary read journey. The fix is testable and tested (component visibility + 403 boundary). Definition of Done is effectively met for the header upload control. - **Scope is explicitly bounded.** The PR names its non-goals (DropZone guard, `canAnnotate` rewiring, read-only transcription #697). Naming non-goals is exactly the scope-creep discipline I want to see. ### The concern — title vs. delivered scope (traceability) The PR title is **"hide write/edit controls from READ_ALL (read-only) users (#696)"** — plural "controls", and it names **edit** explicitly. What actually ships is a single control: the header **upload** button. Edit controls (`documents/[id]/edit`, save/delete affordances) are deferred. This is an **ambiguity/traceability** flag, not a code problem: - A reader of issue #696 would reasonably expect "write/edit controls" (plural) to be addressed. Shipping only the upload button under that title risks the issue being auto-closed (via "Closes #696") while edit-control leaks remain — a classic requirements-debt trap where the tracker says "done" but the user-facing acceptance criterion isn't fully met. **Recommendation (not a blocker for the code):** Either (a) narrow this PR's framing to "header upload button" and keep #696 open for the remaining edit controls, or (b) confirm #696's acceptance criteria were explicitly scoped down to just this control and the rest tracked under #697. Make sure the "done" state of #696 is unambiguous — right now the title promises more than the diff delivers. ### NFR checklist (quick pass) - Security: covered (403 tests). Accessibility: unaffected (label retained). Usability: improved. i18n: no new strings. Performance/observability: N/A. The implementation is sound; my only ask is that the backlog's "done" reflects reality. Resolve the title-vs-scope question and this is a clean close.

marcel added 1 commit 2026-05-31 11:22:44 +02:00

refactor(layout): extract canUpload derived for the upload-button gate (#696) ... 944370dcfd

Move the inline {#if data?.user && data.canWrite} condition into a named
$derived, matching the existing isAdmin / isAuthPage derivations in the
same file. No behaviour change — the 11 layout specs stay green.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel merged commit 944370dcfd into main 2026-05-31 11:31:35 +02:00

marcel deleted branch feat/issue-696-hide-write-controls 2026-05-31 11:31:35 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

needs-discussion

Has an open decision or design question that must be resolved before implementation can start.

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#699