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

Search through association fields #1005

Conversation

jonatasrancan
Copy link
Contributor

After searching the documentation I didn't find anything about searching using the association fields, and also I didn't find any open PR related to this, so I decided to open this PR.

The approach I used was using with_options, when declaring a field on the dashboard, and added a new option called searchable_field.

The dashboard will look like this:

class StateDashboard < Administrate::BaseDashboard 
....
country: Administrate::Fields::BelongsTo.with_options(searchable: true, searchable_field: 'name')
....

end

it "builds the 'where' clause using the joined tables" do
allow(scoped_object).to receive(:joins).with([:role, :address]).and_return(scoped_object)

Choose a reason for hiding this comment

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

Use %i or %I for an array of symbols.
Line is too long. [97/80]

it "joins with the correct association table to query" do
allow(scoped_object).to receive(:where)

expect(scoped_object).to receive(:joins).with([:role, :address]).and_return(scoped_object)

Choose a reason for hiding this comment

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

Use %i or %I for an array of symbols.
Line is too long. [98/80]

Administrate::Search.new(
scoped_object,
MockDashboardWithAssociation,
"Тест Test"

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

class MockDashboardWithAssociation
ATTRIBUTE_TYPES = {
role: Administrate::Field::BelongsTo.with_options(searchable: true, searchable_field: 'name'),
address: Administrate::Field::HasOne.with_options(searchable: true, searchable_field: 'street')

Choose a reason for hiding this comment

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

Put a comma after the last item of a multiline hash.
Line is too long. [99/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -13,6 +13,13 @@ class MockDashboard
}
end

class MockDashboardWithAssociation
ATTRIBUTE_TYPES = {
role: Administrate::Field::BelongsTo.with_options(searchable: true, searchable_field: 'name'),

Choose a reason for hiding this comment

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

Line is too long. [98/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

primary_key: :code,
foreign_key: :country_code,
searchable: true,
searchable_field: 'name'

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[
Administrate::Field::BelongsTo,
Administrate::Field::HasMany,
Administrate::Field::HasOne

Choose a reason for hiding this comment

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

Put a comma after the last item of a multiline array.

attr_reader :resolver, :term

private

Choose a reason for hiding this comment

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

Useless private access modifier.


def column_to_query(attr)
if association_search?(attr)
ActiveRecord::Base.connection.quote_column_name(attribute_types[attr].searchable_field)

Choose a reason for hiding this comment

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

Line is too long. [95/80]

if association_search?(attr)
ActiveRecord::Base.connection.quote_table_name(attr.to_s.pluralize)
else
ActiveRecord::Base.connection.quote_table_name(@scoped_resource.table_name)

Choose a reason for hiding this comment

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

Line is too long. [83/80]


it "builds the 'where' clause using the joined tables" do
allow(scoped_object).to receive(:joins).with(%i(role address))
.and_return(scoped_object)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

allow(scoped_object).to receive(:where)

expect(scoped_object).to receive(:joins).with(%i(role address))
.and_return(scoped_object)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

),
address: Administrate::Field::HasOne.with_options(
searchable: true,
searchable_field: "street"

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

ATTRIBUTE_TYPES = {
role: Administrate::Field::BelongsTo.with_options(
searchable: true,
searchable_field: "name"

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

end

class MockDashboardWithAssociation
ATTRIBUTE_TYPES = {

Choose a reason for hiding this comment

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

Freeze mutable objects assigned to constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know why this still here, it's fixed

double(columns_hash: { column.to_s => :column_info })
double(
columns_hash: { column.to_s => :column_info },
table_name: "table_name"

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

def column_to_query(attr)
if association_search?(attr)
ActiveRecord::Base.connection
.quote_column_name(attribute_types[attr].searchable_field)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

ActiveRecord::Base.connection.quote_table_name(attr.to_s.pluralize)
else
ActiveRecord::Base.connection
.quote_table_name(@scoped_resource.table_name)

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch from 63b56cd to 6141d82 Compare October 16, 2017 19:53
@jonatasrancan jonatasrancan changed the title Feature/search through association fields Search through association fields Oct 24, 2017
@matheuscla
Copy link

Up

@nicolaslazartekaqui
Copy link

up

@nickcharlton
Copy link
Member

Hi @jonatasrancan! Thank you for contributing this. This looks good to me!

Would you be able to look at that final Hound comment?

And add something to the docs about the new feature?

@jonatasrancan
Copy link
Contributor Author

@nickcharlton I think that last Hound comment, is a bug from github, I fixed that, but it continue to appear.
I will update the docs about this.

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch from 6141d82 to 89cfaba Compare December 20, 2017 17:55
@@ -171,6 +171,7 @@ def relation_with_column(column)
double(
klass: double(reflect_on_association: nil),
columns_hash: { column.to_s => :column_info },
table_name: "table_name"

Choose a reason for hiding this comment

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

Put a comma after the last parameter of a multiline method call.

@@ -43,7 +43,7 @@

scenario "user clicks through to the new page" do
visit admin_log_entries_path
click_on("New log entry")
click_on t("administrate.actions.new_resource", name: 'log entries')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -9,7 +9,7 @@ def apply(relation)
return order_by_association(relation) unless
reflect_association(relation).nil?

return relation.reorder("#{attribute} #{direction}") if
return relation.reorder("#{relation.table_name}.#{attribute} #{direction}") if

Choose a reason for hiding this comment

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

Line is too long. [84/80]

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch from 89cfaba to d569835 Compare December 20, 2017 18:00
relation.columns_hash.keys.include?(attribute.to_s)
order = "#{relation.table_name}.#{attribute} #{direction}"

return relation.reorder(order) if relation.columns_hash.keys.include?(attribute.to_s)

Choose a reason for hiding this comment

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

Line is too long. [91/80]

relation.columns_hash.keys.include?(attribute.to_s)
order = "#{relation.table_name}.#{attribute} #{direction}"

return relation.reorder(order) if relation.columns_hash.keys.include?(attribute.to_s)

Choose a reason for hiding this comment

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

Line is too long. [91/80]

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch from d569835 to e999cb4 Compare December 20, 2017 18:06
@jonatasrancan
Copy link
Contributor Author

@nickcharlton This is ready.
The broken test is related to #1054.
Let me now when it get merged, so I can update my fork.

@nickcharlton
Copy link
Member

Hi @jonatasrancan! #1054 is now merged.

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch 2 times, most recently from 76d5087 to 0b57c02 Compare January 16, 2018 12:15
@jonatasrancan
Copy link
Contributor Author

@nickcharlton the PR is updated and all green 😄

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch 2 times, most recently from d27bdb5 to 0ae7f5a Compare February 1, 2018 18:53
@dsolomadin
Copy link
Contributor

@nickcharlton It would be so nice to have this in next release :)

@nickcharlton
Copy link
Member

Thanks for doing that!

@jonatasrancan I had a quick go myself, but rebasing got a bit knarly. Would you be able to take another look so we can get this merged in?

@jonatasrancan jonatasrancan force-pushed the feature/search-through-association-fields branch from 0ae7f5a to 693b047 Compare March 2, 2018 19:12
@jonatasrancan
Copy link
Contributor Author

@nickcharlton all done 😃

@nickcharlton nickcharlton force-pushed the feature/search-through-association-fields branch from 693b047 to 1a47be0 Compare March 2, 2018 21:02
@nickcharlton nickcharlton merged commit 71a5bd0 into thoughtbot:master Mar 2, 2018
@nickcharlton
Copy link
Member

Great, thanks! Merging away…

@jonatasrancan jonatasrancan deleted the feature/search-through-association-fields branch March 5, 2018 13:12
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.

7 participants