Don't attempt to send emails if they have no recipients #6637
Conversation
[ReleasePrep][2018.11.05]RI Dev into master
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
chenriksson,
cristinamanum,
skofman1 and
zhhyu
| <PackageReference Include="NuGet.Services.Messaging"> | ||
| <Version>2.31.0</Version> | ||
| <PackageReference Include="NuGet.Services.Messaging.Email"> | ||
| <Version>2.36.0-sb-emptyto-2190469</Version> |
There was a problem hiding this comment.
Will align with master build once other PR is out.
| @@ -202,11 +202,8 @@ | |||
| <EmbeddedResource Include="Infrastructure\MigrateUserToOrganization.sql" /> | |||
| </ItemGroup> | |||
| <ItemGroup> | |||
| <PackageReference Include="NuGet.Services.Entities"> | |||
There was a problem hiding this comment.
I removed these dependencies because they are transitively pulled in by NuGet.Services.Messaging.Email. I thought having this single dependency to rev will make this easier to manage in the future.
I have no problem with reverting this back if you think this isn't a good idea.
| { | ||
| public static class GalleryEmailRecipientsUtility | ||
| { | ||
| public static IReadOnlyList<MailAddress> GetAddressesWithPermission(User user, ActionRequiringAccountPermissions action) |
There was a problem hiding this comment.
This is EmailRecipientsWithPermission.AddAddressesWithPermission, but moved into a static method.
EmailRecipientsWithPermission was an unnecessary extra layer of abstraction so I removed it as part of this PR.
This is the pattern existing in ServerCommon: see EmailRecipients.GetAllOwners and EmailRecipients.GetOwnersSubscribedToPackagePushedNotification.
| @@ -31,13 +31,10 @@ public class OrganizationMemberRemovedMessage : MarkdownEmailBuilder | |||
|
|
|||
| public override IEmailRecipients GetRecipients() | |||
| { | |||
| if (!Organization.EmailAllowed) | |||
| { | |||
| return EmailRecipients.None; | |||
There was a problem hiding this comment.
I removed the usage of EmailRecipients.None because I think it's misleading. We're using EmailRecipients.None as a signal for "this email shouldn't be sent" when the real signal for when an email shouldn't be sent is whether or not it has any recipients to send to. Because "an email without recipients to send to" and "an email with no recipients at all" are not logically equivalent, this seems odd.
For example, in this function, we return EmailRecipients.None when the intended To address doesn't allow emails. Does whether or not the To address allows emails affect the replyTo field of the email? Technically no. So then why are we returning a completely empty list of recipients when there are people whom replies to the email should go to?
There was a problem hiding this comment.
Makes sense, good call! 👍
| @@ -31,13 +31,10 @@ public class OrganizationMemberRemovedMessage : MarkdownEmailBuilder | |||
|
|
|||
| public override IEmailRecipients GetRecipients() | |||
| { | |||
| if (!Organization.EmailAllowed) | |||
| { | |||
| return EmailRecipients.None; | |||
There was a problem hiding this comment.
Makes sense, good call! 👍
Enabling accepting license metadata. License files are not yet allowed.
* Package delete was broken with recent update. This fixes it. * Moved folder names into its own class. Updated tests to include All folders. * Added tests to verify all folders are covered by tests.
* Upload license expression redirect url logic * fix nit and update
Take new version of ServerCommon to GalleryCore.
scottbommarito
force-pushed
the
sb-fixemailrecipients
branch
from
a1306e7
to
1a4da8d
Compare
Aligns Gallery with NuGet/ServerCommon#222
Also made some coding style improvements from #6638 that I didn't want to make as part of a hotfix.