Hotfix: Do not CC sender for Contact Owners emails (#6602)

Scott Bommarito · web-flow · commit 9070951769c2 · 2018-10-26T15:20:00.000-07:00
diff --git a/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs b/src/NuGetGallery.Core/Infrastructure/mail/CoreMarkdownMessageService.cs
@@ -39,11 +39,16 @@ public async Task SendMessageAsync(IEmailBuilder emailBuilder, bool copySender =
 
             using (var email = CreateMailMessage(emailBuilder))
             {
-                await SendMessageInternalAsync(email, copySender, discloseSenderAddress);
+                await SendMessageInternalAsync(email);
+
+                if (copySender && !discloseSenderAddress)
+                {
+                    await SendMessageToSenderAsync(email);
+                }
             }
         }
 
-        protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false)
+        protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage)
         {
             if (!mailMessage.To.Any())
             {
@@ -72,11 +77,6 @@ protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, b
                     }
                 }
             }
-
-            if (copySender && !discloseSenderAddress)
-            {
-                await SendMessageToSenderAsync(mailMessage);
-            }
         }
 
         protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber)
diff --git a/src/NuGetGallery/Controllers/PackagesController.cs b/src/NuGetGallery/Controllers/PackagesController.cs
@@ -1115,8 +1115,7 @@ public virtual async Task<ActionResult> ContactOwners(string id, string version,
                 package,
                 Url.Package(package, false),
                 HttpUtility.HtmlEncode(contactForm.Message),
-                Url.AccountSettings(relativeUrl: false),
-                contactForm.CopySender);
+                Url.AccountSettings(relativeUrl: false));
 
             await _messageService.SendMessageAsync(contactOwnersMessage, contactForm.CopySender, discloseSenderAddress: false);
 
diff --git a/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs b/src/NuGetGallery/Infrastructure/Mail/BackgroundMarkdownMessageService.cs
@@ -30,7 +30,7 @@ public BackgroundMarkdownMessageService(
         private Func<BackgroundMarkdownMessageService> _messageServiceFactory;
         private bool _sentMessage;
 
-        protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false)
+        protected override Task SendMessageInternalAsync(MailMessage mailMessage)
         {
             // Some MVC controller actions send more than one message. Since this method sends
             // the message async, we need a new IMessageService per email, to avoid calling
@@ -39,7 +39,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c
             if (_sentMessage)
             {
                 var newMessageService = _messageServiceFactory.Invoke();
-                return newMessageService.SendMessageInternalAsync(mailMessage, copySender, discloseSenderAddress);
+                return newMessageService.SendMessageInternalAsync(mailMessage);
             }
             else
             {
@@ -55,7 +55,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c
                     {
                         try
                         {
-                            await base.SendMessageInternalAsync(messageCopy, copySender, discloseSenderAddress);
+                            await base.SendMessageInternalAsync(messageCopy);
                         }
                         catch (Exception ex)
                         {
diff --git a/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs b/src/NuGetGallery/Infrastructure/Mail/Messages/ContactOwnersMessage.cs
@@ -17,24 +17,21 @@ public ContactOwnersMessage(
             Package package,
             string packageUrl,
             string htmlEncodedMessage,
-            string emailSettingsUrl,
-            bool copySender)
+            string emailSettingsUrl)
         {
             _configuration = configuration ?? throw new ArgumentNullException(nameof(configuration));
             FromAddress = fromAddress ?? throw new ArgumentNullException(nameof(fromAddress));
             Package = package ?? throw new ArgumentNullException(nameof(package));
             PackageUrl = packageUrl ?? throw new ArgumentNullException(nameof(packageUrl));
             HtmlEncodedMessage = htmlEncodedMessage ?? throw new ArgumentNullException(nameof(htmlEncodedMessage));
             EmailSettingsUrl = emailSettingsUrl ?? throw new ArgumentNullException(nameof(emailSettingsUrl));
-            CopySender = copySender;
         }
 
         public MailAddress FromAddress { get; }
         public Package Package { get; }
         public string PackageUrl { get; }
         public string HtmlEncodedMessage { get; }
         public string EmailSettingsUrl { get; }
-        public bool CopySender { get; }
 
         public override MailAddress Sender => _configuration.GalleryOwner;
 
@@ -46,7 +43,6 @@ public override IEmailRecipients GetRecipients()
 
             return new EmailRecipients(
                 to,
-                cc: CopySender ? new[] { FromAddress } : null,
                 replyTo: new[] { FromAddress });
         }
 
diff --git a/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs b/tests/NuGetGallery.Facts/Infrastructure/Mail/Messages/ContactOwnersMessageFacts.cs
@@ -43,8 +43,7 @@ public void GivenANullArgument_ItShouldThrow(
                     package,
                     packageUrl,
                     message,
-                    emailSettingsUrl,
-                    copySender));
+                    emailSettingsUrl));
             }
         }
 
