From ef0d665b782d8e62994aec02e2ef3d4244c00904 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 22:16:29 +1000 Subject: [PATCH] fix(advice): return matched_count, and do not advise about the wrong reading set_reading_advice returned modified_count, which is 0 both when the guard refused the write and when the write matched but the text was already identical. The caller logs "the advice was not stored" on 0, so it has to mean "the guard refused it". It now returns matched_count. _start_advice fell back to the newest reading when the reading it was asked about was not in the run. That described the wrong number, and the conditional write would refuse it anyway - so it now says so and stops. The editing token is the primary guard on the write, not the gravity. Every edit writes a fresh token, so a comment-only correction - which leaves the gravity unchanged - is caught too. Verified against live Mongo with the real repository code: the fresh advice lands, the slow stale thread is refused, and the brewer's comment is untouched. 193 tests pass. --- src/flask/mongo/repository.py | 8 ++++++- src/flask/views/brews.py | 10 ++++++++- tests/test_brew_advice.py | 42 +++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+), 2 deletions(-) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index f781ada..d502d65 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -296,7 +296,13 @@ class BeerRepository: "advice_at": _now_iso(), }}, ) - return result.modified_count + # `matched_count`, not `modified_count`. + # + # `modified_count` is 0 when the document matched but the text + # was already identical - which is a successful write, not a + # refused one. The caller logs "the advice was not stored" when + # this is 0, so it has to mean "the guard refused it". + return result.matched_count def set_final_reading(self, beer_run_id: str, reading_date: str, final: bool) -> int: diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 56fb233..29d268c 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -106,7 +106,15 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None # wrong number. saved = next((r for r in readings if r.get("_id") == str(reading_id)), None) if saved is None: - saved = dict(readings[-1]) if readings else {"date": "", "sg": None} + # The reading is not in the run. Advising about the newest + # reading instead would describe the wrong number, and the + # conditional write would then refuse it anyway - so it is + # better to say so plainly and stop. + log.info( + "no advice for reading %s: it is not one of the run's readings", + reading_id, + ) + return latest = dict(saved) latest["_id"] = reading_id diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index 6144911..4eb5860 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -1079,6 +1079,48 @@ def test_the_long_and_short_labels_are_both_in_the_page(client, repo): assert 'Advice' in body +def test_stale_advice_cannot_overwrite_a_comment_only_correction(repo): + """A comment-only correction still invalidates the old advice. + + The gravity alone is not enough to tell two generations apart - a + comment-only correction leaves the gravity unchanged. The editing + token is what tells them apart, because every edit writes a fresh + one. + """ + repo.readings.insert_one({ + "_id": "readingone", + "beer_run_id": "1", + "date": "2026-01-01", + "sg": "1040", + "comment": "original comment", + "final_reading": "False", + "advice_token": "token-one", + }) + + # The slow thread holds the token it was given. + stale_token = "token-one" + + # The brewer corrects only the comment. + repo.readings.update_one( + {"_id": "readingone"}, + {"$set": {"comment": "corrected comment", "advice_token": "token-two"}, + "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}}, + ) + + # The fresh advice lands first, carrying the new token. + assert repo.set_reading_advice( + "readingone", "FRESH ADVICE", ok=True, expected_token="token-two") == 1 + + # The slow thread lands afterwards, carrying the old token. It must + # be refused. + assert repo.set_reading_advice( + "readingone", "STALE ADVICE", ok=True, expected_token=stale_token) == 0 + + doc = repo.readings.find_one({"_id": "readingone"}) + assert doc["advice"] == "FRESH ADVICE" + assert doc["comment"] == "corrected comment" + + def test_a_failed_advice_is_marked_as_such(client, repo): """A generation failure reads differently from real advice.""" repo.users.insert_one({"username": "beer", "password": "beer_pass"})