Move generic_carousel() from templates to Python handlers - #13466
Conversation
github-project-automation
Bot
moved this to Waiting Review/Merge from Staff
in Ray's Project
89dce90 to
89d1364
Compare
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Looking pretty good. Two bits of feedback!
-
get_carousel_data()merges two divergent queries, which empties the staff-picks carousel for non-English UI users. It builds'languageSorter:("English")' + lang_filter, but the homepage previously used only the user-language filter (per #10691), and the loans page previously used onlylanguageSorter:("English"). The AND of the two (languageSorter:("English") AND language:spa, etc.) matches almost nothing for non-English populated languages, so the carousel goes empty. I'd unify both on just the user-language filter (lang_filter). -
/account/loansfetches both carousels and discardsrecently_returned. Consider makingget_carousel_datatake the carousel name(s) it should build (e.g. aLiteral["staff_picks", "recently_returned"], accepting one or an iterable), so loans only requests staff picks — and the loop becomes an easyasyncio.gatheronce the fetch goes async.
github-project-automation
Bot
moved this from Waiting Review/Merge from Staff
to Someone else is working on it
in Ray's Project
a3fc247 to
233c242
Compare
233c242 to
0c7d4f1
Compare
Extract the carousel family (get_carousel_data, generic_carousel, get_ia_carousel_books, format_book_data, presets/config) out of the home page controller into a new canonical openlibrary.core.carousels module, and move caching_prethread into openlibrary.utils.request_context. The homepage and loans page now both build carousels from openlibrary.core: this removes the upstream->openlibrary-plugin import in account.py and the pre-existing core->plugin inversions in lending.py and fulltext.py. The config map is flattened to name->subject, and an explicit empty carousel list now yields no carousels instead of silently falling back to all of them. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
Replace dict[str, dict[str, Any]] with CarouselData and LoadMoreConfig TypedDicts in openlibrary/core/carousels.py to make the contract explicit. No behavior change.
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
@Sanket17052006 ,as always, you did awesome on this one.
I ended up pushing some some refactors that make it so long term things are simplified by having a carousels.py file instead of being concerned about any loops. We were also able to remove custom_ia_carousel.html
To test this PR I compared the homepage and loans page before/after in english and in spanish.
As expected, the results were the same for the homepage and now the loan page sees language aware results (before it wasn't language aware).
github-project-automation
Bot
moved this from Someone else is working on it
to Done
in Ray's Project
2 participants
Part of #13419
Moves
generic_carousel()andcompose_ia_url()network calls out of the Templetor template (custom_ia_carousel.html) into Python handlers (home.py,account.py). Templates now receive pre-fetched data instead of making network calls directly.Technical
home.py-get_carousel_data()fetches books, URLs, and load_more configs; restoresuser_lang_onlylanguage filtering viaget_request_lang(); removes@publicfromgeneric_carousel()custom_ia_carousel.html- accepts pre-fetchedbooks,title,url,load_morehome/index.html- passes translated titles and pre-built load_more configs fromcarousel_dataaccount.pyandloans.html- reusesget_carousel_data()instead of duplicating carousel logictest_home.py- updated for new data flow, removed dead mock codeTesting
Screenshot
N/A
Stakeholders
@RayBB