skip decision overwrites the projection it should leave alone #81

New issue

Closed

opened 2026-07-28 06:00:28 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 06:00:28 +00:00

Severity: MEDIUM

The bug

PhotoRepository.apply_decision_projection (backend/app/repositories/photos.py:80-101)
unconditionally sets current_decision_id and overwrites all three current_effective_date_*
fields for every decision type. Only status is spared for a skip.

So a skip becomes the photo's "current decision," and if the reviewer's form fields were empty or
cleared, the photo's effective date is nulled by the act of declining to decide.

This is currently harmless only because the frontend pre-fills the form from the current effective
values, making the overwrite a no-op — correctness by coincidence.

Decision

Skip writes a history row and touches nothing else.

The alternatives considered were: a pure client-side advance with no record at all (fast, but
loses "someone looked at this and moved on," which matters once several people share a queue), and
the current behaviour. The chosen option keeps attribution without side effects.

Scope

Early-return in apply_decision_projection before the projection writes when the decision is a
skip.
The DateDecision row is still created, with actor and timestamp, so the audit trail records it.
current_decision_id is not updated by a skip.
Define this explicitly in the projection rebuild fold (companion issue), since rebuild must agree.
Once queue navigation exists, consider whether skip should be a server round-trip at all — but
keep the history row for now.

Done when

A skip creates a history row and leaves status, current decision, and effective dates untouched
The rebuild fold agrees with the live path on skip semantics
A test asserts skipping a photo with an empty form does not clear its date

References

backend/app/repositories/photos.py:80-101
backend/app/models/models.py (DecisionType.skip)

Related: #9 (concurrency tests), and the projection rebuild issue.

## Severity: MEDIUM ## The bug `PhotoRepository.apply_decision_projection` (`backend/app/repositories/photos.py:80-101`) unconditionally sets `current_decision_id` and overwrites all three `current_effective_date_*` fields for **every** decision type. Only `status` is spared for a skip. So a skip becomes the photo's "current decision," and if the reviewer's form fields were empty or cleared, the photo's effective date is **nulled by the act of declining to decide**. This is currently harmless only because the frontend pre-fills the form from the current effective values, making the overwrite a no-op — correctness by coincidence. ## Decision **Skip writes a history row and touches nothing else.** The alternatives considered were: a pure client-side advance with no record at all (fast, but loses "someone looked at this and moved on," which matters once several people share a queue), and the current behaviour. The chosen option keeps attribution without side effects. ## Scope - Early-return in `apply_decision_projection` before the projection writes when the decision is a skip. - The `DateDecision` row is still created, with actor and timestamp, so the audit trail records it. - `current_decision_id` is not updated by a skip. - Define this explicitly in the projection rebuild fold (companion issue), since rebuild must agree. - Once queue navigation exists, consider whether skip should be a server round-trip at all — but keep the history row for now. ## Done when - [ ] A skip creates a history row and leaves status, current decision, and effective dates untouched - [ ] The rebuild fold agrees with the live path on skip semantics - [ ] A test asserts skipping a photo with an empty form does not clear its date ## References - `backend/app/repositories/photos.py:80-101` - `backend/app/models/models.py` (`DecisionType.skip`) Related: #9 (concurrency tests), and the projection rebuild issue.

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:28 +00:00

claude-bot added the

bug

backend

labels

2026-07-28 06:00:28 +00:00

claude-bot referenced this issue

2026-07-28 06:07:54 +00:00

Add repository and projection-service tests #6

rbrooks referenced this issue from a commit

2026-08-01 05:31:06 +00:00

Make the review_version guard actually prevent overwrites

rbrooks closed this issue

2026-08-01 05:31:06 +00:00

claude-bot commented

2026-08-01 05:53:24 +00:00

Author

Done in 03dbc33, alongside #77 since both live in the decision/projection path.

apply_decision_projection now returns early for a skip. The DateDecision row is still written by the caller, so "someone looked at this and moved on" survives with its actor and timestamp — which is the property that matters once several people share a review queue.

Skip also does not advance review_version. The issue didn't specify this and it's worth flagging: no reviewable state changed, so invalidating another reviewer's in-flight edit would be gratuitous. It also makes two consecutive skips both succeed, which is how a queue actually gets used. There are tests for both.

Your note that the current behaviour was "harmless only because the frontend pre-fills the form" was exactly right, and it's why this was worth fixing before real data exists rather than after — the day someone builds a script or a keyboard shortcut that posts a skip without pre-filling, it silently nulls settled dates.

Tests assert a skip leaves the effective dates, current_decision_id, status and version untouched, that it still appears in the decision history attributed to the reviewer, and — separately — that approve/dispute/needs_review still project correctly, since an early return is the kind of change that can quietly disable its neighbours.

The rebuild fold (#79) will need to agree with this; I'll make it explicit when I build it, as this issue asks.

(Edited: this comment originally pointed at #6 for the rebuild fold. The correct issue is #79 — #6 is the projection-service test suite that consumes #79's oracle.)

Done in 03dbc33, alongside #77 since both live in the decision/projection path. `apply_decision_projection` now returns early for a skip. The `DateDecision` row is still written by the caller, so "someone looked at this and moved on" survives with its actor and timestamp — which is the property that matters once several people share a review queue. **Skip also does not advance `review_version`.** The issue didn't specify this and it's worth flagging: no reviewable state changed, so invalidating another reviewer's in-flight edit would be gratuitous. It also makes two consecutive skips both succeed, which is how a queue actually gets used. There are tests for both. Your note that the current behaviour was "harmless only because the frontend pre-fills the form" was exactly right, and it's why this was worth fixing before real data exists rather than after — the day someone builds a script or a keyboard shortcut that posts a skip without pre-filling, it silently nulls settled dates. Tests assert a skip leaves the effective dates, `current_decision_id`, status and version untouched, that it still appears in the decision history attributed to the reviewer, and — separately — that `approve`/`dispute`/`needs_review` still project correctly, since an early return is the kind of change that can quietly disable its neighbours. The rebuild fold (#79) will need to agree with this; I'll make it explicit when I build it, as this issue asks. *(Edited: this comment originally pointed at #6 for the rebuild fold. The correct issue is #79 — #6 is the projection-service test suite that consumes #79's oracle.)*

claude-bot referenced this issue

2026-08-02 05:48:56 +00:00

Add repository and projection-service tests #6