Add class decorator support #237

himdel · 2017-01-25T13:26:59Z

This makes it possible to run decorate not only on model instances, but also directly on the classes.

It also replaces Draper with a simpler solution.

Differences from Drapper:

there is only that decorate method, no attempt at implementing decorator_class? or any other Draper methods was made
that decorate method is also available on classes now
delegate_all is done automatically, but only for decorated instances, not much sense in using it for the decorated class
decorate returns nil when no decorator is found, instead of throwing

Usage:

VmServer.decorate will return VmOrTemplateDecorator
vm_instance.decorate will return an instance of VmOrTemplateDecorator which delegates to the original vm_instance object

adding

def self.fonticon
  "foobar"
end

to that decorator (note the self) will cause VmServer.decorate.fonticon to return "foobar".

Closes ManageIQ/manageiq#10313

Done in preparation for ManageIQ/manageiq#13199, cc @epwinchell

~~TODO:~~:

specs
miq PR removing the draper dependency - Remove Draper dependency manageiq#14044
replace API ?decorators with supports - removing the last .decorate use from manageiq - API - VMs collection - replace decorators with SupportsFeatureMixin manageiq#14040 (and VmOrTemplateDecorator - remove supports_console and supports_cockpit #458)

Note that this does monkeypatch ApplicationRecord - adding a class and instance decorate method (and a @_decorator field to cache it once resolved).

To be able to decorate non-AR models, we'll need to extend and include the relevant mixin manually.

himdel · 2017-01-25T13:28:48Z

@hayesr, @martinpovolny you were active in the original discussion about class decorators => ping :)

himdel · 2017-01-25T13:34:35Z

Aand @skateman :)

himdel · 2017-01-25T14:12:16Z

So..

config/initializers/ is maybe not the best solution, but works (provided there is that < ActiveRecord::Base, otherwise it fails with NoMethodError trying to call connection).

