Skip to content

fix(podcasts): complete plays without an explicit date - #829

Merged
ryan-winkler merged 12 commits into
latestfrom
fix/podcast-blank-play-date
Aug 17, 2026
Merged

fix(podcasts): complete plays without an explicit date#829
ryan-winkler merged 12 commits into
latestfrom
fix/podcast-blank-play-date

Conversation

@ryan-winkler

@ryan-winkler ryan-winkler commented Aug 16, 2026

Copy link
Copy Markdown

Summary

A podcast play recorded without a completion date stored NULL. Play counts read history rows WHERE end_date IS NOT NULL, so the episode showed status Completed and a play count of zero. The first three commits on this branch resolve one completion timestamp in the shared service, so the web form and the REST API cannot disagree.

Review of that change found four more defects on the same path. This update corrects them.

  • Record a play at the current server time when the caller sends a blank date or no date.
  • Anchor duplicate detection to the timestamp that is being recorded.
  • Read completion dates in one place, so the web form and the REST API apply the same rule.
  • Refuse a date that the web form cannot read, instead of recording the current time.
  • Regenerate the published API contract, which no longer matched the endpoint.

What review found

Four independent reviews ran against the diff: a QA pass, a red team and blue team panel, a staff engineering panel, and an accessibility panel for ADHD and AuDHD users. A DRY audit ran over the result.

1. A dateless play unmasked already-imported plays

Duplicate detection compared a new play against the newest play only. After one dateless play, that newest play was always today, so every re-imported old play looked new.

Measured before the correction:

Sequence Result
Import a 2020 play twice duplicate=True, 1 play. Correct.
Import 2020, record a dateless play, import 2020 again duplicate=False, 3 plays. Incorrect.

Duplicate detection now matches history rows near the timestamp that is being recorded. The query is one indexed range lookup, and it replaces a sort and a subtraction.

2. The web form recorded a time the user did not choose

podcast_save parsed dates itself. When the value could not be read, both parsers returned None. Before this branch that stored NULL. After the fallback was added, it stored the current time and reported success. The user kept a timestamp they never entered, and nothing told them.

The REST API answered 400 for the same input.

The completion-date reader now lives in app.helpers.parse_completion_datetime. api.helpers.try_parse_datetime_input calls it. The web form reports the problem and records nothing.

3. The error path closed the modal and discarded the input

The first correction redirected away from the form. For an HTMX request that removes the modal, so the user must find the episode again and type the date again. The accessibility panel identified this as an executive-function cost, and the red team identified the same defect as a broken HTMX flow.

An HTMX request now receives HX-Reswap: none and a toast. The modal stays open and the typed date stays on screen.

4. The published contract did not match the endpoint

src/api/contracts/openapi.yaml still carried the previous description. The file is served at runtime by api/contract_views.py, so API clients read a rule the code no longer applied.

The artifact is regenerated in this update.

A byte-exact gate for this already exists and works. At commit 8715914c, app.tests.test_api_contracts.OpenAPIArtifactTests fails 2 of 11 tests. CI on this branch had not been run, so the failure was never reported.

Wording

User-facing text states what happened and what to do next.

Before After
Play already recorded for {episode} Floppy recorded a play for {episode} less than five minutes ago. Floppy did not add a second play.
(none) Floppy cannot read the date "{value}". Enter the date as YYYY-MM-DD. To record this play at the current time, keep the date empty.

The previous duplicate message read as success while the play count did not change.

Validation

Commands run locally against this branch:

  • manage.py test api — 454 tests, all pass.
  • manage.py test app — all pass.
  • manage.py test api.tests.test_fork_podcast — 17 tests, all pass.
  • ruff check src — all checks pass.
  • python -m app.domain_vocabulary --check — in sync. No model vocabulary changed.
  • manage.py spectacular ... --fail-on-warn --validate — regenerated and committed.

Each new regression test was confirmed to fail without its correction:

  • test_reimported_play_stays_a_duplicate_after_a_dateless_play fails with AssertionError: False is not true against the previous duplicate logic.

Browser verification against a local server, signed in as a real user:

Step Result
Record a play with a blank date Progress moves 0/2 to 1/2. end_date is stored.
Send the same play again No second play. One history row.
Send an unreadable date Nothing recorded. Error reported.
Send an unreadable date over HTMX 200, HX-Reswap: none, error toast, modal stays open.

Database state after the sequence: one row, status=Completed, end_date set, completed_play_count=1, one history row.

Performance

  • No new query on the create path.
  • Duplicate detection is one indexed range query. It replaces an ordered fetch of the newest history row.
  • A duplicate play costs 3 queries.
  • timezone.now() is called only when no date is supplied.

Measured separately and not caused by this branch: one new play costs about 80 queries, from signals and cache invalidation. This is recorded below as follow-up work.

Offline and packaged app

The play path was tested with sockets blocked. It makes no network call. It depends only on the database and the system clock, so it works offline and in a packaged build.

Risks and rollback

  • A blank date now means "completed now". A user who knows the exact time can still supply it.
  • Rows that already hold end_date = NULL are not rewritten. See follow-up work.
  • Duplicate detection changed shape. A play within five minutes of any recorded play is now suppressed, not only within five minutes of the newest one. This is the intended correction.
  • No migration and no schema change. The branch is revertible.

Follow-up work, not in this PR

  1. Podcast.end_date moves backwards when an older play is recorded. Every review panel proposed end_date = max(existing, completed_at). That correction does not work here: a probe confirmed history rows copy the parent row at save time, so keeping the newer value would log the backfilled play under the wrong timestamp. The durable fix separates the play log from the parent projection. This needs its own issue.
  2. app/music_album_views.song_save still contains the date-parsing block that was removed here, and fork_services_music.record_song_play has no duplicate detection and assigns end_date unconditionally, so music can still clear a good timestamp. Highest remaining drift risk.
  3. Rows already stored with Completed and end_date = NULL still report zero plays. A silent backfill would invent a timestamp the user never chose, so an operator-run command is the better shape. Needs a product decision.
  4. About 80 queries per play.
  5. try_parse_datetime_input is now a one-line delegate with a name that suggests it returns None on failure. Renaming touches 14 call sites in 6 files, so it belongs in a separate mechanical change.

Relationships

Post-mortem

  • Trigger: the web form and the REST API both accepted an empty date.
  • Fault: the shared service stored that empty value on a completed play.
  • Visible effect: read paths use end_date as the completion signal, so a completed episode reported zero plays.
  • Why review was needed a second time: the first correction resolved the timestamp but left three related paths wrong. Duplicate detection still compared against the newest play, the web form still parsed dates itself, and the published contract still described the old rule.
  • Prevention: regression tests now cover both entry surfaces, the duplicate window on both sides of its edge, and the modal error path.
  • Process note: the contract gate was already in place and already failing. The regression reached review because CI was never run on the branch, not because a check was missing.

Ryan Winkler and others added 6 commits August 17, 2026 09:34
- Move the completion-date reader to app.helpers.parse_completion_datetime.
- api.helpers.try_parse_datetime_input now calls it.

The web forms and the REST API each parsed a user-supplied completion date
with their own copy of the same fourteen lines. No behaviour changes here.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Match history rows near the timestamp being recorded.
- Replace the ordered fetch of the newest play with one indexed range query.

Duplicate detection compared a new play against the newest play only. One
play recorded without a date sat at the top of the history, so every
re-imported older play was measured against today and looked new. Importing
the same 2020 play twice with a dateless play in between produced three
plays instead of one.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Read the date with the shared reader, so the web form and the API agree.
- Report the problem and record nothing when the date cannot be read.
- Keep the modal open on an HTMX request, so the typed date is not lost.
- Say that no second play was added when a play is suppressed as a duplicate.

The web form discarded a date it could not read. Once a missing date began
to mean "now", an unreadable date was recorded as the current time under a
success message, so the user kept a timestamp they never entered. The REST
API answered 400 for the same input.

