Convert to Repository for DistrictAnalyticsQuery numbers #2436

rsanheim · 2021-04-19T21:31:27Z

This make up the details tab of reports, so to keep things consistent we should grab them from the same repository object that backs up other parts of the app.

Story card: ch2625 and ch3343

Because

We are trying to consolidate more queries to one consistent path

This addresses

Moving part of the 'analytics queries' to repository

this make up the details tab of reports, so to keep things consistent we should grab them from the same repository that backs up other parts of the app..

shortcut-integration · 2021-04-21T05:27:35Z

This pull request has been linked to Clubhouse Story #2625: Registration counts on details and overview don't match.

…tails-numbers

getting this all consistent so the tables are easier to manage

…tails-numbers

shortcut-integration · 2021-04-28T18:32:56Z

This pull request has been linked to Clubhouse Story #3343: Convert to Repository for DistrictAnalyticsQuery numbers.

…tails-numbers

we can't change this interface or it could easily break some other clients using this class

…tails-numbers

harimohanraj89 · 2021-05-07T06:33:54Z

Ah I see - so we're moving towards using the repository directly, but not retiring the existing DistrictAnalyticsQuery completely in one fell swoop. 👍

harimohanraj89

This is the right direction, but it sounds like we've still got some work to do in terms of:

Consistent use of the repository
Retiring legacy "query" classes like DistrictAnalyticsQuery

Can we roughly story up the new few big refactors necessary to see this to completion?

vkrmis · 2021-05-07T12:16:23Z

app/models/facility.rb

-    patients
-      .hypertension_follow_ups_by_period(*args)
-      .where(blood_pressures: {facility: self})
+    patients.hypertension_follow_ups_by_period(*args).where(blood_pressures: {facility: self})


[nitpick] I find this less readable than the multiline version

vkrmis · 2021-05-07T12:39:36Z

app/queries/follow_ups_query.rb

+    @formatter = lambda { |v| @period_type == :quarter ? Period.quarter(v) : Period.month(v) }
+  end
+
+  def hypertension


This is partially replicated in the with method, can we just call with instead?

Can we do one of:

Keep all the FollowUpsQuery scopes in Patient and remove .hypertension here

Move all these ^ scopes to FollowUpsQuery

Doing both is a bit confusing.

I'm trying to limit the amount of creeping change going on in this PR...its already getting pretty unwieldy. Our current scopes are called from all over the place, so I wanted to start w/ the hypertension one (needed for the reports) and the with helper to get things consolidated. I'll move the others in a future PR, which we will need to consolidate things from the progress tab.

vkrmis

Some overall thoughts:

There's some follow ups logic that's distributed between Patient and FollowUpsQuery, can we consolidate these in one place?
Is this a good time to scrap DistrictAnalyticsQuery?, it doesn't do much that Repository doesn't give us directly.

Prabhanshu & vkrm

vkrmis · 2021-05-07T12:45:24Z

app/views/reports/regions/details.html.erb

                  <td class="ta-center">
-                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, date))) %>
+                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, period.value))) %>


Can we get bp_measures_by_period from the repository instead? It looks like this is the only place we're still using data from @dashboard_analytics

It's also being used by the new monthly district report but I think we can remove that after this is merged.

The repository doesn't have that query wired up yet, but it will come soon...

vkrmis · 2021-05-07T13:01:59Z

spec/queries/follow_ups_query_spec.rb

+        recorded_at: second_follow_up_date)
+    end
+
+    describe ".follow_ups" do


Since this is testing Patient.follow_ups_by_period, shouldn't these be in Patient instead?
Alternatively, we could test a call to FollowUpsQuery here.

I'm trying to consolidate queries that have lots going on (like follow ups) into their own objects, and then can test them from one spec file. We have so much surface area exposed by scopes from Patient, Facility, and elsewhere....if all those scopes eventually call the same query object, at least there is one place to look and one spec file to read.

All that said, consider the current follow up specs and query a work in progress.

vkrmis · 2021-05-07T13:05:05Z

app/queries/follow_ups_query.rb

+    @formatter = lambda { |v| @period_type == :quarter ? Period.quarter(v) : Period.month(v) }
+  end
+
+  def hypertension


Can we do one of:

