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

Fixes #27764, #27798, #27391 - host form update on change #7023

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@ezr-ondrej
Copy link
Member

ezr-ondrej commented Sep 8, 2019

We need to select host form more reliably as there are cases where some components are adding subforms.

@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Sep 8, 2019

There were the following issues with the commit message:

  • 2f65ad1 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:fix_host_form branch from 2f65ad1 to a583bec Sep 8, 2019
@ezr-ondrej ezr-ondrej changed the title Fix #27798 - host form update on change Fixes #27798 - host form update on change Sep 8, 2019
@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Sep 9, 2019

Nested forms are invalid HTML. This sounds like it should be changed where we add forms inside the form.

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

ezr-ondrej commented Sep 9, 2019

Nested forms are invalid HTML. This sounds like it should be changed where we add forms inside the form.

I agree, but that is going to take some time as it is pattern-fly pagination component.

Copy link
Member

tbrisker left a comment

Can you open an issue to PF to address the issue for the long term? I think there other places where we select the host form that may need updates.

@@ -4,7 +4,7 @@
<%= render "hosts/progress" %>

<% Taxonomy.as_taxonomy @organization , @location do %>

<div id="host_form">
<%= form_for @host, :url => (@host.new_record? ? hosts_path : host_path(:id => @host.id)), :html => {:data => {:id => @host.try(:id), :type_changed => !!@host.type_changed?, :submit => 'progress_bar'}} do |f| %>

This comment has been minimized.

Copy link
@tbrisker

tbrisker Sep 9, 2019

Member

I think it would be better to add an id to the form instead of wrapping it with another div that may affect layout

@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Sep 9, 2019

Looks like this also fixes #27764, please update the commit message to to Fixed #27764, #27798 ... so it gets attached and closed.

@ezr-ondrej ezr-ondrej changed the title Fixes #27798 - host form update on change Fixes #27764, #2779, #27798 - host form update on change Sep 9, 2019
@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:fix_host_form branch 2 times, most recently from 091c336 to 7aa9241 Sep 9, 2019
@ezr-ondrej ezr-ondrej changed the title Fixes #27764, #2779, #27798 - host form update on change Fixes #27764, #27798 - host form update on change Sep 9, 2019
@@ -4,7 +4,7 @@
<%= render "hosts/progress" %>

<% Taxonomy.as_taxonomy @organization , @location do %>

<div id="host_form">
<%= form_for @host, :url => (@host.new_record? ? hosts_path : host_path(:id => @host.id)), :html => {:data => {:id => @host.try(:id), :type_changed => !!@host.type_changed?, :submit => 'progress_bar'}} do |f| %>

This comment has been minimized.

Copy link
@tbrisker

tbrisker Sep 9, 2019

Member
Suggested change
<%= form_for @host, :url => (@host.new_record? ? hosts_path : host_path(:id => @host.id)), :html => {:data => {:id => @host.try(:id), :type_changed => !!@host.type_changed?, :submit => 'progress_bar'}} do |f| %>
<%= form_for @host, :url => (@host.new_record? ? hosts_path : host_path(:id => @host.id)), :html => {:id => 'host_form', :data => {:id => @host.try(:id), :type_changed => !!@host.type_changed?, :submit => 'progress_bar'}} do |f| %>

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Sep 9, 2019

Author Member

You want only one space? 👀

This comment has been minimized.

Copy link
@tbrisker

tbrisker Sep 9, 2019

Member

no, i want the id on the form instead of a wrapper div :)

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Sep 9, 2019

Author Member

ah, it was too far xD I didn't see.
What about this?

@ezr-ondrej ezr-ondrej changed the title Fixes #27764, #27798 - host form update on change Fixes #27764, #27798, #27391 - host form update on change Sep 9, 2019
@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:fix_host_form branch from 7aa9241 to 356bc32 Sep 9, 2019
Copy link
Member

tbrisker left a comment

please also update the selectors in other functions using the form selector. Also, this will break the hostgroup form which uses the same js code - so we need to use a shared class on both forms.

@ezr-ondrej ezr-ondrej force-pushed the ezr-ondrej:fix_host_form branch from 356bc32 to 69130b2 Sep 9, 2019
@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

ezr-ondrej commented Sep 9, 2019

I've made the shared class, updated other occurences.

Copy link
Member

tbrisker left a comment

Thanks @ezr-ondrej ! tested and seems to fix all the issues.

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

ezr-ondrej commented Sep 9, 2019

💚

@tbrisker tbrisker merged commit 16976e1 into theforeman:develop Sep 9, 2019
7 checks passed
7 checks passed
Hound No violations found. Woof!
Redmine issues Valid issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished. 38253 tests run, 15 skipped, 0 failed.
Details
katello Build finished. 4519 tests run, 10 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
upgrade Build finished. No test results found.
Details
@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Sep 9, 2019

1.23 - 17b48be

@ezr-ondrej

This comment has been minimized.

Copy link
Member Author

ezr-ondrej commented Sep 9, 2019

@tbrisker the issue seems to be in 1.22 already according to discourse thread, should we backport it to 1.22 as well?

@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Sep 10, 2019

@ezr-ondrej can you open a cherry-pick pr to 1.22-stable? this did not cleanly apply to 1.23, and i expect 1.22 would be even more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.