Minor fixes to the GET DeleteSymbols endpoint (#6937)

Author: Scott Bommarito

src/NuGetGallery/Controllers/PackagesController.cs

@@ -1497,9 +1497,25 @@ await _readMeService.GetReadMeMdAsync(package),
         [RequiresAccountConfirmation("delete a symbols package")]
         public virtual ActionResult DeleteSymbols(string id, string version)
         {
-            var package = _packageService.FindPackageByIdAndVersion(id, version, SemVerLevelKey.SemVer2);
+            Package package = null;
+
+            // Load all versions of the package.
+            var packages = _packageService.FindPackagesById(id);
+            if (version != null)
+            {
+                // Try to find the exact version if it was specified.
+                package = packages.SingleOrDefault(p => p.NormalizedVersion == NuGetVersionFormatter.Normalize(version));
+            }
+
             if (package == null)
             {
+                // If the exact version was not found, fall back to the latest version.
+                package = _packageService.FilterLatestPackage(packages, SemVerLevelKey.SemVer2, allowPrerelease: true);
+            }
+
+            if (package == null)
+            {
+                // If the package has no versions, return not found.
                 return HttpNotFound();
             }
 
@@ -1511,26 +1527,19 @@ public virtual ActionResult DeleteSymbols(string id, string version)
 
             var model = new DeletePackageViewModel(package, currentUser, DeleteReasons);
 
-            // Fetch all the available symbols package for all the versions from the 
-            // database since the DisplayPackageViewModel(base class for DeletePackageViewModel) does not
-            // set the `LatestSymbolsPackage` data on the model(since it is an unnecessary and expensive db
-            // query). It is fine to do this here when invoking delete page. Note: this could also potentially 
-            // cause unbounded(high number) db calls based on the number of versions associated with a package.
-            var packageViewModelsForAllAvailableSymbolsPackage = package
-                .PackageRegistration
-                .Packages
+            // Fetch all versions of the package with symbols.
+            var versionsWithSymbols = packages
                 .Where(p => p.PackageStatusKey != PackageStatus.Deleted)
-                .Select(p => p.LatestSymbolPackage())
-                .Where(sp => sp != null && sp.StatusKey == PackageStatus.Available)
-                .Select(sp => new PackageViewModel(sp.Package));
+                .Where(p => (p.LatestSymbolPackage()?.StatusKey ?? PackageStatus.Deleted) == PackageStatus.Available)
+                .OrderByDescending(p => new NuGetVersion(p.Version));
 
-            model.VersionSelectList = new SelectList(
-                packageViewModelsForAllAvailableSymbolsPackage
-                .Select(pvm => new
+            model.VersionSelectList = versionsWithSymbols
+                .Select(versionWithSymbols => new SelectListItem
                 {
-                    text = pvm.NuGetVersion.ToFullString() + (pvm.LatestVersionSemVer2 ? " (Latest)" : string.Empty),
-                    url = Url.DeleteSymbolsPackage(pvm)
-                }), "url", "text", Url.DeleteSymbolsPackage(model));
+                    Text = PackageHelper.GetSelectListText(versionWithSymbols),
+                    Value = Url.DeleteSymbolsPackage(new TrivialPackageVersionModel(versionWithSymbols)),
+                    Selected = package == versionWithSymbols
+                });
 
             return View(model);
         }

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

@@ -2629,11 +2629,11 @@ protected override Task<ActionResult> GetManageResult(
             }
         }
 
-        public class TheDeleteSymbolsMethod : TestContainer
+        public abstract class TheDeleteSymbolsMethod : TestContainer
         {
-            private string _packageId = "CrestedGecko";
-            private PackageRegistration _packageRegistration;
-            private Package _package;
+            protected string _packageId = "CrestedGecko";
+            protected PackageRegistration _packageRegistration;
+            protected Package _package;
 
             public TheDeleteSymbolsMethod()
             {
@@ -2646,6 +2646,7 @@ public TheDeleteSymbolsMethod()
                     Key = 2,
                     PackageRegistration = _packageRegistration,
                     Version = "1.0.0+metadata",
+                    NormalizedVersion = "1.0.0",
                     Listed = true,
                     IsLatestSemVer2 = true,
                     HasReadMe = false,
@@ -2656,6 +2657,7 @@ public TheDeleteSymbolsMethod()
                     Key = 1,
                     PackageRegistration = _packageRegistration,
                     Version = "1.0.0-alpha",
+                    NormalizedVersion = "1.0.0-alpha",
                     IsLatest = true,
                     IsLatestSemVer2 = true,
                     Listed = true,
@@ -2672,7 +2674,14 @@ public TheDeleteSymbolsMethod()
             [Fact]
             public void Returns404IfPackageNotFound()
             {
-                var controller = CreateController(GetConfigurationService());
+                var packageService = new Mock<IPackageService>();
+                packageService
+                    .Setup(x => x.FindPackagesById(_packageRegistration.Id, false))
+                    .Returns(new Package[0]);
+
+                var controller = CreateController(
+                    GetConfigurationService(),
+                    packageService: packageService);
 
                 var result = controller.DeleteSymbols(_packageRegistration.Id, _package.Version);
 
@@ -2754,8 +2763,8 @@ public void DisplaysFullVersionStringAndUsesNormalizedVersionsInUrlsInSelectList
 
                 foreach (var pkg in _packageRegistration.Packages)
                 {
-                    var valueField = controller.Url.DeleteSymbolsPackage(model);
-                    var textField = model.NuGetVersion.ToFullString() + (pkg.IsLatestSemVer2 ? " (Latest)" : string.Empty);
+                    var valueField = controller.Url.DeleteSymbolsPackage(new TrivialPackageVersionModel(pkg));
+                    var textField = PackageHelper.GetSelectListText(pkg);
 
                     var selectListItem = model.VersionSelectList
                         .SingleOrDefault(i => string.Equals(i.Text, textField) && string.Equals(i.Value, valueField));
@@ -2766,30 +2775,13 @@ public void DisplaysFullVersionStringAndUsesNormalizedVersionsInUrlsInSelectList
                 }
             }
 
-            [Fact]
-            public void WhenPackageRegistrationIsLockedReturnsLockedState()
+            [Theory]
+            [MemberData(nameof(Owner_Data))]
+            public void WhenPackageRegistrationIsLockedReturnsLockedState(User currentUser, User owner)
             {
-                // Arrange
-                var user = new User("Frodo") { Key = 1 };
-                var packageRegistration = new PackageRegistration { Id = "Foo", IsLocked = true };
-                packageRegistration.Owners.Add(user);
-
-                var package = new Package
-                {
-                    Key = 2,
-                    PackageRegistration = packageRegistration,
-                    Version = "1.0.0+metadata",
-                };
-
-                var packageService = new Mock<IPackageService>(MockBehavior.Strict);
-                packageService.Setup(svc => svc.FindPackageByIdAndVersion("Foo", "1.0.0", SemVerLevelKey.SemVer2, true))
-                    .Returns(package);
-
-                var controller = CreateController(GetConfigurationService(), packageService: packageService);
-                controller.SetCurrentUser(user);
+                _packageRegistration.IsLocked = true;
 
-                // Act
-                var result = controller.DeleteSymbols("Foo", "1.0.0");
+                var result = GetDeleteSymbolsResult(currentUser, owner, out var controller);
 
                 // Assert
                 var model = ResultAssert.IsView<DeletePackageViewModel>(result);
@@ -2800,11 +2792,7 @@ private ActionResult GetDeleteSymbolsResult(User currentUser, User owner, out Pa
             {
                 _packageRegistration.Owners.Add(owner);
 
-                var packageService = new Mock<IPackageService>(MockBehavior.Strict);
-                packageService
-                    .Setup(svc => svc.FindPackageByIdAndVersion(_packageId, _package.Version, SemVerLevelKey.SemVer2, true))
-                    .Returns(_package).Verifiable();
-
+                var packageService = CreatePackageService();
                 controller = CreateController(
                     GetConfigurationService(),
                     packageService: packageService);
@@ -2814,11 +2802,70 @@ private ActionResult GetDeleteSymbolsResult(User currentUser, User owner, out Pa
                 Routes.RegisterRoutes(routeCollection);
                 controller.Url = new UrlHelper(controller.ControllerContext.RequestContext, routeCollection);
 
-                var result = controller.DeleteSymbols(_packageId, _package.Version);
+                var result = InvokeDeleteSymbols(controller);
 
                 packageService.Verify();
                 return result;
             }
+
+            protected abstract Mock<IPackageService> CreatePackageService();
+
+            protected abstract ActionResult InvokeDeleteSymbols(PackagesController controller);
+        }
+
+        public class TheDeleteSymbolsMethodWithExactVersion : TheDeleteSymbolsMethod
+        {
+            protected override Mock<IPackageService> CreatePackageService()
+            {
+                var packageService = new Mock<IPackageService>(MockBehavior.Strict);
+                packageService
+                    .Setup(svc => svc.FindPackagesById(_packageId, false))
+                    .Returns(_packageRegistration.Packages.ToList())
+                    .Verifiable();
+
+                return packageService;
+            }
+
+            protected override ActionResult InvokeDeleteSymbols(PackagesController controller)
+            {
+                return controller.DeleteSymbols(_packageId, _package.Version);
+            }
+        }
+
+        public abstract class TheDeleteSymbolsMethodThatFilters : TheDeleteSymbolsMethod
+        {
+            protected override Mock<IPackageService> CreatePackageService()
+            {
+                var packages = _packageRegistration.Packages.ToList();
+                var packageService = new Mock<IPackageService>(MockBehavior.Strict);
+                packageService
+                    .Setup(svc => svc.FindPackagesById(_packageId, false))
+                    .Returns(packages)
+                    .Verifiable();
+
+                packageService
+                    .Setup(svc => svc.FilterLatestPackage(packages, SemVerLevelKey.SemVer2, true))
+                    .Returns(_package)
+                    .Verifiable();
+
+                return packageService;
+            }
+        }
+
+        public class TheDeleteSymbolsMethodWithMissingVersion : TheDeleteSymbolsMethodThatFilters
+        {
+            protected override ActionResult InvokeDeleteSymbols(PackagesController controller)
+            {
+                return controller.DeleteSymbols(_packageId, "missing");
+            }
+        }
+
+        public class TheDeleteSymbolsMethodWithNullVersion : TheDeleteSymbolsMethodThatFilters
+        {
+            protected override ActionResult InvokeDeleteSymbols(PackagesController controller)
+            {
+                return controller.DeleteSymbols(_packageId, null);
+            }
         }
 
         public class TheDeleteSymbolsPackageMethod : TestContainer