fix: Org Unit split being assigned to incorrect users [DHIS2-18423] (#…

…19239) * fix: Org unit split with correct org units and users [DHIS2-18423] * fix: Add test [DHIS2-18423] * fix: clean up [DHIS2-18423] * fix: Use enum for user org property [DHIS2-18423] * fix: Update user query param comment [DHIS2-18423] * fix: clean up
dhis2 · Nov 21, 2024 · f5e9f17 · f5e9f17
1 parent defe1c6
commit f5e9f17
Show file tree

Hide file tree

Showing 9 changed files with 216 additions and 9 deletions.
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserOrgUnitProperty.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserOrgUnitProperty.java
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2004-2024, University of Oslo
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ * Redistributions of source code must retain the above copyright notice, this
+ * list of conditions and the following disclaimer.
+ *
+ * Redistributions in binary form must reproduce the above copyright notice,
+ * this list of conditions and the following disclaimer in the documentation
+ * and/or other materials provided with the distribution.
+ * Neither the name of the HISP project nor the names of its contributors may
+ * be used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+ * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
+ * ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.hisp.dhis.user;
+
+public enum UserOrgUnitProperty {
+  ORG_UNITS("organisationUnits"),
+  DATA_VIEW_ORG_UNITS("dataViewOrganisationUnits"),
+  TEI_SEARCH_ORG_UNITS("teiSearchOrganisationUnits");
+
+  private final String value;
+
+  UserOrgUnitProperty(String value) {
+    this.value = value;
+  }
+
+  public String getValue() {
+    return value;
+  }
+}
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserQueryParams.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserQueryParams.java
@@ -75,6 +75,7 @@ public class UserQueryParams {
 
   @ToString.Include private UserInvitationStatus invitationStatus;
 
+  /** If empty, no matching condition will be generated */
   private Set<OrganisationUnit> organisationUnits = new HashSet<>();
 
   private Set<UserGroup> userGroups = new HashSet<>();

diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserService.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserService.java
@@ -41,11 +41,13 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import org.hisp.dhis.common.IdentifiableObject;
+import org.hisp.dhis.common.UID;
 import org.hisp.dhis.dataset.DataSet;
 import org.hisp.dhis.feedback.ErrorCode;
 import org.hisp.dhis.feedback.ErrorReport;
 import org.hisp.dhis.feedback.ForbiddenException;
 import org.hisp.dhis.feedback.NotFoundException;
+import org.hisp.dhis.organisationunit.OrganisationUnit;
 
 /**
  * @author Chau Thu Tran
@@ -891,4 +893,14 @@ void validateTwoFactorUpdate(boolean before, boolean after, User userToModify)
   boolean isEmailVerified(User currentUser);
 
   User getUserByVerificationToken(String token);
+
+  /**
+   * Method that retrieves all {@link User}s that have an entry for the {@link OrganisationUnit} in
+   * the given table
+   *
+   * @param orgUnitProperty {@link UserOrgUnitProperty} used to search
+   * @param uid {@link OrganisationUnit} {@link UID} to match on
+   * @return matching {@link User}s
+   */
+  List<User> getUsersWithOrgUnit(@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull UID uid);
 }
diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserStore.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserStore.java
@@ -38,6 +38,8 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import org.hisp.dhis.common.IdentifiableObjectStore;
+import org.hisp.dhis.common.UID;
+import org.hisp.dhis.organisationunit.OrganisationUnit;
 
 /**
  * @author Nguyen Hong Duc
@@ -224,4 +226,14 @@ Map<String, Optional<Locale>> findNotifiableUsersWithPasswordLastUpdatedBetween(
   User getUserByVerificationToken(String token);
 
   User getUserByVerifiedEmail(String email);
+
+  /**
+   * Retrieves all {@link User}s that have an entry for the {@link OrganisationUnit} in the given
+   * table
+   *
+   * @param orgUnitProperty {@link UserOrgUnitProperty} used to search
+   * @param uid {@link OrganisationUnit} {@link UID} to match on
+   * @return matching {@link User}s
+   */
+  List<User> getUsersWithOrgUnit(@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull UID uid);
 }
diff --git a/...ration/src/main/java/org/hisp/dhis/split/orgunit/handler/MetadataOrgUnitSplitHandler.java b/...ration/src/main/java/org/hisp/dhis/split/orgunit/handler/MetadataOrgUnitSplitHandler.java
@@ -29,14 +29,14 @@
 
 import com.google.common.collect.Sets;
 import java.util.List;
-import java.util.Set;
 import lombok.RequiredArgsConstructor;
+import org.hisp.dhis.common.UID;
 import org.hisp.dhis.configuration.Configuration;
 import org.hisp.dhis.configuration.ConfigurationService;
 import org.hisp.dhis.organisationunit.OrganisationUnit;
 import org.hisp.dhis.split.orgunit.OrgUnitSplitRequest;
 import org.hisp.dhis.user.User;
-import org.hisp.dhis.user.UserQueryParams;
+import org.hisp.dhis.user.UserOrgUnitProperty;
 import org.hisp.dhis.user.UserService;
 import org.springframework.stereotype.Service;
 
@@ -92,11 +92,10 @@ public void splitOrganisationUnits(OrgUnitSplitRequest request) {
   }
 
   public void splitUsers(OrgUnitSplitRequest request) {
-    Set<OrganisationUnit> source = Sets.newHashSet(request.getSource());
+    UID sourceOrgUnitUid = UID.of(request.getSource().getUid());
 
     List<User> dataCaptureUsers =
-        userService.getUsers(
-            new UserQueryParams().setCanSeeOwnRoles(true).setOrganisationUnits(source));
+        userService.getUsersWithOrgUnit(UserOrgUnitProperty.ORG_UNITS, sourceOrgUnitUid);
 
     dataCaptureUsers.forEach(
         u -> {
@@ -105,8 +104,7 @@ public void splitUsers(OrgUnitSplitRequest request) {
         });
 
     List<User> dataViewUsers =
-        userService.getUsers(
-            new UserQueryParams().setCanSeeOwnRoles(true).setDataViewOrganisationUnits(source));
+        userService.getUsersWithOrgUnit(UserOrgUnitProperty.DATA_VIEW_ORG_UNITS, sourceOrgUnitUid);
 
     dataViewUsers.forEach(
         u -> {
@@ -115,8 +113,7 @@ public void splitUsers(OrgUnitSplitRequest request) {
         });
 
     List<User> teiSearchOrgUnits =
-        userService.getUsers(
-            new UserQueryParams().setCanSeeOwnRoles(true).setTeiSearchOrganisationUnits(source));
+        userService.getUsersWithOrgUnit(UserOrgUnitProperty.TEI_SEARCH_ORG_UNITS, sourceOrgUnitUid);
 
     teiSearchOrgUnits.forEach(
         u -> {

diff --git a/.../dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserService.java b/.../dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserService.java
@@ -68,6 +68,7 @@
 import org.hisp.dhis.common.CodeGenerator;
 import org.hisp.dhis.common.IdentifiableObject;
 import org.hisp.dhis.common.PasswordGenerator;
+import org.hisp.dhis.common.UID;
 import org.hisp.dhis.common.UserOrgUnitType;
 import org.hisp.dhis.commons.filter.FilterUtils;
 import org.hisp.dhis.dataset.DataSet;
@@ -1577,6 +1578,12 @@ public User getUserByVerificationToken(String token) {
     return userStore.getUserByVerificationToken(token);
   }
 
+  @Override
+  public List<User> getUsersWithOrgUnit(
+      @Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull UID uid) {
+    return userStore.getUsersWithOrgUnit(orgUnitProperty, uid);
+  }
+
   @Override
   public boolean isEmailVerified(User user) {
     return user.getEmail().equals(user.getVerifiedEmail());

diff --git a/...ices/dhis-service-core/src/main/java/org/hisp/dhis/user/hibernate/HibernateUserStore.java b/...ices/dhis-service-core/src/main/java/org/hisp/dhis/user/hibernate/HibernateUserStore.java
@@ -63,6 +63,7 @@
 import org.hibernate.query.Query;
 import org.hisp.dhis.cache.QueryCacheManager;
 import org.hisp.dhis.common.IdentifiableObjectUtils;
+import org.hisp.dhis.common.UID;
 import org.hisp.dhis.common.UserOrgUnitType;
 import org.hisp.dhis.common.hibernate.HibernateIdentifiableObjectStore;
 import org.hisp.dhis.commons.util.SqlHelper;
@@ -78,6 +79,7 @@
 import org.hisp.dhis.user.UserAccountExpiryInfo;
 import org.hisp.dhis.user.UserGroup;
 import org.hisp.dhis.user.UserInvitationStatus;
+import org.hisp.dhis.user.UserOrgUnitProperty;
 import org.hisp.dhis.user.UserQueryParams;
 import org.hisp.dhis.user.UserStore;
 import org.springframework.context.ApplicationEventPublisher;
@@ -642,4 +644,18 @@ public User getUserByVerifiedEmail(String email) {
     query.setParameter("email", email);
     return query.uniqueResult();
   }
+
+  @Override
+  public List<User> getUsersWithOrgUnit(
+      @Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull UID uid) {
+    return getQuery(
+            """
+            select distinct u from User u
+            left join fetch u.%s ous
+            where ous.uid = :uid
+            """
+                .formatted(orgUnitProperty.getValue()))
+        .setParameter("uid", uid.getValue())
+        .getResultList();
+  }
 }
diff --git a/...s-test-integration/src/test/java/org/hisp/dhis/split/orgunit/OrgUnitSplitServiceTest.java b/...s-test-integration/src/test/java/org/hisp/dhis/split/orgunit/OrgUnitSplitServiceTest.java
@@ -34,6 +34,10 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import com.google.common.collect.Lists;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
 import org.hisp.dhis.common.IdentifiableObjectManager;
 import org.hisp.dhis.common.IllegalQueryException;
 import org.hisp.dhis.dataset.DataSet;
@@ -44,7 +48,9 @@
 import org.hisp.dhis.period.PeriodType;
 import org.hisp.dhis.program.Program;
 import org.hisp.dhis.test.integration.PostgresIntegrationTestBase;
+import org.hisp.dhis.user.User;
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.transaction.annotation.Transactional;
@@ -159,4 +165,76 @@ void testSplit() {
     assertNotNull(idObjectManager.get(OrganisationUnit.class, ouB.getUid()));
     assertNotNull(idObjectManager.get(OrganisationUnit.class, ouC.getUid()));
   }
+
+  @Test
+  @DisplayName("OrgUnit split has correct users for new split org units")
+  void orgUnitSplitCorrectUsersTest() {
+    // given multiple users
+    // each of which have different kinds of access to the same org unit
+    Set<OrganisationUnit> source = new HashSet<>(Collections.singletonList(ouA));
+
+    User userWithNoOrgUnits = createAndAddUser("user1");
+
+    User userDataCaptureOrgUnits = createAndAddUser("user2");
+    userDataCaptureOrgUnits.setOrganisationUnits(source);
+
+    User userDataViewOrgUnits = createAndAddUser("user3");
+    userDataViewOrgUnits.setDataViewOrganisationUnits(source);
+
+    User userTeiSearchOrgUnits = createAndAddUser("user4");
+    userTeiSearchOrgUnits.setTeiSearchOrganisationUnits(source);
+
+    User userAllOrgUnits = createAndAddUser("user5");
+    userAllOrgUnits.setOrganisationUnits(source);
+    userAllOrgUnits.setDataViewOrganisationUnits(source);
+    userAllOrgUnits.setTeiSearchOrganisationUnits(source);
+
+    idObjectManager.save(
+        List.of(
+            userWithNoOrgUnits,
+            userAllOrgUnits,
+            userDataCaptureOrgUnits,
+            userDataViewOrgUnits,
+            userTeiSearchOrgUnits));
+
+    assertUserHasExpectedOrgUnits(userAllOrgUnits, 1, 1, 1);
+    assertUserHasExpectedOrgUnits(userWithNoOrgUnits, 0, 0, 0);
+    assertUserHasExpectedOrgUnits(userDataCaptureOrgUnits, 1, 0, 0);
+    assertUserHasExpectedOrgUnits(userDataViewOrgUnits, 0, 1, 0);
+    assertUserHasExpectedOrgUnits(userTeiSearchOrgUnits, 0, 0, 1);
+
+    OrgUnitSplitRequest request =
+        new OrgUnitSplitRequest.Builder()
+            .withSource(ouA)
+            .addTargets(Set.of(ouB, ouC))
+            .withPrimaryTarget(ouB)
+            .withDeleteSource(true)
+            .build();
+
+    // when
+    service.split(request);
+
+    // then all users should have the appropriate access for the split org units
+    assertUserHasExpectedOrgUnits(userAllOrgUnits, 2, 2, 2);
+    assertUserHasExpectedOrgUnits(userWithNoOrgUnits, 0, 0, 0);
+    assertUserHasExpectedOrgUnits(userDataCaptureOrgUnits, 2, 0, 0);
+    assertUserHasExpectedOrgUnits(userDataViewOrgUnits, 0, 2, 0);
+    assertUserHasExpectedOrgUnits(userTeiSearchOrgUnits, 0, 0, 2);
+  }
+
+  private void assertUserHasExpectedOrgUnits(
+      User user, int orgUnits, int dataViewOrgUnits, int teiSearchOrgUnits) {
+    assertEquals(
+        orgUnits,
+        user.getOrganisationUnits().size(),
+        "user should have %s org units".formatted(orgUnits));
+    assertEquals(
+        dataViewOrgUnits,
+        user.getDataViewOrganisationUnits().size(),
+        "user should have %s data view org units".formatted(dataViewOrgUnits));
+    assertEquals(
+        teiSearchOrgUnits,
+        user.getTeiSearchOrganisationUnits().size(),
+        "user should have %s tei search org units".formatted(teiSearchOrgUnits));
+  }
 }
diff --git a/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/user/UserStoreTest.java b/dhis-2/dhis-test-integration/src/test/java/org/hisp/dhis/user/UserStoreTest.java
@@ -27,34 +27,41 @@
  */
 package org.hisp.dhis.user;
 
+import static io.hypersistence.utils.jdbc.validator.SQLStatementCountValidator.assertSelectCount;
 import static org.hisp.dhis.util.DateUtils.parseDate;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import io.hypersistence.utils.jdbc.validator.SQLStatementCountValidator;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
+import org.hisp.dhis.common.UID;
 import org.hisp.dhis.dbms.DbmsManager;
 import org.hisp.dhis.organisationunit.OrganisationUnit;
 import org.hisp.dhis.organisationunit.OrganisationUnitService;
+import org.hisp.dhis.test.config.QueryCountDataSourceProxy;
 import org.hisp.dhis.test.integration.PostgresIntegrationTestBase;
 import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.TestInstance.Lifecycle;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.test.context.ContextConfiguration;
 import org.springframework.transaction.annotation.Transactional;
 
 /**
  * @author Nguyen Hong Duc
  */
 @TestInstance(Lifecycle.PER_CLASS)
 @Transactional
+@ContextConfiguration(classes = {QueryCountDataSourceProxy.class})
 class UserStoreTest extends PostgresIntegrationTestBase {
   public static final String AUTH_A = "AuthA";
 
@@ -278,4 +285,37 @@ void testGetUserByOpenId() {
     User foundUser = userStore.getUserByOpenId(openId1);
     assertEquals(userB.getUid(), foundUser.getUid());
   }
+
+  @Test
+  @DisplayName("Get users by org unit uid with expected select count")
+  void getUsersByOrgUnitUidExpectedSelectCountTest() {
+    // given 2 org units & 4 users
+    OrganisationUnit ou1 = createOrganisationUnit("org unit test 1");
+    OrganisationUnit ou2 = createOrganisationUnit("org unit test 2");
+    organisationUnitService.addOrganisationUnit(ou1);
+    organisationUnitService.addOrganisationUnit(ou2);
+
+    User user1 = createAndAddUser("user1 test", ou1);
+    User user2 = createAndAddUser("user2 test", ou1);
+    User user3 = createAndAddUser("user3 test", ou2);
+    User user4 = createAndAddUser("user4 test no orgs");
+    userService.addUser(user1);
+    userService.addUser(user2);
+    userService.addUser(user3);
+    userService.addUser(user4);
+
+    // when retrieving users by org unit uid
+    SQLStatementCountValidator.reset();
+    List<User> users = userStore.getUsersWithOrgUnit(UserOrgUnitProperty.ORG_UNITS, UID.of(ou1));
+    // getting each org unit to assert later that no other select queries triggered
+    users.forEach(
+        u ->
+            assertTrue(
+                u.getOrganisationUnits().stream()
+                    .allMatch(ou -> ou.getUid().equals(ou1.getUid()))));
+
+    // then only 1 select query is triggered
+    assertSelectCount(1);
+    assertEquals(2, users.size());
+  }
 }