@@ -72,19 +71,9 @@ public void AddsFromAddressToReplyToList()
             }
 
             [Fact]
-            public void AddsFromAddressToCCListWhenCopyingSender()
+            public void DoesNotAddFromAddressToCCList()
             {
-                var message = CreateMessage(copySender: true);
-                var recipients = message.GetRecipients();
-
-                Assert.Equal(1, recipients.CC.Count);
-                Assert.Contains(Fakes.FromAddress, recipients.CC);
-            }
-
-            [Fact]
-            public void DoesNotAddFromAddressToCCListWhenNotCopyingSender()
-            {
-                var message = CreateMessage(copySender: false);
+                var message = CreateMessage();
                 var recipients = message.GetRecipients();
 
                 Assert.Empty(recipients.CC);
@@ -122,16 +111,15 @@ public void SetsGalleryOwnerAsSender()
             Assert.Equal(Configuration.GalleryOwner, message.Sender);
         }
 
-        private static ContactOwnersMessage CreateMessage(bool copySender = false)
+        private static ContactOwnersMessage CreateMessage()
         {
             return new ContactOwnersMessage(
                 Configuration,
                 Fakes.FromAddress,
                 Fakes.Package,
                 Fakes.PackageUrl,
                 "user input",
-                Fakes.EmailSettingsUrl,
-                copySender);
+                Fakes.EmailSettingsUrl);
         }
 
         private const string _expectedMarkdownBody =
diff --git a/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/MessageServiceFacts.cs
@@ -233,7 +233,7 @@ public class TheSendContactOwnersMessageMethod
             : TestContainer
         {
             [Fact]
-            public async Task WillCopySenderIfAsked()
+            public async Task WillSendCopyToSenderIfAsked()
             {
                 // arrange
                 var packageId = "smangit";
@@ -267,8 +267,7 @@ public async Task WillCopySenderIfAsked()
                     package,
                     "http://someurl/",
                     "Test message",
-                    "http://someotherurl/",
-                    copySender: true);
+                    "http://someotherurl/");
 
                 // act
                 await messageService.SendMessageAsync(contactOwnersMessage, copySender: true, discloseSenderAddress: false);
@@ -285,6 +284,8 @@ public async Task WillCopySenderIfAsked()
                 Assert.Equal(TestGalleryOwner, messages[1].From);
                 Assert.Equal(fromAddress, messages[0].ReplyToList.Single().Address);
                 Assert.Equal(fromAddress, messages[1].ReplyToList.Single().Address);
+                Assert.Empty(messages[0].CC);
+                Assert.Empty(messages[1].CC);
             }
 
             [Fact]
@@ -320,11 +321,10 @@ public async Task WillSendEmailToAllOwners()
                     package,
                     "http://packageUrl/",
                     "Test message",
-                    "http://emailSettingsUrl/",
-                    copySender: false);
+                    "http://emailSettingsUrl/");
 
                 await messageService.SendMessageAsync(contactOwnersMessage);
-                var message = messageService.MockMailSender.Sent.Last();
+                var message = messageService.MockMailSender.Sent.Single();
 
                 Assert.Equal(owner1Email, message.To[0].Address);
                 Assert.Equal(owner2Email, message.To[1].Address);
@@ -372,11 +372,10 @@ public async Task WillNotSendEmailToOwnerThatOptsOut()
                     package,
                     "http://someurl/",
                     "Test message",
-                    "http://someotherurl/",
-                    copySender: false);
+                    "http://someotherurl/");
 
                 await messageService.SendMessageAsync(contactOwnersMessage);
-                var message = messageService.MockMailSender.Sent.Last();
+                var message = messageService.MockMailSender.Sent.Single();
 
                 // assert
                 Assert.Equal(ownerAddress, message.To[0].Address);
