From 76a4d0805c298f6926396fa5e02d3ca141a32069 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 15:55:56 +1000 Subject: [PATCH] fix(analysis): the review findings - rising gravity, undated readings, the band A second, deeper review of PR #12 found five things the first review did not. All are fixed here. The band was not the band ------------------------- The fermentation curve's shaded range was drawn as ONE dataset with `fill: "+1"`, which fills to the NEXT dataset - and the next dataset was the median line, not the maximum. So the band shaded min-to-median, the upper half of the range was drawn as a hairline but never shaded, and the tooltip said "range min to max" while the picture showed something else. The band is now two datasets - the min curve filled down to a max curve - with the max hidden from the legend. Rising gravity -------------- `attenuation_percent` rejected a final gravity below 1.000 and one implying more than 100% attenuation, but a run whose gravity went UP produced a NEGATIVE attenuation - which was then averaged into the headline figure and plotted as a negative bar. Verified against the live code: OG 1010 -> FG 1020 gave -100.0%. Now rejected, and reported by `implausible_runs` with the reason "the gravity went up, which is usually a mis-ordered pair of readings". Undated readings ---------------- An undated reading sorted to the front of the run, so it became the original gravity - which put the OG after the FG and produced a negative attenuation. The OG now comes from the earliest *dated* reading, and `days_to_target` skips the readings it cannot place on the timeline rather than giving up on the whole run. Verified: readings 1050 (dated), 1005 (undated), 1010 (dated) gave None instead of 24 days, and an OG of 1005 instead of 1050. Active runs on the curve ------------------------ The curve chart filtered on `curve.length > 1` only, so it plotted the active runs as well - a partial fermentation on a chart meant to describe how a completed one behaves. It now excludes them, along with the suspect runs. Four fetches, one payload ------------------------- `/api/runs` carries every reading of every run, and four charts each fetched it independently - so the analysis page made two identical round-trips of the whole collection. There is now one shared fetch. Also ---- - The "Readings that need a second look" table links to the run, so a suspect reading can be corrected in one click. - `days_to_1015` and the `curves` variable were computed and never used. Removed. - A reading below 1.000 now says what it is - "beer cannot be less dense than water, so this is almost certainly a typo (a 1.012 reading entered as 012?)". The reading is still SAVED: rejecting it would lose the brewer's actual observation, and seeing it recorded is how they spot the mistake. 142 tests (up from 134). --- src/flask/brewing.py | 59 ++++++++++++--- src/flask/static/js/charts.js | 69 ++++++++++++++--- src/flask/templates/analysis.html | 4 +- src/flask/views/analysis.py | 5 -- src/flask/views/brews.py | 27 +++++-- tests/test_analysis.py | 118 +++++++++++++++++++++++++++++- tests/test_app.py | 26 ++++++- 7 files changed, 276 insertions(+), 32 deletions(-) diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 32126bd..65ec584 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -193,14 +193,32 @@ class Run: """Gravity points of every *usable* reading, in chronological order.""" return [r.sg_points for r in self.readings if r.sg_points is not None] + def dated_sg_values(self) -> list[int]: + """Gravity points of the readings that have a date, chronologically. + + An undated reading cannot be placed in the run's timeline, so it + must not be used as the original gravity: it sorts to the front + and would put the OG after the FG. + """ + dated = [r for r in self.readings + if r.sg_points is not None and r.date is not None] + return [r.sg_points for r in dated] + def original_gravity(self) -> int | None: - """OG = the first usable reading of the run.""" - values = self.sg_values() + """OG = the first *dated* usable reading of the run.""" + values = self.dated_sg_values() return values[0] if values else None def final_gravity(self) -> int | None: - """FG = the last usable reading of the run (if the run is finished).""" - values = self.sg_values() + """FG = the last *dated* usable reading of the run (if it is finished). + + For a finished run the final reading is the one flagged as final, + which is the reading the brewer took when they racked it. + """ + final = self.final_reading() + if final is not None and final.sg_points is not None: + return final.sg_points + values = self.dated_sg_values() return values[-1] if values else None def max_sg(self) -> int | None: @@ -491,6 +509,11 @@ def attenuation_percent(run: Run) -> float | None: fg = run.final_gravity() if og is None or fg is None or og <= 1000 or fg < 1000: return None + if fg > og: + # The gravity went UP. That is not attenuation - it is either a + # mis-ordered pair of readings or a contaminated sample, and a + # negative attenuation is not a number a brewer can act on. + return None attenuation = (og - fg) / (og - 1000) * 100 if attenuation > 100: # Cannot ferment away more than the sugar that was there. @@ -520,6 +543,17 @@ def implausible_runs(runs: Iterable[Run]) -> list[dict]: "is probably wrong" ), }) + elif og is not None and fg is not None and fg > og: + problems.append({ + "beer_run_id": run.beer_run_id, + "og_points": og, + "fg_points": fg, + "problem": ( + f"the final gravity {fg / 1000:.3f} is higher than the " + f"original {og / 1000:.3f} — the gravity went up, which " + "is usually a mis-ordered pair of readings" + ), + }) elif og is not None and fg is not None and og > 1000 and fg > 1000 \ and (og - fg) / (og - 1000) * 100 > 100: problems.append({ @@ -537,12 +571,19 @@ def days_to_target(run: Run, target: int) -> int | None: Used to answer "how long does this usually take?", which is the question a brewer actually has when planning the next batch. """ + first = run.first_reading() + if first is None or first.date is None: + # Without a start date there is no elapsed time to measure. + return None + + # Skip the readings that cannot be placed on the timeline (no date) + # rather than giving up on the whole run because of one of them. for reading in run.readings: - if reading.sg_points is not None and reading.sg_points <= target: - first = run.first_reading() - if first is None or first.date is None or reading.date is None: - return None - return days_between(first.date, reading.date) + if reading.sg_points is None or reading.sg_points > target: + continue + if reading.date is None: + continue + return days_between(first.date, reading.date) return None diff --git a/src/flask/static/js/charts.js b/src/flask/static/js/charts.js index 3b2555e..772c451 100644 --- a/src/flask/static/js/charts.js +++ b/src/flask/static/js/charts.js @@ -179,8 +179,7 @@ var canvas = document.getElementById("abv-chart"); if (!canvas) { return; } - fetch("/api/runs", { headers: { "Accept": "application/json" } }) - .then(function (r) { return r.json(); }) + fetchRuns() .then(function (runs) { // Only finished brews have a measured ABV. An unfinished run // has a projection, not a measurement, so it is not plotted @@ -240,8 +239,7 @@ var canvas = document.getElementById("months-chart"); if (!canvas) { return; } - fetch("/api/runs", { headers: { "Accept": "application/json" } }) - .then(function (r) { return r.json(); }) + fetchRuns() .then(function (runs) { var counts = {}; runs.forEach(function (r) { @@ -302,8 +300,7 @@ var canvas = document.getElementById("attenuation-chart"); if (!canvas) { return; } - fetch("/api/runs", { headers: { "Accept": "application/json" } }) - .then(function (r) { return r.json(); }) + fetchRuns() .then(function (runs) { // A run with an impossible reading has no attenuation, and // is flagged as suspect. Both are excluded, so one bad @@ -384,14 +381,18 @@ var canvas = document.getElementById("curve-chart"); if (!canvas) { return; } - fetch("/api/runs", { headers: { "Accept": "application/json" } }) - .then(function (r) { return r.json(); }) + fetchRuns() .then(function (runs) { // A run with an impossible reading has a broken curve - it // plunges to a gravity below water - which would drag the // median line down and stretch the envelope to cover it. + // Only FINISHED brews belong on the curve. An active run's + // curve is a partial fermentation, and the curve is meant to + // describe how a completed fermentation behaves - including + // a partial one pulls the median line off the real shape. var withCurve = runs.filter(function (r) { - return !r.suspect && r.curve && r.curve.length > 1; + return !r.suspect && !r.active && + r.curve && r.curve.length > 1; }); if (!withCurve.length) { return; } @@ -462,6 +463,7 @@ // progress point. var envelope = []; var median = []; + var envelopeMax = []; for (var i = 0; i < samples; i++) { var pct = (i / (samples - 1)) * 100; var values = []; @@ -478,6 +480,7 @@ y: values[0], yMax: values[values.length - 1] }); + envelopeMax.push({ x: pct, y: values[values.length - 1] }); var mid = Math.floor(values.length / 2); median.push({ x: pct, @@ -492,6 +495,12 @@ data: { datasets: [ { + // The band is drawn as TWO datasets: the min + // curve filled down to the max curve. With + // a single dataset and `fill: "+1"` the band + // was filled to the NEXT dataset - which was + // the median line, not the max - so the + // tooltip and the shading disagreed. label: "Range (min–max)", data: envelope, borderColor: withAlpha(ACCENT, 0.35), @@ -501,6 +510,18 @@ fill: "+1", tension: 0.3 }, + { + // The upper edge of the band. It is the + // dataset the one above fills to. + label: "Range max", + data: envelopeMax, + borderColor: withAlpha(ACCENT, 0.35), + backgroundColor: "transparent", + borderWidth: 1, + pointRadius: 0, + fill: false, + tension: 0.3 + }, { label: "Median brew", data: median, @@ -522,7 +543,12 @@ boxWidth: 10, boxHeight: 10, usePointStyle: true, - font: { size: 11 } + font: { size: 11 }, + // The max curve is the helper the band + // fills to, not a series of its own. + filter: function (item, data) { + return item.text !== "Range max"; + } } }, tooltip: { @@ -533,6 +559,9 @@ return "range " + point.y + " to " + (point.yMax || point.y); } + if (ctx.datasetIndex === 1) { + return null; + } return "median " + point.y; } } @@ -569,6 +598,26 @@ .catch(function (err) { console.warn(err); }); } + /* ------------------------------------------------- shared run payload */ + + // The analysis page has two charts that both need /api/runs, and the + // payload contains every reading of every run. Fetch it once and + // share it rather than making two identical round-trips. + var runsPromise = null; + + function fetchRuns() { + if (!runsPromise) { + runsPromise = fetch("/api/runs", { headers: { "Accept": "application/json" } }) + .then(function (r) { return r.json(); }) + .catch(function (err) { + console.warn(err); + runsPromise = null; + return []; + }); + } + return runsPromise; + } + function init() { drawSparklines(); drawRunChart(); diff --git a/src/flask/templates/analysis.html b/src/flask/templates/analysis.html index a626181..747b27f 100644 --- a/src/flask/templates/analysis.html +++ b/src/flask/templates/analysis.html @@ -164,7 +164,9 @@ {% for row in suspect %} - {{ row.beer_run_id }} + + {{ row.beer_run_id }} + {{ '%.3f' % (row.og_points / 1000) }} {{ '%.3f' % (row.fg_points / 1000) }} {{ row.problem }} diff --git a/src/flask/views/analysis.py b/src/flask/views/analysis.py index 44b0ff7..2b1011e 100644 --- a/src/flask/views/analysis.py +++ b/src/flask/views/analysis.py @@ -70,7 +70,6 @@ def deep(): "attenuation": attenuation_percent(run), "days": summary["days_in_fermenter"], "days_to_1010": days_to_target(run, 1010), - "days_to_1015": days_to_target(run, 1015), "readings": summary["reading_count"], "curve": summary["curve"], # A suspect run is shown in the table so it can be corrected, @@ -78,9 +77,6 @@ def deep(): "suspect": run.beer_run_id in suspect_ids, }) - # The rows with a usable curve, for the overlay chart. - curves = [r for r in rows if r["curve"]] - # Only the trustworthy runs feed the statistics. good = [r for r in rows if not r["suspect"]] @@ -109,7 +105,6 @@ def deep(): return render_template( "analysis.html", rows=rows, - curves=curves, suspect=suspect, stats={ "count": len(rows), diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 02a24c1..e1c4d30 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -49,6 +49,10 @@ class ReadingForm(FlaskForm): beer_run_type = StringField("Beer / cider name", validators=[Optional(), Length(max=120)]) Date = DateField("Reading date", validators=[DataRequired()]) + # The floor is deliberately lower than is_plausible_sg's. An + # out-of-range reading is SAVED with a warning rather than rejected: + # the brewer may have mis-typed it, and seeing it recorded is how they + # spot and correct the mistake. sg = IntegerField("Gravity reading (points)", validators=[DataRequired(), NumberRange(min=900, max=1500)]) final_reading = BooleanField("This is the final reading") @@ -82,11 +86,24 @@ def index(): if form.validate_on_submit(): sg_points = int(form.sg.data) if not is_plausible_sg(sg_points): - flash( - f"{sg_points} is outside the usual hydrometer range " - "(990-1200). It was saved anyway — double-check the reading.", - "warning", - ) + if sg_points < 1000: + # Beer cannot be less dense than water, so this is always + # a mis-entry - and a mis-typed "1.012" as "012" is the + # likely cause. It is still saved so the mistake is + # visible and can be corrected. + flash( + f"{sg_points} is below 1.000 — beer cannot be less dense " + "than water, so this is almost certainly a typo (a 1.012 " + "reading entered as 012?). It was saved anyway; correct " + "it on the run's page.", + "warning", + ) + else: + flash( + f"{sg_points} is outside the usual hydrometer range " + "(990-1200). It was saved anyway — double-check the reading.", + "warning", + ) repo().upsert_reading( beer_run_id=str(form.beer_run_id.data), beer_type=(form.beer_run_type.data or "").strip(), diff --git a/tests/test_analysis.py b/tests/test_analysis.py index cdb3dce..ebc712c 100644 --- a/tests/test_analysis.py +++ b/tests/test_analysis.py @@ -8,7 +8,7 @@ from pathlib import Path import pytest from brewing import (Run, attenuation_percent, days_to_target, - fermentation_curve) + fermentation_curve, implausible_runs) from conftest import seed_readings @@ -392,3 +392,119 @@ def test_the_charts_skip_suspect_runs(): assert charts_js.count("!r.suspect") >= 2, ( "the charts must filter out the runs flagged as suspect" ) + +# ------------------------------------------------- rising gravity # + +def test_attenuation_is_none_when_the_gravity_went_up(): + """A negative attenuation is not a number a brewer can act on. + + The guard rejected a final gravity below 1.000 and one implying more + than 100% attenuation, but a run whose gravity ROSE produced a + negative figure that was averaged into the headline number. + """ + run = Run.from_docs("30", [ + _doc(sg="1010", date_="2026-01-01"), + _doc(sg="1020", date_="2026-01-20", final="True"), + ]) + assert attenuation_percent(run) is None + + +def test_a_rising_gravity_run_is_reported_as_implausible(): + """It must be listed for review, not just quietly dropped.""" + run = Run.from_docs("31", [ + _doc(sg="1010", date_="2026-01-01"), + _doc(sg="1020", date_="2026-01-20", final="True"), + ]) + problems = implausible_runs([run]) + assert len(problems) == 1 + assert problems[0]["beer_run_id"] == "31" + assert "went up" in problems[0]["problem"] + + +# ------------------------------------------------- undated readings # + +def test_the_original_gravity_comes_from_the_first_dated_reading(): + """An undated reading cannot be placed on the timeline. + + It used to sort to the front, so it became the OG - which put the OG + after the FG and produced a negative attenuation. + """ + run = Run.from_docs("32", [ + _doc(sg="1050", date_="2026-01-01"), + {"_id": "x", "beer_run_id": "32", "beer_type": "Ale", "sg": "1005", + "date": None, "final_reading": "False", "comment": ""}, + _doc(sg="1010", date_="2026-01-25", final="True"), + ]) + assert run.original_gravity() == 1050 + assert run.final_gravity() == 1010 + assert attenuation_percent(run) is not None + assert attenuation_percent(run) > 0 + + +def test_days_to_target_skips_an_undated_reading(): + """One undated reading must not lose the whole figure. + + The function gave up and returned None when the first reading at or + below the target had no date, even when a later reading did. + """ + run = Run.from_docs("33", [ + _doc(sg="1050", date_="2026-01-01"), + {"_id": "y", "beer_run_id": "33", "beer_type": "Ale", "sg": "1005", + "date": None, "final_reading": "False", "comment": ""}, + _doc(sg="1010", date_="2026-01-25", final="True"), + ]) + assert days_to_target(run, 1010) == 24 + + +# ------------------------------------------------------- the charts # + +def test_the_curve_chart_excludes_active_and_suspect_runs(): + """Only finished, plausible brews belong on the curve. + + An active run's curve is a partial fermentation, and the curve is + meant to describe how a completed fermentation behaves. + """ + charts_js = ( + Path(__file__).resolve().parents[1] + / "src" / "flask" / "static" / "js" / "charts.js" + ).read_text() + + assert "!r.active" in charts_js, ( + "the curve chart must leave the active runs out" + ) + + +def test_the_curve_band_is_filled_to_the_max_not_the_median(): + """The shaded band has to span min to max. + + With one dataset and `fill: "+1"` the band was filled to the NEXT + dataset - which was the median line, so the upper half of the range + was drawn as a hairline but never shaded, and the tooltip disagreed + with the shading. + """ + charts_js = ( + Path(__file__).resolve().parents[1] + / "src" / "flask" / "static" / "js" / "charts.js" + ).read_text() + + assert "envelopeMax" in charts_js, ( + "the band needs a separate max dataset to fill down to" + ) + assert 'label: "Range max"' in charts_js + + +def test_the_charts_fetch_the_run_payload_once(): + """/api/runs carries every reading of every run. + + Four charts each fetched it independently, so the analysis page made + two identical round-trips of the whole collection. + """ + charts_js = ( + Path(__file__).resolve().parents[1] + / "src" / "flask" / "static" / "js" / "charts.js" + ).read_text() + + assert charts_js.count('fetch("/api/runs"') == 1, ( + "the run payload should be fetched once and shared" + ) + assert "function fetchRuns()" in charts_js diff --git a/tests/test_app.py b/tests/test_app.py index 609eb45..091f020 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -186,11 +186,16 @@ def test_logout_clears_the_session(logged_in): def test_out_of_range_reading_is_saved_but_warned(logged_in, repo): + """A reading outside the range is SAVED with a warning. + + Rejecting it would lose the brewer's actual observation, and seeing it + recorded is how they spot and correct a typo. + """ response = logged_in.post("/brews", data={ "beer_run_id": "12", "beer_run_type": "Odd", "Date": "2026-05-01", - "sg": "950", + "sg": "1250", }, follow_redirects=True) assert response.status_code == 200 assert b"outside the usual hydrometer range" in response.data @@ -198,6 +203,25 @@ def test_out_of_range_reading_is_saved_but_warned(logged_in, repo): assert repo.readings.find_one({"beer_run_id": "12"}) is not None +def test_a_reading_below_water_says_it_is_always_a_typo(logged_in, repo): + """A gravity below 1.000 cannot be real, so the warning says so. + + This is the run 12 case: a 1.012 reading entered as "012". The + reading is still saved, but the message names the likely cause. + """ + response = logged_in.post("/brews", data={ + "beer_run_id": "13", + "beer_run_type": "Typo", + "Date": "2026-05-01", + "sg": "900", + }, follow_redirects=True) + assert response.status_code == 200 + assert b"below 1.000" in response.data + assert b"typo" in response.data + # Still saved, so the mistake is visible and can be corrected. + assert repo.readings.find_one({"beer_run_id": "13"}) is not None + + # ------------------------------------------------------- security: passwords # def test_legacy_plaintext_password_is_accepted_and_migrated(repo):