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

Restrict quadicon class demodulization for Physical* records only #3980

Merged
merged 2 commits into from May 24, 2018

Conversation

skateman
Copy link
Member

@skateman skateman commented May 23, 2018

The demodulization introduced by @saulotoledo breaks the quadicon settings key selection for templates. By restricting the demodulization to the models starting with Physical this issue can be solved easily. However, the whole quadicon settings key logic should be refactored in the future.

Before:
screenshot from 2018-05-23 13-00-03

After:
screenshot from 2018-05-23 12-58-21

@miq-bot add_label bug, gtls, gaprindashvili/no
@miq-bot add_reviewer @epwinchell

@miq-bot
Copy link
Member

miq-bot commented May 23, 2018

@skateman 'saulotoledo' is an invalid reviewer, ignoring...

@saulotoledo
Copy link
Member

saulotoledo commented May 23, 2018

Thank you for adding me here, @skateman !

@epwinchell, I agree with the fix. There is already another change in the Physical Switches model that can help here. We just need to have the following PRs approved:

I am not sure about how much time we need to aprove them, but if this PR is approved before I can ask @EsdrasVP to do the refactoring (i.e. remove the lines added here) in that code.

@@ -70,7 +70,7 @@ def self.settings_key(klass, layout)
when "ems_physical_infra", "ems_cloud", "ems_network", "ems_container"
layout.to_sym
end
elsif klass.base_model.name != klass.name.demodulize
elsif klass.name.demodulize.starts_with?("Physical") && klass.base_model.name != klass.name.demodulize
Copy link
Member

Choose a reason for hiding this comment

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

I agree with that if we remove later at #3954
There is a dependency in the core there. So the current PR can be approved faster.

Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

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

Tested. Looks good

@miq-bot
Copy link
Member

miq-bot commented May 24, 2018

Checked commits skateman/manageiq-ui-classic@931f94a~...ce857b1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@mzazrivec mzazrivec self-assigned this May 24, 2018
@mzazrivec mzazrivec added this to the Sprint 87 Ending Jun 4, 2018 milestone May 24, 2018
@mzazrivec mzazrivec merged commit 1d35266 into ManageIQ:master May 24, 2018
@skateman skateman deleted the physical-quad-fix branch May 24, 2018 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants