From 4049739a82db4b76da6d8a2186dc46859bbbbd1c Mon Sep 17 00:00:00 2001 From: Cory Musick Date: Mon, 31 Aug 2026 16:46:17 +0000 Subject: [PATCH 1/2] Stop asking for a postal code on every check-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- backend/app/services/weather_retriever.rb | 92 +++++--- .../api/v1/weathers_controller_spec.rb | 11 + backend/spec/services/checkin/creator_spec.rb | 33 ++- .../spec/services/weather_retriever_spec.rb | 157 ++++++++++++- .../app/components/checkin/weather-step.js | 66 +++--- .../components/checkin/weather-step.hbs | 16 +- .../components/checkin/weather-step-test.js | 210 +++++++++++++++++- 7 files changed, 507 insertions(+), 78 deletions(-) diff --git a/backend/app/services/weather_retriever.rb b/backend/app/services/weather_retriever.rb index ac8f05714..fbdd13398 100644 --- a/backend/app/services/weather_retriever.rb +++ b/backend/app/services/weather_retriever.rb @@ -1,19 +1,20 @@ class WeatherRetriever class << self def get(date, postal_code) + date = date.to_date position = Position.find_or_create_by(postal_code: postal_code) - weather = Weather.find_by(date: date, position_id: position&.id) - - return weather if weather.present? - if position&.latitude.blank? || position&.longitude.blank? Rails.logger.warn "No coordinates found for postal_code #{postal_code}: #{position.inspect}" return end - forecast = get_forecast(date, position) + weather = Weather.find_by(date: date, position_id: position.id) + + return weather if weather.present? + + forecast = get_forecast(position) if forecast.status != 200 Rails.logger.warn "No forecast found for position #{position.inspect}: response code was #{forecast.status}, headers were #{forecast.headers}, body contained #{forecast.body}" @@ -21,12 +22,20 @@ def get(date, postal_code) return end - create_weather(forecast, position.id) + day = daily_forecast_on(forecast, date, position) + + if day.blank? + Rails.logger.warn "No forecast for #{date} at position #{position.inspect}: the forecast endpoint only covers today onwards" + + return + end + + create_weather(day, date, position.id) end private - def get_forecast(date, position) + def get_forecast(position) Tomorrowiorb.forecast( "#{position.latitude},#{position.longitude}", ["1d"], @@ -34,40 +43,67 @@ def get_forecast(date, position) ) end - def create_weather(forecast, position_id) - today = JSON.parse(forecast.body, symbolize_names: true).dig(:timelines, :daily, 0) - the_time = today.dig(:time) - today = today.dig(:values) - rain_intensity = today.dig(:rainIntensityAvg) - sleet_intensity = today.dig(:sleetIntensityAvg) - snow_intensity = today.dig(:snowIntensityAvg) - icon = get_icon_legacy(today) - summary = "General conditions are #{icon}, with an average temperature of #{today[:temperatureAvg]}." - Weather.create( - date: Date.strptime(the_time, "%Y-%m-%d"), - humidity: today.dig(:humidityAvg).round, + # The forecast endpoint takes no date: it always answers with a daily timeline + # starting at the position's today. Pick out the day that was actually asked + # for -- comparing dates in the position's own time zone, since the timeline + # stamps each day in UTC -- so that the record we store is keyed by the date + # the caller wanted and the cache above can find it again. Days outside the + # window (a back-filled check-in, say) have no forecast to store. + def daily_forecast_on(forecast, date, position) + daily = JSON.parse(forecast.body, symbolize_names: true).dig(:timelines, :daily) || [] + time_zone = NearestTimeZone.to(position.latitude.to_f, position.longitude.to_f).presence || "UTC" + + daily.find { |day| local_date(day[:time], time_zone) == date } + end + + def local_date(time, time_zone) + Time.parse(time.to_s).in_time_zone(time_zone).to_date + rescue ArgumentError + nil + end + + def create_weather(day, date, position_id) + values = day.dig(:values) + rain_intensity = values.dig(:rainIntensityAvg) + sleet_intensity = values.dig(:sleetIntensityAvg) + snow_intensity = values.dig(:snowIntensityAvg) + icon = get_icon_legacy(values) + summary = "General conditions are #{icon}, with an average temperature of #{values[:temperatureAvg]}." + weather = Weather.new( + date: date, + humidity: values.dig(:humidityAvg).round, icon: icon, position_id: position_id, precip_intensity: rain_intensity + sleet_intensity + snow_intensity, - pressure: today.dig(:pressureSurfaceLevelAvg), + pressure: values.dig(:pressureSurfaceLevelAvg), summary: summary, - temperature_max: today.dig(:temperatureMax), - temperature_min: today.dig(:temperatureMin) + temperature_max: values.dig(:temperatureMax), + temperature_min: values.dig(:temperatureMin) ) + + return weather if weather.save + + Rails.logger.warn "Could not store weather for #{date} at position #{position_id}: #{weather.errors.full_messages.to_sentence}" + + # Another request cached this day while we were fetching it. Hand back the + # record that won rather than an unsaved one, whose nil id callers would + # store as "this check-in has no weather". + Weather.find_by(date: date, position_id: position_id) end - def get_icon_legacy(today) + def get_icon_legacy(values) # Our icons do not coverage their full range of weather codes. We could pull in their icons (linked below) on the frontend to expand options # This method adapts their weather codes to our existing icons as best as possible # Icons and codes found here: https://docs.tomorrow.io/reference/data-layers-weather-codes # Icon files here: https://github.com/Tomorrow-IO-API/tomorrow-weather-codes # Daily forecast is always daytime weather codes / icons regardless of actual time - code = if today["weatherCodeMin"] - today["weatherCodeMin"] - elsif today["weatherCodeFullDay"] - today["weatherCodeFullDay"] + # The forecast body is parsed with symbolized names, so these keys are symbols + code = if values[:weatherCodeMin] + values[:weatherCodeMin] + elsif values[:weatherCodeFullDay] + values[:weatherCodeFullDay] else - today["weatherCode"] + values[:weatherCode] end case code diff --git a/backend/spec/controllers/api/v1/weathers_controller_spec.rb b/backend/spec/controllers/api/v1/weathers_controller_spec.rb index 65e52fe34..38e2cd642 100644 --- a/backend/spec/controllers/api/v1/weathers_controller_spec.rb +++ b/backend/spec/controllers/api/v1/weathers_controller_spec.rb @@ -39,6 +39,17 @@ it { is_expected.to include(*expected_keys) } it { is_expected.not_to include(*not_expected_keys) } it { expect(response).to have_http_status(:ok) } + it { expect(json_response[:weather][:id]).to eq(weather.id) } + end + + describe "index when no weather is available" do + let(:json_response) { JSON.parse(response.body, symbolize_names: true) } + + before { expect(WeatherRetriever).to receive(:get).and_return(nil) } + before { index_action } + + it { expect(response).to have_http_status(:ok) } + it { expect(json_response).to eq(weathers: []) } end end end diff --git a/backend/spec/services/checkin/creator_spec.rb b/backend/spec/services/checkin/creator_spec.rb index ad3f37d90..c46c2a89c 100644 --- a/backend/spec/services/checkin/creator_spec.rb +++ b/backend/spec/services/checkin/creator_spec.rb @@ -109,18 +109,37 @@ end end - context "when postal code is set on previous checkin" do - let(:weather) { create :weather } + context "when a location is set on the previous checkin", :vcr do + # The recorded forecast is Minneapolis from 2023-12-05 onwards, so pin "today" + # inside that window: the trackings above are only active from today. + let!(:date) { Date.parse("2023-12-05") } + let(:cassete) { "WeatherRetriever/#{postal_code}" } let(:postal_code) { "55403" } - let(:position) { Position.create(postal_code: postal_code) } + let(:position) { VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } } - let!(:previous_checkin) { create :checkin, user_id: user.id, position_id: position.id } + let!(:previous_checkin) do + create :checkin, user_id: user.id, date: date - 1.day, position_id: position.id + end + + subject { VCR.use_cassette(cassete) { Checkin::Creator.new(user.id, date).create! } } + + around { |example| travel_to(date) { example.run } } - before { expect(WeatherRetriever).to receive(:get).and_return(weather) } + before { allow(Tomorrowiorb).to receive(:api_key).and_return("MY_MEGA_TOMORROW_IO_KEY") } - it "should ask for weather" do + it "carries the location over and asks for that day's weather" do expect(subject.position.postal_code).to eq(postal_code) - expect(subject.weather_id).to eq(weather.id) + expect(subject.weather).to be_present + expect(subject.weather.date).to eq(date) + end + + context "when the weather for that date cannot be retrieved" do + before { allow(WeatherRetriever).to receive(:get).and_return(nil) } + + it "still carries the location over" do + expect(subject.position.postal_code).to eq(postal_code) + expect(subject.weather_id).to be_nil + end end end end diff --git a/backend/spec/services/weather_retriever_spec.rb b/backend/spec/services/weather_retriever_spec.rb index 381df756f..f459f46d8 100644 --- a/backend/spec/services/weather_retriever_spec.rb +++ b/backend/spec/services/weather_retriever_spec.rb @@ -6,7 +6,9 @@ # stub it here rather than depending on whatever TOMORROW_IO_KEY happens to hold. before { allow(Tomorrowiorb).to receive(:api_key).and_return("MY_MEGA_TOMORROW_IO_KEY") } - let(:date) { Date.parse "2016-01-06" } + # The cassette was recorded for Minneapolis (America/Chicago) and carries the + # six daily forecasts from 2023-12-05 to 2023-12-10. + let(:date) { Date.parse "2023-12-05" } let(:cassete) { "#{described_class.name}/#{postal_code}" } let(:postal_code) { "55403" } @@ -18,11 +20,53 @@ context "no weather cached" do it { expect(perform).to be_a(Weather) } + it { expect(perform).to be_persisted } it { expect { perform }.to change { Weather.count }.by(1) } + + it "stores the weather under the date that was asked for" do + expect(perform.date).to eq(date) + end + + it "stores the forecast for that date" do + weather = perform + + expect(weather.icon).to eq("cloudy") + expect(weather.summary).to eq("General conditions are cloudy, with an average temperature of -0.76.") + expect(weather.temperature_min).to eq(-1.83) + expect(weather.temperature_max).to eq(0.64) + expect(weather.humidity).to eq(81) + expect(weather.pressure).to eq(991.51) + expect(weather.precip_intensity).to eq(0) + end + + it "stores it against the position for the postal code" do + expect(perform.position.postal_code).to eq(postal_code) + end + + context "when the caller passes a time rather than a date" do + let(:date) { DateTime.new(2023, 12, 5, 17, 43, 12) } + + it "stores the weather under that day" do + expect(perform.date).to eq(Date.parse("2023-12-05")) + end + end + + context "for a later day in the forecast window" do + let(:date) { Date.parse "2023-12-07" } + + it "stores that day's forecast, not the first day's" do + weather = perform + + expect(weather.date).to eq(date) + expect(weather.icon).to eq("clear-day") + expect(weather.temperature_min).to eq(-1.87) + expect(weather.temperature_max).to eq(8.17) + end + end end context "the weather is already cached" do - let(:position) { Position.create(postal_code: postal_code) } + let(:position) { VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } } let!(:weather) { create :weather, date: date, position_id: position.id } before { expect(Weather).not_to receive(:create) } @@ -32,4 +76,113 @@ it { expect(perform).to eq(weather) } it { expect { perform }.not_to change { Weather.count } } end + + # The pre-fix retriever cached every forecast under the date the API answered + # with instead of the date it was asked for, so the lookup above never hit, and + # the second call tripped the date/position uniqueness validation and returned + # an unsaved record whose nil id callers stored as "no weather". + context "asking twice for the same date" do + it "serves the second call from the cache" do + first = perform + second = perform + + expect(second).to be_persisted + expect(second.id).to eq(first.id) + expect(Weather.count).to eq(1) + end + end + + context "another date is already cached for the position" do + let(:other_date) { Date.parse "2023-12-06" } + + it "stores and returns a persisted record for the new date" do + cached = VCR.use_cassette(cassete) { described_class.get(other_date, postal_code) } + weather = perform + + expect(weather).to be_persisted + expect(weather.id).not_to eq(cached.id) + expect(weather.date).to eq(date) + expect(cached.reload.date).to eq(other_date) + end + end + + # The forecast endpoint only covers today onwards, so a back-filled check-in has + # no forecast to store. Storing whatever the API did answer with would file + # another day's weather under this date. + context "the requested date is outside the forecast window" do + let(:date) { Date.parse "2023-12-01" } + + it { expect(perform).to be_nil } + it { expect { perform }.not_to change { Weather.count } } + end + + context "the position's day differs from the UTC day" do + # Sydney is UTC+11 in December: the daily forecast stamped 2023-12-05T20:00:00Z + # is 2023-12-06 there, and that is the date the check-in was filed under. + let(:date) { Date.parse "2023-12-06" } + let(:postal_code) { "2000" } + + let(:forecast_body) do + { + timelines: { + daily: [ + {time: "2023-12-05T20:00:00Z", values: { + weatherCodeMin: 4000, humidityAvg: 62.4, pressureSurfaceLevelAvg: 1011.2, + rainIntensityAvg: 0.5, sleetIntensityAvg: 0, snowIntensityAvg: 0, + temperatureAvg: 22.1, temperatureMin: 18.3, temperatureMax: 26.7 + }} + ] + } + }.to_json + end + + before do + allow(Geocoder).to receive(:search).with(postal_code).and_return( + [double(city: "Sydney", state: "New South Wales", province: nil, country: "Australia", + latitude: -33.8688, longitude: 151.2093)] + ) + allow(Tomorrowiorb).to receive(:forecast).and_return( + Tomorrowiorb::TomorrowioResponse.new(200, {}, forecast_body) + ) + end + + subject { described_class.get(date, postal_code) } + + it "stores the forecast under the local date" do + expect(subject.date).to eq(date) + expect(subject.icon).to eq("rain") + expect(subject.temperature_max).to eq(26.7) + end + + it "serves the next call for that date from the cache" do + first = subject + + expect(described_class.get(date, postal_code).id).to eq(first.id) + end + end + + context "the postal code cannot be geocoded" do + let(:postal_code) { "not a place" } + + before { allow(Geocoder).to receive(:search).with(postal_code).and_return([]) } + before { expect(Tomorrowiorb).not_to receive(:forecast) } + + it { expect(described_class.get(date, postal_code)).to be_nil } + end + + context "the forecast API fails" do + before do + allow(Tomorrowiorb).to receive(:forecast).and_return( + Tomorrowiorb::TomorrowioResponse.new(429, {}, "rate limit exceeded") + ) + end + + it "returns nothing rather than an unsaved record" do + position = VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } + + expect(position).to be_persisted + expect(described_class.get(date, postal_code)).to be_nil + expect(Weather.count).to eq(0) + end + end end diff --git a/frontend/app/components/checkin/weather-step.js b/frontend/app/components/checkin/weather-step.js index 9a06c4c15..4fb7445ca 100644 --- a/frontend/app/components/checkin/weather-step.js +++ b/frontend/app/components/checkin/weather-step.js @@ -1,8 +1,10 @@ import Ember from 'ember'; -let { Component, computed, computed: { alias, notEmpty }, get, set, setProperties } = Ember; +let { Component, computed, computed: { alias, notEmpty }, get, inject: { service }, isBlank, set, setProperties } = Ember; export default Component.extend({ + store: service(), + classNames: ['centered'], weatherTypes: [ 'clear-day', @@ -18,10 +20,11 @@ export default Component.extend({ ], newPostalCode: '', - inputVisible: false, + manuallyOpened: false, validPostalCode: true, checkin: alias('parentView.model.checkin'), + hasLocation: notEmpty('checkin.locationName'), hasWeather: notEmpty('weather'), pressureUnits: alias('session.currentUser.profile.pressureUnits'), temperatureUnits: alias('session.currentUser.profile.temperatureUnits'), @@ -49,43 +52,50 @@ export default Component.extend({ return get(this, 'weatherTypes').includes(icon) ? icon : 'default'; }), - willRender() { - this._super(...arguments); - - if (!get(this, 'hasWeather')) { - set(this, 'inputVisible', true); - } - }, + // The location lives on the check-in and is carried over to the next one, so we + // only ask for it while the check-in has none, or while the user is changing it. + // A day without weather is not a day without a location: the forecast may just + // not be available for it. + inputVisible: computed('hasLocation', 'manuallyOpened', function() { + return get(this, 'manuallyOpened') || !get(this, 'hasLocation'); + }), actions: { updatePostalCode() { - const date = get(this, 'checkin.date'); + const checkin = get(this, 'checkin'); const newPostalCode = get(this, 'newPostalCode'); - if(get(newPostalCode, 'length') === 0) { + if(isBlank(newPostalCode)) { set(this, 'validPostalCode', false); - } else { - this - .store - .queryRecord('weather', { date: date, postal_code: newPostalCode }) - .then(record => { - let checkin = get(this, 'checkin'); - setProperties(checkin, { postalCode: newPostalCode, weather: record }); - - - if(!record) { - set(this, 'validPostalCode', false); - } - - return checkin.save(); - }) - .then(() => set(this, 'inputVisible', false)); + return; } + + return get(this, 'store') + .queryRecord('weather', { date: get(checkin, 'date'), postal_code: newPostalCode }) + // Weather can be missing for a day (no forecast, API down) without the + // location being wrong, so save the location either way. + .catch(() => null) + .then(record => { + setProperties(checkin, { postalCode: newPostalCode, weather: record }); + + return checkin.save(); + }) + .then(() => { + // The API only echoes a postal code back once it has geocoded it into a + // position, so this is what tells us the location itself was understood. + const accepted = get(checkin, 'postalCode') === newPostalCode; + + setProperties(this, { validPostalCode: accepted, manuallyOpened: !accepted }); + }); }, showInput() { - setProperties(this, { inputVisible: true, newPostalCode: get(this, 'checkin.postalCode') }); + setProperties(this, { + manuallyOpened: true, + validPostalCode: true, + newPostalCode: get(this, 'checkin.postalCode'), + }); }, toggleTemperatureUnits() { diff --git a/frontend/app/templates/components/checkin/weather-step.hbs b/frontend/app/templates/components/checkin/weather-step.hbs index 8714ff53d..7afc9b8d4 100644 --- a/frontend/app/templates/components/checkin/weather-step.hbs +++ b/frontend/app/templates/components/checkin/weather-step.hbs @@ -2,12 +2,14 @@ {{#if inputVisible}}
- {{#if hasWeather}} - Set location: - {{else if validPostalCode }} - To enable weather tracking, -
- enter your location below + {{#if validPostalCode}} + {{#if hasLocation}} + Set location: + {{else}} + To enable weather tracking, +
+ enter your location below + {{/if}} {{else}} We couldn't find that location
@@ -56,4 +58,6 @@
+{{else if hasLocation}} +
Weather data is not available for this day.
{{/if}} diff --git a/frontend/tests/integration/components/checkin/weather-step-test.js b/frontend/tests/integration/components/checkin/weather-step-test.js index d58799b94..c43da0088 100644 --- a/frontend/tests/integration/components/checkin/weather-step-test.js +++ b/frontend/tests/integration/components/checkin/weather-step-test.js @@ -1,19 +1,215 @@ +import Ember from 'ember'; import { moduleForComponent, test } from 'ember-qunit'; +import { settled } from '@ember/test-helpers'; import hbs from 'htmlbars-inline-precompile'; +import WeatherStep from 'flaredown/components/checkin/weather-step'; + +const { RSVP, get, set, setProperties } = Ember; + +const LOCATION_NAME = 'Minneapolis, Minnesota, United States'; + +// Stands in for the check-in record. `save()` behaves like the API, which only +// echoes a postal code and a location name back once it has managed to geocode +// the postal code into a position. +function checkinStub(attrs) { + return Ember.Object.create({ + date: '2023-12-05', + postalCode: null, + locationName: null, + weather: null, + geocodable: true, + + save() { + if (get(this, 'geocodable')) { + set(this, 'locationName', LOCATION_NAME); + } else { + setProperties(this, { postalCode: null, locationName: null }); + } + + return RSVP.resolve(this); + }, + }, attrs || {}); +} + +function weatherStub() { + return Ember.Object.create({ + icon: 'cloudy', + summary: 'General conditions are cloudy.', + humidity: 81, + precipIntensity: 0.25, + temperatureMinByUnits() { return 28; }, + temperatureMaxByUnits() { return 33; }, + pressureByUnits() { return 29.28; }, + }); +} + +function text(context) { + return context.$().text().trim().replace(/\s+/g, ' '); +} + +let weatherQueries; +let weatherResponse; moduleForComponent('checkin/weather-step', 'Integration | Component | checkin/weather step', { - integration: true -}); + integration: true, -test('it renders', function(assert) { + beforeEach() { + weatherQueries = []; + weatherResponse = () => RSVP.resolve(null); - // Set any properties with this.set('myProperty', 'value'); - // Handle any actions with this.on('myAction', function(val) { ... }); + // The check-in wizard passes the check-in down through `parentView.model`. + // Override that alias so these tests can hand one straight to the component. + this.register('component:checkin/weather-step', WeatherStep.extend({ checkin: null })); - this.render(hbs`{{checkin/weather-step}}`); + this.register('service:store', Ember.Service.extend({ + queryRecord(modelName, query) { + weatherQueries.push([modelName, query]); + + return weatherResponse(); + } + })); + } +}); + +test('it asks for a location when the check-in has none', function(assert) { + this.set('checkin', checkinStub()); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); assert.equal( - this.$().text().trim().replace(/\s+/g, ' '), + text(this), 'Weather To enable weather tracking, enter your location below Submit' ); + assert.equal(this.$('input').length, 1, 'the location input is shown'); +}); + +test('it shows the saved location instead of asking again when weather is missing', function(assert) { + this.set('checkin', checkinStub({ postalCode: '55403', locationName: LOCATION_NAME })); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + + assert.equal(this.$('input').length, 0, 'the location input stays hidden'); + assert.equal( + text(this), + `Weather ${LOCATION_NAME} Weather data is not available for this day.` + ); +}); + +test('it shows the weather for a check-in that has some', function(assert) { + this.set('checkin', checkinStub({ + postalCode: '55403', + locationName: LOCATION_NAME, + weather: weatherStub(), + })); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + + assert.equal(this.$('input').length, 0, 'the location input stays hidden'); + assert.equal(this.$('.measurement').length, 5, 'the measurements are shown'); + assert.equal( + text(this).indexOf('Weather data is not available'), + -1, + 'no unavailable notice is shown' + ); +}); + +test('clicking the saved location reopens the input, prefilled', function(assert) { + this.set('checkin', checkinStub({ postalCode: '55403', locationName: LOCATION_NAME })); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('.clickable').click(); + + assert.equal(this.$('input').val(), '55403', 'the input is prefilled with the postal code'); + assert.ok(text(this).indexOf('Set location:') > -1, 'the copy is about changing the location'); +}); + +test('submitting a blank location reports it as not found', function(assert) { + this.set('checkin', checkinStub()); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('.save-status').click(); + + assert.deepEqual(weatherQueries, [], 'no weather is requested'); + assert.ok( + text(this).indexOf("We couldn't find that location") > -1, + 'the location is reported as not found' + ); +}); + +test('submitting a location saves it and hides the input', function(assert) { + const checkin = checkinStub(); + + weatherResponse = () => RSVP.resolve(weatherStub()); + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('input').val('55403').trigger('input'); + this.$('.save-status').click(); + + return settled().then(() => { + assert.deepEqual( + weatherQueries, + [['weather', { date: '2023-12-05', postal_code: '55403' }]], + 'the weather for the check-in date and postal code is requested' + ); + assert.equal(get(checkin, 'postalCode'), '55403', 'the postal code is saved on the check-in'); + assert.equal(this.$('input').length, 0, 'the location input is hidden'); + assert.equal(this.$('.measurement').length, 5, 'the measurements are shown'); + assert.ok(text(this).indexOf(LOCATION_NAME) > -1, 'the saved location is shown'); + }); +}); + +test('it keeps the location when there is no weather for the day', function(assert) { + const checkin = checkinStub(); + + // The API answers with no weather when it has no forecast for the day. + weatherResponse = () => RSVP.resolve(null); + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('input').val('55403').trigger('input'); + this.$('.save-status').click(); + + return settled().then(() => { + assert.equal(get(checkin, 'postalCode'), '55403', 'the postal code is still saved'); + assert.equal(this.$('input').length, 0, 'the location input is hidden'); + assert.equal( + text(this), + `Weather ${LOCATION_NAME} Weather data is not available for this day.` + ); + }); +}); + +test('it keeps the location when the weather request fails', function(assert) { + const checkin = checkinStub(); + + weatherResponse = () => RSVP.reject(new Error('500 from the weather endpoint')); + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('input').val('55403').trigger('input'); + this.$('.save-status').click(); + + return settled().then(() => { + assert.equal(get(checkin, 'postalCode'), '55403', 'the postal code is still saved'); + assert.equal(this.$('input').length, 0, 'the location input is hidden'); + }); +}); + +test('a location the API cannot geocode is reported as not found', function(assert) { + const checkin = checkinStub({ geocodable: false }); + + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('input').val('nowhere').trigger('input'); + this.$('.save-status').click(); + + return settled().then(() => { + assert.equal(this.$('input').length, 1, 'the location input stays open'); + assert.ok( + text(this).indexOf("We couldn't find that location") > -1, + 'the location is reported as not found' + ); + }); }); From 59fee26c7ddcf047cdfca1102e6a623dd963883e Mon Sep 17 00:00:00 2001 From: Cory Musick Date: Wed, 2 Sep 2026 20:18:04 -0400 Subject: [PATCH 2/2] Harden weather retrieval and location updates --- backend/app/services/checkin/updater.rb | 47 +++- backend/app/services/weather_retriever.rb | 102 +++++++-- .../initializers/filter_parameter_logging.rb | 8 +- .../config/filter_parameter_logging_spec.rb | 20 ++ backend/spec/services/checkin/updater_spec.rb | 107 +++++++++ .../spec/services/weather_retriever_spec.rb | 205 +++++++++++++++++- .../app/components/checkin/weather-step.js | 8 +- .../components/checkin/weather-step-test.js | 44 +++- 8 files changed, 509 insertions(+), 32 deletions(-) create mode 100644 backend/spec/config/filter_parameter_logging_spec.rb diff --git a/backend/app/services/checkin/updater.rb b/backend/app/services/checkin/updater.rb index 3d0d5e8a7..6f592302e 100644 --- a/backend/app/services/checkin/updater.rb +++ b/backend/app/services/checkin/updater.rb @@ -25,7 +25,25 @@ def initialize(current_user, params) end def update! - checkin.update!(permitted_params.except(:postal_code)) + position = requested_position + update_params = permitted_params.except(:postal_code) + + if location_requested? + if position.persisted? + update_params[:position_id] = position.id + if position.id != checkin.position_id || update_params.key?(:weather_id) + update_params[:weather_id] = matching_weather_id(update_params[:weather_id], position.id) + end + else + # A rejected replacement must not detach weather that still belongs to + # the existing, valid check-in location. + update_params = update_params.except(:weather_id) + end + elsif update_params.key?(:weather_id) + update_params[:weather_id] = matching_weather_id(update_params[:weather_id], checkin.position_id) + end + + checkin.update!(update_params) if checkin.date.today? save_most_recent_doses @@ -33,18 +51,31 @@ def update! end update_trackable_usages - position = Position.find_or_create_by(postal_code: permitted_params[:postal_code]) - - if position.persisted? - checkin.position_id = position.id - checkin.save! - end - checkin end private + def location_requested? + permitted_params.key?(:postal_code) + end + + def requested_position + return unless location_requested? + + Position.find_or_create_by(postal_code: permitted_params[:postal_code]) + end + + def matching_weather_id(weather_id, position_id) + return if weather_id.blank? + + Weather.find_by( + id: weather_id, + position_id: position_id, + date: checkin.date.to_date + )&.id + end + def update_trackables_positions(params) %w[Condition Symptom Treatment].each do |trackable_class_name| update_trackables_positions_on_destroy(trackable_class_name, params) diff --git a/backend/app/services/weather_retriever.rb b/backend/app/services/weather_retriever.rb index fbdd13398..5ae1ee531 100644 --- a/backend/app/services/weather_retriever.rb +++ b/backend/app/services/weather_retriever.rb @@ -1,40 +1,80 @@ +require "digest" + class WeatherRetriever + FORECAST_MISS_TTL = 5.minutes + class << self def get(date, postal_code) date = date.to_date - position = Position.find_or_create_by(postal_code: postal_code) + position = find_or_create_position(postal_code) + + if position.persisted? + weather = Weather.find_by(date: date, position_id: position.id) + + return weather if weather.present? + end if position&.latitude.blank? || position&.longitude.blank? - Rails.logger.warn "No coordinates found for postal_code #{postal_code}: #{position.inspect}" + Rails.logger.warn "No coordinates found for weather position" return end - weather = Weather.find_by(date: date, position_id: position.id) + return if forecast_miss_cached?(date, position.id) - return weather if weather.present? + # This row lock is the short-term concurrency guard for a cache fill. The + # existing unique index on (date, postal_code) cannot arbitrate these writes + # because current records are keyed by position_id and leave postal_code nil. + # Long term, deduplicate existing rows and replace it with a unique index on + # (date, position_id), which will enforce the invariant for every writer. + position.with_lock do + weather = Weather.find_by(date: date, position_id: position.id) - forecast = get_forecast(position) + return weather if weather.present? + return if forecast_miss_cached?(date, position.id) - if forecast.status != 200 - Rails.logger.warn "No forecast found for position #{position.inspect}: response code was #{forecast.status}, headers were #{forecast.headers}, body contained #{forecast.body}" + if historical_date?(date, position) + Rails.logger.warn "No forecast for #{date} at position #{position.id}: the date is before the position's current day" - return - end + return + end - day = daily_forecast_on(forecast, date, position) + forecast = get_forecast(position) - if day.blank? - Rails.logger.warn "No forecast for #{date} at position #{position.inspect}: the forecast endpoint only covers today onwards" + if forecast.status != 200 + Rails.logger.warn "No forecast found for position #{position.id}: response code was #{forecast.status}" + cache_forecast_miss(date, position.id) - return - end + return + end + + day = daily_forecast_on(forecast, date, position) + + if day.blank? + Rails.logger.warn "No forecast for #{date} at position #{position.id}: the date is outside the forecast window" + cache_forecast_miss(date, position.id) - create_weather(day, date, position.id) + return + end + + create_weather(day, date, position.id) + end end private + # Position has no unique postal_code index, so a row lock cannot protect the + # instant before that row exists. A transaction-scoped advisory lock on a + # one-way location hash makes first creation converge on one row without + # putting the submitted address in SQL logs. + def find_or_create_position(postal_code) + Position.transaction(requires_new: true) do + lock_id = Digest::SHA256.digest(postal_code.to_s).unpack1("q>") + Position.connection.execute("SELECT pg_advisory_xact_lock(#{lock_id})") + Position.find_or_create_by(postal_code: postal_code) + end + end + def get_forecast(position) Tomorrowiorb.forecast( "#{position.latitude},#{position.longitude}", @@ -51,11 +91,35 @@ def get_forecast(position) # window (a back-filled check-in, say) have no forecast to store. def daily_forecast_on(forecast, date, position) daily = JSON.parse(forecast.body, symbolize_names: true).dig(:timelines, :daily) || [] - time_zone = NearestTimeZone.to(position.latitude.to_f, position.longitude.to_f).presence || "UTC" + time_zone = time_zone_for(position) daily.find { |day| local_date(day[:time], time_zone) == date } end + def historical_date?(date, position) + date < Time.current.in_time_zone(time_zone_for(position)).to_date + end + + def time_zone_for(position) + NearestTimeZone.to(position.latitude.to_f, position.longitude.to_f).presence || "UTC" + end + + def forecast_miss_cached?(date, position_id) + Rails.cache.read(forecast_miss_cache_key(date, position_id)) == true + end + + def cache_forecast_miss(date, position_id) + Rails.cache.write( + forecast_miss_cache_key(date, position_id), + true, + expires_in: FORECAST_MISS_TTL + ) + end + + def forecast_miss_cache_key(date, position_id) + "weather_retriever/forecast_miss/#{position_id}/#{date.iso8601}" + end + def local_date(time, time_zone) Time.parse(time.to_s).in_time_zone(time_zone).to_date rescue ArgumentError @@ -85,9 +149,9 @@ def create_weather(day, date, position_id) Rails.logger.warn "Could not store weather for #{date} at position #{position_id}: #{weather.errors.full_messages.to_sentence}" - # Another request cached this day while we were fetching it. Hand back the - # record that won rather than an unsaved one, whose nil id callers would - # store as "this check-in has no weather". + # Do not hand callers an unsaved record whose nil id would be persisted as + # "this check-in has no weather". The lookup also tolerates a writer that + # does not participate in the position-row locking protocol above. Weather.find_by(date: date, position_id: position_id) end diff --git a/backend/config/initializers/filter_parameter_logging.rb b/backend/config/initializers/filter_parameter_logging.rb index 3435964bb..1ef2ce37a 100644 --- a/backend/config/initializers/filter_parameter_logging.rb +++ b/backend/config/initializers/filter_parameter_logging.rb @@ -3,6 +3,12 @@ # Configure parameters to be partially matched (e.g. passw matches password) and filtered from the log file. # Use this to limit dissemination of sensitive information. # See the ActiveSupport::ParameterFilter documentation for supported notations and behaviors. +location_filters = [:postal_code, :latitude, :longitude] + Rails.application.config.filter_parameters += [ - :password, :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn + :password, :passw, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, + *location_filters ] + +# Active Record filters SQL bind values separately from request parameters. +ActiveRecord::Base.filter_attributes += location_filters diff --git a/backend/spec/config/filter_parameter_logging_spec.rb b/backend/spec/config/filter_parameter_logging_spec.rb new file mode 100644 index 000000000..f68129a3c --- /dev/null +++ b/backend/spec/config/filter_parameter_logging_spec.rb @@ -0,0 +1,20 @@ +require "rails_helper" + +RSpec.describe "parameter filtering" do + it "redacts submitted locations and derived coordinates from application logs" do + filter = ActiveSupport::ParameterFilter.new(Rails.application.config.filter_parameters) + + location = { + "postal_code" => "123 Main Street", + "latitude" => 44.967486, + "longitude" => -93.2897678 + } + + expect(filter.filter(location)).to eq( + "postal_code" => "[FILTERED]", + "latitude" => "[FILTERED]", + "longitude" => "[FILTERED]" + ) + expect(Position.filter_attributes.map(&:to_s)).to include("postal_code", "latitude", "longitude") + end +end diff --git a/backend/spec/services/checkin/updater_spec.rb b/backend/spec/services/checkin/updater_spec.rb index 79a56b468..cb7a19458 100644 --- a/backend/spec/services/checkin/updater_spec.rb +++ b/backend/spec/services/checkin/updater_spec.rb @@ -22,6 +22,113 @@ end end + context "when changing the check-in location" do + let(:original_position) { Position.create!(postal_code: "55403") } + let(:weather) { create(:weather, date: checkin.date.to_date, position_id: original_position.id) } + + before do + checkin.update!(position_id: original_position.id, weather_id: weather.id) + end + + context "when the replacement location cannot be geocoded" do + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {note: "Updated note", postal_code: "not a place", weather_id: nil} + ) + end + + before { allow(Geocoder).to receive(:search).with("not a place").and_return([]) } + + it "preserves the existing position and weather while saving other changes" do + updated = subject + + expect(updated.note).to eq("Updated note") + expect(updated.position_id).to eq(original_position.id) + expect(updated.weather_id).to eq(weather.id) + end + end + + context "when the replacement location is valid but has no weather" do + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {postal_code: "10001", weather_id: nil} + ) + end + + it "updates the position and clears weather from the old location" do + updated = subject + + expect(updated.position_id).not_to eq(original_position.id) + expect(updated.weather_id).to be_nil + end + end + + context "when a partial request omits weather for a valid replacement location" do + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {postal_code: "10001"} + ) + end + + it "does not retain weather from the old location" do + expect(subject.weather_id).to be_nil + end + end + + context "when a partial request resubmits the current valid location" do + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {postal_code: original_position.postal_code} + ) + end + + it "preserves the current weather" do + expect(subject.weather_id).to eq(weather.id) + end + end + + context "when submitted weather belongs to a different position" do + let(:other_position) { Position.create!(postal_code: "90210") } + let(:other_weather) do + create(:weather, date: checkin.date.to_date, postal_code: nil, position_id: other_position.id) + end + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {postal_code: "10001", weather_id: other_weather.id} + ) + end + + it "does not attach weather from the wrong location" do + expect(subject.weather_id).to be_nil + end + end + + context "when submitted weather matches the replacement location and date" do + let(:replacement_position) { Position.create!(postal_code: "10001") } + let(:replacement_weather) do + create(:weather, date: checkin.date.to_date, postal_code: nil, position_id: replacement_position.id) + end + let(:params) do + ActionController::Parameters.new( + id: checkin.id.to_s, + checkin: {postal_code: replacement_position.postal_code, weather_id: replacement_weather.id} + ) + end + + it "attaches the matching weather" do + updated = subject + + expect(updated.position_id).to eq(replacement_position.id) + expect(updated.weather_id).to eq(replacement_weather.id) + end + end + end + context "when recent dose exists for treatment in user's profile" do let(:treatment) { create(:treatment) } diff --git a/backend/spec/services/weather_retriever_spec.rb b/backend/spec/services/weather_retriever_spec.rb index f459f46d8..5640d9759 100644 --- a/backend/spec/services/weather_retriever_spec.rb +++ b/backend/spec/services/weather_retriever_spec.rb @@ -6,18 +6,69 @@ # stub it here rather than depending on whatever TOMORROW_IO_KEY happens to hold. before { allow(Tomorrowiorb).to receive(:api_key).and_return("MY_MEGA_TOMORROW_IO_KEY") } + around do |example| + travel_to(Time.zone.local(2023, 12, 5, 12)) { example.run } + end + # The cassette was recorded for Minneapolis (America/Chicago) and carries the # six daily forecasts from 2023-12-05 to 2023-12-10. let(:date) { Date.parse "2023-12-05" } let(:cassete) { "#{described_class.name}/#{postal_code}" } let(:postal_code) { "55403" } - let(:perform) do + def perform VCR.use_cassette cassete do described_class.get(date, postal_code) end end + def concurrent_retrieval(forecast_body) + first_forecast_started = Queue.new + release_first_forecast = Queue.new + second_forecast_started = Queue.new + call_count = 0 + call_count_lock = Mutex.new + response = Tomorrowiorb::TomorrowioResponse.new(200, {}, forecast_body) + + allow(Tomorrowiorb).to receive(:forecast) do + this_call = call_count_lock.synchronize { call_count += 1 } + + if this_call == 1 + first_forecast_started << true + release_first_forecast.pop + else + second_forecast_started << true + end + + response + end + + retrieve = lambda do + ActiveRecord::Base.connection_pool.with_connection do + described_class.get(date, postal_code) + end + end + + first_request = Thread.new { retrieve.call } + first_forecast_started.pop + second_request = Thread.new { retrieve.call } + + second_reached_vendor = begin + Timeout.timeout(0.5) { second_forecast_started.pop } + true + rescue Timeout::Error + false + ensure + release_first_forecast << true + end + + { + second_reached_vendor: second_reached_vendor, + call_count: call_count, + weathers: [first_request.value, second_request.value] + } + end + context "no weather cached" do it { expect(perform).to be_a(Weather) } it { expect(perform).to be_persisted } @@ -77,6 +128,33 @@ it { expect { perform }.not_to change { Weather.count } } end + context "historical weather is already cached" do + let(:date) { Date.parse "2023-12-01" } + let(:position) { VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } } + let!(:weather) { create :weather, date: date, position_id: position.id } + + before { expect(Tomorrowiorb).not_to receive(:forecast) } + + it "returns the stored weather for an old check-in" do + expect(perform).to eq(weather) + end + end + + context "weather is cached against a legacy position without coordinates" do + let(:position) do + Position.create!(postal_code: postal_code).tap do |record| + record.update_columns(latitude: nil, longitude: nil) + end + end + let!(:weather) { create :weather, date: date, position_id: position.id } + + before { expect(Tomorrowiorb).not_to receive(:forecast) } + + it "returns the stored weather without requiring coordinates" do + expect(perform).to eq(weather) + end + end + # The pre-fix retriever cached every forecast under the date the API answered # with instead of the date it was asked for, so the lookup above never hit, and # the second call tripped the date/position uniqueness validation and returned @@ -92,6 +170,96 @@ end end + context "two requests arrive before the weather is cached", :js do + let!(:position) { Position.create!(postal_code: postal_code) } + let(:forecast_body) do + { + timelines: { + daily: [ + {time: "2023-12-05T06:00:00Z", values: { + weatherCodeMin: 1102, humidityAvg: 81.2, pressureSurfaceLevelAvg: 991.51, + rainIntensityAvg: 0, sleetIntensityAvg: 0, snowIntensityAvg: 0, + temperatureAvg: -0.76, temperatureMin: -1.83, temperatureMax: 0.64 + }} + ] + } + }.to_json + end + + it "serializes the cache fill and only requests one vendor forecast" do + result = concurrent_retrieval(forecast_body) + + expect(result[:second_reached_vendor]).to be(false) + expect(result[:call_count]).to eq(1) + expect(result[:weathers].map(&:id).uniq.length).to eq(1) + expect(Weather.where(date: date, position_id: position.id).count).to eq(1) + end + + context "when the position is being created by the requests" do + before { position.destroy! } + + it "creates one position and only requests one vendor forecast" do + first_geocode_started = Queue.new + release_first_geocode = Queue.new + second_geocode_started = Queue.new + geocode_count = 0 + geocode_count_lock = Mutex.new + forecast_count = 0 + forecast_count_lock = Mutex.new + geocode_result = double( + city: "Minneapolis", state: "Minnesota", province: nil, + country: "United States", latitude: 44.967486, longitude: -93.2897678 + ) + response = Tomorrowiorb::TomorrowioResponse.new(200, {}, forecast_body) + + allow(Geocoder).to receive(:search) do + this_call = geocode_count_lock.synchronize { geocode_count += 1 } + + if this_call == 1 + first_geocode_started << true + release_first_geocode.pop + else + second_geocode_started << true + end + + [geocode_result] + end + allow(Tomorrowiorb).to receive(:forecast) do + forecast_count_lock.synchronize { forecast_count += 1 } + response + end + + retrieve = lambda do + ActiveRecord::Base.connection_pool.with_connection do + described_class.get(date, postal_code) + end + end + + first_request = Thread.new { retrieve.call } + first_geocode_started.pop + second_request = Thread.new { retrieve.call } + + second_reached_geocoder = begin + Timeout.timeout(0.5) { second_geocode_started.pop } + true + rescue Timeout::Error + false + ensure + release_first_geocode << true + end + + weathers = [first_request.value, second_request.value] + resolved_positions = Position.where(postal_code: postal_code) + + expect(second_reached_geocoder).to be(false) + expect(forecast_count).to eq(1) + expect(weathers.map(&:id).uniq.length).to eq(1) + expect(resolved_positions.count).to eq(1) + expect(Weather.where(date: date, position_id: resolved_positions.first.id).count).to eq(1) + end + end + end + context "another date is already cached for the position" do let(:other_date) { Date.parse "2023-12-06" } @@ -112,10 +280,26 @@ context "the requested date is outside the forecast window" do let(:date) { Date.parse "2023-12-01" } + before { expect(Tomorrowiorb).not_to receive(:forecast) } + it { expect(perform).to be_nil } it { expect { perform }.not_to change { Weather.count } } end + context "a future date is outside the forecast window" do + let(:date) { Date.parse "2023-12-20" } + let(:miss_cache) { ActiveSupport::Cache::MemoryStore.new } + + before { allow(Rails).to receive(:cache).and_return(miss_cache) } + + it "temporarily caches the miss instead of spending quota again" do + expect(Tomorrowiorb).to receive(:forecast).once.and_call_original + + expect(perform).to be_nil + expect(perform).to be_nil + end + end + context "the position's day differs from the UTC day" do # Sydney is UTC+11 in December: the daily forecast stamped 2023-12-05T20:00:00Z # is 2023-12-06 there, and that is the date the check-in was filed under. @@ -168,6 +352,14 @@ before { expect(Tomorrowiorb).not_to receive(:forecast) } it { expect(described_class.get(date, postal_code)).to be_nil } + + it "does not write the submitted location to the application log" do + expect(Rails.logger).to receive(:warn) do |message| + expect(message).not_to include(postal_code) + end + + described_class.get(date, postal_code) + end end context "the forecast API fails" do @@ -184,5 +376,16 @@ expect(described_class.get(date, postal_code)).to be_nil expect(Weather.count).to eq(0) end + + it "identifies the position without logging its location data" do + position = VCR.use_cassette(cassete) { Position.create(postal_code: postal_code) } + + expect(Rails.logger).to receive(:warn) do |message| + expect(message).to include("position #{position.id}") + expect(message).not_to include(postal_code, position.latitude.to_s, position.longitude.to_s) + end + + described_class.get(date, postal_code) + end end end diff --git a/frontend/app/components/checkin/weather-step.js b/frontend/app/components/checkin/weather-step.js index 4fb7445ca..49981896b 100644 --- a/frontend/app/components/checkin/weather-step.js +++ b/frontend/app/components/checkin/weather-step.js @@ -64,6 +64,8 @@ export default Component.extend({ updatePostalCode() { const checkin = get(this, 'checkin'); const newPostalCode = get(this, 'newPostalCode'); + const existingPostalCode = get(checkin, 'postalCode'); + const existingWeather = get(checkin, 'weather'); if(isBlank(newPostalCode)) { set(this, 'validPostalCode', false); @@ -74,8 +76,10 @@ export default Component.extend({ return get(this, 'store') .queryRecord('weather', { date: get(checkin, 'date'), postal_code: newPostalCode }) // Weather can be missing for a day (no forecast, API down) without the - // location being wrong, so save the location either way. - .catch(() => null) + // location being wrong, so save the location either way. If this is only + // a retry of the saved location, however, a request failure must not + // erase weather the check-in already has. + .catch(() => newPostalCode === existingPostalCode ? existingWeather : null) .then(record => { setProperties(checkin, { postalCode: newPostalCode, weather: record }); diff --git a/frontend/tests/integration/components/checkin/weather-step-test.js b/frontend/tests/integration/components/checkin/weather-step-test.js index c43da0088..1cda6d4f5 100644 --- a/frontend/tests/integration/components/checkin/weather-step-test.js +++ b/frontend/tests/integration/components/checkin/weather-step-test.js @@ -117,7 +117,7 @@ test('clicking the saved location reopens the input, prefilled', function(assert this.set('checkin', checkinStub({ postalCode: '55403', locationName: LOCATION_NAME })); this.render(hbs`{{checkin/weather-step checkin=checkin}}`); - this.$('.clickable').click(); + this.$('.grey.clickable').click(); assert.equal(this.$('input').val(), '55403', 'the input is prefilled with the postal code'); assert.ok(text(this).indexOf('Set location:') > -1, 'the copy is about changing the location'); @@ -196,6 +196,48 @@ test('it keeps the location when the weather request fails', function(assert) { }); }); +test('retrying the saved location does not erase weather when the request fails', function(assert) { + const existingWeather = weatherStub(); + const checkin = checkinStub({ + postalCode: '55403', + locationName: LOCATION_NAME, + weather: existingWeather, + }); + + weatherResponse = () => RSVP.reject(new Error('500 from the weather endpoint')); + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('.grey.clickable').click(); + this.$('.save-status').click(); + + return settled().then(() => { + assert.equal(get(checkin, 'weather'), existingWeather, 'the existing weather is preserved'); + assert.equal(this.$('.measurement').length, 5, 'the existing measurements remain visible'); + }); +}); + +test('changing location clears weather from the old location when the request fails', function(assert) { + const checkin = checkinStub({ + postalCode: '55403', + locationName: LOCATION_NAME, + weather: weatherStub(), + }); + + weatherResponse = () => RSVP.reject(new Error('500 from the weather endpoint')); + this.set('checkin', checkin); + + this.render(hbs`{{checkin/weather-step checkin=checkin}}`); + this.$('.grey.clickable').click(); + this.$('input').val('10001').trigger('input'); + this.$('.save-status').click(); + + return settled().then(() => { + assert.equal(get(checkin, 'postalCode'), '10001', 'the new location is saved'); + assert.equal(get(checkin, 'weather'), null, 'weather from the old location is cleared'); + }); +}); + test('a location the API cannot geocode is reported as not found', function(assert) { const checkin = checkinStub({ geocodable: false });