diff --git a/src/NuGet.Services.Messaging.Email/AsynchronousEmailMessageService.cs b/src/NuGet.Services.Messaging.Email/AsynchronousEmailMessageService.cs index da2aa3b8..938a7b5f 100644 --- a/src/NuGet.Services.Messaging.Email/AsynchronousEmailMessageService.cs +++ b/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 _logger; - public AsynchronousEmailMessageService(IEmailMessageEnqueuer emailMessageEnqueuer) + public AsynchronousEmailMessageService( + IEmailMessageEnqueuer emailMessageEnqueuer, + ILogger 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; } diff --git a/src/NuGet.Services.Messaging.Email/CoreMarkdownMessageService.cs b/src/NuGet.Services.Messaging.Email/CoreMarkdownMessageService.cs index ef078b21..8f165338 100644 --- a/src/NuGet.Services.Messaging.Email/CoreMarkdownMessageService.cs +++ b/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) diff --git a/tests/NuGet.Services.Messaging.Email.Tests/AsynchronousEmailMessageServiceFacts.cs b/tests/NuGet.Services.Messaging.Email.Tests/AsynchronousEmailMessageServiceFacts.cs index 4b7ef109..ad2fc476 100644 --- a/tests/NuGet.Services.Messaging.Email.Tests/AsynchronousEmailMessageServiceFacts.cs +++ b/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(() => new AsynchronousEmailMessageService(null)); + Assert.Throws(() => new AsynchronousEmailMessageService( + null, + Mock.Of>())); + } + + [Fact] + public void GivenANullLogger_ItShouldThrow() + { + Assert.Throws(() => new AsynchronousEmailMessageService( + new Mock().Object, + null)); } } @@ -26,7 +37,9 @@ public class TheSendMessageAsyncMethod public void ThrowsArgumentNullExceptionForNullEmailBuilder() { var emailMessageEnqueuer = new Mock().Object; - var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuer); + var messageService = new AsynchronousEmailMessageService( + emailMessageEnqueuer, + Mock.Of>()); Assert.ThrowsAsync(() => messageService.SendMessageAsync(null, It.IsAny(), It.IsAny())); } @@ -37,11 +50,13 @@ public void DoesNotEnqueueMessageWhenRecipientsToListEmpty() var emailBuilder = new Mock(); emailBuilder .Setup(m => m.GetRecipients()) - .Returns(EmailRecipients.None) + .Returns(new EmailRecipients(to: new MailAddress[0])) .Verifiable(); var emailMessageEnqueuerMock = new Mock(); - var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object); + var messageService = new AsynchronousEmailMessageService( + emailMessageEnqueuerMock.Object, + Mock.Of>()); messageService.SendMessageAsync(emailBuilder.Object, false, false); @@ -79,7 +94,9 @@ public void ThrowsArgumentExceptionWhenPlainTextAndHtmlBodyEmpty() .Verifiable(); var emailMessageEnqueuerMock = new Mock(); - var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object); + var messageService = new AsynchronousEmailMessageService( + emailMessageEnqueuerMock.Object, + Mock.Of>()); Assert.ThrowsAsync(() => messageService.SendMessageAsync(emailBuilder.Object, false, false)); @@ -105,7 +122,9 @@ public void ThrowsArgumentExceptionWhenSenderNull() .Verifiable(); var emailMessageEnqueuerMock = new Mock(); - var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object); + var messageService = new AsynchronousEmailMessageService( + emailMessageEnqueuerMock.Object, + Mock.Of>()); Assert.ThrowsAsync(() => messageService.SendMessageAsync(emailBuilder.Object, false, false)); @@ -155,7 +174,9 @@ public void CreatesAndSendsExpectedMessageOnce() .Verifiable(); var emailMessageEnqueuerMock = new Mock(); - var messageService = new AsynchronousEmailMessageService(emailMessageEnqueuerMock.Object); + var messageService = new AsynchronousEmailMessageService( + emailMessageEnqueuerMock.Object, + Mock.Of>()); messageService.SendMessageAsync(emailBuilder.Object, false, false);