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

Fixed breadcrumb links for packages with build metadata #8157

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

rocklan
Copy link
Contributor

@rocklan rocklan commented Aug 13, 2020

Fixes breadcrumb links for packages with build metadata:

image

The only question now is if we want to continue to display the build metadata in the link (the +6...) part. I think it's ok, but that's me.

Addresses #8136

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @rocklan 🏆!
Let's ensure the null behavior doesn't cause problems.

@joelverhagen joelverhagen self-assigned this Aug 13, 2020
@joelverhagen
Copy link
Member

Would it be possible to get a unit test to cover this case?

@rocklan rocklan changed the title Fixed breadcrumb links for packages with build metadata #8136 Fixed breadcrumb links for packages with build metadata Aug 13, 2020
@rocklan
Copy link
Contributor Author

rocklan commented Aug 13, 2020

Would it be possible to get a unit test to cover this case?

I've added a unit test to cover this in the ThePackageBaseHelperMethod class. And having done so, I can see how this bug snuck in. There is already a test to cover this exact scenario, inside the ThePackageHelperMethod class, but the problem is it calls one of the overloads of Package, where a Package object is passed through, and this function passes through the package.NormalizedVersion. However the code that renders the breadcrumbs doesn't call this method, it calls the one that accepts two strings, and it passes though the raw version instead of the normalised one. I don't think calling normalize() twice will cause any issues.

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Awesome, thanks so much! The unit test is 💯

@joelverhagen
Copy link
Member

Hey, it looks like this test is failing on your branch (per CI):

##[error]    NuGetGallery.ApiControllerFacts+TheGetStatsDownloadsAction.VerifyRecentPopularityStatsDownloads [FAIL]

      unexpected content result[3].Gallery
      Expected: True
      Actual:   False
      Stack Trace:
        tests\NuGetGallery.Facts\Controllers\ApiControllerFacts.cs(0,0): at NuGetGallery.ApiControllerFacts.TheGetStatsDownloadsAction.<VerifyRecentPopularityStatsDownloads>d__0.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
```

@rocklan
Copy link
Contributor Author

rocklan commented Aug 18, 2020

Hey, it looks like this test is failing on your branch (per CI): NuGetGallery.ApiControllerFacts+TheGetStatsDownloadsAction.VerifyRecentPopularityStatsDownloads [FAIL]

I've committed a fix for this, it was previously checking that the version number on the end of a link was 1.1 but now as they are normalized it should be checking for 1.1.0

@joelverhagen joelverhagen merged commit 9b85f4c into NuGet:dev Aug 18, 2020
@joelverhagen
Copy link
Member

Awesome, thanks for this contribution @rocklan! 🥇

@rocklan rocklan deleted the rocklan-8136 branch August 19, 2020 23:19
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.

2 participants