Too many emails are being sent with changes via the GuildController

[timyates]

As part of reducing the Mockito usage in the tests, I created a PR which replaces the mock EmailSender

#2547

I believe this has exposed an issue whereby we are sending too many emails when members are added or removed.

The failure can be seen in the develop branch if we add:

        verifyNoMoreInteractions(emailSender);

After

check-ins/server/src/test/java/com/objectcomputing/checkins/services/guild/GuildControllerTest.java

Lines 84 to 88 in c1ac42b

	verify(emailSender).sendEmail(null, null,
	"Membership Changes have been made to the Ninja guild",
	"<h3>Changes have been made to the Ninja guild.</h3><h4>The following members have been added:</h4><ul><li>Bill Charles</li></ul><a href=\"https://checkins.objectcomputing.com/guilds\">Click here</a> to view the changes in the Check-Ins app.",
	"billm@objectcomputing.com"
	);

in the tests

Investigation results

So the GuildMemberServices save method sends an email

check-ins/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java

Lines 87 to 91 in 931a73d

	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])
	);

As does the delete method

check-ins/server/src/main/java/com/objectcomputing/checkins/services/guild/member/GuildMemberServicesImpl.java

Lines 169 to 173 in 931a73d

	emailSender
	.sendEmail(null, null, "Membership Changes have been made to the " + guild.getName() + " guild",
	constructEmailContent(guildMember, false),
	getGuildLeadsEmails(guildLeads, guildMember).toArray(new String[0])
	);

These are called from the GuildServices#update method

check-ins/server/src/main/java/com/objectcomputing/checkins/services/guild/GuildServicesImpl.java

Lines 174 to 197 in 931a73d

	//add new members to the guild
	guildDTO.getGuildMembers().forEach(updatedMember -> {
	Optional<GuildMember> 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));
	addedMembers.add(existingMember);
	} else {
	newMembers.add(fromMemberEntity(guildMemberServices.update(fromMemberDTO(updatedMember, newGuildEntity.getId())), existingMember));
	}
	});
	//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());
	removedMembers.add(memberProfileServices.getById(existingMember.getMemberId()));
	}
	});
	updated = fromEntity(newGuildEntity, newMembers);
	if (!emailsOfGuildLeadsExcludingChanger.isEmpty() && (!addedMembers.isEmpty() || !removedMembers.isEmpty())){
	sendGuildMemberChangeNotification(addedMembers, removedMembers, newGuildEntity.getName(), emailsOfGuildLeadsExcludingChanger);
	}

Which -- at the bottom of the code above -- then also sends an email to the guild leads...

Potential solution...

As GuildMemberServices#save and GuildMemberServices#delete is called from multiple places in the code, we could add a boolean to the two methods denoting whether an email should be sent.

Then for all places excluding GuildServices#update pass a true value (so an email is sent), but in GuildServices#update send a false as the email will be sent by the method itself once it's finished

Not 100% sure this is a fix rather than a band-aid though 🤔