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

set @root_class only if it is not already set by subclass #5658

Merged
merged 1 commit into from May 31, 2019

Conversation

h-kataria
Copy link
Contributor

setting @root_class here was causing incorrect filters to load for Instances/Images Accordions in Cloud/Instances explorer.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1715462

before
before1

before2

after
aftr1

after2

setting `@root_class` here was causing incorrect filters to load for Instances/Images Accordions in Cloud/Instances explorer.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1715462
@miq-bot
Copy link
Member

miq-bot commented May 30, 2019

Checked commit h-kataria@89fc580 with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/presenters/tree_builder_vms_filter.rb

@@ -1,6 +1,6 @@
class TreeBuilderVmsFilter < TreeBuilder
def initialize(*args)
@root_class = 'ManageIQ::Providers::InfraManager::Vm'
@root_class ||= 'ManageIQ::Providers::InfraManager::Vm'
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this can be previously set 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

TreeBuilderImagesFilter, TreeBuilderVmsInstancesFilter, TreeBuilderInstancesFilter, TreeBuilderTemplatesImagesFilter and TreeBuilderTemplateFilter all set @root_class before calling super in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

@skateman do you want this changed, or are you ok with this as is?

(We could probably just move the assignment after super in all the classes, solving the problem too.)

Copy link
Member

Choose a reason for hiding this comment

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

oh, yes constructor redefinition call order, sure I'm fine

@himdel himdel closed this May 31, 2019
@himdel himdel reopened this May 31, 2019
@himdel
Copy link
Contributor

himdel commented May 31, 2019

LGTM, without this PR I see the same list of filters everywhere (instances, images, vms, templates),
with this PR, I can see different lists, including filters added in those accordions 👍

@himdel himdel added this to the Sprint 113 Ending Jun 10, 2019 milestone May 31, 2019
@himdel himdel merged commit e305c24 into ManageIQ:master May 31, 2019
@h-kataria h-kataria deleted the fix_to_load_correct_filters branch July 29, 2019 21:50
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

4 participants