SHIBUI-2394

Updates and additional unit testing for adding/editing/removing approvers to a group

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

@@ -9,6 +9,7 @@
 import javax.persistence.Column;
 import javax.persistence.Entity;
 import javax.persistence.EntityListeners;
+import javax.persistence.FetchType;
 import javax.persistence.Id;
 import javax.persistence.OneToMany;
 import javax.persistence.Transient;
@@ -54,7 +55,7 @@ public class Group implements Owner {
     @Column(name = "validation_regex")
     private String validationRegex;
 
-    @OneToMany
+    @OneToMany(fetch = FetchType.LAZY)
     private List<Approvers> approversList = new ArrayList<>();
 
     /**

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/repository/ApproversRepository.java

@@ -4,4 +4,5 @@
 import org.springframework.data.jpa.repository.JpaRepository;
 
 public interface ApproversRepository extends JpaRepository<Approvers, String> {
+    Approvers findByResourceId(String resourceId);
 }

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

@@ -4,7 +4,9 @@
 import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException;
 import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException;
 import edu.internet2.tier.shibboleth.admin.ui.security.exception.InvalidGroupRegexException;
+import edu.internet2.tier.shibboleth.admin.ui.security.model.Approvers;
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Group;
+import edu.internet2.tier.shibboleth.admin.ui.security.repository.ApproversRepository;
 import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository;
 import edu.internet2.tier.shibboleth.admin.ui.security.repository.OwnershipRepository;
 import lombok.NoArgsConstructor;
@@ -16,6 +18,7 @@
 import javax.script.ScriptEngine;
 import javax.script.ScriptEngineManager;
 import javax.script.ScriptException;
+import java.util.ArrayList;
 import java.util.List;
 
 @Service
@@ -25,28 +28,29 @@ public class GroupServiceImpl implements IGroupService {
     private static final String REGEX_MATCHER = "function validate(r, s){ return RegExp(r).test(s);};validate(rgx, str);";
     private final ScriptEngine engine = new ScriptEngineManager().getEngineByName("JavaScript");
 
+    @Autowired
+    protected ApproversRepository approversRepository;
+
     @Autowired
     protected GroupsRepository groupRepository;
 
     @Autowired
     protected OwnershipRepository ownershipRepository;
 
-    public GroupServiceImpl(GroupsRepository repo, OwnershipRepository ownershipRepository) {
-        this.groupRepository = repo;
-        this.ownershipRepository = ownershipRepository;
-    }
+//    public GroupServiceImpl(GroupsRepository repo, OwnershipRepository ownershipRepository) {
+//        this.groupRepository = repo;
+//        this.ownershipRepository = ownershipRepository;
+//    }
 
     @Override
     @Transactional
     public Group createGroup(Group group) throws GroupExistsConflictException, InvalidGroupRegexException {
-        Group foundGroup = find(group.getResourceId());
         // If already defined, we don't want to create a new one, nor do we want this call update the definition
-        if (foundGroup != null) {
-            throw new GroupExistsConflictException(
-                            String.format("Call update (PUT) to modify the group with resource id: [%s] and name: [%s]",
-                                            foundGroup.getResourceId(), foundGroup.getName()));
+        if (groupRepository.existsById(group.getResourceId())) {
+            throw new GroupExistsConflictException(String.format("Call update (PUT) to modify the group with resource id: [%s] and name: [%s]", group.getResourceId(), group.getName()));
         }
         validateGroupRegex(group);
+        manageApproversList(group);
         return groupRepository.save(group);
     }
 
@@ -59,6 +63,7 @@ public void deleteDefinition(String resourceId) throws PersistentEntityNotFound,
                             "Unable to delete group with resource id: [%s] - remove all items owned by / associated with the group first",
                             resourceId));
         }
+        approversRepository.deleteAll(group.getApproversList());
         groupRepository.delete(group);
     }
 
@@ -117,6 +122,7 @@ public List<Group> 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]",
@@ -126,6 +132,21 @@ public Group updateGroup(Group group) throws PersistentEntityNotFound, InvalidGr
         return groupRepository.save(group);
     }
 
+    private void manageApproversList(Group group) {
+        if (group.getApproversList().isEmpty()) {
+            return;
+        }
+        List<Approvers> updatedApprovers = new ArrayList<>();
+        group.getApproversList().forEach(approvers -> {
+            Approvers savedApprovers = approversRepository.findByResourceId(approvers.getResourceId());
+            savedApprovers = savedApprovers == null ? approvers : savedApprovers;
+            savedApprovers.setApproverGroups(approvers.getApproverGroups());
+            Approvers updatedApp = approversRepository.save(savedApprovers);
+            updatedApprovers.add(updatedApp);
+        });
+        group.setApproversList(updatedApprovers);
+    }
+
     /**
      * If the regex is blank simply return
      */

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

@@ -4,6 +4,7 @@ import edu.internet2.tier.shibboleth.admin.ui.AbstractBaseDataJpaTest
 import edu.internet2.tier.shibboleth.admin.ui.exception.PersistentEntityNotFound
 import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException
 import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException
+import edu.internet2.tier.shibboleth.admin.ui.security.model.Approvers
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Group
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Role
 import edu.internet2.tier.shibboleth.admin.ui.security.model.User
@@ -47,36 +48,65 @@ class GroupsControllerIntegrationTests extends AbstractBaseDataJpaTest {
     @WithMockAdmin
     def 'POST new group persists properly'() {
         given:
-        def newGroup = [name: 'Foo',
-                        description: 'Bar',
-                        resourceId: 'FooBar']
-
-        def expectedJson = """
-  {
-    "name":"Foo",
-    "description":"Bar",
-    "resourceId":"FooBar"
-  }
-"""
+        def newGroup = [name: 'Foo', description: 'Bar', resourceId: 'FooBar']
+
         when:
-        def result = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON)
-                            .content(JsonOutput.toJson(newGroup)).accept(MediaType.APPLICATION_JSON))
+        def result = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup)).accept(MediaType.APPLICATION_JSON))
 
         then:
         result.andExpect(status().isCreated())
