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

[JENKINS-68712] updateCenter successfully installs nothing where there's nothing to install #7041

Merged
merged 15 commits into from Sep 14, 2022

Conversation

frankie139506
Copy link
Contributor

@frankie139506 frankie139506 commented Aug 28, 2022

See JENKINS-68712.
Before:
gif2

After:
gif2

Proposed changelog entries

  • Don't navigate to the update center tab if you hit enter on the plugin manager search bar.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) 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) and are in the imperative mood. 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
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).
  • 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 accurate, human-readable, and in the imperative mood
  • 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 must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

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.

sounds reasonable, untested this, reproduced issue on weekly.ci.jenkins.io

@NotMyFault NotMyFault added the bug For changelog: Minor bug. Will be listed after features label Aug 28, 2022
@NotMyFault
Copy link
Member

Is there a specific reason, why the change affects the "Available" tab only, but not the "Updates" tab?

@frankie139506
Copy link
Contributor Author

Is there a specific reason, why the change affects the "Available" tab only, but not the "Updates" tab?

Apologize, I didn't notice, I have made some change, please take a look.

@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Aug 28, 2022
@NotMyFault NotMyFault requested review from NotMyFault and a team August 28, 2022 19:16
@NotMyFault NotMyFault added the web-ui The PR includes WebUI changes which may need special expertise label Aug 28, 2022
@daniel-beck
Copy link
Member

While this addresses the problem of unintentional form submissions by pressing Enter, it does not address the reported issue which could as easily be triggered by pressing one of the buttons. The user would still land on the same page, seeing the same confusing message.

@frankie139506
Copy link
Contributor Author

frankie139506 commented Aug 29, 2022

While this addresses the problem of unintentional form submissions by pressing Enter, it does not address the reported issue which could as easily be triggered by pressing one of the buttons. The user would still land on the same page, seeing the same confusing message.

In this issue, reporter said his expectation is nothing happened, or a more user friendly dialog, so I just make pressing enter nothing happens.

@daniel-beck
Copy link
Member

While this addresses the problem of unintentional form submissions by pressing Enter, it does not address the reported issue which could as easily be triggered by pressing one of the buttons. The user would still land on the same page, seeing the same confusing message.

In this issue, reporter said his expectation is nothing happened, or a more user friendly dialog, so I just make pressing enter nothing happens.

Right, but with minimally different steps (Press Button instead of Enter), the same problem occurs. Perhaps disable the buttons too, until at least one plugin is selected for installation?

@KalleOlaviNiemitalo
Copy link

If some plugin updates are already waiting for Jenkins to be restarted, I think it should be possible to proceed from the list of available plugins to the installation screen where the "restart when no jobs are running" check box is, even if the user does not select any more plugins to update or install. I did not test whether this PR allows that.

@frankie139506
Copy link
Contributor Author

frankie139506 commented Aug 30, 2022

While this addresses the problem of unintentional form submissions by pressing Enter, it does not address the reported issue which could as easily be triggered by pressing one of the buttons. The user would still land on the same page, seeing the same confusing message.

In this issue, reporter said his expectation is nothing happened, or a more user friendly dialog, so I just make pressing enter nothing happens.

Right, but with minimally different steps (Press Button instead of Enter), the same problem occurs. Perhaps disable the buttons too, until at least one plugin is selected for installation?

I'm not sure I understand what you mean or not but I have made some change, please take a look.

@daniel-beck
Copy link
Member

daniel-beck commented Aug 30, 2022

If some plugin updates are already waiting for Jenkins to be restarted, I think it should be possible to proceed from the list of available plugins to the installation screen where the "restart when no jobs are running" check box is, even if the user does not select any more plugins to update or install. I did not test whether this PR allows that.

That's already possible through a sidepanel link that appears when there's something to show.

@daniel-beck daniel-beck self-requested a review August 30, 2022 08:14
@frankie139506
Copy link
Contributor Author

frankie139506 commented Aug 30, 2022

@daniel-beck Sorry, perhap I misunderstand your mean, but I hope if there is anything I can improve please let me know.

@daniel-beck
Copy link
Member

@frankie139506 I'll try to review and test this PR tomorrow. Right now I have no requests for changes. My previous message was responding to a different comment. Thanks for checking!

@frankie139506
Copy link
Contributor Author

frankie139506 commented Aug 31, 2022

@daniel-beck I made some changes, when I don't have something selected to update, the loading plugin extension doesn't appear when I press another button, I think this change makes the question more reasonable, please take a look if you have time. Thanks.

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

The submission by pressing Enter is correctly blocked.

The change to the button is incorrect and breaks its functionality, that should be reverted.

What to do about the buttons? As I wrote previously, my recommendation would be to disable the "Download" buttons by default, and only enable them while any checkboxes are checked. That would prevent people from clicking them when nothing would be installed.

I think just preventing pressing Enter as it is currently is a fine change too, if you don't want to bother with the buttons. Skipping the buttons leaves part of the reported issue open IMO so we either should leave the Jira issue open, or file a new one for the buttons.

@frankie139506
Copy link
Contributor Author

@daniel-beck I have made some change, please take a look.

@frankie139506 frankie139506 requested review from daniel-beck and removed request for NotMyFault September 1, 2022 02:57
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

It's probably best to revert the change to row.jelly and otherwise leave the PR as is, following up in a separate PR with the rest?

@frankie139506
Copy link
Contributor Author

frankie139506 commented Sep 1, 2022

Apologize, I cannot solved this problem at this moment. I have reverted the row.jelly. Thanks for your suggestion, if I have any idea I can push other PR to fix it.

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Sep 1, 2022
Copy link
Member

@NotMyFault NotMyFault 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 now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@NotMyFault NotMyFault added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 2, 2022
Co-authored-by: Alexander Brandes <brandes.alexander@web.de>
@frankie139506
Copy link
Contributor Author

Thanks for your suggestion.

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 6, 2022

Please take a moment and address the merge conflicts of your pull request. Thanks!

# Conflicts:
#	core/src/main/resources/hudson/PluginManager/available.jelly
#	core/src/main/resources/hudson/PluginManager/table.jelly
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 9, 2022
@timja timja merged commit 4796392 into jenkinsci:master Sep 14, 2022
timja added a commit that referenced this pull request Sep 14, 2022
…ere there's nothing to install (#7041)"

This reverts commit 4796392.
@timja timja added the skip-changelog Should not be shown in the changelog label Sep 14, 2022
@timja
Copy link
Member

timja commented Sep 14, 2022

@frankie139506 I've reverted this as the merge conflicts weren't resolved properly with #6783

the pages and titles were inconsistent and markup was incorrect in some places

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
5 participants