Repository navigation
Record a past front entry without disturbing the live one - #97
Merged
Merged
Conversation
Adding a history entry created it open and then patched it closed. Between those two calls the new entry IS the current front, and no replace_fronts was passed so the system default applied, which is on. The create therefore ended whoever was actually fronting and back-dated their end to the historical start. Where that start preceded the live front's own start, the end landed before its own beginning, ck_fronts_ended_after_started rejected it and the whole request 500d: back-dating failed with "Internal server error" whenever anybody was fronting, which is the common case, and appeared to work only when nobody was. Where the start did not precede it, there was no constraint to trip and the live front was simply ended in the past without a word. The constraint is the only thing that stood between this pattern and quiet data loss. POST /v1/fronts now takes ended_at and records a closed entry in one call, skipping both live-roster behaviours: no auto-end, and no refusal of a member set that is already fronting, so noting that the same people fronted last week works while they are fronting today. The request also sends replace_fronts=false whenever an end is supplied. A server that understands ended_at ignores it, since a closed entry replaces nothing. A server that predates the field drops ended_at and opens a front instead, and this is what stops that front taking the live one with it; the entry comes back open, which the caller notices and closes with the old PATCH. That is detection rather than a version check, and the fallback is now the safe version of what it used to do. Starting a front is untouched: with no end supplied, replace_fronts stays null and the system default decides, which is what it is for. Every other createFront call in the app is a live switch with a started_at of now and is unaffected.
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
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.
Android has had an Add entry sheet on the history screen since well before the web client got one. It was doing the thing the backend's new
ended_atfield exists to replace, and it had the bug that goes with it.What was wrong
addFrontEntrycreated the entry open and then patched it closed:No
replace_frontswas passed, soreplace_fronts_defaultapplied, which is on. Between those two calls the new entry is the current front, so the create auto-ended whoever was really fronting and set theirended_atto the historical start.Two outcomes, neither good:
ck_fronts_ended_after_startedrejects it, and the whole request 500s. So back-dating failed with "Internal server error" whenever anybody was fronting, and appeared to work only when nobody was.That check constraint is the only thing that stood between this pattern and quiet data loss. Credit where it is due.
The fix
POST /v1/frontsnow acceptsended_atand records a closed entry in one call. A closed entry can never be the current front, so the server skips both live-roster behaviours for it: no auto-end, and no refusal of a duplicate open member set, which means noting that the same people fronted last week works fine while they are fronting today.The request also sends
replace_fronts=falsewhenever an end is supplied. This is load-bearing for older servers rather than for new ones:ended_atignores it, because a closed entry replaces nothing anyway.ended_atand opens a front instead.replace_fronts=falseis what stops that front taking the live one with it. The entry then comes back open, the caller notices, and closes it with the old PATCH.That is detection from the response rather than a version check, per the rule that a version string picks wording and behaviour is decided by asking. The upshot is that the fallback path is now the safe version of what the app used to do unconditionally: no auto-end, no back-dated end, no 500.
Starting a front is untouched. With no end supplied,
replace_frontsstays null and the system default decides, which is exactly what it is for.Scope
Every other
createFrontcall in the app is a live switch with astarted_atof now: home quick-switch, the member screens, the widget trampoline, the sync worker, and the watch. Auto-ending is correct in all of them, and none are changed.Tests
Three, around
addFrontEntrywith a mocked API. The one that matters asserts a historical entry is a single call carryingended_atwithreplace_fronts=false; it fails against the old implementation, which I checked by reverting the change and running it rather than assuming. The others pin that an ongoing entry is still a switch withreplace_frontsleft null, and that a server which dropsended_atstill gets the entry closed afterwards.One thing left open
The sheet still offers "still ongoing", which creates an open back-dated front. That is a real switch rather than history, and the server handles it correctly in the ordinary case, but a start earlier than the live front's own start is contradictory data and will still 500. The web dialog sidesteps this by requiring both times. Worth deciding separately whether Android should validate it client-side against the open fronts, drop the toggle for new entries, or leave it.
Also: the server field is unreleased as of writing, so the changelog entry says "a server too old to support that" rather than naming a version. Happy to pin one once it ships.