diff --git a/CLAUDE.md b/CLAUDE.md
index 29e5ae0c..f328b0ec 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -177,7 +177,13 @@ restores the image but never the schema, so keep migrations additive.
lacks is sent from `/cameras/report#new` as its maker, its marking and
photos (`report_proposals`, migration 028), and publishing it from the
review queue adds the board (`boards.CreateModel`, alias source `club`)
- -- the only way a model is made besides the importers. A report sent from
+ -- the only way a model is made besides the importers. A member may edit
+ their own report from `/club` (`reports.Store.Edit`, migration 031): the
+ note, the camera proposed, files out and in; ipctool's output and a backup
+ stay as sent. Like takedown it stands the guard down for its transaction,
+ and every change is logged in append-only `report_edits`. An edit to a
+ report already decided adds a review row `edit`: pending again, off the
+ board until a maintainer accepts it once more, the stars settled then. A report sent from
the camera is a member's when it carries a one-time code from `/club`
(`report_codes`, migration 029), in the note because every ipctool in the
field can send one: `ipctool upload --note club-XXXX-XXXX`. A code may
diff --git a/data/locales/boards.en.yml b/data/locales/boards.en.yml
index 084b4ae5..8ac9ea28 100644
--- a/data/locales/boards.en.yml
+++ b/data/locales/boards.en.yml
@@ -557,6 +557,15 @@ en:
rename_save: 'Save'
review_note_label: 'Reviewer'
your_note: 'Your note'
+ edit: 'Edit'
+ edit_save: 'Save'
+ edit_save_rereview: 'Save and send for review again'
+ edit_cancel: 'Cancel'
+ edit_rereview: 'A maintainer has already decided on this report. Once you save, it goes back to them, and it is off the catalogue until they accept it again.'
+ edit_remove: 'Take out'
+ edit_add: 'Add'
+ edited_at: 'Edited {time}'
+ review_edited_label: 'Edited by the sender'
cameras_title: 'My cameras on the Open Wall'
cameras_count: '{n} of {max} count towards stars'
cameras_empty: 'No camera linked yet. Link one below: it earns stars for every month it keeps sending real pictures to the Open Wall.'
diff --git a/data/locales/boards.ru.yml b/data/locales/boards.ru.yml
index 3bf8863b..e9d6fc33 100644
--- a/data/locales/boards.ru.yml
+++ b/data/locales/boards.ru.yml
@@ -598,6 +598,15 @@ ru:
rename_save: 'Сохранить'
review_note_label: 'Проверяющий'
your_note: 'Ваше примечание'
+ edit: 'Изменить'
+ edit_save: 'Сохранить'
+ edit_save_rereview: 'Сохранить и отправить на повторную проверку'
+ edit_cancel: 'Отмена'
+ edit_rereview: 'Мейнтейнер уже принял решение по этому отчёту. После сохранения он вернётся на проверку и пропадёт из каталога, пока его не примут снова.'
+ edit_remove: 'Убрать'
+ edit_add: 'Добавить'
+ edited_at: 'Изменён {time}'
+ review_edited_label: 'Изменён отправителем'
cameras_title: 'Мои камеры на Open Wall'
cameras_count: 'Звёзды приносят {n} из {max}'
cameras_empty: 'Пока ни одной камеры. Привяжите камеру ниже: она будет приносить звёзды за каждый месяц, в который присылает на Open Wall настоящие снимки.'
diff --git a/data/locales/boards.zh.yml b/data/locales/boards.zh.yml
index d15bc690..79f0e304 100644
--- a/data/locales/boards.zh.yml
+++ b/data/locales/boards.zh.yml
@@ -552,6 +552,15 @@ zh:
rename_save: '保存'
review_note_label: '审核者'
your_note: '你的备注'
+ edit: '编辑'
+ edit_save: '保存'
+ edit_save_rereview: '保存并重新提交审核'
+ edit_cancel: '取消'
+ edit_rereview: '维护者已经审核过这份报告。保存后它会重新进入审核,在再次通过之前不会出现在目录中。'
+ edit_remove: '移除'
+ edit_add: '添加'
+ edited_at: '编辑于 {time}'
+ review_edited_label: '发送者已编辑'
cameras_title: '我在 Open Wall 上的摄像头'
cameras_count: '{n} / {max} 台计入星星'
cameras_empty: '还没有绑定摄像头。在下方绑定一台:它每持续一个月向 Open Wall 发送真实画面,就会获得星星。'
diff --git a/frontend/apps/site/src/components/club/Club.tsx b/frontend/apps/site/src/components/club/Club.tsx
index f7be1762..8339269c 100644
--- a/frontend/apps/site/src/components/club/Club.tsx
+++ b/frontend/apps/site/src/components/club/Club.tsx
@@ -19,6 +19,7 @@ import { size } from '../../lib/reports';
import Cameras from './Cameras';
import Crashes from './Crashes';
import IpctoolCode from './IpctoolCode';
+import EditReport from './EditReport';
import { STATUS_TONE, Stars } from './parts';
type Load = { state: 'loading' } | { state: 'ok'; me: Me } | { state: 'error' };
@@ -244,9 +245,8 @@ function MemberPage({ member, ways, locale, t, onChange }: {
// asks /me on every page and needs only the total.
const pending = (reports ?? []).reduce((n, r) => n + (r.status === 'pending' ? r.pending : 0), 0);
- useEffect(() => {
- fetchMine().then((r) => setReports(r.reports)).catch(() => setError(true));
- }, [member.id]);
+ const loadReports = () => fetchMine().then((r) => setReports(r.reports)).catch(() => setError(true));
+ useEffect(() => { void loadReports(); }, [member.id]);
const loadCams = () => fetchCameras().then(setCams).catch(() => setError(true));
useEffect(() => { void loadCams(); }, [member.id]);
// A link to #cameras (the bot's "My cameras") lands on the section once it exists.
@@ -339,14 +339,15 @@ function MemberPage({ member, ways, locale, t, onChange }: {
{reports && reports.length === 0 && (
diff --git a/frontend/apps/site/src/i18n/boards.en.json b/frontend/apps/site/src/i18n/boards.en.json
index 4fcd56ae..98c6845e 100644
--- a/frontend/apps/site/src/i18n/boards.en.json
+++ b/frontend/apps/site/src/i18n/boards.en.json
@@ -67,6 +67,14 @@
"confirm_text": "This link was asked for in another browser. Sign in only if {who} is yours: whatever you send while signed in goes to that account.",
"confirm_title": "Sign in as {who}?",
"duplicate": "The catalogue already has this dump",
+ "edit": "Edit",
+ "edit_add": "Add",
+ "edit_cancel": "Cancel",
+ "edit_remove": "Take out",
+ "edit_rereview": "A maintainer has already decided on this report. Once you save, it goes back to them, and it is off the catalogue until they accept it again.",
+ "edit_save": "Save",
+ "edit_save_rereview": "Save and send for review again",
+ "edited_at": "Edited {time}",
"email_button": "Email me a link",
"email_label": "We'll email you a link that signs you in. No password.",
"email_placeholder": "you@example.com",
@@ -137,6 +145,7 @@
"review_done_board": "Published, and {board} added to the catalogue. {points} ★ to the sender.",
"review_done_publish": "Published. {points} ★ to the sender.",
"review_done_reject": "Rejected.",
+ "review_edited_label": "Edited by the sender",
"review_empty": "Nothing is waiting.",
"review_existing": "The catalogue already has {board}: the report is published there unless you change the board ids below.",
"review_forbidden": "Only OpenIPC's maintainers review. Sign in with GitHub as a member of the OpenIPC organisation.",
diff --git a/frontend/apps/site/src/i18n/boards.ru.json b/frontend/apps/site/src/i18n/boards.ru.json
index 97f7154a..6d45fe60 100644
--- a/frontend/apps/site/src/i18n/boards.ru.json
+++ b/frontend/apps/site/src/i18n/boards.ru.json
@@ -69,6 +69,14 @@
"confirm_text": "Эту ссылку запросили в другом браузере. Входите, только если {who} — это вы: всё, что вы пришлёте после входа, попадёт в эту учётную запись.",
"confirm_title": "Войти как {who}?",
"duplicate": "Такой дамп в каталоге уже есть",
+ "edit": "Изменить",
+ "edit_add": "Добавить",
+ "edit_cancel": "Отмена",
+ "edit_remove": "Убрать",
+ "edit_rereview": "Мейнтейнер уже принял решение по этому отчёту. После сохранения он вернётся на проверку и пропадёт из каталога, пока его не примут снова.",
+ "edit_save": "Сохранить",
+ "edit_save_rereview": "Сохранить и отправить на повторную проверку",
+ "edited_at": "Изменён {time}",
"email_button": "Прислать ссылку",
"email_label": "Пришлём на почту ссылку для входа. Без пароля.",
"email_placeholder": "you@example.com",
@@ -139,6 +147,7 @@
"review_done_board": "Опубликовано, {board} добавлена в каталог. Отправителю {points} ★.",
"review_done_publish": "Опубликовано. Отправителю {points} ★.",
"review_done_reject": "Отклонено.",
+ "review_edited_label": "Изменён отправителем",
"review_empty": "Ничего не ждёт проверки.",
"review_existing": "В каталоге уже есть {board}: отчёт будет опубликован там, если не поменять идентификаторы плат ниже.",
"review_forbidden": "Проверяют только мейнтейнеры OpenIPC. Войдите через GitHub как участник организации OpenIPC.",
diff --git a/frontend/apps/site/src/i18n/boards.zh.json b/frontend/apps/site/src/i18n/boards.zh.json
index ee69544f..1aa0bb33 100644
--- a/frontend/apps/site/src/i18n/boards.zh.json
+++ b/frontend/apps/site/src/i18n/boards.zh.json
@@ -67,6 +67,14 @@
"confirm_text": "此链接是在另一个浏览器中请求的。只有当 {who} 是你本人时才登录:登录后提交的内容都会进入该账户。",
"confirm_title": "以 {who} 身份登录?",
"duplicate": "目录中已有此转储",
+ "edit": "编辑",
+ "edit_add": "添加",
+ "edit_cancel": "取消",
+ "edit_remove": "移除",
+ "edit_rereview": "维护者已经审核过这份报告。保存后它会重新进入审核,在再次通过之前不会出现在目录中。",
+ "edit_save": "保存",
+ "edit_save_rereview": "保存并重新提交审核",
+ "edited_at": "编辑于 {time}",
"email_button": "发送登录链接",
"email_label": "我们会通过邮件发送登录链接,无需密码。",
"email_placeholder": "you@example.com",
@@ -136,6 +144,7 @@
"review_done_board": "已发布,{board} 已加入目录。发送者获得 {points} ★。",
"review_done_publish": "已发布。提交者获得 {points} ★。",
"review_done_reject": "已拒绝。",
+ "review_edited_label": "发送者已编辑",
"review_empty": "没有待审核的内容。",
"review_existing": "目录中已有 {board}:除非修改下方的电路板 ID,报告将发布在那里。",
"review_forbidden": "只有 OpenIPC 维护者可以审核。请以 OpenIPC 组织成员身份使用 GitHub 登录。",
diff --git a/frontend/apps/site/src/lib/club.ts b/frontend/apps/site/src/lib/club.ts
index eb06de4a..c2bcddd6 100644
--- a/frontend/apps/site/src/lib/club.ts
+++ b/frontend/apps/site/src/lib/club.ts
@@ -65,6 +65,8 @@ export interface MemberReport {
review_note?: string;
board?: BoardRef;
proposal?: Proposal;
+ /** When its sender last changed it. */
+ edited_at?: string;
/** The report this one was sent to go with, by a Club code. */
joins?: string;
chip?: string;
@@ -94,6 +96,9 @@ export interface Queued {
proposal?: Proposal;
new_board?: Suggestion;
joins?: string;
+ edited_at?: string;
+ /** Its sender's edits; sent back with a decision, which is refused if it moved. */
+ revision: number;
}
const BASE = '/api/v1/club';
@@ -181,9 +186,10 @@ export const fetchMine = () => call<{ member: Member; reports: MemberReport[] }>
export const fetchQueue = (status = 'pending') => call<{ reports: Queued[] }>(`/review?${new URLSearchParams({ status })}`);
-export const decide = (id: string, decision: 'publish' | 'reject', models: string[], note: string, newBoard?: NewBoard) =>
- post<{ points: number; total: number; board?: string }>(`/review/${encodeURIComponent(id)}`,
- newBoard ? { decision, models, note, new_board: newBoard } : { decision, models, note });
+export const decide = (id: string, decision: 'publish' | 'reject', models: string[], note: string, newBoard?: NewBoard, revision?: number) =>
+ post<{ points: number; total: number; board?: string }>(`/review/${encodeURIComponent(id)}`, {
+ decision, models, note, ...(newBoard ? { new_board: newBoard } : {}), ...(revision !== undefined ? { revision } : {}),
+ });
/** A code that makes `ipctool upload --note ` the member's report. */
export interface ReportCode { code: string; expires_at: string; joins?: string }
@@ -195,6 +201,15 @@ export const newReportCode = (joins?: string) =>
/** The send form's answer: the report's receipt. */
export interface Sent { id: string; receipt_url: string; files: { kind: string; name: string; private?: boolean }[] }
+/** A member's change to a report of theirs; pending again if it had been decided. */
+export async function editReport(id: string, form: FormData): Promise<{ status: ReportState; rereview: boolean }> {
+ const r = await fetch(`${BASE}/reports/${encodeURIComponent(id)}/edit`, { method: 'POST', body: form, credentials: 'same-origin' });
+ let body: unknown = null;
+ try { body = await r.json(); } catch { /* not JSON */ }
+ if (!r.ok) throw new ClubError(r.status, (body as { error?: string } | null)?.error ?? `HTTP ${r.status}`);
+ return body as { status: ReportState; rereview: boolean };
+}
+
export async function sendReport(form: FormData): Promise {
const r = await fetch(`${BASE}/reports`, { method: 'POST', body: form, credentials: 'same-origin' });
let body: unknown = null;
diff --git a/service/cmd/openipc/main.go b/service/cmd/openipc/main.go
index 93d07b47..12bc4fd4 100644
--- a/service/cmd/openipc/main.go
+++ b/service/cmd/openipc/main.go
@@ -199,6 +199,7 @@ var routes = []Route{
{"web", "POST", "/api/v1/club/reports"},
{"web", "GET", "/api/v1/club/reports"},
{"web", "POST", "/api/v1/club/reports/code"},
+ {"web", "POST", "/api/v1/club/reports/{id}/edit"},
{"web", "GET", "/api/v1/club/reports/{id}/files/{position}"},
{"web", "GET", "/api/v1/club/review"},
{"web", "POST", "/api/v1/club/review/{id}"},
diff --git a/service/cmd/openipc/reports.go b/service/cmd/openipc/reports.go
index cf6d7376..ccf909bc 100644
--- a/service/cmd/openipc/reports.go
+++ b/service/cmd/openipc/reports.go
@@ -107,7 +107,7 @@ func reportsCommand(ctx context.Context, cfg *config.Config, log *slog.Logger, a
return err
}
}
- d, err := st.Decide(ctx, pos[0], cmd, *by, *note, models, nb)
+ d, err := st.Decide(ctx, pos[0], cmd, *by, *note, models, nb, nil)
if err != nil {
return err
}
diff --git a/service/deploytest/reports_test.go b/service/deploytest/reports_test.go
index 7631ca36..852e3d8d 100644
--- a/service/deploytest/reports_test.go
+++ b/service/deploytest/reports_test.go
@@ -18,8 +18,8 @@ import (
// names the reports' tables in SQL, stands the guard down, or touches their
// files' root is an error here, in Go, SQL and shell alike. Tests may.
func TestOnlyTheReportsPackageTouchesReports(t *testing.T) {
- sql := regexp.MustCompile(`(?i)\b(from|into|update|join|truncate|table|references)\s+(only\s+)?(reports|report_files|report_reviews|report_models|report_key|report_proposals)\b`)
- named := regexp.MustCompile(`\b(report_files|report_reviews|report_models|report_key|report_proposals|reports_guard)\b`)
+ sql := regexp.MustCompile(`(?i)\b(from|into|update|join|truncate|table|references)\s+(only\s+)?(reports|report_files|report_reviews|report_models|report_key|report_proposals|report_edits)\b`)
+ named := regexp.MustCompile(`\b(report_files|report_reviews|report_models|report_key|report_proposals|report_edits|reports_guard)\b`)
root := regexp.MustCompile(`REPORTS_ROOT|owner-reports`)
migration := regexp.MustCompile(`^service/internal/db/migrations/\d+_reports?(_[a-z_]+)?\.sql$`)
diff --git a/service/internal/club/api.go b/service/internal/club/api.go
index 65944dd7..ed894bdf 100644
--- a/service/internal/club/api.go
+++ b/service/internal/club/api.go
@@ -92,6 +92,7 @@ func (a *API) Handlers() map[string]http.Handler {
"POST /api/v1/club/reports": a.post(a.send),
"GET /api/v1/club/reports": http.HandlerFunc(a.mine),
"POST /api/v1/club/reports/code": a.post(a.reportCode),
+ "POST /api/v1/club/reports/{id}/edit": a.post(a.editReport),
"GET /api/v1/club/reports/{id}/files/{position}": http.HandlerFunc(a.file),
"GET /api/v1/club/review": http.HandlerFunc(a.queue),
"POST /api/v1/club/review/{id}": a.post(a.decide),
@@ -530,6 +531,20 @@ func (a *API) send(w http.ResponseWriter, r *http.Request) {
a.Reports.Submit(w, r, member)
}
+// editReport is POST /api/v1/club/reports/{id}/edit: the member changing a
+// report of theirs (reports.API.Edit). One a maintainer had decided is
+// pending again, and the boards drop it until it is accepted once more.
+func (a *API) editReport(w http.ResponseWriter, r *http.Request) {
+ m, ok := a.signedIn(w, r)
+ if !ok {
+ return
+ }
+ by := m.Name + " (" + m.ID + ")"
+ if a.Reports.Edit(w, r, m.ID, by, r.PathValue("id")) && a.OnReviewed != nil {
+ a.OnReviewed(context.WithoutCancel(r.Context()))
+ }
+}
+
// reportCode is POST /api/v1/club/reports/code {"joins": "r-..."}: a code
// for a report sent from the camera -- `ipctool upload --note ` -- that
// makes it the member's, joining one of their reports when joins names it.
@@ -645,7 +660,9 @@ func (a *API) decide(w http.ResponseWriter, r *http.Request) {
Decision string `json:"decision"`
Models []string `json:"models"`
NewBoard *boards.NewModel `json:"new_board"`
- Note string `json:"note"`
+ // Revision is the queue's, so a report changed since is not decided.
+ Revision *int `json:"revision"`
+ Note string `json:"note"`
}
if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, 16<<10)).Decode(&in); err != nil {
a.refuse(w, http.StatusBadRequest, `send {"decision": "publish" or "reject", "models": [...], "note": "..."}`)
@@ -653,7 +670,11 @@ func (a *API) decide(w http.ResponseWriter, r *http.Request) {
}
id := r.PathValue("id")
by := m.Name + " (" + m.ID + ")"
- d, err := a.Reports.Store().Decide(r.Context(), id, in.Decision, by, in.Note, in.Models, in.NewBoard)
+ d, err := a.Reports.Store().Decide(r.Context(), id, in.Decision, by, in.Note, in.Models, in.NewBoard, in.Revision)
+ if errors.Is(err, reports.ErrChanged) {
+ a.refuse(w, http.StatusConflict, err.Error())
+ return
+ }
if errors.Is(err, reports.ErrNotFound) {
a.refuse(w, http.StatusNotFound, "no report has this id")
return
diff --git a/service/internal/club/edit_test.go b/service/internal/club/edit_test.go
new file mode 100644
index 00000000..4a97b19e
--- /dev/null
+++ b/service/internal/club/edit_test.go
@@ -0,0 +1,193 @@
+package club
+
+import (
+ "bytes"
+ "context"
+ "encoding/json"
+ "mime/multipart"
+ "strconv"
+ "strings"
+ "testing"
+)
+
+// edit is POST /api/v1/club/reports/{id}/edit as /club sends it.
+func (b *browser) edit(t *testing.T, id string, fields map[string]string, files map[string][]byte, remove ...int) (int, map[string]any) {
+ t.Helper()
+ var body bytes.Buffer
+ mw := multipart.NewWriter(&body)
+ for k, v := range fields {
+ _ = mw.WriteField(k, v)
+ }
+ for _, pos := range remove {
+ _ = mw.WriteField("remove", strconv.Itoa(pos))
+ }
+ for k, v := range files {
+ w, _ := mw.CreateFormFile(strings.SplitN(k, "#", 2)[0], k+".bin")
+ _, _ = w.Write(v)
+ }
+ _ = mw.Close()
+ rec := b.do(t, "POST", "/api/v1/club/reports/"+id+"/edit", &body, mw.FormDataContentType())
+ var out map[string]any
+ _ = json.Unmarshal(rec.Body.Bytes(), &out)
+ return rec.Code, out
+}
+
+func (e *env) status(t *testing.T, b *browser, id string) map[string]any {
+ t.Helper()
+ _, mine := b.json(t, "GET", "/api/v1/club/reports", nil)
+ for _, r := range mine["reports"].([]any) {
+ if r := r.(map[string]any); r["id"] == id {
+ return r
+ }
+ }
+ t.Fatalf("no report %s", id)
+ return nil
+}
+
+// A member edits what they sent. While it waits, an edit is just made; once
+// a maintainer has decided, an edit puts it back in the queue -- off the
+// board and the public page -- and the next decision settles the stars:
+// what was taken out is taken back, what was added is paid.
+func TestAMemberEditsAReportAndAnEditAfterReviewIsReviewedAgain(t *testing.T) {
+ e := newEnv(t)
+ ctx := context.Background()
+ ivan := e.signedIn(t, 777, "ivan_k", "198.51.100.1")
+ _, out := ivan.send(t, map[string]string{"channel": "web", "maker": "Jooan", "board": "Q9", "note": "from a market"},
+ map[string][]byte{"photo": photo, "photo#2": photo})
+ id := out["id"].(string)
+
+ // Pending: the note, the marking, one more photo; still pending, no review.
+ code, d := ivan.edit(t, id, map[string]string{"note": "bought on Ozon as Q9 Pro", "maker": "Jooan", "board": "Q9 Pro"}, map[string][]byte{"photo": photo})
+ if code != 200 || d["status"] != "pending" || d["rereview"] != false {
+ t.Fatalf("edit while pending: %d %v", code, d)
+ }
+ r := e.status(t, ivan, id)
+ if r["note"] != "bought on Ozon as Q9 Pro" || r["proposal"].(map[string]any)["board"] != "Q9 Pro" || len(r["files"].([]any)) != 3 ||
+ r["pending"].(float64) != 3 || r["edited_at"] == nil {
+ t.Fatalf("after the edit: %v", r)
+ }
+
+ // What an edit may not do.
+ petr := e.signedIn(t, 888, "petr", "198.51.100.2")
+ if code, _ := petr.edit(t, id, map[string]string{"note": "mine now"}, nil); code != 404 {
+ t.Errorf("another member's edit: %d", code)
+ }
+ if code, d := ivan.edit(t, id, map[string]string{"note": "bought on Ozon as Q9 Pro"}, nil); code != 400 || !strings.Contains(d["error"].(string), "nothing to change") {
+ t.Errorf("an edit that changes nothing: %d %v", code, d)
+ }
+ if code, d := ivan.edit(t, id, nil, nil, 1, 2, 3); code != 400 || !strings.Contains(d["error"].(string), "at least one photo") {
+ t.Errorf("taking out every photo of a new camera: %d %v", code, d)
+ }
+ if code, _ := ivan.edit(t, id, nil, nil, 9); code != 400 {
+ t.Errorf("taking out a file it does not have: %d", code)
+ }
+ if code, _ := ivan.edit(t, id, map[string]string{"yaml": "chip:\n"}, nil); code != 400 {
+ t.Errorf("editing ipctool's output: %d", code)
+ }
+
+ // Published as a new board: 3 stars, the photos on the board.
+ maint := e.maintainer(t)
+ _, q := maint.json(t, "GET", "/api/v1/club/review", nil)
+ nb := q["reports"].([]any)[0].(map[string]any)["new_board"]
+ if code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish", "new_board": nb}); code != 200 || d["points"].(float64) != 3 {
+ t.Fatalf("publish: %d %v", code, d)
+ }
+
+ // Edited after review: one photo out, a boot log in. Pending again, off
+ // the board and the public page, and in the queue as edited.
+ code, d = ivan.edit(t, id, nil, map[string][]byte{"boot_log": []byte("U-Boot 2016.11\n")}, 2)
+ if code != 200 || d["status"] != "pending" || d["rereview"] != true {
+ t.Fatalf("edit after review: %d %v", code, d)
+ }
+ if pub, _ := e.api.Reports.Store().Public(ctx, id); pub.Status.State != "pending" || pub.Note != "" {
+ t.Errorf("the public page still shows it: %+v", pub.Status)
+ }
+ if texts, _ := e.api.Reports.Store().PublishedTexts(ctx); len(texts) != 0 {
+ t.Errorf("still on the board: %+v", texts)
+ }
+ _, q = maint.json(t, "GET", "/api/v1/club/review", nil)
+ queued := q["reports"].([]any)
+ if len(queued) != 1 || queued[0].(map[string]any)["id"] != id || queued[0].(map[string]any)["edited_at"] == nil {
+ t.Fatalf("not back in the queue: %v", queued)
+ }
+ if m := ivan.me(t); m["stars"].(float64) != 3 {
+ t.Errorf("stars change before the review: %v", m["stars"])
+ }
+
+ // Accepted again, on the board it made: the photo taken out is taken
+ // back, the boot log is paid -- still 3, and the positions never reused.
+ if code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish"}); code != 200 || d["points"].(float64) != 0 || d["total"].(float64) != 3 {
+ t.Fatalf("publish again: %d %v", code, d)
+ }
+ var positions []int
+ rows, _ := e.pool.Query(ctx, `SELECT position FROM report_files WHERE report_id = $1 ORDER BY position`, id)
+ for rows.Next() {
+ var p int
+ _ = rows.Scan(&p)
+ positions = append(positions, p)
+ }
+ rows.Close()
+ if len(positions) != 3 || positions[0] != 1 || positions[1] != 3 || positions[2] != 4 {
+ t.Errorf("positions %v, want 1 3 4", positions)
+ }
+ var revoked int
+ _ = e.pool.QueryRow(ctx, `SELECT count(*) FROM report_stars WHERE report_id = $1 AND position = 2 AND kind = 'revoke'`, id).Scan(&revoked)
+ if revoked != 1 {
+ t.Errorf("the removed photo's star was not taken back")
+ }
+ if texts, _ := e.api.Reports.Store().PublishedTexts(ctx); len(texts) != 1 || len(texts[0].Files) != 3 {
+ t.Errorf("back on the board: %+v", texts)
+ }
+ var edits int
+ _ = e.pool.QueryRow(ctx, `SELECT count(*) FROM report_edits WHERE report_id = $1`, id).Scan(&edits)
+ if edits != 2 {
+ t.Errorf("%d edits recorded", edits)
+ }
+ if _, err := e.pool.Exec(ctx, `UPDATE report_edits SET what = 'x'`); err == nil {
+ t.Error("the edit history accepted a change")
+ }
+}
+
+// What review found (#412): a decision on a report edited since the queue
+// was read is refused; a backup cannot be taken out; changing the camera
+// unlinks the board the old one was published on.
+func TestAnEditNeverSlipsPastAReviewer(t *testing.T) {
+ e := newEnv(t)
+ ctx := context.Background()
+ ivan := e.signedIn(t, 777, "ivan_k", "198.51.100.1")
+ maint := e.maintainer(t)
+
+ _, out := ivan.send(t, map[string]string{"channel": "web", "maker": "Jooan", "board": "Q9"}, map[string][]byte{"photo": photo})
+ id := out["id"].(string)
+ _, q := maint.json(t, "GET", "/api/v1/club/review", nil)
+ seen := q["reports"].([]any)[0].(map[string]any)
+ if code, _ := ivan.edit(t, id, nil, map[string][]byte{"photo": photo}); code != 200 {
+ t.Fatal("edit")
+ }
+ code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish", "new_board": seen["new_board"], "revision": seen["revision"]})
+ if code != 409 || !strings.Contains(d["error"].(string), "changed this report since you opened it") {
+ t.Fatalf("a decision on what the reviewer did not see: %d %v", code, d)
+ }
+ _, q = maint.json(t, "GET", "/api/v1/club/review", nil)
+ now := q["reports"].([]any)[0].(map[string]any)
+ if code, d := maint.json(t, "POST", "/api/v1/club/review/"+id, map[string]any{"decision": "publish", "new_board": now["new_board"], "revision": now["revision"]}); code != 200 || d["points"].(float64) != 2 {
+ t.Fatalf("a decision on what the reviewer saw: %d %v", code, d)
+ }
+
+ // A new marking: the board the old one made is no longer the report's.
+ if code, _ := ivan.edit(t, id, map[string]string{"maker": "Jooan", "board": "Q9 Max"}, nil); code != 200 {
+ t.Fatal("camera edit")
+ }
+ var links int
+ _ = e.pool.QueryRow(ctx, `SELECT count(*) FROM report_models WHERE report_id = $1`, id).Scan(&links)
+ if links != 0 {
+ t.Errorf("still linked to the old camera's board (%d)", links)
+ }
+
+ // A backup stays as sent.
+ _, out = ivan.send(t, map[string]string{"channel": "web", "model": "anjoy-ms-j10"},
+ map[string][]byte{"backup": bytes.Repeat([]byte{0x5a}, 1<<20), "photo": photo})
+ if code, d := ivan.edit(t, out["id"].(string), nil, nil, 1); code != 400 || !strings.Contains(d["error"].(string), "backup stays") {
+ t.Errorf("taking out a backup: %d %v", code, d)
+ }
+}
diff --git a/service/internal/db/migrations/031_report_edits.sql b/service/internal/db/migrations/031_report_edits.sql
new file mode 100644
index 00000000..bdb386ca
--- /dev/null
+++ b/service/internal/db/migrations/031_report_edits.sql
@@ -0,0 +1,26 @@
+-- A member may change what they sent -- the note, the camera they proposed,
+-- a file removed or added -- through the reports package only (Store.Edit),
+-- as Takedown does: the guard stands down for that one transaction. Every
+-- change is recorded here, append-only, so the history is still the state.
+--
+-- An edit to a report a maintainer has already decided adds a review row of
+-- its own, decision 'edit': the report is pending again, off the board and
+-- the public pages, until a maintainer accepts it once more. An edit to a
+-- pending report needs no such row.
+ALTER TABLE report_reviews DROP CONSTRAINT report_reviews_decision_check;
+ALTER TABLE report_reviews ADD CONSTRAINT report_reviews_decision_check
+ CHECK (decision IN ('publish', 'reject', 'withdraw', 'edit'));
+
+CREATE TABLE report_edits (
+ id bigint GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
+ report_id text NOT NULL REFERENCES reports ON DELETE RESTRICT,
+ member_id text NOT NULL,
+ at timestamptz NOT NULL DEFAULT now(),
+ -- what changed, for the reviewer: "note", "camera", "+2 photo", "-1 file"
+ what text NOT NULL CHECK (what <> '')
+);
+CREATE INDEX report_edits_by_report ON report_edits (report_id, at);
+CREATE TRIGGER report_edits_guard BEFORE UPDATE OR DELETE ON report_edits
+ FOR EACH ROW EXECUTE FUNCTION reports_guard();
+CREATE TRIGGER report_edits_no_truncate BEFORE TRUNCATE ON report_edits
+ FOR EACH STATEMENT EXECUTE FUNCTION reports_guard();
diff --git a/service/internal/reports/club.go b/service/internal/reports/club.go
index a1aec7a2..108bc2cd 100644
--- a/service/internal/reports/club.go
+++ b/service/internal/reports/club.go
@@ -54,6 +54,8 @@ type MemberReport struct {
// ReviewNote is what the reviewer wrote for the sender with the decision.
ReviewNote string `json:"review_note,omitempty"`
Board *ViewModel `json:"board,omitempty"`
+ // EditedAt: when its sender last changed it (Store.Edit).
+ EditedAt *time.Time `json:"edited_at,omitempty"`
// Joins: the report this one was sent to go with (a Club code's).
Joins string `json:"joins,omitempty"`
// Proposal: the camera the sender named, when the catalogue had none.
@@ -151,7 +153,10 @@ func (s *Store) memberReport(ctx context.Context, id string) (*MemberReport, err
if m.Joins, err = s.JoinsOf(ctx, id); err != nil {
return nil, err
}
- potential, err := s.potential(ctx, id, yaml != "")
+ if err := s.DB.QueryRow(ctx, `SELECT max(at) FROM report_edits WHERE report_id = $1`, id).Scan(&m.EditedAt); err != nil {
+ return nil, err
+ }
+ potential, err := s.potential(ctx, s.DB, id, yaml != "")
if err != nil {
return nil, err
}
@@ -206,12 +211,12 @@ func (s *Store) memberReport(ctx context.Context, id string) (*MemberReport, err
// potential is what each part of a report would earn now: position 0 for
// ipctool's output, the file's position for each file.
-func (s *Store) potential(ctx context.Context, id string, hasYAML bool) (map[int]int, error) {
+func (s *Store) potential(ctx context.Context, q rowsQuerier, id string, hasYAML bool) (map[int]int, error) {
out := map[int]int{}
if hasYAML {
out[0] = Points("yaml", false)
}
- rows, err := s.DB.Query(ctx, `
+ rows, err := q.Query(ctx, `
SELECT f.position, f.kind,
f.kind = 'backup' AND (
EXISTS (SELECT 1 FROM report_files o JOIN reports ro ON ro.id = o.report_id
@@ -272,6 +277,11 @@ type Queued struct {
NewBoard *boards.Suggestion `json:"new_board,omitempty"`
// Joins: the report this one was sent to go with.
Joins string `json:"joins,omitempty"`
+ // EditedAt: when its sender last changed it.
+ EditedAt *time.Time `json:"edited_at,omitempty"`
+ // Revision counts its sender's edits; a decision sent with it is refused
+ // when the report has changed since (ErrChanged).
+ Revision int `json:"revision"`
}
// Queue is the review queue: the reports in one state (pending by default),
@@ -299,7 +309,10 @@ func (s *Store) Queue(ctx context.Context, state string) ([]Queued, error) {
if err != nil {
return nil, err
}
- q.Board, q.FileList, q.Proposal, q.Joins = m.Board, m.Files, m.Proposal, m.Joins
+ q.Board, q.FileList, q.Proposal, q.Joins, q.EditedAt = m.Board, m.Files, m.Proposal, m.Joins, m.EditedAt
+ if q.Revision, err = revisionOf(ctx, s.DB, l.ID); err != nil {
+ return nil, err
+ }
if p := m.Proposal; p != nil {
sug, err := boards.Suggest(ctx, s.DB, p.Maker, p.Board, p.SoC)
if err != nil {
@@ -338,7 +351,7 @@ type Decided struct {
// the board newBoard describes first when the catalogue did not have it,
// and awards each accepted part; rejecting takes back anything it had
// earned.
-func (s *Store) Decide(ctx context.Context, id, decision, by, note string, models []string, newBoard *boards.NewModel) (Decided, error) {
+func (s *Store) Decide(ctx context.Context, id, decision, by, note string, models []string, newBoard *boards.NewModel, revision *int) (Decided, error) {
var d Decided
if decision != "publish" && decision != "reject" {
return d, fmt.Errorf("a review publishes or rejects")
@@ -358,8 +371,6 @@ func (s *Store) Decide(ctx context.Context, id, decision, by, note string, model
models = []string{*hint}
}
}
- // The potential is read before the decision, so this report's own
- // backup is not found "already published" by itself.
var yaml string
if err := s.DB.QueryRow(ctx, `SELECT yaml FROM reports WHERE id = $1`, id).Scan(&yaml); err != nil {
if errors.Is(err, pgx.ErrNoRows) {
@@ -367,10 +378,6 @@ func (s *Store) Decide(ctx context.Context, id, decision, by, note string, model
}
return d, err
}
- potential, err := s.potential(ctx, id, yaml != "")
- if err != nil {
- return d, err
- }
// The board a review adds, the report's links, the review and the
// sender's stars are one transaction: a board that cannot be added, a
// link to one the catalogue does not have, or a ledger that cannot be
@@ -385,6 +392,24 @@ func (s *Store) Decide(ctx context.Context, id, decision, by, note string, model
if _, err := tx.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended('report-stars:' || $1, 0))`, id); err != nil {
return err
}
+ // What the reviewer saw is what is decided: a report its sender
+ // changed since the queue was read is refused, to be read again.
+ if revision != nil {
+ now, err := revisionOf(ctx, tx, id)
+ if err != nil {
+ return err
+ }
+ if now != *revision {
+ return ErrChanged
+ }
+ }
+ // The potential is read under the lock an edit holds, so it is the
+ // files as they are; and before this decision's row, so the report's
+ // own backup is not found "already published" by itself.
+ potential, err := s.potential(ctx, tx, id, yaml != "")
+ if err != nil {
+ return err
+ }
if decision == "publish" {
if newBoard != nil {
board, err := boards.CreateModel(ctx, tx, *newBoard, s.SoC)
diff --git a/service/internal/reports/edit.go b/service/internal/reports/edit.go
new file mode 100644
index 00000000..cb922044
--- /dev/null
+++ b/service/internal/reports/edit.go
@@ -0,0 +1,383 @@
+package reports
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "net/http"
+ "sort"
+ "strings"
+ "unicode/utf8"
+
+ "github.com/jackc/pgx/v5"
+)
+
+// A member changes what they sent (migration 031): the note, the camera
+// they proposed, a file taken out, photos and text added. ipctool's output
+// and a backup are what the camera said and stay as sent. An edit to a
+// report a maintainer has decided puts it back in the queue -- a review row
+// 'edit', after which it is pending: off the board and the public pages
+// until it is accepted again, its stars settled by that decision.
+
+var (
+ // ErrNotEditable is a report its member cannot change: withdrawn.
+ ErrNotEditable = errors.New("a withdrawn report cannot be edited")
+ // ErrChanged is a decision on a report its sender has edited since the
+ // reviewer read it.
+ ErrChanged = errors.New("the sender has changed this report since you opened it: read it again before deciding")
+ // ErrNothingChanged is an edit that changes nothing.
+ ErrNothingChanged = errors.New("nothing to change: edit the note or the camera, or add or remove a file")
+)
+
+// Change is one edit, as the handler read it. Nil fields stay as they are.
+type Change struct {
+ Note *string
+ Proposal *Proposal
+ Remove []int
+ Add []File
+}
+
+// Edited is what an edit did.
+type Edited struct {
+ Status string
+ // Rereview: the report had been decided, and is pending again.
+ Rereview bool
+ // Orphans: stored files no report names any more, for the caller to
+ // delete once the edit is committed (RemoveUnreferenced).
+ Orphans []string
+}
+
+// Edit applies a member's change to their report, in one transaction under
+// the report's lock (the one Decide holds, so a review and an edit do not
+// interleave). place puts the added files in the store.
+func (s *Store) Edit(ctx context.Context, id, member, by string, c Change, notePublic func(string) string, place func() error) (Edited, error) {
+ var out Edited
+ var what []string
+ err := Unguarded(ctx, s.DB, func(tx pgx.Tx) error {
+ if _, err := tx.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended('report-stars:' || $1, 0))`, id); err != nil {
+ return err
+ }
+ var owner *string
+ err := tx.QueryRow(ctx, `SELECT member_id FROM report_submissions WHERE report_id = $1`, id).Scan(&owner)
+ if errors.Is(err, pgx.ErrNoRows) || err == nil && (owner == nil || *owner != member) {
+ return ErrNotFound
+ }
+ if err != nil {
+ return err
+ }
+ st, err := s.status(ctx, tx, id)
+ if err != nil {
+ return err
+ }
+ if st.State == "withdrawn" {
+ return ErrNotEditable
+ }
+
+ var note string
+ if err := tx.QueryRow(ctx, `SELECT note FROM reports WHERE id = $1`, id).Scan(¬e); err != nil {
+ return err
+ }
+ if c.Note != nil && *c.Note != note {
+ if _, err := tx.Exec(ctx, `UPDATE reports SET note = $2, note_public = $3 WHERE id = $1`,
+ id, *c.Note, notePublic(*c.Note)); err != nil {
+ return err
+ }
+ what = append(what, "note")
+ }
+
+ var had Proposal
+ err = tx.QueryRow(ctx, `SELECT maker, board, soc FROM report_proposals WHERE report_id = $1`, id).Scan(&had.Maker, &had.Board, &had.SoC)
+ hasProposal := err == nil
+ if err != nil && !errors.Is(err, pgx.ErrNoRows) {
+ return err
+ }
+ if c.Proposal != nil && (!hasProposal || *c.Proposal != had) {
+ if !hasProposal {
+ return refusal{errors.New("this report names a catalogue board; only a report about a new camera has a maker and a marking to edit")}
+ }
+ if _, err := tx.Exec(ctx, `DELETE FROM report_proposals WHERE report_id = $1`, id); err != nil {
+ return err
+ }
+ if _, err := tx.Exec(ctx, `INSERT INTO report_proposals (report_id, maker, board, soc) VALUES ($1, $2, $3, $4)`,
+ id, c.Proposal.Maker, c.Proposal.Board, c.Proposal.SoC); err != nil {
+ return err
+ }
+ // The board a review linked it to was for the camera it named
+ // then: the next review decides afresh.
+ if _, err := tx.Exec(ctx, `DELETE FROM report_models WHERE report_id = $1`, id); err != nil {
+ return err
+ }
+ what = append(what, "camera")
+ }
+
+ // What the report holds now, and what it will.
+ rows, err := tx.Query(ctx, `SELECT position, kind, sha256, coalesce(public_sha256, '') FROM report_files WHERE report_id = $1`, id)
+ if err != nil {
+ return err
+ }
+ type held struct{ kind, sum, pub string }
+ files := map[int]held{}
+ for rows.Next() {
+ var pos int
+ var h held
+ if err := rows.Scan(&pos, &h.kind, &h.sum, &h.pub); err != nil {
+ rows.Close()
+ return err
+ }
+ files[pos] = h
+ }
+ rows.Close()
+ if err := rows.Err(); err != nil {
+ return err
+ }
+ removed := map[int]bool{}
+ for _, pos := range c.Remove {
+ h, ok := files[pos]
+ if !ok {
+ return refusal{fmt.Errorf("remove: the report has no file %d", pos)}
+ }
+ if h.kind == "backup" {
+ return refusal{errors.New("remove: a backup stays as it was sent; ask a maintainer to take the report down instead")}
+ }
+ removed[pos] = true
+ }
+ if len(files)-len(removed)+len(c.Add) > maxFiles {
+ return refusal{fmt.Errorf("at most %d files per report", maxFiles)}
+ }
+ if hasProposal {
+ photos := 0
+ for pos, h := range files {
+ if h.kind == "photo" && !removed[pos] {
+ photos++
+ }
+ }
+ for _, f := range c.Add {
+ if f.Kind == "photo" {
+ photos++
+ }
+ }
+ if photos == 0 {
+ return refusal{errors.New("a new camera keeps at least one photo of it")}
+ }
+ }
+
+ // Files go out, then in: a position is never used twice, not even by
+ // one that earned stars and was taken out, so the ledger's rows keep
+ // meaning what they meant.
+ var gone []string
+ for pos := range removed {
+ if _, err := tx.Exec(ctx, `DELETE FROM report_files WHERE report_id = $1 AND position = $2`, id, pos); err != nil {
+ return err
+ }
+ gone = append(gone, files[pos].sum)
+ if p := files[pos].pub; p != "" && p != files[pos].sum {
+ gone = append(gone, p)
+ }
+ }
+ if len(removed) > 0 {
+ what = append(what, fmt.Sprintf("-%d file", len(removed)))
+ }
+ if len(c.Add) > 0 {
+ for _, f := range c.Add {
+ for _, sum := range []string{f.SHA256, f.PublicSHA256} {
+ if sum == "" {
+ continue
+ }
+ if _, err := tx.Exec(ctx, lockFileSh, sum); err != nil {
+ return err
+ }
+ }
+ }
+ if err := place(); err != nil {
+ return err
+ }
+ var next int
+ // The removed rows are gone by now; their positions live on in
+ // the ledger, and in the files map read before.
+ for pos := range files {
+ next = max(next, pos)
+ }
+ var ledger int
+ if err := tx.QueryRow(ctx, `SELECT coalesce(max(position), 0) FROM report_stars WHERE report_id = $1`, id).Scan(&ledger); err != nil {
+ return err
+ }
+ next = max(next, ledger) + 1
+ kinds := map[string]int{}
+ for i, f := range c.Add {
+ var pub *string
+ var pubBytes *int64
+ if f.PublicSHA256 != "" {
+ pub, pubBytes = &c.Add[i].PublicSHA256, &c.Add[i].PublicBytes
+ }
+ if _, err := tx.Exec(ctx, `
+ INSERT INTO report_files (report_id, position, kind, name, mime, sha256, bytes, public_sha256, public_bytes)
+ VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9)`,
+ id, next+i, f.Kind, f.Name, f.Mime, f.SHA256, f.Bytes, pub, pubBytes); err != nil {
+ return err
+ }
+ kinds[f.Kind]++
+ }
+ var added []string
+ for k, n := range kinds {
+ added = append(added, fmt.Sprintf("+%d %s", n, k))
+ }
+ sort.Strings(added)
+ what = append(what, added...)
+ }
+ if len(what) == 0 {
+ return ErrNothingChanged
+ }
+ if _, err := tx.Exec(ctx, `INSERT INTO report_edits (report_id, member_id, what) VALUES ($1, $2, $3)`,
+ id, member, strings.Join(what, ", ")); err != nil {
+ return err
+ }
+ out.Status = st.State
+ if st.State == "published" || st.State == "rejected" {
+ if _, err := tx.Exec(ctx, `INSERT INTO report_reviews (report_id, decision, by, note) VALUES ($1, 'edit', $2, $3)`,
+ id, by, strings.Join(what, ", ")); err != nil {
+ return err
+ }
+ out.Status, out.Rereview = "pending", true
+ }
+ for _, sum := range gone {
+ var used bool
+ if err := tx.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM report_files WHERE sha256 = $1 OR public_sha256 = $1)`, sum).Scan(&used); err != nil {
+ return err
+ }
+ if !used {
+ out.Orphans = append(out.Orphans, sum)
+ }
+ }
+ return nil
+ })
+ return out, err
+}
+
+// revisionOf counts a report's edits.
+func revisionOf(ctx context.Context, q querier, id string) (int, error) {
+ var n int
+ err := q.QueryRow(ctx, `SELECT count(*) FROM report_edits WHERE report_id = $1`, id).Scan(&n)
+ return n, err
+}
+
+type rowsQuerier interface {
+ Query(ctx context.Context, sql string, args ...any) (pgx.Rows, error)
+}
+
+// refusal is an edit the request asked wrongly for, answered 400.
+type refusal struct{ error }
+
+// Edit is POST /api/v1/club/reports/{id}/edit, for its signed-in member
+// (internal/club decides who): the parts are an upload's -- note, maker,
+// board, soc, photo and text files -- and `remove` with a file's position.
+// It answers itself and says whether the edit was made.
+func (a *API) Edit(w http.ResponseWriter, r *http.Request, member, by, id string) bool {
+ ctx := r.Context()
+ st := a.store()
+ key, err := st.Key(ctx)
+ if err != nil {
+ a.fail(w, "the report key", err)
+ return false
+ }
+ in, status, err := a.read(r, true)
+ if in != nil {
+ defer in.discard()
+ }
+ if err != nil {
+ a.refuse(w, status, err.Error())
+ return false
+ }
+ if in.yaml != "" {
+ a.refuse(w, http.StatusBadRequest, "an edit is multipart: note, maker, board, soc, files to add, and remove")
+ return false
+ }
+ var c Change
+ if note, ok := in.fields["note"]; ok {
+ c.Note = ¬e
+ }
+ _, mk := in.fields["maker"]
+ _, bd := in.fields["board"]
+ _, sc := in.fields["soc"]
+ if mk || bd || sc {
+ p := &Proposal{Maker: in.fields["maker"], Board: in.fields["board"], SoC: in.fields["soc"]}
+ switch {
+ case p.Maker == "" || p.Board == "":
+ a.refuse(w, http.StatusBadRequest, "a new camera needs its maker and its board's marking or model name")
+ return false
+ case utf8.RuneCountInString(p.Maker) > 80 || utf8.RuneCountInString(p.Board) > 80:
+ a.refuse(w, http.StatusBadRequest, "maker and board: at most 80 characters each")
+ return false
+ case utf8.RuneCountInString(p.SoC) > 40:
+ a.refuse(w, http.StatusBadRequest, "soc: at most 40 characters")
+ return false
+ }
+ c.Proposal = p
+ }
+ c.Remove = in.removes
+
+ // The report's own facts redact what the edit adds, as they did what it
+ // was sent with; any MAC is caught regardless (Redact).
+ rep, err := st.Private(ctx, id)
+ if errors.Is(err, ErrNotFound) {
+ a.refuse(w, http.StatusNotFound, "no report of yours has this id")
+ return false
+ }
+ if err != nil {
+ a.fail(w, "the report", err)
+ return false
+ }
+ var facts Facts
+ if rep.YAML != "" {
+ _, facts, _ = Parse(rep.YAML)
+ }
+ var places []func() error
+ for _, p := range in.parts {
+ f, err := a.prepare(p, "", facts, key, &places)
+ if err != nil {
+ a.fail(w, "a file", err)
+ return false
+ }
+ c.Add = append(c.Add, f)
+ }
+ res, err := st.Edit(ctx, id, member, by, c, func(note string) string { return Redact(note, facts, key) }, func() error {
+ for _, place := range places {
+ if err := place(); err != nil {
+ return err
+ }
+ }
+ return nil
+ })
+ if err != nil {
+ // What place put in the store before the edit failed belongs to no
+ // report: take it back out (a sum another report holds stays).
+ for _, f := range c.Add {
+ for _, sum := range []string{f.SHA256, f.PublicSHA256} {
+ if sum != "" {
+ _, _ = st.RemoveUnreferenced(ctx, a.Files, sum)
+ }
+ }
+ }
+ }
+ switch {
+ case errors.Is(err, ErrNotFound):
+ a.refuse(w, http.StatusNotFound, "no report of yours has this id")
+ return false
+ case errors.Is(err, ErrNotEditable), errors.Is(err, ErrNothingChanged):
+ a.refuse(w, http.StatusBadRequest, err.Error())
+ return false
+ case errors.As(err, new(refusal)):
+ a.refuse(w, http.StatusBadRequest, err.Error())
+ return false
+ case err != nil:
+ a.fail(w, "the edit", err)
+ return false
+ }
+ for _, sum := range res.Orphans {
+ if _, err := st.RemoveUnreferenced(ctx, a.Files, sum); err != nil {
+ a.Log.Warn("reports: an edit's removed file stays", "report", id, "sha256", sum, "err", err)
+ }
+ }
+ a.Log.Info("reports: edited", "report", id, "member", member, "rereview", res.Rereview, "files_added", len(c.Add), "files_removed", len(c.Remove))
+ writeJSON(w, http.StatusOK, map[string]any{"id": id, "status": res.Status, "rereview": res.Rereview})
+ return true
+}
diff --git a/service/internal/reports/handler.go b/service/internal/reports/handler.go
index 30db2e7c..a74f9daa 100644
--- a/service/internal/reports/handler.go
+++ b/service/internal/reports/handler.go
@@ -74,10 +74,12 @@ type part struct {
// received is an upload read off the wire.
type received struct {
- yaml string
- backup *Incoming
- parts []part
- fields map[string]string
+ yaml string
+ backup *Incoming
+ parts []part
+ fields map[string]string
+ // removes: the positions an edit takes out (Edit).
+ removes []int
discard func()
}
@@ -120,7 +122,7 @@ func (a *API) Submit(w http.ResponseWriter, r *http.Request, member string) {
return
}
- in, status, err := a.read(r)
+ in, status, err := a.read(r, false)
if in != nil {
defer in.discard()
}
@@ -248,31 +250,7 @@ func (a *API) Submit(w http.ResponseWriter, r *http.Request, member string) {
// transaction that names them, and a takedown removing the same bytes
// waits for it (Store.Insert, Store.RemoveUnreferenced).
var places []func() error
- prepare := func(p part) (File, error) {
- f := File{Kind: p.kind, Name: p.name, Mime: p.mime, Bytes: p.in.Bytes, SHA256: p.in.Sum()}
- inc := p.in
- places = append(places, func() error { _, err := a.Files.Keep(inc); return err })
- switch {
- case p.kind == "backup" && consent != "public":
- // stored for the maintainers, never served
- case textKinds[p.kind]:
- rd, err := p.in.Open()
- if err != nil {
- return f, err
- }
- text, err := io.ReadAll(rd)
- if err != nil {
- return f, err
- }
- pub := []byte(Redact(string(text), facts, key))
- ps := sha256.Sum256(pub)
- f.PublicSHA256, f.PublicBytes = hex.EncodeToString(ps[:]), int64(len(pub))
- places = append(places, func() error { _, err := a.Files.Put(pub); return err })
- default:
- f.PublicSHA256, f.PublicBytes = f.SHA256, f.Bytes
- }
- return f, nil
- }
+ prepare := func(p part) (File, error) { return a.prepare(p, consent, facts, key, &places) }
all := in.parts
if in.backup != nil {
name := "backup.bin"
@@ -383,6 +361,35 @@ func proposalOf(in *received, channel, model string) (*Proposal, error) {
return nil, errors.New("a new camera needs at least one photo of it")
}
+// prepare makes a received file a report's, and adds to places what puts it
+// in the store: the bytes, and for text a copy with the board's identifiers
+// replaced. A backup is served only when its owner said public.
+func (a *API) prepare(p part, consent string, facts Facts, key string, places *[]func() error) (File, error) {
+ f := File{Kind: p.kind, Name: p.name, Mime: p.mime, Bytes: p.in.Bytes, SHA256: p.in.Sum()}
+ inc := p.in
+ *places = append(*places, func() error { _, err := a.Files.Keep(inc); return err })
+ switch {
+ case p.kind == "backup" && consent != "public":
+ // stored for the maintainers, never served
+ case textKinds[p.kind]:
+ rd, err := p.in.Open()
+ if err != nil {
+ return f, err
+ }
+ text, err := io.ReadAll(rd)
+ if err != nil {
+ return f, err
+ }
+ pub := []byte(Redact(string(text), facts, key))
+ ps := sha256.Sum256(pub)
+ f.PublicSHA256, f.PublicBytes = hex.EncodeToString(ps[:]), int64(len(pub))
+ *places = append(*places, func() error { _, err := a.Files.Put(pub); return err })
+ default:
+ f.PublicSHA256, f.PublicBytes = f.SHA256, f.Bytes
+ }
+ return f, nil
+}
+
// flashImage: the size of a whole NOR or NAND chip, 1 to 256 MB.
func flashImage(n int64) bool {
return n >= 1<<20 && n <= MaxBackup && n&(n-1) == 0
@@ -419,8 +426,10 @@ func fileName(given, kind string, i int) string {
}
// read takes the body apart, streaming every file to .incoming/ as it
-// arrives, so a 128 MB backup never sits in memory.
-func (a *API) read(r *http.Request) (*received, int, error) {
+// arrives, so a 128 MB backup never sits in memory. An edit's body (Edit)
+// is the same parts but for a report that already has its YAML and backup:
+// note, maker, board and soc, files to add, and `remove` positions.
+func (a *API) read(r *http.Request, edit bool) (*received, int, error) {
in := &received{fields: map[string]string{}}
in.discard = func() {
in.backup.Discard()
@@ -456,6 +465,21 @@ func (a *API) read(r *http.Request) (*received, int, error) {
return in, http.StatusBadRequest, errors.New("the multipart body is cut short")
}
name := p.FormName()
+ if edit {
+ switch {
+ case p.FileName() == "" && name == "remove":
+ b, _ := io.ReadAll(io.LimitReader(p, 12))
+ pos, err := strconv.Atoi(strings.TrimSpace(string(b)))
+ if err != nil || pos < 1 {
+ return in, http.StatusBadRequest, errors.New("remove: a file's position")
+ }
+ in.removes = append(in.removes, pos)
+ continue
+ case p.FileName() == "" && name != "note" && name != "maker" && name != "board" && name != "soc",
+ name == "yaml", name == "backup":
+ return in, http.StatusBadRequest, fmt.Errorf("%q cannot be edited: an edit changes the note, the camera proposed, and adds or removes photos and text files", name)
+ }
+ }
// "note" is both a field (a line of text) and a file kind (a note
// file): a part with a filename is a file, one without is a field.
isField := p.FileName() == "" && formFields[name]
@@ -500,7 +524,7 @@ func (a *API) read(r *http.Request) (*received, int, error) {
return in, http.StatusBadRequest, fmt.Errorf("%q is not a part a report has: yaml, backup, photo, boot_log, uboot_env, note, document, consent, channel, tool, model, maker, board, soc, club", name)
}
}
- if in.yaml == "" && in.backup == nil && in.fields["model"] == "" && in.fields["maker"] == "" && in.fields["board"] == "" {
+ if !edit && in.yaml == "" && in.backup == nil && in.fields["model"] == "" && in.fields["maker"] == "" && in.fields["board"] == "" {
return in, http.StatusBadRequest, errors.New("a report needs ipctool's output: a yaml part, or a backup")
}
return in, 0, nil
diff --git a/service/internal/reports/store.go b/service/internal/reports/store.go
index 4e217881..a3b09681 100644
--- a/service/internal/reports/store.go
+++ b/service/internal/reports/store.go
@@ -264,7 +264,9 @@ type Status struct {
At *time.Time `json:"reviewed_at,omitempty"`
}
-var states = map[string]string{"publish": "published", "reject": "rejected", "withdraw": "withdrawn"}
+// An 'edit' row is the member changing a decided report (Store.Edit): it is
+// pending again until a maintainer decides once more.
+var states = map[string]string{"publish": "published", "reject": "rejected", "withdraw": "withdrawn", "edit": "pending"}
func (s *Store) status(ctx context.Context, q querier, id string) (Status, error) {
var decision string
@@ -290,7 +292,7 @@ var ErrNotFound = errors.New("no such report")
// Review records a decision. Publishing and rejecting are rows added, never
// a row changed: the history is the state.
func (s *Store) Review(ctx context.Context, id, decision, by, note string) error {
- if _, ok := states[decision]; !ok || decision == "withdraw" {
+ if _, ok := states[decision]; !ok || decision == "withdraw" || decision == "edit" {
return fmt.Errorf("a review publishes or rejects; withdrawing is takedown")
}
if err := s.exists(ctx, id); err != nil {
@@ -351,6 +353,11 @@ func (s *Store) Takedown(ctx context.Context, id, by, note string) ([]string, er
}
var orphans []string
err := Unguarded(ctx, s.DB, func(tx pgx.Tx) error {
+ // The report's lock, as an edit and a review hold it: an edit cannot
+ // write into a report this takes down.
+ if _, err := tx.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended('report-stars:' || $1, 0))`, id); err != nil {
+ return err
+ }
rows, err := tx.Query(ctx, `
DELETE FROM report_files WHERE report_id = $1 RETURNING sha256, public_sha256`, id)
if err != nil {
diff --git a/service/internal/reports/view.go b/service/internal/reports/view.go
index 8b5a4c96..bd1444f4 100644
--- a/service/internal/reports/view.go
+++ b/service/internal/reports/view.go
@@ -205,7 +205,7 @@ type Listed struct {
func (s *Store) List(ctx context.Context, state string) ([]Listed, error) {
rows, err := s.DB.Query(ctx, `
SELECT r.id, r.received_at, r.channel,
- coalesce((SELECT CASE decision WHEN 'publish' THEN 'published' WHEN 'reject' THEN 'rejected' ELSE 'withdrawn' END
+ coalesce((SELECT CASE decision WHEN 'publish' THEN 'published' WHEN 'reject' THEN 'rejected' WHEN 'withdraw' THEN 'withdrawn' ELSE 'pending' END
FROM report_reviews rv WHERE rv.report_id = r.id ORDER BY rv.id DESC LIMIT 1), 'pending'),
trim(r.chip_vendor || ' ' || r.chip_model), r.sensor, trim(r.board_vendor || ' ' || r.board_model),
(SELECT count(*) FROM report_files f WHERE f.report_id = r.id)::int, r.backup_consent,
diff --git a/service/routes.json b/service/routes.json
index 1fad2fa3..0ffa5c20 100644
--- a/service/routes.json
+++ b/service/routes.json
@@ -224,6 +224,11 @@
"method": "POST",
"path": "/api/v1/club/reports/code"
},
+ {
+ "role": "web",
+ "method": "POST",
+ "path": "/api/v1/club/reports/{id}/edit"
+ },
{
"role": "web",
"method": "GET",