AO3-7457 Preload unrevealed collections for work blurbs - #5907
AO3-7457 Preload unrevealed collections for work blurbs#5907nateberkopec wants to merge 11 commits into
Conversation
nateberkopec
changed the title
73fd150 to
9d22ce5
Compare
| <% if item.approved_collections.unrevealed.present? %> | ||
| <h5 class="heading"><%= h(ts("Part of ")) + show_collections_data(item.approved_collections.unrevealed) %></h5> | ||
| <% if approved_unrevealed_collections.any? %> | ||
| <h5 class="heading"><%= h(t(".part_of", default: "Part of ")) + show_collections_data(item.approved_unrevealed_collections) %></h5> |
There was a problem hiding this comment.
Change is unrelated but was enforced on me by Rubocop
4755e28 to
0f54639
Compare
0f54639 to
19131f7
Compare
sarken
changed the title
|
Noticed some missing stuff re polymorphism. |
Bilka2
left a comment
Bilka2
left a comment
There was a problem hiding this comment.
Thank you a lot for this, especially the additions of for_blurb all over the place!
I mostly have a bunch of questions to make sure I understand the reasoning behind specific implementations that you went for (and to document them for future!us via comments here on GitHub)
|
|
||
| ActiveRecord::Associations::Preloader.new( | ||
| records: work_items, | ||
| associations: [:pseuds, :approved_unrevealed_collections] |
There was a problem hiding this comment.
Why only some of the associations, not all from Work's with_includes_for_blurb, similar to how you do it for bookmarks here?
| collection_item.item if collection_item.item_type == "Bookmark" | ||
| end | ||
|
|
||
| ActiveRecord::Associations::Preloader.new( |
There was a problem hiding this comment.
I suppose my general question is why we need this code that seems to grab into Rails internals instead of being able to use something like includes(work: :pseuds, bookmark: :bookmarkable) - does Rails not support that?
Edit: I remembered that we were told not to set up the work and bookmark associations here the way we are, but my question still stands if we scoped them by type instead
github-actions
Bot
added
Coder Has Actioned Review
and removed
Reviewed: Action Needed
labels
2 participants
Issue
https://otwarchive.atlassian.net/browse/AO3-7457
Purpose
This removes an N+1 when rendering mystery blurbs on many, many different pages.
Prior to this change, rendering a mystery blurb called:
This is a guaranteed N+1, because scope calls on associations cannot be preloaded.
The fix here is to change that into a relation,
approved_unrevealed_collections, which can be preloaded.As a regression test/enforcement mechanism, we use strict_loading: true to ensure that exceptions are raised in dev/test if this new association is ever not preloaded. Once that exception was raised, all I had to do was backfill through all the controllers to add
for_blurbpreloads where missing and then the exceptions went away.Because this adds
for_blurbpreloads in a number of places, it probably also removes additional N+1s from the non-mystery blurb as well.This is part of (but does not close) AO3-7457 because it is cleaning up data access for this set of partials before adding fragment caching around it.