From 9eb796515edde6b324eafa112d21cb5854330e37 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 12:58:50 -0700 Subject: [PATCH 01/15] Move package query logic to AdminControllerBase --- .../Admin/Controllers/AdminControllerBase.cs | 70 +++++++ .../Admin/Controllers/DeleteController.cs | 51 +----- .../Controllers/AdminControllerBaseFacts.cs | 173 ++++++++++++++++++ .../NuGetGallery.Facts.csproj | 1 + 4 files changed, 246 insertions(+), 49 deletions(-) create mode 100644 tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index 8ec479a7db..fed5e77542 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -1,6 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Collections.Generic; +using System.Linq; +using NuGet.Services.Entities; +using NuGet.Versioning; using NuGetGallery.Filters; namespace NuGetGallery.Areas.Admin.Controllers @@ -8,5 +13,70 @@ namespace NuGetGallery.Areas.Admin.Controllers [UIAuthorize(Roles="Admins")] public class AdminControllerBase : AppController { + internal List SearchForPackages(IPackageService packageService, string query) + { + // Search suports several options: + // 1) Full package id (e.g. jQuery) + // 2) Full package id + version (e.g. jQuery 1.9.0, jQuery/1.9.0) + // 3) Any of the above separated by comma + // We are not using Lucene index here as we want to have the database values. + + var queryParts = query.Split(new[] { ',', '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); + + var packages = new List(); + var completedQueries = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var queryPart in queryParts) + { + var spitQuery = queryPart.Split(new[] { ' ', '/' }, StringSplitOptions.RemoveEmptyEntries); + if (spitQuery.Length == 1) + { + // Don't make the same query twice. + var id = spitQuery[0].Trim(); + if (!completedQueries.Add(id)) + { + continue; + } + + var resultingRegistration = packageService.FindPackageRegistrationById(id); + if (resultingRegistration != null) + { + packages.AddRange(resultingRegistration + .Packages + .OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); + } + } + else if (spitQuery.Length == 2) + { + // Don't make the same query twice. + var id = spitQuery[0].Trim(); + var version = spitQuery[1].Trim(); + if (!completedQueries.Add(id + "/" + version)) + { + continue; + } + + var resultingPackage = packageService.FindPackageByIdAndVersionStrict(id, version); + if (resultingPackage != null) + { + packages.Add(resultingPackage); + } + } + } + + // Ensure only unique package instances are returned. + var uniquePackagesKeys = new HashSet(); + var uniquePackages = new List(); + foreach (var package in packages) + { + if (!uniquePackagesKeys.Add(package.Key)) + { + continue; + } + + uniquePackages.Add(package); + } + + return uniquePackages; + } } } \ No newline at end of file diff --git a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs index 63408679a9..8d262b5408 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs @@ -12,14 +12,12 @@ namespace NuGetGallery.Areas.Admin.Controllers { - public partial class DeleteController : AdminControllerBase + public class DeleteController : AdminControllerBase { private readonly IPackageService _packageService; private readonly IPackageDeleteService _packageDeleteService; private readonly ITelemetryService _telemetryService; - protected DeleteController() { } - public DeleteController( IPackageService packageService, IPackageDeleteService packageDeleteService, @@ -50,55 +48,10 @@ public virtual ActionResult Index() [HttpGet] public virtual ActionResult Search(string query) { - // Search suports several options: - // 1) Full package id (e.g. jQuery) - // 2) Full package id + version (e.g. jQuery 1.9.0) - // 3) Any of the above separated by comma - // We are not using Lucene index here as we want to have the database values. - - var queryParts = query.Split(new[] { ',', '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries); - - var packages = new List(); - var completedQueryParts = new HashSet(StringComparer.OrdinalIgnoreCase); - foreach (var queryPart in queryParts) - { - // Don't make the same query twice. - if (!completedQueryParts.Add(queryPart.Trim())) - { - continue; - } - - var splitQueryPart = queryPart.Split(new[] {' '}, StringSplitOptions.RemoveEmptyEntries); - if (splitQueryPart.Length == 1) - { - var resultingRegistration = _packageService.FindPackageRegistrationById(splitQueryPart[0].Trim()); - if (resultingRegistration != null) - { - packages.AddRange(resultingRegistration - .Packages - .OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); - } - } - else if (splitQueryPart.Length == 2) - { - var resultingPackage = _packageService.FindPackageByIdAndVersionStrict(splitQueryPart[0].Trim(), splitQueryPart[1].Trim()); - if (resultingPackage != null) - { - packages.Add(resultingPackage); - } - } - } - - // Filter out duplicate packages and create the view model. - var uniquePackagesKeys = new HashSet(); + var packages = SearchForPackages(_packageService, query); var results = new List(); foreach (var package in packages) { - if (!uniquePackagesKeys.Add(package.Key)) - { - continue; - } - results.Add(CreateDeleteSearchResult(package)); } diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs new file mode 100644 index 0000000000..e9cf51a1d4 --- /dev/null +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs @@ -0,0 +1,173 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Linq; +using Moq; +using NuGet.Services.Entities; +using Xunit; + +namespace NuGetGallery.Areas.Admin.Controllers +{ + public class AdminControllerBaseFacts + { + public class TheSearchForPackagesMethod : FactsBase + { + [Fact] + public void DoesNotMakeDuplicateIdQueries() + { + // Arrange + Query = "NuGet.Versioning" + Environment.NewLine + " NUGET.VERSIONING "; + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + PackageService.Verify( + x => x.FindPackageRegistrationById("NuGet.Versioning"), + Times.Once); + } + + [Fact] + public void DoesNotMakeDuplicateVersionQueries() + { + // Arrange + Query = "NuGet.Versioning 4.3.0" + Environment.NewLine + " NUGET.VERSIONING 4.3.0 "; + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.3.0"), + Times.Once); + } + + [Fact] + public void CanSearchForSpecificVersionSplitBySlash() + { + // Arrange + Query = "NuGet.Versioning/4.3.0\nNuGet.VERSIONING/4.4.0"; + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + Assert.Equal(2, result.Count); + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.3.0"), + Times.Once); + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.VERSIONING", "4.4.0"), + Times.Once); + } + + [Fact] + public void SplitsQueriesByComma() + { + // Arrange + Query = "NuGet.Versioning/4.3.0,NuGet.Versioning/4.4.0"; + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + Assert.Equal(2, result.Count); + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.3.0"), + Times.Once); + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.4.0"), + Times.Once); + } + + [Fact] + public void DoesNotReturnDuplicatePackages() + { + // Arrange + Packages.Add(Packages[0]); + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + Assert.Equal(2, result.Count); + Assert.Equal("4.3.0", result[0].NormalizedVersion); + Assert.Equal("4.4.0", result[1].NormalizedVersion); + } + + [Fact] + public void UsesVersionSpecificIdIfAvailable() + { + // Arrange + Packages[0].Id = "nuget.versioning"; + + // Act + var result = Target.SearchForPackages(PackageService.Object, Query); + + // Assert + Assert.Equal("NuGet.Versioning", result[0].PackageRegistration.Id); + Assert.Equal("4.3.0", result[0].NormalizedVersion); + Assert.Equal("NuGet.Versioning", result[1].PackageRegistration.Id); + Assert.Equal("4.4.0", result[1].NormalizedVersion); + } + } + + public class FactsBase + { + public FactsBase() + { + PackageService = new Mock(); + + Query = "NuGet.Versioning"; + var packageRegistration = new PackageRegistration + { + Id = "NuGet.Versioning", + Owners = new[] + { + new User { Username = "microsoft" }, + new User { Username = "nuget" }, + } + }; + Packages = new List() + { + new Package + { + Key = 2, + PackageRegistration = packageRegistration, + NormalizedVersion = "4.4.0", + }, + new Package + { + Key = 1, + PackageRegistration = packageRegistration, + NormalizedVersion = "4.3.0", + }, + }; + + PackageService + .Setup(x => x.FindPackageRegistrationById(It.IsAny())) + .Returns(x => new PackageRegistration + { + Packages = Packages.Where(y => y.PackageRegistration.Id.Equals(x, StringComparison.OrdinalIgnoreCase)).ToList(), + }); + + PackageService + .Setup(x => x.FindPackageByIdAndVersionStrict(It.IsAny(), It.IsAny())) + .Returns((i, v) => Packages + .Where(x => + x.PackageRegistration.Id.Equals(i, StringComparison.OrdinalIgnoreCase) && + x.NormalizedVersion.Equals(NuGetVersionFormatter.Normalize(v), StringComparison.OrdinalIgnoreCase)) + .FirstOrDefault()); + + Target = new AdminControllerBase(); + } + + public Mock PackageService { get; } + public string Query { get; set; } + public List Packages { get; } + public AdminControllerBase Target { get; } + } + } +} diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index 5a13c85526..dea8fb8720 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -63,6 +63,7 @@ + From b7b6ae1ed9c693f46aaaf1ac225632e0520c3967 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 13:19:56 -0700 Subject: [PATCH 02/15] Refactor DeleteSearchResult to PackageSearchResult --- .../Admin/Controllers/AdminControllerBase.cs | 27 ++++++++++ .../Admin/Controllers/DeleteController.cs | 53 +++++++++---------- ...SearchResult.cs => PackageSearchResult.cs} | 2 +- src/NuGetGallery/NuGetGallery.csproj | 2 +- .../Controllers/AdminControllerBaseFacts.cs | 46 +++++++++++++++- .../Controllers/DeleteControllerFacts.cs | 4 +- 6 files changed, 101 insertions(+), 33 deletions(-) rename src/NuGetGallery/Areas/Admin/ViewModels/{DeleteSearchResult.cs => PackageSearchResult.cs} (94%) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index fed5e77542..b7a3be4054 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -6,6 +6,7 @@ using System.Linq; using NuGet.Services.Entities; using NuGet.Versioning; +using NuGetGallery.Areas.Admin.ViewModels; using NuGetGallery.Filters; namespace NuGetGallery.Areas.Admin.Controllers @@ -78,5 +79,31 @@ internal List SearchForPackages(IPackageService packageService, string return uniquePackages; } + + internal PackageSearchResult CreatePackageSearchResult(Package package) + { + return new PackageSearchResult + { + PackageId = package.Id, + PackageVersionNormalized = !string.IsNullOrEmpty(package.NormalizedVersion) + ? package.NormalizedVersion + : NuGetVersion.Parse(package.Version).ToNormalizedString(), + DownloadCount = package.DownloadCount, + Created = package.Created.ToNuGetShortDateString(), + Listed = package.Listed, + PackageStatus = package.PackageStatusKey.ToString(), + Owners = package + .PackageRegistration + .Owners + .Select(u => u.Username) + .OrderBy(u => u, StringComparer.OrdinalIgnoreCase) + .Select(username => new UserViewModel + { + Username = username, + ProfileUrl = Url.User(username), + }) + .ToList() + }; + } } } \ No newline at end of file diff --git a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs index 8d262b5408..693a20d390 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs @@ -12,6 +12,29 @@ namespace NuGetGallery.Areas.Admin.Controllers { + public class UnlistController : AdminControllerBase + { + private readonly IPackageService _packageService; + + public UnlistController(IPackageService packageService) + { + _packageService = packageService; + } + + [HttpGet] + public virtual ActionResult Search(string query) + { + var packages = SearchForPackages(_packageService, query); + var results = new List(); + foreach (var package in packages) + { + results.Add(CreatePackageSearchResult(package)); + } + + return Json(results, JsonRequestBehavior.AllowGet); + } + } + public class DeleteController : AdminControllerBase { private readonly IPackageService _packageService; @@ -49,41 +72,15 @@ public virtual ActionResult Index() public virtual ActionResult Search(string query) { var packages = SearchForPackages(_packageService, query); - var results = new List(); + var results = new List(); foreach (var package in packages) { - results.Add(CreateDeleteSearchResult(package)); + results.Add(CreatePackageSearchResult(package)); } return Json(results, JsonRequestBehavior.AllowGet); } - private DeleteSearchResult CreateDeleteSearchResult(Package package) - { - return new DeleteSearchResult - { - PackageId = package.Id, - PackageVersionNormalized = !string.IsNullOrEmpty(package.NormalizedVersion) - ? package.NormalizedVersion - : NuGetVersion.Parse(package.Version).ToNormalizedString(), - DownloadCount = package.DownloadCount, - Created = package.Created.ToNuGetShortDateString(), - Listed = package.Listed, - PackageStatus = package.PackageStatusKey.ToString(), - Owners = package - .PackageRegistration - .Owners - .Select(u => u.Username) - .OrderBy(u => u) - .Select(username => new UserViewModel - { - Username = username, - ProfileUrl = Url.User(username), - }) - .ToList() - }; - } - [HttpGet] public virtual ActionResult Reflow() { diff --git a/src/NuGetGallery/Areas/Admin/ViewModels/DeleteSearchResult.cs b/src/NuGetGallery/Areas/Admin/ViewModels/PackageSearchResult.cs similarity index 94% rename from src/NuGetGallery/Areas/Admin/ViewModels/DeleteSearchResult.cs rename to src/NuGetGallery/Areas/Admin/ViewModels/PackageSearchResult.cs index f9f6eeb573..4ca15c87f3 100644 --- a/src/NuGetGallery/Areas/Admin/ViewModels/DeleteSearchResult.cs +++ b/src/NuGetGallery/Areas/Admin/ViewModels/PackageSearchResult.cs @@ -5,7 +5,7 @@ namespace NuGetGallery.Areas.Admin.ViewModels { - public class DeleteSearchResult + public class PackageSearchResult { public string PackageId { get; set; } public string PackageVersionNormalized { get; set; } diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index fc70273b6d..5dad1a6813 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -195,7 +195,7 @@ - + diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs index e9cf51a1d4..238f992c36 100644 --- a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/AdminControllerBaseFacts.cs @@ -4,14 +4,55 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Web; using Moq; using NuGet.Services.Entities; +using NuGetGallery.Framework; using Xunit; namespace NuGetGallery.Areas.Admin.Controllers { public class AdminControllerBaseFacts { + public class TheCreatePackageSearchResultMethod : FactsBase + { + [Fact] + public void MapsExpectedProperties() + { + var package = new Package + { + Id = "Newtonsoft.Json", + NormalizedVersion = "9.0.1", + DownloadCount = 1337, + Created = new DateTime(2021, 7, 6, 13, 5, 30), + Listed = true, + PackageStatusKey = PackageStatus.Available, + PackageRegistration = new PackageRegistration + { + Owners = new List + { + new User { Username = "gates" }, + new User { Username = "bill" }, + }, + } + }; + + var result = Target.CreatePackageSearchResult(package); + + Assert.Equal("Newtonsoft.Json", result.PackageId); + Assert.Equal("9.0.1", result.PackageVersionNormalized); + Assert.Equal(1337, result.DownloadCount); + Assert.Equal(package.Created.ToNuGetShortDateString(), result.Created); + Assert.True(result.Listed); + Assert.Equal("Available", result.PackageStatus); + Assert.Equal(2, result.Owners.Count); + Assert.Equal("bill", result.Owners[0].Username); + Assert.Equal("/profiles/bill", result.Owners[0].ProfileUrl); + Assert.Equal("gates", result.Owners[1].Username); + Assert.Equal("/profiles/gates", result.Owners[1].ProfileUrl); + } + } + public class TheSearchForPackagesMethod : FactsBase { [Fact] @@ -114,7 +155,7 @@ public void UsesVersionSpecificIdIfAvailable() } } - public class FactsBase + public class FactsBase : TestContainer { public FactsBase() { @@ -162,6 +203,9 @@ public FactsBase() .FirstOrDefault()); Target = new AdminControllerBase(); + + var httpContextBase = new Mock(); + TestUtility.SetupHttpContextMockForUrlGeneration(httpContextBase, Target); } public Mock PackageService { get; } diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/DeleteControllerFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/DeleteControllerFacts.cs index 572ed8e00c..023fc8e54b 100644 --- a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/DeleteControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/DeleteControllerFacts.cs @@ -44,7 +44,7 @@ public void DoesNotReturnDuplicatePackages() // Assert var jsonResult = Assert.IsType(result); - var searchResults = Assert.IsType>(jsonResult.Data); + var searchResults = Assert.IsType>(jsonResult.Data); var uniqueIdentities = searchResults.Select(p => $"{p.PackageId} {p.PackageVersionNormalized}").Distinct(); Assert.Equal(searchResults.Count, uniqueIdentities.Count()); } @@ -60,7 +60,7 @@ public void UsesVersionSpecificIdIfAvailable() // Assert var jsonResult = Assert.IsType(result); - var searchResults = Assert.IsType>(jsonResult.Data); + var searchResults = Assert.IsType>(jsonResult.Data); Assert.Equal("NuGet.Versioning", searchResults[0].PackageId); Assert.Equal("4.3.0", searchResults[0].PackageVersionNormalized); Assert.Equal("nuget.versioning", searchResults[1].PackageId); From ab061c7b6f303cdfe225af7c285177786a1b105d Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 17:04:07 -0700 Subject: [PATCH 03/15] Add bulk listed update service using same logic as deprecation --- .../Gallery/ThrowingTelemetryService.cs | 5 ++ .../IPackageUpdateService.cs | 9 +++ .../PackageManagement/PackageUpdateService.cs | 67 ++++++++++++++++++- .../Telemetry/ITelemetryService.cs | 2 + .../Telemetry/TelemetryService.cs | 15 +++++ .../Fakes/FakeTelemetryService.cs | 5 ++ 6 files changed, 101 insertions(+), 2 deletions(-) diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs index 9d95eb5a21..2da9bde11f 100644 --- a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs +++ b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs @@ -291,6 +291,11 @@ public void TrackPackageRevalidate(Package package) throw new NotImplementedException(); } + public void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed) + { + throw new NotImplementedException(); + } + public void TrackPackageUnlisted(Package package) { throw new NotImplementedException(); diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs index dd1e3552a2..f151ed9d3b 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs @@ -17,6 +17,15 @@ public interface IPackageUpdateService /// If true, will be called. Task MarkPackageUnlistedAsync(Package package, bool commitChanges = true, bool updateIndex = true); + /// + /// Updates the listed status on a batch of packages. All of the packages must be related to the same package registration. + /// Packages that are deleted or have failed validation are not allowed. Packages that already have a matching listed state + /// will be ignored. + /// + /// The packages to update. + /// True to make the packages listed, false to make the packages unlisted. + Task UpdateListedInBulkAsync(IReadOnlyList packages, bool listed); + /// /// Marks as listed. /// diff --git a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs index 78bde7c665..b258453829 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs @@ -32,6 +32,69 @@ public PackageUpdateService( _indexingService = indexingService ?? throw new ArgumentNullException(nameof(indexingService)); } + public async Task UpdateListedInBulkAsync(IReadOnlyList packages, bool listed) + { + if (packages == null || !packages.Any()) + { + throw new ArgumentException("At least one package must be provided."); + } + + foreach (var package in packages) + { + if (package.PackageStatusKey == PackageStatus.Deleted) + { + throw new ArgumentException("A deleted package cannot have its listed status changed."); + } + + if (package.PackageStatusKey == PackageStatus.FailedValidation) + { + throw new ArgumentException("A package that failed validation cannot have its listed status changed."); + } + } + + var registration = packages.First().PackageRegistration; + if (packages.Select(p => p.PackageRegistrationKey).Distinct().Count() > 1) + { + throw new ArgumentException("All packages to change the listing status of must have the same ID.", nameof(packages)); + } + + using (var strategy = new SuspendDbExecutionStrategy()) + using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) + { + var updatedPackages = new List(); + foreach (var package in packages) + { + if (package.Listed != listed) + { + package.Listed = listed; + updatedPackages.Add(package); + } + } + + if (updatedPackages.Any()) + { + await _packageService.UpdateIsLatestAsync(registration, commitChanges: false); + + await _entitiesContext.SaveChangesAsync(); + + await UpdatePackagesAsync(updatedPackages); + + transaction.Commit(); + + _telemetryService.TrackPackagesUpdateListed(updatedPackages, listed); + + foreach (var package in updatedPackages) + { + await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord( + package, + listed ? AuditedPackageAction.List : AuditedPackageAction.Unlist)); + + _indexingService.UpdatePackage(package); + } + } + } + } + public async Task MarkPackageListedAsync(Package package, bool commitChanges = true, bool updateIndex = true) { if (package == null) @@ -46,12 +109,12 @@ public async Task MarkPackageListedAsync(Package package, bool commitChanges = t if (package.PackageStatusKey == PackageStatus.Deleted) { - throw new InvalidOperationException("A deleted package should never be listed!"); + throw new InvalidOperationException("A deleted package should never be listed."); } if (package.PackageStatusKey == PackageStatus.FailedValidation) { - throw new InvalidOperationException("A package that failed validation should never be listed!"); + throw new InvalidOperationException("A package that failed validation should never be listed."); } package.Listed = true; diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs index fafe720fdb..10cb497de0 100644 --- a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs @@ -38,6 +38,8 @@ public interface ITelemetryService void TrackPackageListed(Package package); + void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed); + void TrackPackageDelete(Package package, bool isHardDelete); void TrackPackageReupload(Package package); diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs index beafc5edde..1ae855cae6 100644 --- a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs @@ -46,6 +46,7 @@ public class Events public const string PackageReflow = "PackageReflow"; public const string PackageUnlisted = "PackageUnlisted"; public const string PackageListed = "PackageListed"; + public const string PackageUpdateListed = "PackageUpdateListed"; public const string PackageDelete = "PackageDelete"; public const string PackageDeprecate = "PackageDeprecate"; public const string PackageReupload = "PackageReupload"; @@ -131,6 +132,9 @@ public class Events public const string PackageVersion = "PackageVersion"; public const string PackageVersions = "PackageVersions"; + // Package listed properties + public const string Listed = "Listed"; + // Package deprecate properties public const string DeprecationReason = "PackageDeprecationReason"; public const string DeprecationAlternatePackageId = "PackageDeprecationAlternatePackageId"; @@ -460,6 +464,17 @@ public void TrackPackageListed(Package package) TrackMetricForPackage(Events.PackageListed, package); } + public void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed) + { + TrackMetricForPackageVersions( + Events.PackageUpdateListed, + packages, + properties => + { + properties.Add(Listed, listed.ToString()); + }); + } + public void TrackPackageDelete(Package package, bool isHardDelete) { TrackMetricForPackage(Events.PackageDelete, package, properties => diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs index b729fcda84..043379f89b 100644 --- a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs +++ b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs @@ -288,6 +288,11 @@ public void TrackPackageRevalidate(Package package) throw new NotImplementedException(); } + public void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed) + { + throw new NotImplementedException(); + } + public void TrackPackageUnlisted(Package package) { throw new NotImplementedException(); From 0cd423ef248654eb3dd81f12b1552fb16b4ee89f Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 17:04:45 -0700 Subject: [PATCH 04/15] Add UpdateListedController --- .../Admin/Controllers/AdminControllerBase.cs | 8 +- .../Admin/Controllers/DeleteController.cs | 23 --- .../Controllers/UpdateListedController.cs | 91 ++++++++++++ .../Admin/Views/UpdateListed/Index.cshtml | 140 ++++++++++++++++++ src/NuGetGallery/NuGetGallery.csproj | 5 + .../RequestModels/UpdateListedRequest.cs | 21 +++ 6 files changed, 258 insertions(+), 30 deletions(-) create mode 100644 src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs create mode 100644 src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml create mode 100644 src/NuGetGallery/RequestModels/UpdateListedRequest.cs diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index b7a3be4054..40ecc331d5 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -38,13 +38,7 @@ internal List SearchForPackages(IPackageService packageService, string continue; } - var resultingRegistration = packageService.FindPackageRegistrationById(id); - if (resultingRegistration != null) - { - packages.AddRange(resultingRegistration - .Packages - .OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); - } + packages.AddRange(packageService.FindPackagesById(id).OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); } else if (spitQuery.Length == 2) { diff --git a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs index 693a20d390..e7f6f7105a 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/DeleteController.cs @@ -12,29 +12,6 @@ namespace NuGetGallery.Areas.Admin.Controllers { - public class UnlistController : AdminControllerBase - { - private readonly IPackageService _packageService; - - public UnlistController(IPackageService packageService) - { - _packageService = packageService; - } - - [HttpGet] - public virtual ActionResult Search(string query) - { - var packages = SearchForPackages(_packageService, query); - var results = new List(); - foreach (var package in packages) - { - results.Add(CreatePackageSearchResult(package)); - } - - return Json(results, JsonRequestBehavior.AllowGet); - } - } - public class DeleteController : AdminControllerBase { private readonly IPackageService _packageService; diff --git a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs new file mode 100644 index 0000000000..8c92b4c5e2 --- /dev/null +++ b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs @@ -0,0 +1,91 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using System.Web.Mvc; +using NuGet.Services.Entities; +using NuGetGallery.Areas.Admin.ViewModels; + +namespace NuGetGallery.Areas.Admin.Controllers +{ + public class UpdateListedController : AdminControllerBase + { + private readonly IPackageService _packageService; + private readonly IPackageUpdateService _packageUpdateService; + + public UpdateListedController( + IPackageService packageService, + IPackageUpdateService packageUpdateService) + { + _packageService = packageService; + _packageUpdateService = packageUpdateService; + } + + [HttpGet] + public virtual ActionResult Index() + { + return View(new UpdateListedRequest()); + } + + [HttpGet] + public virtual ActionResult Search(string query) + { + var packages = SearchForPackages(_packageService, query); + var results = new List(); + foreach (var package in packages) + { + results.Add(CreatePackageSearchResult(package)); + } + + return Json(results, JsonRequestBehavior.AllowGet); + } + + [HttpPost] + [ValidateAntiForgeryToken] + public async Task UpdateListed(UpdateListedRequest updateListed) + { + if (ModelState.IsValid) + { + var idToVersions = updateListed + .Packages + .Select(x => x.Split(new[] { '|' }, StringSplitOptions.RemoveEmptyEntries)) + .Where(x => x.Length == 2) + .GroupBy(x => x[0], x => x[1], StringComparer.OrdinalIgnoreCase); + + var packageRegistrationCount = 0; + var packageCount = 0; + var noOpCount = 0; + foreach (var group in idToVersions) + { + var normalizedVersions = group + .Select(x => NuGetVersionFormatter.Normalize(x)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + var packages = _packageService + .FindPackagesById(group.Key, PackageDeprecationFieldsToInclude.DeprecationAndRelationships) + .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) + .Where(x => x.Listed != updateListed.Listed) + .ToList(); + + packageRegistrationCount++; + packageCount += packages.Count; + noOpCount += normalizedVersions.Count - packages.Count; + + await _packageUpdateService.UpdateListedInBulkAsync(packages, updateListed.Listed); + } + + TempData["Message"] = $"{packageCount} packages across {packageRegistrationCount} package IDs have " + + $"been {(updateListed.Listed ? "relisted" : "unlisted")}. {noOpCount} packages were already " + + $"up-to-date and were left unchanged."; + return RedirectToAction(nameof(Index)); + } + else + { + TempData["Message"] = "The provided input is not valid."; + return RedirectToAction(nameof(Index)); + } + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml new file mode 100644 index 0000000000..b0f350bf5f --- /dev/null +++ b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml @@ -0,0 +1,140 @@ +@model NuGetGallery.UpdateListedRequest + +@{ + ViewBag.Title = "Unlist/relist packages"; +} + +
+

Unlist/relist packages

+ +
+
+ +
+ + @using (Html.BeginForm("UpdateListed", "UpdateListed", new { area = "Admin" }, FormMethod.Post, new { id = "update-listed-form" })) + { + @Html.AntiForgeryToken() + + + + + + + + + + + + + + + + + + + + + + + + + + +
+

No packages found.

+
+ + + } +
+ +@section BottomScripts { + +} \ No newline at end of file diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 5dad1a6813..70aec32b11 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -143,6 +143,7 @@ + Url_Edit.ascx @@ -310,6 +311,7 @@ + @@ -2319,6 +2321,9 @@ + + + 10.0 $(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion) diff --git a/src/NuGetGallery/RequestModels/UpdateListedRequest.cs b/src/NuGetGallery/RequestModels/UpdateListedRequest.cs new file mode 100644 index 0000000000..dfa9b0921d --- /dev/null +++ b/src/NuGetGallery/RequestModels/UpdateListedRequest.cs @@ -0,0 +1,21 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using NuGetGallery.Infrastructure; + +namespace NuGetGallery +{ + public class UpdateListedRequest + { + public UpdateListedRequest() + { + Packages = new List(); + } + + public List Packages { get; set; } + + public bool Listed { get; set; } + } +} From 977de8dbf7fb3b9bf711da865958fec760d9c3e5 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 17:16:17 -0700 Subject: [PATCH 05/15] Add the new link to admin home, fix a bug --- .../Controllers/UpdateListedController.cs | 9 ++++++-- .../Areas/Admin/Views/Home/Index.cshtml | 23 ++++++++++++++----- .../Admin/Views/UpdateListed/Index.cshtml | 2 +- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs index 8c92b4c5e2..8f11f23a99 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs @@ -67,13 +67,18 @@ public async Task UpdateListed(UpdateListedRequest updateListed) .FindPackagesById(group.Key, PackageDeprecationFieldsToInclude.DeprecationAndRelationships) .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) .Where(x => x.Listed != updateListed.Listed) + .Where(x => x.PackageStatusKey != PackageStatus.Deleted) + .Where(x => x.PackageStatusKey != PackageStatus.FailedValidation) .ToList(); - packageRegistrationCount++; packageCount += packages.Count; noOpCount += normalizedVersions.Count - packages.Count; - await _packageUpdateService.UpdateListedInBulkAsync(packages, updateListed.Listed); + if (packages.Any()) + { + packageRegistrationCount++; + await _packageUpdateService.UpdateListedInBulkAsync(packages, updateListed.Listed); + } } TempData["Message"] = $"{packageCount} packages across {packageRegistrationCount} package IDs have " + diff --git a/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml index 25b76c2377..007d82e0d9 100644 --- a/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml @@ -54,6 +54,17 @@ Delete packages from the gallery.

