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"})