From 408c186d397397793f9b42ffc690963c6d9e5701 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Tue, 29 Jun 2021 17:04:03 -0500 Subject: [PATCH 01/12] Start adding new mat views --- app/services/refresh_materialized_views.rb | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/app/services/refresh_materialized_views.rb b/app/services/refresh_materialized_views.rb index bba2931216..c1f79c8312 100644 --- a/app/services/refresh_materialized_views.rb +++ b/app/services/refresh_materialized_views.rb @@ -28,8 +28,11 @@ def benchmark_and_statsd(operation) end def call - benchmark_and_statsd("all") do - refresh + benchmark_and_statsd("all_v1") do + refresh_v1 + end + benchmark_and_statsd("all_v2") do + refresh_v2 end end @@ -39,9 +42,9 @@ def self.tz delegate :tz, :set_last_updated_at, to: self - def refresh - # LatestBloodPressuresPerPatientPerMonth should be refreshed before - # LatestBloodPressuresPerPatientPerQuarter and LatestBloodPressuresPerPatient + # LatestBloodPressuresPerPatientPerMonth should be refreshed before + # LatestBloodPressuresPerPatientPerQuarter and LatestBloodPressuresPerPatient + def refresh_v1 ActiveRecord::Base.transaction do ActiveRecord::Base.connection.execute("SET LOCAL TIME ZONE '#{tz}'") @@ -72,4 +75,13 @@ def refresh set_last_updated_at end end + + def refresh_v2 + %w[reporting_patient_blood_pressures_per_month + reporting_patient_visits_per_month + reporting_patient_states_per_month].each do |matview| + + + end + end end From 96eb6cc3b5ef782af6c0a0e968f900cc083ef5cc Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Tue, 29 Jun 2021 17:04:47 -0500 Subject: [PATCH 02/12] Add BPs per month --- .../reporting_pipeline/patient_blood_pressures_per_month.rb | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 app/models/reporting_pipeline/patient_blood_pressures_per_month.rb diff --git a/app/models/reporting_pipeline/patient_blood_pressures_per_month.rb b/app/models/reporting_pipeline/patient_blood_pressures_per_month.rb new file mode 100644 index 0000000000..cf1e5a76ab --- /dev/null +++ b/app/models/reporting_pipeline/patient_blood_pressures_per_month.rb @@ -0,0 +1,6 @@ +module ReportingPipeline + class PatientBloodPressuresPerMonth < Matview + self.table_name = "reporting_patient_blood_pressures_per_month" + belongs_to :patient + end +end From d35e6d47e553408482afcd46e3e58848893a3c10 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Tue, 29 Jun 2021 17:47:58 -0500 Subject: [PATCH 03/12] Refactor and add specs for v2 --- app/services/refresh_materialized_views.rb | 56 +++++++++---------- .../refresh_materialized_views_spec.rb | 14 ++++- 2 files changed, 40 insertions(+), 30 deletions(-) diff --git a/app/services/refresh_materialized_views.rb b/app/services/refresh_materialized_views.rb index c1f79c8312..4ab2f46dc6 100644 --- a/app/services/refresh_materialized_views.rb +++ b/app/services/refresh_materialized_views.rb @@ -42,46 +42,44 @@ def self.tz delegate :tz, :set_last_updated_at, to: self + V1_MATVIEWS = %w[ + LatestBloodPressuresPerPatientPerMonth + LatestBloodPressuresPerPatient + LatestBloodPressuresPerPatientPerQuarter + BloodPressuresPerFacilityPerDay + PatientRegistrationsPerDayPerFacility + MaterializedPatientSummary + ] + V2_MATVIEWS = %w[ + ReportingPipeline::PatientBloodPressuresPerMonth + ReportingPipeline::PatientStatesPerMonth + ReportingPipeline::PatientVisitsPerMonth + ] + # LatestBloodPressuresPerPatientPerMonth should be refreshed before # LatestBloodPressuresPerPatientPerQuarter and LatestBloodPressuresPerPatient def refresh_v1 ActiveRecord::Base.transaction do ActiveRecord::Base.connection.execute("SET LOCAL TIME ZONE '#{tz}'") - - benchmark_and_statsd("LatestBloodPressuresPerPatientPerMonth") do - LatestBloodPressuresPerPatientPerMonth.refresh + V1_MATVIEWS.each do |name| + benchmark_and_statsd(name) do + klass = name.constantize + klass.refresh + end end - - benchmark_and_statsd("LatestBloodPressuresPerPatient") do - LatestBloodPressuresPerPatient.refresh - end - - benchmark_and_statsd("LatestBloodPressuresPerPatientPerQuarter") do - LatestBloodPressuresPerPatientPerQuarter.refresh - end - - benchmark_and_statsd("BloodPressuresPerFacilityPerDay") do - BloodPressuresPerFacilityPerDay.refresh - end - - benchmark_and_statsd("PatientRegistrationsPerDayPerFacility") do - PatientRegistrationsPerDayPerFacility.refresh - end - - benchmark_and_statsd("MaterializedPatientSummary") do - MaterializedPatientSummary.refresh - end - set_last_updated_at end end def refresh_v2 - %w[reporting_patient_blood_pressures_per_month - reporting_patient_visits_per_month - reporting_patient_states_per_month].each do |matview| - - + ActiveRecord::Base.transaction do + ActiveRecord::Base.connection.execute("SET LOCAL TIME ZONE '#{tz}'") + V2_MATVIEWS.each do |name| + benchmark_and_statsd(name) do + klass = name.constantize + klass.refresh + end + end end end end diff --git a/spec/services/refresh_materialized_views_spec.rb b/spec/services/refresh_materialized_views_spec.rb index 4663d53d31..22a14f3ab3 100644 --- a/spec/services/refresh_materialized_views_spec.rb +++ b/spec/services/refresh_materialized_views_spec.rb @@ -24,10 +24,10 @@ time = Time.current # Just adding enough data to smoke test this; we test these views # more thoroughly via various reporting specs - create_list(:blood_pressure, 2) expect { Timecop.freeze(time) do + create_list(:blood_pressure, 2) RefreshMaterializedViews.call end }.to change { LatestBloodPressuresPerPatientPerMonth.count }.from(0).to(2) @@ -37,4 +37,16 @@ .and change { PatientRegistrationsPerDayPerFacility.count }.from(0).to(2) .and change { RefreshMaterializedViews.last_updated_at }.from(nil).to(time) end + + it "updates v2 matviews" do + time = Time.current + expect { + Timecop.freeze(time) do + create_list(:blood_pressure, 2) + RefreshMaterializedViews.call + end + }.to change { ReportingPipeline::PatientBloodPressuresPerMonth.count }.from(0).to(2) + .and change { ReportingPipeline::PatientStatesPerMonth.count }.from(0).to(2) + .and change { ReportingPipeline::PatientVisitsPerMonth.count }.from(0).to(2) + end end From 48630b6bd36186c61716c78b75445cf7b91a0645 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Tue, 29 Jun 2021 17:48:26 -0500 Subject: [PATCH 04/12] freeze --- app/services/refresh_materialized_views.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/refresh_materialized_views.rb b/app/services/refresh_materialized_views.rb index 4ab2f46dc6..83cbe50b59 100644 --- a/app/services/refresh_materialized_views.rb +++ b/app/services/refresh_materialized_views.rb @@ -49,12 +49,12 @@ def self.tz BloodPressuresPerFacilityPerDay PatientRegistrationsPerDayPerFacility MaterializedPatientSummary - ] + ].freeze V2_MATVIEWS = %w[ ReportingPipeline::PatientBloodPressuresPerMonth ReportingPipeline::PatientStatesPerMonth ReportingPipeline::PatientVisitsPerMonth - ] + ].freeze # LatestBloodPressuresPerPatientPerMonth should be refreshed before # LatestBloodPressuresPerPatientPerQuarter and LatestBloodPressuresPerPatient From 7d0d611a54df4ebc82c1ed99ca0f3e71de1a7b9d Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Tue, 29 Jun 2021 18:58:34 -0500 Subject: [PATCH 05/12] linting --- spec/services/refresh_materialized_views_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/refresh_materialized_views_spec.rb b/spec/services/refresh_materialized_views_spec.rb index 22a14f3ab3..a6fb92859a 100644 --- a/spec/services/refresh_materialized_views_spec.rb +++ b/spec/services/refresh_materialized_views_spec.rb @@ -46,7 +46,7 @@ RefreshMaterializedViews.call end }.to change { ReportingPipeline::PatientBloodPressuresPerMonth.count }.from(0).to(2) - .and change { ReportingPipeline::PatientStatesPerMonth.count }.from(0).to(2) - .and change { ReportingPipeline::PatientVisitsPerMonth.count }.from(0).to(2) + .and change { ReportingPipeline::PatientStatesPerMonth.count }.from(0).to(2) + .and change { ReportingPipeline::PatientVisitsPerMonth.count }.from(0).to(2) end end From f4a4db547110c7b04c08731be647ab079c73fc77 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 12:53:49 -0500 Subject: [PATCH 06/12] Turn cascade off, it refreshes too many tables See https://github.com/scenic-views/scenic/issues/275 --- app/models/reporting_pipeline/matview.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/reporting_pipeline/matview.rb b/app/models/reporting_pipeline/matview.rb index 2b3b4ec232..8a6df49b29 100644 --- a/app/models/reporting_pipeline/matview.rb +++ b/app/models/reporting_pipeline/matview.rb @@ -3,7 +3,7 @@ class Matview < ActiveRecord::Base def self.refresh ActiveRecord::Base.transaction do ActiveRecord::Base.connection.execute("SET LOCAL TIME ZONE '#{Period::REPORTING_TIME_ZONE}'") - Scenic.database.refresh_materialized_view(table_name, concurrently: false, cascade: true) + Scenic.database.refresh_materialized_view(table_name, concurrently: false, cascade: false) end end end From 22d5c552f47a1e4138733d70423e852c3e62e911 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 13:56:43 -0500 Subject: [PATCH 07/12] Use RefreshMaterializedViews to refresh the new matviews --- .../patient_states_per_month_spec.rb | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/spec/models/reporting_pipeline/patient_states_per_month_spec.rb b/spec/models/reporting_pipeline/patient_states_per_month_spec.rb index 4eb4416bb3..6bc99163b1 100644 --- a/spec/models/reporting_pipeline/patient_states_per_month_spec.rb +++ b/spec/models/reporting_pipeline/patient_states_per_month_spec.rb @@ -19,7 +19,7 @@ patient_registered_13m_ago = Timecop.freeze(13.months.ago) { create(:patient) } Timecop.freeze(13.months.ago) { create(:blood_pressure, patient: patient_registered_13m_ago) } - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class .where(htn_care_state: "lost_to_follow_up", month_date: Date.current.beginning_of_month) @@ -34,7 +34,7 @@ patient_registered_12m_ago = Timecop.freeze(12.months.ago) { create(:patient) } patient_registered_11m_ago = Timecop.freeze(11.months.ago) { create(:patient) } - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class .where(htn_care_state: "lost_to_follow_up", month_date: Date.current.beginning_of_month) @@ -56,7 +56,7 @@ patient_with_recent_bp = Timecop.freeze(13.months.ago) { create(:patient) } Timecop.freeze(11.months.ago) { create(:blood_pressure, patient: patient_with_recent_bp) } - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class .where(htn_care_state: "lost_to_follow_up", month_date: Date.current.beginning_of_month) @@ -75,7 +75,7 @@ create(:blood_pressure, patient: under_care_patient, recorded_at: june_2021[:under_12_months_ago]) create(:blood_pressure, patient: ltfu_patient, recorded_at: june_2021[:over_12_months_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class @@ -100,7 +100,8 @@ create(:blood_pressure, patient: under_care_patient, recorded_at: june_2021[:end_of_month] - 1.minute) create(:blood_pressure, patient: ltfu_patient, recorded_at: june_2021[:end_of_month] + 1.minute) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 + with_reporting_time_zones do expect(described_class .where(htn_care_state: "lost_to_follow_up", month_date: june_2021[:beginning_of_month]) @@ -121,7 +122,7 @@ under_care_patient = create(:patient, recorded_at: june_2021[:under_12_months_ago]) ltfu_patient = create(:patient, recorded_at: june_2021[:over_12_months_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class .where(htn_care_state: "lost_to_follow_up", month_date: june_2021[:beginning_of_month]) @@ -147,7 +148,7 @@ patient_2 = create(:patient, recorded_at: june_2021[:long_ago]) create(:encounter, patient: patient_2, encountered_on: june_2021[:under_3_months_ago]) patient_3 = create(:patient, recorded_at: june_2021[:long_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class.where(htn_treatment_outcome_in_last_3_months: "missed_visit", month_date: june_2021[:now]).pluck(:patient_id)) @@ -180,7 +181,7 @@ facility: patient_with_no_bp.registration_facility, patient: patient_with_no_bp, user: patient_with_no_bp.registration_user) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class.where(htn_treatment_outcome_in_last_3_months: "visited_no_bp", month_date: june_2021[:now]).pluck(:patient_id)) @@ -200,7 +201,7 @@ patient_bp_over_3_months = create(:patient, recorded_at: june_2021[:long_ago]) create(:blood_pressure, :with_encounter, patient: patient_bp_over_3_months, recorded_at: june_2021[:over_3_months_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class.where(htn_treatment_outcome_in_last_3_months: "controlled", month_date: june_2021[:now]).pluck(:patient_id)) @@ -220,7 +221,7 @@ patient_3 = create(:patient, recorded_at: june_2021[:now]) patient_4 = create(:patient, recorded_at: june_2021[:over_3_months_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(described_class.find_by(patient_id: patient_1.id, month_string: june_2021[:month_string]).months_since_registration).to eq 11 expect(described_class.find_by(patient_id: patient_2.id, month_string: june_2021[:month_string]).months_since_registration).to eq 12 @@ -243,7 +244,7 @@ patient = create(:patient, registration_facility: registration_facility, assigned_facility: assigned_facility) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do patient_state = described_class.find_by(patient_id: patient.id, month_string: june_2021[:month_string]) @@ -275,7 +276,7 @@ patient = create(:patient, registration_facility: registration_facility, assigned_facility: assigned_facility) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do patient_state = described_class.find_by(patient_id: patient.id, month_string: june_2021[:month_string]) @@ -306,7 +307,7 @@ patient_no_bp = create(:patient, recorded_at: june_2021[:long_ago]) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do controlled_state = described_class.find_by(patient_id: patient_controlled.id, month_string: june_2021[:month_string]) @@ -348,7 +349,7 @@ def patient_states(patient, from: nil, to: nil) create(:prescription_drug, patient: patient, device_created_at: eight_months_ago) create(:blood_pressure, :with_encounter, patient: patient, recorded_at: five_months_ago, systolic: 140, diastolic: 90) - described_class.refresh + RefreshMaterializedViews.new.refresh_v2 with_reporting_time_zones do expect(patient_states(patient).pluck(:months_since_registration)).to eq((0..24).to_a) From 05040af74376295f187d83d0bec60c589d8d0883 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 15:22:32 -0500 Subject: [PATCH 08/12] Visits depend up on States, so refresh states first --- app/services/refresh_materialized_views.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/refresh_materialized_views.rb b/app/services/refresh_materialized_views.rb index 83cbe50b59..ace8314e9f 100644 --- a/app/services/refresh_materialized_views.rb +++ b/app/services/refresh_materialized_views.rb @@ -52,8 +52,8 @@ def self.tz ].freeze V2_MATVIEWS = %w[ ReportingPipeline::PatientBloodPressuresPerMonth - ReportingPipeline::PatientStatesPerMonth ReportingPipeline::PatientVisitsPerMonth + ReportingPipeline::PatientStatesPerMonth ].freeze # LatestBloodPressuresPerPatientPerMonth should be refreshed before From f1baffafe66f8d8c985768149acd2cb205043dae Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 15:22:32 -0500 Subject: [PATCH 09/12] States depend up on Visits, so refresh Visits first --- app/services/refresh_materialized_views.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/refresh_materialized_views.rb b/app/services/refresh_materialized_views.rb index 83cbe50b59..ace8314e9f 100644 --- a/app/services/refresh_materialized_views.rb +++ b/app/services/refresh_materialized_views.rb @@ -52,8 +52,8 @@ def self.tz ].freeze V2_MATVIEWS = %w[ ReportingPipeline::PatientBloodPressuresPerMonth - ReportingPipeline::PatientStatesPerMonth ReportingPipeline::PatientVisitsPerMonth + ReportingPipeline::PatientStatesPerMonth ].freeze # LatestBloodPressuresPerPatientPerMonth should be refreshed before From 372b67eb9ef5146c935d091c57a785b89e1fdb5b Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 15:24:02 -0500 Subject: [PATCH 10/12] Enforce a known good time for all these specs --- .../patient_states_per_month_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/models/reporting_pipeline/patient_states_per_month_spec.rb b/spec/models/reporting_pipeline/patient_states_per_month_spec.rb index 6bc99163b1..0c7ddd4190 100644 --- a/spec/models/reporting_pipeline/patient_states_per_month_spec.rb +++ b/spec/models/reporting_pipeline/patient_states_per_month_spec.rb @@ -5,6 +5,17 @@ it { should belong_to(:patient) } end + around do |example| + # We need to enforce a known time for this test, otherwise we will have intermittent failures. For example, + # if we use live system time, many of these specs will fail after 18:30 UTC (ie 14:30 ET) when on the last day of a month, + # because that falls into the next day in IST (our reporting time zone). So to prevent confusing failures for + # developers or CI during North American afternoons, we freeze to a time that will be the end of the month for + # UTC, ET, and IST. Timezones! 🤯 + Timecop.freeze("June 30 2021 5:30 UTC") do # June 30th 23:00 IST time + example.run + end + end + context "indicators" do describe "htn_care_state" do it "marks a dead patient dead" do From 50ad31b6d2f70ca521d7185ea28e10f0382f20c3 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 15:34:36 -0500 Subject: [PATCH 11/12] Enforce end of June for UTC, ET, and IST --- .../reporting_pipeline/patient_visits_per_month_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/models/reporting_pipeline/patient_visits_per_month_spec.rb b/spec/models/reporting_pipeline/patient_visits_per_month_spec.rb index 1e7bbc0f46..133b215f27 100644 --- a/spec/models/reporting_pipeline/patient_visits_per_month_spec.rb +++ b/spec/models/reporting_pipeline/patient_visits_per_month_spec.rb @@ -5,6 +5,12 @@ it { should belong_to(:patient) } end + around do |example| + Timecop.freeze("June 30 2021 5:30 UTC") do # June 30th 23:00 IST time + example.run + end + end + describe "the visit definition" do it "considers a BP measurement as a visit" do bp = create(:blood_pressure, :with_encounter, recorded_at: june_2021[:now]) From 843931521f9dca3dc717b97532c428492e15f5b2 Mon Sep 17 00:00:00 2001 From: Rob Sanheim Date: Wed, 30 Jun 2021 15:54:08 -0500 Subject: [PATCH 12/12] Freeze time to avoid intermittent failures --- .../reports/regions_controller_spec.rb | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/spec/controllers/reports/regions_controller_spec.rb b/spec/controllers/reports/regions_controller_spec.rb index cc06afd02e..ca195602db 100644 --- a/spec/controllers/reports/regions_controller_spec.rb +++ b/spec/controllers/reports/regions_controller_spec.rb @@ -514,16 +514,18 @@ def refresh_views end it "calls csv service and returns 200 with csv data" do - facility - sign_in(cvho.email_authentication) + Timecop.freeze("June 15th 2020") do + facility + sign_in(cvho.email_authentication) - expect_any_instance_of(MonthlyDistrictDataService).to receive(:report).and_call_original - get :monthly_district_data_report, params: {id: region.slug, report_scope: "district", format: "csv"} - expect(response.status).to eq(200) - expect(response.body).to include("Monthly District Data: #{region.name} #{Date.current.strftime("%B %Y")}") - report_date = Date.current.strftime("%b-%Y").downcase - expected_filename = "monthly-district-data-#{region.slug}-#{report_date}.csv" - expect(response.headers["Content-Disposition"]).to include(%(filename="#{expected_filename}")) + expect_any_instance_of(MonthlyDistrictDataService).to receive(:report).and_call_original + get :monthly_district_data_report, params: {id: region.slug, report_scope: "district", format: "csv"} + expect(response.status).to eq(200) + expect(response.body).to include("Monthly District Data: #{region.name} #{Date.current.strftime("%B %Y")}") + report_date = Date.current.strftime("%b-%Y").downcase + expected_filename = "monthly-district-data-#{region.slug}-#{report_date}.csv" + expect(response.headers["Content-Disposition"]).to include(%(filename="#{expected_filename}")) + end end it "works for facility districts" do