adding new indicators for ethiopia #5421

aagrawalrtsl · 2024-05-06T13:27:02Z

Story card: https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex
https://app.shortcut.com/simpledotorg/story/12432/export-treatment-outcome-for-cohort-of-patient-registered-for-hypertension-treatment-six-months-prior-to-the-reporting-peridod
https://app.shortcut.com/simpledotorg/story/12431/export-total-number-of-cohort-hypertensive-patients-enrolled-to-care-six-month-prior-to-the-reporting-month
https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex
https://app.shortcut.com/simpledotorg/story/12430/export-hypertensive-patients-by-timing-of-enrollment
https://app.shortcut.com/simpledotorg/story/12433/export-hypertensive-patients-enrolled-to-care-by-type-of-treatment

Because

There are new indicators required for Ethiopia DHIS2

This addresses

This export is already behind a feature flag "dhis2_export". Hence, using the same one and not adding any new flags
Removing the old indicators
Adding functionality for new indicators.

Test instructions

Enable "dhis2_export" feature flag.
Ensure FacilityBusinessIdentifier.identifier_id is linked to the correct facility/organisation id in the DHIS2 that is used for testing.
Set proper credentials in the test environment for the following variables
DHIS2_PASSWORD
DHIS2_URL
DHIS2_USERNAME
DHIS2_DATA_ELEMENTS_FILE

Ensure the file used for DHIS2_DATA_ELEMENTS_FILE has the correct data_element ID mappings
run rake dhis2:ethiopia_export

Data should be exported to DHIS2

github-actions · 2024-05-19T01:22:40Z

This PR is stale. Please review/update it or close it.

PriyangaPKini · 2024-05-23T10:05:58Z

Story card: https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex https://app.shortcut.com/simpledotorg/story/12432/export-treatment-outcome-for-cohort-of-patient-registered-for-hypertension-treatment-six-months-prior-to-the-reporting-peridod https://app.shortcut.com/simpledotorg/story/12431/export-total-number-of-cohort-hypertensive-patients-enrolled-to-care-six-month-prior-to-the-reporting-month https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex https://app.shortcut.com/simpledotorg/story/12430/export-hypertensive-patients-by-timing-of-enrollment https://app.shortcut.com/simpledotorg/story/12433/export-hypertensive-patients-enrolled-to-care-by-type-of-treatment

Because

There are new indicators required for Ethiopia DHIS2

This addresses

This export is already behind a feature flag "dhis2_export". Hence, using the same one and not adding any new flags Removing the old indicators Adding functionality for new indicators.

Test instructions

Enable "dhis2_export" feature flag. Ensure FacilityBusinessIdentifier.identifier_id is linked to the correct facility/organisation id in the DHIS2 that is used for testing. Set proper credentials in the test environment for the following variables DHIS2_PASSWORD DHIS2_URL DHIS2_USERNAME DHIS2_DATA_ELEMENTS_FILE

Ensure the file used for DHIS2_DATA_ELEMENTS_FILE has the correct data_element ID mappings run rake dhis2:ethiopia_export

Data should be exported to DHIS2

Do you have a sample DHIS2_DATA_ELEMENTS_FILE that you used for testing?

aagrawalrtsl · 2024-05-23T10:13:05Z

Story card: https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex https://app.shortcut.com/simpledotorg/story/12432/export-treatment-outcome-for-cohort-of-patient-registered-for-hypertension-treatment-six-months-prior-to-the-reporting-peridod https://app.shortcut.com/simpledotorg/story/12431/export-total-number-of-cohort-hypertensive-patients-enrolled-to-care-six-month-prior-to-the-reporting-month https://app.shortcut.com/simpledotorg/story/12468/hypertensive-patients-enrolled-to-care-by-age-and-sex https://app.shortcut.com/simpledotorg/story/12430/export-hypertensive-patients-by-timing-of-enrollment https://app.shortcut.com/simpledotorg/story/12433/export-hypertensive-patients-enrolled-to-care-by-type-of-treatment

Because

There are new indicators required for Ethiopia DHIS2

This addresses

This export is already behind a feature flag "dhis2_export". Hence, using the same one and not adding any new flags Removing the old indicators Adding functionality for new indicators.

Test instructions

Enable "dhis2_export" feature flag. Ensure FacilityBusinessIdentifier.identifier_id is linked to the correct facility/organisation id in the DHIS2 that is used for testing. Set proper credentials in the test environment for the following variables DHIS2_PASSWORD DHIS2_URL DHIS2_USERNAME DHIS2_DATA_ELEMENTS_FILE
Ensure the file used for DHIS2_DATA_ELEMENTS_FILE has the correct data_element ID mappings run rake dhis2:ethiopia_export
Data should be exported to DHIS2

Do you have a sample DHIS2_DATA_ELEMENTS_FILE that you used for testing?

I have checked in config/data/dhis2/ethiopia-staging.yml which I used for testing. While testing this would fail for htn_cohort_registered because I have not got the ID for this. Will update that once I receive it from the team

tfidfwastaken

Left a few comments about the PR—I usually categorise my comments:

nonblocking: Any suggestion with this label means that I don't consider it a blocker to merge.
question: This is me asking for a clarification that may or may not lead to any change in the PR.
nitpick: Usually something subjective, you may choose to ignore it, never a blocker to merge

Overall, most things look good 👍

I mostly want to understand if there's a reason why we are bucketing on the application side, rather than the database side (which is what we did for the Bangladesh Job). I don't hold a strong opinion on doing it either way, but might be nice to reuse the same methods that do it on the DB side.

There's also a small correction on the age disaggregation logic, where we should use the current_age column and not the age column, unless we explicitly want the age at the time of registration for some reason.

tfidfwastaken · 2024-06-03T09:41:04Z

app/jobs/dhis2/ethiopia_exporter_job.rb

+    def format_gender_age_data(patient_counts)
+      formatted_gender_age_data = {}
+      REQUIRED_GENDERS.each do |gender|
+        gender_age_data = patient_counts.select { |gender_age_array, _patient_count| gender_age_array.first == gender }
+        AGE_RANGES.each do |age_range|
+          gender_age_key = "#{age_range.first}_#{age_range.last}_#{gender}"
+          formatted_gender_age_data[gender_age_key] = gender_age_data.select { |gender_age_array, _patient_count| gender_age_array.last >= age_range.first && gender_age_array.last <= age_range.last }.values.sum
+        end
+      end
+      formatted_gender_age_data
+    end


[question] It seems like the data is being grouped by gender and age and summed up in buckets on the application side. The Dhis2ExporterJob already has a method that does something quite similar on the server side (confer usages on the Bangladesh Exporter). Is there a reason we want to do this disaggregation differently here, instead of reusing that method?

[suggestion, nonblocking] Also this method is called format_* which suggests that it is merely formatting data, but it looks like there is some aggregation and bucketing logic here as well. Consider separating these concerns, here and in other format_* methods.

tfidfwastaken · 2024-06-03T09:52:05Z

app/queries/patient_states/hypertension/registrations_under_care_gender_age_aggregated_query.rb

+      )
+      .where(hypertension: "yes")
+      .where(htn_care_state: "under_care")
+      .group(:gender, :age).count


[issue]

Suggested change

.group(:gender, :age).count

.group(:gender, :current_age).count

One quirk in our reporting table—the age column of reporting_patient_states is the age at the time of registration (if available) and not the current age. I think the intention here would be to bucket based on current age, which is stored in the current_age column (not ideal, I know :/). You can check out the columns on Metabase to see how the values differ if you are curious.

tfidfwastaken · 2024-06-03T10:49:43Z

app/jobs/dhis2/ethiopia_exporter_job.rb

+      formatted_gender_age_data
+    end
+
+    def format_facility_period_data(facility_data, facility_identifier, period)


[suggestion, nonblocking] I didn't immediately realise the superclass is being overridden here. There are a few things about this that I'm not too comfortable with from a long-term code maintenance perspective:

The superclass (Dhis2ExporterJob) is no longer exactly substitutable with this subclass. This is because the Ethiopia exporter job has extra keys in the output that the superclass has no knowledge of.

In the call sites of this method, it's harder to tell whether the superclass method or the subclass method was called—the control mechanism is now split up. In a hierarchy like this, it's a lot simpler to have the superclass be the source for issuing control for shared functionality—the subclass may only specialise on certain bits.

From what I can tell in the code, we are actually never using the base class version of format_facility_period_data at all—consider removing it entirely and have it raise a NotImplementedError, since every exporter that we have written is overriding it with its own logic.

Ideally these methods should be implemented in the sub classes, but there is a job "BangladeshExporterJob" which uses the superclass method, so keeping it for now.

tfidfwastaken · 2024-06-03T10:56:21Z

app/jobs/dhis2/ethiopia_exporter_job.rb

+
+    def format_treatment_data(under_care_patients)
+      htn_under_care_patient_count = under_care_patients.count
+      htn_under_care_patient_lsm_count = under_care_patients.count { |patient| patient.prescription_drug_id.nil? }


[question]
LSM = lifestyle modification?

I don't know how they are managing things in Ethiopia, but in some regions I have seen that a prescription drug is added with the name "lsm" or "lifestyle modification". Does this happen in Ethiopia, and if yes, should we count those?

tfidfwastaken · 2024-06-03T10:58:07Z

config/data/dhis2/ethiopia-staging.yml

+
+dhis2_treatment_category_elements:
+  lsm: Pstp0QNNjm5
+  pharma_mangement: cHCvSLCnh18


[question] Is this a typo from our side or from the DHIS2 side? If it's the latter, than no action necessary.

tfidfwastaken · 2024-06-03T11:11:06Z

spec/jobs/dhis2/ethiopia_exporter_job_spec.rb

+    it "returns the data in gender and age range key" do
+      output = subject
+      expect(output.count).to eq 8
+      expect(output["18_29_male"]).to eq 0


[question] What happens if we receive data outside the given ranges, like say, when there are patients that are under the age of 18? Are we supposed to discard it? If yes, can we include that case in the test?

tfidfwastaken · 2024-06-03T11:16:15Z

app/jobs/dhis2/ethiopia_exporter_job.rb

+      CountryConfig.dhis2_data_elements.fetch(category_element_key.to_sym)
+    end
+
+    def get_attiribute_option_mapping


[nitpick, nonblocking] This seems to return a singular ID and not a mapping. How about renaming this to attribute_option_id.

So the call site would look like this:

attribute_option_combo: attribute_option_id,

no need to "get" it into a variable that is used just once that way.

similarly the method above this could be named category_element_map

aagrawalrtsl requested a review from jamiecarter7 as a code owner May 6, 2024 13:27

aagrawalrtsl requested review from govindkrjoshi and removed request for jamiecarter7 May 7, 2024 08:20

aagrawalrtsl force-pushed the 11492-ethiopia-dhis2-indicators branch from d21c49c to 1727a13 Compare May 8, 2024 02:22

github-actions bot added the stale-pr label May 19, 2024

aagrawalrtsl removed the stale-pr label May 22, 2024

aagrawalrtsl force-pushed the 11492-ethiopia-dhis2-indicators branch from 5f13130 to 9f6db88 Compare May 29, 2024 05:45

aagrawalrtsl assigned tfidfwastaken May 30, 2024

tfidfwastaken requested changes Jun 3, 2024

View reviewed changes

Ayushi Agrawal added 12 commits June 5, 2024 10:15

adding new indicators for ethiopia

a3bb677

fixing spec failures

99ae1ae

lint fixes

8413fb2

adding attribute option combo

5e55bdb

fix failing test

433e194

lint fixes

fed0367

correcting the ID for cohort data element

4a19997

lint fixes

4edcb3f

PR feedback changes

c068270

removing superclass format method implementation

e76f703

minor fix

a6a34c5

fixing spec failures

e91a7d5

aagrawalrtsl force-pushed the 11492-ethiopia-dhis2-indicators branch from 57a1020 to e91a7d5 Compare June 5, 2024 04:45

Ayushi Agrawal added 3 commits June 5, 2024 15:32

fixing specs

34b0c90

lint fixes

3baaa02

lint fix

edb2bda

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding new indicators for ethiopia #5421

adding new indicators for ethiopia #5421

aagrawalrtsl commented May 6, 2024

github-actions bot commented May 19, 2024

PriyangaPKini commented May 23, 2024

Because

This addresses

Test instructions

aagrawalrtsl commented May 23, 2024

Because

This addresses

Test instructions

tfidfwastaken left a comment •

edited

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

aagrawalrtsl Jun 4, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

tfidfwastaken Jun 3, 2024

	.group(:gender, :age).count
	.group(:gender, :current_age).count

adding new indicators for ethiopia #5421

Are you sure you want to change the base?

adding new indicators for ethiopia #5421

Conversation

aagrawalrtsl commented May 6, 2024

Because

This addresses

Test instructions

github-actions bot commented May 19, 2024

PriyangaPKini commented May 23, 2024

Because

This addresses

Test instructions

aagrawalrtsl commented May 23, 2024

Because

This addresses

Test instructions

tfidfwastaken left a comment • edited

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

tfidfwastaken left a comment •

edited