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

Support association search for other types of association fields #1398

Merged
merged 2 commits into from
Mar 17, 2020

Conversation

josephktcheung
Copy link
Contributor

@josephktcheung josephktcheung commented Aug 13, 2019

This issue and the proposed fix is very similar to #1176

Right now association search doesn't work for new types of association fields.

Administrate::Search hard-codes the possible types of association fields in a list to determine if association search should be used. This means that new types of fields cannot perform association search and things break.

My proposed solution (see lib/administrate/search.rb) does a programmatic search for field classes that inherit from Administrate::Field::Associative. This allows plugin authors not to worry about adding their new type to a list or any other sort of setup.

A potential problem with this approach is that it includes Administrate::Field::Polymorphic on the list. I don't know if this can cause problems down the line or not.

].include?(attribute_types[attribute].deferred_class)
association_classes.include?(attribute_types[attribute].deferred_class)
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/TrailingWhitespace: Trailing whitespace detected.

@jshum
Copy link

jshum commented Sep 18, 2019

^ any updates on this? I'm seeing this issue as well on my setup
cc: @nickcharlton

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative to mapping over ObjectSpace (and maybe even skipping the memoization), have you considered using Class#descendants?

class C; end
C.descendants # => []

class B < C; end
C.descendants # => [B]

class A < B; end
C.descendants # => [B, A]

class D < C; end
C.descendants # => [B, A, D]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use ancestors from the deferred_class to see if it includes Administrate::Field::Associative. This would allow for any plugins that extend the BelongTo, HasMany etc to also be included.

This could be:

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

Copy link
Collaborator

@pablobm pablobm Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In defence of this solution, I'd say it just does the same thing that already happens at lib/administrate/base_dashboard.rb (and I myself introduced at #1176).

I'd be happy with merging this, then having another PR to refactor both instances.

@nickcharlton nickcharlton added feature new functionality that’s not yet implemented search finding things through our models labels Jan 2, 2020
@pablobm pablobm merged commit bab5bb7 into thoughtbot:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new functionality that’s not yet implemented search finding things through our models
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants