cells icon indicating copy to clipboard operation
cells copied to clipboard

#controller is available only for the first model in a collection

Open BlackFoks opened this issue 9 years ago • 15 comments

When rendering a collection #controller is available only for the first model in a collection.

Let's we want to render a list of books:

books = [Book.find(1), Book.find(2), Book.find(3)]

Then #controller will be available only for Book#1, when rendering Book#2:

def show
  controller # => nil
end

I don't think it's an expected behaviour. I found what caused this https://github.com/apotonick/cells/blob/f785b3431b226b5c98a5cd58d4fc76e13239f0d7/lib/cell/twin.rb#L17

super(build_twin(model, options), controller: options.delete(:controller))

We delete :controller from options. I see 2 possible ways to fix this:

  1. Don't options.delete(:controller), just do options[:controller]
  2. When rendering collection do options.dup or something like this.

I can send a PR if you tell me what's the best way. I'd like 1st but I'm not sure about consequences (maybe it's a required action or just a try to hide controller from twin).

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

Twin is not supported in Cells 4 (yet) - why do you have that included?

apotonick avatar Jun 24 '15 05:06 apotonick

I have a twin class to share some code between different representers. I just want to use its properties in a cell (concept actually).

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

Hm, when you copy it, it should work. I will re-add twin support when I release Disposable.

apotonick avatar Jun 24 '15 05:06 apotonick

It works without twin, here's the proof: https://github.com/apotonick/cells/blob/master/test/public_test.rb#L35 :stuck_out_tongue_winking_eye:

Can you paste some code, I'm curious to know how you use twins?

apotonick avatar Jun 24 '15 05:06 apotonick

Yes, it works without twin. I just tested it. Ok, here is some code:

class Invoice < ActiveRecord::Base
  # ...
end

class Invoice::Twin < BaseTwin
  # properties here...

  module XeroIntegration
    def invoice_number
      "INV-#{ id }"
    end

    def xero_url
      if bill?
        xero_bill_url
      else
        xero_invoice_url
      end
    end

    protected

    def xero_bill_url
      # ...
    end

    def xero_invoice_url
      # ...
    end
  end
  include XeroIntegration
end

class Invoice::Concept < Concept
  include ::Cell::Twin

  def show
    #...
  end

  private

  twin ::Invoice::Twin
end

class Invoice::XeroRepresenter
  def initialize(invoice)
    @invoice = ::Invoice::Twin.new(invoice)
  end

  def to_hash
    {
      number: invoice.invoice_number,
      # other properties
    }
  end
end

So I use twin as a decorator. I didn't want to put twin's code into a model so I though it was a good idea to use twins for this.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

Yeah, that's exactly how it's meant to work. :grimacing: You should use a representer to compile your to_hash hash with Representable. You can infer a representer from a twin.

apotonick avatar Jun 24 '15 05:06 apotonick

Yes, I can I know. It's not an issue :) The issue is that including include ::Cell::Twin into Invoice::Concept breaks collection rendering.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

I just updated cells to 4.0.1 and it broke device helpers in cells. I already read that helper_method is not available anymore so I tried to use something like delegate :current_user, to: :controller to get a current user inside a cell. It works well expect collections because controller is nil for all non-first elements.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

But it only breaks with Twin included, right?

apotonick avatar Jun 24 '15 05:06 apotonick

Yes, I think it's due to options.delete(:controller) and then options is passed to the next element with no :controller in options.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

It's here https://github.com/apotonick/cells/blob/f785b3431b226b5c98a5cd58d4fc76e13239f0d7/lib/cell/twin.rb#L17

So it builds the first element with controller and then controller gets lost.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

I know, I know, I have to rewrite Twin for cells anyway.

apotonick avatar Jun 24 '15 05:06 apotonick

Ok, thanks again.

Just one thought: since anyone can modify options in one cell and it will affect all next cells in a collection maybe it would be good to pass a copy of original options to each cell to prevent such issues in future?

BlackFoks avatar Jun 24 '15 05:06 BlackFoks

This will dramatically slow down rendering collections! Hash operations are expensive!

apotonick avatar Jun 24 '15 05:06 apotonick

Oh, I see.

BlackFoks avatar Jun 24 '15 05:06 BlackFoks