fix: Org Unit split being assigned to incorrect users [DHIS2-18423] #19239
Conversation
|
| @@ -224,4 +226,15 @@ Map<String, Optional<Locale>> findNotifiableUsersWithPasswordLastUpdatedBetween( | |||
| User getUserByVerificationToken(String token); | |||
|
|
|||
| User getUserByVerifiedEmail(String email); | |||
|
|
|||
| /** | |||
| * Method that retrieves all {@link User}s that have an entry for the {@link OrganisationUnit} in | |||
There was a problem hiding this comment.
Perhaps not use "Method that" in method Javadoc, as it is implicit.
| * WARNING: If this field has an empty collection, it can potentially result in all users being | ||
| * returned from the database. Check the generated query to make sure you're seeing expected | ||
| * queries and results. | ||
| */ |
There was a problem hiding this comment.
I think this phrasing is a bit misleading. This is one of many potential filters that when empty does not filter away anything. That sounds like expected behaviour to me as long as it is clear that empty means "not set" and not "match against an empty set". I think the comment we leave here should make that clear.
| * @param uid {@link OrganisationUnit} {@link UID} to match on | ||
| * @return matching {@link User}s | ||
| */ | ||
| List<User> getUsersWithOrgUnit(@Nonnull String orgUnitTable, @Nonnull UID uid); |
There was a problem hiding this comment.
I don't think we should have names of tables escape the store. That is one of the main points to have a store abstraction. Maybe use an enum?
While these 3 properties are backed by mapping tables in the DB in the object model these are properties so the wording should be accordingly.
Issue
Org Unit split feature is assigning the split org units to incorrect users (sometimes all the users in the DB in certain circumstances)
Cause
After creating the
UserQueryParamswith the sourceOrganisationUnit, thatOrganisationUnitwas being overridden to use theUser'sOrganisationUnits carrying out the split.In cases where the
Usereither had noorganisationUnits,dataViewOrganisationUnits, orteiSearchOrganisationUnits, then all theUsers in the DB would be returned from the query, resulting in allUsers getting the splitOrganisationUnits.Fix avoids using:
UserService#handleUserQueryParamswhich was overriding the sourceOrganisationUnitalready setUserStore#getUserswhich tries to generate a handcrafted SQL query taking into consideration a multitude of options & checks usingUserQueryParams(this looks too complex and tries to accommodate too many things). This resulted in returning allUsers in the DBFix
Use a dedicated query to fetch the
Users to be actioned.Fewer select queries are generated as a result of the fix, and is not proportionate to the number of users found (old query was triggering 1 extra query per user found - manual testing).
Warning added to
UserQueryParamsproperty.Testing
Automated
2 integration tests added