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

Sort available plugins by popularity #4588

Merged
merged 2 commits into from
Apr 19, 2020
Merged

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Mar 16, 2020

Downstream from jenkins-infra/update-center2#356 but can be merged independently. This works as is today.

Should work even better when combined with #4580.

Plugins are now sorted by popularity (unit-less and undefined except larger is better; currently roughly implemented as percentage of instances with the plugin installed), descending, on the "Available" tab of the plugin manager.

Screenshots

Nothing to see except that OWASP Markup Formatter is the most popular plugin in the subset of plugins I have locally (see testing notes for setup):

Screenshot

I also played a bit with a UI representation of the popularity before leaving that to whoever feels like adding it, here I use a border on the left side of the table, opacity ~ popularity:

Screenshot

Screenshot

Proposed changelog entries

  • Sort plugins on the "Available" plugin manager tab by popularity by default. This requires that the chosen update site reports this additional metadata.

Testing notes

(For use with core-pr-tester, you need to bind-mount a volume from the host where you run update-center2.)

Build jenkins-infra/update-center2#356 and then run:

java -jar target/update-center2-2.1-SNAPSHOT-bin/update-center2-2.1-SNAPSHOT.jar -id default -connectionCheckUrl http://www.google.com/ -no-experimental -skip-release-history -www www -cap 2.999 -capCore 2.999 -stableCore -maxPlugins 20 (or however many plugins you want to wait for)

Configure the update center URL (Manage Plugins » Advanced) as follows, then Save:

file:///path/to/update-center2/www/update-center.json

Script Console:

DownloadService.signatureCheck = false

Check for updates.

Proposed upgrade guidelines

N/A

Submitter checklist

  • [n/a] JIRA issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

@daniel-beck daniel-beck added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Mar 16, 2020
@fqueiruga
Copy link
Contributor

TBH I'm not sure what's better UX-wise. While I've definitely felt the pain of searching for the Git plugin before, I'm not sure that doing without alphabetical order would be better.

Now that you've done great work setting a popularity value, maybe we could add some sort of filtering by that field, instead of ordering. Maybe have 3 options like "Popular, commonly used, barely used" (I'm sure there are better categories). WDYT?

Other than that, I'm neutral to these changes, I don't have an informed opinion on this

@daniel-beck
Copy link
Member Author

I'm not sure that doing without alphabetical order would be better

We're not -- this is simply the default sort order, and if you sort by the previously "unsortable" left column. You can still go alphabetical by clicking the "name" column table header.

This is the problem with the plugin manager: The UI was written for dozens of plugins. It still works well enough for Updates and Installed, even if the latter can go into the low hundreds and almost never is below 50 anymore. But it's useless for Available, with 1500+ items, where sorting by some "priority", not showing everything by default, and easily available searching/filtering should be the main ways to interact with it.

@oleg-nenashev oleg-nenashev added the web-ui The PR includes WebUI changes which may need special expertise label Mar 17, 2020
@daniel-beck
Copy link
Member Author

But it's useless for Available, with 1500+ items, where sorting by some "priority", not showing everything by default, and easily available searching/filtering should be the main ways to interact with it.

FTR, the rest of this is implemented in #4580.

@varyvol varyvol added the unresolved-merge-conflict There is a merge conflict with the target branch. label Apr 16, 2020
@daniel-beck daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Apr 16, 2020
@daniel-beck
Copy link
Member Author

Conflicts with another merged PR that would hide this column from non-admin users. Unsure how to proceed.

@daniel-beck
Copy link
Member Author

Resolved the merge conflict by just keeping the column hidden (and the feature to re-sort by popularity unavailable) to users with System Read permission. Feedback, ideally with alternative suggestions, welcome.

@daniel-beck daniel-beck removed needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging unresolved-merge-conflict There is a merge conflict with the target branch. labels Apr 16, 2020
@daniel-beck daniel-beck requested a review from timja April 16, 2020 14:55
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

not tested manually, but looks ok, once it's mainlined and update center has it by default I'll see if there's something more to do for system read

@timja
Copy link
Member

timja commented Apr 18, 2020

One thought, would it be possible to hide meta plugins in popularity calculations? Users don’t need to care about http client, Jackson and some of the ui plugins that are just dependencies of plugins, could be added on as an enhancement too

@daniel-beck
Copy link
Member Author

@timja Future enhancement I think, possibly only in update-center2: E.g. just advertise popularity 0 if it has the api-plugin label, or something similar (needs validation).

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 18, 2020
@timja
Copy link
Member

timja commented Apr 18, 2020

Thanks for this nice enhancement let’s merge it after 24 hours if there’s no negative feedback

@daniel-beck
Copy link
Member Author

@timja Future enhancement I think, possibly only in update-center2: E.g. just advertise popularity 0 if it has the api-plugin label, or something similar (needs validation).

FWIW given the recent behavior (not yet shown in screenshots) of requiring filter terms for Available plugins, I don't think this is much of a problem. Unless you search for a specific utility / library name, it's unlikely you'll see them, even with very high popularity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants