Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor fixes to the GET DeleteSymbols endpoint #6937

Merged
merged 2 commits into from
Mar 5, 2019
Merged

Minor fixes to the GET DeleteSymbols endpoint #6937

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87056b8
deletesymbols should use selectlist
Feb 26, 2019
11bbc53
order symbols descending
Mar 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 26 additions & 18 deletions src/NuGetGallery/Controllers/PackagesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1493,9 +1493,25 @@ public virtual async Task<ActionResult> Manage(string id, string version = null)
[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();
}

Expand All @@ -1507,26 +1523,18 @@ 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of ?? PackageStatus.Deleted? null by itself is not equal to any enum value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enums are ints internally, which means they don't support null.
Therefore, I must convert the null to a valid enum value before I can compare.


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);
}
Expand Down
117 changes: 82 additions & 35 deletions tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2527,11 +2527,11 @@ public class TheManageMethodWithMissingVersion : TheManageMethodWithLatestVersio
}
}

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()
{
Expand All @@ -2544,6 +2544,7 @@ public TheDeleteSymbolsMethod()
Key = 2,
PackageRegistration = _packageRegistration,
Version = "1.0.0+metadata",
NormalizedVersion = "1.0.0",
Listed = true,
IsLatestSemVer2 = true,
HasReadMe = false,
Expand All @@ -2554,6 +2555,7 @@ public TheDeleteSymbolsMethod()
Key = 1,
PackageRegistration = _packageRegistration,
Version = "1.0.0-alpha",
NormalizedVersion = "1.0.0-alpha",
IsLatest = true,
IsLatestSemVer2 = true,
Listed = true,
Expand All @@ -2570,7 +2572,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);

Expand Down Expand Up @@ -2652,8 +2661,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));
Expand All @@ -2664,30 +2673,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);
Expand All @@ -2698,11 +2690,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);
Expand All @@ -2712,11 +2700,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
Expand Down