Skip to content

Commit

Permalink
Merge pull request #648 from isaacphysics/improvement/sort-users-like…
Browse files Browse the repository at this point in the history 
…-excel

Sort users in groups more consistently with Excel
  • Loading branch information
@axlewin
axlewin authored Oct 21, 2024
2 parents 68594fc + ed7e0af commit bc90b39
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 19 deletions.
21 changes: 12 additions & 9 deletions src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/GroupManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ public List<RegisteredUserDTO> getUsersInGroup(final UserGroupDTO group) throws

List<RegisteredUserDTO> users = userManager.findUsers(groupMemberIds);
// Sort the users by name
return this.orderUsersByName(users);
this.orderUsersByName(users);
return users;
}

/**
Expand Down Expand Up @@ -214,15 +215,17 @@ public GroupMembershipStatus getGroupMembershipStatus(Long userId, Long groupId)
* @param users
* - list of users.
*/
private List<RegisteredUserDTO> orderUsersByName(final List<RegisteredUserDTO> users) {
// Replaces apostrophes with tildes so that string containing them are ordered in the same way as in
// Excel. i.e. we want that "O'Sully" > "Ogbobby"
private void orderUsersByName(final List<RegisteredUserDTO> users) {
// Remove apostrophes so that string containing them are ordered in the same way as in Excel.
// I.e. we want that "O'Aaa" < "Obbb" < "O'Ccc"
Comparator<String> excelStringOrder = Comparator.nullsLast((String a, String b) ->
String.CASE_INSENSITIVE_ORDER.compare(a.replaceAll("'", "~"), b.replaceAll("'", "~")));
return users.stream()
.sorted(Comparator.comparing(RegisteredUserDTO::getGivenName, excelStringOrder))
.sorted(Comparator.comparing(RegisteredUserDTO::getFamilyName, excelStringOrder))
.collect(Collectors.toList());
String.CASE_INSENSITIVE_ORDER.compare(a.replaceAll("'", ""), b.replaceAll("'", "")));

// If names differ only by an apostrophe (i.e. "O'A" and "Oa"), break ties using name including any apostrophes:
users.sort(Comparator
.comparing(RegisteredUserDTO::getFamilyName, excelStringOrder)
.thenComparing(RegisteredUserDTO::getGivenName, excelStringOrder)
.thenComparing(RegisteredUserDTO::getFamilyName));
}

/**
Expand Down
26 changes: 16 additions & 10 deletions src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/GroupManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,26 @@ public final void setUp() {
@Test
public void orderUsersByName_ordersBySurnamePrimarily() throws Exception {
List<RegisteredUserDTO> users = Stream.of(
new RegisteredUserDTO("A", "Ab", "aab@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("B", "Ar", "bar@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.FEMALE, somePastDate, "", null, false),
new RegisteredUserDTO("C", "Ax", "caz@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO(null, "Ax", "NONEax@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.FEMALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "Ba", "dba@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.FEMALE, somePastDate, "", null, false),
new RegisteredUserDTO("B", "Bb", "ebb@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("C", "Bf", "fbf@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.FEMALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", null, "aNONE@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.FEMALE, somePastDate, "", null, false)
new RegisteredUserDTO("A", "Ab", "a1@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("B", "Ar", "a2@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("C", "Ax", "a3@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO(null, "Ax", "a4@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "Ba", "b1@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("B", "Bb", "b2@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("C", "Bf", "b3@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "O'A", "o1@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "Obe", "o2@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "O'c", "o3@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", "Oc", "o4@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO("A", null, "-1@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false),
new RegisteredUserDTO(null, null, "--@test.com", EmailVerificationStatus.VERIFIED, somePastDate, Gender.MALE, somePastDate, "", null, false)
).peek(user -> user.setId((long) ("" + user.getGivenName() + user.getFamilyName()).hashCode())).collect(Collectors.toList());

List<RegisteredUserDTO> shuffledUsers = new ArrayList<>(users);
Collections.shuffle(shuffledUsers);

List<RegisteredUserDTO> sortedUsers = Whitebox.invokeMethod(groupManager, "orderUsersByName", shuffledUsers);
assertEquals(users, sortedUsers);
assertNotEquals(users, shuffledUsers);
Whitebox.invokeMethod(groupManager, "orderUsersByName", shuffledUsers);
assertEquals(users, shuffledUsers);
}
}

0 comments on commit bc90b39

Please sign in to comment.