chore: use alternative isDescendant check where possible #15715
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #15715 +/- ##
============================================
- Coverage 66.30% 66.29% -0.01%
+ Complexity 31392 31385 -7
============================================
Files 3486 3486
Lines 130010 129991 -19
Branches 15207 15201 -6
============================================
- Hits 86201 86184 -17
+ Misses 36718 36715 -3
- Partials 7091 7092 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -63,5 +63,6 @@ public interface TrackedEntityMapper extends PreheatMapper<TrackedEntity> { | |||
| @Mapping(target = "name") | |||
| @Mapping(target = "attributeValues") | |||
| @Mapping(target = "user") | |||
| @Mapping(target = "parent") | |||
There was a problem hiding this comment.
@enricocolasante does this update look ok to you? It helped resolve what seemed to be a troublesome Tracker Test which I also updated below.
The TrackedEntity import wasn't getting its OrgUnit parent set.
There was a problem hiding this comment.
I guess it is ok.
We always need to be careful around that because it generates a recursive call through the orgUnit hierarchy but the number of levels should be around 7, 10 maximum.
So we are not loading too many objects in memory.
There was a problem hiding this comment.
Cool, thanks Enrico. At least ye are aware of the change in case it does cause any issues. There were a couple of Tracker tests that were failing until I made this change (as part of this PR change).
| User user = userService.getUser(USER_8); | ||
| user.setTeiSearchOrganisationUnits(new HashSet<>(user.getDataViewOrganisationUnits())); | ||
| userService.updateUser(user); | ||
| dbmsManager.clearSession(); |
There was a problem hiding this comment.
@enricocolasante the change to the TrackedEntityMapper above seems to have resolved this setup issue. Does it look ok to you?
|
Kudos, SonarCloud Quality Gate passed! |
| @@ -63,5 +63,6 @@ public interface TrackedEntityMapper extends PreheatMapper<TrackedEntity> { | |||
| @Mapping(target = "name") | |||
| @Mapping(target = "attributeValues") | |||
| @Mapping(target = "user") | |||
| @Mapping(target = "parent") | |||
There was a problem hiding this comment.
I guess it is ok.
We always need to be careful around that because it generates a recursive call through the orgUnit hierarchy but the number of levels should be around 7, 10 maximum.
So we are not loading too many objects in memory.
Summary
While working on #15639 it was discovered that a lot of other areas of the code were also making unnecessary DB calls (get
OrgUnit) when checking if anOrgUnitwas a descendant of anotherOrgUnit.This PR removes the
OrganisationUnitServicemethods (that contain DB calls):boolean isDescendant(OrganisationUnit organisationUnit, Set<OrganisationUnit> ancestors)boolean isDescendant(OrganisationUnit organisationUnit, OrganisationUnit ancestor)and replaces them with the
OrganisationUnitmethods directly (that perform checks on in-memory Objects):boolean isDescendant(Collection<OrganisationUnit> ancestors)boolean isDescendant(OrganisationUnit ancestor)Testing
Notes
Added a
parentmapping in theTrackedEntityMapperclass. This helped resolve a Tracker test with some setup issues. Tagged a Tracker Dev in the PR to check if it looks ok.