Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix filter on enum value in list view v2 #3684

Conversation

robotfelix
Copy link

@robotfelix robotfelix commented Jun 7, 2024

Filtering on legacy enums in a list view gives errors under Rails Admin 3.x (see #3651)

This PR adds to #3647 by adding a test for the fix provided there, as well as providing a similar fix for the Fields::Types::Enum#enum_method method.

I've left the original commit by @fuegas as-is to give due credit, but happy to squash the fix with the associated tests if preferred.

I don't fully grasp why the method is called without bindings (not even an empty hash), and it's not clear if that is something that should ever be expected or is a symptom of a bug somewhere else, however either way it is true that the code paths using abstract_model were not covered by the existing tests.

fuegas and others added 3 commits June 7, 2024 17:27
Filtering on enum (in list view) gave the following error:
```
ActionView::Template::Error (undefined method `[]' for nil:NilClass

              (bindings[:object] || abstract_model.model.new).send(enum_method)
                       ^^^^^^^^^):
    54: <div id="list">
    55:   <%= form_tag(index_path(params.except(*%w[page f query])), method: :get) do %>
    56:     <div class="card mb-3 p-3 bg-light">
    57:       <div class="row" data-options="<%= ordered_filter_options.to_json %>" id="filters_box"></div>
    58:       <hr class="filters_box" style="display:<%= ordered_filters.empty? ? 'none' : 'block' %>"/>
    59:       <div class="row">
    60:         <div class="col-sm-6">

rails_admin (3.1.2) lib/rails_admin/config/fields/types/enum.rb:32:in `block in <class:Enum>'
rails_admin (3.1.2) lib/rails_admin/config/configurable.rb:77:in `instance_eval'
rails_admin (3.1.2) lib/rails_admin/config/configurable.rb:77:in `block in register_instance_option'
rails_admin (3.1.2) lib/rails_admin/config/fields/types/enum.rb:14:in `block in <class:Enum>'
rails_admin (3.1.2) lib/rails_admin/config/configurable.rb:77:in `instance_eval'
rails_admin (3.1.2) lib/rails_admin/config/configurable.rb:77:in `block in register_instance_option'
rails_admin (3.1.2) lib/rails_admin/config/fields/base.rb:106:in `filter_options'
```

This is because bindings is nil and thus the hash lookup fails. By adding safe navigation we circumvent this error and use the second option namely `abstract_model.model.new`.
A lack of tests for these code paths led to a regression for legacy enums in Rails Admin 3.0 (railsadminteam#3651).

This tests the code path using `abstract_model` that was fixed by @fuegas in the previous commit.
Similar to `Fields::Types::Enum#enum` the code path using `abstract_model` was untested and the implementation did not account for the possibility that the `bindings` may be nil.
@mshibuya
Copy link
Member

mshibuya commented Jun 8, 2024

For now I'm not merging this as d62f604 will be the proper fix, but I really appreciate your attempt to address this.
Thank you so much, and looking forward to another contribution 👍

@mshibuya mshibuya closed this Jun 8, 2024
@robotfelix
Copy link
Author

@mshibuya Thanks. d62f604 definitely looks like a better fix! 🎉

Let me know if you'd like me to adapt the tests into some "when the binding don't provide an object" test cases (eg: with an empty binding rather than no binding) instead to add test coverage for the code paths through abstract_model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants