Skip to content

Commit

Permalink
fix: Org unit split assigned to correct users [DHIS2-18423](2.39) (#1…
Browse files Browse the repository at this point in the history
…9257)

* fix: Org unit split assigned to correct users [DHIS2-18423](2.39)

* fix: clean up

* fix: fix test setup
  • Loading branch information
david-mackessy authored Nov 22, 2024
1 parent 226fa11 commit 77ae773
Show file tree
Hide file tree
Showing 9 changed files with 209 additions and 11 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright (c) 2004-2022, 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;

/**
* @author Lars Helge Overland
*/
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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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<>();
Expand Down
12 changes: 12 additions & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserService.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@
import java.util.Optional;
import java.util.UUID;
import java.util.function.Consumer;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.hisp.dhis.dataset.DataSet;
import org.hisp.dhis.feedback.ErrorReport;
import org.hisp.dhis.organisationunit.OrganisationUnit;

/**
* @author Chau Thu Tran
Expand Down Expand Up @@ -461,4 +463,14 @@ boolean canCurrentUserCanModify(
User currentUser, User userToModify, Consumer<ErrorReport> errors);

void generateTwoFactorSecret(User user);

/**
* 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} uid to match on
* @return matching {@link User}s
*/
List<User> getUsersWithOrgUnit(@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull String uid);
}
12 changes: 12 additions & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@
import java.util.Map;
import java.util.Optional;
import java.util.UUID;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.hisp.dhis.common.IdentifiableObjectStore;
import org.hisp.dhis.organisationunit.OrganisationUnit;

/**
* @author Nguyen Hong Duc
Expand Down Expand Up @@ -181,4 +183,14 @@ Map<String, Optional<Locale>> findNotifiableUsersWithPasswordLastUpdatedBetween(
User getUserByUuid(UUID uuid);

List<User> getHasAuthority(String authority);

/**
* 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} uid to match on
* @return matching {@link User}s
*/
List<User> getUsersWithOrgUnit(@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull String uid);
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,13 @@

import com.google.common.collect.Sets;
import java.util.List;
import java.util.Set;
import lombok.AllArgsConstructor;
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;

Expand Down Expand Up @@ -92,11 +91,10 @@ public void splitOrganisationUnits(OrgUnitSplitRequest request) {
}

public void splitUsers(OrgUnitSplitRequest request) {
Set<OrganisationUnit> source = Sets.newHashSet(request.getSource());
String sourceOrgUnitUid = request.getSource().getUid();

List<User> dataCaptureUsers =
userService.getUsers(
new UserQueryParams().setCanSeeOwnRoles(true).setOrganisationUnits(source));
userService.getUsersWithOrgUnit(UserOrgUnitProperty.ORG_UNITS, sourceOrgUnitUid);

dataCaptureUsers.forEach(
u -> {
Expand All @@ -105,8 +103,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 -> {
Expand All @@ -115,8 +112,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 -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
Expand Down Expand Up @@ -900,4 +901,10 @@ public void generateTwoFactorSecret(User user) {
user.setSecret(Base32.random());
updateUser(user);
}

@Override
public List<User> getUsersWithOrgUnit(
@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull String uid) {
return userStore.getUsersWithOrgUnit(orgUnitProperty, uid);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
import org.hisp.dhis.user.User;
import org.hisp.dhis.user.UserAccountExpiryInfo;
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;
Expand Down Expand Up @@ -578,4 +579,15 @@ public List<User> getUserByUsernames(Collection<String> usernames) {

return query.getResultList();
}

@Override
public List<User> getUsersWithOrgUnit(
@Nonnull UserOrgUnitProperty orgUnitProperty, @Nonnull String uid) {
return getQuery(
String.format(
"select distinct u from User u left join fetch u.%s ous where ous.uid = :uid ",
orgUnitProperty.getValue()))
.setParameter("uid", uid)
.getResultList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -44,6 +48,9 @@
import org.hisp.dhis.period.PeriodType;
import org.hisp.dhis.program.Program;
import org.hisp.dhis.test.integration.TransactionalIntegrationTest;
import org.hisp.dhis.user.User;
import org.hisp.dhis.user.UserService;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;

Expand All @@ -53,9 +60,8 @@
class OrgUnitSplitServiceTest extends TransactionalIntegrationTest {

@Autowired private OrgUnitSplitService service;

@Autowired private UserService _userService;
@Autowired private IdentifiableObjectManager idObjectManager;

@Autowired private PeriodService periodService;

private PeriodType ptA;
Expand All @@ -68,6 +74,7 @@ class OrgUnitSplitServiceTest extends TransactionalIntegrationTest {

@Override
public void setUpTest() {
userService = _userService;
ptA = periodService.getPeriodTypeByClass(MonthlyPeriodType.class);
ouA = createOrganisationUnit('A');
ouB = createOrganisationUnit('B');
Expand Down Expand Up @@ -156,4 +163,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(),
String.format("user should have %s org units", orgUnits));
assertEquals(
dataViewOrgUnits,
user.getDataViewOrganisationUnits().size(),
String.format("user should have %s data view org units", dataViewOrgUnits));
assertEquals(
teiSearchOrgUnits,
user.getTeiSearchOrganisationUnits().size(),
String.format("user should have %s tei search org units", teiSearchOrgUnits));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.hisp.dhis.organisationunit.OrganisationUnit;
import org.hisp.dhis.organisationunit.OrganisationUnitService;
import org.hisp.dhis.test.integration.SingleSetupIntegrationTestBase;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;

Expand Down Expand Up @@ -218,4 +219,35 @@ void testGetUserWithAuthority() {
List<User> usersWithAuthorityB = userService.getUsersWithAuthority(AUTH_D);
assertFalse(usersWithAuthorityB.contains(userB));
}

@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
List<User> users = userStore.getUsersWithOrgUnit(UserOrgUnitProperty.ORG_UNITS, ou1.getUid());
// 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
assertEquals(2, users.size());
}
}

0 comments on commit 77ae773

Please sign in to comment.