From 84cc3718452334b5c9cc4e56b5292416ea14eeac Mon Sep 17 00:00:00 2001 From: Tim Yates Date: Fri, 12 Jul 2024 16:29:03 +0100 Subject: [PATCH 1/2] Limit emails sent by GuildMemberServices When GuildMemberServices save or delete is called from the GuildMemberController, we want to send an email. However when these methods are called from the GuildServices we don't, as this email is sent by GuildServices. This PR adds a flag as to whether an email should be sent, and fixes the mockito in the tests to ensure a single email is sent --- .../services/guild/GuildServicesImpl.java | 4 +-- .../guild/member/GuildMemberController.java | 8 ++++-- .../guild/member/GuildMemberServices.java | 4 +-- .../guild/member/GuildMemberServicesImpl.java | 28 +++++++++++-------- .../services/guild/GuildControllerTest.java | 3 ++ .../member/GuildMemberControllerTest.java | 5 ++++ 6 files changed, 33 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/com/objectcomputing/checkins/services/guild/GuildServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/guild/GuildServicesImpl.java index 575a173c6e..7a632f70a8 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/guild/GuildServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/guild/GuildServicesImpl.java @@ -176,7 +176,7 @@ public GuildResponseDTO update(GuildUpdateDTO guildDTO) { Optional first = existingGuildMembers.stream().filter(existing -> existing.getMemberId().equals(updatedMember.getMemberId())).findFirst(); MemberProfile existingMember = memberProfileServices.getById(updatedMember.getMemberId()); if(first.isEmpty()) { - newMembers.add(fromMemberEntity(guildMemberServices.save(fromMemberDTO(updatedMember, newGuildEntity.getId())), existingMember)); + newMembers.add(fromMemberEntity(guildMemberServices.save(fromMemberDTO(updatedMember, newGuildEntity.getId()), false), existingMember)); addedMembers.add(existingMember); } else { newMembers.add(fromMemberEntity(guildMemberServices.update(fromMemberDTO(updatedMember, newGuildEntity.getId())), existingMember)); @@ -186,7 +186,7 @@ public GuildResponseDTO update(GuildUpdateDTO guildDTO) { //delete any removed members from guild existingGuildMembers.forEach(existingMember -> { if(!guildDTO.getGuildMembers().stream().filter(updatedTeamMember -> updatedTeamMember.getMemberId().equals(existingMember.getMemberId())).findFirst().isPresent()) { - guildMemberServices.delete(existingMember.getId()); + guildMemberServices.delete(existingMember.getId(), false); removedMembers.add(memberProfileServices.getById(existingMember.getMemberId())); } }); diff --git a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberController.java b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberController.java index d4f0d29ba6..49d7bc139d 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberController.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberController.java @@ -37,8 +37,10 @@ public GuildMemberController(GuildMemberServices guildMemberServices) { @Post public HttpResponse createMembers(@Body @Valid GuildMemberCreateDTO guildMember, HttpRequest request) { - GuildMember newGuildMember = guildMemberServices.save(new GuildMember(guildMember.getGuildId(), - guildMember.getMemberId(), guildMember.getLead())); + GuildMember newGuildMember = guildMemberServices.save( + new GuildMember(guildMember.getGuildId(), guildMember.getMemberId(), guildMember.getLead()), + true + ); return HttpResponse .created(newGuildMember) .headers(headers -> headers.location( @@ -95,7 +97,7 @@ public Set findGuildMembers(@Nullable UUID guildid, */ @Delete("/{id}") public HttpResponse deleteGuildMember(@NotNull UUID id) { - guildMemberServices.delete(id); + guildMemberServices.delete(id, true); return HttpResponse .ok(); } diff --git a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServices.java b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServices.java index 191bf7d84a..660224b3e2 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServices.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServices.java @@ -5,13 +5,13 @@ public interface GuildMemberServices { - GuildMember save(GuildMember guildMember); + GuildMember save(GuildMember guildMember, boolean sendEmail); GuildMember read(UUID id); GuildMember update(GuildMember guildMember); - void delete(UUID id); + void delete(UUID id, boolean sendEmail); Set findByFields(UUID guildid, UUID memberid, Boolean lead); } diff --git a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java index 37a1f4101a..a3fbb5e5d1 100644 --- a/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java +++ b/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java @@ -57,7 +57,7 @@ public void setEmailSender(EmailSender emailSender) { this.emailSender = emailSender; } - public GuildMember save(@Valid @NotNull GuildMember guildMember) { + public GuildMember save(@Valid @NotNull GuildMember guildMember, boolean sendEmail) { final UUID guildId = guildMember.getGuildId(); final UUID memberId = guildMember.getMemberId(); Optional guild = guildRepo.findById(guildId); @@ -84,11 +84,13 @@ else if (!currentUserServices.isAdmin() && !guildMember.getMemberId().equals(cur throw new PermissionException(NOT_AUTHORIZED_MSG); } - emailSender - .sendEmail(null, null, "Membership changes have been made to the " + guild.get().getName() + " guild", - constructEmailContent(guildMember, true), - getGuildLeadsEmails(guildLeads, guildMember).toArray(new String[0]) - ); + if (sendEmail) { + emailSender + .sendEmail(null, null, "Membership changes have been made to the " + guild.get().getName() + " guild", + constructEmailContent(guildMember, true), + getGuildLeadsEmails(guildLeads, guildMember).toArray(new String[0]) + ); + } GuildMember guildMemberSaved = guildMemberRepo.save(guildMember); guildMemberHistoryRepository.save(buildGuildMemberHistory(guildId,memberId,"Added", LocalDateTime.now())); @@ -145,7 +147,7 @@ public Set findByFields(@Nullable UUID guildId, @Nullable UUID memb return guildMembers; } - public void delete(@NotNull UUID id) { + public void delete(@NotNull UUID id, boolean sendEmail) { MemberProfile currentUser = currentUserServices.getCurrentUser(); GuildMember guildMember = guildMemberRepo.findById(id).orElse(null); @@ -166,11 +168,13 @@ public void delete(@NotNull UUID id) { Guild guild = guildRepo.findById(guildMember.getGuildId()) .orElseThrow(() -> new NotFoundException("No Guild found with id " + guildMember.getGuildId())); - emailSender - .sendEmail(null, null, "Membership Changes have been made to the " + guild.getName() + " guild", - constructEmailContent(guildMember, false), - getGuildLeadsEmails(guildLeads, guildMember).toArray(new String[0]) - ); + if (sendEmail) { + emailSender + .sendEmail(null, null, "Membership Changes have been made to the " + guild.getName() + " guild", + constructEmailContent(guildMember, false), + getGuildLeadsEmails(guildLeads, guildMember).toArray(new String[0]) + ); + } guildMemberRepo.deleteById(id); guildMemberHistoryRepository.save(buildGuildMemberHistory(guildMember.getGuildId(),guildMember.getMemberId(),"Deleted", LocalDateTime.now())); diff --git a/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java index 8dcb461be4..bfa87f227c 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java @@ -31,6 +31,7 @@ import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; class GuildControllerTest extends TestContainersSuite implements GuildFixture, MemberProfileFixture, RoleFixture, GuildMemberFixture { @@ -86,6 +87,7 @@ void testEmailSentToGuildLeadWhenGuildMembersAdded() { "

Changes have been made to the Ninja guild.

The following members have been added:

  • Bill Charles
Click here to view the changes in the Check-Ins app.", "billm@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @Test @@ -115,6 +117,7 @@ void testEmailSentToGuildLeadWhenGuildMembersRemoved() { "

Changes have been made to the Ninja guild.

The following members have been removed:

  • Bill Charles
Click here to view the changes in the Check-Ins app.", "billm@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @Test diff --git a/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java index 4a70ec57f6..1f813bb967 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java @@ -32,6 +32,7 @@ import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; class GuildMemberControllerTest extends TestContainersSuite implements GuildFixture, MemberProfileFixture, RoleFixture, GuildMemberFixture { @@ -70,6 +71,7 @@ void testEmailSentWhenMemberRemovesThemselves() { "

Bill Charles has left the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @Test @@ -90,6 +92,7 @@ void testEmailSentWhenMemberAddsThemselves() { "

Bill Charles has joined the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @@ -113,6 +116,7 @@ void testEmailSentToMultipleRecipientsWhenMemberAddsThemselves() { "nobody@objectcomputing.com", "billm@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @@ -135,6 +139,7 @@ void noEmailSentToGuildLeadsThatRemoveThemselves() { "

Bill Charles has left the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com" ); + verifyNoMoreInteractions(emailSender); } @Test From 3ca2aaccf952b18e7e8695e85eba44ec5e15e30c Mon Sep 17 00:00:00 2001 From: Michael Kimberlin Date: Tue, 6 Aug 2024 11:55:09 -0500 Subject: [PATCH 2/2] Fixed tests broken by merge from develop --- .../services/guild/GuildControllerTest.java | 16 ++++------------ .../guild/member/GuildMemberControllerTest.java | 7 ------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java index c59edbdf9d..dd31663ddd 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java @@ -43,9 +43,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; @Property(name = "replace.mailjet.factory", value = StringUtils.TRUE) class GuildControllerTest extends TestContainersSuite implements GuildFixture, @@ -101,17 +98,14 @@ void testEmailSentToGuildLeadWhenGuildMembersAdded() { final HttpRequest request = HttpRequest.PUT("/", requestBody).basicAuth(memberProfileOfAdmin.getWorkEmail(), ADMIN_ROLE); client.toBlocking().exchange(request, GuildResponseDTO.class); - assertEquals(2, emailSender.events.size()); + assertEquals(1, emailSender.events.size()); assertEquals( List.of( - // Email to the Guild lead - List.of("SEND_EMAIL", "null", "null", "Membership changes have been made to the Ninja guild", "

Bill Charles has joined the Ninja guild.

Click here to view the changes in the Check-Ins app.", memberProfile.getWorkEmail() + "," + memberProfile2.getWorkEmail()), // Email to both the Guild leads List.of("SEND_EMAIL", "null", "null", "Membership Changes have been made to the Ninja guild", "

Changes have been made to the Ninja guild.

The following members have been added:

  • Bill Charles
Click here to view the changes in the Check-Ins app.", memberProfile.getWorkEmail() + "," + memberProfile2.getWorkEmail()) ), emailSender.events ); - verifyNoMoreInteractions(emailSender); } @Test @@ -136,14 +130,12 @@ void testEmailSentToGuildLeadWhenGuildMembersRemoved() { final HttpRequest request = HttpRequest.PUT("/", requestBody).basicAuth(memberProfileOfAdmin.getWorkEmail(), ADMIN_ROLE); client.toBlocking().exchange(request, GuildResponseDTO.class); - assertEquals(2, emailSender.events.size()); + assertEquals(1, emailSender.events.size()); assertEquals(List.of( - List.of("SEND_EMAIL", "null", "null", "Membership Changes have been made to the Ninja guild", "

Bill Charles has left the Ninja guild.

Click here to view the changes in the Check-Ins app.", memberProfile.getWorkEmail()), List.of("SEND_EMAIL", "null", "null", "Membership Changes have been made to the Ninja guild", "

Changes have been made to the Ninja guild.

The following members have been removed:

  • Bill Charles
Click here to view the changes in the Check-Ins app.", memberProfile.getWorkEmail()) ), emailSender.events ); - verifyNoMoreInteractions(emailSender); } @Test @@ -493,7 +485,7 @@ void deleteGuildByAdmin() { createDefaultGuildMember(guildEntity, memberProfileOfAdmin); final MutableHttpRequest request = HttpRequest.DELETE(String.format("/%s", guildEntity.getId())).basicAuth(memberProfileOfAdmin.getWorkEmail(), ADMIN_ROLE); - final HttpResponse response = client.toBlocking().exchange(request); + final HttpResponse response = client.toBlocking().exchange(request); assertEquals(HttpStatus.OK, response.getStatus()); } @@ -509,7 +501,7 @@ void deleteGuildByGuildLead() { // createDefaultGuildMember(guild, memberProfileOfGuildMember); final MutableHttpRequest request = HttpRequest.DELETE(String.format("/%s", guildEntity.getId())).basicAuth(memberProfileofGuildLeadEntity.getWorkEmail(), MEMBER_ROLE); - final HttpResponse response = client.toBlocking().exchange(request); + final HttpResponse response = client.toBlocking().exchange(request); assertEquals(HttpStatus.OK, response.getStatus()); } diff --git a/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java b/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java index f4826f4de7..b9c86ea75c 100644 --- a/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java +++ b/server/src/test/java/com/objectcomputing/checkins/services/guild/member/GuildMemberControllerTest.java @@ -39,9 +39,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; @Property(name = "replace.mailjet.factory", value = StringUtils.TRUE) class GuildMemberControllerTest extends TestContainersSuite implements GuildFixture, MemberProfileFixture, RoleFixture, GuildMemberFixture { @@ -78,7 +75,6 @@ void testEmailSentWhenMemberRemovesThemselves() { List.of("SEND_EMAIL", "null", "null", "Membership Changes have been made to the " + guild.getName() +" guild", "

Bill Charles has left the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com"), emailSender.events.getFirst() ); - verifyNoMoreInteractions(emailSender); } @Test @@ -99,7 +95,6 @@ void testEmailSentWhenMemberAddsThemselves() { List.of("SEND_EMAIL", "null", "null", "Membership changes have been made to the " + guild.getName() +" guild", "

Bill Charles has joined the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com"), emailSender.events.getFirst() ); - verifyNoMoreInteractions(emailSender); } @@ -122,7 +117,6 @@ void testEmailSentToMultipleRecipientsWhenMemberAddsThemselves() { List.of("SEND_EMAIL", "null", "null", "Membership changes have been made to the " + guild.getName() +" guild", "

Bill Charles has joined the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com,billm@objectcomputing.com"), emailSender.events.getFirst() ); - verifyNoMoreInteractions(emailSender); } @@ -145,7 +139,6 @@ void noEmailSentToGuildLeadsThatRemoveThemselves() { List.of("SEND_EMAIL", "null", "null", "Membership Changes have been made to the " + guild.getName() +" guild", "

Bill Charles has left the Ninja guild.

Click here to view the changes in the Check-Ins app.", "nobody@objectcomputing.com"), emailSender.events.getFirst() ); - verifyNoMoreInteractions(emailSender); } @Test