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

Tables won't always be named after models #1569

Merged
merged 1 commit into from
Mar 21, 2020

Conversation

pablobm
Copy link
Collaborator

@pablobm pablobm commented Mar 13, 2020

Alternative to #1264.
Fixes #1179.

Currently, Administrate::Search::Query makes the assumption that an association's table name will have the same name as the association attribute, ignoring a :class_name option that signals otherwise.

This change uses the :class_name option when given.

Changing the example app also provides a way to test this as part of the existing feature specs.

@pablobm pablobm force-pushed the association-search-table-name branch 2 times, most recently from 9376645 to 3d59c6b Compare March 13, 2020 18:57
@pablobm
Copy link
Collaborator Author

pablobm commented Mar 13, 2020

@sedubois @ksearfos @bjgaynor - Would you be able to check if this fixes your issues? Thank you.

@pablobm pablobm force-pushed the association-search-table-name branch from 3d59c6b to b2cb529 Compare March 13, 2020 19:09
@sedubois
Copy link
Contributor

@pablobm I tried to load the branch in my project as follows:

# Gemfile
gem 'administrate', git: 'https://github.com/pablobm/administrate.git', branch: 'association-search-table-name'

But I get

$ bundle 
Fetching https://github.com/pablobm/administrate.git
fatal: Could not parse object 'b2cb529f710915bb12ade19b2b9cd6ca30f5496a'.
Revision b2cb529f710915bb12ade19b2b9cd6ca30f5496a does not exist in the repository https://github.com/pablobm/administrate.git. Maybe you
misspelled it?

Any idea what I might be doing wrong?

@pablobm
Copy link
Collaborator Author

pablobm commented Mar 14, 2020

That's strange. It looks to me like a GitHub issue. I upgraded from Administrate 0.12 to 0.13 on a dummy app and it installed the SHA that your setup claims to be missing:

Using administrate 0.12.0 (was 0.13.0) from https://github.com/pablobm/administrate.git (at association-search-table-name@b2cb529)

That SHA is also present on Github at pablobm@b2cb529

Perhaps try going back to 0.12, bundle, then 0.13 again? Or wiping out the gem from your machine and trying again. For example, in my machine it's at ~/.asdf/installs/ruby/2.6.5/lib/ruby/gems/2.6.0/bundler/gems/administrate-b2cb529f7109. I use asdf, so it will be different with rbenv, RVM, etc.

@sedubois
Copy link
Contributor

sedubois commented Mar 14, 2020

@pablobm no luck but as a workaround I cloned your branch and used gem 'administrate', path: '~/Repos/administrate-pablobm'.

I encountered one issue: association_search? expects the field to be HasOne, HasMany or BelongsTo, but my field is NestedHasMany. It seems to be fixed as follows:

# lib/administrate/search.rb

    def association_search?(attribute)
      return unless attribute_types[attribute].respond_to?(:deferred_class)
      
      attribute_types[attribute].deferred_class < Administrate::Field::Associative
    end

EDIT: updated code above which contained an error.

EDIT2: this is actually the same problem that happened in base_controller.rb as discussed here, there seems to be an opportunity to DRY up the code.

@sedubois
Copy link
Contributor

sedubois commented Mar 15, 2020

@pablobm I updated previous comment, with that change now the search works for me 🎉

@sedubois
Copy link
Contributor

@pablobm also this PR seems partly duplicate of #1203. Would it be possible to integrate the changes it contains to support multiple association search fields (searchable_fields array)?

@pablobm pablobm force-pushed the association-search-table-name branch from b2cb529 to 3719ce5 Compare March 17, 2020 15:30
@pablobm
Copy link
Collaborator Author

pablobm commented Mar 17, 2020

Uh, I thought I had merged a fix for this some time ago. Turns out I hadn't: #1398 Merged now.

That other PR I just merged takes a bit more of a heavy-handed approach. I like your solution using < instead. Would you be up for a separate MR that refactors that method, and also the following (mentioned on the PR) to use your approach?

def association_classes
@association_classes ||=
ObjectSpace.each_object(Class).
select { |klass| klass < Administrate::Field::Associative }
end

As for this PR here, I just rebased it. Would you be able to check that the problem is actually fixed now?

@sedubois
Copy link
Contributor

sedubois commented Mar 19, 2020

@pablobm I confirm the search now works for my Post::Translation associations of type NestedHasMany without any extra modifications (tested with gem 'administrate', github: 'pablobm/administrate', branch: 'association-search-table-name').

I've also created PR #1576.

@pablobm pablobm merged commit 9135c97 into thoughtbot:master Mar 21, 2020
@pablobm
Copy link
Collaborator Author

pablobm commented Mar 21, 2020

Thank you @sedubois!

@nickcharlton nickcharlton added models models, associations and fetching the underlying data search finding things through our models labels Mar 26, 2020
pedantic-git pushed a commit to fishpercolator/administrate that referenced this pull request May 7, 2020
@pablobm pablobm deleted the association-search-table-name branch June 24, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
models models, associations and fetching the underlying data search finding things through our models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error on Search on belongs_to with class_name option
3 participants