feat(photos): fix trash permanent delete and add empty trash
Permanently deleting a photo whose album was deleted failed because authorization resolved through the album. Fall back to uploader ownership when the photo is albumless. Add a bulk DELETE /trash that removes every trashed photo the caller may delete (uncapped listing) and returns the deleted count, with an «Очистить корзину» action on the trash screen.
This commit is contained in:
@@ -0,0 +1,2 @@
|
||||
schema: spec-driven
|
||||
created: 2026-10-11
|
||||
@@ -0,0 +1,94 @@
|
||||
# Design
|
||||
|
||||
## Context
|
||||
|
||||
See `proposal.md` - Why. Relevant current state:
|
||||
|
||||
- `photos.Service.PermanentDelete` (backend/internal/photos/service.go:592) resolves
|
||||
authorization via `albums.Access(albumID, userID)` before `HardDelete`. A photo
|
||||
whose album was deleted has `album_id = NULL`; `Access("")` returns
|
||||
`album_not_found`, so the delete can never proceed. `Restore` already special-cases
|
||||
the missing album (`album_required`), but `PermanentDelete` does not.
|
||||
- The trash listing (`photos.Store.ListTrash`) is capped at `LIMIT 200` for display,
|
||||
while `TrashCount` is uncapped. A bulk purge must not silently stop at 200.
|
||||
- Object cleanup after `HardDelete` is per-photo: `PhotoObjectIDs` then
|
||||
`MarkObjectDeleting` for orphans, with a background worker removing blobs.
|
||||
- The trash screen (frontend/src/routes/(app)/trash/+page.svelte) has per-photo
|
||||
actions only, and `ConfirmDialog` is already available for confirmations.
|
||||
|
||||
## Goals / Non-Goals
|
||||
|
||||
**Goals:**
|
||||
- Permanently deleting an albumless trashed photo works for its uploader.
|
||||
- One action empties the trash for everything the caller may delete, regardless of the
|
||||
200-item display cap, and reports the number removed.
|
||||
- Consistent object garbage collection for every removed photo.
|
||||
|
||||
**Non-Goals:**
|
||||
- Trash retention/auto-expiry policies.
|
||||
- Changing trash visibility rules (`ListTrash`) or the 200-item display cap.
|
||||
- Set-based SQL bulk deletion in this change.
|
||||
|
||||
## Decisions
|
||||
|
||||
### Authorize albumless permanent deletion by uploader
|
||||
When `p.AlbumID == ""`, `PermanentDelete` SHALL fall back to `p.UploaderID == userID`
|
||||
for authorization instead of calling `albums.Access`. When an album is present, the
|
||||
existing album-role check is unchanged. This mirrors `Restore`, which already treats a
|
||||
missing album as a distinct path, and keeps the permission model identical: only a
|
||||
photo's uploader can remove a photo that no longer belongs to an album.
|
||||
|
||||
- Alternative considered: require the client to always supply a target album for
|
||||
delete. Rejected - permanent deletion has no target album, and the trash legitimately
|
||||
holds albumless photos.
|
||||
|
||||
### Empty trash as a server-side operation on the trash resource
|
||||
Add `DELETE /trash` returning `{ "deleted": <n> }`, backed by
|
||||
`photos.Service.EmptyTrash(ctx, userID)`. It iterates the caller's accessible trashed
|
||||
photos and calls the existing `PermanentDelete` for each, counting successes and
|
||||
skipping photos the caller may not delete. Reusing `PermanentDelete` keeps authorization
|
||||
and object GC identical to single-photo deletion.
|
||||
|
||||
- Alternative considered: client enumerates trash and calls `POST /photo-batch/...`
|
||||
with every id. Rejected - the display list is capped at 200 and this is racy; the
|
||||
server already knows the full accessible set.
|
||||
- Alternative considered: a single `DELETE FROM photos ... WHERE <authorization>`. Rejected
|
||||
for now - album-role authorization lives in Go (`roles.CanDelete*`), and per-photo object
|
||||
GC is required anyway.
|
||||
|
||||
### Bypass the 200-item display cap for purging
|
||||
`EmptyTrash` must see more than the display page. Add a store method that returns all
|
||||
accessible trashed photo identities (id, album_id, uploader_id) with no display cap, and
|
||||
authorize each in the service. This avoids looping on a capped list (which would also
|
||||
risk an infinite loop when some items are not deletable).
|
||||
|
||||
- Alternative considered: paginate `ListTrash` until a pass removes nothing. Rejected -
|
||||
undeletable items would reappear and a zero-progress pass is needed to stop; the
|
||||
uncapped identity listing is simpler and bounded by the user's own trash size.
|
||||
|
||||
### Frontend: explicit action plus confirmation
|
||||
Add an «Очистить корзину» control on the trash screen header, enabled when the list is
|
||||
non-empty. It opens the existing `ConfirmDialog`; on confirm it calls `DELETE /trash`,
|
||||
then reloads the list and refreshes `trash` store count. Show a busy state while running.
|
||||
|
||||
## Risks / Trade-offs
|
||||
|
||||
- [Large trash makes the per-photo loop slow] -> Each `PermanentDelete` is a small fixed
|
||||
number of queries; bulk deletes are O(n) round trips. Acceptable for now; a set-based
|
||||
purge can replace the loop later without changing the spec behavior. Keep the frontend
|
||||
busy state to make latency legible.
|
||||
- [Uncapped listing memory] -> The query returns only the caller's trashed photo
|
||||
identities; bounded in practice by trash size. Revisit with chunking if it grows.
|
||||
- [Partial failure mid-purge] -> `EmptyTrash` skips failures and returns the count
|
||||
deleted; the remaining items stay in the trash and can be retried.
|
||||
- [Albumless deletes and realtime] -> `PermanentDelete` only publishes when `AlbumID != ""`;
|
||||
albumless removals publish nothing, which is correct since no album is watching.
|
||||
|
||||
## Migration Plan
|
||||
|
||||
No schema change. Deploy backend and frontend together; the new endpoint is additive and
|
||||
the authorizaton fix is behavior-only. Rollback is reverting the deploy.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None blocking.
|
||||
@@ -0,0 +1,45 @@
|
||||
# Proposal
|
||||
|
||||
## Why
|
||||
|
||||
Permanently deleting a photo from the trash fails when the photo has no album
|
||||
(`album_id IS NULL`) — the normal result of deleting an album, whose photos move
|
||||
to the trash unbound (`photos/spec.md`, «Удаление альбома»). `PermanentDelete`
|
||||
authorizes through the album, so these photos can never be removed, and the UI
|
||||
surfaces `album_not_found`. In the local database 4010 of 4011 trashed photos are
|
||||
in this state. There is also no way to clear the trash in bulk.
|
||||
|
||||
## What Changes
|
||||
|
||||
- Fix `PermanentDelete` authorization so a photo without an album can be
|
||||
permanently deleted by a user who may delete it (its uploader), instead of
|
||||
failing on a missing album.
|
||||
- Add a bulk permanent-delete operation that empties the trash for the caller,
|
||||
skipping photos the caller is not allowed to remove, and reporting how many
|
||||
were deleted.
|
||||
- Add an «Очистить корзину» action on the trash screen with a confirmation
|
||||
dialog, refreshing the list and the rail counter afterwards.
|
||||
|
||||
## Capabilities
|
||||
|
||||
### New Capabilities
|
||||
|
||||
<!-- none: this extends existing photo lifecycle and workspace behavior -->
|
||||
|
||||
### Modified Capabilities
|
||||
|
||||
- `photos`: «Окончательное удаление» also covers photos with no album
|
||||
(deleted album); add an «Очистка корзины» requirement for bulk permanent
|
||||
removal.
|
||||
- `ui-workspace`: trash screen gains an «Очистить корзину» control with
|
||||
confirmation and post-action refresh.
|
||||
|
||||
## Impact
|
||||
|
||||
- Backend: `backend/internal/photos/service.go` (`PermanentDelete`), the photos
|
||||
repository/store (new bulk purge method), `handlers.go`, and a route in
|
||||
`backend/internal/app/app.go`.
|
||||
- Frontend: `frontend/src/lib/api.ts`, `frontend/src/lib/stores/trash.svelte.ts`,
|
||||
`frontend/src/routes/(app)/trash/+page.svelte`.
|
||||
- Tests: e2e/contract coverage for permanently deleting an albumless photo and
|
||||
for the bulk empty-trash path.
|
||||
@@ -0,0 +1,48 @@
|
||||
# Spec Delta
|
||||
|
||||
## MODIFIED Requirements
|
||||
|
||||
### Requirement: Корзина фото
|
||||
Удаление фото SHALL помещать его в корзину (мягкое удаление). Восстановление SHALL возвращать
|
||||
фото в его альбом. Окончательное удаление SHALL удалять запись, а объекты хранилища SHALL
|
||||
удаляться только когда на них не остаётся ссылок ни в одном альбоме. Окончательное удаление
|
||||
SHALL быть доступно и для фото без альбома (например, после удаления альбома).
|
||||
|
||||
#### Scenario: Удаление в корзину
|
||||
- **WHEN** пользователь удаляет фото
|
||||
- **THEN** фото пропадает из альбома и появляется в корзине
|
||||
|
||||
#### Scenario: Восстановление
|
||||
- **WHEN** пользователь восстанавливает фото из корзины
|
||||
- **THEN** фото возвращается в свой альбом
|
||||
|
||||
#### Scenario: Окончательное удаление без ссылок
|
||||
- **WHEN** пользователь удаляет фото из корзины навсегда, и на те же объекты нет ссылок в других альбомах
|
||||
- **THEN** запись и объекты хранилища удаляются
|
||||
|
||||
#### Scenario: Окончательное удаление при наличии ссылок
|
||||
- **WHEN** на объект хранилища ссылается копия в другом альбоме
|
||||
- **THEN** запись удаляется, а объект хранилища сохраняется
|
||||
|
||||
#### Scenario: Окончательное удаление фото без альбома
|
||||
- **WHEN** пользователь удаляет навсегда фото из корзины, у которого нет альбома
|
||||
- **THEN** запись удаляется, а операция не завершается ошибкой отсутствующего альбома
|
||||
|
||||
## ADDED Requirements
|
||||
|
||||
### Requirement: Очистка корзины
|
||||
Пользователь SHALL иметь возможность окончательно удалить все элементы корзины одним действием.
|
||||
Система SHALL удалять только те элементы, которые пользователь вправе удалить навсегда, и SHALL
|
||||
пропускать остальные. Операция SHALL возвращать число удалённых элементов.
|
||||
|
||||
#### Scenario: Очистка корзины
|
||||
- **WHEN** пользователь очищает корзину
|
||||
- **THEN** все элементы, которые он вправе удалить навсегда, удаляются
|
||||
|
||||
#### Scenario: Чужие элементы
|
||||
- **WHEN** в корзине есть фото, которые пользователь не вправе удалить навсегда
|
||||
- **THEN** они остаются в корзине, а остальные удаляются
|
||||
|
||||
#### Scenario: Пустая корзина
|
||||
- **WHEN** пользователь очищает пустую корзину
|
||||
- **THEN** операция завершается успешно, удалив ноль элементов
|
||||
@@ -0,0 +1,20 @@
|
||||
# Spec Delta
|
||||
|
||||
## ADDED Requirements
|
||||
|
||||
### Requirement: Очистка корзины в интерфейсе
|
||||
Экран корзины SHALL содержать действие «Очистить корзину», доступное, когда в корзине есть
|
||||
элементы. Действие SHALL открывать подтверждение и после подтверждения очищать корзину, а
|
||||
список и счётчик корзины в рейле SHALL отражать результат.
|
||||
|
||||
#### Scenario: Наличие действия
|
||||
- **WHEN** пользователь открывает корзину с элементами
|
||||
- **THEN** на экране показано действие «Очистить корзину»
|
||||
|
||||
#### Scenario: Подтверждение очистки
|
||||
- **WHEN** пользователь выбирает «Очистить корзину»
|
||||
- **THEN** открывается диалог подтверждения, и только после подтверждения корзина очищается
|
||||
|
||||
#### Scenario: Обновление после очистки
|
||||
- **WHEN** корзина очищена
|
||||
- **THEN** на экране показывается пустая корзина, а счётчик корзины в рейле обновляется
|
||||
@@ -0,0 +1,28 @@
|
||||
# Tasks
|
||||
|
||||
## 1. Fix permanent delete for albumless photos
|
||||
|
||||
- [x] 1.1 In `photos.Service.PermanentDelete`, when `p.AlbumID == ""`, authorize by `p.UploaderID == userID` instead of calling `albums.Access`; keep the album-role check when an album is present. Verify with a Go test that permanently deletes an albumless trashed photo (add to `backend/internal/e2e/sqlite_test.go`) and passes.
|
||||
- [x] 1.2 Confirm object garbage collection still runs for albumless deletes (`PhotoObjectIDs` + `MarkObjectDeleting`); verify the test asserts the photo row is gone and an unbound object is marked deleting.
|
||||
|
||||
## 2. Add empty-trash operation
|
||||
|
||||
- [x] 2.1 Add a photos repository/store method that lists all accessible trashed photo identities (`id`, `album_id`, `uploader_id`) without the 200-item display cap; verify with a test that seeds more than 200 trashed photos and asserts all are returned.
|
||||
- [x] 2.2 Add `Service.EmptyTrash(ctx, userID)` that authorizes each listed photo with the same rules as `PermanentDelete` (album role when bound, uploader when albumless), skips forbidden ones, and returns the deleted count; verify with a test where a shared album holds another user's trashed photo — the foreign photo remains, the caller's are removed, and the count matches.
|
||||
- [x] 2.3 Add `Handler.EmptyTrash` and the `DELETE /trash` route returning `{ "deleted": n }`; verify with an HTTP-level test asserting the status code and response body.
|
||||
- [x] 2.4 Add end-to-end coverage: emptying an empty trash returns `0`, and after emptying a non-empty trash `TrashCount` is `0`; verify with `make test`.
|
||||
|
||||
## 3. Trash screen action
|
||||
|
||||
- [x] 3.1 Add `photosApi.emptyTrash()` calling `DELETE /trash` in `frontend/src/lib/api.ts`; verify with `npm run check`.
|
||||
- [x] 3.2 Add an «Очистить корзину» control to `frontend/src/routes/(app)/trash/+page.svelte`, enabled only when the list is non-empty, opening the existing `ConfirmDialog`; on confirm call `emptyTrash`, then reload the list and call `trash.load()`; verify by clearing a non-empty trash and observing the empty state and a busy state during the call.
|
||||
- [x] 3.3 Verify the rail counter reflects the result: after clearing, the trash count in `AlbumRail` becomes `0` (observable in the running app).
|
||||
|
||||
## 4. Integration verification
|
||||
|
||||
- [x] 4.1 Run `make test` (backend) and `npm run check` plus `npm run build` (frontend); verify all pass with no regressions.
|
||||
|
||||
## Workflow follow-up
|
||||
|
||||
- Run `/opsx-apply` to implement this change.
|
||||
- Archive the change after review and verification (sync delta specs into main specs).
|
||||
Reference in New Issue
Block a user