fix(advice): return matched_count, and do not advise about the wrong reading (#20)

fix(advice): return matched_count, and do not advise about the wrong reading (#20)

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:

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
 

tests/test_brew_advice.py

@@ -1079,6 +1079,48 @@ def test_the_long_and_short_labels_are_both_in_the_page(client, repo):
     assert '<span class="short">Advice</span>' 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"})