-
Notifications
You must be signed in to change notification settings - Fork 897
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
Fix saving network manager in belongsto filter #18504
Fix saving network manager in belongsto filter #18504
Conversation
app/models/miq_filter.rb
Outdated
def self.belongsto2object(tag) | ||
belongsto2object_list(tag).last | ||
end | ||
|
||
def self.find_object_by_special_class(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both these methods, it would be more generic if you can inspect the name here to determine the class and return an error if it's not one of the supported "special" classes.
7b9790e
to
35ad01c
Compare
@gtanzillo I did couple changes as you pointed out. In addition I found the real reason for this fix. |
app/models/miq_filter.rb
Outdated
def self.belongsto2object(tag) | ||
belongsto2object_list(tag).last | ||
end | ||
|
||
def self.find_descendant_class_by(klass, name) | ||
if ALLOWED_DESCENDANT_CLASSES_FROM_MODEL.include?(klass.to_s) && name.end_with?(ManageIQ::Providers::NetworkManager::PROVIDER_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@libor Perhaps this logic can be moved into the NetworkManager
class and this code can be something like
if ALLOWED_DESCENDANT_CLASSES_FROM_MODEL.include?(klass.to_s) && klass.respond_to?(:belongsto_descendant_class)
return klass.belongsto_descendant_class(name)
else
...
The in NetworkManager
def belongsto_descendant_class(name)
ManageIQ::Providers::NetworkManager if name.end_with?(ManageIQ::Providers::NetworkManager::PROVIDER_NAME)
end
This way the provider specific code doesn't have to live in Filterer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, I followed pattern but I need to put to the ExtManagementSystem
because klass is ExtManagementSystem
, thanks!
…n MiqFillter methods handling with this special case behaviour has added also
35ad01c
to
709ff9e
Compare
Checked commits lpichler/manageiq@f069348~...709ff9e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
belongsto filters are set in group in Host & Cluster tab:
In this filter are listed Providers, Hosts, Storage managers and also Network Providers.
But when you checked any network provider then it was stored to the group but it was not reflected in UI because the method MiqFilter#belongsto2object_list did not work properly with network providers.
This PR fixes method MiqFilter#belongsto2object_list to count also with network providers.
Links
@miq-bot assign @gtanzillo
@miq-bot add_label bug