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

Fix ordering HasOne fields without explicit order #2584

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

mikz
Copy link
Contributor

@mikz mikz commented May 13, 2024

This fixes a bug in awesome #2325.

Ordering by a primary key of the HasOne field wasn't working
and resulted in invalid sql, because it did not join the associated table.

This unifies the code paths to use the same logic to add the ordering
from another table and use reflection to get relevant table information.

Ordering by a primary key of the HasOne field wasn't working
and resulted in invalid sql, because it did not join the table.

This unifies the code paths to use the same logic to add the ordering
from another table and use reflection to get relevant table information.
@mikz
Copy link
Contributor Author

mikz commented May 25, 2024

@nickcharlton looks like even after #2498 approval is needed to run the workflow in a fork. Could I get approval from someone?

@shouichi
Copy link
Contributor

shouichi commented Jun 4, 2024

I also encountered this problem. I tried this PR locally and it worked as expected 👍

@nickcharlton
Copy link
Member

@nickcharlton looks like even after #2498 approval is needed to run the workflow in a fork

Yeah, this is, unfortunately, expected behaviour. That PR allowed running at all, but then you need a maintainer (or equivalent, I don't remember the default roles off the top of my head) to approve them to run. The intention is to protect against data exfil, I think, or using lots of compute, maybe? It doesn't really apply to this project, so it's just another hurdle to have to jump over.

@mikz
Copy link
Contributor Author

mikz commented Jun 27, 2024

@nickcharlton yeah, I understand. But can you approve it, so we can get a build and merge this fix?

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.

I've tried to finish reviewing this four times, but kept getting pulled away 🤦.

Anyway, this looks good, thanks!

@nickcharlton nickcharlton merged commit 5e2fda6 into thoughtbot:main Jul 1, 2024
@mikz
Copy link
Contributor Author

mikz commented Jul 1, 2024

@nickcharlton Thank you very much! 🎉

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