Return custom buttons for service having nil service template #17703

d-m-u · 2018-07-13T14:56:48Z

For bz https://bugzilla.redhat.com/show_bug.cgi?id=1595051.

Services with nonexistent service templates should return all buttons applicable to services, I spose.

GM's ideas were to use a prepend or alias_method for this. The custom_action method exists in both Service and the CustomActionsMixin and in the event of a dearth of service template on the service, we want to run the mixin method. The mixin is used basically everywhere which complicates this somewhat. If anyone (ie @bdunne) has a better idea about how to handle this, I'm all 👂s.

@eclarizio can you 👀 for me please?
@tinaafitz

d-m-u · 2018-07-13T14:57:19Z

@miq-bot add_label bug, blocker

JPrause · 2018-07-13T17:01:54Z

@d-m-u if this can be backported can you add the gaprindashvili/yes label.

d-m-u · 2018-07-13T17:04:28Z

@miq-bot add_label gaprindashvili/yes

eclarizio

One small change requested in the specs, but otherwise looks good.

eclarizio · 2018-07-13T17:06:24Z

spec/models/service_spec.rb

+      describe "#custom_action_buttons" do
+        it "get list of custom action buttons on services" do
+          expect(service).to receive(:custom_action_buttons).and_return(@custom_button)
+          service.custom_action_buttons


I think you want to re-word this expect so that it doesn't read like you've stubbed out service.custom_action_buttons since that's what you're testing.

Should be something more like:
expect(service.custom_action_buttons).to equal(@custom_button)

You can also change the @custom_button to be a let for consistency, it shouldn't have to be an instance variable. In this case, since lets are lazily loaded, you can use let! if you need the button to be created before the spec gets ran.

d-m-u · 2018-07-13T18:22:33Z

@miq-bot remove_label wip

gmcculloug · 2018-07-13T19:33:35Z

app/models/service.rb

+      CustomButton.where(:applies_to_class => "Service")
+    else
+      service_template.custom_action_buttons(self)
+    end


This looks fine, although I might suggest if service_template and swapping the if-else blocks. Doing a 1-line ternary operator comes out to 121 characters. So close.

Anyway, the real reason I'm here is to ask about the custom_actions method just above this method which looks like it will suffer the same fate and should be addressed as well.

Doing a 1-line ternary operator comes out to 121 characters. So close.

There's no line length limit, so 121 is fine

if service_template

👍

gmcculloug · 2018-07-16T14:43:56Z

app/models/service.rb

+    if service_template.nil?
+      buttons = CustomButton.where(:applies_to_class => "Service")
+      button_groups = CustomButtonSet.all.select { |f| f.set_data[:applies_to_class] == "Service" }
+      {:buttons => buttons, :button_groups => button_groups}


Looking at app/models/mixins/custom_actions_mixin.rb#L11-L20 there looks like there is more to this.

Could the Service class include the mixin and then call it when service_template is not available? I think it would avoid rewriting this logic.

Then since I'm using the mixin logic should the custom_action_button query just be generic_custom_buttons from the mixin here: https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/custom_actions_mixin.rb#L52 ?

gmcculloug · 2018-07-16T17:20:26Z

Yes, that looks correct.

d-m-u · 2018-07-16T20:30:51Z

@gmcculloug @syncrou can you 👀 again for me please?

gmcculloug · 2018-07-17T17:09:07Z

@d-m-u Please see failing tests.

We initially looked at using prepend but found that the included block in the CustomActionsMixin was not run which lead to other issues so we went with the current solution.
https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/custom_actions_mixin.rb#L4-L9

Open to suggestions on this one.

miq-bot · 2018-07-17T18:01:52Z

Checked commit d-m-u@14c4be1 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/service.rb

❗ - Line 109, Col 3 - Style/Alias - Use alias instead of alias_method in a class body.

bdunne · 2018-07-17T19:15:20Z

We initially looked at using prepend but found that the included block

Yeah, @d-m-u asked me about that, include and prepend are very different. I'm still not sure what the goal is...

syncrou

LGTM 👍

gmcculloug · 2018-07-17T20:25:41Z

@bdunne Services can be backed by a ServiceTemplate or created without one.

Custom buttons can be attached directly to a ServiceTemplate and/or to the Service class (meaning all service instances get them).

The existing code assumes that there is a ServiceTemplate and if so we get the buttons from there. If there is no ServiceTemplate then nothing is returned. This change handles a stand-along Service (no parent ServiceTemplate) and supports the custom buttons that are defined on the Service class.

gmcculloug · 2018-07-17T21:32:58Z

@eclarizio PTAL

eclarizio

👍

Return custom buttons for service having nil service template (cherry picked from commit 2d8f3e8) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1603031

simaishi · 2018-07-19T09:15:09Z

Gaprindashvili backport details:

