No way to exclude a scan that is not a photograph #106
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Severity: MEDIUM
The problem
docs/circa-spec.md§6.4 gives every entity adeleted_at, but thePhotomodel has none andthere is no way to exclude anything from review. Only comments soft-delete.
Real scan batches contain blank pages, envelope scans, album-cover shots, accidental duplicates of
the scanner lid, and outright junk. A reviewer hits "this isn't a photo" within the first hour and
can currently do nothing but leave it
pendingforever — where it inflates every count, cluttersevery queue, and never goes away.
Scope
deleted_at(or an explicitexcludedstatus with a reason) toPhoto.Nothing is ever hard-deleted — consistent with §6.4.
Done when
References
backend/app/models/models.py(Photo)docs/circa-spec.md§6.4Done in
e9eef6a, CI green (run 64).Soft delete, not a fifth status, and the measurement made that decision look better than it did on paper.
PhotoStatusis computed from decision history byservices/projections.fold, whose contract is that it is derivable from that history; anexcludedmember would put a non-derived value inside the one function that may not hold one — #79 exists because status was decided in three places. It would also overwrite the review state, so restoring a misclassified exclusion could not put it back. The end-to-end test approves a photograph, excludes it, restores it and asserts it comes back approved, against a real database.The database and the wire say
deleted(spec §6.4); the interface says excluded. That is load-bearing copy, not fussiness: someone who reads "delete" beside an irreplaceable scan will not press it, and then the queue still never empties.The reason is picked, and picking it is the action — two interactions, on the thing a reviewer does most in the first hour of a batch. Only "Other" asks for words, because "Other" alone answers nothing a year later.
Two measured results that inverted part of the design
50,000 rows, production-like proportions, best of five.
A plain index on
(collection_id, deleted_at, id)for the excluded view is a catastrophe. SQLite readsdeleted_at IS NULLas an equality constraint on the second column, judges that index most selective, abandonsix_photo_collection_createdand sorts 48,000 rows in a temp B-tree. Every live listing goes 0.06 ms → ~250 ms, 3,800x, with the answers still correct — so nothing above the query plan would report it. #84's finding and migration 012's, a third time: a regression caused by adding an index. Making it partial removes it from consideration.And that index is
(collection_id, created_at, id), not thedeleted_atshape it reads as though it should have. The excluded queue islist_photoslike any other listing and is ordered by(created_at, id), so an index leading with the predicate seeks and then sorts: 5.357 ms vs 0.062 ms. 86x, and the same lessondate_sorttaught — the index must carry the order the query asks for, not the predicate it filters on.With those settled, the five partial listing indexes earn their keep on the dashboard, not the list: identical listing times either way at 4% excluded, but
/photos/statsgoes 47.3 ms → 9.4 ms, because a plain index makes SQLite fetch all 50,000 rows to read adeleted_atit does not carry.Two things found on the way
A partial index is only usable when the query carries the predicate, which makes "the repository forgot
deleted_at IS NULL" a 250 ms regression rather than merely a wrong answer — andtest_query_plans.pycould not have caught it, because by convention it explains hand-written SQL. It now compiles and explains the querylist_photosactually sends.op.add_columnwith aForeignKeycannot be used at all: on SQLite it raisesNotImplementedErrorafter adding the column, leaving a failed migration with the column and no constraint.batch_alter_tablewas not the answer either — SQLAlchemy's SQLite reflection skips partial indexes, so rebuildingphotowould silently dropuq_photo_canonical_sha256(#91) along with the five new ones. Plain DDL with a real FK, confirmed byPRAGMA foreign_key_listand a refused insert.Scope notes
list_photos(excluded=False)is the default, so export inherits the exclusion when it is built rather than needing to remember it.One deliberate deviation from spec §6.4, which says deleted records are "recoverable by an admin": both exclude and restore use the reviewer floor. Nothing is destroyed, exclusion is the fast path used dozens of times in the first hour of a batch, and requiring an admin to undo a misclick on a scanner-lid scan would make the excluded queue unusable by the people who work it. A viewer still cannot reach either. Say if you would rather restore were admin-only — it is a one-line change.
One known cost, not paid: reading the excluded queue in date order is unindexed and sorts ~2,000 rows at about 5.5 ms. A sixth partial index would fix it and would cost writes only for the 4% of rows in it; migration 012 declined that trade for filter-only shapes, so this follows suit.
Backend tests 1206 → 1282, frontend 190 → 202, end-to-end 10 → 11.