RN-603 de dg multi mapping #4158

IgorNadj · 2022-09-09T03:21:01Z

Issue #: RN-603

Replaces #4081

Changes:

Allow mapping a Data Element to multiple data services based on country.

Screenshots:

https://linear.app/bes/issue/RN-603#comment-2a6c52be

IgorNadj · 2022-09-12T04:57:06Z

Tests were failing because I accidentally missed a commit after a rebase onto dev, cherry-picked it back in now

rohan-bes

Nice work @IgorNadj, really love how you broke out the dataService resolution logic into a separate class.

So they were cool going with a separate country for UNFPA Warehouse? That's great, makes life easier for us 😅

Couple of code style comments, but nothing major so pre-approving

rohan-bes · 2022-09-12T06:42:30Z

packages/data-broker/src/services/DataServiceResolver.js

+   * @param {DataSource[]} dataSources
+   * @param {Entity} [orgUnit]
+   * @returns {Promise<DataServiceMapping>}
+   */


Loving all the JsDocs! 😍

rohan-bes · 2022-09-12T06:48:58Z

packages/data-broker/src/services/DataServiceResolver.js

+    const mapping = new DataServiceMapping();
+    mapping.dataElementMapping = await this.resolveDataElements(dataElements, orgUnits);
+    mapping.dataGroupMapping = await this.resolveDataGroups(dataGroups);
+    return mapping;


It's not possible to make properties readonly in javascript is it? This class feels like it ought to have immutable data...
Regardless I feel it reads a touch nice to do this without assigning the properties explicitly?

Suggested change

const mapping = new DataServiceMapping();

mapping.dataElementMapping = await this.resolveDataElements(dataElements, orgUnits);

mapping.dataGroupMapping = await this.resolveDataGroups(dataGroups);

return mapping;

const dataElementMapping = await this.resolveDataElements(dataElements, orgUnits);

const dataGroupMapping = await this.resolveDataGroups(dataGroups);

return new DataServiceMapping(dataElementMapping, dataGroupMapping);

Yeah I was thinking of just waiting until this is typescript, then can lean on that for readonly keyword

rohan-bes · 2022-09-12T07:09:01Z

packages/data-broker/src/services/DataServiceResolver.js

+        // If they all resolve to the same data service, this is fine
+        const resolveToTheSameDataService = (mappingA, mappingB) => {
+          if (mappingA.service_type !== mappingB.service_type) return false;
+          return isequal(mappingA.service_config, mappingB.service_config);
+        };


Up to you, but might be nice to move this function out to somewhere external as it doesn't require any local state 👍

Reworked, this is gone

rohan-bes · 2022-09-12T07:16:44Z

packages/data-broker/src/services/DataServiceResolver.js

+      if (deMappings.length > 1) {
+        // If they all resolve to the same data service, this is fine
+        const resolveToTheSameDataService = (mappingA, mappingB) => {
+          if (mappingA.service_type !== mappingB.service_type) return false;
+          return isequal(mappingA.service_config, mappingB.service_config);
+        };
+        for (const mappingA of deMappings) {
+          for (const mappingB of deMappings) {
+            if (mappingA === mappingB) continue;
+            if (!resolveToTheSameDataService(mappingA, mappingB)) {
+              throw new Error(
+                `Conflicting mappings found for Data Source ${
+                  dataElement.code
+                } when fetching for orgUnits ${orgUnits.map(o => o.code).join(',')}`,
+              );
+            }
+          }
+        }
+        // all data element mappings are the same, use first
+        const [deMapping] = deMappings;
+        resolved.push({
+          dataSource: dataElement,
+          service_type: deMapping.service_type,
+          config: deMapping.service_config,
+        });
+        continue;
+      }
+
+      if (deMappings.length === 1) {
+        const [deMapping] = deMappings;
+        resolved.push({
+          dataSource: dataElement,
+          service_type: deMapping.service_type,
+          config: deMapping.service_config,
+        });
+        continue;
+      }
+
+      // No mappings, use default
+      resolved.push({
+        dataSource: dataElement,
+        service_type: dataElement.service_type,
+        config: dataElement.config,
+      });


Suggested change

if (deMappings.length > 1) {

// If they all resolve to the same data service, this is fine

const resolveToTheSameDataService = (mappingA, mappingB) => {

if (mappingA.service_type !== mappingB.service_type) return false;

return isequal(mappingA.service_config, mappingB.service_config);

};

for (const mappingA of deMappings) {

for (const mappingB of deMappings) {

if (mappingA === mappingB) continue;

if (!resolveToTheSameDataService(mappingA, mappingB)) {

throw new Error(

`Conflicting mappings found for Data Source ${

dataElement.code

} when fetching for orgUnits ${orgUnits.map(o => o.code).join(',')}`,

);

}

}

}

// all data element mappings are the same, use first

const [deMapping] = deMappings;

resolved.push({

dataSource: dataElement,

service_type: deMapping.service_type,

config: deMapping.service_config,

});

continue;

}

if (deMappings.length === 1) {

const [deMapping] = deMappings;

resolved.push({

dataSource: dataElement,

service_type: deMapping.service_type,

config: deMapping.service_config,

});

continue;

}

// No mappings, use default

resolved.push({

dataSource: dataElement,

service_type: dataElement.service_type,

config: dataElement.config,

});

if (deMappings.length === 0) {

// No mappings, use default

resolved.push({

dataSource: dataElement,

service_type: dataElement.service_type,

config: dataElement.config,

});

continue;

}

// If they all resolve to the same data service, this is fine

const resolveToTheSameDataService = (mappingA, mappingB) => {

if (mappingA.service_type !== mappingB.service_type) return false;

return isequal(mappingA.service_config, mappingB.service_config);

};

for (const mappingA of deMappings) {

for (const mappingB of deMappings) {

if (mappingA === mappingB) continue;

if (!resolveToTheSameDataService(mappingA, mappingB)) {

throw new Error(

`Conflicting mappings found for Data Source ${

dataElement.code

} when fetching for orgUnits ${orgUnits.map(o => o.code).join(',')}`,

);

}

}

}

// all data element mappings are the same, use first

const [deMapping] = deMappings;

resolved.push({

dataSource: dataElement,

service_type: deMapping.service_type,

config: deMapping.service_config,

});

Optional, but this cuts down the nesting and line count a bit if you like?

Reworked, most of this is gone

IgorNadj · 2022-09-15T04:49:42Z

@rohan-bes I added new commits to take care of the pull-with-multiple-countries problem. We discussed extracting this out to a class like PullGrouper but it seemed simple enough to keep it inline in DataBroker.

rohan-bes

Requesting a change to have each uniquePull just pull data for the dataElements/countries that it's mapped to. But also totally happy to talk it through if this is tricky to support for some reason I'm missing.

Aside from that it's looking great!

rohan-bes · 2022-09-15T06:49:32Z

packages/data-broker/src/DataBroker.js

@@ -303,4 +298,71 @@ export class DataBroker {
      dataServiceMapping,
    };
  }
+
+  async getPulls(dataSources, orgUnitCodes) {


We had discussed breaking this out into a separate class right? I'm not too fussed though, feel it's okay being here as well 👍

rohan-bes · 2022-09-15T07:03:41Z

packages/data-broker/src/DataBroker.js

+  async getPulls(dataSources, orgUnitCodes) {
+    const orgUnits = await this.models.entity.find({ code: orgUnitCodes });
+
+    // Note: each service will pull for ALL org units and ALL data sources.


Is there a reason we can't fetch for just the countries/data elements in each mapping? We work out which countries and dataElements/groups belong to each along the way, so we'd just need to combine them when building the uniqueMapping?

It feels a little odd to pull for dataElements and countries that we know aren't in the data service (and also could create issues if they actually are in the data service but Tupaia isn't configured to fetch them from there...)

Discussed.

ORIGINAL FETCH: orgUnits: FJ, TO de: DrugA, DrugB MAPPING: DrugA & FJ => superset DrugB & FJ => dhis (dhis-regional) DrugA & TO => dhis (dhis-tonga) DrugB & TO => dhis (dhis-tonga) UNIQUE FETCHES: superset: orgUnits: FJ de: DrugA mapping: DrugA: superset dhis: orgUnits: TO, FJ de: DrugA, DrugB mapping: DrugA: dhis-tonga DrugB: dhis-regional DrugB: dhis-tonga

The issue is that our DataServiceMapping class and mapping resolution doesn't play nice with double ups like DrugB in dhis there.

Options are to rework the mapping, or to leave it until we do the full implementation which would drop the need for a DataServiceMapping class at all, make all this trivial. I say we leave it.

rohan-bes · 2022-09-15T07:04:52Z

packages/data-broker/src/services/DataServiceMapping.js

+   * @param {DataServiceMapping} other
+   * @returns boolean
+   */
+  equals(other) {


This warms an ex-Java dev's heart 😄 Bonus points for adding if (this === other) return true; at the start 👍

rohan-bes · 2022-09-15T07:06:44Z

packages/data-broker/src/services/DataServiceResolver.js

-        });
-        continue;
+        throw new Error(
+          `Multiple mappings found for Data Element ${dataElement.code} for country ${countryCode}`,


I wonder if this should be a constraint in the database? It seems like we'd never there to be multiple dataElementDataservices with the same country_code and data_element_code. Optional though 👍

rohan-bes

Good discussion on this one @IgorNadj. Lets get the full data services refactor done and then this whole area of code will simplify itself 👍

Base automatically changed from rn-625-disable-1-many-mapping-dhis to dev September 11, 2022 23:48

IgorNadj added 12 commits September 12, 2022 09:53

Add DataElementDataService model

77d4ca0

Add DataServiceResolver

d560286

DataBroker use DataServiceMapping and pass to services

312bfd0

DhisService use mapping

325edec

SupersetService use mapping

e21a09e

Copy constraint to new table

b30fcd2

SupersetService use mapping (2)

9394db9

data-broker add lodash.isequal

60db205

DataServiceResolver can work with multiple org units

20b105b

DataService respect multiple org units + mapping on pull

1abebc3

SupersetService use mapping (3)

b19530c

SupersetService use mapping (4)

2128be7

IgorNadj force-pushed the rn-603-de-dg-multi-mapping branch from 3713d84 to 2128be7 Compare September 11, 2022 23:54

Add data_element_data_service table

5ab7e4b

rohan-bes approved these changes Sep 12, 2022

View reviewed changes

IgorNadj added 4 commits September 15, 2022 13:18

Fix Superset API not getting new access token when expires

4d41219

Make DataServiceResolver only work for a single org unit again

cb252c9

Add DataServiceMapping.equals()

27ec605

Support multi-country use case

2d8f662

IgorNadj requested a review from rohan-bes September 15, 2022 04:49

Fix tests: add missing country

8adce9c

rohan-bes requested changes Sep 15, 2022

View reviewed changes

Merge branch 'dev' into rn-603-de-dg-multi-mapping

3256747

rohan-bes approved these changes Sep 16, 2022

View reviewed changes

Fix bug where where wrong mapping used

07fe15a

IgorNadj merged commit 7b15bde into dev Sep 19, 2022

IgorNadj deleted the rn-603-de-dg-multi-mapping branch September 19, 2022 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN-603 de dg multi mapping #4158

RN-603 de dg multi mapping #4158

IgorNadj commented Sep 9, 2022 •

edited

Loading

IgorNadj commented Sep 12, 2022

rohan-bes left a comment

rohan-bes Sep 12, 2022

rohan-bes Sep 12, 2022

IgorNadj Sep 15, 2022

rohan-bes Sep 12, 2022

IgorNadj Sep 15, 2022 •

edited

Loading

rohan-bes Sep 12, 2022

IgorNadj Sep 15, 2022 •

edited

Loading

IgorNadj commented Sep 15, 2022

rohan-bes left a comment

rohan-bes Sep 15, 2022

rohan-bes Sep 15, 2022

IgorNadj Sep 16, 2022 •

edited

Loading

rohan-bes Sep 15, 2022

rohan-bes Sep 15, 2022

rohan-bes left a comment

RN-603 de dg multi mapping #4158

RN-603 de dg multi mapping #4158

Conversation

IgorNadj commented Sep 9, 2022 • edited Loading

Issue #: RN-603

Changes:

Screenshots:

IgorNadj commented Sep 12, 2022

rohan-bes 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

IgorNadj Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

IgorNadj commented Sep 15, 2022

rohan-bes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorNadj Sep 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

IgorNadj commented Sep 9, 2022 •

edited

Loading

IgorNadj Sep 15, 2022 •

edited

Loading

IgorNadj Sep 15, 2022 •

edited

Loading

IgorNadj Sep 16, 2022 •

edited

Loading