Missing/empty attribute_fields. Regression introduced with 235eae3.

[haslinger]

I updated one of my applications using ransack to Rails 5 and found, that the attribute_fields method returns an empty string.

I tried to debug the issue, and found that within the attribute_fields helper the call to the search_fields method returns an empty string. In the old version (Rails 3.x) the proper html string for the dropdown was returned.

To reproduce the issue easily, I did the same with the ransack_demo app and found the same issue there - see Issue activerecord-hackery/ransack_demo#10.

[igorkasyanchuk]

if we have form and I want to build dynamic search using code below

      <%= f.condition_fields do |c| %>
        <%= c.attribute_fields do |a| %>
          <%= a.attribute_select %>
        <% end %>
      <% end %>

I'm receiving an empty string instead of select (in version 1.7 of this gem this code was working).

Quick investigation showed me that this code

self.attributes << attribute if attribute.valid? was returning false (in condition.rb). Looks like attribute doesn't have a parent. I was trying to fix, but code really looks complex :) hopefully it can save time for you to investigate the issue

[zeknox]

I also am having the same issue described by the OP. The issue is present even on Rails version 4 using Ransack v 1.8.0. If I downgrade the Ransack gem to version 1.7.0 it appears to work properly again.

[jaydorsey]

Just a little bit of additional information because this impacted me as well.

I have some time to take a look at this today. I'll start with 235eae3 because it appears to be the first commit that breaks the functionality

You can test using the advanced search feature on an updated ransack search demo. All commits that I sampled after the commit above are missing the dropdown per the screenshot

[jaydorsey]

The attributes aren't being added, I think, because this line always seems to return false for .valid?, because bound? always returns false here

It doesn't appear in the commits prior that the .valid? check was being called. There was a build_attribute method (is called, didn't have any .valid? check) and a separate attributes= method (didn't appear to be called, did have the .valid? checks) and when the commit referenced above was done there was some refactoring that introduced the .valid? checks that didn't appear to be used before.

I put together a new branch and a corresponding demo branch which appear to be functional.

All tests are still passing and I haven't seen any other issues/bugs. I'm not comfortable submitting a PR with this because I'm not fully familiar with the codebase but it's only a single line modified so shouldn't take too much time to look at and see if this is the fix, or if the binding/bound method should be looked at.

I only tested against the ransack demo, using ActiveRecord, not mongoid.

[haslinger]

@jaydorsey I gave your fix a try in my app and it seems to work fine for my app as well.
I will test quite a lot there during the next days, but meanwhile a big THANK YOU!

[jonatack]

Thanks everyone for the combined info.

The takeaway is that we are missing tests for this behavior.

In the meantime, @jaydorsey would you like to make a PR for your patch? It is true that it doesn't break any tests, though we are missing some here. Following which, we'll release Ransack 1.8.1 as a patch release while solving it correctly.

TODOs:

• Add tests to cover this -- a fun PR to make if anyone is interested.
• Update the Ransack demo app -- @jaydorsey, would be great if you'd like to leverage the work you've done to make a PR over there :)

[jonatack]

@jaydorsey, @haslinger has also updated ransack_demo (to Rails 5, perfect). Would you two like to coordinate on making PRs to update it? It looks like some of your work is complementary.

[igorkasyanchuk]

@jonatack I don't think that PR is correct. It's simply commenting "if valid?" line which may cause other problems. I believe real fix will require more efforts.

[jonatack]

@igorkasyanchuk I agree. To work on this, we need tests in place first. This bug slipped through because I don't use this feature and perhaps @avit (who did the work on #645) doesn't either and the test suite passed.

[jonatack]

@igorkasyanchuk Would you like to make a PR that adds the missing tests?

[jaydorsey]

I didn't submit a PR because it wasn't clear what the valid check was checking. The last functional version I noted above didn't actually appear to use the valid check but I didn't test it comprehensively. I agree with @igorkasyanchuk that there could be unknown side effects.

I'll reach out to @haslinger re: getting the demo updated on his repo

[haslinger]

@jonatack , @jaydorsey created pull request for the demo app activerecord-hackery/ransack_demo#11.

[rutte]

I still have this issue with rails 3.2.22.2 and mongoid 3.1.1. Using ransack 1.7.0 works, attributes are there. But with 1.8.0 or 1.8.1 attributes are nil.

[jonatack]

Hi everyone, no one has added a test as mentioned above. Here's the deal: If someone cares enough to provide passing/failing regression tests to cover this, then I'm willing to look at the bug. This is open-source software and everyone can contribute ;)

@rutte, when accepting addition of the Mongoid adapter, I stipulated that I don't use Mongoid and maintain its Ransack adapter. Anything Mongoid-related is up to the community.

[igorkasyanchuk]

I'm trying to add spec, and I did following

  describe '#condition_fields' do
    it 'returns selects with available fields for conditions' do
      html = @f.grouping_fields do |c|
        c.attribute_fields do |a|
          a.attribute_select
        end
      end
      expect(html).to match /"abc"/
    end
  end

but I'm receiving an empty string. Not sure how I can generate html. I used version 1.7.0 (I just took commit from history from 2015). Also I've tried to do following: @f.instance_variable_get(:@object).build_grouping but again it won't help

[jonatack]

Everybody, please chime in if the issue is resolved and Ransack is working correctly for you with this?

Use Ransack master: gem 'ransack', github: "activerecord-hackery/ransack"

If yes, will make a release with the fix.

We still need test coverage for this behavior 😬 if anyone would like to chip in.

[haslinger]

Yes fixes it for me => PR for ransack-demo is coming in a minute.
Update: Ah - I see, you did that already.
Thanks for fixing!

[jonatack]

Great 👍. Yes, I'm overhauling ransack-demo right now. Will deploy an updated version to Heroku soon.