A timezone bugfix that took us through API and OOP design discussions
Considerations while interpreting a date in local timezone
At Buoy we have a GraphQL API (served with Ruby on Rails and PostgreSQL)
that exposes a completedDonationsCount
field. It takes optional date arguments to search within a time window.
It lets API clients count donations that happened in the last 180 days,
one of the checks we run while checking a plasma donors’ eligibility.
Two columns store this data: localized_completed_on
of type date
, and completed_at
of type datetime
.
Here’s an example spec of how the query works, set in Mountain Time, where some of our donation facilities are.
Given two donations, one on January 1st at 6pm and one on January 3rd at midday,
if querying for donations completed on or after January 2nd, it expects to see 1
(the last donation only):
it "filters properly with time zones" do
donor = create(:donor)
create(
:donation,
:completed,
donor: donor,
completed_at: "2023-01-01T18:00:00-06:00",
)
create(
:donation,
:completed,
donor: donor,
completed_at: "2023-01-03T12:00:00-06:00",
)
query = <<~QUERY
query {
findDonor(id: #{donor.id}) {
completedDonationsCount(filter: {
completedAt: { gteq: "2023-01-02" }
})
}
}
QUERY
execute_query query
expect(result["data"]).to eq(
"findDonor" => {
"completedDonationsCount" => 1
}
)
end
The spec fails: the query finds the first donation too, that happened in the afternoon Mountain Time but it’s midnight UTC.
Our thought process towards fixing that spec follows.
First approach: change the API to receive a datetime
A date doesn’t know its timezone, but if we add a time to the API parameter clients can send the beginning of day in the facility’s timezone.
We quickly dismissed it as bad API design: our product and clients care about the calendar day, we don’t want to push down the responsibility to client implementors to shape their argument according to the facility’s timezone, that they may need to fetch beforehand. A screener helping a donor check in looks at a calendar to count days, and similarly the API clients shouldn’t need to think of hour boundaries and timezones.
A better approach: server translates the date into the facility’s timezone
The server queried the localized_completed_on
date column, but it needs to
query the completed_at
datetime field instead. The server has access to the
facilities
database table so it can easily convert dates.
First, get the spec green with inline code
I made the server query the datetime column rather than the date-only,
and I tweaked the filter
parameters in the class that exposes the completedDonationsCount
field
to send the given date as a localized datetime.
This is only enough code to get the spec above green, then we can refactor.
+ # Parse the given date into a time
+ datetime = Time.new(filter.to_h[:completed_at].to_h[:gteq])
InfirmaryEngine::ArelFilter.call(
- filter,
+ # Recreate params hash with `time` in the proper zone
+ { completed_at: { gteq: datetime.in_time_zone(object.facility.time_zone) } },
scope: completed_donations
).count
This got it green.
But we don’t want to pollute our ArelFilter
class with the very specific responsibility.
And we added a test case for other options than gteq
.
Second, refactor with composition
I called my colleague Anthony for pairing.
We decided to call a class that composed ArelFilter
with an added time_zone
argument instead.
We undid the changes to ArelFilter
and wrote a TimezonedArelFilter
class that adds time and zone to date parameters, to then call ArelFilter
with proper values.
Each class has a single reason to exist.
class TimezonedArelFilter
def initialize(scope, filters, time_zone)
@scope = scope
@filters = filters
@time_zone = time_zone
end
def filter
time_zoned_filters = filters.to_h.transform_values do |predicates|
predicates.to_h.transform_values do |value|
if value.respond_to?(:in_time_zone)
value.in_time_zone(time_zone)
else
value
end
end
end
ArelFilter.call(time_zoned_filters, scope: scope)
end
In the process we recalled Rails provides Hash#transform_values
to
replace an earlier inject({}) do |hash, (key, value)|
implementation.
I pushed it for code review and called it a day.
Third, refactor with simpler new object
But the next morning we noticed the TimezonedArelFilter
class doesn’t use
@scope
more than to forward it. There is still room for improvement.
Anthony suggested we simplify the class to mutate the values as needed, without
the need for changing the ArelFilter
original call or implementation whatsoever.
It sounded better, and it was:
# Call a new class to mutate the dates
augmented_filters = InfirmaryEngine::Filters::TimeZone.augment(
filter,
time_zone: object.facility.time_zone
)
InfirmaryEngine::ArelFilter.call(
augmented_filters,
scope: completed_donations
).count
And the new object:
module Filters
class TimeZone
def self.augment(filters, time_zone:)
new(filters, time_zone).augment
end
def initialize(filters, time_zone)
@filters = filters
@time_zone = time_zone
end
def augment
filters.to_h.transform_values do |predicates|
predicates.to_h.transform_values do |value|
if value.respond_to?(:in_time_zone)
value.in_time_zone(time_zone)
else
value
end
end
end
end
# ...
We got a failing spec, we got it green, and we did two rounds of refactors. We felt that the API and the OOP designs were now as self-explanatory as we could and wanted. Now clients can query for donation counts by dates, the server will respond correctly according to facility’s timezone, and the code is simpler than previous alternatives.
And we merged it in. ✨