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

Refactors custom action mixin method #16371

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Nov 1, 2017

We have a bunch of models that need the generic_custom_buttons method and it makes more sense to have that code in the mixin rather than duplicating it in all the models. This's 30 lines vs the almost four hundred, almost identical lines that the comparable PR requires.

Adds the custom actions mixin to all models that support custom buttons and changes mixin method to do something.

wip because needs UI components

Related to:
(Host:
eclarizio@8ca5858) [unnecessary if we're using this rather than https://github.com//pull/16369 because this includes the mixin and doesn't need the method or tests found in Erik's host PR]
eclarizio/manageiq-ui-classic@f7d05b4
eclarizio/manageiq-api@e637616 [unnecessary also cause it's covered in the linked api one, was just template for drew]

Everything else:
ManageIQ/manageiq-api#163

@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 1, 2017

@miq-bot assign @gmcculloug
@miq-bot add_label enhancement

Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

@d-m-u With this change you can delete the generic_custom_buttons method in the Vm model as well.

https://github.com/ManageIQ/manageiq/blob/master/app/models/vm.rb#L116-L118

@@ -48,6 +48,6 @@ def serialize_button(button)
end

def generic_custom_buttons
raise "called abstract method generic_custom_buttons"
CustomButton.buttons_for(self.class.name)
Copy link
Member

@gmcculloug gmcculloug Nov 1, 2017

Choose a reason for hiding this comment

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

@imtayadeway Wanted to get your thoughts on this change. Previously only 3 files used this mixin and 2 need to specify other classes (Service -> ServiceTemplate, GenericObject -> GenericObjectDefinition).

We want to support this on all object that are exposed to custom buttons (see list here) and now only those two exceptions above would need to override the generic_custom_buttons button.

Seems a much better approach then having to define generic_custom_buttons on all the classes that use it.

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 good with me 👍

@@ -65,6 +65,7 @@ class Service < ApplicationRecord
include TenancyMixin
include SupportsFeatureMixin
include Metric::CiMixin
include CustomActionsMixin
Copy link
Member

Choose a reason for hiding this comment

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

Services get their custom buttons through the service_template (app/models/service.rb#L56) model which already includes this mixin. This is not needed.

@@ -1,4 +1,6 @@
class GenericObject < ApplicationRecord
include CustomActionsMixin
Copy link
Member

Choose a reason for hiding this comment

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

GenericObjects get their custom buttons through the generic_object_definition (app/models/generic_object.rb#L12-L18) model which already includes this mixin. This is not needed.

@d-m-u d-m-u force-pushed the refactor_generic_custom_button_in_custom_action_mixin branch from d9d375b to 029abc9 Compare November 1, 2017 15:23
@miq-bot
Copy link
Member

miq-bot commented Nov 1, 2017

Checked commits d-m-u/manageiq@e5ea9ab~...029abc9 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
27 files checked, 0 offenses detected
Everything looks fine. 🍰

@d-m-u d-m-u changed the title [WIP] Refactors custom action mixin method Refactors custom action mixin method Nov 1, 2017
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 1, 2017

@miq-bot add_label gaprindashvili/yes

@gmcculloug gmcculloug removed the wip label Nov 1, 2017
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Yeah, I'm good with just having to add in the mixin. As long as we don't have any weird cases where the CustomButton.buttons_for needs to pass in a slightly different class name, but then I guess we can just override it anyway.

@gmcculloug
Copy link
Member

@eclarizio That is the case with ServiceTemplate and GenericObjectDefinition models, but as you state it can be handled by overriding the method in the model. The default mixin logic now handles the majority of models.

@gmcculloug gmcculloug merged commit aea2443 into ManageIQ:master Nov 1, 2017
@gmcculloug gmcculloug added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 1, 2017
@d-m-u d-m-u deleted the refactor_generic_custom_button_in_custom_action_mixin branch November 1, 2017 15:58
simaishi pushed a commit that referenced this pull request Nov 6, 2017
…n_custom_action_mixin

Refactors custom action mixin method
(cherry picked from commit aea2443)
@simaishi
Copy link
Contributor

simaishi commented Nov 6, 2017

Gaprindashvili backport details:

$ git log -1
commit 141079d5b0a379cb10634493057f7beeaa6a9303
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Nov 1 11:54:20 2017 -0400

    Merge pull request #16371 from d-m-u/refactor_generic_custom_button_in_custom_action_mixin
    
    Refactors custom action mixin method
    (cherry picked from commit aea2443b4fa4e0bbfbdbc69f655df7d673d7a04c)

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.

6 participants