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

Order attributes by association if it exists #999

Merged
merged 6 commits into from
Dec 19, 2017

Conversation

rnice01
Copy link
Contributor

@rnice01 rnice01 commented Oct 8, 2017

Adding an order clause to the relation for an associated attribute was having no effect on the dashboard's sorting. When the order is applied, the current relation will check if the attribute is an association and reorder by count on the attribute's id column or on the relation's {attribute}_id column following the Rails convention of naming FK columns. The relation order method was changed to reorder to override any default scoping that may exist for the relation.

#471

lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
@rnice01 rnice01 force-pushed the scope-sort-to-association branch 2 times, most recently from e9b9f98 to 72a5bd6 Compare October 8, 2017 03:51
relation.
left_joins(attribute.to_sym).
group(:id).
reorder("COUNT(#{attribute}.id) #{direction}")

Choose a reason for hiding this comment

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

Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

def order_by_count(relation)
relation.
left_joins(attribute.to_sym).
group(:id).

Choose a reason for hiding this comment

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

Use 2 (not 0) spaces for indenting an expression spanning multiple lines.


def order_by_count(relation)
relation.
left_joins(attribute.to_sym).

Choose a reason for hiding this comment

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

Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

@rnice01 rnice01 force-pushed the scope-sort-to-association branch from 72a5bd6 to 5054c7e Compare October 8, 2017 03:53
@rnice01
Copy link
Contributor Author

rnice01 commented Oct 8, 2017

I'm not sure if I have completely missed the mark or not with these changes for #471 would love some feedback so I can make changes as needed and add specs as well.

@nickcharlton
Copy link
Member

Hi @rnice01!

This looks like it's off to a good start and should solve a couple of issues in one go (#471 and #265). Do you think you'd be able to contribute some tests?

@rnice01
Copy link
Contributor Author

rnice01 commented Oct 16, 2017

@nickcharlton
Thanks for following up! Yes, I can definitely add some tests for this!

spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
@rnice01 rnice01 force-pushed the scope-sort-to-association branch from f5e3066 to dadf9f2 Compare October 28, 2017 10:24
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

This is looking good! There's a couple of Hound comments and I left a couple, too.

lib/administrate/order.rb Outdated Show resolved Hide resolved
spec/lib/administrate/order_spec.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/example_app/app/policies/application_policy.rb Outdated Show resolved Hide resolved
app/controllers/administrate/application_controller.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/lib/fields/date_time_spec.rb Outdated Show resolved Hide resolved
spec/example_app/app/policies/application_policy.rb Outdated Show resolved Hide resolved
app/controllers/administrate/application_controller.rb Outdated Show resolved Hide resolved
Adding an order clause to the relation for an associated
attribute was having no effect on the dashboard's sorting.
When the order is applied, the current relation will check
if the attribute is an association and reorder by count
on the attribute's id column or on the relation's
{attribute}_id column following the Rails convention
of naming FK columns. The relation order method was
changed to reorder to override any default scoping
that may exist for the relation.

thoughtbot#471
@rnice01 rnice01 force-pushed the scope-sort-to-association branch from 46f160b to 564a86e Compare December 15, 2017 04:33
@nickcharlton
Copy link
Member

Thanks! I'm going to merge this as the two CI failures are around elsewhere, too.

@nickcharlton nickcharlton merged commit 218cb9d into thoughtbot:master Dec 19, 2017
@rnice01
Copy link
Contributor Author

rnice01 commented Dec 20, 2017

Awesome, thanks for all the help!

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