$ git log -1
commit 0378482d9f90600e3614cf65573b87f372bf04c2
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Jul 17 19:01:41 2018 -0400

    Merge pull request #17703 from d-m-u/bz1595051
    
    Return custom buttons for service having nil service template
    (cherry picked from commit 2d8f3e83bcc4e9ea519596957e99d9e69baf5041)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1603031

miq-bot added blocker bug labels Jul 13, 2018

d-m-u force-pushed the bz1595051 branch 2 times, most recently from 994844b to 8857491 Compare July 13, 2018 16:47

d-m-u changed the title ~~Return custom buttons for service having nil service template~~ [WIP] Return custom buttons for service having nil service template Jul 13, 2018

miq-bot added gaprindashvili/yes wip labels Jul 13, 2018

eclarizio suggested changes Jul 13, 2018

View reviewed changes

d-m-u force-pushed the bz1595051 branch from 8857491 to 32ad562 Compare July 13, 2018 17:36

d-m-u changed the title ~~[WIP] Return custom buttons for service having nil service template~~ Return custom buttons for service having nil service template Jul 13, 2018

miq-bot removed the wip label Jul 13, 2018

gmcculloug self-assigned this Jul 13, 2018

gmcculloug reviewed Jul 13, 2018

View reviewed changes

d-m-u force-pushed the bz1595051 branch 3 times, most recently from fd3520c to 9b94e7c Compare July 16, 2018 13:58

gmcculloug reviewed Jul 16, 2018

View reviewed changes

d-m-u force-pushed the bz1595051 branch 2 times, most recently from e2d3181 to db92445 Compare July 16, 2018 20:29

d-m-u force-pushed the bz1595051 branch 2 times, most recently from fc208c1 to 834e726 Compare July 17, 2018 15:50

d-m-u force-pushed the bz1595051 branch from 834e726 to 1c348a6 Compare July 17, 2018 17:16

Return custom buttons for service having nil service template

14c4be1

d-m-u force-pushed the bz1595051 branch from 1c348a6 to 14c4be1 Compare July 17, 2018 17:57

syncrou approved these changes Jul 17, 2018

View reviewed changes

eclarizio approved these changes Jul 17, 2018

View reviewed changes

gmcculloug merged commit 2d8f3e8 into ManageIQ:master Jul 17, 2018

gmcculloug added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 17, 2018

simaishi pushed a commit that referenced this pull request Jul 19, 2018

Merge pull request #17703 from d-m-u/bz1595051

0378482

Return custom buttons for service having nil service template (cherry picked from commit 2d8f3e8) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1603031

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jul 19, 2018

d-m-u deleted the bz1595051 branch October 25, 2018 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return custom buttons for service having nil service template #17703

Return custom buttons for service having nil service template #17703

d-m-u commented Jul 13, 2018 •

edited

Loading

d-m-u commented Jul 13, 2018

JPrause commented Jul 13, 2018

d-m-u commented Jul 13, 2018

eclarizio left a comment

eclarizio Jul 13, 2018

d-m-u commented Jul 13, 2018

gmcculloug Jul 13, 2018

bdunne Jul 16, 2018

gmcculloug Jul 16, 2018

d-m-u Jul 16, 2018

gmcculloug commented Jul 16, 2018

d-m-u commented Jul 16, 2018

gmcculloug commented Jul 17, 2018

miq-bot commented Jul 17, 2018

bdunne commented Jul 17, 2018

syncrou left a comment

gmcculloug commented Jul 17, 2018

gmcculloug commented Jul 17, 2018

eclarizio left a comment

simaishi commented Jul 19, 2018

Return custom buttons for service having nil service template #17703

Return custom buttons for service having nil service template #17703

Conversation

d-m-u commented Jul 13, 2018 • edited Loading

d-m-u commented Jul 13, 2018

JPrause commented Jul 13, 2018

d-m-u commented Jul 13, 2018

eclarizio left a comment

Choose a reason for hiding this comment

eclarizio Jul 13, 2018

Choose a reason for hiding this comment

d-m-u commented Jul 13, 2018

gmcculloug Jul 13, 2018

Choose a reason for hiding this comment

bdunne Jul 16, 2018

Choose a reason for hiding this comment

gmcculloug Jul 16, 2018

Choose a reason for hiding this comment

d-m-u Jul 16, 2018

Choose a reason for hiding this comment

gmcculloug commented Jul 16, 2018

d-m-u commented Jul 16, 2018

gmcculloug commented Jul 17, 2018

miq-bot commented Jul 17, 2018

bdunne commented Jul 17, 2018

syncrou left a comment

Choose a reason for hiding this comment

gmcculloug commented Jul 17, 2018

gmcculloug commented Jul 17, 2018

eclarizio left a comment

Choose a reason for hiding this comment

simaishi commented Jul 19, 2018

d-m-u commented Jul 13, 2018 •

edited

Loading