Include failure reasons in email sent when package validation fails (#471)

Author: Scott Bommarito

src/NuGet.Services.Validation.Orchestrator/IMessageService.cs

@@ -8,8 +8,7 @@ namespace NuGet.Services.Validation.Orchestrator
     public interface IMessageService<T> where T : class, IEntity
     {
         void SendPublishedMessage(T entity);
-        void SendValidationFailedMessage(T entity);
-        void SendSignedValidationFailedMessage(T entity);
+        void SendValidationFailedMessage(T entity, PackageValidationSet validationSet);
         void SendValidationTakingTooLongMessage(T entity);
     }
 }

src/NuGet.Services.Validation.Orchestrator/MessageService.cs

@@ -55,23 +55,15 @@ public void SendPublishedMessage(Package package)
             _coreMessageService.SendPackageAddedNotice(package, galleryPackageUrl, packageSupportUrl, _emailConfiguration.EmailSettingsUrl);
         }
 
-        public void SendValidationFailedMessage(Package package)
+        public void SendValidationFailedMessage(Package package, PackageValidationSet validationSet)
         {
             package = package ?? throw new ArgumentNullException(nameof(package));
+            validationSet = validationSet ?? throw new ArgumentNullException(nameof(validationSet));
 
             var galleryPackageUrl = GalleryPackageUrl(package);
             var packageSupportUrl = PackageSupportUrl(package);
 
-            _coreMessageService.SendPackageValidationFailedNotice(package, galleryPackageUrl, packageSupportUrl);
-        }
-
-        public void SendSignedValidationFailedMessage(Package package)
-        {
-            package = package ?? throw new ArgumentNullException(nameof(package));
-
-            var galleryPackageUrl = GalleryPackageUrl(package);
-
-            _coreMessageService.SendSignedPackageNotAllowedNotice(package, galleryPackageUrl, _emailConfiguration.AnnouncementsUrl, _emailConfiguration.TwitterUrl);
+            _coreMessageService.SendPackageValidationFailedNotice(package, validationSet, galleryPackageUrl, packageSupportUrl, _emailConfiguration.AnnouncementsUrl, _emailConfiguration.TwitterUrl);
         }
 
         public void SendValidationTakingTooLongMessage(Package package)

src/NuGet.Services.Validation.Orchestrator/ValidationOutcomeProcessor.cs

