RN-403 Clean up data services configuration #3783

IgorNadj · 2022-03-15T06:22:00Z

Background:

This is phase 1 of refactoring isDataRegional style config values.

Phase 1: data_element & data_group
Phase 2+: entity based resolution, detectDataServices, dhis_sync_queue, legacy report configs, legacy map overlay configs

Changes:

Phase 1 changes:

Migrate data_source config isDataRegional: bool -> dhisInstanceCode: string|null
Create DhisInstance as a first class model
Rename @tupaia/utils/dhis.js functions to legacy_*
Create new classes and functions in Data Broker which resolve which Dhis Instance to use

IgorNadj · 2022-05-16T00:35:29Z

packages/admin-panel/src/pages/resources/SurveysPage.js

@@ -94,8 +94,8 @@ const SURVEY_COLUMNS = [
          editConfig: {
            options: SERVICE_TYPES,
            setFieldsOnChange: (newValue, currentRecord) => {
-              const { isDataRegional = true } = currentRecord['data_source.config'];
-              const config = newValue === 'dhis' ? { isDataRegional } : {};
+              const { dhisInstanceCode = 'regional' } = currentRecord['data_source.config'];


Throughout the codebase we hardcode which Dhis Instance should be the default. I have not cleaned this up, the PR was already getting too big, will leave for a separate PR in the future.

IgorNadj · 2022-05-16T01:10:47Z

packages/data-broker/src/__mocks__/utils.js

@@ -0,0 +1,8 @@
+/**


The existence of this file in __mocks__ tells jest to auto mock the utils package. The alternative of doing it in the test file itself doesn't work due to a limitation of jest.

IgorNadj · 2022-05-16T01:15:48Z

packages/data-broker/src/__tests__/services/dhis/DhisService.stubs.js


-  return dhisApi;
+export const stubGetDhisApi = mockDhisApi => {


Split this into two functions because we don't always need to stub as well as mock this import.

IgorNadj · 2022-05-16T01:18:50Z

packages/database/src/migrations/20220513014400-AddDhisInstances-modifies-data.js

+];
+
+/**
+ * DataSources and Entities are linked to a specific DHIS server in many ways:


This is the main explainer for how things work... FYI

packages/database/src/migrations/20220513014400-AddDhisInstances-modifies-data.js

packages/central-server/src/database/utilities/assertCanAddDataElementInGroup.js

rohan-bes

It's looking really good man! Thanks so much for doing this!

Just a few comments, I might give this one a second pass. But first I was hoping we could have a bit of a high level chat? You've added dhis_instances here as a concept, but I had maybe thought we'd be adding data_services as a concept, where a data service could be:

A specific dhis2 instance
A data-lake instance
The internal data-api instance
etc.

Basically where we currently have data sources attached to a 'service type', they'd instead be linked to a specific service instance, which we could have many of.

Now all that wouldn't necessarily be in scope here, and we'd still need to use a lot of this cleanup code you've written anyway. Either way lets chat about it on Thursday and share thoughts?

rohan-bes · 2022-05-24T00:01:37Z

packages/database/src/migrations/20220513014400-AddDhisInstances-modifies-data.js

+ * class model. We link data_source to dhis_instance via config option `data_source.config.dhisInstanceCode: <string>`.
+ *
+ * For entity based resolution, where previously we had isDataRegional = false, we now leave dhisInstanceCode
+ * not set, which indicates no specific dhisInstance, and that we need to resolve it from the entity.


Would be keen to chat on how we think we might wanna resolve this one in the long run? It's a bit of an awkward practice, and one that I think we've really moved away from. But there's still a lot of data elements that are present in multiple DHIS2 instances...

Discussed, have created ~~https://linear.app/bes/issue/RN-537/one-data-element-per-dhis-server~~
Replaced with https://linear.app/bes/issue/RN-558/specify-dhis-server-explicitly

rohan-bes · 2022-05-24T06:01:29Z

packages/data-broker/src/services/dhis/DhisInstanceResolver.js

+    const allDhisInstanceCodes = new Set();
+    const allDhisInstances = [];
+    for (const entityCode of entityCodes) {
+      const dhisInstance = await this.resolveSingleEntityCode(entityCode);
+      allDhisInstanceCodes.add(dhisInstance.code);
+      allDhisInstances.push(dhisInstance);
+    }
+    if (allDhisInstanceCodes.size > 1) {
+      throw new Error(
+        `All entities must use the same DHIS2 instance (got [${[...allDhisInstanceCodes].join(
+          ',',
+        )}])`,
+      );
+    }
+    return allDhisInstances[0];


Suggested change

const allDhisInstanceCodes = new Set();

const allDhisInstances = [];

for (const entityCode of entityCodes) {

const dhisInstance = await this.resolveSingleEntityCode(entityCode);

allDhisInstanceCodes.add(dhisInstance.code);

allDhisInstances.push(dhisInstance);

}

if (allDhisInstanceCodes.size > 1) {

throw new Error(

`All entities must use the same DHIS2 instance (got [${[...allDhisInstanceCodes].join(

',',

)}])`,

);

}

return allDhisInstances[0];

const serverName = await legacy_getDhisServerName({

isDataRegional: false,

entityCodes,

});

return this.getFromServerName(serverName);

Would this work? Just cos legacy_getDhisServerName() takes entityCodes and already does the validation to ensure they all use the same serverName..?

I left this as multiple single calls to legacy_getDhisServerName to make mocking easier. This way we can have a test in data-broker to make sure an error gets thrown if it resolves to multiple dhis servers. I thought about having this test in utils, on the legacy_getDhisServerName function, but I thought it might make more sense to have it on the non-legacy side of the codebase rather than the legacy side. I think the reason was that the legacy side is really hard to write tests for because mocking is hard. As this class should disappear once the data is cleaned up, are you happy to leave as is?

Yup that sounds reasonable to me 👍

rohan-bes

Nice work @IgorNadj this is a great step forward! Hopefully we can pick up and carry on with phase 2 before too long.

A small comment from the first pass, but overall really happy and approving. Really appreciate the tests and comments btw (this is all becoming rather arcane knowledge)

Thoughts around testing on this? What's the regression risk looking like? Obviously there's a bunch of vizes... Does this break older survey import files too?

rohan-bes · 2022-05-26T07:44:08Z

packages/admin-panel/src/pages/resources/DataSourcesPage.js

+          fieldName: 'dhisInstanceCode',
+          optionsEndpoint: 'dhisInstances',
+          optionLabelKey: 'dhisInstances.code',
+          optionValueKey: 'dhisInstances.code',


Reckon it's a tad awkward to leave this field empty if using country specific server? The code kinda forces us in this direction tho huh? Might be nice if we could have a 'Country specific server' option in the UI which sets it as undefined in the backend...

Yeah I don't love it either. ~~Were you thinking something like this? https://linear.app/bes/issue/RN-403#comment-deae01b8~~

I have set this field label to "DHIS Server", and this should encourage people to fill it out.

As we don't need the entity based resolution functionality (In this investigation I confirmed we don't have any data elements that are stored on multiple DHIS Servers, so there is a 1:1 map), after changing all of these from null to a specific DHIS Server, I will create a new PR to set this column to NOT NULL.

Here is the new card for this cleanup: https://linear.app/bes/issue/RN-558/specify-dhis-server-explicitly

rohan-bes · 2022-05-26T07:50:46Z

packages/central-server/src/database/utilities/assertCanAddDataElementInGroup.js

+const areDifferentAllowingNull = (a, b) => {
+  if (a === null && b === null) return false;
+  if (a === null && b !== null) return true;
+  if (a !== null && b === null) return true;
+  return a !== b;
+};


Out of curiousity, how is this different to a !== b? null !== null => false right?

Ah good catch, updated.

packages/data-broker/src/__mocks__/utils.js

packages/data-broker/src/services/dhis/getDhisApi.js

packages/database/src/migrations/20220512232924-CreateDhisInstanceTable-modifies-schema.js

packages/utils/src/legacyDhis.js

IgorNadj · 2022-05-27T06:02:48Z

You've added dhis_instances here as a concept, but I had maybe thought we'd be adding data_services as a concept

Discussed, data_service was the plan originally, but it wound up being harder than expected because doing so would mean removing data_source.service_type which would mean lots of changes throughout the codebase to find the service type for a data source, converting sync code to async for the lookup etc. We still want to do this, I just reduced the scope of this PR to get a chewable bite out of the problem.

IgorNadj · 2022-06-03T03:10:41Z

TODO: remove ability for users to import/set dhisInstanceCode as null. We don't want new dataElements/groups to use entity based resolution. There are no cases where want to do this out of necessity where a DE/DG lives on multiple DHIS Servers legitimately: https://linear.app/bes/issue/RN-537#comment-bf70ca96

Have created a cleanup card, after which I will create a new PR to not allow null.

IgorNadj · 2022-06-16T01:51:53Z

Have rebased on dev, fixed up PR from feedback.

rohan-bes

Great stuff @IgorNadj really appreciate all this work!

merge in latest dev for testing

merge latest dev in for testing

IgorNadj marked this pull request as draft March 15, 2022 06:22

IgorNadj added 9 commits May 13, 2022 09:30

Refactor: deprecate getDhisConfig

36e21fb

Export legacy_getDhisServerName from utils

3e746c9

Add hardcoded legacy to modern conversion function

738b22f

Add new model DhisInstance

4b53d07

Migrate data_source config isDataRegional to dhisInstanceCode

1a7da4c

Create DhisInstanceResolver

91bc206

Centralise responsibility of getting DHIS Api into single file.

64e8855

Use newly centralised getDhisApi in DhisService

f4e2189

Use dhisInstanceCode instead of isDataRegional throughout codebase

ba2f6a2

IgorNadj force-pushed the rn-403-clean-up-data-services branch from fb18438 to ba2f6a2 Compare May 13, 2022 06:54

IgorNadj added 3 commits May 16, 2022 10:17

Add GETDhisInstances route

f06ea08

Update data element - group check for dhisInstanceCode

18d7758

Update admin panel for dhisInstanceCode

e7e54eb

IgorNadj marked this pull request as ready for review May 16, 2022 00:20

Fix test

bd10ee6

IgorNadj changed the title ~~Refactor: deprecate getDhisConfig~~ Clean up data services configuration May 16, 2022

IgorNadj commented May 16, 2022

View reviewed changes

IgorNadj changed the title ~~Clean up data services configuration~~ RN-403 Clean up data services configuration May 16, 2022

IgorNadj added 2 commits May 16, 2022 15:15

DRY route handler

4dd6b2e

Change dhisInstanceCode to be string | null to limit possible values

52799d9

IgorNadj commented May 16, 2022

View reviewed changes

packages/central-server/src/database/utilities/assertCanAddDataElementInGroup.js Show resolved Hide resolved

rohan-bes reviewed May 24, 2022

View reviewed changes

rohan-bes approved these changes May 26, 2022

View reviewed changes

IgorNadj added 2 commits June 14, 2022 14:41

Update admin panel form labels

ef09ab6

Merge branch 'dev' into rn-403-clean-up-data-services

c5ee57b

IgorNadj force-pushed the rn-403-clean-up-data-services branch from ef6e5f0 to c5ee57b Compare June 16, 2022 01:51

PR feedback

4eacbfa

IgorNadj requested a review from rohan-bes June 16, 2022 01:52

IgorNadj added 3 commits June 16, 2022 13:18

Fix merge issues

a312bc9

Merge branch 'dev' into rn-403-clean-up-data-services

c5d8c17

Fix tests, add missing data source types

02a8452

rohan-bes approved these changes Jun 27, 2022

View reviewed changes

avaek and others added 10 commits July 4, 2022 13:31

Merge pull request #4013 from beyondessential/dev

b1bef10

merge in latest dev for testing

Fix merge from dev: update table names

a748e79

Fix migration, syntax errors

9eac74c

Merge pull request #4020 from beyondessential/dev

cdce529

merge in latest dev for testing

Merge branch 'dev' into rn-403-clean-up-data-services

88ced18

Hotfix: force install internal deps

82aa11f

Fix dhis push async bug

70f7b94

Minor: better logging dhis sync push errors

11cadd5

Merge pull request #4040 from beyondessential/dev

4d824ed

merge latest dev in for testing

Merge branch 'dev' into rn-403-clean-up-data-services

f6413a5

IgorNadj force-pushed the rn-403-clean-up-data-services branch from db491d7 to f6413a5 Compare August 5, 2022 03:44

IgorNadj merged commit 9ca901c into dev Aug 5, 2022

IgorNadj deleted the rn-403-clean-up-data-services branch August 5, 2022 05:56

IgorNadj mentioned this pull request Aug 5, 2022

Release 2022-32 #4050

Closed

3 tasks

chris-pollard mentioned this pull request Aug 8, 2022

Release 2022-32 #4074

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN-403 Clean up data services configuration #3783

RN-403 Clean up data services configuration #3783

IgorNadj commented Mar 15, 2022 •

edited

Loading

IgorNadj May 16, 2022

IgorNadj May 16, 2022 •

edited

Loading

IgorNadj May 16, 2022

IgorNadj May 16, 2022

rohan-bes left a comment

rohan-bes May 24, 2022

IgorNadj May 27, 2022 •

edited

Loading

rohan-bes May 24, 2022

IgorNadj Jun 16, 2022

rohan-bes Jun 27, 2022

rohan-bes left a comment

rohan-bes May 26, 2022

IgorNadj May 30, 2022 •

edited

Loading

rohan-bes May 26, 2022

IgorNadj Jun 16, 2022

IgorNadj commented May 27, 2022

IgorNadj commented Jun 3, 2022 •

edited

Loading

IgorNadj commented Jun 16, 2022

rohan-bes left a comment


		return dhisApi;
		export const stubGetDhisApi = mockDhisApi => {

RN-403 Clean up data services configuration #3783

RN-403 Clean up data services configuration #3783

Conversation

IgorNadj commented Mar 15, 2022 • edited Loading

Background:

Changes:

Choose a reason for hiding this comment

IgorNadj May 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohan-bes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj May 27, 2022 • edited Loading

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

rohan-bes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj commented May 27, 2022

IgorNadj commented Jun 3, 2022 • edited Loading

IgorNadj commented Jun 16, 2022

rohan-bes left a comment

Choose a reason for hiding this comment

IgorNadj commented Mar 15, 2022 •

edited

Loading

IgorNadj May 16, 2022 •

edited

Loading

IgorNadj May 27, 2022 •

edited

Loading

IgorNadj May 30, 2022 •

edited

Loading

IgorNadj commented Jun 3, 2022 •

edited

Loading