From c9c549ed224d20ebaea71aa77dca2715ea76586b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 15:51:14 +0000 Subject: [PATCH] Let a member who posted a picture be erased MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a member returned 500. The cause is mine and hours old: attachments.uploaded_by is NOT NULL and does not cascade, so once somebody has posted a photo SQLite refuses to delete their row. erase_member knew about threads, comments and created_by — every table that existed when it was written — and Phase B added a fourth without telling it. Reproduced before fixing: a member with a thread erases cleanly, the same member with an attachment raises FOREIGN KEY constraint failed. The picture is reassigned to the tombstone rather than deleted, the same rule the words around it already follow: the thread survives as "Miembro eliminado" and keeps its shape. Removing the picture means deleting the post it hangs off. The lists of columns to reassign and to clear are now module constants that erase_member iterates, and a test reads the live schema with PRAGMA foreign_key_list and fails if any table points at members(id), does not cascade, and is not in them. Checked against the pre-fix lists: it names attachments.uploaded_by. The next table to be added will fail a test instead of a button — and this one failed at the moment somebody exercised a right they are entitled to, which is the worst time to find out. 201 tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NizVpJ2dwzCbjCrTLCjeHn --- apps/board/members.py | 30 +++++++++++++++-- apps/board/tests/test_members.py | 58 ++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/apps/board/members.py b/apps/board/members.py index 09a3ce9..e327200 100644 --- a/apps/board/members.py +++ b/apps/board/members.py @@ -74,6 +74,24 @@ def transfer_ownership(db: sqlite3.Connection, owner_id: int, target_id: int) -> raise +# Every column pointing at members(id) that does not cascade has to be dealt +# with before the row can go, or SQLite refuses the delete and the admin gets a +# 500 with nothing to read. These two sets are the list, and +# test_members.py checks them against the schema's actual foreign keys — so a +# table added later fails a test here rather than a button in production. That +# is not hypothetical: `attachments` was added and forgotten, and the first +# person who tried to remove a member who had posted a photo got the 500. +REASSIGNED_ON_ERASE = { + ("threads", "author_id"), + ("comments", "author_id"), + # The picture belongs to the thread, which survives as "Miembro eliminado", + # so it is reassigned rather than deleted, exactly like the words around it. + # Removing the picture itself means deleting the post it is attached to. + ("attachments", "uploaded_by"), +} +CLEARED_ON_ERASE = {("members", "created_by")} + + def erase_member(db: sqlite3.Connection, member_id: int) -> None: """Remove a member and their personal data, keeping the conversation intact. @@ -84,9 +102,15 @@ def erase_member(db: sqlite3.Connection, member_id: int) -> None: ghost = tombstone_id(db) db.execute("BEGIN IMMEDIATE") try: - db.execute("UPDATE threads SET author_id = ? WHERE author_id = ?", (ghost, member_id)) - db.execute("UPDATE comments SET author_id = ? WHERE author_id = ?", (ghost, member_id)) - db.execute("UPDATE members SET created_by = NULL WHERE created_by = ?", (member_id,)) + # Table and column names come from the module constants above, never + # from a request, so the interpolation is not a place user input can + # reach. + for table, column in sorted(REASSIGNED_ON_ERASE): + db.execute(f"UPDATE {table} SET {column} = ? WHERE {column} = ?", + (ghost, member_id)) + for table, column in sorted(CLEARED_ON_ERASE): + db.execute(f"UPDATE {table} SET {column} = NULL WHERE {column} = ?", + (member_id,)) db.execute("DELETE FROM members WHERE id = ? AND role != 'owner'", (member_id,)) db.execute("COMMIT") except Exception: diff --git a/apps/board/tests/test_members.py b/apps/board/tests/test_members.py index 13f4e64..cbe204d 100644 --- a/apps/board/tests/test_members.py +++ b/apps/board/tests/test_members.py @@ -227,3 +227,61 @@ def test_ticking_it_anyway_still_creates_nothing(app, client, db, post, owner_id assert "GITEA_ADMIN_TOKEN" in response.get_data(as_text=True) assert db.execute("SELECT 1 FROM members WHERE gitea_login = 'maria'").fetchone() is None + + +# --- erasing somebody who has left traces -------------------------------- + +def test_erasing_a_member_who_posted_a_picture(app, db, post, owner_id, make_member, sign_in): + """This failed in production with a 500 the first time it was tried. + + `attachments.uploaded_by` is NOT NULL and does not cascade, so with a + picture in the database SQLite refuses to delete the member — and the admin + sees "Internal Server Error" with nothing to act on.""" + author = make_member("saliente") + db.execute("INSERT INTO threads (author_id, title, body_md) VALUES (?, 'Hola', 'Texto')", + (author,)) + db.execute( + """INSERT INTO attachments + (thread_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (1, 'foto-abc123abc123.jpg', 'foto.jpg', 'image/jpeg', 10, ?)""", + (author,), + ) + sign_in(owner_id) + + response = post(f"/comunidad/miembros/{author}/eliminar") + + assert response.status_code == 302 + assert db.execute("SELECT 1 FROM members WHERE id = ?", (author,)).fetchone() is None + # The picture stays with the thread, which survives as "Miembro eliminado" — + # the same rule the words follow. Deleting the post removes the picture. + row = db.execute( + """SELECT m.display_name FROM attachments a JOIN members m ON m.id = a.uploaded_by""" + ).fetchone() + assert row["display_name"] == "Miembro eliminado" + + +def test_every_table_pointing_at_members_is_accounted_for(db): + """The guard that makes the bug above unrepeatable. + + Read out of the live schema rather than written down twice: any future + table with a foreign key to members(id) either cascades, or is named in + members.py's erase lists. Otherwise erasure breaks — and it breaks at the + moment somebody exercises a right they are entitled to, which is the worst + possible time to find out.""" + from apps.board.members import CLEARED_ON_ERASE, REASSIGNED_ON_ERASE + handled = REASSIGNED_ON_ERASE | CLEARED_ON_ERASE + + tables = [row["name"] for row in db.execute( + "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'")] + unhandled = [ + f"{table}.{fk['from']}" + for table in tables + for fk in db.execute(f"PRAGMA foreign_key_list('{table}')").fetchall() + if fk["table"] == "members" + and (fk["on_delete"] or "").upper() != "CASCADE" + and (table, fk["from"]) not in handled + ] + assert not unhandled, ( + "these point at members(id), do not cascade, and erase_member does not " + f"touch them, so erasing a member will fail: {unhandled}" + )