Don't attempt to send emails if they have no recipients #222
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
chenriksson,
cristinamanum,
skofman1 and
zhhyu
scottbommarito
changed the title
| { | ||
| // Optimization: no need to construct message body when no recipients. | ||
| _logger.LogInformation("Message has no recipients. Cannot send."); |
There was a problem hiding this comment.
Cannot send [](start = 67, length = 11)
Cannot send. could be confused as an error. What do you think of a message like Skipping message as it has no recipients?
There was a problem hiding this comment.
Split it into two log messages.
Cannot create message to send as it has no recipients. in this function
Skipping enqueueing message because it is null or has no recipients. in EnqueueMessageAsync
| @@ -27,6 +32,9 @@ public AsynchronousEmailMessageService(IEmailMessageEnqueuer emailMessageEnqueue | |||
| throw new ArgumentNullException(nameof(emailBuilder)); | |||
| } | |||
|
|
|||
| _logger.LogInformation("Sending message with CopySender {CopySender} and DiscloseSenderAddress {DiscloseSenderAddress}", | |||
| copySender, discloseSenderAddress); | |||
|
|
|||
| var message = CreateMessage( | |||
There was a problem hiding this comment.
CreateMessage [](start = 26, length = 13)
It's not clear here that CreateMessage may return null, which EnqueueMessageAsync handles by doing no work. What do you think of renaming CreateMessage to CreateMessageOrNull, and not calling EnqueueMessageAsync if CreateMessageOrNull returned a null message?
There was a problem hiding this comment.
I don't want to change the structure of the code here that much. I agree it's confusing however.
NuGet/NuGetGallery#6632
EmailRecipients.Noneis being compared to by reference, which always fails because it is unused outside of this check.