SHIBUI-2394

corrections to services and tests

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Approvers.java

@@ -1,5 +1,6 @@
 package edu.internet2.tier.shibboleth.admin.ui.security.model;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import lombok.Data;
 import lombok.NoArgsConstructor;
 
@@ -8,6 +9,7 @@
 import javax.persistence.Id;
 import javax.persistence.ManyToMany;
 import javax.persistence.OneToMany;
+import javax.persistence.Transient;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -20,8 +22,24 @@
 public class Approvers {
     @Id
     @Column(name = "resource_id")
+    @JsonIgnore
     private String resourceId = UUID.randomUUID().toString();
 
     @ManyToMany
+    @JsonIgnore
     private List<Group> approverGroups = new ArrayList<>();
+
+    @Transient
+    private List<String> approverGroupIds = new ArrayList<>();
+
+    public List<String> getApproverGroupIds() {
+        if (approverGroupIds.isEmpty()) {
+            approverGroups.forEach(group -> approverGroupIds.add(group.getResourceId()));
+        }
+        return approverGroupIds;
+    }
+
+    public void setApproverGroups(List<Group> appGroups) {
+        this.approverGroups = appGroups;
+    }
 }

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java

@@ -108,4 +108,9 @@ public List<String> getApproveForList() {
         }
         return approveForList;
     }
+
+    @Override
+    public String toString() {
+        return "Group resourceId=" + resourceId;
+    }
 }

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java

@@ -115,16 +115,13 @@ public List<Group> findAll() {
         return groupRepository.findAll();
     }
 