@@ -418,8 +417,7 @@ public async Task WillNotAttemptToSendIfNoOwnersAllow()
                     package,
                     "http://someurl/",
                     "Test message",
-                    "http://someotherurl/",
-                    copySender: false);
+                    "http://someotherurl/");
 
                 await messageService.SendMessageAsync(contactOwnersMessage);
 
@@ -428,7 +426,7 @@ public async Task WillNotAttemptToSendIfNoOwnersAllow()
             }
 
             [Fact]
-            public async Task WillNotCopySenderIfNoOwnersAllow()
+            public async Task WillNotSendCopyToSenderIfNoOwnersAllow()
             {
                 // arrange
                 var packageId = "smangit";
@@ -461,8 +459,7 @@ public async Task WillNotCopySenderIfNoOwnersAllow()
                     package,
                     "http://someurl/",
                     "Test message",
-                    "http://someotherurl/",
-                    copySender: false);
+                    "http://someotherurl/");
                 await messageService.SendMessageAsync(contactOwnersMessage);
 
                 // assert

Original file line number	Diff line number	Diff line change
`@@ -39,11 +39,16 @@ public async Task SendMessageAsync(IEmailBuilder emailBuilder, bool copySender =`
`39`	`39`
`40`	`40`	`using (var email = CreateMailMessage(emailBuilder))`
`41`	`41`	`{`
`42`		`- await SendMessageInternalAsync(email, copySender, discloseSenderAddress);`
	`42`	`+ await SendMessageInternalAsync(email);`
	`43`	`+`
	`44`	`+ if (copySender && !discloseSenderAddress)`
	`45`	`+ {`
	`46`	`+ await SendMessageToSenderAsync(email);`
	`47`	`+ }`
`43`	`48`	`}`
`44`	`49`	`}`
`45`	`50`
`46`		`- protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false)`
	`51`	`+ protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage)`
`47`	`52`	`{`
`48`	`53`	`if (!mailMessage.To.Any())`
`49`	`54`	`{`
`@@ -72,11 +77,6 @@ protected virtual async Task SendMessageInternalAsync(MailMessage mailMessage, b`
`72`	`77`	`}`
`73`	`78`	`}`
`74`	`79`	`}`
`75`		`-`
`76`		`- if (copySender && !discloseSenderAddress)`
`77`		`- {`
`78`		`- await SendMessageToSenderAsync(mailMessage);`
`79`		`- }`
`80`	`80`	`}`
`81`	`81`
`82`	`82`	`protected virtual Task AttemptSendMessageAsync(MailMessage mailMessage, int attemptNumber)`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ public BackgroundMarkdownMessageService(`
`30`	`30`	`private Func<BackgroundMarkdownMessageService> _messageServiceFactory;`
`31`	`31`	`private bool _sentMessage;`
`32`	`32`
`33`		`- protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool copySender = false, bool discloseSenderAddress = false)`
	`33`	`+ protected override Task SendMessageInternalAsync(MailMessage mailMessage)`
`34`	`34`	`{`
`35`	`35`	`// Some MVC controller actions send more than one message. Since this method sends`
`36`	`36`	`// the message async, we need a new IMessageService per email, to avoid calling`
`@@ -39,7 +39,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c`
`39`	`39`	`if (_sentMessage)`
`40`	`40`	`{`
`41`	`41`	`var newMessageService = _messageServiceFactory.Invoke();`
`42`		`- return newMessageService.SendMessageInternalAsync(mailMessage, copySender, discloseSenderAddress);`
	`42`	`+ return newMessageService.SendMessageInternalAsync(mailMessage);`
`43`	`43`	`}`
`44`	`44`	`else`
`45`	`45`	`{`
`@@ -55,7 +55,7 @@ protected override Task SendMessageInternalAsync(MailMessage mailMessage, bool c`
`55`	`55`	`{`
`56`	`56`	`try`
`57`	`57`	`{`
`58`		`- await base.SendMessageInternalAsync(messageCopy, copySender, discloseSenderAddress);`
	`58`	`+ await base.SendMessageInternalAsync(messageCopy);`
`59`	`59`	`}`
`60`	`60`	`catch (Exception ex)`
`61`	`61`	`{`