From 324e46eda33b6381cc6f932e431ac4e6b9e1e114 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Mon, 10 Oct 2022 14:57:34 -0700 Subject: [PATCH] SHIBUI-2394 corrections to services and tests --- .../admin/ui/security/model/Approvers.java | 18 +++++++++++ .../admin/ui/security/model/Group.java | 5 +++ .../ui/security/service/GroupServiceImpl.java | 31 ++++++++++++------- .../GroupsControllerIntegrationTests.groovy | 12 +++---- .../service/GroupServiceForTesting.groovy | 1 + .../security/service/GroupServiceTests.groovy | 22 +++++-------- 6 files changed, 58 insertions(+), 31 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Approvers.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Approvers.java index dd0c4bec4..2a0739646 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Approvers.java +++ b/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 approverGroups = new ArrayList<>(); + + @Transient + private List approverGroupIds = new ArrayList<>(); + + public List getApproverGroupIds() { + if (approverGroupIds.isEmpty()) { + approverGroups.forEach(group -> approverGroupIds.add(group.getResourceId())); + } + return approverGroupIds; + } + + public void setApproverGroups(List appGroups) { + this.approverGroups = appGroups; + } } \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java index cc2570320..2591e36b5 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Group.java @@ -108,4 +108,9 @@ public List getApproveForList() { } return approveForList; } + + @Override + public String toString() { + return "Group resourceId=" + resourceId; + } } \ No newline at end of file diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java index a9607534f..917b84fd7 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java @@ -115,16 +115,13 @@ public List 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 getGroupListFromIds(List approverGroupIds) { + List 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 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 */ diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy index 6fae2ed87..a4c46e1b1 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy +++ b/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 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 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 diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceForTesting.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceForTesting.groovy index a3f223efb..6d18c2cd8 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceForTesting.groovy +++ b/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() diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy index 16045bca7..6da1ff232 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceTests.groovy +++ b/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 apprGroups = new ArrayList<>() + List 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 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<>()