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.

Sign up for GitHub

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

Jump to bottom

with_rich_text_#{name} eager loading does not work #18

Closed
sedubois opened this issue Jan 10, 2022 · 5 comments · Fixed by #24
Closed

with_rich_text_#{name} eager loading does not work #18

sedubois opened this issue Jan 10, 2022 · 5 comments · Fixed by #24

Comments

@sedubois
Copy link
Owner

This test currently does not pass:

mobility-actiontext/test_app/test/mobility_action_text_test.rb

Lines 98 to 106 in 8ad7fbd

test 'post content is eager loaded' do
post = assert_queries(2) { Post.with_rich_text_content.last }
assert_no_queries do
assert_equal 'Hello world!', post.content.to_plain_text
skip('FIXME: this should execute no queries')
end
end

Creating this issue for visibility, in case someone would be willing to investigate and offer a PR.
@doits
Copy link
Contributor

doits commented Feb 23, 2022 •
edited
Loading

Just found out you can do Post.includes(:rich_text_translations) to eager load all translations, so this could maybe be used by .with_all_rich_text?

Not sure how to eager load one specific rich text attribute with this though.

@sedubois
Copy link
Owner Author

sedubois commented Feb 26, 2022 •
edited
Loading

That's good to know @doits, would you feel like submitting a PR (with test) for implementing with_all_rich_text?

@doits doits mentioned this issue Feb 26, 2022
Merged
@doits
Copy link
Contributor

doits commented Feb 26, 2022

Sure, gave it a try in #23.

@sedubois sedubois changed the title with_rich_text eager loading does not seem to work with_rich_text_#{name} eager loading does not work Feb 27, 2022
@sedubois
Copy link
Owner Author

Thanks @doits! with_all_rich_text is now fixed by #23. with_rich_text_#{name} still needs fixing.

doits added a commit to doits/mobility-actiontext that referenced this issue Feb 27, 2022
@doits
make eager loading single translation work
efde31f 
Since mobility's key value store [loads all translations][1] we check if
our is already eager loaded by using [association_cached?][2] before and
return it if the locale matches.

fixes sedubois#18

[1]: https://github.com/shioyama/mobility/blob/4bf96b713991d5cf379aa6caeae2a3c448c73302/lib/mobility/backends/active_record/key_value.rb#L226
[2]: https://github.com/rails/rails/blob/75a9e1be75769ae633a938d81d51e06852a69ea3/activerecord/lib/active_record/associations.rb#L311
@doits doits mentioned this issue Feb 27, 2022
Merged
@doits
Copy link
Contributor

doits commented Feb 27, 2022

@sedubois see #24 for a possible fix.

sedubois pushed a commit that referenced this issue Feb 28, 2022
@doits
make eager loading single translation work (#24)
d02cae1 
Since mobility's key value store [loads all translations][1] we check if ours is already eager loaded by using [association_cached?][2] before and return it if the locale matches.

fixes #18

[1]: https://github.com/shioyama/mobility/blob/4bf96b713991d5cf379aa6caeae2a3c448c73302/lib/mobility/backends/active_record/key_value.rb#L226
[2]: https://github.com/rails/rails/blob/75a9e1be75769ae633a938d81d51e06852a69ea3/activerecord/lib/active_record/associations.rb#L311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

make eager loading single translation work doits/mobility-actiontext
2 participants
@doits @sedubois