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 ac8f05714..5ae1ee531 100644
--- a/backend/app/services/weather_retriever.rb
+++ b/backend/app/services/weather_retriever.rb
@@ -1,32 +1,81 @@
+require "digest"
+
class WeatherRetriever
+ FORECAST_MISS_TTL = 5.minutes
+
class << self
def get(date, postal_code)
- position = Position.find_or_create_by(postal_code: postal_code)
+ date = date.to_date
+ position = find_or_create_position(postal_code)
- weather = Weather.find_by(date: date, position_id: position&.id)
+ if position.persisted?
+ weather = Weather.find_by(date: date, position_id: position.id)
- return weather if weather.present?
+ 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
- forecast = get_forecast(date, position)
+ 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}"
+ # 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)
- return
- end
+ return weather if weather.present?
+ return if forecast_miss_cached?(date, position.id)
+
+ 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
+
+ forecast = get_forecast(position)
+
+ 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)
- create_weather(forecast, position.id)
+ 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)
+
+ return
+ end
+
+ create_weather(day, date, position.id)
+ end
end
private
- def get_forecast(date, position)
+ # 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}",
["1d"],
@@ -34,40 +83,91 @@ 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 = 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
+ 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}"
+
+ # 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
- 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/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/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/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 381df756f..5640d9759 100644
--- a/backend/spec/services/weather_retriever_spec.rb
+++ b/backend/spec/services/weather_retriever_spec.rb
@@ -6,23 +6,118 @@
# 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" }
+ 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 }
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 +127,265 @@
it { expect(perform).to eq(weather) }
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
+ # 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 "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" }
+
+ 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" }
+
+ 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.
+ 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 }
+
+ 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
+ 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
+
+ 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 9a06c4c15..49981896b 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,54 @@ 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');
+ const existingPostalCode = get(checkin, 'postalCode');
+ const existingWeather = get(checkin, 'weather');
- 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. 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 });
+
+ 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..1cda6d4f5 100644
--- a/frontend/tests/integration/components/checkin/weather-step-test.js
+++ b/frontend/tests/integration/components/checkin/weather-step-test.js
@@ -1,19 +1,257 @@
+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,
+
+ beforeEach() {
+ weatherQueries = [];
+ weatherResponse = () => RSVP.resolve(null);
+
+ // 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.register('service:store', Ember.Service.extend({
+ queryRecord(modelName, query) {
+ weatherQueries.push([modelName, query]);
-test('it renders', function(assert) {
+ return weatherResponse();
+ }
+ }));
+ }
+});
- // Set any properties with this.set('myProperty', 'value');
- // Handle any actions with this.on('myAction', function(val) { ... });
+test('it asks for a location when the check-in has none', function(assert) {
+ this.set('checkin', checkinStub());
- this.render(hbs`{{checkin/weather-step}}`);
+ 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.$('.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');
+});
+
+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('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 });
+
+ 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'
+ );
+ });
});