The first correction redirected away from the form. For an HTMX request that
removes the modal, so the user had to find the episode and type the date
again. The response now sets HX-Reswap to none and raises an error toast.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Regenerate src/api/contracts/openapi.yaml from the endpoint docstring.

The committed artifact still described the previous rule. api/contract_views
serves this file at runtime, so API clients read a rule the code no longer
applied. No test and no CI job detects this drift.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Cover a re-imported play after a play recorded without a date.
- Cover the duplicate window on both sides of its edge, at 299s and 301s.
- Cover a blank date, an unreadable date, and an explicit date on the web form.
- Cover the HTMX error response, which must not close the modal.

Each test was confirmed to fail without its correction.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Cover the duplicate message on the web form.

Raised by the final review pass: the duplicate branch changed wording without
a test on that path.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ryan-winkler pushed a commit that referenced this pull request Aug 17, 2026
Makes the SQLite entrypoint parking test deterministic.

The test starts the real entrypoint.sh with stub wrappers on PATH, then waits for
the parking sleep to write a pidfile. It never reached that sleep in CI, because
sqlite_recovery_server.serve() binds port 8000 and calls serve_forever() when the
bind succeeds. The test only ever passed where port 8000 was already occupied,
which is true on a developer machine running Floppy and false on a runner.

Stubbing `python -m config.sqlite_recovery_server` in the wrapper reaches the
fallback parking loop deterministically, and matching on "$*" rather than "$2"
is what makes both the -c and -m invocations reachable.

Also isolates VIRTUAL_ENV for both entrypoint subprocess tests. entrypoint.sh
keeps a virtual environment first on PATH unless it is the one at $PWD/.venv, so
a venv anywhere else put the real python ahead of the test's wrappers and the
stubs never fired.

Measured with port 8000 free and VIRTUAL_ENV pointing outside the working
directory: both entrypoint tests went from failing to passing, and the config
suite is green at 121 tests.

Closes #825. Unblocks #824, #829, #830 and #833, which were all failing on this
single test.
Brings in the entrypoint parking test fix (#826), which was the only failure on
this branch's CI.
@ryan-winkler
ryan-winkler marked this pull request as ready for review August 17, 2026 12:25
Ryan Winkler and others added 2 commits August 17, 2026 13:38
* fix(music): complete listens without an explicit date

- Resolve one completion timestamp before the row is written.
- Stop a dateless listen clearing the date an earlier listen recorded.

Listen counts read history records that have an end_date, so a completed
listen stored without one did not appear. The update path assigned whatever
the caller sent, so a later dateless listen wrote NULL over a good timestamp
and the earlier listen stopped being counted.

This is the music half of the defect corrected for podcasts in #827.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(music): refuse a listen date the web form cannot read

- Read the date with the shared reader, so the web form and the API agree.
- Report the problem and record nothing when the date cannot be read.
- Keep the modal open on an HTMX request, so the typed date is not lost.

song_save held its own copy of the date-parsing block and discarded a value
it could not read. Once a missing date means "now", that silently recorded
the current time instead. The REST API answers 400 for the same input.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(music): cover blank, dateless, and unreadable listen dates

- Cover a blank date and a dateless listen over the API.
- Cover blank, unreadable, and explicit dates on the web form.
- Cover the HTMX error response, which must not close the modal.

Each test was confirmed to fail without its correction.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(music): keep the error path off an unvalidated redirect

- Return to the album instead of following ?next= on an unreadable date.
- Assert the dateless listen reaches history with a date, which is the point.
- Cover a date with no time, the form the error copy asks for.

Raised by an outside review of this branch. The success path takes ?next=
without validating it, and an error path is not the place to widen that. The
earlier tests asserted end_date on the row, which does not prove the history
record carries one, and the copy asks for YYYY-MM-DD while only a full
timestamp was covered.

Refs #827

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@ryan-winkler
ryan-winkler merged commit 0db3056 into latest Aug 17, 2026
9 checks passed
@ryan-winkler
ryan-winkler deleted the fix/podcast-blank-play-date branch August 17, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Podcast episode without date causes crashes

1 participant