Don't attempt to send emails if they have no recipients

src/NuGet.Services.Messaging.Email/AsynchronousEmailMessageService.cs

@@ -5,16 +5,21 @@
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading.Tasks;
+using Microsoft.Extensions.Logging;
 
 namespace NuGet.Services.Messaging.Email
 {
     public class AsynchronousEmailMessageService : IMessageService
     {
         private readonly IEmailMessageEnqueuer _emailMessageEnqueuer;
+        private readonly ILogger<AsynchronousEmailMessageService> _logger;
 
-        public AsynchronousEmailMessageService(IEmailMessageEnqueuer emailMessageEnqueuer)
+        public AsynchronousEmailMessageService(
+            IEmailMessageEnqueuer emailMessageEnqueuer,
+            ILogger<AsynchronousEmailMessageService> logger)
         {
             _emailMessageEnqueuer = emailMessageEnqueuer ?? throw new ArgumentNullException(nameof(emailMessageEnqueuer));
+            _logger = logger ?? throw new ArgumentNullException(nameof(logger));
         }
 
         public Task SendMessageAsync(
@@ -27,6 +32,9 @@ public Task SendMessageAsync(
                 throw new ArgumentNullException(nameof(emailBuilder));
             }
 
+            _logger.LogInformation("Sending message with CopySender {CopySender} and DiscloseSenderAddress {DiscloseSenderAddress}",
+                copySender, discloseSenderAddress);
+
             var message = CreateMessage(
                 emailBuilder,
                 copySender,
@@ -35,16 +43,16 @@ public Task SendMessageAsync(
             return EnqueueMessageAsync(message);
         }
 
-        private static EmailMessageData CreateMessage(
+        private EmailMessageData CreateMessage(
             IEmailBuilder emailBuilder,
             bool copySender = false,
             bool discloseSenderAddress = false)
         {
             var recipients = emailBuilder.GetRecipients();
 
-            if (recipients == EmailRecipients.None)
+            if (!recipients.To.Any())
             {
-                // Optimization: no need to construct message body when no recipients.
+                _logger.LogInformation("Cannot create message to send as it has no recipients.");
                 return null;
             }
 
@@ -94,6 +102,7 @@ private Task EnqueueMessageAsync(EmailMessageData message)
         {
             if (message == null || !message.To.Any())
             {
+                _logger.LogInformation("Skipping enqueueing message because it is null or has no recipients.");
                 return Task.CompletedTask;
             }
 

src/NuGet.Services.Messaging.Email/CoreMarkdownMessageService.cs

@@ -39,6 +39,12 @@ public async Task SendMessageAsync(IEmailBuilder emailBuilder, bool copySender =
 
             using (var email = CreateMailMessage(emailBuilder))
             {
+                if (email == null || !email.To.Any())
+                {
+                    // A null email or one without recipients cannot be sent.
+                    return;
+                }
+
                 await SendMessageInternalAsync(email);
 
                 if (copySender && !discloseSenderAddress)
@@ -50,11 +56,6 @@ public async Task SendMessageAsync(IEmailBuilder emailBuilder, bool copySender =
 
         protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage)
         {
-            if (!mailMessage.To.Any())
-            {
-                return;
-            }
-
             var attempt = 0;
             var success = false;
             while (!success)

tests/NuGet.Services.Messaging.Email.Tests/AsynchronousEmailMessageServiceFacts.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Linq;
 using System.Net.Mail;
+using Microsoft.Extensions.Logging;
 using Moq;
 using Xunit;
 
@@ -16,7 +17,17 @@ public class TheConstructor
             [Fact]
             public void GivenANullEnqueuer_ItShouldThrow()
             {
-                Assert.Throws<ArgumentNullException>(() => new AsynchronousEmailMessageService(null));
+                Assert.Throws<ArgumentNullException>(() => new AsynchronousEmailMessageService(
+                    null,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>()));
+            }
+
+            [Fact]
+            public void GivenANullLogger_ItShouldThrow()
+            {
+                Assert.Throws<ArgumentNullException>(() => new AsynchronousEmailMessageService(
+                    new Mock<IEmailMessageEnqueuer>().Object,
+                    null));
             }
         }
 
@@ -26,7 +37,9 @@ public class TheSendMessageAsyncMethod
             public void ThrowsArgumentNullExceptionForNullEmailBuilder()
             {
                 var emailMessageEnqueuer = new Mock<IEmailMessageEnqueuer>().Object;
-                var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuer);
+                var messageService = new AsynchronousEmailMessageService(
+                    emailMessageEnqueuer,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>());
 
                 Assert.ThrowsAsync<ArgumentNullException>(() => messageService.SendMessageAsync(null, It.IsAny<bool>(), It.IsAny<bool>()));
             }
@@ -37,11 +50,13 @@ public void DoesNotEnqueueMessageWhenRecipientsToListEmpty()
                 var emailBuilder = new Mock<IEmailBuilder>();
                 emailBuilder
                     .Setup(m => m.GetRecipients())
-                    .Returns(EmailRecipients.None)
+                    .Returns(new EmailRecipients(to: new MailAddress[0]))
                     .Verifiable();
 
                 var emailMessageEnqueuerMock = new Mock<IEmailMessageEnqueuer>();
-                var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object);
+                var messageService = new AsynchronousEmailMessageService(
+                    emailMessageEnqueuerMock.Object,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>());
 
                 messageService.SendMessageAsync(emailBuilder.Object, false, false);
 
@@ -79,7 +94,9 @@ public void ThrowsArgumentExceptionWhenPlainTextAndHtmlBodyEmpty()
                      .Verifiable();
 
                 var emailMessageEnqueuerMock = new Mock<IEmailMessageEnqueuer>();
-                var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object);
+                var messageService = new AsynchronousEmailMessageService(
+                    emailMessageEnqueuerMock.Object,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>());
 
                 Assert.ThrowsAsync<ArgumentException>(() => messageService.SendMessageAsync(emailBuilder.Object, false, false));
 
@@ -105,7 +122,9 @@ public void ThrowsArgumentExceptionWhenSenderNull()
                      .Verifiable();
 
                 var emailMessageEnqueuerMock = new Mock<IEmailMessageEnqueuer>();
-                var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object);
+                var messageService = new AsynchronousEmailMessageService(
+                    emailMessageEnqueuerMock.Object,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>());
 
                 Assert.ThrowsAsync<ArgumentException>(() => messageService.SendMessageAsync(emailBuilder.Object, false, false));
 
@@ -155,7 +174,9 @@ public void CreatesAndSendsExpectedMessageOnce()
                      .Verifiable();
 
                 var emailMessageEnqueuerMock = new Mock<IEmailMessageEnqueuer>();
-                var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object);
+                var messageService = new AsynchronousEmailMessageService(
+                    emailMessageEnqueuerMock.Object,
+                    Mock.Of<ILogger<AsynchronousEmailMessageService>>());
 
                 messageService.SendMessageAsync(emailBuilder.Object, false, false);