Fixes BZ#2238952 - Selecting host group change form URL #609
Conversation
stejskalleos
force-pushed
the
ls/hosts_url
branch
2 times, most recently
from
b44b52e
to
117222b
Compare
There was a problem hiding this comment.
Looks quite good, kudos for documentation! a couple of readability comments:
I would reverse the conditions of the method though:
return discovered_host if controller == 'discovered_hosts'
return discovered_host if controller == 'hosts' && action == 'hostgroup' && Host::Discovered.exists?(id)
super
I think it's a bit more readable.
Also I would use exists? instead of find_by , it's much less memory consuming:
https://api.rubyonrails.org/v6.1.3/classes/ActiveRecord/FinderMethods.html#method-i-exists-3F
stejskalleos
force-pushed
the
ls/hosts_url
branch
from
117222b
to
e646fd9
Compare
| # The hostgroup onChange action is replacing the form url from `/discovered_hosts/:id` | ||
| # to `/hosts/:id``, which is not correct (in this case) and it breaks the form submission | ||
| return discovered_host_path(host) if controller_name == 'hosts' && | ||
| action_name == 'process_hostgroup' && |
There was a problem hiding this comment.
While I'm OK with specifying this particular method as it's the one that causes the bug, I wonder if we shouldn't include all the AJAX methods from the controller
There was a problem hiding this comment.
Maybe, but I'm worried about the impact it can have across the plugins. This fix is right now important for downstream release, can we go with it as it is and later on think about the refactoring? Dont want to block QAs who are waiting for this fix.
There was a problem hiding this comment.
Sounds good for me. Let's start with this, and deal with the fallout later.
|
The CI is broken, but not because of the changes in this PR, it's been red for a while. |
Fixed issue: