From 0359880b8793f197414043202f0573e2e9e35c33 Mon Sep 17 00:00:00 2001 From: pjaspers Date: Mon, 26 Oct 2020 13:53:27 +0100 Subject: [PATCH 1/5] Add spec to check for duplicates in PeriodResolver --- lib/orbf/rules_engine/fetch_data/periods_resolver.rb | 1 + .../lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/lib/orbf/rules_engine/fetch_data/periods_resolver.rb b/lib/orbf/rules_engine/fetch_data/periods_resolver.rb index fc066e6..52ddc3d 100644 --- a/lib/orbf/rules_engine/fetch_data/periods_resolver.rb +++ b/lib/orbf/rules_engine/fetch_data/periods_resolver.rb @@ -8,6 +8,7 @@ def initialize(package, invoice_period) @invoice_period = invoice_period end + # Will return an array of periods, guaranteed to be unique. def call from_values_span(package, invoice_period).merge( from_package_frequency(package, invoice_period) diff --git a/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb b/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb index 7f9c02e..f25ed5a 100644 --- a/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb +++ b/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb @@ -20,6 +20,10 @@ Orbf::RulesEngine::Formula.new( "increase", "safe_div(achieved,sum(%{achieved_previous_year_same_quarter_monthly_values}/4)" + ), + Orbf::RulesEngine::Formula.new( + "increase", + "safe_div(achieved,sum(%{achieved_last_24_months_window_values})" ) ] ) From 99f64879f99416551e5b051af2f66db56d0ccf21 Mon Sep 17 00:00:00 2001 From: pjaspers Date: Mon, 26 Oct 2020 13:54:25 +0100 Subject: [PATCH 2/5] Fix: Analytics call ran multiple times for same period If you had multiple packages, which had formulas which returned a similar set of periods, the overlapping periods would result in a double call. This only happened if you had more than 5 periods (the point at which we split the analytics call into multiple calls), so even if you had overlapping ones, if the total was not over 5, it would work. The reason the test didn't catch this was that all the package_arguments were set up with the same array of periods, so when we ran: map(&:periods).uniq It compared the arrays, instead of the elements, and because the arrays were exactly the same, we had no problems. I've made the spec a bit more wordy about this, and set up a scenario where we are triggering this behavior. I've also sorted the resulting periods, mostly because this reads nicer when debugging the calls. (And much easier to stub the requests) --- .../fetch_data/fetch_data_analytics.rb | 2 +- .../fetch_data/fetch_data_analytics_spec.rb | 88 +++++++++++++++---- 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb b/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb index 92a221b..6c66f61 100644 --- a/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb +++ b/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb @@ -75,7 +75,7 @@ def orgunit_ext_ids end def without_yearly_periods - package_arguments.map(&:periods).map { |p| p[0..-3] }.uniq.flatten + package_arguments.map(&:periods).map { |p| p[0..-3] }.flatten.uniq.sort end end end diff --git a/spec/lib/orbf/rules_engine/fetch_data/fetch_data_analytics_spec.rb b/spec/lib/orbf/rules_engine/fetch_data/fetch_data_analytics_spec.rb index 1fe3c39..0670b3d 100644 --- a/spec/lib/orbf/rules_engine/fetch_data/fetch_data_analytics_spec.rb +++ b/spec/lib/orbf/rules_engine/fetch_data/fetch_data_analytics_spec.rb @@ -99,24 +99,81 @@ WebMock::Config.instance.query_values_notation = nil end - it "splits out the API calls with too many different periods" do - periods = ["202004", "202005", "202006", "202007","202008", "202009", "202010", "202011"] - periods_with_years = periods.each_slice(4).map { |slice| slice += ["2020", "2019July"] } + describe '#without_yearly_periods' do + it "removes the yearly periods" do + periods = ["202004", "202005", "202006", "202007"] + yearly_periods = ["2020", "2019July"] # Added in PeriodResolver#from_package_frequency + periods_with_years = (periods + yearly_periods) + + package_arguments = [ + Orbf::RulesEngine::PackageArguments.with( + periods: periods_with_years, + orgunits: Orbf::RulesEngine::OrgUnits.new( + orgunits: [orgunit_1], package: package + ), + datasets_ext_ids: [], + package: package + ) + ] + + fetcher = described_class.new(dhis2_connection, package_arguments) + expect(fetcher.send(:without_yearly_periods)).to eq(periods) + end - package_arguments = periods_with_years.map do |periods| + it "removes duplicate periods if any" do + periods = [ + ["202004", "202005", "202006", "202007"], + ["202004", "202008", "202009", "202010"], + ["202004", "202008", "202010"], + ] + periods_with_years = periods.map { |slice| slice += ["2020", "2019July"] } + + # Create three PackageArguments with some overlap in between + package_arguments = periods_with_years.map do |periods| + Orbf::RulesEngine::PackageArguments.with( + periods: periods, + orgunits: Orbf::RulesEngine::OrgUnits.new( + orgunits: [orgunit_1], package: package + ), + datasets_ext_ids: [], + package: package + ) + end + fetcher = described_class.new(dhis2_connection, package_arguments) + # Only expect from 04 to 11, and all of them once + expected = (4..10).map {|i| "2020%02d" % i} + expect(fetcher.send(:without_yearly_periods)).to eq(expected) + end + end + + it "splits out the API calls with too many different periods" do + max_periods = described_class::MAX_PERIODS_PER_FETCH + # Let's set up MAX_PERIODS_PER_FETCH+1 periods + periods = (0..(max_periods + 1)).map {|i| "2020%02d" % (i+1)} + yearly_periods = ["2020", "2019July"] + org_unit = Orbf::RulesEngine::OrgUnits.new(orgunits: [orgunit_1], package: package) + + # Now create three packages, which each use a different subset of the periods + package_arguments = [ + periods[0..2], + periods[2..4], + periods + ].map do |period_subset| Orbf::RulesEngine::PackageArguments.with( - periods: periods, - orgunits: Orbf::RulesEngine::OrgUnits.new( - orgunits: [orgunit_1], package: package - ), + periods: period_subset + yearly_periods, + orgunits: org_unit, datasets_ext_ids: [], package: package ) end - fetcher = described_class.new(dhis2_connection, package_arguments) - - stubbed_requests = periods.each_slice(described_class::MAX_PERIODS_PER_FETCH).map do |period_slice| + # We know this will take two requests (MAX_PERIODS_PER_FETCH+1) + # First one will be from 0..MAX_PERIODS_PER_FETCH-1 + # Second one will be the MAX_PERIODS_PER_FETCH til the end + stubbed_requests = [ + periods[0...max_periods], + periods[max_periods..-1] + ].map do |period_slice| pe = period_slice.join(";") rows = period_slice.inject([]) do |result, period| result << ["dhis2_de_1", orgunit_1.ext_id, period, "1.4"] @@ -125,17 +182,16 @@ result end stub_request(:get, "https://play.dhis2.org/2.28/api/analytics?dimension=dx:dhis2_de_1%3Bdhis2_indic_1%3Bdhis2_de_1.coc_1&dimension=ou:1&dimension=pe:#{pe}") - .to_return(status: 200, body: JSON.pretty_generate( - "rows" => rows - ), headers: {}) + .to_return(status: 200, body: JSON.pretty_generate( + "rows" => rows + ), headers: {}) end + fetcher = described_class.new(dhis2_connection, package_arguments) values = fetcher.call stubbed_requests.each {|sr| expect(sr).to have_been_made.once } - periods.each do |period| - expect(values).to include({ "attributeOptionCombo" => "default", "categoryOptionCombo" => "default", "dataElement" => "dhis2_de_1", From 47ac0d2942a2d8f75a4590cb52b08eea88d33707 Mon Sep 17 00:00:00 2001 From: pjaspers Date: Mon, 26 Oct 2020 13:58:55 +0100 Subject: [PATCH 3/5] Remove logging --- lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb b/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb index 6c66f61..93d722b 100644 --- a/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb +++ b/lib/orbf/rules_engine/fetch_data/fetch_data_analytics.rb @@ -12,10 +12,6 @@ def call return [] if analytics_activity_states.none? combined_response = Array(without_yearly_periods).each_slice(MAX_PERIODS_PER_FETCH).inject({}) do |result, period_slice| - puts({periods: period_slice.join(";"), - organisation_units: orgunit_ext_ids, - data_elements: data_elements}.to_json) - analytics_response = dhis2_connection.analytics.list( periods: period_slice.join(";"), organisation_units: orgunit_ext_ids, From 54d0f47225fbe4418da98b22bb212120566b4973 Mon Sep 17 00:00:00 2001 From: pjaspers Date: Mon, 26 Oct 2020 14:32:00 +0100 Subject: [PATCH 4/5] Add spec for the paginated calls to analytics --- spec/analytics_datavalues_spec.rb | 49 +++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/spec/analytics_datavalues_spec.rb b/spec/analytics_datavalues_spec.rb index ad6f217..64b74ef 100644 --- a/spec/analytics_datavalues_spec.rb +++ b/spec/analytics_datavalues_spec.rb @@ -85,6 +85,11 @@ build_activity_formula( "de_m_from_indic", "real_indicator_value", "" + ), + + build_activity_formula( + "de_m_average", "avg(%{real_indicator_value_last_6_months_window_values})", + "" ) ] ) @@ -144,25 +149,39 @@ it "builds problem per category option combo" do WebMock::Config.instance.query_values_notation = :flat_array + all = %w(202002 202003 202004 202005 202006 202007 202008 202009) - stub_request(:get, "https://sample/api/analytics?dimension=dx:dNaUIFe7vkY&dimension=ou:kH9XaP6eoDo&dimension=pe:202007%3B202008%3B202009%3B2020Q3") - .to_return(status: 200, body: JSON.pretty_generate( - { - "height": 3, "rows": [ - ["dNaUIFe7vkY", "kH9XaP6eoDo", "202007", "2.0"], - ["dNaUIFe7vkY", "kH9XaP6eoDo", "202008", "2.0"], - ["dNaUIFe7vkY", "kH9XaP6eoDo", "2020Q3", "4.0"] - ], "width": 4 - } - )) + [ + all[0...5], + all[5..-1] + ["2020Q3"] + ].each do |periods| + pe = periods.join(";") + rows = periods.map do |period| + if "Q3" == period[0..-2] + value = "134" + else + value = period[-2..-1].to_i*100 + end + ["dNaUIFe7vkY", "kH9XaP6eoDo", period, value] + end + + stub_request(:get, "https://sample/api/analytics?dimension=dx:dNaUIFe7vkY&dimension=ou:kH9XaP6eoDo&dimension=pe:#{pe}") + .to_return(status: 200, body: JSON.pretty_generate( + { + "height": 3, "rows": rows, "width": 4 + } + )) + + end - stub_request(:get, "https://sample/api/dataValueSets?children=false&dataSet=ds1&dataSet=ds2&orgUnit=path&period=2020&period=202007&period=202008&period=202009&period=2020July&period=2020Q3") + pe = (all + %w(2020 2020July 2020Q3)).sort.map{|s| "period=#{s}" }.join("&") + stub_request(:get, "https://sample/api/dataValueSets?children=false&dataSet=ds1&dataSet=ds2&orgUnit=path&#{pe}") .to_return(status: 200, body: JSON.pretty_generate( { "dataValues" => [ - { "dataElement": indicator_dataelement_id, period: "202008", value: "2.0" ,categoryCombo:"Vo4mFUa9rlC"}, - { "dataElement": indicator_dataelement_id, period: "202008", value: "0.0" ,categoryCombo:"PQrXhDwCZBF"}, - { "dataElement": indicator_dataelement_id, period: "202009", value: "2.0" ,categoryCombo:"Vo4mFUa9rlC"}, - { "dataElement": indicator_dataelement_id, period: "202009", value: "0.0" ,categoryCombo:"PQrXhDwCZBF"} + { "dataElement": indicator_dataelement_id, period: "202008", value: "80.0" ,categoryCombo:"Vo4mFUa9rlC"}, + { "dataElement": indicator_dataelement_id, period: "202008", value: "81.0" ,categoryCombo:"PQrXhDwCZBF"}, + { "dataElement": indicator_dataelement_id, period: "202009", value: "90.0" ,categoryCombo:"Vo4mFUa9rlC"}, + { "dataElement": indicator_dataelement_id, period: "202009", value: "91.0" ,categoryCombo:"PQrXhDwCZBF"} ] } )) From 7f02b46d3934d945e2b073573fb4c1b18b0d3d59 Mon Sep 17 00:00:00 2001 From: pjaspers Date: Mon, 26 Oct 2020 14:40:30 +0100 Subject: [PATCH 5/5] Make spec a bit more logical --- .../fetch_data/periods_resolver_spec.rb | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb b/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb index f25ed5a..a184696 100644 --- a/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb +++ b/spec/lib/orbf/rules_engine/fetch_data/periods_resolver_spec.rb @@ -23,7 +23,7 @@ ), Orbf::RulesEngine::Formula.new( "increase", - "safe_div(achieved,sum(%{achieved_last_24_months_window_values})" + "safe_div(achieved,sum(%{achieved_last_12_months_window_values})" ) ] ) @@ -56,8 +56,16 @@ end it "resolve periods from package frequency and activity span _values" do - expect(described_class.new(quantity_package, "2016Q1").call).to eq( - %w[201501 201502 201503 201601 201602 201603 2016 2015July] - ) + previous_year_same_quarter = [ + "201501", "201502", "201503", "201504", "201505" + ] + last_12_months = [ + "201603", "201602", "201601", "201512", "201511", + "201510", "201509", "201508", "201507", "201506", + "201505", "201504" + ] + yearlies = ["2016", "2015July"] + expected = (previous_year_same_quarter + last_12_months).uniq.sort + yearlies + expect(described_class.new(quantity_package, "2016Q1").call).to eq(expected) end end