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-48516] Uniquify distinct groups of radio buttons #55

Closed
wants to merge 2 commits into from

Conversation

@dwnusbaum
Copy link
Member

commented Oct 4, 2018

See JENKINS-48516.

Right now, if you have multiple shared libraries defined, when you load the configuration page, only the last library's SCM settings will be visible.

The reason for this seems to be that the names of radio button groups are not distinct for each library. repeatable.js tries to add a prefix to uniquify radio buttons in distinct repeatable-chunks, but here we have ambiguity even within a single repeatable-chunk, and I'm not sure if the repeatable.js code works correctly when loading existing data vs adding new radio buttons to a live page dynamically using repeatable:add buttons (Based on the generated HTML I'd think that there should be no issue, so it seems like the JS-based uniquifying might happen too late to fix dupes during the initial page load).

I'd like to upstream this fix into core (EDIT: jenkinsci/jenkins#3672) for all f:descriptorRadioLists, since I don't think it would ever hurt, but I am opening this PR independently since I assume we won't want to bump our core dependency to the bleeding edge for a fix like this.

@reviewbybees

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

Currently trying to create an HtmlUnit test for this bug, but am having issues getting it to fail before the change. If I can't get a regression test working by tomorrow I will probably give up on the test.

@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2018

Here is a Gist of the test I created. For some reason, the SCM configurations seem to be displayable even without this fix (also verified by dumping the raw HTML and checking that their enclosing <tr> did not have style="display: none;"), even though when loaded in an actual browser (Latest versions of Chrome, Firefox, and Safari), the <tr> is not visible and has style="display: none;". Maybe just an HtmlUnit quirk, but I don't think it's worth spending more time trying to create a test for this.

@dwnusbaum dwnusbaum requested a review from rsandell Oct 4, 2018
@dwnusbaum

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2018

Given that the fix was backported to the 2.138 LTS line in 2.138.3, I don't think it's worth adding this workaround here. Users on 2.138.3 or higher will get the bugfix automatically.

@dwnusbaum dwnusbaum closed this Nov 9, 2018
@dwnusbaum dwnusbaum deleted the dwnusbaum:JENKINS-48516 branch Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.