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
There was a problem hiding this comment.
Code-wise this looks good to me.
I went through the discussion on slack linked in the Jira issue and now I'm somewhat confused that this PR exists:
I guess my only question for general implementation if anyone would ever explicitly want to have the descendant roots as independent nodes in the tree
[...]
According to Morten it's a feature that is used by some. [...] The adding to user root org units when splitting seems 100% like a bug. The deduplication is actually less clear cut. I would say it's a bug, some might say it's a feature. If we can settle on this discussion, we should probably fix it in the Web API rather than the frontend. If we want to keep the "feature" around in the backend, do we also want to keep supporting it in the UI in the same way? Or should we then have a "change root dropdown" or sth? The more I am thinking about it the more nuances I see
This is where the discussion about the deduplication ends.
So is this a feature or a bug? How many users would be negatively affected by this change?
Should we make this "fix" optional (add a prop for enabling this fix rather than making it the default behavior)?
To me it seems that the issue is not the deduplication, but bugs like the one in the splitting logic, that seem to be wrong. So I'd err on the side of caution here and turn this into a feature that needs to be actively chosen (by using a prop) and focus on fixing the bugs that are true bugs.
|
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.