@@ -72,22 +72,7 @@ public async Task ProcessValidationOutcomeAsync(PackageValidationSet validationS
                 if (validatingEntity.Status == PackageStatus.Validating)
                 {
                     await _packageStateProcessor.SetStatusAsync(validatingEntity, validationSet, PackageStatus.FailedValidation);
-
-                    var issuesExistAndAllPackageSigned = validationSet
-                        .PackageValidations
-                        .SelectMany(pv => pv.PackageValidationIssues)
-                        .Select(pvi => pvi.IssueCode == ValidationIssueCode.PackageIsSigned)
-                        .DefaultIfEmpty(false)
-                        .All(v => v);
-
-                    if (issuesExistAndAllPackageSigned)
-                    {
-                        _messageService.SendSignedValidationFailedMessage(validatingEntity.EntityRecord);
-                    }
-                    else
-                    {
-                        _messageService.SendValidationFailedMessage(validatingEntity.EntityRecord);
-                    }
+                    _messageService.SendValidationFailedMessage(validatingEntity.EntityRecord, validationSet);
                 }
                 else
                 {

src/Validation.Common.Job/Validation.Common.Job.csproj

@@ -108,7 +108,7 @@
       <Version>2.26.0-master-33404</Version>
     </PackageReference>
     <PackageReference Include="NuGetGallery.Core">
-      <Version>4.4.4-dev-33346</Version>
+      <Version>4.4.5-dev-34153</Version>
     </PackageReference>
     <PackageReference Include="Serilog">
       <Version>2.5.0</Version>

src/Validation.Common.Job/Validation.Common.Job.nuspec

@@ -21,7 +21,7 @@
         <dependency id="NuGet.Services.Sql" version="2.26.0-master-33196" />
         <dependency id="NuGet.Services.Storage" version="2.25.0" />
         <dependency id="NuGet.Services.Validation" version="2.26.0-master-33404" />
-        <dependency id="NuGetGallery.Core" version="4.4.4-dev-33346" />
+        <dependency id="NuGetGallery.Core" version="4.4.5-dev-34153" />
         <dependency id="Serilog" version="2.5.0" />
         <dependency id="System.Net.Http" version="4.3.3" />
     </dependencies>

tests/NuGet.Services.Validation.Orchestrator.Tests/MessageServiceFacts.cs

@@ -52,8 +52,8 @@ public void ConstructorThrowsWhenEmailConfigurationValueIsNull()
         private const string ValidSettingsUrl = "https://example.com";
         public static IEnumerable<object[]> EmailConfigurationPropertyValuesCombinations =>
             (from invalidValue in InvalidValuesToTest
-            select new [] 
-            {
+             select new[]
+             {
                 new object[] { invalidValue,   ValidValue, ValidSettingsUrl, "PackageUrlTemplate" },
                 new object[] {   ValidValue, invalidValue, ValidSettingsUrl, "PackageSupportTemplate" },
                 new object[] {   ValidValue,   ValidValue,     invalidValue, "EmailSettingsUrl" }
@@ -132,30 +132,40 @@ public void SendPackageValidationFailedMessageCallsCoreMessageService()
 
             var service = new PackageMessageService(CoreMessageServiceMock.Object, EmailConfigurationAccessorMock.Object, LoggerMock.Object);
 
-            var ex = Record.Exception(() => service.SendValidationFailedMessage(Package));
+            var ex = Record.Exception(() => service.SendValidationFailedMessage(Package, ValidationSet));
             Assert.Null(ex);
 
             CoreMessageServiceMock
-                .Verify(cms => cms.SendPackageValidationFailedNotice(Package, expectedPackageUrl, expectedSupportUrl), Times.Once());
+                .Verify(cms => cms.SendPackageValidationFailedNotice(Package, ValidationSet, expectedPackageUrl, expectedSupportUrl, EmailConfiguration.AnnouncementsUrl, EmailConfiguration.TwitterUrl), Times.Once());
             CoreMessageServiceMock
-                .Verify(cms => cms.SendPackageValidationFailedNotice(It.IsAny<Package>(), It.IsAny<string>(), It.IsAny<string>()), Times.Once());
+                .Verify(cms => cms.SendPackageValidationFailedNotice(It.IsAny<Package>(), It.IsAny<PackageValidationSet>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), Times.Once());
         }
 
         [Fact]
         public void SendPackageValidationFailedMessageThrowsWhenPackageIsNull()
         {
             var service = new PackageMessageService(CoreMessageServiceMock.Object, EmailConfigurationAccessorMock.Object, LoggerMock.Object);
-            var ex = Assert.Throws<ArgumentNullException>(() => service.SendValidationFailedMessage(null));
+            var ex = Assert.Throws<ArgumentNullException>(() => service.SendValidationFailedMessage(null, new PackageValidationSet()));
             Assert.Equal("package", ex.ParamName);
         }
 
+        [Fact]
+        public void SendPackageValidationFailedMessageThrowsWhenValidationSetIsNull()
+        {
+            var service = new PackageMessageService(CoreMessageServiceMock.Object, EmailConfigurationAccessorMock.Object, LoggerMock.Object);
+            var ex = Assert.Throws<ArgumentNullException>(() => service.SendValidationFailedMessage(new Package(), null));
+            Assert.Equal("validationSet", ex.ParamName);
+        }
+
         public MessageServiceFacts()
         {
             EmailConfiguration = new EmailConfiguration
             {
                 PackageUrlTemplate = "https://example.com/package/{0}/{1}",
                 PackageSupportTemplate = "https://example.com/packageSupport/{0}/{1}",
-                EmailSettingsUrl = ValidSettingsUrl
+                EmailSettingsUrl = ValidSettingsUrl,
+                AnnouncementsUrl = "https://announcements.com",
+                TwitterUrl = "https://twitter.com/nuget"
             };
 
             EmailConfigurationAccessorMock
@@ -169,12 +179,15 @@ public MessageServiceFacts()
                 NormalizedVersion = "1.2.3"
             };
             Package.PackageRegistration.Packages = new List<Package> { Package };
+
+            ValidationSet = new PackageValidationSet();
         }
 
         private Mock<ICoreMessageService> CoreMessageServiceMock { get; set; } = new Mock<ICoreMessageService>();
         private Mock<IOptionsSnapshot<EmailConfiguration>> EmailConfigurationAccessorMock { get; set; } = new Mock<IOptionsSnapshot<EmailConfiguration>>();
         private Mock<ILogger<PackageMessageService>> LoggerMock { get; set; } = new Mock<ILogger<PackageMessageService>>();
         private Package Package { get; set; }
+        private PackageValidationSet ValidationSet { get; set; }
         private EmailConfiguration EmailConfiguration { get; set; }
     }
 }

tests/NuGet.Services.Validation.Orchestrator.Tests/ValidationOutcomeProcessorFacts.cs

@@ -40,26 +40,9 @@ public async Task ProcessesFailedValidationAccordingToFailureBehavior(Validation
         [InlineData(new[] { ValidationIssueCode.PackageIsSigned, ValidationIssueCode.Unknown })]
         [InlineData(new[] { ValidationIssueCode.Unknown, ValidationIssueCode.Unknown })]
         [InlineData(new[] { ValidationIssueCode.Unknown, ValidationIssueCode.PackageIsSigned })]
-        public async Task SendsFailureEmailOnFailedValidation(ValidationIssueCode[] issueCodes)
-        {
-            AddValidation("validation1", ValidationStatus.Failed);
-            ValidationSet.PackageValidations.First().PackageValidationIssues = issueCodes
-                .Select(ic => new PackageValidationIssue { IssueCode = ic })
-                .ToList();
-
-            var processor = CreateProcessor();
-            await processor.ProcessValidationOutcomeAsync(ValidationSet, PackageValidatingEntity, ProcessorStats);
-
-            MessageServiceMock
-                .Verify(ms => ms.SendValidationFailedMessage(Package), Times.Once());
-            MessageServiceMock
-                .Verify(ms => ms.SendValidationFailedMessage(It.IsAny<Package>()), Times.Once());
-        }
-
-        [Theory]
         [InlineData(new[] { ValidationIssueCode.PackageIsSigned })]
         [InlineData(new[] { ValidationIssueCode.PackageIsSigned, ValidationIssueCode.PackageIsSigned })]
-        public async Task SendsPackageSignedFailureEmail(ValidationIssueCode[] issueCodes)
+        public async Task SendsFailureEmailOnFailedValidation(ValidationIssueCode[] issueCodes)
         {
             AddValidation("validation1", ValidationStatus.Failed);
             ValidationSet.PackageValidations.First().PackageValidationIssues = issueCodes
@@ -70,9 +53,9 @@ public async Task SendsPackageSignedFailureEmail(ValidationIssueCode[] issueCode
             await processor.ProcessValidationOutcomeAsync(ValidationSet, PackageValidatingEntity, ProcessorStats);
 
             MessageServiceMock
-                .Verify(ms => ms.SendSignedValidationFailedMessage(Package), Times.Once());
+                .Verify(ms => ms.SendValidationFailedMessage(Package, ValidationSet), Times.Once());
             MessageServiceMock
-                .Verify(ms => ms.SendSignedValidationFailedMessage(It.IsAny<Package>()), Times.Once());
+                .Verify(ms => ms.SendValidationFailedMessage(It.IsAny<Package>(), It.IsAny<PackageValidationSet>()), Times.Once());
         }
 
         [Fact]
@@ -484,10 +467,7 @@ public async Task DoesNotTakeDownAvailablePackages()
                 x => x.SetStatusAsync(It.IsAny<PackageValidatingEntity>(), It.IsAny<PackageValidationSet>(), It.IsAny<PackageStatus>()),
                 Times.Never);
             MessageServiceMock.Verify(
-                x => x.SendSignedValidationFailedMessage(It.IsAny<Package>()),
-                Times.Never);
-            MessageServiceMock.Verify(
-                x => x.SendValidationFailedMessage(It.IsAny<Package>()),
+                x => x.SendValidationFailedMessage(It.IsAny<Package>(), It.IsAny<PackageValidationSet>()),
                 Times.Never);
             MessageServiceMock.Verify(
                 x => x.SendPublishedMessage(It.IsAny<Package>()),