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.

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

Code and tests for issue #2567 #3458

Merged
merged 2 commits into from
Apr 5, 2017
Merged

Code and tests for issue #2567 #3458

merged 2 commits into from
Apr 5, 2017

Conversation

supergibbs
Copy link
Contributor

@supergibbs supergibbs commented Jan 7, 2017

URLs like https://www.nuget.org/packages/PACKAGE/absoluteLatest will display the latest version.

@dnfclas
Copy link

dnfclas commented Jan 7, 2017

Hi @supergibbs, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@supergibbs
Copy link
Contributor Author

supergibbs commented Mar 22, 2017

Any update on this PR? @ryuyu @skofman1

@skofman1
Copy link
Contributor

Hi @supergibbs, thanks for the PR!

Can you please explain what is the feature you are implementing and why this feature is important, and in what scenarios?

Thanks!

@supergibbs
Copy link
Contributor Author

Sure, please see my #2567 (comment)

@skofman1
Copy link
Contributor

Thanks @supergibbs . @ryuyu will review the PR and merge it.

@ryuyu ryuyu changed the base branch from master to dev March 31, 2017 21:20
@ryuyu
Copy link
Contributor

ryuyu commented Apr 3, 2017

@supergibbs A few issues here.

  1. The VersionRouteConstraint needs to be updated to allow the "prerelease" string as a valid version (https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/App_Start/VersionRouteConstraint.cs)
  2. FindPackageByIdAndVersion should not accept "prerelease" as a valid version string. We should instead add a new method that gets called by the view specially when we pass "prerelease".
  3. Recognition of the "prerelease" url should be at the view level, in DisplayPackage (https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/Controllers/PackagesController.cs#L301). This method should be updated to call the new helper we add above.

This is so that we add this explicitly on only the display endpoint. The rest of the endpoints should require specifying a version (these are inserted on the page when the page is created).

Thanks!

@supergibbs
Copy link
Contributor Author

Thanks for the notes @ryuyu. I started over off the latest dev branch and re-implemented accordingly. Let me know if there is anything else!

var package = _packageService.FindPackageByIdAndVersion(id, version);

Package package;
if (version.Equals("prerelease", StringComparison.InvariantCultureIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a null check (version != null) here before we do the equals check.
Currently, it will throw if you try to go to an empty version url (Ie nuget.org/packages/{id}/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I added a test for that too.

.Include(p => p.LicenseReports)
.Include(p => p.PackageRegistration)
.Where(p => (p.PackageRegistration.Id == id))
.Where(p => p.IsPrerelease)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's drop this condition and change the name of this method to FindAbsoluteLatestPackageById.
This way, if a package has no prerelease versions, the call to this will return whatever is the most recent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, how do you feel about filtering out unlisted packages? I feel like that is what most owners would want. Possibly in FindPackageByIdAndVersion() too when version is null.

@@ -32,9 +33,14 @@ public bool Match(HttpContextBase httpContext, Route route, string parameterName
{
return true;
}


if (versionText.Equals("prerelease", StringComparison.InvariantCultureIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change this string from "prerelease" to "absoluteLatest" or something of that nature.
With the other changes suggested, this will make it behave as expected if the most recent version is a prerelease, and it is still descriptive of the case where there are no prerelease versions.

Copy link
Contributor Author

@supergibbs supergibbs Apr 4, 2017

Choose a reason for hiding this comment

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

Will do. Should I add this "magic string" to the Constants.cs/somewhere else or are you okay with it as is?

Used in two places and two tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. Yea that seems like a good idea. Let's call it AbsoluteLatestUrlString or something of that nature.

@ryuyu
Copy link
Contributor

ryuyu commented Apr 4, 2017

👍
A few suggestions here, but it's looking good.
I think we should change the url from "prerelease" to something like "absoluteLatest" so that it is descriptive of the case where the most recent version is not a prerelease version, or in the case where a package has no prerelease versions.

-Thanks!

- Renamed `FindPackageByLatestPrerelease` to `FindAbsoluteLatestPackageById`
- Added null version check and test
@ryuyu ryuyu merged commit 2c584e7 into NuGet:dev Apr 5, 2017
@supergibbs supergibbs deleted the issue_2567 branch April 5, 2017 18:42
@skofman1 skofman1 mentioned this pull request Apr 5, 2017
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants