Refactor AffiliateLinks to use async partials with parallelized fetching#12292
Refactor AffiliateLinks to use async partials with parallelized fetching#12292bhardwajparth51 wants to merge 10 commits intointernetarchive:masterfrom
Conversation
125f642 to
98811f3
Compare
45a328d to
1f95c24
Compare
8e62829 to
4f88ffb
Compare
60e0cba to
8dcd577
Compare
ba8127b to
e9afc8d
Compare
31d931f to
e9c92a0
Compare
for more information, see https://pre-commit.ci
|
@RayBB up for a review whenever you have bandwidth |
RayBB
left a comment
There was a problem hiding this comment.
@bhardwajparth51 thanks for taking a swing at this one.
Currently this PR has too much going on in one change. I think all of the ideas here are good but we should split it up more for easier review and testing.
I realize it wasn't super clear in the issue (the parent issue said it a bit more clearly) I tagged you on but this PR should NOT make the functions async. It should just focus on pulling out the code from the template into a python function that we can call to pass in the data. Doing it in a smaller step like this makes it much easier to review especially since the async code alone is a bit difficult to test.
So please remove the concurrency frontend changes and generally try to minimize the changes for this PR. The frontend change is interesting but I don't know if the added complexity is worth it since it's already high up on the page and loads often.
Thanks!
|
Hi @RayBB, apologies for the late response. I’ve been caught up with university work lately. Thanks for the feedback, I understand your point about the PR having too many moving parts. I’m currently working on simplifying it and will update the PR soon. |
…nd intersection observer
RayBB
left a comment
There was a problem hiding this comment.
For the most part this looks correct. However, a few things to fix before I start testing this.
Frontend changes are out of scope for this PR. Please remove them.
| affiliate_server_url = None | ||
| BWB_AFFILIATE_LINK = 'http://www.anrdoezrs.net/links/{}/type/dlg/http://www.betterworldbooks.com/-id-%s'.format( | ||
| h.affiliate_id('betterworldbooks') | ||
| BWB_AFFILIATE_LINK_TEMPLATE = ( |
There was a problem hiding this comment.
Can you explain the benefit of making this change? It doesn't seem needed and if it's not please remove it.
| """Standardizes how we return price info from Better World Books. | ||
|
|
||
| :param str qlt: Quality of the book, e.g. "new", "used" | ||
| :param str price: Price of the book as a decimal str, e.g. "4.28" | ||
| We collect the raw price and quality info here to make it easier | ||
| to display in the UI or use for comparison. | ||
| """ | ||
| price_fmt = f"${price} ({qlt})" if price and qlt else None | ||
| try: | ||
| bwb_id = h.affiliate_id('betterworldbooks') | ||
| except (LookupError, AttributeError): | ||
| bwb_id = '' | ||
| url = BWB_AFFILIATE_LINK_TEMPLATE.format(bwb_id) % isbn | ||
| return { | ||
| 'url': BWB_AFFILIATE_LINK % isbn, | ||
| 'url': url, |
There was a problem hiding this comment.
Same here, is there a reason we need this change?
|
@bhardwajparth51 thanks for starting this. It has become a high priority change so I'm going to need to take this over. Apologies for that. I'm converting to a draft for now. |
Refactor AffiliateLinks to Remove Template-Side Network Requests
Closes #12271
refactor Moves high-latency price fetching from the template layer to an asynchronous FastAPI partial.
Technical
AffiliateLinksPartialwithasyncio.gatherfor parallel Better World Books and Amazon API metadata fetches.affiliate-links.jsto useIntersectionObserver, deferring network requests until the "Buy" section is visible.AffiliateLinks.htmlinto a sharedget_affiliate_storeshelper invendors.py.h.affiliate_idto ensurevendors.pyfunctions work correctly in non-web contexts (tests/CLI).Testing
/books/OL7353617M)./partials/AffiliateLinks.json.pytest openlibrary/tests/core/test_vendors_async.py.Screenshot
Stakeholders
@RayBB