Conversation
Add "Delete permanently" to the dashboard gallery card menu, wiring up the existing (previously unused) api.deleteGallery client and the existing DELETE /galleries/:id route. - Require the gallery to be archived first, enforced both in the menu and server-side (409 gallery_not_archived) since the API is also used outside this UI. - Guard against deleting a gallery with any print orders (409 gallery_has_print_orders) — PrintOrderItem references File via a Restrict FK, so this also avoids an unfriendly DB constraint error. - Add role-based delete authorization (canDeleteGallery in gallery-access.ts, deliberately stricter than the general access model): admin -> any visible gallery, owner -> only their own, member -> never. Exposed to the frontend as a canDelete field on each gallery so the UI never computes permission logic itself. - Confirmation requires typing the gallery's exact title via the shared usePrompt() primitive. - New apiError.* i18n keys for the two new error cases. Closes markusthiel#24. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The frontend confirmation currently accepts non-exact title matches and the new 403 error code (delete_not_allowed) lacks an i18n translation, both conflicting with the intended UX/error messaging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a safe “permanent delete” capability for galleries by exposing the existing hard-delete endpoint through the Studio dashboard, while enforcing stricter server-side guards (authorization, archived-only, and print-order protection) and surfacing delete permission to the UI.
Changes:
- Frontend: adds “Delete permanently” to archived gallery card menu with typed-title confirmation and
api.deleteGallerywiring. - API: introduces
canDeleteGalleryrole rule, returnscanDeletein gallery list/collection responses, and enforces delete guards inDELETE /galleries/:id(403/409 cases). - Tests/i18n/changelog: adds guard-order unit tests, new i18n keys for 409 cases, and documents the feature in the changelog.
File summaries
| File | Description |
|---|---|
| CHANGELOG.md | Documents the new permanent delete capability and constraints. |
| apps/frontend/src/lib/i18n/it.ts | Adds translated UI + API error strings for delete flow (Italian). |
| apps/frontend/src/lib/i18n/fi.ts | Adds translated UI + API error strings for delete flow (Finnish). |
| apps/frontend/src/lib/i18n/en.ts | Adds translated UI + API error strings for delete flow (English). |
| apps/frontend/src/lib/i18n/de.ts | Adds translated UI + API error strings for delete flow (German). |
| apps/frontend/src/lib/api.ts | Extends Gallery type with backend-computed canDelete. |
| apps/frontend/src/app/studio/page.tsx | Implements “Delete permanently” menu item and typed confirmation flow. |
| apps/api/src/routes/galleries.ts | Adds delete guards (permission, archived, print orders) + canDelete in list responses. |
| apps/api/src/routes/galleries.delete.test.ts | Adds unit tests covering delete permission rules and guard ordering. |
| apps/api/src/routes/collections.ts | Includes canDelete in smart-collection gallery results. |
| apps/api/src/lib/gallery-access.ts | Introduces canDeleteGallery stricter role-based delete authorization. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (typed.trim().toLowerCase() !== g.title.trim().toLowerCase()) { | ||
| notify(t("studio.deleteTitleMismatch")); | ||
| return; | ||
| } |
| emailTaken: "That email address is already in use in this studio.", | ||
| galleryHasPrintOrders: | ||
| "This gallery has print orders and cannot be deleted.", | ||
| galleryNotArchived: "Archive the gallery before deleting it permanently.", |
| customerMismatch: "Session gehört nicht zu diesem Tenant.", | ||
| emailTaken: "Diese E-Mail-Adresse ist im Studio bereits vergeben.", | ||
| galleryHasPrintOrders: | ||
| "Diese Galerie hat Druckbestellungen und kann nicht gelöscht werden.", | ||
| galleryNotArchived: | ||
| "Die Galerie muss vor dem endgültigen Löschen archiviert werden.", |
| customerMismatch: "Questa sessione non appartiene a questo tenant.", | ||
| emailTaken: "Questo indirizzo email è già in uso in questo studio.", | ||
| galleryHasPrintOrders: | ||
| "Questa galleria ha ordini di stampa e non può essere eliminata.", | ||
| galleryNotArchived: | ||
| "Archivia la galleria prima di eliminarla definitivamente.", |
| emailTaken: "Tämä sähköpostiosoite on jo käytössä tässä studiossa.", | ||
| galleryHasPrintOrders: | ||
| "Tällä gallerialla on tulostustilauksia, eikä sitä voi poistaa.", | ||
| galleryNotArchived: "Arkistoi galleria ennen sen pysyvää poistamista.", |
Review of the automated Copilot findingsI checked both issues Copilot raised against the actual source rather than taking them at face value: 1. 2. Case-insensitive title match — assessed as by design, not a bug, but flagging for a second opinion. No other issues found on an independent pass of the diff. Not pushing any changes yet — will apply the Finnish translation review@canja006 — since you authored the original Finnish translation (
|
useErrorText() falls back to the raw ApiError.message when no
apiError.<code> key matches, and DELETE /galleries/:id sends
{ error: "delete_not_allowed" } with no message field — so without a
translation, the raw snake_case code would leak into a user-facing
toast on the rare stale-canDelete race (role/ownership changed
between page load and click).
Add apiError.deleteNotAllowed to all 4 locale files, matching the
existing galleryHasPrintOrders/galleryNotArchived keys added for the
same guard chain.
Addresses the review comment on PR markusthiel#25 (flagged by Copilot).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed 9232c2d: adds Left the case-insensitive title match as-is per the earlier discussion (matches |
|
Checked all seven — six are natural Finnish as written, nice work. One grammar fix needed in |
|
Thank @canja006, I'll update that |
deleteConfirmMessage used the nominative "galleria" where the total object of "poistaa" (delete) requires the genitive-accusative "gallerian". Per @canja006's native-speaker review on PR markusthiel#25. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for this — the guard work is exactly right, and I like that The role matrix needs adjusting, and I think the naming is to blame rather than anything you did. In Lumio As written, the studio owner would be the one person unable to delete a gallery an employee created, while an admin could delete any gallery shared with them. That collides with why the owner role has gallery-wide reach at all — the comment above For deletion I don't want a distinction between owner and admin: an admin is an admin. Which makes this simpler than the matrix I put in #24, because the visibility check already does the scoping. The lookup with /**
* Delete-Berechtigung. Owner und Admin duerfen loeschen, Member nie.
* Die REICHWEITE ergibt sich aus galleryAccessWhere(), das in der Route
* vorher greift und sonst 404 liefert — fuer den Owner also jede
* Galerie des Studios, fuer den Admin seine eigenen plus Freigaben.
* Deshalb braucht diese Funktion die Galerie selbst nicht.
*/
export function canDeleteGallery(s: SessionContext): boolean {
return s.user.role === "owner" || s.user.role === "admin";
}That drops the One consequence I want to name rather than leave implicit: an admin will be able to permanently delete a colleague's gallery that was shared with them. I'm fine with that — a collaboration invite inside one studio isn't an arm's-length relationship, and archived-only plus the typed title are already two brakes. But it's a real widening compared to your version, and you've been closer to this code than I have this week. If you think the stricter rule is the right one, make the case and I'll listen. #24's acceptance criteria carry my original assumption, so those two bullets want updating too — otherwise the issue and the code will disagree once this lands. I'll fix the issue text. On the case-insensitive title match: agreed, leave it. Matching |
- Custom slug: owner-or-admin instead of owner-only, matching the role rule settled in markusthiel#25 (galleryAccessWhere already scopes what an admin can reach, so "not a member" is the right check here too). Renames the 403 code from gallery_slug_owner_only to gallery_slug_forbidden since the old name became inaccurate. - Catches the P2002 race between the availability check and the update (two concurrent claims on the same slug) and maps it to the existing gallery_slug_taken response instead of a 500, same pattern as the webhook-event dedup in routes/billing.ts. - GallerySlugEditor shows a second warning line when the gallery is public and passwordless, since the slug is the access control in that case and a readable one is easier to guess. - Documents the guessability tradeoff in MULTI_TENANT.md (EN/de/it). Tenant-scoping the slug namespace (markusthiel#30) is intentionally out of scope here — it's a prerequisite this PR will rebase onto once it lands.
|
Picking this back up — I think it just needs the role change and it's done. You agreed over on #29 that the slug shouldn't be stricter than deletion, and implemented owner-or-admin there in Same shape as what you ended up with on #29: since export function canDeleteGallery(s: SessionContext): boolean {
return s.user.role === "owner" || s.user.role === "admin";
}That plus the call site in the list route, and the two acceptance-criteria bullets in #24 — I said I'd fix those and haven't yet, so I'll do that rather than leave it on you. Everything else here I'm happy with as it stands. |
|
Agreed on the shape — thanks for picking up the role-change and the two Nothing else from me on this one — ready to merge whenever that commit is in. |
Summary
existing (previously unused)
api.deleteGalleryclient and the existingDELETE /galleries/:idroute.server-side (409
gallery_not_archived) — the API is also used outside thisUI, so the guard lives in the API itself.
(409
gallery_has_print_orders) —PrintOrderItemrows referenceFileviaa
RestrictFK, so this also prevents an unfriendly DB constraint error.canDeleteGalleryingallery-access.ts, deliberately stricter than the general access model):Admin → any visible gallery, Owner → only their own, Member → never. Exposed
to the frontend as a
canDeletefield on each gallery, so the UI nevercomputes permission logic itself.
usePrompt()primitive.apiError.*i18n keys for the two new error cases.Closes #24.
Test plan
npm run type-checkandnpm run check:i18ninapps/frontendnpm run testinapps/api(9 new tests covering the permission +guard-order logic, 200/200 total passing)
confirm it disappears and an audit log entry / cleanup job appear
Note:
npm run lint(next lint) currently fails on this repo's Next.js 16regardless of branch — pre-existing tooling issue, not introduced here.