-              .andExpect(content().contentType(MediaType.APPLICATION_JSON))
-              .andExpect(content().json(expectedJson, false))
-
+            .andExpect(content().contentType(MediaType.APPLICATION_JSON))
+            .andExpect(jsonPath("\$.name").value("Foo"))
+            .andExpect(jsonPath("\$.resourceId").value("FooBar"))
+            .andExpect(jsonPath("\$.description").value("Bar"))
 
-       //'Try to create with an existing resource id'
+        //'Try to create with an existing resource id'
         try {
             mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON)
                                               .content(JsonOutput.toJson(newGroup))
                                               .accept(MediaType.APPLICATION_JSON))
-            false
+            false // failure if the call didn't throw
         } catch (Throwable expected) {
             expected instanceof GroupExistsConflictException
         }
+
+        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({
+                it.name = name
+                it.description = name
+                it.resourceId = name
+                it
+            })
+            groupRepository.save(group)
+        }}
+        entityManager.flush()
+        entityManager.clear()
+
+        groupNames.each {name ->{
+            if (!name.equals('AAA')) {
+                apprGroups.add(groupRepository.findByResourceId(name))
+            }
+        }}
+        Approvers approvers = new Approvers()
+        approvers.setApproverGroups(apprGroups)
+        def apprList = new ArrayList<>()
+        apprList.add(approvers)
+        def newGroup2 = [name: 'Foo', description: 'Bar', resourceId: 'FooBar', approversList: apprList]
+        def result2 = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup2)).accept(MediaType.APPLICATION_JSON))
+
+        then:
+        result2.andExpect(status().isCreated())
+            .andExpect(content().contentType(MediaType.APPLICATION_JSON))
+            .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"))
     }
 
     @WithMockAdmin

backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/repository/GroupsRepositoryTests.groovy

@@ -176,11 +176,11 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest {
         def groupFromDb = gList.get(0) as Group
         groupFromDb == group
 
-        // update check
+        // update check - equality for groups should be by id
         groupFromDb.with {
             it.description = "some new text that wasn't there before"
         }
-        groupFromDb.equals(group) == false
+        groupFromDb.equals(group) == true
 
         when:
         groupsRepo.save(groupFromDb)
@@ -189,7 +189,7 @@ class GroupsRepositoryTests extends AbstractBaseDataJpaTest {
         def gList2 = groupsRepo.findAll()
         gList2.size() == 1
         def groupFromDb2 = gList2.get(0) as Group
-        groupFromDb2.equals(group) == false
+        groupFromDb2.equals(group) == true
         groupFromDb2 == groupFromDb
 
         // delete tests

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

@@ -84,7 +84,7 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
 
     def "CRUD operations - approver groups" () {
         given:
-        groupService.clearAllForTesting();
+        groupService.clearAllForTesting()
         List<Group> apprGroups = new ArrayList<>()
         String[] groupNames = ['AAA', 'BBB', 'CCC', 'DDD']
         groupNames.each {name -> {
@@ -94,38 +94,62 @@ class GroupServiceTests extends AbstractBaseDataJpaTest {
                 it.resourceId = name
                 it
             })
-            group = groupRepository.save(group)
+            groupRepository.save(group)
         }}
         entityManager.flush()
         entityManager.clear()
 
+        when: "Adding approval list to a group"
         groupNames.each {name ->{
             if (!name.equals('AAA')) {
                 apprGroups.add(groupRepository.findByResourceId(name))
             }
         }}
-
-        when: "Adding approval list to a group"
         Approvers approvers = new Approvers()
         approvers.setApproverGroups(apprGroups)
-        approvers = approversRepository.save(approvers)
-        entityManager.flush()
-        entityManager.clear()
-
-        List<Approvers> apprList = new ArrayList<>()
+        def apprList = new ArrayList<>()
         apprList.add(approvers)
         Group aaaGroup = groupService.find('AAA')
         aaaGroup.setApproversList(apprList)
         groupService.updateGroup(aaaGroup)
-        Group lookupGroup = groupService.find('AAA')
 
         then:
+        def lookupGroup = groupService.find('AAA')
         lookupGroup.getApproversList().size() == 1
-        List<Group> approvalGroups = lookupGroup.getApproversList().get(0).getApproverGroups()
+        def approvalGroups = lookupGroup.getApproversList().get(0).getApproverGroups()
         approvalGroups.size() == 3
         apprGroups.each {group -> {
             assert approvalGroups.contains(group)}
         }
+
+        when: "removing approver group from existing list"
+        approvers.getApproverGroups().remove(groupService.find('BBB'))
+        apprList = new ArrayList<>()
+        apprList.add(approvers)
+        aaaGroup.setApproversList(apprList)
+        groupService.updateGroup(aaaGroup)
+
+        then:
+        def lookupGroup2 = groupService.find('AAA')
+        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)
+                }
+            }
+        }
+
+        when: "removing all approver groups"
+        apprList = new ArrayList<>()
+        aaaGroup.setApproversList(apprList)
+        groupService.updateGroup(aaaGroup)
+
+        then:
+        def lookupGroup3 = groupService.find('AAA')
+        lookupGroup3.getApproversList().isEmpty() == true
     }
 
 }