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

Make Save button respond properly on changes in NTP Servers section while editing a Zone #6162

Merged

Conversation

hstastna
Copy link

@hstastna hstastna commented Sep 6, 2019

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

Save button did not respond properly on changes in NTP Servers section while editing an existing Zone. Let me explain the fix. Here, the same as in other screens, values of @edit[:new] and @edit[:current] are crucial for detecting changes in the fields and response of Add/Save buttons. In our case, we need to take care of @edit[:new][:ntp] and @edit[:current][:ntp].
Beginning of editing a Zone:

[18] pry(#<OpsController>)> @edit[:new][:ntp]
=> {:server=>["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org"]}
[19] pry(#<OpsController>)> @edit[:current][:ntp]
=> {:server=>["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org"]}

After we make some change and put it back, the result will look like this:

[6] pry(#<OpsController>)> @edit[:new][:ntp]
=> {:server=>["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org"], "ntp_server_2"=>"1.pool.ntp.org"}
[7] pry(#<OpsController>)> @edit[:current][:ntp]
=> {:server=>["0.pool.ntp.org", "1.pool.ntp.org", "2.pool.ntp.org"]}

We are adding something extra to @edit[:new][:ntp]. We always just add something, not remove. And this is core of the problem why Save button remained always enabled if we touched anything in NTP Servers section, no matter we really made a change or not.

So we need to remove unnecessary key "ntp_server_2" from @edit[:new][:ntp] to achieve proper response of Save button. And as we can see that the field/server numbers in the loop are from 1 to 3, we have to look at field_num - 1th item in the array of @edit[:new][:ntp][:server] to remove the right key from @edit[:new][:ntp] as the second position (field_num == 2) in array is in position 1, obviously.

After: (Save button becomes disabled after we put back the original values in fields in NTP Servers)
ntp_after

@hstastna
Copy link
Author

hstastna commented Sep 6, 2019

@miq-bot add_label bug, ivanchuk/yes, hammer/yes

@hstastna hstastna force-pushed the Save_button_NTP_Servers_edit_Zone branch 2 times, most recently from e64c3d3 to 94d5617 Compare September 9, 2019 09:28
Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested changes, fixes the issue. 👍

@@ -806,6 +806,8 @@ def settings_get_form_vars
field = "ntp_server_#{field_num}"
next unless params.key?(field)
@edit[:new][:ntp][field] = params[field]
# remove unnecessary key from @edit[:new][:ntp] if there is no change
@edit[:new][:ntp].except!(*field) if params[field] == @edit[:new][:ntp][:server][field_num - 1]
Copy link
Member

Choose a reason for hiding this comment

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

the asterisk does not seem necessary to me

Copy link
Author

Choose a reason for hiding this comment

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

The change is done.

@hstastna hstastna force-pushed the Save_button_NTP_Servers_edit_Zone branch from 94d5617 to bb7d490 Compare September 9, 2019 15:19
@miq-bot
Copy link
Member

miq-bot commented Sep 9, 2019

Checked commit hstastna@bb7d490 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@h-kataria h-kataria self-assigned this Sep 9, 2019
@h-kataria h-kataria added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 9, 2019
@h-kataria h-kataria merged commit 0779432 into ManageIQ:master Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants