Better registration query using groupdate #1124
Conversation
This works well in my test and gets all the counts, but fails in the browser due to some time zone issues going on. I tried to use the date_to_period_sql but was struggling, as that still didn't seem to solve things. I may need to dig into https://phili.pe/posts/timestamps-and-time-zones-in-postgresql/ to understand how timezones are working with PostgreSQL here.
We can use a regular ruby module and extend it where we need it as a class method, and include it where we want it as an instance method.
…ns-with-one-query
This gets rid of a pretty complicated SQL query where we also have to worry about handling time zone conversion ourselves. The ruby here is pretty straightforward and will be easier to maintain long term. This also changes the interface to take the district instead of a list of facilities, which is really the object we care about.
| @@ -19,7 +17,7 @@ def show | |||
| else | |||
| Date.current.advance(months: -1) | |||
| end | |||
| @data = DistrictReportService.new(facilities: @district.facilities, selected_date: @selected_date).call | |||
| @data = DistrictReportService.new(district: @district, selected_date: @selected_date).call | |||
There was a problem hiding this comment.
Non-blocking: I like the idea of letting this service class continue accepting a relation of facilities. Perhaps we can rename it? Seems like better setup to support state/block reports in the future, or whatever arbitrary division of facilities.
There was a problem hiding this comment.
I actually wanted to allow either, but for this iteration it needs to take a district because we use
district.patients.with_hypertension
to get registration counts. I imagine we could find a way to go from the set of facilities to get registration counts, but it would be more indirect I think.
| @@ -35,6 +33,9 @@ def district_params | |||
| def set_time_zone | |||
| time_zone = Rails.application.config.country[:time_zone] || DEFAULT_ANALYTICS_TIME_ZONE | |||
|
|
|||
| Groupdate.time_zone = time_zone | |||
There was a problem hiding this comment.
Is there a reason we can't put this at the ApplicationController level?
There was a problem hiding this comment.
The rest of our application code likely assumes that Groupdate operates in UTC, so I'd be wary of applying this change globally. Step-by-step seems to be the way to go.
|
Closing this in favor of the more comprehensive PR #1131. |
This gets rid of a pretty complicated SQL query where we also have to
worry about handling time zone conversion ourselves. It uses
groupdateto get the counts, and then does the running total with plain ruby.The ruby here is pretty straightforward and will be easier to maintain
long term.
This also changes the interface to take the district instead of a list
of facilities, which is really the object we care about.