-
Notifications
You must be signed in to change notification settings - Fork 981
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
Fixes #27478 - extract onchange event handlers #6945
Fixes #27478 - extract onchange event handlers #6945
Conversation
Issues: #27478 |
app/assets/javascripts/host_edit.js
Outdated
$(document).on('change', '#host_hostgroup_id', function(evt) { | ||
hostgroup_changed(evt.target); | ||
}); | ||
$(document).on('change', '#host_compute_resource_id, #compute_profile', function(evt) { |
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.
are we certain there are no other matching elements anywhere in foreman or any of the plugins?
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.
should be as it is purpose of ID, but fair point... I can find some unique parent.
2726f4b
to
4ff77fd
Compare
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.
Thanks @ezr-ondrej
Looks like there are relevant failing tests:
https://ci.theforeman.org/job/test_develop_pr_core/1605/database=postgresql-integrations,ruby=2.5,slave=fast/testReport/junit/(root)/HostgroupJSTest/test_0004_parameters_change_after_parent_update/
$(document) | ||
.on('change', '.hostgroup-select', function(evt) { | ||
hostgroup_changed(evt.target); | ||
}).on('change', '.host-form-compute-resource-handle', function(evt) { |
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.
What's the difference between select
suffix and handle
suffix?
Aren't they all selects?
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.
The handle class is used more than once 🤔
I am terrible with naming... any ideas?
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.
What about something like this? (not sure if it's the right syntax)
}).on('change', '.host-form-compute-resource-handle', function(evt) { | |
}).on('change', '.host-form-compute-resource-select, .host-form-compute-profile-select', function(evt) { |
@@ -23,7 +23,7 @@ | |||
|
|||
<div class="tab-pane active" id="primary"> | |||
<%= select_f(f, :parent_id, parent_hostgroups, :id, :to_label, {:include_blank => true}, {:label => _('Parent'), | |||
:onchange => 'hostgroup_changed(this);', :'data-url'=> process_hostgroup_hostgroups_path, |
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.
Looks like the data-url
is needed here:
foreman/app/assets/javascripts/host_edit.js
Line 332 in 10c8905
var url = $(element).data('url'); |
It might be the reason https://ci.theforeman.org/job/test_develop_pr_core/1605/database=postgresql-integrations,ruby=2.5,slave=fast/testReport/junit/(root)/HostgroupJSTest/test_0004_parameters_change_after_parent_update/ is failing
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.
yeah, I meant to rewrite it, but it slipped my fingers 🙄 🤦♂️
(:arrow_down:)
4ff77fd
to
b516af1
Compare
@amirfefer can you take a look, so we can merge the webcomponents? |
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.
Tested, works but somehow .host-form-compute-resource-handle
's change event triggers right after other events (i.e after changing a taxonomy)
This seems to be intentional, not an regression and it got called only when compute resource is 'Bare Metal': foreman/app/assets/javascripts/host_edit.js Line 354 in b90f097
in such case the event is just ensuring the VM related fields are hidden. |
b516af1
to
322193f
Compare
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.
Thanks @ezr-ondrej LGTM !
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.
Thanks @ezr-ondrej
322193f
to
de83d76
Compare
[test foreman] |
merged, díky @ezr-ondrej! |
No description provided.