Putting this in app/models/ causes Draper's collection decorator take precedence, which doesn't work :(.

So.. I guess we can either keep it here, or remove draper and reimplement the decorate method here as well.

EDIT: actually, putting it in (ui-classic) app/models means it never gets loaded

chrisarcand

This is good idea, but I think we should do some changes as to the implementation.

chrisarcand · 2017-01-25T16:55:36Z

lib/miq_decorator.rb

+  def method_missing(*args)
+    @_decorated_instance.send(*args)
+  end
+end


I would strongly suggest using an actual delegator rather than crossing your fingers with method_missing.

Are you familiar with some of Ruby's core Delegator classes? I would use SimpleDelegator here.

class MiqDecorator < SimpleDelegator

👍 on using the SimpleDelegator

chrisarcand · 2017-01-25T17:03:05Z

config/initializers/class_decorator.rb

+  def decorate
+    self.class.decorator_for(self.class).new(self)
+  end
+end


(Follow this after addressing comment here)

Patching something that doesn't belong to you is a bad idea, especially re-opening a class in our lovely project 😄

Instead, you can do away with this entirely and create these methods in MiqDecorator itself. MiqDecorator can take in instances and klasses and return the proper decorator (which it owns) instance or class for you.

class MiqDecorator < SimpleDelegator self << class def decorator_for(klass) "#{klass.name}Decorator".safe_constantize end def decorate(instance) decorator = decorator_for(instance.class) decorator.new(instance) end end end

Disclaimer: I just wrote this pseudo code up quick, a few details are plainly missing.

👍
Yay for naming conventions in decorator_for

Patching something that doesn't belong to you is a bad idea, especially re-opening a class in our lovely project 😄

I would take this a bit further and state that as you move to the REST API, you can't patch platform classes, so doing so here might make it harder to move forward. However, using the decoration pattern above, it is clear which methods are owned by the UI and not the platform, and those can be converted to code that eventually decorates the result of the REST API.

chrisarcand · 2017-01-25T17:04:38Z

app/controllers/application_controller.rb

@@ -959,7 +959,7 @@ def listicon_item(view, id = nil)
  # Return the icon classname for the list view icon of a db,id pair
  # this always supersedes listicon_image if not nil
  def listicon_icon(item)
-    item.decorate.try(:fonticon) if item.decorator_class?
+    item.decorate.try(:fonticon)


With my other comments addressed, this now becomes a non-patched call, entirely dependent on the UI's own code, with MiqDecorator.decorate(item). You of course can feel free to create a top level controller helper like decorate_model that does the same thing without MiqDecorator explicitly.

kbrock

Looking good. just 2 style ideas that can be ignored.

I like chris's idea of using a delegator

kbrock · 2017-01-25T17:11:36Z

spec/helpers/quadicon_helper_spec.rb

@@ -963,7 +963,7 @@

      context "when item is not CIM or decorated" do
        before(:each) do
-          allow(item).to receive(:decorator_class?) { false }
+          allow(item).to receive(:decorate) { nil }


nit: could you use and_return?

allow(item).to receive(:decorate).and_return(nil)

kbrock · 2017-01-25T17:12:25Z

config/initializers/class_decorator.rb

+class ApplicationRecord < ActiveRecord::Base
+  class << self
+    def decorate
+      @_decorator || (@_decorator = decorator_for(self))


another nit. I think this is more clear with ||=:

@_decorator ||= decorator_for(self)

(But you won't need this code given proper delegation, anyway)

chrisarcand · 2017-01-25T17:13:30Z

Your comment about initializers here should be addressed/fixed given my feedback.

hayesr · 2017-01-25T17:14:14Z

I think it might be possible to extend ApplicationRecord with an initializer block in engine.rb, but I think I like @chrisarcand's idea better. It feels a lot more object-oriented, and I bet we'll get some happy testing benefits as well.

Fryguy · 2017-01-25T17:20:12Z

Putting this in app/models/ causes Draper's collection decorator take precedence, which doesn't work :(.

So.. I guess we can either keep it here, or remove draper and reimplement the decorate method here as well.

It sounds like you want to remove Draper, so in that case, I would say remove draper.

chrisarcand · 2017-01-25T17:25:08Z

Indeed, Draper should be replaced given where we are now.

Let me be clear that your implementation is understandable given that you were basically implementing how Draper does it. But Draper, with the UI split having been done, now doesn't make any sense for us at all because of how it patches things in much the same way. The UI doesn't really own it's models that it's trying to decorate anymore.

miq-bot · 2017-01-26T20:42:44Z

This pull request is not mergeable. Please rebase and repush.

martinpovolny · 2017-01-27T10:10:34Z

But Draper, with the UI split having been done, now doesn't make any sense for us at all because of how it patches things in much the same way. The UI doesn't really own it's models that it's trying to decorate anymore.

Looking forward that's true. But for now it's not that much imporant. We need to be able to decorate to be able to do cleanups and move forward.

As @Fryguy wrote, we might be decorating stuff returned from the API in the future. But for now we need the decorators for both instances and classes of the models.

miq-bot · 2017-02-17T11:43:00Z

This pull request is not mergeable. Please rebase and repush.

This makes it possible to run `decorate` not only on model instances, but also directly on the classes. Differences from Drapper: * there is only that `decorate` method, no attempt at implementing `decorator_class?` or any other Draper methods was made * `delegate_all` works out of the box (meaning `Host.decorate.foo` calls `Host.foo` when the decorator does not implement that method, and does have the `delegate_all` call) * `decorate` returns nil when no decorator is found, instead of throwing Usage: `VmServer.decorate` will return `VmOrTemplateDecorator` adding ``` def self.fonticon "foobar" end ``` to that decorator (note the `self`) will cause `VmServer.decorate.fonticon` to return `"foobar"`.

pretty much just expects an instance passed to the constructor, so it can delegate everything to it

…corate

…qDecorator.for so that MiqDecorator.for(Klass) returns KlassDecorator, if it exists

now that decorate doesn't throw but returns nil instead

himdel · 2017-02-23T15:46:16Z

So... I've rebased and addressed most of the concerns, including moving the class resolution logic to MiqDecorator, rebased, etc.

But... do we really want to write MiqDecorate.decorate_instance(instance) and MiqDecorate.decorate_class(VmOrTemplate) instead of instance.decorate and VmOrTemplate.decorate? To me, that sounds like the exact opposite of right :). (Also, harder to cache :).)

because sometimes Foo::Bar means class Foo, class Foo::Bar and not module Foo > class Bar

…asses

…r::Klass and just call ApplicationRecord.send(extend/include...)

miq-bot · 2017-02-24T13:23:11Z

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/de3561070a93f4dd69f140bcc29a8e75805728bc~...5c1fec047e408741352f64d5fbdb0f164bbaacce with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
48 files checked, 3 offenses detected

app/decorators/miq_event_definition_decorator.rb

❕ - Line 1, Col 1 - Metrics/ClassLength - Class has too many lines. [181/100]

spec/lib/miq_decorator_spec.rb

❗ - Line 1, Col 1 - Metrics/BlockLength - Block has too many lines. [59/25]
❗ - Line 2, Col 3 - Metrics/BlockLength - Block has too many lines. [57/25]

himdel · 2017-02-24T13:26:16Z

Tested this does not break the API, so we do not depend on ManageIQ/manageiq#14040.
Tested that removing Draper can be done after, so we do not depend on ManageIQ/manageiq#14044 either.

himdel · 2017-02-24T15:34:47Z

... doing a resume of a gitter discussion - ☝️ February 24, 2017 3:18 PM

We will use .decorate for now, to keep the code same, and not have to depend on the API changes,
but as step 2, we will stop patching ApplicationRecord and do MiqDecorator.for(instance) and MiqDecorator.for(klass) instead.

chrisarcand · 2017-02-24T15:38:18Z

👍 Please note for anyone having an issue with the MiqDecorator.for(object) (which I think is great, personally...) You can use whatever helper method you want as long as it's within the scope of the UI project and doesn't directly edit ApplicationRecord or any other core class that doesn't belong to the UI (as we are doing above, simply to keep the .decorate syntax working). This is important for continued decoupling of the core and UI projects, making our lives betterer. Thanks. :)

himdel added enhancement euwe/no labels Jan 25, 2017

himdel added the wip label Jan 25, 2017

himdel changed the title ~~Add class decorator support~~ [WIP] Add class decorator support Jan 25, 2017

chrisarcand suggested changes Jan 25, 2017

View reviewed changes

kbrock reviewed Jan 25, 2017

View reviewed changes

miq-bot added the unmergeable label Jan 26, 2017

martinpovolny self-assigned this Jan 27, 2017

miq-bot removed the unmergeable label Feb 17, 2017

miq-bot added the unmergeable label Feb 17, 2017

miq-bot removed the unmergeable label Feb 23, 2017

himdel added 5 commits February 23, 2017 14:59

MiqDecorator - a superclass for all our decorators

304bc4e

pretty much just expects an instance passed to the constructor, so it can delegate everything to it

Replace Draper by implementing instance.decorate the same as class.de…

8562276

…corate

MiqDecorator - move the logic to find the right decorator class to Mi…

900e2a1

…qDecorator.for so that MiqDecorator.for(Klass) returns KlassDecorator, if it exists

Replace .decorator_class? with .decorate

88d2b39

now that decorate doesn't throw but returns nil instead

Decorators - inherit from MiqDecorator instead of Draper::Decorator

14462aa

himdel added 5 commits February 23, 2017 16:19

fix decorate to handle missing decorator class

ce35a6b

decorators - fix wrongly namespaced decorators

85447ce

because sometimes Foo::Bar means class Foo, class Foo::Bar and not module Foo > class Bar

Rename initializer to decorate, since it no longer deals only with cl…

8e3f93d

…asses

Move decorate method defs to MiqDecorator::Instance and MiqDecorato…

ab54a9a

…r::Klass and just call ApplicationRecord.send(extend/include...)

MiqDecorator - specs

5c1fec0

himdel changed the title ~~[WIP] Add class decorator support~~ Add class decorator support Feb 24, 2017

miq-bot removed the wip label Feb 24, 2017

martinpovolny mentioned this pull request Feb 24, 2017

Need to specify alternate infra dashboard for OpenStack provider #423

Closed

martinpovolny merged commit 7f5bdac into ManageIQ:master Feb 24, 2017

martinpovolny added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 24, 2017

This was referenced Feb 28, 2017

ApplicationRecord - temporarily add .decorate and self.decorate ManageIQ/manageiq#14093

Merged

Move decorate to ApplicationRecord #506

Merged

himdel deleted the decorate-class branch February 28, 2017 13:53

lgalis mentioned this pull request Mar 2, 2017

Decorator update for inventory and credentials #538

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add class decorator support #237

Add class decorator support #237

himdel commented Jan 25, 2017 •

edited

Loading

himdel commented Jan 25, 2017

himdel commented Jan 25, 2017

himdel commented Jan 25, 2017 •

edited

Loading

chrisarcand left a comment

chrisarcand Jan 25, 2017

martinpovolny Jan 27, 2017

chrisarcand Jan 25, 2017 •

edited

Loading

hayesr Jan 25, 2017 •

edited

Loading

Fryguy Jan 25, 2017

chrisarcand Jan 25, 2017

kbrock left a comment

kbrock Jan 25, 2017

kbrock Jan 25, 2017

chrisarcand Jan 25, 2017

chrisarcand commented Jan 25, 2017

hayesr commented Jan 25, 2017

Fryguy commented Jan 25, 2017

chrisarcand commented Jan 25, 2017

miq-bot commented Jan 26, 2017

martinpovolny commented Jan 27, 2017

miq-bot commented Feb 17, 2017

himdel commented Feb 23, 2017 •

edited

Loading

miq-bot commented Feb 24, 2017

himdel commented Feb 24, 2017

himdel commented Feb 24, 2017

chrisarcand commented Feb 24, 2017

Add class decorator support #237

Add class decorator support #237

Conversation

himdel commented Jan 25, 2017 • edited Loading

himdel commented Jan 25, 2017

himdel commented Jan 25, 2017

himdel commented Jan 25, 2017 • edited Loading

chrisarcand left a comment

Choose a reason for hiding this comment

chrisarcand Jan 25, 2017

Choose a reason for hiding this comment

martinpovolny Jan 27, 2017

Choose a reason for hiding this comment

chrisarcand Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

hayesr Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Fryguy Jan 25, 2017

Choose a reason for hiding this comment

chrisarcand Jan 25, 2017

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

kbrock Jan 25, 2017

Choose a reason for hiding this comment

kbrock Jan 25, 2017

Choose a reason for hiding this comment

chrisarcand Jan 25, 2017

Choose a reason for hiding this comment

chrisarcand commented Jan 25, 2017

hayesr commented Jan 25, 2017

Fryguy commented Jan 25, 2017

chrisarcand commented Jan 25, 2017

miq-bot commented Jan 26, 2017

martinpovolny commented Jan 27, 2017

miq-bot commented Feb 17, 2017

himdel commented Feb 23, 2017 • edited Loading

miq-bot commented Feb 24, 2017

himdel commented Feb 24, 2017

himdel commented Feb 24, 2017

chrisarcand commented Feb 24, 2017

himdel commented Jan 25, 2017 •

edited

Loading

himdel commented Jan 25, 2017 •

edited

Loading

chrisarcand Jan 25, 2017 •

edited

Loading

hayesr Jan 25, 2017 •

edited

Loading

himdel commented Feb 23, 2017 •

edited

Loading