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

Disable dependents toggle in plugin manager with system read #9463

Merged

Conversation

timja
Copy link
Member

@timja timja commented Jul 13, 2024

It was reported as jenkinsci/dark-theme-plugin#258 although nothing to do with dark theme.
One of the tds has some metadata added with display:none which was being excluded by the isAdmin check.

This narrows the isAdmin down so dependents and dependencies correctly show up in the metadata

Testing done

Tested with system read:

image

Tested with admin:

image

Proposed changelog entries

  • Correct styling for plugins that can't be disabled in plugin manager when user has system read permission.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

N/A

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

Maintainer checklist

@timja timja requested a review from NotMyFault July 13, 2024 22:26
@timja timja added the needs-security-review Awaiting review by a security team member label Jul 13, 2024
@timja timja requested review from a team July 13, 2024 22:27
@timja timja added the bug For changelog: Minor bug. Will be listed after features label Jul 13, 2024
@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 Jul 15, 2024
@NotMyFault
Copy link
Member

Tests are failing

@NotMyFault NotMyFault removed the request for review from a team August 13, 2024 05:34
@NotMyFault NotMyFault added the needs-test-fix One or more test case(s) need to be updated label Aug 24, 2024
checkbox.addEventListener("click", () => {
setTimeout(() => {
updateButton.disabled = !anyCheckboxesSelected();
if (updateButton) {
Copy link
Member Author

Choose a reason for hiding this comment

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

minor fix I noticed while checking this again, tests were logging a giant stacktrace, this button is only on the updates page and not the installed page so its event listener shouldn't be active

@timja timja removed the needs-test-fix One or more test case(s) need to be updated label Aug 26, 2024
@timja timja requested review from NotMyFault, a team and mawinter69 and removed request for NotMyFault and a team August 26, 2024 21:08
@timja timja requested a review from a team August 29, 2024 19:11
@timja
Copy link
Member Author

timja commented Sep 4, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot 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 4, 2024
@timja timja merged commit 525a8be into jenkinsci:master Sep 7, 2024
16 checks passed
@timja timja deleted the system-read-correctly-disable-dependents branch September 7, 2024 19:19
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants