Fix passing object with partials w/o locals (#378). #435

siegfault · 2018-04-17T00:23:02Z

This uses @SGospodinov's specs written on the report from @yusufali2205. I ran into this issue as well today and, while I don't think this necessarily the final solution, I wanted to move the conversation along.

Previous behavior: Passing objects to partial fails if the below format is used:

json.comments @post.comments, partial: 'comments/comment', as: :comment, show: false

It succeeds if locals option is used:

json.comments @post.comments, partial: 'comments/comment', locals: { show: false }, as: :comment

Swapping to options is a change to revitalize the conversation. I'm not
sure it's a permanent solution.

Previous behavior: Passing objects to partial fails if the below format is used: ``` json.comments @post.comments, partial: 'comments/comment', as: :comment, show: false ``` It succeeds if locals option is used: ``` json.comments @post.comments, partial: 'comments/comment', locals: { show: false }, as: :comment ``` Swapping to options is a change to revitalize the conversation. I'm not sure it's a permanent solution.

rails-bot · 2018-04-17T00:23:11Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

siegfault · 2018-05-14T16:56:39Z

@pixeltrix would love any thoughts or advice on this one that you can provide! Thanks!

yusufali2205 · 2018-06-11T15:13:19Z

lib/jbuilder/jbuilder_template.rb

      array! collection do |member|
-        member_locals = locals.clone
+        member_locals = options.clone


Does it mean using locals won't work now?

json.comments @post.comments, partial: 'comments/comment', locals: { show: false }, as: :comment

I wrote a spec that I'm pretty sure confirms your suspicion that locals don't work now but haven't pushed it. I've been pretty swamped the last couple weeks but should be much freer next week to take a more in depth look. Thanks for taking a look @yusufali2205!

@yusufali2205

This spec validates @yusufali2205's concerns that passing in locals explicitly no longer works.

This is not a good solution because it puts all of options, related to locals or not, into the locals section of options. Will want advice for a better solution, but this does make the spec pass.

siegfault · 2018-07-03T19:19:39Z

@yusufali2205 just pushed a change that highlights and seems to fix the concern you had. I don't think it's a good solution for the reasons I mentioned in the commit that I'll copy here. Definitely looking for advice!

This is not a good solution because it puts all of options, related to
locals or not, into the locals section of options. Will want advice for
a better solution, but this does make the spec pass.

Revert previous attempt which duplicated options in favor of a version that handles setting up locals in the same spot other locals are set up. This allows us to only use the parameters that aren't used by the base options to pull in locals.

Rather than add completely new partials to highlight the issue, tie in to the existing simple partial which still reproduces the error. Adds specs for all 4 combinations of with/without partial key and with/without locals key. Delete existing added specs.

siegfault · 2018-07-10T20:52:56Z

test/jbuilder_template_test.rb

    JBUILDER

    assert_equal "hello", result["content"]
  end

-  test "partial! + locals via :locals option" do
+  test "partial! + locals without :partial key with :locals key" do


Wouldn't mind some help making these descriptions less bulky.

siegfault · 2018-07-10T20:53:43Z

test/jbuilder_template_test.rb

+    assert_equal "howdy", result["content"]
+  end
+
+  test "partial! + locals with :partial key without :locals key" do


This is the spec that currently would fail with the above

- "_partial.json.jbuilder" => "foo ||= 'hello'; json.content foo", + "_partial.json.jbuilder" => "json.content foo",

change.

siegfault · 2018-07-10T20:55:05Z

test/jbuilder_template_test.rb

@@ -44,7 +44,7 @@ def initialize(id, name)
 ActionView::Template.register_template_handler :jbuilder, JbuilderHandler

 PARTIALS = {
-  "_partial.json.jbuilder"  => "foo ||= 'hello'; json.content foo",


Removed to be able to reproduce the undefined method error. This caused me to add

- json.partial! "partial" + json.partial! "partial", foo: 'hello'

Happy to discuss if we'd rather go back to defining its own partial.

siegfault · 2018-07-10T20:58:37Z

lib/jbuilder/jbuilder_template.rb

@@ -199,6 +199,7 @@ def _render_explicit_partial(name_or_options, locals = {})
    when ::Hash
      # partial! partial: 'name', foo: 'bar'
      options = name_or_options
+      options[:locals] ||= options.except(:partial, :as, :collection)


If we want to take these out of the original options hash, I think we'd need to do something like

options[:locals] ||= options.except(:partial, :as, :collection).keys.each_with_object({}) do |local, hash| hash[local] = options.delete(local) end

Thoughts?

siegfault · 2018-07-10T21:00:29Z

I think this is now in a better place and ready for review @yusufali2205 / @pixeltrix. Happy to make any changes and would love any feedback. Thanks!

siegfault · 2018-07-30T20:42:56Z

@pixeltrix let me know if you have any advice or if you want me to ping someone else. Thanks!

siegfault · 2018-10-01T23:59:41Z

@rwz pinging you since it looks like you were getting assigned PRs before @pixeltrix. Any ideas on who I should pull in to take alook at this? Thanks!

@yusufali2205 also pinging you since you originally reported the issue and reviewed this earlier. This seems to fix the issue for me. Mind making sure that it also does for you? Thanks!

siegfault · 2018-10-31T23:02:34Z

Hi @rafaelfranca! Pinging you since you seem to be getting assigned on PRs now (though it's noting that the repo may be misconfigured). Are you the right person to take a look? If not, do you know who would be? Either way, thanks!

The previous location relied on calling `_render_partial_with_options` but there was another case where we call ``` json.bars foo.bars, partial: 'bars', as: :bar, include_baz: true ``` where, because there were args to the partial, we end up hitting `_set_inline_partial` which did not route through `_render_explicit_partial`. Putting it in a common location allows it to work for both.

…als_without_locals

siegfault · 2018-11-13T00:11:17Z

Hi @dhh! Pinging you since you merged a PR in the repo last week. Not necessarily asking for review unless you want to take a look, but since the repo is misconfigured and I haven't gotten much traction here, was wondering if you had suggestions for who to reach out to. Thanks!

dhh · 2018-11-16T23:06:09Z

Hey, sorry you haven't gotten any traction on this yet. Thank for you for keep pushing!

This looks correct to me. Merging. Thanks again!

siegfault · 2018-11-26T18:57:29Z

Thanks!

yusufali2205 · 2018-12-26T22:27:35Z

Sorry for not replying to the messages, I didn't check on this since long time. Thanks @siegfault for fixing this.

rails-bot assigned pixeltrix Apr 17, 2018

yusufali2205 reviewed Jun 11, 2018

View reviewed changes

siegfault added 2 commits July 3, 2018 11:22

Add spec to verify failure of locals param.

bec93ef

This spec validates @yusufali2205's concerns that passing in locals explicitly no longer works.

Attempt to fix spec and allow locals param.

a32a317

This is not a good solution because it puts all of options, related to locals or not, into the locals section of options. Will want advice for a better solution, but this does make the spec pass.

siegfault added 2 commits July 10, 2018 13:12

Fix locals in a cleaner way.

bb229d1

Revert previous attempt which duplicated options in favor of a version that handles setting up locals in the same spot other locals are set up. This allows us to only use the parameters that aren't used by the base options to pull in locals.

Key in to existing tests.

f6de4c4

Rather than add completely new partials to highlight the issue, tie in to the existing simple partial which still reproduces the error. Adds specs for all 4 combinations of with/without partial key and with/without locals key. Delete existing added specs.

siegfault commented Jul 10, 2018

View reviewed changes

siegfault mentioned this pull request Nov 6, 2018

Object with partials without locals procore/jbuilder#1

Merged

siegfault mentioned this pull request Nov 12, 2018

Move the override one step lower. procore/jbuilder#2

Merged

Merge remote-tracking branch 'upstream/master' into object_with_parti…

3ccf568

…als_without_locals

dhh merged commit 91c4eee into rails:master Nov 16, 2018

siegfault deleted the object_with_partials_without_locals branch November 16, 2018 23:11

jeremy mentioned this pull request Jul 17, 2019

Passing object into partial fails if locals: not used #378

Closed

Fix passing object with partials w/o locals (#378). #435

Fix passing object with partials w/o locals (#378). #435

Uh oh!

Conversation

siegfault commented Apr 17, 2018

Uh oh!

rails-bot commented Apr 17, 2018

Uh oh!

siegfault commented May 14, 2018

Uh oh!

yusufali2205 Jun 11, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault Jun 19, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault commented Jul 3, 2018

Uh oh!

siegfault Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault Jul 10, 2018

Choose a reason for hiding this comment

Uh oh!

siegfault commented Jul 10, 2018

Uh oh!

siegfault commented Jul 30, 2018

Uh oh!

siegfault commented Oct 1, 2018

Uh oh!

siegfault commented Oct 31, 2018

Uh oh!

siegfault commented Nov 13, 2018

Uh oh!

dhh commented Nov 16, 2018

Uh oh!

siegfault commented Nov 26, 2018

Uh oh!

yusufali2205 commented Dec 26, 2018

Uh oh!

Uh oh!