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-32827] Add possibility to hide form elements #9

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

cpoenisch
Copy link

  • added new script option to configure the visibility of parameters
  • visibility script must return a boolean value, true by default
  • allow to use referenced parameters for cascade updates
  • added some unit tests

- added new script option to configure the visibility of parameters
- visibility script must return a boolean value, true by default
- allow to use referenced parameters for cascade updates
- added some unit tests
@kinow
Copy link
Member

kinow commented Jun 14, 2016

Code well written, simple to review, and with updated tests. Will try to include it in the next development cycle for review.

Thanks!!

@cpoenisch
Copy link
Author

Is there a plan for the next plugin release including this PR?

@kinow
Copy link
Member

kinow commented Oct 25, 2016

Hi @cpoenisch , planning a release in the next week or the week after that.

Could you explain why you needed a visibility script? I would prefer to keep the current script, plus the fallback script, with an option under Advanced if possible.

@cpoenisch
Copy link
Author

Hi @kinow, we need the visibility option to hide some parameters dependent on previously selected parameter values in order to provide a more wizard-like user experience.
Moving the visibility script in Advanced section would be okay for me.

@kinow
Copy link
Member

kinow commented Mar 12, 2017

Maybe we could have a new Wizard parameter type? Won't be able to include this issue in 1.5.4 release (scheduled for next weeks). Just finished 1.5.3; if everything goes well with 1.5.4, I'll try to either cut a 1.5.5 with remaining issues, or 1.6 with new features like this one. Thanks!

@sebastienbonami
Copy link

Any news on this? @cpoenisch @kinow

@StKob
Copy link

StKob commented Mar 13, 2019

@cpoenisch @kinow Any progress on this?

@cpoenisch
Copy link
Author

@sebastienbonami @StKob Sorry for my late response!

I rebased with the current master, improved some stuff and added more tests.

@kinow Could you please have a look on this and merge, if applicable?

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Hi @cpoenisch , I think something went wrong while merging your changes with latest changes from master. Could you please take another look and try to rebase your changes onto master? The pom.xml and other files (36 changed files in the current PR) make it a bit harder to see what changed and what's current already.

@peter4431
Copy link

Any news on this? @cpoenisch @kinow

@kinow
Copy link
Member

kinow commented Feb 25, 2020

Hi @peter4431 I haven't reviewed yet. Last time I had some spare time to go around looking for issues & PR's, this one had conflicts. Once they are fixed once I get another development cycle for this plugin I will try to take a look and include in a next release 👍

@maxamj
Copy link

maxamj commented Jun 7, 2020

Hello @kinow . There were problems with the Active choice plugin after upgrading to 2.3. If you select from the field, select the desired text. If I use input, then it does not display it. in version 2.2 it worked.
This my conf and result:
<------------------------------------------------------------------------------->
switch(branch){
case~/Test/:
winerec='-----------------------'
return "${winerec}"
break
case ~/Release/:
winerec='<input name="value" type="text" class="setting-input " value="">'
return "${winerec}"
break
}
<------------------------------------------------------------------------------->
If i switch TEST ->
image
All is fine. But if i switch RELEASE, plugin don't show . in 2.2 it's worked.
image
Does this problem of 2.3?

@stevenacalhoun
Copy link

This would be a very helpful feature to add, would. love to see this cleaned up and released

@kinow
Copy link
Member

kinow commented Nov 1, 2020

Hello @kinow . There were problems with the Active choice plugin after upgrading to 2.3. If you select from the field, select the desired text. If I use input, then it does not display it. in version 2.2 it worked.
This my conf and result:
<------------------------------------------------------------------------------->
switch(branch){
case~/Test/:
winerec='-----------------------'
return "${winerec}"
break
case ~/Release/:
winerec=''
return "${winerec}"
break
}
<------------------------------------------------------------------------------->
If i switch TEST ->
image
All is fine. But if i switch RELEASE, plugin don't show . in 2.2 it's worked.
image
Does this problem of 2.3?

Hi @maxamj,

I think this is not directly related to this PR. The biouno mailing list, or the jenkins mailing list, or a separate issue in JIRA with detailed info to reproduce, may be best places to discuss that.

@kinow
Copy link
Member

kinow commented Nov 1, 2020

This would be a very helpful feature to add, would. love to see this cleaned up and released

Hi @stevenacalhoun, I am working on some issues that could be regressions after enforcing groovy script security in 2.5.1, and trying to squeeze in some other issues & pull requests. Thanks for the comment here, as I had not seen this PR.

@cpoenisch might be busy (which is not an issue, as he already spent some time with this great contribution!), so I will try to rebase the changes myself, and remind me of the context for this issue. From what I recall, it just needed some reviewing to confirm no regressions added, old behavior was still there, and the new behavior wouldn't surprise users.

Feel free to ping me if no action in the next 2/3 weeks (aiming at a release of 2.5.2 in December, or by the last week of November).

Bruno

@scorpion35
Copy link

This would be a very helpful feature to add, would. love to see this cleaned up and released

Hi @stevenacalhoun, I am working on some issues that could be regressions after enforcing groovy script security in 2.5.1, and trying to squeeze in some other issues & pull requests. Thanks for the comment here, as I had not seen this PR.

@cpoenisch might be busy (which is not an issue, as he already spent some time with this great contribution!), so I will try to rebase the changes myself, and remind me of the context for this issue. From what I recall, it just needed some reviewing to confirm no regressions added, old behavior was still there, and the new behavior wouldn't surprise users.

Feel free to ping me if no action in the next 2/3 weeks (aiming at a release of 2.5.2 in December, or by the last week of November).

Bruno

Hey @kinow! wondering if you have an update on releasing this fix. Would be extremely useful for us and many others I believe.

@thehustler088
Copy link

Hi @kinow do you have an update releasing this fix. This is very useful feature. Thanks!

@kinow
Copy link
Member

kinow commented Dec 20, 2020

Hi, no updates yet from me. I'm working on other issues, and will see if I am able to include this one in the next release. I normally cut a release after x-mas or new year during my vacation, but there is quite a backlog this time, sorry. Bruno.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

The git conflicts do not really worry me as the code was well written, and it's easy to spot what was the intention of the PR.

My only concern is adding one more script, now to handle visibility. We could have other scripts to handle whether the parameter is enabled or not. Or to style it. That would eventually grow to a great number of scripts, which I think would be harder to maintain, than a single script.

Ideally, IMO, the simplest way to add this feature would be to keep using the normal Groovy script, but have a way to indicate that the parameter needs to be hidden. It could be by throwing a special exception, like DisabledParameterException; but that would mean misusing exceptions — they are supposed to denote an exceptional behavior, or error. Or we could have a global special value returned by the script, like return PARAMETER_DISABLED.

My point is that one more script to maintain (in Scriptler, or in git, XML backups of Jenkins) may be troublesome to some users. Out-weighting the benefits of the new feature. I will think of more alternatives, and pros and cons. But I think we can have a workaround for this in pure JavaScript as well (will have a try).

@kinow
Copy link
Member

kinow commented Dec 26, 2020

Alas I couldn't come up with a workaround. First tried JS code to hide the parameter, but we have limited HTML attributes allowed in the element (for security reasons). And then tried a CSS selector, but it's tricky to select the parent element.

I can hide the parameter completely with a Formatted Hidden HTML. But I cannot dynamically hide and display the element 😕

@Diskiddin
Copy link

Kinda reviving this discussion as this functionality would be very useful.

@kinow I've read through this discussion and took a good look at the code. While I understand the sentiment for sticking to one script, wouldn't increasing the scope of the single script increase testing complexity? Having separate functionally distinct scripts allows for better coverage in tests without adding complexity.

Just my two cents, what're your thoughts?

@kinow
Copy link
Member

kinow commented Mar 19, 2022

Kinda reviving this discussion as this functionality would be very useful.

Thanks for doing that. I also think this is a useful functionality, that I think can be achieved with the dynamic reference parameter and JavaScript (I think @imoutsatsos showed me that before) but definitely not user-friendly.

