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 #37068 - fix redirect after provisioning discovered host #616

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

nofaralfasi
Copy link
Contributor

Redirect to the correct page after provisioning discovered hosts, based on user settings.

@stejskalleos
Copy link
Contributor

@nofaralfasi Related failed tests:

Failure:
DiscoveredHostsControllerTest#test_edit_form_quick_submit [foreman_discovery/test/functional/discovered_hosts_controller_test.rb:104]
Minitest::Assertion: Expected response to be a redirect to <http://test.host/hosts/macaabbccddeeff.example12.com> but was a redirect to <http://test.host/new/hosts/macaabbccddeeff.example12.com>.
Expected "http://test.host/hosts/macaabbccddeeff.example12.com" to be === "http://test.host/new/hosts/macaabbccddeeff.example12.com".

This one is failing all the time, I need to take a look at it:

Failure:
HostDiscoveredTest#test_0024_should refresh facts and NICs of an existing discovered host [foreman_discovery/test/unit/host_discovered_test.rb:274]
Minitest::Assertion: Expected: "Dishwasher DW400"
  Actual: "IBM System x -[78[70](https://github.com/theforeman/foreman_discovery/actions/runs/7602436753/job/20703002127?pr=616#step:13:71)K4G]-"

@stejskalleos
Copy link
Contributor

Can you please rebase? The other failing test has been fixed

@@ -83,7 +83,8 @@ def perform_update(host, success_message = nil)
::ForemanDiscovery::HostConverter.set_build_clean_facts(host)
::ForemanDiscovery::HostConverter.unused_ip_for_host(host)
if host.save
success_options = { :success_redirect => host_path(host), :redirect_xhr => request.xhr? }
host_path = Setting['host_details_ui'] ? helpers.host_details_page_path(host) : host_path(host)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
host_path = Setting['host_details_ui'] ? helpers.host_details_page_path(host) : host_path(host)
host_path = Setting['host_details_ui'] ? host_details_page_path(host) : host_path(host)

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

One nit, otherwise works as expected

Redirect to the correct page after provisioning discovered hosts,
based on user settings.
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

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

🍏 LGTM

Failing tests are not related, fixed in #619

@stejskalleos stejskalleos merged commit 1a11647 into theforeman:develop Jan 25, 2024
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants