From 9678716f9d295e3334770d440bd6cb7133efe40b Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 14:26:17 +0000 Subject: [PATCH 01/10] failing test exposing bug in duplicate name check --- .../member/services/MemberServiceTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java index 91be02f26..0f354a497 100644 --- a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java +++ b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java @@ -180,6 +180,29 @@ public Member answer(InvocationOnMock invocation) throws Throwable { Mockito.verify(userService, Mockito.never()).updateUsersSalesforceId(Mockito.anyString(), Mockito.anyString()); } + @Test + void testUpdateMemberWithDuplicateName() { + Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation()); + Mockito.when(memberRepository.findById(Mockito.anyString())).thenReturn(Optional.of(getMember())); + + // return a record when name is checked against db + Mockito.when(memberRepository.findByClientName(Mockito.anyString())).thenReturn(Optional.of(getMember())); + + Member member = getMember(); + member.setId("id"); + member.setSalesforceId("three"); + member.setClientName("new client name"); + + Exception e = Assertions.assertThrows(BadRequestAlertException.class, () -> { + memberService.updateMember(member); + }); + + assertThat(e.getMessage()).isEqualTo("Invalid member name"); + + Mockito.verify(memberRepository, Mockito.never()).save(Mockito.any(Member.class)); + Mockito.verify(userService, Mockito.never()).updateUsersSalesforceId(Mockito.anyString(), Mockito.anyString()); + } + @Test void testUpdateMemberWithSalesforceIdUpdateFailure_userFailure() { Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation()); From 752eafbcba4cae699e0d371266bc053b40eb8140 Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 14:26:38 +0000 Subject: [PATCH 02/10] fixed duplicate name check bug --- .../service/member/services/MemberService.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java index f61cb203c..06449ea3c 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java @@ -133,13 +133,6 @@ public Member updateMember(Member member) { } Member existingMember = optional.get(); - existingMember.setClientId(member.getClientId()); - existingMember.setClientName(member.getClientName()); - existingMember.setParentSalesforceId(member.getParentSalesforceId()); - existingMember.setLastModifiedBy(SecurityUtils.getCurrentUserLogin().get()); - existingMember.setLastModifiedDate(Instant.now()); - existingMember.setAssertionServiceEnabled(member.getAssertionServiceEnabled()); - existingMember.setIsConsortiumLead(member.getIsConsortiumLead()); // Check if name changed if (!existingMember.getClientName().equals(member.getClientName())) { @@ -149,6 +142,16 @@ public Member updateMember(Member member) { } } + existingMember.setClientId(member.getClientId()); + existingMember.setClientName(member.getClientName()); + existingMember.setParentSalesforceId(member.getParentSalesforceId()); + existingMember.setLastModifiedBy(SecurityUtils.getCurrentUserLogin().get()); + existingMember.setLastModifiedDate(Instant.now()); + existingMember.setAssertionServiceEnabled(member.getAssertionServiceEnabled()); + existingMember.setIsConsortiumLead(member.getIsConsortiumLead()); + + + // Check if salesforceId changed if (!existingMember.getSalesforceId().equals(member.getSalesforceId())) { Optional optionalSalesforceId = memberRepository.findBySalesforceId(member.getSalesforceId()); From a2d8bef4f0495bfd5d185c635579bef257b68694 Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 14:59:29 +0000 Subject: [PATCH 03/10] refactor --- .../member/services/MemberService.java | 113 ++++++++++-------- 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java index 06449ea3c..16dbe02bf 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java @@ -122,26 +122,10 @@ public Member createMember(Member member) { } public Member updateMember(Member member) { - MemberValidation validation = memberValidator.validate(member, userService.getLoggedInUser()); - if (!validation.isValid()) { - throw new BadRequestAlertException("Member invalid", "member", "validation.string"); - } - Optional optional = memberRepository.findById(member.getId()); - if (!optional.isPresent()) { - throw new BadRequestAlertException("Invalid id", "member", "idunavailable.string"); - } + validateMemberUpdate(member, optional); Member existingMember = optional.get(); - - // Check if name changed - if (!existingMember.getClientName().equals(member.getClientName())) { - Optional optionalMember = memberRepository.findByClientName(member.getClientName()); - if (optionalMember.isPresent()) { - throw new BadRequestAlertException("Invalid member name", "member", "memberNameUsed.string"); - } - } - existingMember.setClientId(member.getClientId()); existingMember.setClientName(member.getClientName()); existingMember.setParentSalesforceId(member.getParentSalesforceId()); @@ -150,54 +134,77 @@ public Member updateMember(Member member) { existingMember.setAssertionServiceEnabled(member.getAssertionServiceEnabled()); existingMember.setIsConsortiumLead(member.getIsConsortiumLead()); + return propagateUpdatesAndSave(member, existingMember); + } + private Member propagateUpdatesAndSave(Member member, Member existingMember) { + String oldSalesforceId = existingMember.getSalesforceId(); + String newSalesforceId = member.getSalesforceId(); // Check if salesforceId changed - if (!existingMember.getSalesforceId().equals(member.getSalesforceId())) { + if (!oldSalesforceId.equals(newSalesforceId)) { Optional optionalSalesforceId = memberRepository.findBySalesforceId(member.getSalesforceId()); if (optionalSalesforceId.isPresent()) { throw new BadRequestAlertException("Invalid salesForceId", "member", "salesForceIdUsed.string"); } - String oldSalesforceId = existingMember.getSalesforceId(); - String newSalesforceId = member.getSalesforceId(); + updateAssertionSalesforceIds(oldSalesforceId, newSalesforceId); + updateUserSalesforceIds(oldSalesforceId, newSalesforceId); + return updateMemberSalesforceId(existingMember, newSalesforceId, oldSalesforceId); + } + return memberRepository.save(existingMember); + } - try { - // update affiliations and users associated with the member - assertionService.updateAssertionsSalesforceId(oldSalesforceId, newSalesforceId); - } catch (Exception e) { - LOG.error("Error updating assertion salesforce ids", e); - throw new RuntimeException(e); - } + private void updateAssertionSalesforceIds(String oldSalesforceId, String newSalesforceId) { + try { + // update affiliations and users associated with the member + assertionService.updateAssertionsSalesforceId(oldSalesforceId, newSalesforceId); + } catch (Exception e) { + LOG.error("Error updating assertion salesforce ids", e); + throw new RuntimeException(e); + } - try { - userService.updateUsersSalesforceId(oldSalesforceId, newSalesforceId); - } catch (Exception e) { - LOG.error("Error updating users's salesforce id", e); - LOG.error("Error updating users' salesforce id from {} to {}", oldSalesforceId, newSalesforceId); - LOG.info("Attempting to perform salesforce id rollback on affiliations"); - assertionService.updateAssertionsSalesforceId(newSalesforceId, oldSalesforceId); - LOG.info("Affiliation salesforce id rollback successfull"); - throw new RuntimeException(e); - } - existingMember.setSalesforceId(member.getSalesforceId()); - - try { - return memberRepository.save(existingMember); - } catch (Exception e) { - LOG.error("Error updating member", e); - LOG.error("Error updating member's salesforce id from {} to {}", oldSalesforceId, newSalesforceId); - LOG.info("Attempting to perform salesforce id rollback on affiliations"); - assertionService.updateAssertionsSalesforceId(newSalesforceId, oldSalesforceId); - LOG.info("Affiliation salesforce id rollback successfull"); - - LOG.info("Attempting to perform salesforce id rollback on users"); - userService.updateUsersSalesforceId(newSalesforceId, oldSalesforceId); - LOG.info("User salesforce id rollback successfull"); - throw new RuntimeException(e); + } + + private void updateUserSalesforceIds(String oldSalesforceId, String newSalesforceId) { + try { + userService.updateUsersSalesforceId(oldSalesforceId, newSalesforceId); + } catch (Exception e) { + LOG.error("Error updating users' salesforce id from {} to {}. Rolling back assertion changes", oldSalesforceId, newSalesforceId); + assertionService.updateAssertionsSalesforceId(newSalesforceId, oldSalesforceId); + throw new RuntimeException(e); + } + } + + private Member updateMemberSalesforceId(Member existingMember, String newSalesforceId, String oldSalesforceId) { + existingMember.setSalesforceId(newSalesforceId); + try { + return memberRepository.save(existingMember); + } catch (Exception e) { + LOG.error("Error updating member's salesforce id from {} to {}. Rolling back user and assertion changes", oldSalesforceId, newSalesforceId); + assertionService.updateAssertionsSalesforceId(newSalesforceId, oldSalesforceId); + userService.updateUsersSalesforceId(newSalesforceId, oldSalesforceId); + throw new RuntimeException(e); + } + } + + private void validateMemberUpdate(Member member, Optional existingMember) { + MemberValidation validation = memberValidator.validate(member, userService.getLoggedInUser()); + if (!validation.isValid()) { + throw new BadRequestAlertException("Member invalid", "member", "validation.string"); + } + + if (existingMember.isEmpty()) { + throw new BadRequestAlertException("Invalid id", "member", "idunavailable.string"); + } + + // Check if name changed + if (!existingMember.get().getClientName().equals(member.getClientName())) { + Optional optionalMember = memberRepository.findByClientName(member.getClientName()); + if (optionalMember.isPresent()) { + throw new BadRequestAlertException("Invalid member name", "member", "memberNameUsed.string"); } } - return memberRepository.save(existingMember); } public MemberValidation validateMember(Member member) { From 7928e0e002d8dd66b8d47a76f6740d5122a3dcbf Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 15:53:55 +0000 Subject: [PATCH 04/10] sf ids of tokens now get updated with assertion sf id updates --- .../repository/OrcidRecordRepository.java | 3 +- .../OrcidRecordRepositoryCustom.java | 14 +++++++ .../impl/OrcidRecordRepositoryCustomImpl.java | 40 +++++++++++++++++++ .../assertion/services/AssertionService.java | 4 +- .../services/OrcidRecordService.java | 11 +++-- .../assertion/web/rest/AssertionResource.java | 3 +- .../services/AssertionServiceTest.java | 3 ++ 7 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepositoryCustom.java create mode 100644 assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepository.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepository.java index 25a7c3df1..5c4b73123 100644 --- a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepository.java +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepository.java @@ -11,7 +11,7 @@ import org.springframework.stereotype.Repository; @Repository -public interface OrcidRecordRepository extends MongoRepository { +public interface OrcidRecordRepository extends MongoRepository, OrcidRecordRepositoryCustom { Optional findOneByEmail(String email); @@ -20,4 +20,5 @@ public interface OrcidRecordRepository extends MongoRepository findBySalesforceId(String salesforceId, Pageable pageable); + } diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepositoryCustom.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepositoryCustom.java new file mode 100644 index 000000000..b7a8b6c56 --- /dev/null +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/OrcidRecordRepositoryCustom.java @@ -0,0 +1,14 @@ +package org.orcid.memberportal.service.assertion.repository; + +import org.orcid.memberportal.service.assertion.domain.Assertion; +import org.orcid.memberportal.service.assertion.domain.MemberAssertionStatusCount; +import org.springframework.data.domain.Pageable; + +import java.util.Iterator; +import java.util.List; + +public interface OrcidRecordRepositoryCustom { + + void updateTokenSalesforceIds(String oldSalesforceId, String newSalesforceId); + +} diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java new file mode 100644 index 000000000..29c3cd99f --- /dev/null +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java @@ -0,0 +1,40 @@ +package org.orcid.memberportal.service.assertion.repository.impl; + +import com.mongodb.MongoClient; +import com.mongodb.client.DistinctIterable; +import com.mongodb.client.model.Filters; +import org.orcid.memberportal.service.assertion.domain.Assertion; +import org.orcid.memberportal.service.assertion.domain.MemberAssertionStatusCount; +import org.orcid.memberportal.service.assertion.domain.OrcidRecord; +import org.orcid.memberportal.service.assertion.domain.enumeration.AssertionStatus; +import org.orcid.memberportal.service.assertion.repository.AssertionRepositoryCustom; +import org.orcid.memberportal.service.assertion.repository.OrcidRecordRepositoryCustom; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.domain.Pageable; +import org.springframework.data.mongodb.core.MongoTemplate; +import org.springframework.data.mongodb.core.aggregation.*; +import org.springframework.data.mongodb.core.query.Criteria; +import org.springframework.data.mongodb.core.query.Query; +import org.springframework.data.mongodb.core.query.Update; +import org.springframework.stereotype.Repository; + +import java.util.Iterator; +import java.util.List; + +@Repository +public class OrcidRecordRepositoryCustomImpl implements OrcidRecordRepositoryCustom { + + @Autowired + private MongoTemplate mongoTemplate; + + public OrcidRecordRepositoryCustomImpl(MongoTemplate mongoTemplate) { + this.mongoTemplate = mongoTemplate; + } + + + @Override + public void updateTokenSalesforceIds(String oldSalesforceId, String newSalesforceId) { + mongoTemplate.updateMulti(Query.query(Criteria.where("tokens.salesforce_id").is(oldSalesforceId)), + new Update().set("tokens.$.salesforce_id", oldSalesforceId), OrcidRecord.class); + } +} diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/AssertionService.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/AssertionService.java index c13ecf713..0e8d05e00 100644 --- a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/AssertionService.java +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/AssertionService.java @@ -235,7 +235,9 @@ private String getAssertionStatus(Assertion assertion) { } public boolean updateAssertionsSalesforceId(String from, String to) { - return updateAssertionsSalesforceId(from, to, true); + boolean updated = updateAssertionsSalesforceId(from, to, true); + updated = updated && orcidRecordService.updateTokenSalesforceIds(from, to); + return updated; } private boolean updateAssertionsSalesforceId(String from, String to, boolean rollback) { diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/OrcidRecordService.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/OrcidRecordService.java index e98fa9bda..32150f396 100644 --- a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/OrcidRecordService.java +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/services/OrcidRecordService.java @@ -17,6 +17,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.domain.Page; import org.springframework.stereotype.Service; @Service @@ -117,7 +118,7 @@ public String generateLinkForEmail(String email) { String salesforceId = assertionsUserService.getLoggedInUserSalesforceId(); return generateLinkForEmailAndSalesforceId(email, salesforceId); } - + public String generateLinkForEmailAndSalesforceId(String email, String salesforceId) { String landingPageUrl = applicationProperties.getLandingPageUrl(); Optional record = orcidRecordRepository.findOneByEmail(email); @@ -178,14 +179,18 @@ public void deleteOrcidRecordTokenByEmailAndSalesforceId(String email, String sa } } } - + public boolean userHasGrantedOrDeniedPermission(String email, String salesforceId) { Optional orcidRecordOptional = findOneByEmail(email); if (orcidRecordOptional.isEmpty()) { return false; } - + return !StringUtils.isBlank(orcidRecordOptional.get().getToken(salesforceId, true)); } + public boolean updateTokenSalesforceIds(String salesforceId, String newSalesforceId) { + orcidRecordRepository.updateTokenSalesforceIds(salesforceId, newSalesforceId); + return true; + } } diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResource.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResource.java index 3a28cae7b..d2fd0ce44 100644 --- a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResource.java +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResource.java @@ -105,7 +105,7 @@ public class AssertionResource { @Autowired private NotificationService notificationService; - + @Autowired private MemberService memberService; @@ -452,6 +452,7 @@ private String encodeUrl(String urlString) throws MalformedURLException, URISynt public ResponseEntity updateSalesforceId(@PathVariable String salesforceId, @PathVariable String newSalesforceId) { LOG.debug("REST request to update Assertions by salesforce : {}", salesforceId); boolean success = assertionService.updateAssertionsSalesforceId(salesforceId, newSalesforceId); + success = success && orcidRecordService.updateTokenSalesforceIds(salesforceId, newSalesforceId); if (success) { return ResponseEntity.ok().headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, "assertion", salesforceId)).build(); } else { diff --git a/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/services/AssertionServiceTest.java b/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/services/AssertionServiceTest.java index 1d336b123..769c2217a 100644 --- a/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/services/AssertionServiceTest.java +++ b/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/services/AssertionServiceTest.java @@ -1835,10 +1835,13 @@ void testUpdateAssertionsSalesforceId() { ; Mockito.when(assertionRepository.findBySalesforceId(Mockito.eq("salesforce-id"), Mockito.any(Pageable.class))).thenReturn(new PageImpl(firstPage)) .thenReturn(new PageImpl(secondPage)).thenReturn(new PageImpl(thirdPage)).thenReturn(new PageImpl(new ArrayList<>())); + Mockito.when(orcidRecordService.updateTokenSalesforceIds(Mockito.anyString(), Mockito.anyString())).thenReturn(true); + boolean success = assertionService.updateAssertionsSalesforceId("salesforce-id", "new-salesforce-id"); assertThat(success).isTrue(); Mockito.verify(assertionRepository, Mockito.times((AssertionService.REGISTRY_SYNC_BATCH_SIZE * 3) - 10)).save(assertionCaptor.capture()); + Mockito.verify(orcidRecordService).updateTokenSalesforceIds(Mockito.anyString(), Mockito.anyString()); List saved = assertionCaptor.getAllValues(); saved.forEach(a -> assertThat(a.getSalesforceId()).isEqualTo("new-salesforce-id")); } From 3a069122ac00cf101a9eb8a39feee73b6ad8278f Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 15:57:56 +0000 Subject: [PATCH 05/10] refactor --- .../service/member/services/MemberService.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java index 16dbe02bf..311a01352 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java @@ -143,11 +143,6 @@ private Member propagateUpdatesAndSave(Member member, Member existingMember) { // Check if salesforceId changed if (!oldSalesforceId.equals(newSalesforceId)) { - Optional optionalSalesforceId = memberRepository.findBySalesforceId(member.getSalesforceId()); - if (optionalSalesforceId.isPresent()) { - throw new BadRequestAlertException("Invalid salesForceId", "member", "salesForceIdUsed.string"); - } - updateAssertionSalesforceIds(oldSalesforceId, newSalesforceId); updateUserSalesforceIds(oldSalesforceId, newSalesforceId); return updateMemberSalesforceId(existingMember, newSalesforceId, oldSalesforceId); @@ -205,6 +200,13 @@ private void validateMemberUpdate(Member member, Optional existingMember throw new BadRequestAlertException("Invalid member name", "member", "memberNameUsed.string"); } } + + if (!existingMember.get().getSalesforceId().equals(member.getSalesforceId())) { + Optional optionalSalesforceId = memberRepository.findBySalesforceId(member.getSalesforceId()); + if (optionalSalesforceId.isPresent()) { + throw new BadRequestAlertException("Invalid salesForceId", "member", "salesForceIdUsed.string"); + } + } } public MemberValidation validateMember(Member member) { From b95416a686d5d28c5dbbdfbc0724fa12b9587b99 Mon Sep 17 00:00:00 2001 From: George Nash Date: Mon, 3 Mar 2025 16:58:38 +0000 Subject: [PATCH 06/10] added update for user member name field when member name is updated in member service --- .../web/rest/AssertionResourceTest.java | 33 +++++++++-------- .../member/client/UserServiceClient.java | 3 ++ .../member/services/MemberService.java | 37 ++++++++----------- .../service/member/services/UserService.java | 7 ++++ .../member/services/MemberServiceTest.java | 28 -------------- .../user/repository/CustomUserRepository.java | 20 ++++++++++ .../user/repository/UserRepository.java | 2 +- .../impl/CustomUserRepositoryImpl.java | 28 ++++++++++++++ .../service/user/services/UserService.java | 5 ++- .../service/user/web/rest/UserResource.java | 21 +++++++++++ .../user/services/UserServiceTest.java | 10 +++++ .../user/web/rest/UserResourceTest.java | 7 ++++ 12 files changed, 132 insertions(+), 69 deletions(-) create mode 100644 user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java create mode 100644 user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java diff --git a/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResourceTest.java b/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResourceTest.java index 18f553251..56bfd1bd4 100644 --- a/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResourceTest.java +++ b/assertion-service/src/test/java/org/orcid/memberportal/service/assertion/web/rest/AssertionResourceTest.java @@ -86,10 +86,10 @@ class AssertionResourceTest { @Mock private RorOrgValidator rorOrgValidator; - + @Mock private MemberService memberService; - + @Mock private NotificationService notificationService; @@ -101,7 +101,7 @@ class AssertionResourceTest { @Mock private GridOrgValidator gridOrgValidator; - + @InjectMocks private AssertionResource assertionResource; @@ -115,7 +115,7 @@ public void setUp() { Mockito.when(assertionsUserService.getLoggedInUser()).thenReturn(getUser()); Mockito.when(assertionsUserService.getLoggedInUserSalesforceId()).thenReturn(DEFAULT_SALESFORCE_ID); } - + @Test void testGetAssertionOfPendingStatus() { Assertion pendingAssertion = new Assertion(); @@ -126,7 +126,7 @@ void testGetAssertionOfPendingStatus() { Mockito.verify(assertionService).findById(Mockito.eq("test")); Mockito.verify(assertionService).populatePermissionLink(Mockito.any(Assertion.class)); } - + @Test void testGetAssertionOfNotificationSentStatus() { Assertion pendingAssertion = new Assertion(); @@ -137,7 +137,7 @@ void testGetAssertionOfNotificationSentStatus() { Mockito.verify(assertionService).findById(Mockito.eq("test")); Mockito.verify(assertionService).populatePermissionLink(Mockito.any(Assertion.class)); } - + @Test void testGetAssertionOfDeniedAccessStatus() { Assertion pendingAssertion = new Assertion(); @@ -148,7 +148,7 @@ void testGetAssertionOfDeniedAccessStatus() { Mockito.verify(assertionService).findById(Mockito.eq("test")); Mockito.verify(assertionService).populatePermissionLink(Mockito.any(Assertion.class)); } - + @Test void testGetAssertionOfRevokedAccessStatus() { Assertion pendingAssertion = new Assertion(); @@ -159,7 +159,7 @@ void testGetAssertionOfRevokedAccessStatus() { Mockito.verify(assertionService).findById(Mockito.eq("test")); Mockito.verify(assertionService).populatePermissionLink(Mockito.any(Assertion.class)); } - + @Test void testGetAssertionOfInOrcidStatus() { Assertion pendingAssertion = new Assertion(); @@ -169,7 +169,7 @@ void testGetAssertionOfInOrcidStatus() { Mockito.verify(assertionService).findById(Mockito.eq("test")); Mockito.verify(assertionService, Mockito.never()).populatePermissionLink(Mockito.any(Assertion.class)); } - + @Test void testSendNotifications() { Mockito.when(assertionsUserService.getLoggedInUser()).thenReturn(getUser()); @@ -180,23 +180,23 @@ void testSendNotifications() { Mockito.verify(assertionService).markPendingAssertionsAsNotificationRequested(Mockito.eq(DEFAULT_SALESFORCE_ID)); Mockito.verify(memberService).updateMemberDefaultLanguage(Mockito.eq(DEFAULT_SALESFORCE_ID), Mockito.eq("en")); } - + @Test void testGetNotificationRequestInProgress_inProgressIsTrue() { Mockito.when(assertionsUserService.getLoggedInUserSalesforceId()).thenReturn(DEFAULT_SALESFORCE_ID); Mockito.when(notificationService.requestInProgress(Mockito.eq(DEFAULT_SALESFORCE_ID))).thenReturn(true); - + ResponseEntity response = assertionResource.getNotificationRequestInProgress(); assertTrue(response.getStatusCode().is2xxSuccessful()); assertNotNull(response.getBody()); assertTrue(response.getBody().getInProgress()); } - + @Test void testGetNotificationRequestInProgress_inProgressIsFalse() { Mockito.when(assertionsUserService.getLoggedInUserSalesforceId()).thenReturn(DEFAULT_SALESFORCE_ID); Mockito.when(notificationService.requestInProgress(Mockito.eq(DEFAULT_SALESFORCE_ID))).thenReturn(false); - + ResponseEntity response = assertionResource.getNotificationRequestInProgress(); assertTrue(response.getStatusCode().is2xxSuccessful()); assertNotNull(response.getBody()); @@ -390,14 +390,15 @@ void testGetAssertions() throws BadRequestAlertException, org.codehaus.jettison. ResponseEntity> page = assertionResource.getAssertions(Mockito.mock(Pageable.class), new HttpHeaders(), UriComponentsBuilder.newInstance(), ""); assertNotNull(page.getBody()); } - + @Test void testUpdateSalesforceId() { Mockito.when(assertionService.updateAssertionsSalesforceId(Mockito.eq("salesforce-id"), Mockito.eq("new-salesforce-id"))).thenReturn(true); + Mockito.when(orcidRecordService.updateTokenSalesforceIds(Mockito.eq("salesforce-id"), Mockito.eq("new-salesforce-id"))).thenReturn(true); ResponseEntity response = assertionResource.updateSalesforceId("salesforce-id", "new-salesforce-id"); assertTrue(response.getStatusCode().is2xxSuccessful()); } - + @Test void testUpdateSalesforceIdWithError() { Mockito.when(assertionService.updateAssertionsSalesforceId(Mockito.eq("salesforce-id"), Mockito.eq("new-salesforce-id"))).thenReturn(false); @@ -477,7 +478,7 @@ private AssertionServiceUser getUser() { user.setLangKey("en"); return user; } - + private NotificationRequest getNotificationRequest() { NotificationRequest request = new NotificationRequest(); request.setLanguage("en"); diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java index 5f410710d..8f7ca9388 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java @@ -42,4 +42,7 @@ public interface UserServiceClient { ResponseEntity deleteUser(@PathVariable("loginOrId") String loginOrId, @RequestParam(value = "noMainContactCheck", required = false) boolean noMainContactCheck); + @RequestMapping(method = RequestMethod.PUT, value = "/api/users/memberName/{oldMemberName}/{newMemberName}", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) + @HystrixProperty(name = "hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", value = "50000") + ResponseEntity updateUsersMemberNames(String salesforceId, String oldMemberName, String newMemberName); } diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java index 311a01352..16db6051d 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java @@ -134,20 +134,25 @@ public Member updateMember(Member member) { existingMember.setAssertionServiceEnabled(member.getAssertionServiceEnabled()); existingMember.setIsConsortiumLead(member.getIsConsortiumLead()); - return propagateUpdatesAndSave(member, existingMember); + propagateUpdatesAndSave(member, existingMember); + + return memberRepository.save(existingMember); } - private Member propagateUpdatesAndSave(Member member, Member existingMember) { - String oldSalesforceId = existingMember.getSalesforceId(); - String newSalesforceId = member.getSalesforceId(); + private void propagateUpdatesAndSave(Member member, Member existingMember) { + if (!existingMember.getSalesforceId().equals(member.getSalesforceId())) { + updateAssertionSalesforceIds(existingMember.getSalesforceId(), member.getSalesforceId()); + updateUserSalesforceIds(existingMember.getSalesforceId(), member.getSalesforceId()); + existingMember.setSalesforceId(member.getSalesforceId()); + } - // Check if salesforceId changed - if (!oldSalesforceId.equals(newSalesforceId)) { - updateAssertionSalesforceIds(oldSalesforceId, newSalesforceId); - updateUserSalesforceIds(oldSalesforceId, newSalesforceId); - return updateMemberSalesforceId(existingMember, newSalesforceId, oldSalesforceId); + if (!member.getClientName().equals(existingMember.getClientName())) { + updateUserMemberNames(existingMember.getSalesforceId(), existingMember.getClientName(), member.getClientName()); } - return memberRepository.save(existingMember); + } + + private void updateUserMemberNames(String salesforceId, String oldClientName, String newClientName) { + userService.updateUsersMemberNames(salesforceId, oldClientName, newClientName); } private void updateAssertionSalesforceIds(String oldSalesforceId, String newSalesforceId) { @@ -171,18 +176,6 @@ private void updateUserSalesforceIds(String oldSalesforceId, String newSalesforc } } - private Member updateMemberSalesforceId(Member existingMember, String newSalesforceId, String oldSalesforceId) { - existingMember.setSalesforceId(newSalesforceId); - try { - return memberRepository.save(existingMember); - } catch (Exception e) { - LOG.error("Error updating member's salesforce id from {} to {}. Rolling back user and assertion changes", oldSalesforceId, newSalesforceId); - assertionService.updateAssertionsSalesforceId(newSalesforceId, oldSalesforceId); - userService.updateUsersSalesforceId(newSalesforceId, oldSalesforceId); - throw new RuntimeException(e); - } - } - private void validateMemberUpdate(Member member, Optional existingMember) { MemberValidation validation = memberValidator.validate(member, userService.getLoggedInUser()); if (!validation.isValid()) { diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java index 50137c1eb..ff506fd3b 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java @@ -88,4 +88,11 @@ public void deleteUserById(String loginOrId, boolean noMainContactCheck) { } } + public void updateUsersMemberNames(String salesforceId, String oldClientName, String newClientName) { + ResponseEntity response = userServiceClient.updateUsersMemberNames(salesforceId, oldClientName, newClientName); + if (!response.getStatusCode().is2xxSuccessful()) { + LOG.warn("Error updating user member names {}, sf id {}, response code {}", oldClientName, salesforceId, response.getStatusCodeValue()); + throw new RuntimeException("Failed to update users' member names"); + } + } } diff --git a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java index 0f354a497..440c181b1 100644 --- a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java +++ b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java @@ -232,34 +232,6 @@ public Member answer(InvocationOnMock invocation) throws Throwable { Mockito.verify(memberRepository, Mockito.never()).save(Mockito.any(Member.class)); } - @Test - void testUpdateMemberWithSalesforceIdUpdateWithMemberFailure() { - Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation()); - Mockito.when(memberRepository.findById(Mockito.anyString())).thenReturn(Optional.of(getMember())); - Mockito.when(memberRepository.save(Mockito.any(Member.class))).thenAnswer(new Answer() { - @Override - public Member answer(InvocationOnMock invocation) throws Throwable { - return (Member) invocation.getArgument(0); - } - }); - - Mockito.doThrow(new RuntimeException()).when(memberRepository).save(Mockito.any(Member.class)); - - Member member = getMember(); - member.setId("id"); - member.setSalesforceId("three"); - - Assertions.assertThrows(RuntimeException.class, () -> { - memberService.updateMember(member); - }); - - // check assertion and user changes rolled back - Mockito.verify(assertionService, Mockito.times(1)).updateAssertionsSalesforceId(Mockito.eq("two"), Mockito.eq("three")); - Mockito.verify(assertionService, Mockito.times(1)).updateAssertionsSalesforceId(Mockito.eq("three"), Mockito.eq("two")); - Mockito.verify(userService, Mockito.times(1)).updateUsersSalesforceId(Mockito.eq("two"), Mockito.eq("three")); - Mockito.verify(userService, Mockito.times(1)).updateUsersSalesforceId(Mockito.eq("three"), Mockito.eq("two")); - } - @Test void testUpdateMemberWithSalesforceIdUpdate() { Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation()); diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java new file mode 100644 index 000000000..7c49a9576 --- /dev/null +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java @@ -0,0 +1,20 @@ +package org.orcid.memberportal.service.user.repository; + +import org.orcid.memberportal.service.user.domain.User; +import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; +import org.springframework.data.mongodb.repository.MongoRepository; +import org.springframework.stereotype.Repository; + +import java.time.Instant; +import java.util.List; +import java.util.Optional; + +/** + * Spring Data MongoDB repository for the {@link User} entity. + */ +@Repository +public interface CustomUserRepository { + + boolean updateMemberNames(String salesforceId, String oldMemberName, String newMemberName); +} diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/UserRepository.java b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/UserRepository.java index ea043b843..a589fc378 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/UserRepository.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/UserRepository.java @@ -14,7 +14,7 @@ * Spring Data MongoDB repository for the {@link User} entity. */ @Repository -public interface UserRepository extends MongoRepository { +public interface UserRepository extends MongoRepository, CustomUserRepository { Optional findOneByActivationKey(String activationKey); diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java new file mode 100644 index 000000000..2d3b057a2 --- /dev/null +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java @@ -0,0 +1,28 @@ +package org.orcid.memberportal.service.user.repository.impl; + +import org.orcid.memberportal.service.user.domain.User; +import org.orcid.memberportal.service.user.repository.CustomUserRepository; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.mongodb.core.MongoTemplate; +import org.springframework.data.mongodb.core.query.Criteria; +import org.springframework.data.mongodb.core.query.Query; +import org.springframework.data.mongodb.core.query.Update; +import org.springframework.stereotype.Repository; + +@Repository +public class CustomUserRepositoryImpl implements CustomUserRepository { + + + @Autowired + private MongoTemplate mongoTemplate; + + public CustomUserRepositoryImpl(MongoTemplate mongoTemplate) { + this.mongoTemplate = mongoTemplate; + } + + @Override + public boolean updateMemberNames(String salesforceId, String oldMemberName, String newMemberName) { + mongoTemplate.updateMulti(Query.query(Criteria.where("salesforceId").is(salesforceId).and("memberName").is(oldMemberName)), new Update().set("memberName", newMemberName), User.class); + return true; + } +} diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java b/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java index 0ef743074..ed2a246c2 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java @@ -9,9 +9,7 @@ import java.util.ArrayList; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.stream.Collectors; -import java.util.stream.Stream; import org.apache.commons.lang3.RandomStringUtils; import org.apache.commons.lang3.StringUtils; @@ -638,4 +636,7 @@ private boolean validLong(String code) { } } + public boolean updateUsersMemberName(String salesforceId, String oldMemberName, String newMemberName) { + return userRepository.updateMemberNames(salesforceId, oldMemberName, newMemberName); + } } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java index 23c96cb7f..dd1bf5eac 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java @@ -424,6 +424,27 @@ public ResponseEntity updateUsersSalesforceId(@PathVariable String salesfo } } + /** + * {@code PUT /users/memberName/:oldMemberName/:newMemberName} : Updates memberName + * for existing Users. + * + * @param salesforceId the salesforceId for finding users to update + * @param oldMemberName the old value of the memberName to update + * @param newMemberName the new Value of the memberName to update + * @return the {@link ResponseEntity} with status {@code 200 (OK)}. + */ + @PutMapping("/users/memberName/{oldMemberName}/{newMemberName}") + @PreAuthorize("hasRole(\"" + AuthoritiesConstants.ADMIN + "\")") + public ResponseEntity updateUsersMemberName(@PathVariable String salesforceId, @PathVariable String oldMemberName, @PathVariable String newMemberName) { + LOG.debug("REST request to update users' member names id from {} to {}", oldMemberName, newMemberName); + boolean success = userService.updateUsersMemberName(salesforceId, oldMemberName, newMemberName); + if (success) { + return ResponseEntity.ok().headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, "user", salesforceId)).build(); + } else { + return ResponseEntity.status(500).build(); + } + } + /** * {@code GET /users/:saleforceId}/owner : get the "login" user. * diff --git a/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java b/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java index f9273fc88..6b5e4e141 100644 --- a/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java +++ b/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java @@ -620,6 +620,16 @@ public String answer(InvocationOnMock invocation) throws Throwable { }); } + @Test + void testUpdateUsersMemberName() { + Mockito.when(userRepository.updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName"))).thenReturn(true); + + boolean success = userService.updateUsersMemberName("salesforce-id", "oldName", "newName"); + assertThat(success).isTrue(); + + Mockito.verify(userRepository).updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName")); + } + @Test void testUpdateUsersSalesforceId() { List firstPage = getUsersForSalesforceId("salesforce-id", 0, UserService.BATCH_SIZE); diff --git a/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java b/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java index ef3ed6927..c3691617b 100644 --- a/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java +++ b/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java @@ -80,6 +80,13 @@ void testUpdateSalesforceId() { assertTrue(response.getStatusCode().is2xxSuccessful()); } + @Test + void testUpdateUsersMemberName() { + Mockito.when(userService.updateUsersMemberName(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName"))).thenReturn(true); + ResponseEntity response = userResource.updateUsersMemberName("salesforce-id", "oldName", "newName"); + assertTrue(response.getStatusCode().is2xxSuccessful()); + } + @Test void testUpdateSalesforceIdWithError() { Mockito.when(userService.updateUsersSalesforceId(Mockito.eq("salesforce-id"), Mockito.eq("new-salesforce-id"))).thenReturn(false); From 3e10b5f4bafe817a072de614a6ea9e72bfd26c95 Mon Sep 17 00:00:00 2001 From: George Nash Date: Tue, 4 Mar 2025 18:52:55 +0000 Subject: [PATCH 07/10] corrected rest param bug --- .../memberportal/service/member/client/UserServiceClient.java | 4 ++-- .../memberportal/service/user/web/rest/UserResource.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java index 8f7ca9388..936a07e6d 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java @@ -42,7 +42,7 @@ public interface UserServiceClient { ResponseEntity deleteUser(@PathVariable("loginOrId") String loginOrId, @RequestParam(value = "noMainContactCheck", required = false) boolean noMainContactCheck); - @RequestMapping(method = RequestMethod.PUT, value = "/api/users/memberName/{oldMemberName}/{newMemberName}", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) + @RequestMapping(method = RequestMethod.PUT, value = "/api/users/memberName/{salesforceId}/{oldMemberName}/{newMemberName}", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) @HystrixProperty(name = "hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", value = "50000") - ResponseEntity updateUsersMemberNames(String salesforceId, String oldMemberName, String newMemberName); + ResponseEntity updateUsersMemberNames(@PathVariable("salesforceId") String salesforceId, @PathVariable("oldMemberName") String oldMemberName, @PathVariable("newMemberName") String newMemberName); } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java index dd1bf5eac..55c43bc50 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java @@ -433,7 +433,7 @@ public ResponseEntity updateUsersSalesforceId(@PathVariable String salesfo * @param newMemberName the new Value of the memberName to update * @return the {@link ResponseEntity} with status {@code 200 (OK)}. */ - @PutMapping("/users/memberName/{oldMemberName}/{newMemberName}") + @PutMapping("/users/memberName/{salesforceId}/{oldMemberName}/{newMemberName}") @PreAuthorize("hasRole(\"" + AuthoritiesConstants.ADMIN + "\")") public ResponseEntity updateUsersMemberName(@PathVariable String salesforceId, @PathVariable String oldMemberName, @PathVariable String newMemberName) { LOG.debug("REST request to update users' member names id from {} to {}", oldMemberName, newMemberName); From 8181ec569f566ded34548ea004898248de8f9195 Mon Sep 17 00:00:00 2001 From: George Nash Date: Tue, 4 Mar 2025 19:53:55 +0000 Subject: [PATCH 08/10] enabled edit sf id and fixed mongo query for tokens based on sf id --- .../repository/impl/OrcidRecordRepositoryCustomImpl.java | 2 +- ui/src/app/member/member-update.component.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java index 29c3cd99f..035575d13 100644 --- a/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java +++ b/assertion-service/src/main/java/org/orcid/memberportal/service/assertion/repository/impl/OrcidRecordRepositoryCustomImpl.java @@ -35,6 +35,6 @@ public OrcidRecordRepositoryCustomImpl(MongoTemplate mongoTemplate) { @Override public void updateTokenSalesforceIds(String oldSalesforceId, String newSalesforceId) { mongoTemplate.updateMulti(Query.query(Criteria.where("tokens.salesforce_id").is(oldSalesforceId)), - new Update().set("tokens.$.salesforce_id", oldSalesforceId), OrcidRecord.class); + new Update().set("tokens.$.salesforce_id", newSalesforceId), OrcidRecord.class); } } diff --git a/ui/src/app/member/member-update.component.ts b/ui/src/app/member/member-update.component.ts index 30c16e12f..b43dd7b76 100644 --- a/ui/src/app/member/member-update.component.ts +++ b/ui/src/app/member/member-update.component.ts @@ -155,7 +155,7 @@ export class MemberUpdateComponent implements OnInit { } if (sfId) { - this.editForm.get('salesforceId')?.disable() + // this.editForm.get('salesforceId')?.disable() } } From 5aad5f4fffeda48419e1174bc1c4e7f0aacedc67 Mon Sep 17 00:00:00 2001 From: George Nash Date: Tue, 4 Mar 2025 20:25:51 +0000 Subject: [PATCH 09/10] removed unnecessary old client name param from updating user member names, fixed bug where member not updated --- .../service/member/client/UserServiceClient.java | 4 ++-- .../service/member/services/MemberService.java | 8 ++++---- .../service/member/services/UserService.java | 6 +++--- .../service/user/repository/CustomUserRepository.java | 2 +- .../user/repository/impl/CustomUserRepositoryImpl.java | 4 ++-- .../memberportal/service/user/services/UserService.java | 4 ++-- .../memberportal/service/user/web/rest/UserResource.java | 9 ++++----- .../service/user/services/UserServiceTest.java | 6 +++--- .../service/user/web/rest/UserResourceTest.java | 4 ++-- 9 files changed, 23 insertions(+), 24 deletions(-) diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java index 936a07e6d..69466f997 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/client/UserServiceClient.java @@ -42,7 +42,7 @@ public interface UserServiceClient { ResponseEntity deleteUser(@PathVariable("loginOrId") String loginOrId, @RequestParam(value = "noMainContactCheck", required = false) boolean noMainContactCheck); - @RequestMapping(method = RequestMethod.PUT, value = "/api/users/memberName/{salesforceId}/{oldMemberName}/{newMemberName}", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) + @RequestMapping(method = RequestMethod.PUT, value = "/api/users/memberName/{salesforceId}/{newMemberName}", consumes = MediaType.APPLICATION_JSON, produces = MediaType.APPLICATION_JSON) @HystrixProperty(name = "hystrix.command.default.execution.isolation.thread.timeoutInMilliseconds", value = "50000") - ResponseEntity updateUsersMemberNames(@PathVariable("salesforceId") String salesforceId, @PathVariable("oldMemberName") String oldMemberName, @PathVariable("newMemberName") String newMemberName); + ResponseEntity updateUsersMemberNames(@PathVariable("salesforceId") String salesforceId, @PathVariable("newMemberName") String newMemberName); } diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java index 16db6051d..7675406a0 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/MemberService.java @@ -127,7 +127,6 @@ public Member updateMember(Member member) { Member existingMember = optional.get(); existingMember.setClientId(member.getClientId()); - existingMember.setClientName(member.getClientName()); existingMember.setParentSalesforceId(member.getParentSalesforceId()); existingMember.setLastModifiedBy(SecurityUtils.getCurrentUserLogin().get()); existingMember.setLastModifiedDate(Instant.now()); @@ -147,12 +146,13 @@ private void propagateUpdatesAndSave(Member member, Member existingMember) { } if (!member.getClientName().equals(existingMember.getClientName())) { - updateUserMemberNames(existingMember.getSalesforceId(), existingMember.getClientName(), member.getClientName()); + updateUserMemberNames(existingMember.getSalesforceId(), member.getClientName()); + existingMember.setClientName(member.getClientName()); } } - private void updateUserMemberNames(String salesforceId, String oldClientName, String newClientName) { - userService.updateUsersMemberNames(salesforceId, oldClientName, newClientName); + private void updateUserMemberNames(String salesforceId, String newClientName) { + userService.updateUsersMemberNames(salesforceId, newClientName); } private void updateAssertionSalesforceIds(String oldSalesforceId, String newSalesforceId) { diff --git a/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java b/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java index ff506fd3b..b7bf31360 100644 --- a/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java +++ b/member-service/src/main/java/org/orcid/memberportal/service/member/services/UserService.java @@ -88,10 +88,10 @@ public void deleteUserById(String loginOrId, boolean noMainContactCheck) { } } - public void updateUsersMemberNames(String salesforceId, String oldClientName, String newClientName) { - ResponseEntity response = userServiceClient.updateUsersMemberNames(salesforceId, oldClientName, newClientName); + public void updateUsersMemberNames(String salesforceId, String newClientName) { + ResponseEntity response = userServiceClient.updateUsersMemberNames(salesforceId, newClientName); if (!response.getStatusCode().is2xxSuccessful()) { - LOG.warn("Error updating user member names {}, sf id {}, response code {}", oldClientName, salesforceId, response.getStatusCodeValue()); + LOG.warn("Error updating user member names to {} for sf id {}, response code {}", newClientName, salesforceId, response.getStatusCodeValue()); throw new RuntimeException("Failed to update users' member names"); } } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java index 7c49a9576..a0247b56d 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/CustomUserRepository.java @@ -16,5 +16,5 @@ @Repository public interface CustomUserRepository { - boolean updateMemberNames(String salesforceId, String oldMemberName, String newMemberName); + boolean updateMemberNames(String salesforceId, String newMemberName); } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java index 2d3b057a2..5f981c4b8 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/repository/impl/CustomUserRepositoryImpl.java @@ -21,8 +21,8 @@ public CustomUserRepositoryImpl(MongoTemplate mongoTemplate) { } @Override - public boolean updateMemberNames(String salesforceId, String oldMemberName, String newMemberName) { - mongoTemplate.updateMulti(Query.query(Criteria.where("salesforceId").is(salesforceId).and("memberName").is(oldMemberName)), new Update().set("memberName", newMemberName), User.class); + public boolean updateMemberNames(String salesforceId, String newMemberName) { + mongoTemplate.updateMulti(Query.query(Criteria.where("salesforceId").is(salesforceId)), new Update().set("memberName", newMemberName), User.class); return true; } } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java b/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java index ed2a246c2..a935d285d 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/services/UserService.java @@ -636,7 +636,7 @@ private boolean validLong(String code) { } } - public boolean updateUsersMemberName(String salesforceId, String oldMemberName, String newMemberName) { - return userRepository.updateMemberNames(salesforceId, oldMemberName, newMemberName); + public boolean updateUsersMemberName(String salesforceId, String newMemberName) { + return userRepository.updateMemberNames(salesforceId, newMemberName); } } diff --git a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java index 55c43bc50..5648d20c5 100644 --- a/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java +++ b/user-service/src/main/java/org/orcid/memberportal/service/user/web/rest/UserResource.java @@ -429,15 +429,14 @@ public ResponseEntity updateUsersSalesforceId(@PathVariable String salesfo * for existing Users. * * @param salesforceId the salesforceId for finding users to update - * @param oldMemberName the old value of the memberName to update * @param newMemberName the new Value of the memberName to update * @return the {@link ResponseEntity} with status {@code 200 (OK)}. */ - @PutMapping("/users/memberName/{salesforceId}/{oldMemberName}/{newMemberName}") + @PutMapping("/users/memberName/{salesforceId}/{newMemberName}") @PreAuthorize("hasRole(\"" + AuthoritiesConstants.ADMIN + "\")") - public ResponseEntity updateUsersMemberName(@PathVariable String salesforceId, @PathVariable String oldMemberName, @PathVariable String newMemberName) { - LOG.debug("REST request to update users' member names id from {} to {}", oldMemberName, newMemberName); - boolean success = userService.updateUsersMemberName(salesforceId, oldMemberName, newMemberName); + public ResponseEntity updateUsersMemberName(@PathVariable String salesforceId, @PathVariable String newMemberName) { + LOG.debug("REST request to update users' member names id to {}", newMemberName); + boolean success = userService.updateUsersMemberName(salesforceId, newMemberName); if (success) { return ResponseEntity.ok().headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, "user", salesforceId)).build(); } else { diff --git a/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java b/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java index 6b5e4e141..7bf811a18 100644 --- a/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java +++ b/user-service/src/test/java/org/orcid/memberportal/service/user/services/UserServiceTest.java @@ -622,12 +622,12 @@ public String answer(InvocationOnMock invocation) throws Throwable { @Test void testUpdateUsersMemberName() { - Mockito.when(userRepository.updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName"))).thenReturn(true); + Mockito.when(userRepository.updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("newName"))).thenReturn(true); - boolean success = userService.updateUsersMemberName("salesforce-id", "oldName", "newName"); + boolean success = userService.updateUsersMemberName("salesforce-id", "newName"); assertThat(success).isTrue(); - Mockito.verify(userRepository).updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName")); + Mockito.verify(userRepository).updateMemberNames(Mockito.eq("salesforce-id"), Mockito.eq("newName")); } @Test diff --git a/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java b/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java index c3691617b..db42ba417 100644 --- a/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java +++ b/user-service/src/test/java/org/orcid/memberportal/service/user/web/rest/UserResourceTest.java @@ -82,8 +82,8 @@ void testUpdateSalesforceId() { @Test void testUpdateUsersMemberName() { - Mockito.when(userService.updateUsersMemberName(Mockito.eq("salesforce-id"), Mockito.eq("oldName"), Mockito.eq("newName"))).thenReturn(true); - ResponseEntity response = userResource.updateUsersMemberName("salesforce-id", "oldName", "newName"); + Mockito.when(userService.updateUsersMemberName(Mockito.eq("salesforce-id"), Mockito.eq("newName"))).thenReturn(true); + ResponseEntity response = userResource.updateUsersMemberName("salesforce-id", "newName"); assertTrue(response.getStatusCode().is2xxSuccessful()); } From 0e17b9031cc58fd11373b08c8b6296e0147f6488 Mon Sep 17 00:00:00 2001 From: George Nash Date: Tue, 4 Mar 2025 20:49:55 +0000 Subject: [PATCH 10/10] added extra unit test --- .../member/services/MemberServiceTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java index 440c181b1..b17d947fc 100644 --- a/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java +++ b/member-service/src/test/java/org/orcid/memberportal/service/member/services/MemberServiceTest.java @@ -256,6 +256,29 @@ public Member answer(InvocationOnMock invocation) throws Throwable { assertThat(saved.getSalesforceId()).isEqualTo("three"); } + @Test + void testUpdateMemberWithMemberName() { + Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation()); + Mockito.when(memberRepository.findById(Mockito.anyString())).thenReturn(Optional.of(getMember())); + Mockito.when(memberRepository.save(Mockito.any(Member.class))).thenAnswer(new Answer() { + @Override + public Member answer(InvocationOnMock invocation) throws Throwable { + return (Member) invocation.getArgument(0); + } + }); + Member member = getMember(); + member.setId("id"); + member.setClientName("a new member name"); + memberService.updateMember(member); + + // check assertion and user changes rolled back + Mockito.verify(userService, Mockito.times(1)).updateUsersMemberNames(Mockito.eq("two"), Mockito.eq("a new member name")); + Mockito.verify(memberRepository, Mockito.times(1)).save(memberCaptor.capture()); + + Member saved = memberCaptor.getValue(); + assertThat(saved.getClientName()).isEqualTo("a new member name"); + } + @Test void testUpdateMemberWithAssertionEnabledUpdate() { Mockito.when(memberValidator.validate(Mockito.any(Member.class), Mockito.any(MemberServiceUser.class))).thenReturn(getValidValidation());