+
  • +

    + + + Unlist/relist Packages + +

    +

    + Unlist or relist many packages at once. +

    +
  • @@ -82,13 +93,13 @@ this, formId: "clear-content-cache-form", htmlContent: @ - - Clear Content Cache + + Clear Content Cache , - actionName: "ClearContentCache", - controllerName: "Home", - role: string.Empty, - area: "Admin") + actionName: "ClearContentCache", + controllerName: "Home", + role: string.Empty, + area: "Admin")

    Clear Content Cache diff --git a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml index b0f350bf5f..c9d557a52d 100644 --- a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml @@ -1,7 +1,7 @@ @model NuGetGallery.UpdateListedRequest @{ - ViewBag.Title = "Unlist/relist packages"; + ViewBag.Title = "Unlist/relist Packages"; }

    From fd1c9c3c1372012872a25d88fd88924066183bcc Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 17:47:57 -0700 Subject: [PATCH 06/15] Add tests to PackageUpdateServiceFacts and fix broken tests --- .../Admin/Controllers/AdminControllerBase.cs | 8 +- .../Services/PackageUpdateServiceFacts.cs | 127 ++++++++++++++++++ .../Services/TelemetryServiceFacts.cs | 4 + 3 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index 40ecc331d5..b7a3be4054 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -38,7 +38,13 @@ internal List SearchForPackages(IPackageService packageService, string continue; } - packages.AddRange(packageService.FindPackagesById(id).OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); + var resultingRegistration = packageService.FindPackageRegistrationById(id); + if (resultingRegistration != null) + { + packages.AddRange(resultingRegistration + .Packages + .OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); + } } else if (spitQuery.Length == 2) { diff --git a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs index d5b764982b..2ee93e7033 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs @@ -24,6 +24,133 @@ public enum PackageLatestState LatestStableSemVer2 } + + public class TheUpdateDeprecationMethod : TestContainer + { + public static IEnumerable ThrowsIfPackagesEmpty_Data => MemberDataHelper.AsDataSet(null, new Package[0]); + + [Theory] + [MemberData(nameof(ThrowsIfPackagesEmpty_Data))] + public async Task ThrowsIfPackagesEmpty(IReadOnlyList packages) + { + var service = Get(); + + var ex = await Assert.ThrowsAsync( + () => service.UpdateListedInBulkAsync(packages, listed: false)); + Assert.Equal("At least one package must be provided.", ex.Message); + } + + [Theory] + [InlineData(PackageStatus.Deleted, "A deleted package cannot have its listed status changed.")] + [InlineData(PackageStatus.FailedValidation, "A package that failed validation cannot have its listed status changed.")] + public async Task ThrowsIfPackageHasInvalidStatus(PackageStatus packageStatus, string message) + { + var service = Get(); + + var packages = new[] + { + new Package { PackageStatusKey = packageStatus }, + }; + + var ex = await Assert.ThrowsAsync( + () => service.UpdateListedInBulkAsync(packages, listed: true)); + Assert.Equal(message, ex.Message); + } + + [Fact] + public async Task ThrowsIfPackagesHaveDifferentRegistrations() + { + var service = Get(); + + var packages = new[] + { + new Package { PackageRegistrationKey = 1 }, + new Package { PackageRegistrationKey = 2 }, + }; + + var ex = await Assert.ThrowsAsync( + () => service.UpdateListedInBulkAsync(packages, listed: true)); + Assert.StartsWith("All packages to change the listing status of must have the same ID.", ex.Message); + } + + [Theory] + [InlineData(false, AuditedPackageAction.Unlist, "WHERE [Key] IN (20002)")] + [InlineData(true, AuditedPackageAction.List, "WHERE [Key] IN (10001)")] + public async Task UpdateListedStatus(bool listed, AuditedPackageAction action, string sqlWhere) + { + // Arrange + var id = "theId"; + var registration = new PackageRegistration { Id = id }; + var listedPackage = new Package + { + Key = 20002, + PackageRegistration = registration, + NormalizedVersion = "1.0.0", + Listed = true, + }; + + var unlistedPackage = new Package + { + Key = 10001, + PackageRegistration = registration, + NormalizedVersion = "2.0.0", + Listed = false, + }; + + var packages = new[] + { + listedPackage, + unlistedPackage, + }; + + var packageServiceMock = GetMock(); + packageServiceMock + .Setup(x => x.UpdateIsLatestAsync(registration, false)) + .Returns(Task.CompletedTask) + .Verifiable(); + + var transactionMock = new Mock(); + transactionMock + .Setup(x => x.Commit()) + .Verifiable(); + + var databaseMock = new Mock(); + databaseMock + .Setup(x => x.BeginTransaction()) + .Returns(transactionMock.Object); + databaseMock + .Setup(x => x.ExecuteSqlCommandAsync(It.Is(q => q.Contains(sqlWhere)), It.IsAny())) + .ReturnsAsync(2) + .Verifiable(); + + var context = GetFakeContext(); + context.SetupDatabase(databaseMock.Object); + + var auditingService = GetService(); + + var telemetryService = GetMock(); + telemetryService + .Setup(x => x.TrackPackagesUpdateListed(packages, listed)) + .Verifiable(); + + var service = Get(); + + // Act + await service.UpdateListedInBulkAsync(packages, listed); + + // Assert + packageServiceMock.Verify(); + context.VerifyCommitChanges(); + databaseMock.Verify(); + transactionMock.Verify(); + telemetryService.Verify(); + + Assert.Equal(listed, listedPackage.Listed); + Assert.Equal(listed, unlistedPackage.Listed); + auditingService.WroteRecord(r => r.Action == action); + } + } + public class TheMarkPackageListedMethod : TestContainer { [Fact] diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index 057eb2bd1c..5014a7bf02 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -112,6 +112,10 @@ public static IEnumerable TrackMetricNames_Data (TrackAction)(s => s.TrackPackageDelete(package, isHardDelete: true)) }; + yield return new object[] { "PackagesUpdateListed", + (TrackAction)(s => s.TrackPackagesUpdateListed(new[] { package }, listed: true)) + }; + yield return new object[] { "PackageReupload", (TrackAction)(s => s.TrackPackageReupload(package)) }; From ef6c0044ee197c57960b74168d96975951aedeaf Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 17:54:17 -0700 Subject: [PATCH 07/15] Use FindPackageByIdAndVersionStrict when there is one version --- .../Controllers/UpdateListedController.cs | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs index 8f11f23a99..abbd82d869 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs @@ -63,9 +63,28 @@ public async Task UpdateListed(UpdateListedRequest updateListed) var normalizedVersions = group .Select(x => NuGetVersionFormatter.Normalize(x)) .ToHashSet(StringComparer.OrdinalIgnoreCase); - var packages = _packageService - .FindPackagesById(group.Key, PackageDeprecationFieldsToInclude.DeprecationAndRelationships) - .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) + + List packages; + if (normalizedVersions.Count == 1) + { + packages = new List(); + var package = _packageService.FindPackageByIdAndVersionStrict(group.Key, normalizedVersions.First()); + if (package != null) + { + packages.Add(package); + } + } + else + { + // Include the deprecation information since it is used in the auditing event. + packages = _packageService.FindPackagesById( + group.Key, + PackageDeprecationFieldsToInclude.DeprecationAndRelationships) + .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) + .ToList(); + } + + packages = packages .Where(x => x.Listed != updateListed.Listed) .Where(x => x.PackageStatusKey != PackageStatus.Deleted) .Where(x => x.PackageStatusKey != PackageStatus.FailedValidation) From 659c7930d151207e3361908ffff187c9fbac83a7 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 18:45:05 -0700 Subject: [PATCH 08/15] Add unit tests for UpdateListedController --- .../Telemetry/TelemetryService.cs | 4 +- .../Controllers/UpdateListedController.cs | 102 ++++--- .../Admin/Views/UpdateListed/Index.cshtml | 4 +- .../UpdateListedControllerFacts.cs | 262 ++++++++++++++++++ .../NuGetGallery.Facts.csproj | 1 + 5 files changed, 313 insertions(+), 60 deletions(-) create mode 100644 tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs index 1ae855cae6..0252d411d7 100644 --- a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs @@ -46,7 +46,7 @@ public class Events public const string PackageReflow = "PackageReflow"; public const string PackageUnlisted = "PackageUnlisted"; public const string PackageListed = "PackageListed"; - public const string PackageUpdateListed = "PackageUpdateListed"; + public const string PackagesUpdateListed = "PackagesUpdateListed"; public const string PackageDelete = "PackageDelete"; public const string PackageDeprecate = "PackageDeprecate"; public const string PackageReupload = "PackageReupload"; @@ -467,7 +467,7 @@ public void TrackPackageListed(Package package) public void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed) { TrackMetricForPackageVersions( - Events.PackageUpdateListed, + Events.PackagesUpdateListed, packages, properties => { diff --git a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs index abbd82d869..1f07292cb3 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs @@ -27,11 +27,11 @@ public UpdateListedController( [HttpGet] public virtual ActionResult Index() { - return View(new UpdateListedRequest()); + return View(); } [HttpGet] - public virtual ActionResult Search(string query) + public ActionResult Search(string query) { var packages = SearchForPackages(_packageService, query); var results = new List(); @@ -47,69 +47,61 @@ public virtual ActionResult Search(string query) [ValidateAntiForgeryToken] public async Task UpdateListed(UpdateListedRequest updateListed) { - if (ModelState.IsValid) + var idToVersions = updateListed + .Packages + .Select(x => x.Split(new[] { '|' }, StringSplitOptions.RemoveEmptyEntries)) + .Where(x => x.Length == 2) + .GroupBy(x => x[0], x => x[1], StringComparer.OrdinalIgnoreCase); + + var packageRegistrationCount = 0; + var packageCount = 0; + var noOpCount = 0; + foreach (var group in idToVersions) { - var idToVersions = updateListed - .Packages - .Select(x => x.Split(new[] { '|' }, StringSplitOptions.RemoveEmptyEntries)) - .Where(x => x.Length == 2) - .GroupBy(x => x[0], x => x[1], StringComparer.OrdinalIgnoreCase); + var normalizedVersions = group + .Select(x => NuGetVersionFormatter.Normalize(x)) + .ToHashSet(StringComparer.OrdinalIgnoreCase); - var packageRegistrationCount = 0; - var packageCount = 0; - var noOpCount = 0; - foreach (var group in idToVersions) + List packages; + if (normalizedVersions.Count == 1) { - var normalizedVersions = group - .Select(x => NuGetVersionFormatter.Normalize(x)) - .ToHashSet(StringComparer.OrdinalIgnoreCase); - - List packages; - if (normalizedVersions.Count == 1) - { - packages = new List(); - var package = _packageService.FindPackageByIdAndVersionStrict(group.Key, normalizedVersions.First()); - if (package != null) - { - packages.Add(package); - } - } - else + packages = new List(); + var package = _packageService.FindPackageByIdAndVersionStrict(group.Key, normalizedVersions.First()); + if (package != null) { - // Include the deprecation information since it is used in the auditing event. - packages = _packageService.FindPackagesById( - group.Key, - PackageDeprecationFieldsToInclude.DeprecationAndRelationships) - .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) - .ToList(); + packages.Add(package); } - - packages = packages - .Where(x => x.Listed != updateListed.Listed) - .Where(x => x.PackageStatusKey != PackageStatus.Deleted) - .Where(x => x.PackageStatusKey != PackageStatus.FailedValidation) + } + else + { + // Include the deprecation information since it is used in the auditing event. + packages = _packageService.FindPackagesById( + group.Key, + PackageDeprecationFieldsToInclude.DeprecationAndRelationships) + .Where(x => normalizedVersions.Contains(x.NormalizedVersion)) .ToList(); + } - packageCount += packages.Count; - noOpCount += normalizedVersions.Count - packages.Count; + packages = packages + .Where(x => x.Listed != updateListed.Listed) + .Where(x => x.PackageStatusKey != PackageStatus.Deleted) + .Where(x => x.PackageStatusKey != PackageStatus.FailedValidation) + .ToList(); - if (packages.Any()) - { - packageRegistrationCount++; - await _packageUpdateService.UpdateListedInBulkAsync(packages, updateListed.Listed); - } - } + packageCount += packages.Count; + noOpCount += normalizedVersions.Count - packages.Count; - TempData["Message"] = $"{packageCount} packages across {packageRegistrationCount} package IDs have " + - $"been {(updateListed.Listed ? "relisted" : "unlisted")}. {noOpCount} packages were already " + - $"up-to-date and were left unchanged."; - return RedirectToAction(nameof(Index)); - } - else - { - TempData["Message"] = "The provided input is not valid."; - return RedirectToAction(nameof(Index)); + if (packages.Any()) + { + packageRegistrationCount++; + await _packageUpdateService.UpdateListedInBulkAsync(packages, updateListed.Listed); + } } + + TempData["Message"] = $"{packageCount} packages across {packageRegistrationCount} package IDs have " + + $"been {(updateListed.Listed ? "relisted" : "unlisted")}. {noOpCount} packages were already " + + $"up-to-date and were left unchanged."; + return RedirectToAction(nameof(Index)); } } } \ No newline at end of file diff --git a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml index c9d557a52d..9ec1d9b8c9 100644 --- a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml @@ -1,6 +1,4 @@ -@model NuGetGallery.UpdateListedRequest - -@{ +@{ ViewBag.Title = "Unlist/relist Packages"; } diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs new file mode 100644 index 0000000000..c760e2bd8f --- /dev/null +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs @@ -0,0 +1,262 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using System.Web; +using System.Web.Mvc; +using Moq; +using NuGet.Services.Entities; +using NuGetGallery.Areas.Admin.ViewModels; +using NuGetGallery.Framework; +using Xunit; + +namespace NuGetGallery.Areas.Admin.Controllers +{ + public class UpdateListedControllerFacts + { + public class TheSearchAction : FactsBase + { + [Fact] + public void QueriesForPackages() + { + // Arrange & Act + var result = Target.Search(PackageRegistrationA.Id); + + // Assert + var jsonResult = Assert.IsType(result); + var searchResults = Assert.IsType>(jsonResult.Data); + Assert.Equal(3, searchResults.Count); + Assert.Equal("4.3.0", searchResults[0].PackageVersionNormalized); + Assert.Equal("4.4.0", searchResults[1].PackageVersionNormalized); + Assert.Equal("4.5.0", searchResults[2].PackageVersionNormalized); + } + } + + public class TheUpdateListedAction : FactsBase + { + [Fact] + public async Task ProcessesPackagesGroupedById() + { + // Arrange + var input = new UpdateListedRequest + { + Listed = false, + Packages = new List + { + "NuGet.Versioning|9.9.9", // Does not exist + "NuGet.Versioning|4.3.0", + "NuGet.Versioning|4.4.0", + "NuGet.Frameworks|5.3.0", + "NuGet.Frameworks|5.4.0", + } + }; + + // Act + var result = await Target.UpdateListed(input); + + // Assert + PackageService.Verify( + x => x.FindPackagesById("NuGet.Versioning", PackageDeprecationFieldsToInclude.DeprecationAndRelationships), + Times.Once); + PackageService.Verify( + x => x.FindPackagesById("NuGet.Frameworks", PackageDeprecationFieldsToInclude.DeprecationAndRelationships), + Times.Once); + PackageUpdateService.Verify( + x => x.UpdateListedInBulkAsync(It.Is>(l => l.All(i => i.Id == "NuGet.Versioning")), false), + Times.Once); + PackageUpdateService.Verify( + x => x.UpdateListedInBulkAsync(It.Is>(l => l.All(i => i.Id == "NuGet.Frameworks")), false), + Times.Once); + Assert.Equal( + "4 packages across 2 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + Target.TempData["Message"]); + } + + [Theory] + [InlineData(PackageStatus.Deleted)] + [InlineData(PackageStatus.FailedValidation)] + public async Task FiltersOutWrongPackageStatus(PackageStatus packageStatus) + { + // Arrange + var input = new UpdateListedRequest + { + Listed = false, + Packages = new List + { + "NuGet.Versioning|4.4.0", + "NuGet.Versioning|4.3.0", + } + }; + PackageRegistrationA.Packages.Single(x => x.NormalizedVersion == "4.3.0").PackageStatusKey = packageStatus; + + // Act + var result = await Target.UpdateListed(input); + + // Assert + PackageService.Verify( + x => x.FindPackagesById("NuGet.Versioning", PackageDeprecationFieldsToInclude.DeprecationAndRelationships), + Times.Once); + PackageUpdateService.Verify( + x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), + Times.Once); + Assert.Equal( + "1 packages across 1 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + Target.TempData["Message"]); + } + + [Fact] + public async Task FiltersOutPackagesWithMatchingListed() + { + // Arrange + var input = new UpdateListedRequest + { + Listed = false, + Packages = new List + { + "NuGet.Versioning|4.4.0", + "NuGet.Versioning|4.3.0", + } + }; + PackageRegistrationA.Packages.Single(x => x.NormalizedVersion == "4.3.0").Listed = false; + + // Act + var result = await Target.UpdateListed(input); + + // Assert + PackageService.Verify( + x => x.FindPackagesById("NuGet.Versioning", PackageDeprecationFieldsToInclude.DeprecationAndRelationships), + Times.Once); + PackageUpdateService.Verify( + x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), + Times.Once); + Assert.Equal( + "1 packages across 1 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + Target.TempData["Message"]); + } + + [Fact] + public async Task UsesPointQueryForSingleVersion() + { + // Arrange + var input = new UpdateListedRequest + { + Listed = false, + Packages = new List + { + "NuGet.Versioning|4.4.0", + } + }; + PackageService + .Setup(x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.4.0")) + .Returns(() => PackageRegistrationA.Packages.Single(x => x.NormalizedVersion == "4.4.0")); + + // Act + var result = await Target.UpdateListed(input); + + // Assert + PackageService.Verify( + x => x.FindPackageByIdAndVersionStrict("NuGet.Versioning", "4.4.0"), + Times.Once); + PackageUpdateService.Verify( + x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), + Times.Once); + Assert.Equal( + "1 packages across 1 package IDs have been unlisted. 0 packages were already up-to-date and were left unchanged.", + Target.TempData["Message"]); + } + } + + public abstract class FactsBase : TestContainer + { + public FactsBase() + { + PackageService = new Mock(); + PackageUpdateService = new Mock(); + HttpContextBase = new Mock(); + + PackageRegistrationA = new PackageRegistration + { + Id = "NuGet.Versioning", + Owners = new[] + { + new User { Username = "microsoft" }, + new User { Username = "nuget" }, + }, + }; + PackageRegistrationA.Packages = new List + { + new Package + { + Key = 3, + NormalizedVersion = "4.5.0", + PackageRegistration = PackageRegistrationA, + }, + new Package + { + Key = 2, + NormalizedVersion = "4.4.0", + PackageRegistration = PackageRegistrationA, + }, + new Package + { + Key = 1, + NormalizedVersion = "4.3.0", + PackageRegistration = PackageRegistrationA, + }, + }; + PackageRegistrationB = new PackageRegistration + { + Id = "NuGet.Frameworks", + Owners = new[] + { + new User { Username = "nuget" }, + }, + }; + PackageRegistrationB.Packages = new List + { + new Package + { + Key = 6, + NormalizedVersion = "5.5.0", + PackageRegistration = PackageRegistrationB, + }, + new Package + { + Key = 5, + NormalizedVersion = "5.4.0", + PackageRegistration = PackageRegistrationB, + }, + new Package + { + Key = 4, + NormalizedVersion = "5.3.0", + PackageRegistration = PackageRegistrationB, + }, + }; + + PackageService + .Setup(x => x.FindPackageRegistrationById(PackageRegistrationA.Id)) + .Returns(() => PackageRegistrationA); + PackageService + .Setup(x => x.FindPackageRegistrationById(PackageRegistrationB.Id)) + .Returns(() => PackageRegistrationB); + PackageService + .Setup(x => x.FindPackagesById(PackageRegistrationA.Id, It.IsAny())) + .Returns(() => (IReadOnlyCollection)PackageRegistrationA.Packages); + PackageService + .Setup(x => x.FindPackagesById(PackageRegistrationB.Id, It.IsAny())) + .Returns(() => (IReadOnlyCollection)PackageRegistrationB.Packages); + + Target = new UpdateListedController(PackageService.Object, PackageUpdateService.Object); + TestUtility.SetupHttpContextMockForUrlGeneration(HttpContextBase, Target); + } + + public Mock PackageService { get; } + public Mock PackageUpdateService { get; } + public Mock HttpContextBase { get; } + public PackageRegistration PackageRegistrationA { get; } + public string Query { get; set; } + public PackageRegistration PackageRegistrationB { get; } + public UpdateListedController Target { get; } + } + } +} diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index dea8fb8720..fd54906141 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -70,6 +70,7 @@ + From e88023d83dbb4c792dbd7afe088250ed5a5c6ce6 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 16 Jul 2021 19:29:32 -0700 Subject: [PATCH 09/15] Fix unit test --- tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs index 2ee93e7033..053fc26a26 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs @@ -130,7 +130,7 @@ public async Task UpdateListedStatus(bool listed, AuditedPackageAction action, s var telemetryService = GetMock(); telemetryService - .Setup(x => x.TrackPackagesUpdateListed(packages, listed)) + .Setup(x => x.TrackPackagesUpdateListed(It.Is>(l => l.Count == 1), listed)) .Verifiable(); var service = Get(); From e3ba000f6a065d68b7628f350c7fb64346145843 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Tue, 20 Jul 2021 12:36:28 -0700 Subject: [PATCH 10/15] Update src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> --- .../Areas/Admin/Controllers/AdminControllerBase.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index b7a3be4054..af580ae783 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -16,7 +16,7 @@ public class AdminControllerBase : AppController { internal List SearchForPackages(IPackageService packageService, string query) { - // Search suports several options: + // Search supports several options: // 1) Full package id (e.g. jQuery) // 2) Full package id + version (e.g. jQuery 1.9.0, jQuery/1.9.0) // 3) Any of the above separated by comma @@ -106,4 +106,4 @@ internal PackageSearchResult CreatePackageSearchResult(Package package) }; } } -} \ No newline at end of file +} From 69570cd5584adf4f8ae3d2831101a94bbe1602fc Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Tue, 20 Jul 2021 12:37:49 -0700 Subject: [PATCH 11/15] Change tabs to spaces --- src/NuGetGallery.Services/Telemetry/ITelemetryService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs index 10cb497de0..130ac3a98b 100644 --- a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs @@ -38,7 +38,7 @@ public interface ITelemetryService void TrackPackageListed(Package package); - void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed); + void TrackPackagesUpdateListed(IReadOnlyList packages, bool listed); void TrackPackageDelete(Package package, bool isHardDelete); From 77fdda79ed2b38a829010e8eba5b651a8bc6a326 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Tue, 20 Jul 2021 20:08:02 -0700 Subject: [PATCH 12/15] Address offline comments to allow reflow --- .../PackageManagement/PackageUpdateService.cs | 32 +++++++------------ .../Controllers/UpdateListedController.cs | 6 ++-- .../Admin/Views/UpdateListed/Index.cshtml | 6 ++-- .../UpdateListedControllerFacts.cs | 12 +++---- .../Services/PackageUpdateServiceFacts.cs | 12 +++---- 5 files changed, 30 insertions(+), 38 deletions(-) diff --git a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs index b258453829..9208403d3c 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs @@ -61,36 +61,28 @@ public async Task UpdateListedInBulkAsync(IReadOnlyList packages, bool using (var strategy = new SuspendDbExecutionStrategy()) using (var transaction = _entitiesContext.GetDatabase().BeginTransaction()) { - var updatedPackages = new List(); foreach (var package in packages) { - if (package.Listed != listed) - { - package.Listed = listed; - updatedPackages.Add(package); - } + package.Listed = listed; } - if (updatedPackages.Any()) - { - await _packageService.UpdateIsLatestAsync(registration, commitChanges: false); + await _packageService.UpdateIsLatestAsync(registration, commitChanges: false); - await _entitiesContext.SaveChangesAsync(); + await _entitiesContext.SaveChangesAsync(); - await UpdatePackagesAsync(updatedPackages); + await UpdatePackagesAsync(packages); - transaction.Commit(); + transaction.Commit(); - _telemetryService.TrackPackagesUpdateListed(updatedPackages, listed); + _telemetryService.TrackPackagesUpdateListed(packages, listed); - foreach (var package in updatedPackages) - { - await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord( - package, - listed ? AuditedPackageAction.List : AuditedPackageAction.Unlist)); + foreach (var package in packages) + { + await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord( + package, + listed ? AuditedPackageAction.List : AuditedPackageAction.Unlist)); - _indexingService.UpdatePackage(package); - } + _indexingService.UpdatePackage(package); } } } diff --git a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs index 1f07292cb3..6f43a3cbe5 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/UpdateListedController.cs @@ -83,7 +83,6 @@ public async Task UpdateListed(UpdateListedRequest updateListed) } packages = packages - .Where(x => x.Listed != updateListed.Listed) .Where(x => x.PackageStatusKey != PackageStatus.Deleted) .Where(x => x.PackageStatusKey != PackageStatus.FailedValidation) .ToList(); @@ -99,8 +98,9 @@ public async Task UpdateListed(UpdateListedRequest updateListed) } TempData["Message"] = $"{packageCount} packages across {packageRegistrationCount} package IDs have " + - $"been {(updateListed.Listed ? "relisted" : "unlisted")}. {noOpCount} packages were already " + - $"up-to-date and were left unchanged."; + $"been {(updateListed.Listed ? "relisted" : "unlisted")}. {noOpCount} packages were skipped because " + + $"they are deleted or they failed validation."; + return RedirectToAction(nameof(Index)); } } diff --git a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml index 9ec1d9b8c9..68da575541 100644 --- a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml @@ -47,10 +47,10 @@ } @@ -84,7 +84,7 @@ $self.selectAllState(false); $self.searchResults.removeAll(); for (var i = 0; i < data.length; i++) { - data[i].Selected = ko.observable(false); + data[i].Selected = ko.observable(!!data[i].Listed); } $self.searchResults(data); } diff --git a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs index c760e2bd8f..eedd8f28db 100644 --- a/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Areas/Admin/Controllers/UpdateListedControllerFacts.cs @@ -67,7 +67,7 @@ public async Task ProcessesPackagesGroupedById() x => x.UpdateListedInBulkAsync(It.Is>(l => l.All(i => i.Id == "NuGet.Frameworks")), false), Times.Once); Assert.Equal( - "4 packages across 2 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + "4 packages across 2 package IDs have been unlisted. 1 packages were skipped because they are deleted or they failed validation.", Target.TempData["Message"]); } @@ -99,12 +99,12 @@ public async Task FiltersOutWrongPackageStatus(PackageStatus packageStatus) x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), Times.Once); Assert.Equal( - "1 packages across 1 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + "1 packages across 1 package IDs have been unlisted. 1 packages were skipped because they are deleted or they failed validation.", Target.TempData["Message"]); } [Fact] - public async Task FiltersOutPackagesWithMatchingListed() + public async Task DoesNotFilterOutPackagesWithMatchingListed() { // Arrange var input = new UpdateListedRequest @@ -126,10 +126,10 @@ public async Task FiltersOutPackagesWithMatchingListed() x => x.FindPackagesById("NuGet.Versioning", PackageDeprecationFieldsToInclude.DeprecationAndRelationships), Times.Once); PackageUpdateService.Verify( - x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), + x => x.UpdateListedInBulkAsync(It.Is>(l => l.Count == 2), false), Times.Once); Assert.Equal( - "1 packages across 1 package IDs have been unlisted. 1 packages were already up-to-date and were left unchanged.", + "2 packages across 1 package IDs have been unlisted. 0 packages were skipped because they are deleted or they failed validation.", Target.TempData["Message"]); } @@ -160,7 +160,7 @@ public async Task UsesPointQueryForSingleVersion() x => x.UpdateListedInBulkAsync(It.Is>(l => l.Single().NormalizedVersion == "4.4.0"), false), Times.Once); Assert.Equal( - "1 packages across 1 package IDs have been unlisted. 0 packages were already up-to-date and were left unchanged.", + "1 packages across 1 package IDs have been unlisted. 0 packages were skipped because they are deleted or they failed validation.", Target.TempData["Message"]); } } diff --git a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs index 053fc26a26..0ba0dfd51c 100644 --- a/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageUpdateServiceFacts.cs @@ -74,9 +74,9 @@ public async Task ThrowsIfPackagesHaveDifferentRegistrations() } [Theory] - [InlineData(false, AuditedPackageAction.Unlist, "WHERE [Key] IN (20002)")] - [InlineData(true, AuditedPackageAction.List, "WHERE [Key] IN (10001)")] - public async Task UpdateListedStatus(bool listed, AuditedPackageAction action, string sqlWhere) + [InlineData(false, AuditedPackageAction.Unlist)] + [InlineData(true, AuditedPackageAction.List)] + public async Task UpdateListedStatus(bool listed, AuditedPackageAction action) { // Arrange var id = "theId"; @@ -119,8 +119,8 @@ public async Task UpdateListedStatus(bool listed, AuditedPackageAction action, s .Setup(x => x.BeginTransaction()) .Returns(transactionMock.Object); databaseMock - .Setup(x => x.ExecuteSqlCommandAsync(It.Is(q => q.Contains(sqlWhere)), It.IsAny())) - .ReturnsAsync(2) + .Setup(x => x.ExecuteSqlCommandAsync(It.Is(q => q.Contains("WHERE [Key] IN (10001, 20002)")), It.IsAny())) + .ReturnsAsync(4) .Verifiable(); var context = GetFakeContext(); @@ -130,7 +130,7 @@ public async Task UpdateListedStatus(bool listed, AuditedPackageAction action, s var telemetryService = GetMock(); telemetryService - .Setup(x => x.TrackPackagesUpdateListed(It.Is>(l => l.Count == 1), listed)) + .Setup(x => x.TrackPackagesUpdateListed(It.Is>(l => l.Count == 2), listed)) .Verifiable(); var service = Get(); From 13daa1aa443961954718e28d4592794164547a5b Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 23 Jul 2021 10:05:03 -0700 Subject: [PATCH 13/15] Update src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Loïc Sharma <737941+loic-sharma@users.noreply.github.com> --- src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index af580ae783..b6629387df 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -28,7 +28,7 @@ internal List SearchForPackages(IPackageService packageService, string var completedQueries = new HashSet(StringComparer.OrdinalIgnoreCase); foreach (var queryPart in queryParts) { - var spitQuery = queryPart.Split(new[] { ' ', '/' }, StringSplitOptions.RemoveEmptyEntries); + var splitQuery = queryPart.Split(new[] { ' ', '/' }, StringSplitOptions.RemoveEmptyEntries); if (spitQuery.Length == 1) { // Don't make the same query twice. From dcf7a47cb27d5f8f5511b5ffd1ff449e3cf92678 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 23 Jul 2021 10:06:22 -0700 Subject: [PATCH 14/15] Address comments --- .../PackageManagement/IPackageUpdateService.cs | 2 +- .../PackageManagement/PackageUpdateService.cs | 8 +++----- .../Areas/Admin/Controllers/AdminControllerBase.cs | 10 +++++----- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs index f151ed9d3b..94cd62663c 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageUpdateService.cs @@ -20,7 +20,7 @@ public interface IPackageUpdateService /// /// Updates the listed status on a batch of packages. All of the packages must be related to the same package registration. /// Packages that are deleted or have failed validation are not allowed. Packages that already have a matching listed state - /// will be ignored. + /// will not be skipped, to enable reflow of listed status. /// /// The packages to update. /// True to make the packages listed, false to make the packages unlisted. diff --git a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs index 9208403d3c..e99dba3f31 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageUpdateService.cs @@ -1,12 +1,12 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using NuGet.Services.Entities; -using NuGetGallery.Auditing; using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using NuGet.Services.Entities; +using NuGetGallery.Auditing; namespace NuGetGallery { @@ -70,7 +70,7 @@ public async Task UpdateListedInBulkAsync(IReadOnlyList packages, bool await _entitiesContext.SaveChangesAsync(); - await UpdatePackagesAsync(packages); + await UpdatePackagesAsync(packages, updateIndex: true); transaction.Commit(); @@ -81,8 +81,6 @@ public async Task UpdateListedInBulkAsync(IReadOnlyList packages, bool await _auditingService.SaveAuditRecordAsync(new PackageAuditRecord( package, listed ? AuditedPackageAction.List : AuditedPackageAction.Unlist)); - - _indexingService.UpdatePackage(package); } } } diff --git a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs index b6629387df..e07ba9d62e 100644 --- a/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs +++ b/src/NuGetGallery/Areas/Admin/Controllers/AdminControllerBase.cs @@ -29,10 +29,10 @@ internal List SearchForPackages(IPackageService packageService, string foreach (var queryPart in queryParts) { var splitQuery = queryPart.Split(new[] { ' ', '/' }, StringSplitOptions.RemoveEmptyEntries); - if (spitQuery.Length == 1) + if (splitQuery.Length == 1) { // Don't make the same query twice. - var id = spitQuery[0].Trim(); + var id = splitQuery[0].Trim(); if (!completedQueries.Add(id)) { continue; @@ -46,11 +46,11 @@ internal List SearchForPackages(IPackageService packageService, string .OrderBy(p => NuGetVersion.Parse(p.NormalizedVersion))); } } - else if (spitQuery.Length == 2) + else if (splitQuery.Length == 2) { // Don't make the same query twice. - var id = spitQuery[0].Trim(); - var version = spitQuery[1].Trim(); + var id = splitQuery[0].Trim(); + var version = splitQuery[1].Trim(); if (!completedQueries.Add(id + "/" + version)) { continue; From 3b95a5a5edea1c35db9bd0281a51d725daf345bf Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Fri, 23 Jul 2021 10:27:56 -0700 Subject: [PATCH 15/15] Add select listed/unlist --- .../Areas/Admin/Views/Home/Index.cshtml | 10 ++++----- .../Admin/Views/UpdateListed/Index.cshtml | 22 ++++++++++++++++++- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml index 007d82e0d9..bc160a8b53 100644 --- a/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml @@ -92,14 +92,14 @@ @ViewHelpers.PostLink( this, formId: "clear-content-cache-form", + actionName: "ClearContentCache", + controllerName: "Home", + role: string.Empty, + area: "Admin", htmlContent: @ Clear Content Cache - , - actionName: "ClearContentCache", - controllerName: "Home", - role: string.Empty, - area: "Admin") + )

    Clear Content Cache diff --git a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml index 68da575541..5794424d4f 100644 --- a/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml +++ b/src/NuGetGallery/Areas/Admin/Views/UpdateListed/Index.cshtml @@ -8,6 +8,8 @@


    + +
    @using (Html.BeginForm("UpdateListed", "UpdateListed", new { area = "Admin" }, FormMethod.Post, new { id = "update-listed-form" })) @@ -84,7 +86,7 @@ $self.selectAllState(false); $self.searchResults.removeAll(); for (var i = 0; i < data.length; i++) { - data[i].Selected = ko.observable(!!data[i].Listed); + data[i].Selected = ko.observable(false); } $self.searchResults(data); } @@ -104,6 +106,24 @@ return true; }; + this.selectListed = function (e) { + ko.utils.arrayForEach($self.searchResults(), function (result) { + if (result.Listed) { + result.Selected(true); + } + }); + return true; + }; + + this.selectUnlisted = function (e) { + ko.utils.arrayForEach($self.searchResults(), function (result) { + if (!result.Listed) { + result.Selected(true); + } + }); + return true; + }; + this.generateValue = function (package) { return package.PackageId + '|' + package.PackageVersionNormalized; };