@kinow I've read through this discussion and took a good look at the code. While I understand the sentiment for sticking to one script, wouldn't increasing the scope of the single script increase testing complexity? Having separate functionally distinct scripts allows for better coverage in tests without adding complexity.

Just my two cents, what're your thoughts?

I think it'd be a lot easier to maintain the plug-in with a single script (even the fallback script won't be necessary in a future version if we provide clear user error information on the UI for the user, IMO). Adding a new script means changing constructors, having to keep old configuration working (which we did not handle well in 2.6.0 with DSL, that's why 2.6.1 was released after a user fixed in a pull request an issue that had been annoying other users.)

So I'm +1 for the functionality, but the implementation would have to be different. I'm strapped for time these days because of $work and other OSS where I'm sponsored, but whenever I see a pull request (or helpful comments like yours, reviving an important thread) I always try to take some time and reply, or merge & cut a release.

If we could come up with a modified version of this PR I could review/merge/release it pretty quickly 👍

Bruno

p.s. I'm also open to having other devs joining as maintainers, but first I'd expect them to contribute via PR's & issues, then I'd contact Jenkins admin to add a new maintainer. That'd remove me as bottleneck in some issues, and also have more people working on decisions like this.

@Diskiddin
Copy link

I think it'd be a lot easier to maintain the plug-in with a single script (even the fallback script won't be necessary in a future version if we provide clear user error information on the UI for the user, IMO). Adding a new script means changing constructors, having to keep old configuration working (which we did not handle well in 2.6.0 with DSL, that's why 2.6.1 was released after a user fixed in a pull request an issue that had been annoying other users.)

That makes a lot more sense. I don't have that much experience in this development area so thanks for explaining it a bit more.

p.s. I'm also open to having other devs joining as maintainers

I don't have much spare time as well, but if I do get the chance I would like to give this my best try. Do you think you could point me towards some development resources to help me contribute to this issue? I've frankly never developed a Jenkins plugin before.

Thanks!

@kinow
Copy link
Member

kinow commented Mar 21, 2022

Hi @Diskiddin

I think the first step would be to imagine a simpler way to achieve the same feature. Then what I normally do is try to copy existing code. If you imagine a checkbox being used, then just look at an existing checkbox (it probably involves Java and Jelly/XML code). Then modify the code locally, and mvn clean install and mvn hpi:run to start the server in development mode. Look at errors, test it, iterate.

That's my workflow. It takes a while to get used to the Jenkins API, and the concepts like databoundconstructor, Jelly, Stapler, etc. But for this change I think one might be able to ignore those aspects (or just abstract them.)

Cheers
Bruno

@imoutsatsos
Copy link
Member

@kinow reading the original Jenkins-32827 issue, I am confused as to whether the requirement is for a conditionally visible AC parameter, or for one that is always hidden. I have dozens of jobs with always or conditionally hidden Reactive AC Reference parameters that perform all kinds of functions. Many don't even use have any referenced parameters! So could somebody please, help me to clarify the requirement. It seems that a reactive AC Reference parameter already does what is suggested here, but then again I may be missing something. Thank you

@kinow
Copy link
Member

kinow commented Mar 22, 2022

Hi @imoutsatsos I think the way you hide/show a parameter if via JavaScript? I believe the idea of the issue & PR was to have a configuration setting to toggle when the user wanted a parameter to be visible or not in the form with parameters. If we are able to show/hide any parameter, then maybe this feature only needs to be documented.

@imoutsatsos
Copy link
Member

Hi @imoutsatsos I think the way you hide/show a parameter if via JavaScript?

@kinow It is typically a combination of Choice Type: Formatted Hidden HTML and the hidden attribute of HTML elements. Not so much JavaScript, although I suppose it could be done through that. I think posting some examples of the desired functionality would be useful and reduce the confusion

@mandruis7
Copy link

It is typically a combination of Choice Type: Formatted Hidden HTML and the hidden attribute of HTML elements.

Hi @imoutsatsos ,

Do you still have by any chance an example of this done in Groovy or .xml? Because using Active Choices I'm able to hide input of the value but not he parameter name itself.

Example:
image
image

@ShIRannx
Copy link

Any news on this? @kinow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet