Collection fix

[samstickland]

This PR fixes passing context into collections. This probably only affects Cells in rails (since cells-rails is providing the default context that is breaking the collection handling), so there's a potential discussion as to whether this fix should go in Cells or Cells-rails.

If you think the fix is appropriate here let me know and I'll add a test case to this PR.

[timoschilling]

this should be handled in the calling code section:

cell('...', model, context: option[:context].merge({...}))

[samstickland]

@timoschilling Sorry, I don't understand. ViewModel#call is the public entry point.

I could fix it in the calling code in rails-cells but then rails-cells would need to be aware of the collection handling logic, which doesn't seem right.

The issue is described here: #413 (comment)

[apotonick]

I don't understand that.. the context is passed into collections, there's tests here: https://github.com/apotonick/cells/blob/master/test/context_test.rb#L18

Could you provide a test that fails for your fix?

[samstickland]

Sure, I'll add a test.

The symptom of the problem is in rails, given this:

= cell(:comment, collection: Comment.recent, context: { current_user:
current_user } )

The contents of the context hash will be over-written with a new hash
containing only 'controller'.

Rails-cells isn't collection aware so it sees an empty options hash (as the
options are in the model variable at this stage) and then creates options
and adds :controller to :context. In Cells when it merges the options given
by rails-cells it's not a deep merge, so the original context gets
overwritten by the one provided by rails-cells

The problem is on the boundary between rails-cells and cells so it's not
clear in which one the fix should be.. complicated by the fact that
rails-cells is not aware of how collections are handled.

I don't understand that.. the context is passed into collections, there's
tests here:
https://github.com/apotonick/cells/blob/master/test/context_test.rb#L18

Could you provide a test that fails for your fix?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#415 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AJfZU_U_CsJuKx392-P3mxuhBUV-9u2Jks5qL8M_gaJpZM4Iy5wl
.

[timoschilling]

This line is useless, merged_options always contains the :collection key.

[timoschilling]

The problem is that you overwrite the context in your example. You need to merge your current_user into the existing context.

[timoschilling]

# wrong
= cell(:comment, collection: Comment.recent, context: { current_user:
current_user } )
# right
= cell(:comment, collection: Comment.recent, context: option[:context].merge(current_user: current_user))

[samstickland]

@apotonick This cells-rails fork includes a demonstration of this problem

samstickland/cells-rails@9540a5e

The added test fails in the existing version of cells, but passes using my PR above.

@timoschilling But context is overwritten inside of the cells gem when it merges the two options hashes - one provided by the user, one provided by rails-cells.

[samstickland]

@apotonick @timoschilling Right, I've rewritten that test case to be contained within a single file, rather than spread out across routes, controllers etc.

samstickland/cells-rails@3eae04f

require "test_helper"

class ContextCollectionTest < MiniTest::Spec
  include Cell::RailsExtensions::ActionController
  class ArtistCell < Cell::ViewModel
    def show
      # Test will fail if current_user is not present
      raise Exception, "no current_user in context" unless context[:current_user]
      return "success"
    end
  end

  it do
    size = 3
    artists = [Artist.new] * size

    result = cell(ArtistCell, collection: artists, context: { current_user: Object.new }).()

    assert_equal "success" * size, result
  end
end

[samstickland]

If you run that test case you will see the problem straight away! ;)

[samstickland]

@timoschilling Do you mean this line? I'm not sure where I'm looking at to see the linenote:

merged_options = nil if merged_options.empty?

I had to add that line to make some of the existing test cases pass in the cells test suite. I'll need to remove it and run it again to remember which ones they were.

@apotonick , @timoschilling I don't proposing that we merge this PR in it's current state, I threw it together - along with the rough test case - to demonstrate the bug in collection handling ;)

Like I said it could be argued that the bug is on the boundary between cells and cells-rails - hence why my proposed fix is currently in cells and the test case is in cells-rails.

I'm happy to try to explain this again, but if you run the test case above you'll instantly see the problem!

And then I'd a steer as to where this should be fixed, and I'll tidy up this PR.

[toastercup]

I'm aware it's not good practice to comment on a PR/issue and clutter it with "me too!" (one should normally just 'subscribe'), but since this hasn't been touched by the maintainers in a month, I'll give @samstickland a boost and say that this affects our Rails/Cells setup, as well - and that the proposed PR works as intended.

[apotonick]

Hahaha thanks @toastercup I'm having a look today!

[samstickland]

Any news? :)

[Startouf]

me too!

:P

[Startouf]

Well I guess I'll be using the code from that commit until it is merged to the main branch, because I really need that.

[apotonick]

Just giving you a heads-up: we are planning to introduce a new collection API with a dedicated collection cell that you can override, etc. That is no excuse, though, why I haven't merged and fine-tuned this PR, though 😊. Though.

[samstickland]

@apotonick I'm happy to tidy this up myself, but this is the first time I've even really received an acknowledgement from a maintainer that this problem is genuine! ;)

I also, as mentioned above, made a demo test-case for this here: samstickland/cells-rails@9540a5e

But as you can see the test is in cells-rails and the fix is in cells. Hardly ideal. If you can let me know what needs to happen or be tidied up for this to be accepted I'll make it happen!

[apotonick]

It's actually a bug in this gem, so you're doing right in posting that here. It shouldn't merge the options hash that way!

[samstickland]

This PR is still open, but I think the fix has made it onto master via another route?