fix(OrganisationUnitTree): deduplicate orgunit roots #1625
Conversation
Birkbjo
changed the title
|
🚀 Deployed on https://pr-1625--dhis2-ui.netlify.app |
| * @returns {Array} - A filtered list of the minimum root units | ||
| */ | ||
| export const deduplicateOrgUnitRoots = (unitsWithLevel) => { | ||
| const sorted = unitsWithLevel.sort((a, b) => a.level - b.level) |
There was a problem hiding this comment.
Note that we could use the "path" instead of level here - because path consists of the ids, delimited by / for each level. So we could just count those (that's actually the implementation of the backend, see https://github.com/dhis2/dhis2-core/blob/master/dhis-2/dhis-api/src/main/java/org/hisp/dhis/organisationunit/OrganisationUnit.java#L610) .
But in my opinion it's more semantically correct to use the level, even if I had to add another field to the request.
| const previousUnits = array.slice(0, index) | ||
| // if a previous unit has a path that is a prefix of the current path, | ||
| // then the current path is a child and should not be included | ||
| return !previousUnits.some((pu) => ou.path.startsWith(pu.path)) |
There was a problem hiding this comment.
this would be run on all orgUnits from all levels, right? it seems like an expensive operation having a nested loop if it's performed on hundreds or thousands of org units
There was a problem hiding this comment.
Not really, this is run on the rootUnits passed to the component. This is usually just a couple of units - eg. the assigned organisation units to an user from /me api. If you're passing thousands of units, something is not right with the configuration of the instance.
Also the optimization of just checking previousUnits would prevent it from being unnecessary expensive. The inner .some() would most likely return pretty early due to an identified root. However it's probably possible to optimize it further by imperatively adding identified roots to an array - and loop through those instead of previous units - even though practically it shouldn't matter that much - since it should find an "ancestor" within the first couple of units - practically it's bound by the number of units on the lowest level.
But again; if you passed thousands of units to the component, you would send a request for each unit - which would cause more trouble than this loop. Please see my sidenote in description about that.
There was a problem hiding this comment.
lgtm - but maybe someone (@HendrikThePendric or @Mohammer5?) who know more about this component can have a look too
Implements LIBS-702
Description
In OrganisationUnitTree component, you can pass roots that are descendants of other roots, e.g. if you pass <OrganisationUnitTree roots=[OrgUnitA, OrgUnitB, OrgUnitC] /> where OrgUnitB is a descendant of OrgUnitA and where OrgUnitC is a descendant of OrgUnitA, then you presumably want to display only OrgUnitA as a root (e.g. the deduplicated root).
This PR implements a function
findCommonOrgUnitRootsthat will check the levels of the units, and only return the true "minimal roots" - and ignore rendering input IDs as roots, if another root contains it.Know issues
in-filter. It quite wasteful currently.Checklist
All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.