-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Application of validation template applies all users - fix #21230 #21262
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
base: 11.0/bugfixes
Are you sure you want to change the base?
Conversation
$("#dropdown_items_id_target{{ rand }}").val(data.items_id_target); | ||
$("#dropdown_items_id_target{{ rand }}").trigger('change'); | ||
} else { | ||
$("#dropdown_items_id_target{{ rand }}").trigger('setValue', data.items_id_target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setValue
was added as an event for another reason besides just grouping .val()
and .trigger('change')
into a single call.
A better solution is fixing the handling of the event listener(s) for setValue
in common.js
or changing each call to trigger the event so the array is packed within another array or an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that making setValue
able to handle single values and also arrays is a good point.
Function able to do lots of things results in more complex code, hard to maintain. Maybe implementing a new method for array is a solution (it depends if there is more than a couple of candidates to use it).
I think this core change can be postponed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't a separate thing. It is setting a value and triggering the change event regardless of if it is one selection or multiple. The only part of setValue handling that seems different to me is one listener that triggers an AJAX call. I haven't looked too much at it to know what it is doing exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree on the general principal that fixing/improving the js core is the best general bet, I think making a change that has possible impact on all glpi is not what we should target now. I guess you should open another issue for that point, for later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this issue may affect any dropdown, a global fix should be made to make the setValue
handlers compatible with multiple values.
Checklist before requesting a review
Please delete options that are not relevant.
fix #21230