Stop asking for a postal code on every check-in - #899
Draft
CoryMCodes wants to merge 1 commit into
Draft
Conversation
Weather is meant to be entered once: Checkin::Creator copies the previous check-in's position forward and re-fetches the forecast for the new day, and the weather step has a branch that renders the stored location as a tappable element to change it. In practice users were re-typing their postal code day after day, and re-typing it did not make the prompt go away. Two independent defects, one on each side. WeatherRetriever asked for the wrong day and then cached the answer under a key it would never look up again. The Dark Sky call this replaced in rubyforgood#689 passed `time:` for the requested date, resolved in the position's own zone; tomorrowio_rb's `forecast(location, timesteps, units)` has no date parameter at all, so the argument became decorative: the request always returned the timeline starting at today, and the record was stored under whatever date the response led with. `Weather.find_by(date:, position_id:)` therefore missed on every subsequent call for that date, the re-fetch tripped the date/position uniqueness validation, and because the write went through `Weather.create` the failure was silent and handed back an unsaved record. Checkin::Creator stores `...get(date, postal_code)&.id`, so nil id became "this check-in has no weather". Rows are keyed by (date, position) and shared by every user with that postal code, so one fetch of today was enough to poison any request for a different date — which is every back-filled check-in, and every check-in by a user whose local day is not the UTC day. The retriever now selects the day it was asked for out of the timeline, comparing dates in the position's time zone (NearestTimeZone, as before the migration) so eastern-hemisphere positions stop landing a day early, and stores it under the requested date so the cache is reachable. A losing race returns the record that won rather than an unsaved one. A date outside the window logs and returns nothing: the forecast endpoint has no history, and filing today's forecast under a back-filled day is worse for a tracker that charts weather against symptoms than having no row. Back-filled days consequently show no weather now, and say so. The frontend asked for a location whenever there was no weather, which is not the same question. `willRender` forced `inputVisible` true on every render pass while `hasWeather` was false, so a check-in with a stored location was still prompted, and the `set(..., false)` after a successful save was undone by the next pass — the input could not be dismissed. Nothing else in the app displays `locationName`, so it also looked like the save had not happened, and a submission that geocoded fine but had no forecast reported "We couldn't find that location". `inputVisible` is now derived from `hasLocation` and whether the user opened the input, so it is no longer re-decided during render. A weather query that resolves empty or rejects no longer stops the location from being saved. The two failure modes are told apart by comparing the postal code the API echoes back — it only returns one once it has geocoded it into a position — and a day with a location but no forecast shows the location plus a note. Also in scope by necessity: get_icon_legacy read string keys out of a body parsed with `symbolize_names: true`, so every icon was "default" and every summary read "General conditions are default, with an average temperature of X". Specs asserting the intended forecast fields could not have been written around that. And the component now declares `store: service()` instead of leaning on the app-wide component/store injection, which does not exist in component tests. The creator spec stubbed WeatherRetriever.get and asserted it was called, which is why none of this showed up in CI; it now goes through the retriever against the cassette, and separately pins that the location carries forward even when the forecast does not. Backend 334 examples, frontend 460 tests, standardrb, erblint and eslint all clean. Reverting each fix in isolation fails 7 of the 18 retriever examples and 6 of the 9 component tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CoryMCodes
marked this pull request as draft
August 31, 2026 17:56
Collaborator
|
Sean mentioned over the weekend at Ruby for Good that we can just push directly instead of from forked repos since we are contributors. It should help with the feedback loop a bit |
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.
Weather is meant to be entered once: Checkin::Creator copies the previous
check-in's position forward and re-fetches the forecast for the new day, and
the weather step has a branch that renders the stored location as a tappable
element to change it. In practice users were re-typing their postal code
day after day, and re-typing it did not make the prompt go away.
Two independent defects, one on each side.
WeatherRetriever asked for the wrong day and then cached the answer under a
key it would never look up again. The Dark Sky call this replaced in #689
passed
time:for the requested date, resolved in the position's own zone;tomorrowio_rb's
forecast(location, timesteps, units)has no date parameterat all, so the argument became decorative: the request always returned the
timeline starting at today, and the record was stored under whatever date the
response led with.
Weather.find_by(date:, position_id:)therefore missed onevery subsequent call for that date, the re-fetch tripped the date/position
uniqueness validation, and because the write went through
Weather.createthe failure was silent and handed back an unsaved record. Checkin::Creator
stores
...get(date, postal_code)&.id, so nil id became "this check-in hasno weather". Rows are keyed by (date, position) and shared by every user with
that postal code, so one fetch of today was enough to poison any request for
a different date — which is every back-filled check-in, and every check-in by
a user whose local day is not the UTC day.
The retriever now selects the day it was asked for out of the timeline,
comparing dates in the position's time zone (NearestTimeZone, as before the
migration) so eastern-hemisphere positions stop landing a day early, and
stores it under the requested date so the cache is reachable. A losing race
returns the record that won rather than an unsaved one. A date outside the
window logs and returns nothing: the forecast endpoint has no history, and
filing today's forecast under a back-filled day is worse for a tracker that
charts weather against symptoms than having no row. Back-filled days
consequently show no weather now, and say so.
The frontend asked for a location whenever there was no weather, which is not
the same question.
willRenderforcedinputVisibletrue on every renderpass while
hasWeatherwas false, so a check-in with a stored location wasstill prompted, and the
set(..., false)after a successful save was undoneby the next pass — the input could not be dismissed. Nothing else in the app
displays
locationName, so it also looked like the save had not happened,and a submission that geocoded fine but had no forecast reported "We couldn't
find that location".
inputVisibleis now derived fromhasLocationand whether the user openedthe input, so it is no longer re-decided during render. A weather query that
resolves empty or rejects no longer stops the location from being saved. The
two failure modes are told apart by comparing the postal code the API echoes
back — it only returns one once it has geocoded it into a position — and a
day with a location but no forecast shows the location plus a note.
Also in scope by necessity: get_icon_legacy read string keys out of a body
parsed with
symbolize_names: true, so every icon was "default" and everysummary read "General conditions are default, with an average temperature of
X". Specs asserting the intended forecast fields could not have been written
around that. And the component now declares
store: service()instead ofleaning on the app-wide component/store injection, which does not exist in
component tests.
The creator spec stubbed WeatherRetriever.get and asserted it was called,
which is why none of this showed up in CI; it now goes through the retriever
against the cassette, and separately pins that the location carries forward
even when the forecast does not. Backend 334 examples, frontend 460 tests,
standardrb, erblint and eslint all clean. Reverting each fix in isolation
fails 7 of the 18 retriever examples and 6 of the 9 component tests.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com