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

Enable ordering the BelongsTo fields by using order option. #2208

Conversation

patrickgramatowski
Copy link
Contributor

@patrickgramatowski patrickgramatowski commented Jun 7, 2022

Enable ordering the BelongsTo fields by using order option. The change let the gem users to be able to order the fields by the column they'll specify, instead of ordering by id.

I've also updated the query to solve the possible issues with PG::AmbiguousColumn errors - for now it's not handled in any way, so if the user is using the tables that use the same columns names, then the error will apear and the only way to fix that is to monkey patch the gem. To solve that, I've added the column name as the prefix for both possible forms of the query.

Based on #1215
And kinda related to #1805 the result is exactly as presented on videos attached to this task's description.

@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch 14 times, most recently from 8aa9b1a to dedec29 Compare June 8, 2022 09:39
The change let the gem users to be able to order the fields by
the column they'll specify, instead of ordering by `id`.
I've also updated the query to solve the possible issues with
`PG::AmbiguousColumn` errors - for now it's not handled in
any way, so if the user is using the tables that use the same
columns names, then the error will apear and the only way to fix
that is to monkey patch the gem. To solve that, I've added
the column name as the prefix for both possible forms of the query.
@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch from dedec29 to dd86160 Compare June 8, 2022 09:42
@patrickgramatowski patrickgramatowski marked this pull request as ready for review June 8, 2022 09:55
@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch from 0dbd2fa to 9a808a7 Compare June 14, 2022 11:44
Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

Looking good! I have left comments on minor points. Let me know what you think.

lib/administrate/order.rb Outdated Show resolved Hide resolved
lib/administrate/order.rb Outdated Show resolved Hide resolved
@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch 3 times, most recently from 03c5b62 to d38cd56 Compare July 8, 2022 07:42
@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch from d38cd56 to 7265439 Compare July 8, 2022 07:47
@patrickgramatowski
Copy link
Contributor Author

patrickgramatowski commented Jul 8, 2022

Hi @pablobm, AFAIU the failing CI tests are not related to my changes. I think I've done everything that you asked me to update/change/fix. 👍

Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

Great 🙂 Just one more thing and I think we can merge this.

docs/customizing_dashboards.md Outdated Show resolved Hide resolved
@patrickgramatowski patrickgramatowski force-pushed the enable-sort-by-association-attribute branch from 7265439 to 1491ba3 Compare July 8, 2022 20:27
Copy link
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

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

Thank you!

@pablobm pablobm merged commit c16c062 into thoughtbot:main Jul 9, 2022
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.

2 participants