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 #36869 - Redirect /hosts to /new/hosts in index #9879

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

parthaa
Copy link
Contributor

@parthaa parthaa commented Oct 26, 2023

Also removed logic in the hosts controller and application helper to rewrite the URL. Decided to go with the redirect on index approach instead since we want this redirect to happen only on get requests.

Test this

  • Go to Administer-> Settings -> New Hosts Page. Set that to Yes
  • Go to http://<satellite>/hosts and notice that it should get redirected to http://<satellite>/new/hosts
  • Make operations like create/update/delete hosts all work ok.

ekohl
ekohl previously requested changes Oct 27, 2023
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I'm not a fan of this. With the host detail page we took an approach of making sure the links to it were correct, but still retained the old detail page as a fallback. This implementation means you effectively disable the whole old host page. If you go that route, why don't you make /hosts return the new content?

Comment on lines 50 to 53
if Setting[:new_hosts_page]
redirect_to(new_hosts_index_page_path)
next
end
Copy link
Member

Choose a reason for hiding this comment

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

If you do this, why can't it be at at the very beginning? So line 42.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to try your approach. @jeremylenz what do you think ?

@jeremylenz
Copy link
Contributor

Well, for what it's worth, I just tested this and it unbreaks host create. So 👍 in that regard.

@parthaa parthaa force-pushed the 36869 branch 2 times, most recently from 2c65e61 to 344051d Compare October 31, 2023 20:13
jeremylenz
jeremylenz previously approved these changes Oct 31, 2023
Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

LGTM

Host create works
also did some spot checks and links go to the correct place.

APJ (and @ekohl) 👍

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I was late on the other PR, but one thing I really didn't like about the new host detail page was the URL change.

In your current implementation you're treating the pages as if they're equal: you can go to either one or the other. Can't we somehow implement the page in the controller to either render the old or the new version? Either based on preference or cookie.

The thing I'm mostly worried about is this scenario:

  • Have the setting set to prefer the new overview
  • Go to the old overview
  • Use a bulk action
  • Expect to be redirect back to old overview

In reality this will redirect to the new overview.

Another case I fear about is plugins. They may redirect to the hosts overview as well.

@@ -684,11 +684,11 @@ def statuses
render :json => statuses
end

def hosts_path(*args)
def current_hosts_path(*args)
Copy link
Member

Choose a reason for hiding this comment

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

Don't controllers include all helpers? So isn't this duplicating the current_hosts_path from ApplicationHelper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it use helpers.current_hosts_path instead

@@ -423,11 +423,11 @@ def current_host_details_path(host)
Setting['host_details_ui'] ? host_details_page_path(host) : host_path(host)
end

def hosts_path(*args)
def current_hosts_path(*args)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: is current the best naming? Perhaps preferred is better, though it's also not great either.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is consistent with current_host_details_path which was already there. 🤷

@@ -55,14 +55,14 @@ def global_statuses
query = send("#{status_name}_total_query")
return if query.empty?

Rails.application.routes.url_helpers.hosts_path(search: query)
Rails.application.routes.url_helpers.current_hosts_path(search: query)
Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

@parthaa
Copy link
Contributor Author

parthaa commented Oct 31, 2023

The thing I'm mostly worried about is this scenario:

  • Have the setting set to prefer the new overview
  • Go to the old overview
  • Use a bulk action
  • Expect to be redirect back to old overview

You bring up an interesting idea here.

May be we could try to be more conservative here instead of doing the s/hosts_path/current_hosts_path en masse. Basically have host_controllers always return hosts_path unless the new hosts page implements those actions? . So for example right now only delete/create/change content source actions are available in New Hosts. You 'd have to go to old hosts for the rest of the actions.

Actually I just realized most of the actions on host_controller do what you were suggesting. Something like redirect_back_or_to hosts_path already.
https://github.com/theforeman/foreman/blob/develop/app/controllers/hosts_controller.rb#L446 for example.

Which then runs

    def redirect_back(fallback_location:, allow_other_host: true, **args)
      referer = request.headers["Referer"]
      redirect_to_referer = referer && (allow_other_host || _url_host_allowed?(referer))
      redirect_to redirect_to_referer ? referer : fallback_location, **args
    end

This would mean that if the referer pointed to the old page they'd be redirected back to that and the new url would only be a fall back

@parthaa
Copy link
Contributor Author

parthaa commented Nov 1, 2023

@ekohl any thoughts on ^ ?

@ekohl
Copy link
Member

ekohl commented Nov 16, 2023

That sounds like it would be less of a problem than I was afraid of, which is good.

@parthaa
Copy link
Contributor Author

parthaa commented Nov 21, 2023

@jeremylenz can you run a quick test again on this and ack if it works

@jeremylenz
Copy link
Contributor

[test unit]

jeremylenz
jeremylenz previously approved these changes Nov 21, 2023
Copy link
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Tested host create again and spot checked a few links, still works well for me

@jeremylenz
Copy link
Contributor

[test unit]

2 similar comments
@parthaa
Copy link
Contributor Author

parthaa commented Nov 21, 2023

[test unit]

@parthaa
Copy link
Contributor Author

parthaa commented Nov 22, 2023

[test unit]

@jeremylenz jeremylenz merged commit 3ecce16 into theforeman:develop Nov 28, 2023
9 checks passed
@parthaa parthaa deleted the 36869 branch November 28, 2023 20:05
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.

3 participants