RN-603 de dg multi mapping #4081
Conversation
There was a problem hiding this comment.
Really nice work man! Love how simple this was to implement 👍
One general request, looks like with the implementation where this is a separate column, it can't be configured via the Admin Panel? I'd really love if that were possible (anything to avoid needing more migrations)
Thoughts on either moving the dataServiceMapping into the config field, or adding the appropriate boilerplate to allow configuring this as part of the Admin Panel?
Aside from that just some minor code style comments, but nothing major. Love the tests btw 👍
| }; | ||
|
|
||
| exports.up = async function (db) { | ||
| await db.runSql(`ALTER TABLE data_element ADD COLUMN data_service_mapping JSONB DEFAULT '[]'`); |
There was a problem hiding this comment.
Not a major thing, and we are planning on removing this in the long run anyway, but I guess I had supposed this would be part of the config attribute? Given it's going to be used so rarely, seems it mightn't warrant a column of its own?
| }; | ||
|
|
||
| const entity = orgUnitCode ? await this.models.entity.findOne({ code: orgUnitCode }) : null; | ||
| const { countryCode } = entity ?? {}; |
There was a problem hiding this comment.
Do we want to throw an error here if we can't find an entity/countryCode? Feels a tad unsafe that we could follow through to to the below logic with a undefined countryCode.
Bit on the fence but just wanted to raise it
| if (!map[serviceType]) { | ||
| map[serviceType] = []; | ||
| } | ||
| addToMap(serviceType, dataSource); |
There was a problem hiding this comment.
| if (!map[serviceType]) { | |
| map[serviceType] = []; | |
| } | |
| addToMap(serviceType, dataSource); | |
| addToMap(serviceType, dataSource); |
I think your nifty addToMap() function handles the case if there's no entry for the service type 👍
|
@rohan-bes Thanks for the review, I'm going to have to rework this to handle both country level and individual entities, will ping when ready again |
Sima create instance
sima create instance
IgorNadj
force-pushed
the
rn-603-de-dg-multi-mapping
branch
from
f14208a to
d29a9da
Compare
|
Closing this, will re-create PR and link back so we don't lose feedback |
Issue #: RN-603
Changes: