Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix belongs_to associations with block #51

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

There is attempt to fix issue #50.

The only difference between has_many and belongs_to for show_for is that has_many returns array, and belongs_to returns single object, that is not wrapped inside array. So simple Array.wrap fixes the issue. Only one line of code. But wrapping belongs_to 'collection' inside collection_tag is not needed, it is better to display it inline with its label, so some additional code is needed.

It maybe looks like a hack, because options[:collection_tag] is set to empty string, and wrap_with was modified to handle empty tags correctly. I'm not sure is it better to use some instance variable instead.

Contributor

lucasmazza commented Oct 7, 2012

@denispeplin I might not be the best one to evaluate your fix, but could you add at least a test in association_test.rb for this? I think that something similar to this test case that deals with associations and blocks will do the job.

I don't use Test::Unit for my applications and currently don't completely understand how show_for testing package works. I've tried to make failing test for unpatched show_for, without success.
Actually, I recently developed a gem, which also uses Test::Unit, but with dummy Rails application, so it is much easier to setup, and models behavior is closer to real-life models. Maybe it makes the difference, but I'm not sure.

Contributor

lucasmazza commented Oct 10, 2012

You could try to do something like this:

  test "show_for accepts a block with an argument in belongs_to associations" do
    with_association_for @user, :company do |company|
      company.name.upcase
    end

    assert_select "div.show_for p.wrapper", /PLATAFORMATEC/
  end

/cc @nashby @carlosantoniodasilva

Collaborator

nashby commented Oct 10, 2012

@lucasmazza yeah, this test is OK. Anyway I've come up with following solution:

➜  show_for git:(master) ✗ git diff          
diff --git a/lib/show_for/content.rb b/lib/show_for/content.rb
index 2668c3f..c0798e3 100644
--- a/lib/show_for/content.rb
+++ b/lib/show_for/content.rb
@@ -27,7 +27,7 @@ module ShowFor
         when NilClass, Numeric
           value.to_s
         else
-          value
+          block_handler(value, &block)
       end

       options[:content_html] = options.except(:content_tag) i
@@ -46,6 +46,14 @@ module ShowFor
       wrap_with(:collection, response, options)
     end

+    def block_handler(value, &block)
+      if block
+        template.capture(value, &block)
+      else
+        value
+      end
+    end
+
     def translate_blank_html
       template.t(:'show_for.blank_html', :default => translat
     end
diff --git a/test/association_test.rb b/test/association_test.
index ed0ce69..6269861 100644
--- a/test/association_test.rb
+++ b/test/association_test.rb
@@ -42,6 +42,14 @@ class AssociationTest < ActionView::TestCas
     end
   end

+  test "show_for accepts a block with an argument in belongs_
+    with_association_for @user, :company do |company|
+      company.name.upcase
+    end
+
+    assert_select "div.show_for p.wrapper", /PLATAFORMATEC/
+  end
+
   test "show_for accepts :using as option to tell how to retr
     with_association_for @user, :tags, :using => :alternate_n
     assert_select "div.show_for p.wrapper ul.collection"
Collaborator

nashby commented Oct 10, 2012

This works, passes with patch and fails without it!

2012/10/10 Lucas Mazza notifications@github.com

You could try to do something like this:

test "show_for accepts a block with an argument in belongs_to associations" do
with_association_for @user, :company do |company|
company.name.upcase
end

assert_select "div.show_for p.wrapper", /PLATAFORMATEC/

end

And I also checked patch to content.rb within my application. It works as expected.

Collaborator

rafaelfranca commented Oct 14, 2012

Sorry for the delay guys.

@nashby your solution seems good. But I would add a elsif block check instead of a new method. Could you open a pull request?

@lucasmazza you are allowed to merge pull requests, don't need to wait me or @carlosantoniodasilva to take a decision if you think it is good.

@denispeplin thank you to send this pull request and to confirming the fix.

nashby added a commit to nashby/show_for that referenced this pull request Oct 15, 2012

Collaborator

nashby commented Oct 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment