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

DialogRunner - fix miq_observe functionality on selectpickers #6476

Merged
merged 4 commits into from Feb 11, 2016
Merged

DialogRunner - fix miq_observe functionality on selectpickers #6476

merged 4 commits into from Feb 11, 2016

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Feb 2, 2016

depends on 9a828a4, which is the important part of the fix

this fixes initializeDialogSelectPicker to not assume trigger_auto_refresh by default
and calls that instead of miqInitSelectPicker + miqSelectPickerEvent (both are called from initializeDialogSelectPicker)

also, the data-miq_observe attribue is removed from drop_downs, since that functionality is handled by miqSelectPickerEvent for selectpickers

https://bugzilla.redhat.com/show_bug.cgi?id=1302331

depends on 9a828a4, which is the important part of the fix

this fixes initializeDialogSelectPicker to not assume trigger_auto_refresh by default
and calls that instead of miqInitSelectPicker + miqSelectPickerEvent (both are called from initializeDialogSelectPicker)

also selectedValue is made optional (because it is already handled by select_tag)

also, the data-miq_observe attribue is removed from drop_downs, since that functionality is handled by miqSelectPickerEvent for selectpickers

https://bugzilla.redhat.com/show_bug.cgi?id=1302331
@himdel himdel changed the title DialogRunner - fix miq_observe functionality on selectpickers [WIP] DialogRunner - fix miq_observe functionality on selectpickers Feb 2, 2016
@himdel
Copy link
Contributor Author

himdel commented Feb 2, 2016

wip because Uncaught SyntaxError: Unexpected token <

aand fixed in 30e5964

@miq-bot miq-bot added the wip label Feb 2, 2016
…e jQuery

this fixes `refreshRadioList` and `addOptionsToDropDownList` to always use jQuery to create the new elements (`option` or `input` + `label`).

That way, it won't break on options with angle brackets - like `{values: {refreshed_values: [[null, "<Script error>"]], checked_value: ""}}`.

Should be otherwise identical, except we're no longed passing the `onclick=` part of the onClickString argument (and passing it to new Function instead of creating the node with it).
@himdel himdel changed the title [WIP] DialogRunner - fix miq_observe functionality on selectpickers DialogRunner - fix miq_observe functionality on selectpickers Feb 2, 2016
@miq-bot miq-bot removed the wip label Feb 2, 2016
fixes failing tests, this is handled by initializeDialogSelectPicker

also removed duplicate :remote
@dclarizio
Copy link

@mfalesni can you try out this fix? Thx, Dan

@mfalesni
Copy link
Contributor

mfalesni commented Feb 8, 2016

@dclarizio I can try applying the commits to a 5.5 appliance.

@himdel
Copy link
Contributor Author

himdel commented Feb 8, 2016

@mfalesni If you do that, please make sure 30e5964 (not part of this PR) is included

@mfalesni
Copy link
Contributor

mfalesni commented Feb 9, 2016

So, the tag control is working again, however I get a lot of errors of field_changed and they are sporadic, I cannot reproduce them properly. If any dev would mind looking in my appliance and go through to check the errors ...

@dclarizio
Copy link

@himdel can you take a look at @mfalesni's environment?

@dclarizio
Copy link

@eclarizio can you also review this? Thx, Dan

@eclarizio
Copy link
Member

@himdel The only thing I'd like to see is an adjustment to dialog_field_refresh_spec.js to accommodate the addition of the triggerAutoRefresh parameter in the initializeDialogSelectPicker function, maybe another test ensuring if you pass false that the trigger auto refresh gets called with false. Since the rest of the changes in the dialog_field_refresh.js file are simply a cleaner refactoring, nothing else needs to change in the spec.

@himdel
Copy link
Contributor Author

himdel commented Feb 11, 2016

@eclarizio right, that's probably a good idea, will do :)

@himdel
Copy link
Contributor Author

himdel commented Feb 11, 2016

Added a test for just that :)

@miq-bot
Copy link
Member

miq-bot commented Feb 11, 2016

Checked commits https://github.com/himdel/manageiq/compare/8f45d8e8258195f6226c77db4ea52ba369f72753~...bd428c0c6a36b578326d36fd2b70a9bdf7c618ad with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
3 files checked, 0 offenses detected
Everything looks good. ⭐

@eclarizio
Copy link
Member

@himdel Cool, looks good 👍

@chessbyte
Copy link
Member

@dclarizio please review now that @eclarizio has given his 👍

dclarizio pushed a commit that referenced this pull request Feb 11, 2016
DialogRunner - fix miq_observe functionality on selectpickers
@dclarizio dclarizio merged commit 2f1ba45 into ManageIQ:master Feb 11, 2016
@dclarizio dclarizio added this to the Sprint 36 Ending Feb 16, 2016 milestone Feb 11, 2016
@himdel himdel deleted the bz1302331-dialog-select-picker branch February 12, 2016 13:44
lgalis pushed a commit to lgalis/manageiq that referenced this pull request Mar 17, 2016
dialog_field_refresh - don't construct new DOM elements textually, use jQuery

this fixes `refreshRadioList` and `addOptionsToDropDownList` to always use jQuery to create the new elements (`option` or `input` + `label`).

That way, it won't break on options with angle brackets - like `{values: {refreshed_values: [[null, "<Script error>"]], checked_value: ""}}`.

Should be otherwise identical, except we're no longed passing the `onclick=` part of the onClickString argument (and passing it to new Function instead of creating the node with it).

(cherry picked from commit 30e5964)

---

5.5 BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1312049  
upstream PR: ManageIQ#6476 (commit 30e5964)  
clean cherry-pick except for `__('text') -> 'text'` because we have no JS gettext in 5.5

See merge request !824
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants