SHIBUI-1740

Correcting logic on setting groupid for users
TIER · Jul 23, 2021 · e7d89c3 · e7d89c3
1 parent 3f350e0
commit e7d89c3
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 18 deletions.
diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java
@@ -117,28 +117,28 @@ public Set<UserGroup> getUserGroups() {
      * we didn't intend (thanks JPA!!). 
      */
     public void setGroup(Group assignedGroup) {
-        // if the incoming group is the current group, make no changes to our sets
-        if (assignedGroup.getResourceId().equals(groupId)) {
-            userGroups.forEach(ug -> {
-                if (ug.getGroup().getResourceId().equals(groupId)) {
-                    ug.setGroup(assignedGroup);
-                }
-            });
-            return;
-        }
-
-        // stash the current groups for removal
-        getUserGroups().forEach(g -> {
-            // If the assignedGroup is in the current list, don't bother putting it in the "delete" list
-            if (!g.getGroup().equals(assignedGroup)) {
-                oldUserGroups.add(g);
+        UserGroup theUserGroup = new UserGroup();
+        // Go through the existing UserGroups:
+        // 1) If a UG doesn't match the incoming assignment, move it out of the list and into the old for deletion
+        // 2) If it DOES match, update the group object so hibernate doesn't have a cow
+        userGroups.forEach(ug -> {
+            if (ug.getGroup().getResourceId().equals(groupId)) {
+                theUserGroup.setGroup(assignedGroup);
+                theUserGroup.setUser(this);
+                theUserGroup.setId(ug.getId());
+            } else {
+                oldUserGroups.add(ug);
             }
         });
         userGroups.clear();
 
         // Assign the new group
-        UserGroup ug = new UserGroup(assignedGroup, this);
-        userGroups.add(ug);
+        if (theUserGroup.getUser() == null) {
+            UserGroup ug = new UserGroup(assignedGroup, this);
+            userGroups.add(ug);
+        } else {
+            userGroups.add(theUserGroup);
+        }
 
         // Set reference for the UI
         groupId = assignedGroup.getResourceId();

diff --git a/...rnet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy b/...rnet2/tier/shibboleth/admin/ui/security/controller/UsersControllerIntegrationTests.groovy
@@ -20,6 +20,8 @@ import org.springframework.test.web.servlet.result.MockMvcResultHandlers
 import spock.lang.Ignore
 import spock.lang.Specification
 import edu.internet2.tier.shibboleth.admin.ui.security.model.User
+import edu.internet2.tier.shibboleth.admin.ui.security.model.UserGroup
+import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserGroupRepository
 import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService
 
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete
@@ -41,10 +43,12 @@ import java.time.format.DateTimeFormatter
 @ActiveProfiles(["no-auth", "dev"])
 @DirtiesContext
 class UsersControllerIntegrationTests extends Specification {
-
     @Autowired
     IGroupService groupService
 
+    @Autowired 
+    UserGroupRepository ugRepo
+
     @Autowired
     private MockMvc mockMvc
 
@@ -242,6 +246,9 @@ class UsersControllerIntegrationTests extends Specification {
         resultNewGroup.andExpect(status().isOk())
                       .andExpect(content().contentType(MediaType.APPLICATION_JSON))
                       .andExpect(jsonPath("\$.groupId").value("AAA"))
+
+        def List<UserGroup> groups = ugRepo.findAllByUser(user)
+        groups.size() == 1
     }
 
     @WithMockUser(value = "admin", roles = ["ADMIN"])