-    @Override
-    public Group updateGroup(Group group) throws PersistentEntityNotFound, InvalidGroupRegexException {
-        manageApproversList(group); // have to make sure that approvers have been saved before a fetch or we can get data integrity errors on lookup...
-        Group g = find(group.getResourceId());
-        if (g == null) {
-            throw new PersistentEntityNotFound(String.format("Unable to find group with resource id: [%s] and name: [%s]",
-                            group.getResourceId(), group.getName()));
+    private List<Group> getGroupListFromIds(List<String> approverGroupIds) {
+        List<Group> result = new ArrayList<>();
+        for (String id : approverGroupIds) {
+            Group g = find(id);
+            result.add(g);
         }
-        validateGroupRegex(group);
-        return groupRepository.save(group);
+        return result;
     }
 
     private void manageApproversList(Group group) {
@@ -134,14 +131,26 @@ private void manageApproversList(Group group) {
         List<Approvers> updatedApprovers = new ArrayList<>();
         group.getApproversList().forEach(approvers -> {
             Approvers savedApprovers = approversRepository.findByResourceId(approvers.getResourceId());
-            savedApprovers = savedApprovers == null ? approvers : savedApprovers;
-            savedApprovers.setApproverGroups(approvers.getApproverGroups());
+            savedApprovers = savedApprovers == null ? approversRepository.save(approvers) : savedApprovers;
+            savedApprovers.setApproverGroups(getGroupListFromIds(approvers.getApproverGroupIds()));
             Approvers updatedApp = approversRepository.save(savedApprovers);
             updatedApprovers.add(updatedApp);
         });
         group.setApproversList(updatedApprovers);
     }
 
+    @Override
+    public Group updateGroup(Group group) throws PersistentEntityNotFound, InvalidGroupRegexException {
+        manageApproversList(group); // have to make sure that approvers have been saved before a fetch or we can get data integrity errors on lookup...
+        Group g = find(group.getResourceId());
+        if (g == null) {
+            throw new PersistentEntityNotFound(String.format("Unable to find group with resource id: [%s] and name: [%s]",
+                            group.getResourceId(), group.getName()));
+        }
+        validateGroupRegex(group);
+        return groupRepository.save(group);
+    }
+
     /**
      * If the regex is blank simply return
      */

backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy

@@ -72,7 +72,6 @@ class GroupsControllerIntegrationTests extends AbstractBaseDataJpaTest {
 
         when: "POST new group with approvers"
         groupService.clearAllForTesting()
-        List<Group> apprGroups = new ArrayList<>()
         String[] groupNames = ['AAA', 'BBB', 'CCC', 'DDD']
         groupNames.each {name -> {
             Group group = new Group().with({
@@ -86,13 +85,14 @@ class GroupsControllerIntegrationTests extends AbstractBaseDataJpaTest {
         entityManager.flush()
         entityManager.clear()
 
+        List<String> apprGroups = new ArrayList<>()
         groupNames.each {name ->{
             if (!name.equals('AAA')) {
-                apprGroups.add(groupRepository.findByResourceId(name))
+                apprGroups.add(name)
             }
         }}
         Approvers approvers = new Approvers()
-        approvers.setApproverGroups(apprGroups)
+        approvers.setApproverGroupIds(apprGroups)
         def apprList = new ArrayList<>()
         apprList.add(approvers)
         def newGroup2 = [name: 'Foo', description: 'Bar', resourceId: 'FooBar', approversList: apprList]
@@ -104,9 +104,9 @@ class GroupsControllerIntegrationTests extends AbstractBaseDataJpaTest {
             .andExpect(jsonPath("\$.name").value("Foo"))
             .andExpect(jsonPath("\$.resourceId").value("FooBar"))
             .andExpect(jsonPath("\$.description").value("Bar"))
-            .andExpect(jsonPath("\$.approversList[0].approverGroups[0].resourceId").value("BBB"))
-            .andExpect(jsonPath("\$.approversList[0].approverGroups[1].resourceId").value("CCC"))
-            .andExpect(jsonPath("\$.approversList[0].approverGroups[2].resourceId").value("DDD"))
+            .andExpect(jsonPath("\$.approversList[0].approverGroupIds[0]").value("BBB"))
+            .andExpect(jsonPath("\$.approversList[0].approverGroupIds[1]").value("CCC"))
+            .andExpect(jsonPath("\$.approversList[0].approverGroupIds[2]").value("DDD"))
     }
 
     @WithMockAdmin

backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceForTesting.groovy

@@ -14,6 +14,7 @@ class GroupServiceForTesting extends GroupServiceImpl {
 
     @Transactional
     void clearAllForTesting() {
+        approversRepository.deleteAll()
         groupRepository.deleteAll()
         ownershipRepository.clearAllOwnedByGroup()
         ensureAdminGroupExists()

backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy

@@ -85,7 +85,7 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
     def "CRUD operations - approver groups" () {
         given:
         groupService.clearAllForTesting()
-        List<Group> apprGroups = new ArrayList<>()
+        List<String> apprGroups = new ArrayList<>()
         String[] groupNames = ['AAA', 'BBB', 'CCC', 'DDD']
         groupNames.each {name -> {
             Group group = new Group().with({
@@ -102,12 +102,12 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
         when: "Adding approval list to a group"
         groupNames.each {name ->{
             if (!name.equals('AAA')) {
-                apprGroups.add(groupRepository.findByResourceId(name))
+                apprGroups.add(name)
             }
         }}
         Approvers approvers = new Approvers()
-        approvers.setApproverGroups(apprGroups)
-        def apprList = new ArrayList<>()
+        approvers.setApproverGroupIds(apprGroups)
+        List<Approvers> apprList = new ArrayList<>()
         apprList.add(approvers)
         Group aaaGroup = groupService.find('AAA')
         aaaGroup.setApproversList(apprList)
@@ -118,12 +118,12 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
         lookupGroup.getApproversList().size() == 1
         def approvalGroups = lookupGroup.getApproversList().get(0).getApproverGroups()
         approvalGroups.size() == 3
-        apprGroups.each {group -> {
-            assert approvalGroups.contains(group)}
+        approvalGroups.each {group -> {
+            assert apprGroups.contains(group.getResourceId())}
         }
 
         when: "removing approver group from existing list"
-        approvers.getApproverGroups().remove(groupService.find('BBB'))
+        approvers.setApproverGroupIds(Arrays.asList("CCC", "DDD"))
         apprList = new ArrayList<>()
         apprList.add(approvers)
         aaaGroup.setApproversList(apprList)
@@ -134,13 +134,7 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
         lookupGroup2.getApproversList().size() == 1
         def approvalGroups2 = lookupGroup2.getApproversList().get(0).getApproverGroups()
         approvalGroups2.size() == 2
-        apprGroups.each { group ->
-            {
-                if (group.getResourceId() != 'BBB') {
-                    assert approvalGroups2.contains(group)
-                }
-            }
-        }
+        approvalGroups2.forEach(group -> group.getResourceId().equals("CCC") || group.getResourceId().equals("DDD"))
 
         when: "removing all approver groups"
         apprList = new ArrayList<>()