Vestigial reviewer_notes field: rename, deduplicate, or add real UI #85

Closed
opened 2026-08-13 08:36:08 +00:00 by agent-pi · 1 comment
Collaborator

reviewer_notes exists on both posts and review_feedback, described as "notes from human reviewer" — but there has never been a review-UI input for it. The only thing that writes it is the auto-reject system (post.reviewer_notes = "[auto-rejected] {reason}") and the REST API (PATCH /posts/{id}), and nothing in the web UI ever reads or displays it.

PR #81 removes the one place it had any UI: the "Set Manual Spam Score" modal's "Notes (optional)" textarea. After that, the field is effectively just an auto-reject annotation plus an API field — the "human reviewer notes" concept doesn't actually exist anywhere user-facing.

Decision to make (pick one):

  1. Rename to reflect reality — e.g. auto_reject_reason on posts, scoped to the auto-reject path only, and stop pretending there's a human-notes feature.
  2. Deduplicate — the value is stored redundantly on both posts.reviewer_notes and review_feedback.reviewer_notes; keep it in one place only.
  3. Actually wire up a notes input in the review UI if leaving a note when approving/rejecting is wanted going forward (currently impossible).

Context: the field today is mostly [auto-rejected] … strings produced by reject_blocked_posts, plus whatever the API sets.

`reviewer_notes` exists on both `posts` and `review_feedback`, described as "notes from human reviewer" — but there has never been a review-UI input for it. The only thing that writes it is the auto-reject system (`post.reviewer_notes = "[auto-rejected] {reason}"`) and the REST API (`PATCH /posts/{id}`), and nothing in the web UI ever reads or displays it. PR #81 removes the one place it had any UI: the "Set Manual Spam Score" modal's "Notes (optional)" textarea. After that, the field is effectively just an auto-reject annotation plus an API field — the "human reviewer notes" concept doesn't actually exist anywhere user-facing. Decision to make (pick one): 1. **Rename to reflect reality** — e.g. `auto_reject_reason` on `posts`, scoped to the auto-reject path only, and stop pretending there's a human-notes feature. 2. **Deduplicate** — the value is stored redundantly on both `posts.reviewer_notes` and `review_feedback.reviewer_notes`; keep it in one place only. 3. **Actually wire up a notes input** in the review UI if leaving a note when approving/rejecting is wanted going forward (currently impossible). Context: the field today is mostly `[auto-rejected] …` strings produced by `reject_blocked_posts`, plus whatever the API sets.
Author
Collaborator

Findings from exploring reviewer_notes:

  • posts.reviewer_notes is written in exactly one place — reject_blocked_posts sets "[auto-rejected] {reason}". It is read only by PostResponse (the API); the web review UI never reads it (it recomputes blocked reasons from the filters, not from this field).
  • review_feedback.reviewer_notes is written by create_review_feedback (auto-reject copies the post note; PATCH /posts/{id} passes a note only when approved is also present). Nothing in the codebase reads it — ReviewFeedback is itself a write-only audit table.
  • PostUpdate.reviewer_notes (the PATCH input) is described as "Notes from human reviewer" but only lands on review_feedback, which nobody reads.

Recommendation — combine options 1 + 2 (both apply):

  1. Rename posts.reviewer_notesposts.auto_reject_reason (its only writer is auto-reject).
  2. Drop review_feedback.reviewer_notes entirely (redundant duplicate, never read).
  3. Remove reviewer_notes from PostUpdate; expose auto_reject_reason on PostResponse.
  4. create_review_feedback loses its reviewer_notes parameter.
  5. One alembic migration (rename column + drop column); update tests and docs.

Plan of attack: models.pyservices/review.py + services/export_eligibility.pyschemas.pyapi/__init__.py → migration → tests → docs.

Branch: feat/issue-85-auto-reject-reason, PR into feat/issue-80-collect with Refs #85.

(The stale templates/review.html TODO is a separate follow-up, not part of this change.)

Findings from exploring `reviewer_notes`: - `posts.reviewer_notes` is written in exactly one place — `reject_blocked_posts` sets `"[auto-rejected] {reason}"`. It is read only by `PostResponse` (the API); the web review UI never reads it (it recomputes blocked reasons from the filters, not from this field). - `review_feedback.reviewer_notes` is written by `create_review_feedback` (auto-reject copies the post note; `PATCH /posts/{id}` passes a note only when `approved` is also present). Nothing in the codebase reads it — `ReviewFeedback` is itself a write-only audit table. - `PostUpdate.reviewer_notes` (the PATCH input) is described as "Notes from human reviewer" but only lands on `review_feedback`, which nobody reads. Recommendation — combine options 1 + 2 (both apply): 1. Rename `posts.reviewer_notes` → `posts.auto_reject_reason` (its only writer is auto-reject). 2. Drop `review_feedback.reviewer_notes` entirely (redundant duplicate, never read). 3. Remove `reviewer_notes` from `PostUpdate`; expose `auto_reject_reason` on `PostResponse`. 4. `create_review_feedback` loses its `reviewer_notes` parameter. 5. One alembic migration (rename column + drop column); update tests and docs. Plan of attack: `models.py` → `services/review.py` + `services/export_eligibility.py` → `schemas.py` → `api/__init__.py` → migration → tests → docs. Branch: `feat/issue-85-auto-reject-reason`, PR into `feat/issue-80-collect` with `Refs #85`. (The stale `templates/review.html` TODO is a separate follow-up, not part of this change.)
Sign in to join this conversation.
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
marvin8/dujiangyan#85
No description provided.