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

package list can be sorted by ID, owners, downloads and version #8158

Merged
merged 11 commits into from
Sep 4, 2020

Conversation

rocklan
Copy link
Contributor

@rocklan rocklan commented Aug 13, 2020

This adds support for ordering the "Published Packages" and "Unlisted Packages" tables on the "Manage my packages" screen, eg:

sorting

Clicking on any of the column headers sorts by that column. I am by no means a JS guru but hopefully this does what we want and works for most people.

Addresses #7806

@joelverhagen
Copy link
Member

OMG this is amazing. /cc @jcjiang @skofman1

This may be a tall order, but would it be possible to have a gif recording of the behavior in action? Like initial state -> sort by downloads -> sort by downloads -> sort by package ID? If not I may be able to cook it up myself (i'm not a GIF recording pro)

One concern I have is the performance on owners with many package IDs. For example, the Microsoft org has nearly 5000 packages. Would it be possible to do some rough tests of how it "feels" as the number of package IDs goes up? If we run into trouble we can say "well, it's still okay for small pages" or we can disable the behavior for when there are too many rows (based on some hard coded safe threshold).

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

Is there an associated issue for this PR? I see #123 in the description (perhaps from the template) but this doesn't seem right. If an issue does not exist, could you open one and start a discussion about the pain you have, your proposed solution, and other thoughts?

@rocklan
Copy link
Contributor Author

rocklan commented Aug 14, 2020

Yes it's issue #7806. I've updated the description.

Copy link
Contributor Author

@rocklan rocklan left a comment

Choose a reason for hiding this comment

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

OMG this is amazing. /cc @jcjiang @skofman1

This may be a tall order, but would it be possible to have a gif recording of the behavior in action? Like initial state -> sort by downloads -> sort by downloads -> sort by package ID? If not I may be able to cook it up myself (i'm not a GIF recording pro)

One concern I have is the performance on owners with many package IDs. For example, the Microsoft org has nearly 5000 packages. Would it be possible to do some rough tests of how it "feels" as the number of package IDs goes up? If we run into trouble we can say "well, it's still okay for small pages" or we can disable the behavior for when there are too many rows (based on some hard coded safe threshold).

ScreenToGif is your friend 👍

If you have a lot of packages, like 5000, on my machine it takes about 3-4 seconds to sort in chrome. (I don't have access to production data so I can't perf test it properly :) - The sorting is all done client side so it depends on the speed of the clients machine. For me, it seems ok, especially when downloading the page and expanding the list of packages takes 3-5 seconds anyway.

sorting-perf

@joelverhagen
Copy link
Member

Hey @jcjiang, could you take a look and provide some feedback from the PM perspective?

@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

like this test is failing on your branch (per CI): VerifyRecentPopularityStatsDownloads

This test and all other tests pass on my machine, not sure why this one failed. It doesn't seem related to the changes that I've made. Maybe the build can just be kicked off again?

@joelverhagen
Copy link
Member

Already on the second build. Let me try locally and I'll get back to you. This line was suspicious to me because 1.1 is a non-normalized version:

Assert.True((string)result[3]["Gallery"] == "/packages/B/1.1", "unexpected content result[3].Gallery");

@joelverhagen
Copy link
Member

Okay, I am crazy. I crossed my wires with your other PR #8157. Sorry about the confusion. I'll copy my comment over there for clarity.

@joelverhagen
Copy link
Member

The test should be changed to be 1.1.0 but of course on the other PR (#8157).

@joelverhagen
Copy link
Member

I am going to deploy this to our test environment and to give it another spin. After the UT and this smoke test, we should be good to go 😄

@joelverhagen
Copy link
Member

I'm seeing some strange behavior with packages with some package versions being out of order. For example:
image

Could you take a look and see what's going on with this bug?
Packages

@rocklan
Copy link
Contributor Author

rocklan commented Sep 4, 2020

I'm seeing some strange behavior with packages with some package versions being out of order

Ahha, found it. I was using the following javascript code to check if the "SortBy" attribute+value existed:

var sortby = td.data('sortby');

if (sortby) {
    return sortby;
}

so it turns out if the value of sortby is zero, it will return false, any other value it will return true :) as a result, the first item in the list will always appear last.

An easy fix:

if (typeof sortby !== 'undefined') {
    return sortby;
}

which is the correct way of checking if the attribute has a defined value or not.

@rocklan rocklan closed this Sep 4, 2020
@joelverhagen
Copy link
Member

@rocklan, this should not be closed, right?

@rocklan rocklan reopened this Sep 4, 2020
@joelverhagen
Copy link
Member

Okay great! Once we get that UT mentioned in my comment #8158 (comment) above, we'll be good to go! We've got PM sign off and several devs have looked it over.

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.

This PR is ready to rock(lan) 😉!
Thanks!

@joelverhagen joelverhagen merged commit 165984b into NuGet:dev Sep 4, 2020
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