Keep all the FollowUpsQuery scopes in Patient and remove .hypertension here

Move all these ^ scopes to FollowUpsQuery

Doing both is a bit confusing.

vkrmis · 2021-05-07T13:19:22Z

app/models/reports/repository.rb

+      registration_counts_by_user.each_with_object({}) do |(slug, period_counts), totals|
+        totals[slug] = {}
+        # collect all the user ids in a region that we need to count for
+        user_ids = period_counts.each_with_object(Set.new) { |(period, counts), sum| sum.merge(counts.keys) }


Can we rename:

counts to user_counts

sum to user_ids

Can this be simplified to something like user_ids = period_counts.values.map(&:keys).flatten.to_set

vkrmis · 2021-05-07T13:27:45Z

app/models/reports/repository.rb

@@ -209,7 +256,7 @@ def denominator(region, period)
    #     region_2_slug: { period_1: value, period_2: value }
    #   }
    #
-    def cached_query(calculation, &block)
+    def region_period_cached_query(calculation, &block)


Have we added other _cached_queries? Wondering why we're being more specific here. Is cached_query not descriptive enough?

I vote for keeping the change. I know from experience with this service object that a lot of the data is in form of { region: { period1: ..., period2: ...}}
this makes it clear at a glance that this should be structured that way.

Yeah there are other cached queries that return region based items instead of region/period based items.

vkrmis · 2021-05-07T13:31:12Z

app/views/reports/regions/details.html.erb

              </td>
            <% end %>
          </tr>
          <% current_admin.accessible_users(:view_reports).order(:full_name).each do |resource| %>
            <% if @dashboard_analytics[resource.id].present? %>
              <tr>
                <td class="row-title">
-                  <%= link_to resource.send(:full_name), send(:admin_user_path, resource, period: @period) %>
+                  <%= link_to resource.full_name, admin_user_path(resource, period: @period) %>


kpethtel

Really nice!

kpethtel · 2021-05-06T18:31:15Z

app/models/medication.rb

-  CREATED_TIME ||= File.ctime("config/data/medications.yml").in_time_zone("UTC")
-  UPDATED_TIME ||= File.mtime("config/data/medications.yml").in_time_zone("UTC")
+  CREATED_TIME = File.ctime("config/data/medications.yml").in_time_zone("UTC")
+  UPDATED_TIME = File.mtime("config/data/medications.yml").in_time_zone("UTC")


Thank you. I was wondering about this.

kpethtel · 2021-05-07T18:36:29Z

app/models/period.rb

@@ -159,11 +159,11 @@ def inspect
    "<Period type:#{type} value=#{value}>"
  end

-  def to_s
+  def to_s(format = :mon_year)


This is a nice usability addition.

kpethtel · 2021-05-07T18:38:26Z

app/models/reports/repository.rb

@@ -58,6 +58,8 @@ def initialize(regions, periods:)
      end
    end

+    # Adjusted patient counts are the patient counts from three months ago (the adjusted period) that


Thank you for adding these comments. I find them useful when I haven't been in this code recently. Once you're more familiar with this object, the methods seem pretty intuitive, but before that, the comments help reduce the amount of code you have read to understand what's really happening.

kpethtel · 2021-05-07T18:41:38Z

app/models/reports/repository.rb

@@ -209,7 +256,7 @@ def denominator(region, period)
    #     region_2_slug: { period_1: value, period_2: value }
    #   }
    #
-    def cached_query(calculation, &block)
+    def region_period_cached_query(calculation, &block)


I vote for keeping the change. I know from experience with this service object that a lot of the data is in form of { region: { period1: ..., period2: ...}}
this makes it clear at a glance that this should be structured that way.

kpethtel · 2021-05-07T18:45:18Z

app/views/reports/regions/details.html.erb

@@ -49,14 +50,14 @@
                Total registered<br>
                patients
            </th>
-            <% dates_for_periods(@period.type, 6, include_current_period: @show_current_period).each do |date| %>
+            <% @period_range.each do |period| %>


Much appreciated.

kpethtel · 2021-05-07T18:46:05Z

app/views/reports/regions/details.html.erb

              </td>
            <% end %>
          </tr>
          <% current_admin.accessible_users(:view_reports).order(:full_name).each do |resource| %>
            <% if @dashboard_analytics[resource.id].present? %>
              <tr>
                <td class="row-title">
-                  <%= link_to resource.send(:full_name), send(:admin_user_path, resource, period: @period) %>
+                  <%= link_to resource.full_name, admin_user_path(resource, period: @period) %>


kpethtel · 2021-05-07T18:54:57Z

app/views/reports/regions/details.html.erb

                  <td class="ta-center">
-                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, date))) %>
+                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, period.value))) %>


It's also being used by the new monthly district report but I think we can remove that after this is merged.

kpethtel · 2021-05-07T19:01:23Z

spec/queries/district_analytics_query_spec.rb

          end

          patients_2.each do |patient|
-            create(:blood_pressure, patient: patient, facility: facility_2)
+            create(:blood_pressure, patient: patient, facility: facility_2, user: user)


I assume because the factories were making unique users per bp reading. This is definitely the kind of thing that makes factorybot hard to use efficiently.

rsanheim

Some responses to feedback.

rsanheim · 2021-05-09T23:36:43Z

app/models/reports/repository.rb

@@ -209,7 +256,7 @@ def denominator(region, period)
    #     region_2_slug: { period_1: value, period_2: value }
    #   }
    #
-    def cached_query(calculation, &block)
+    def region_period_cached_query(calculation, &block)


Yeah there are other cached queries that return region based items instead of region/period based items.

rsanheim · 2021-05-09T23:38:09Z

app/queries/follow_ups_query.rb

+    @formatter = lambda { |v| @period_type == :quarter ? Period.quarter(v) : Period.month(v) }
+  end
+
+  def hypertension


I'm trying to limit the amount of creeping change going on in this PR...its already getting pretty unwieldy. Our current scopes are called from all over the place, so I wanted to start w/ the hypertension one (needed for the reports) and the with helper to get things consolidated. I'll move the others in a future PR, which we will need to consolidate things from the progress tab.

rsanheim · 2021-05-09T23:38:48Z

app/views/reports/regions/details.html.erb

                  <td class="ta-center">
-                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, date))) %>
+                    <%= number_with_delimiter(dash_if_zero(@dashboard_analytics.dig(resource.id, :bp_measures_by_period, period.value))) %>


The repository doesn't have that query wired up yet, but it will come soon...

rsanheim · 2021-05-09T23:41:55Z

config/initializers/time_and_date_formats.rb

@@ -1,6 +1,9 @@
 Time::DATE_FORMATS[:mon_year] = "%b-%Y"
 Date::DATE_FORMATS[:mon_year] = "%b-%Y"

+Date::DATE_FORMATS[:mon_year_multiline] = lambda { |date| date.strftime("%b-%Y").tr("-", "\n") }
+Time::DATE_FORMATS[:mon_year_multiline] = lambda { |date| date.strftime("%b-%Y").tr("-", "\n") }


the answer is yes 👍

rsanheim · 2021-05-09T23:44:23Z

spec/queries/follow_ups_query_spec.rb

+        recorded_at: second_follow_up_date)
+    end
+
+    describe ".follow_ups" do


I'm trying to consolidate queries that have lots going on (like follow ups) into their own objects, and then can test them from one spec file. We have so much surface area exposed by scopes from Patient, Facility, and elsewhere....if all those scopes eventually call the same query object, at least there is one place to look and one spec file to read.

All that said, consider the current follow up specs and query a work in progress.

…)" This reverts commit 4341a40.

) * Revert "Convert to Repository for DistrictAnalyticsQuery numbers (#2436)" This reverts commit 4341a40. * Disable intermittent failing test

Convert to Repository for DistrictAnalyticsQuery numbers

9ce02a7

this make up the details tab of reports, so to keep things consistent we should grab them from the same repository that backs up other parts of the app..

harimohanraj89 temporarily deployed to simple-review-pr-2436 April 19, 2021 21:35 Inactive

Merge branch 'master' into same-queries-for-details-numbers

7c283da

rsanheim temporarily deployed to simple-review-pr-2436 April 20, 2021 16:28 Inactive

Grab the data more directly

1987372

harimohanraj89 temporarily deployed to simple-review-pr-2436 April 20, 2021 19:19 Inactive

Grab from repository where we can; better helper

45e020a

rsanheim temporarily deployed to simple-review-pr-2436 April 20, 2021 20:22 Inactive

rsanheim added 2 commits April 20, 2021 15:30

Grab facility regions for repo; more succint helpers

9286b12

grab from repository

86d3629

rsanheim temporarily deployed to simple-review-pr-2436 April 20, 2021 20:36 Inactive

Merge remote-tracking branch 'origin/master' into same-queries-for-de…

cf43fc4

…tails-numbers

rsanheim temporarily deployed to simple-review-pr-2436 April 21, 2021 17:53 Inactive

rsanheim added 7 commits April 27, 2021 12:39

Merge branch 'master' into same-queries-for-details-numbers

b04053c

clenaup

1e237b1

Use period_range for date display; add custom format

b8daf2a

getting this all consistent so the tables are easier to manage

Grab more numbers from repository

3da5e38

checking to see if used

89aac4f

Extract FollowUpsQuery

7b7e0ae

Merge remote-tracking branch 'origin/master' into same-queries-for-de…

7faea68

…tails-numbers

rsanheim added 2 commits April 28, 2021 13:33

linting

9b72255

Moving specs; getting follow ups to parity

f8efada

rsanheim mentioned this pull request Apr 29, 2021

3327 who report #2458

Merged

rsanheim added 5 commits April 29, 2021 15:30

Merge remote-tracking branch 'origin/master' into same-queries-for-de…

8f97654

…tails-numbers

Dont worry about encounters yet

95308c0

Consolidate more queries to FollowUpsQuery

12f12de

Clean up spec

058ee89

more spec cleanup

e707996

rsanheim requested a review from harimohanraj89 May 6, 2021 03:29

rsanheim added 7 commits May 6, 2021 12:36

Maintain same interface for DistrictAnalyticsQuery

7a2fdc2

we can't change this interface or it could easily break some other clients using this class

linting

b4e30f7

Use consistent group_by naming

4d0a5a6

Fix the group_by arg

f6f81a5

Merge remote-tracking branch 'origin/master' into same-queries-for-de…

17475a0

…tails-numbers

more repo numbers, cleanup

7249764

linting

3c6b7c4

rsanheim marked this pull request as ready for review May 6, 2021 20:04

harimohanraj89 approved these changes May 7, 2021

View reviewed changes

vkrmis reviewed May 7, 2021

View reviewed changes

Merge branch 'master' into same-queries-for-details-numbers

ae3a700

rsanheim requested a review from claudiovallejo as a code owner May 7, 2021 18:52

kpethtel approved these changes May 7, 2021

View reviewed changes

rsanheim added 3 commits May 9, 2021 18:30

Merge branch 'master' into same-queries-for-details-numbers

9a10bbd

renames

a6f9546

No need for the block

5bbefe9

rsanheim commented May 10, 2021

View reviewed changes

linting

8c053e8

rsanheim merged commit 4341a40 into master May 10, 2021

rsanheim deleted the same-queries-for-details-numbers branch May 10, 2021 14:49

rsanheim added a commit that referenced this pull request May 10, 2021

Revert "Convert to Repository for DistrictAnalyticsQuery numbers (#2436…

6f2228b

…)" This reverts commit 4341a40.

rsanheim mentioned this pull request May 10, 2021

Revert "Convert to Repository for DistrictAnalyticsQuery numbers" #2479

Merged

rsanheim restored the same-queries-for-details-numbers branch May 10, 2021 15:54

rsanheim mentioned this pull request May 10, 2021

Repository details page take two #2480

Merged

rsanheim deleted the same-queries-for-details-numbers branch November 18, 2021 21:32

Convert to Repository for DistrictAnalyticsQuery numbers #2436

Convert to Repository for DistrictAnalyticsQuery numbers #2436

Conversation

rsanheim commented Apr 19, 2021 • edited Loading

Because

This addresses

shortcut-integration bot commented Apr 21, 2021

shortcut-integration bot commented Apr 28, 2021

harimohanraj89 commented May 7, 2021

harimohanraj89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkrmis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpethtel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsanheim commented Apr 19, 2021 •

edited

Loading