From fd20ca41b52820ed3106a25b64705e776605ce12 Mon Sep 17 00:00:00 2001 From: Tim Lovell-Smith Date: Thu, 3 Oct 2013 14:41:34 -0700 Subject: [PATCH 1/3] Low level part of fixing #403. Make the LATEST STABLE package version the one returned by default when version == null. Make the LATEST version the one returned if no STABLE version exists and allowPrerelease == true. --- src/NuGetGallery/Services/PackageService.cs | 14 +-- .../Services/PackageServiceFacts.cs | 115 ++++++++++-------- 2 files changed, 69 insertions(+), 60 deletions(-) diff --git a/src/NuGetGallery/Services/PackageService.cs b/src/NuGetGallery/Services/PackageService.cs index 00155b2572..ce35dd2b16 100644 --- a/src/NuGetGallery/Services/PackageService.cs +++ b/src/NuGetGallery/Services/PackageService.cs @@ -106,27 +106,27 @@ public virtual Package FindPackageByIdAndVersion(string id, string version, bool .Include(p => p.LicenseReports) .Include(p => p.PackageRegistration) .Where(p => (p.PackageRegistration.Id == id)); + if (String.IsNullOrEmpty(version) && !allowPrerelease) { // If there's a specific version given, don't bother filtering by prerelease. You could be asking for a prerelease package. packagesQuery = packagesQuery.Where(p => !p.IsPrerelease); } + var packageVersions = packagesQuery.ToList(); Package package; - if (version == null) + if (String.IsNullOrEmpty(version)) { - if (allowPrerelease) + package = packageVersions.FirstOrDefault(p => p.IsLatestStable); + + if (package == null && allowPrerelease) { package = packageVersions.FirstOrDefault(p => p.IsLatest); } - else - { - package = packageVersions.FirstOrDefault(p => p.IsLatestStable); - } // If we couldn't find a package marked as latest, then - // return the most recent one. + // return the most recent one (prerelease ones were already filtered out if appropriate...) if (package == null) { package = packageVersions.OrderByDescending(p => p.Version).FirstOrDefault(); diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index a69530cc58..4225730f9a 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -8,6 +8,7 @@ using NuGet; using NuGetGallery.Packaging; using Xunit; +using Xunit.Extensions; namespace NuGetGallery { @@ -1046,33 +1047,14 @@ public void WillThrowIfThePackageDoesNotExist() public class TheFindPackageByIdAndVersionMethod { [Fact] - public void WillGetTheLatestVersionWhenTheVersionArgumentIsNull() - { - var packageRegistration = new PackageRegistration { Id = "theId" }; - var packages = new[] - { - new Package { Version = "1.0", PackageRegistration = packageRegistration }, - new Package - { Version = "2.0", PackageRegistration = packageRegistration, IsLatestStable = true, IsLatest = true } - }.AsQueryable(); - var packageRepository = new Mock>(); - packageRepository.Setup(r => r.GetAll()).Returns(packages); - var service = CreateService(packageRepository: packageRepository); - - var package = service.FindPackageByIdAndVersion("theId", null); - - Assert.Equal("2.0", package.Version); - } - - [Fact] - public void WillGetSpecifiedVersionWhenTheVersionArgumentIsNotNull() + public void ReturnsTheRequestedPackageVersion() { var service = CreateService(setup: - mockPackageService => - { - mockPackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny())).Throws( - new Exception("This should not be called when the version is specified.")); - }); + mockPackageService => + { + mockPackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny())).Throws( + new Exception("This should not be called when the version is specified.")); + }); Assert.DoesNotThrow(() => service.FindPackageByIdAndVersion("theId", "1.0.42")); @@ -1080,74 +1062,101 @@ public void WillGetSpecifiedVersionWhenTheVersionArgumentIsNotNull() // What we're testing via the throw above is that it didn't load the registration and get the latest version. } - [Fact] - public void WillThrowIfIdIsNull() + [Theory] + [InlineData(null)] + [InlineData("")] + public void WillThrowIfIdIsNull(string id) { var service = CreateService(); + var ex = Assert.Throws(() => service.FindPackageByIdAndVersion(id, "1.0.42")); + Assert.Equal("id", ex.ParamName); + } - var ex = Assert.Throws(() => service.FindPackageByIdAndVersion(null, "1.0.42")); + [Fact] + public void ReturnsTheLatestStableVersionIfAvailable() + { + // Arrange + var repository = new Mock>(MockBehavior.Strict); + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package1 = new Package { Version = "1.0", PackageRegistration = packageRegistration, Listed = true, IsLatestStable = true }; + var package2 = new Package { Version = "1.0.0a", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = true, IsLatest = true }; - Assert.Equal("id", ex.ParamName); + repository + .Setup(repo => repo.GetAll()) + .Returns(new[] { package1, package2 }.AsQueryable()); + var service = CreateService(packageRepository: repository); + + // Act + var result = service.FindPackageByIdAndVersion("theId", version: null); + + // Assert + Assert.NotNull(result); + Assert.Equal("1.0", result.Version); } [Fact] - public void FindPackageReturnsTheLatestVersionIfAvailable() + public void ReturnsTheLatestVersionIfNoLatestStableVersionIsAvailable() { // Arrange var repository = new Mock>(MockBehavior.Strict); - var package = CreatePackage("Foo", "1.0.0"); - package.IsLatest = true; - package.IsLatestStable = true; - var packageA = CreatePackage("Foo", "1.0.0a"); + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package1 = new Package { Version = "1.0.0b", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = true, IsLatest = true }; + var package2 = new Package { Version = "1.0.0a", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = true }; - repository.Setup(repo => repo.GetAll()) - .Returns(new[] { package, packageA }.AsQueryable()); + repository + .Setup(repo => repo.GetAll()) + .Returns(new[] { package1, package2 }.AsQueryable()); var service = CreateService(packageRepository: repository); // Act - var result = service.FindPackageByIdAndVersion("Foo", version: null); + var result = service.FindPackageByIdAndVersion("theId", version: null); // Assert - Assert.Equal(package, result); + Assert.NotNull(result); + Assert.Equal("1.0.0b", result.Version); } [Fact] - public void FindPackageReturnsTheLatestVersionIfNoLatestStableVersionIsAvailable() + public void ReturnsNullIfNoLatestStableVersionIsAvailableAndPrereleaseIsDisallowed() { // Arrange var repository = new Mock>(MockBehavior.Strict); - var package = CreatePackage("Foo", "1.0.0b"); - package.IsLatest = true; - var packageA = CreatePackage("Foo", "1.0.0a"); + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package1 = new Package { Version = "1.0.0b", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = true, IsLatest = true }; + var package2 = new Package { Version = "1.0.0a", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = true }; - repository.Setup(repo => repo.GetAll()) - .Returns(new[] { package, packageA }.AsQueryable()); + repository + .Setup(repo => repo.GetAll()) + .Returns(new[] { package1, package2 }.AsQueryable()); var service = CreateService(packageRepository: repository); // Act - var result = service.FindPackageByIdAndVersion("Foo", null); + var result = service.FindPackageByIdAndVersion("theId", version: null, allowPrerelease: false); // Assert - Assert.Equal(package, result); + Assert.Null(result); } [Fact] - public void FindPackageReturnsTheLatestVersionIfNoLatestVersionIsAvailable() + public void ReturnsTheMostRecentVersionIfNoLatestVersionIsAvailable() { // Arrange var repository = new Mock>(MockBehavior.Strict); - var package = CreatePackage("Foo", "1.0.0b"); - var packageA = CreatePackage("Foo", "1.0.0a"); + var packageRegistration = new PackageRegistration { Id = "theId" }; + var package1 = new Package { Version = "1.0.0b", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = false }; + var package2 = new Package { Version = "1.0.0a", PackageRegistration = packageRegistration, IsPrerelease = true, Listed = false }; - repository.Setup(repo => repo.GetAll()) - .Returns(new[] { package, packageA }.AsQueryable()); + repository + .Setup(repo => repo.GetAll()) + .Returns(new[] { package1, package2 }.AsQueryable()); var service = CreateService(packageRepository: repository); // Act - var result = service.FindPackageByIdAndVersion("Foo", null); + var result = service.FindPackageByIdAndVersion("theId", null); // Assert - Assert.Equal(package, result); + Assert.NotNull(result); + Assert.Equal("1.0.0b", result.Version); } } From 3593a2288011f9ffcf841d416ec3ecb0e1894034 Mon Sep 17 00:00:00 2001 From: Tim Lovell-Smith Date: Thu, 3 Oct 2013 14:46:45 -0700 Subject: [PATCH 2/3] UI fixes to reflect that we now take you to the latest stable package version by default. --- .../ViewModels/DisplayPackageViewModel.cs | 14 -------------- .../Views/Packages/DisplayPackage.cshtml | 11 ++++++----- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs b/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs index 98c38992fe..6b8d872b6d 100644 --- a/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs +++ b/src/NuGetGallery/ViewModels/DisplayPackageViewModel.cs @@ -45,19 +45,5 @@ public void SetPendingMetadata(PackageEdit pendingMetadata) public string Copyright { get; set; } public bool HasPendingMetadata { get; private set; } - - public bool IsLatestVersionAvailable - { - get - { - // A package can be identified as the latest available a few different ways - // First, if it's marked as the latest stable version - return LatestStableVersion - // Or if it's marked as the latest version (pre-release) - || LatestVersion - // Or if it's the current version and no version is marked as the latest (because they're all unlisted) - || (IsCurrent(this) && !PackageVersions.Any(p => p.LatestVersion)); - } - } } } \ No newline at end of file diff --git a/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml b/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml index e3984c711c..1280b2b435 100644 --- a/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml +++ b/src/NuGetGallery/Views/Packages/DisplayPackage.cshtml @@ -169,11 +169,10 @@ This is a prerelease version of @Model.Title.

} - else if (!Model.IsLatestVersionAvailable) + else if (!Model.LatestVersion) {

- This is not the latest - version of @Model.Title available. + There is a newer prerelease version of this package available. See the version list below for details.

}
@@ -249,7 +248,8 @@ } @if (Model.Dependencies.DependencySets == null) { - if(Model.IsOwner(User)) { + if (Model.IsOwner(User)) + {

Dependencies

An error occurred processing dependencies. @@ -259,7 +259,8 @@ Note: This message is only visible to you and any other package owners.

} } - else { + else + {

Dependencies

@Html.Partial("_PackageDependencies", Model.Dependencies) } From 809f3b200a90fa4b8ab2734366599b0812511c7d Mon Sep 17 00:00:00 2001 From: Tim Lovell-Smith Date: Thu, 3 Oct 2013 15:31:41 -0700 Subject: [PATCH 3/3] MINOR - test naming nit --- tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs index 4225730f9a..00da9b58c2 100644 --- a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs @@ -1065,7 +1065,7 @@ public void ReturnsTheRequestedPackageVersion() [Theory] [InlineData(null)] [InlineData("")] - public void WillThrowIfIdIsNull(string id) + public void WillThrowIfIdIsNullOrEmpty(string id) { var service = CreateService(); var ex = Assert.Throws(() => service.FindPackageByIdAndVersion(id, "1.0.42"));