fix(schedules): honest one-shot records, reject past send times, add relative scheduling - #95
Open
maxlamagna wants to merge 1 commit into
Open
Conversation
…relative scheduling Three problems with one-shot schedules and one new option. They share the same popover state, the same submit path and the same API/store invariant, so they are one change rather than several. A one-shot was stored with daily_at set, so the record claimed a daily recurrence it did not have. A one-shot now stores next_run only, and the schedule strip renders it as "once at 14:30" instead of "daily at 14:30". A send time in the past was accepted at every layer and fired within 30 seconds, showing a blank countdown on the way. ScheduleStore.create() now refuses a send time that is in the past, non-finite, or beyond the year 2100, raising ValueError, and POST /api/schedules turns that into a 400 carrying the reason rather than a 500 error page. The browser and the server could disagree about what "today at 3pm" meant, because the server re-parsed a date string in its own timezone. The endpoint now accepts an explicit send_at epoch and the browser resolves the chosen date and time itself, which removes the second, divergent code path instead of patching it. send_at is optional, read only when one_shot is true, and ignored for a recurring request, so existing send_at_date callers behave exactly as before and the store stays the final validator. Date options are now built from local calendar fields rather than toISOString(), which also fixes a pre-existing wrong-calendar-day bug for anyone east or west of UTC. New: "In a while from now", in hours and minutes. It is mutually exclusive with Recurring and resets when the popover reopens, so a stale value cannot be used to slip past the new past-time check. It is for "in 2h 30m" when you are waiting on something rather than aiming at a clock time. The popover also stays open and shows the reason when a request is rejected, re-checks its target as you type instead of failing silently, and guards against a double submit. Tests: 21 new in tests/test_schedules.py. Each new guard was mutation-checked by reverting it on its own and confirming the matching test fails. static/chat.js has no test framework in this repo, so the browser side is covered by review plus a static check that every DOM id and inline handler it references resolves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in one-shot schedules, plus a new relative option that runs through the same popover state, the same submit path and the same store invariant. They are one PR because splitting them would mean shipping a cut-down version of those functions and then rewriting the same lines.
Bug 1: a one-shot described itself as a daily recurrence
Open the schedule popover, pick a date and time, leave Recurring unchecked, schedule it. The strip reads
daily at 14:30, and the record indata/schedules.jsoncarriesdaily_at: "14:30"alongsideone_shot: true.It did fire once, so the behaviour was correct. The record and the UI both described a recurrence that did not exist.
A one-shot no longer stores
daily_at.next_runis the whole story, and the strip now rendersonce at 14:30,once tomorrow at 14:30, oronce on Fri, 22 Aug at 14:30.Bug 2: a time in the past was accepted and fired immediately
At 15:00, schedule a one-shot for 14:00 today. It is accepted, the countdown renders blank, and it sends on the next tick.
ScheduleStore.create()now refuses a send time that is in the past, non-finite, or beyond the year 2100, raisingValueError.POST /api/schedulesturns that into a 400 carrying the reason instead of a 500 error page, and the popover stays open and shows it.The client and the server could disagree about "today at 3pm"
The browser sent a date string and a time string; the server re-parsed them with
strptime(...).timestamp(), which resolves in the server's timezone. On one machine both sides agree, so it is invisible in the usual setup, but two independent places were deciding what the user meant.The endpoint now accepts an explicit
send_atepoch and the browser resolves its own selection, which removes the second code path rather than patching it.send_atis additive and optional. It is read only whenone_shotis true, ignored for a recurring request, and a request that omits it behaves exactly as before, so existingsend_at_datecallers are unaffected. The store stays the final validator either way.Separately, the date dropdown was built with
toISOString(), which is UTC, so anyone far enough east or west could be offered the wrong calendar day. It now uses local calendar fields.New: "In a while from now"
Hours and minutes, relative to the moment you schedule. Mutually exclusive with Recurring, and it resets when the popover reopens so a stale value cannot be used to slip past the past-time check. It covers "in 2h 30m" when you are waiting on something rather than aiming at a clock time.
Also in the popover
It stays open and shows the reason when a request is rejected instead of closing silently, it re-checks its target as you type rather than acting on a stale one, and it guards against a double submit.
Tests
21 new in
tests/test_schedules.py. Each new guard was mutation-checked by reverting it on its own and confirming the matching test fails.static/chat.jshas no test framework in this repo, so the browser side is covered by review plus a static check that every DOM id and inline handler it references resolves.Full suite on this branch: 102 tests, 0 failures, 1 skipped.
README
One paragraph added to the Scheduled messages section, since it currently describes only a fixed date/time and Recurring.
One thing to know if you merge this second
A companion PR adds an optional per-server subtitle. Both PRs bump
style.css?v=andchat.js?v=instatic/index.html, and both bump them to the same values. They merge cleanly, but the second merge then leaves the stamps unchanged from the first, so a browser that loaded after the first merge would keep those assets. Please bump both stamps once more when you take the second of the two.Closes #97
Closes #98
Closes #99