openlibrary icon indicating copy to clipboard operation
openlibrary copied to clipboard

Make BWB + Amz prices lazy load on editions page

Open mekarpeles opened this issue 1 year ago • 10 comments

Next Steps

  • [ ] Decide on approach (e.g. partials + js to fetch prices?) @jimchamp
  • [ ] Needs breakdown on which files to edit
  • [ ] Needs instructions for how to test -- for now if BWB doesn't return back data, have the partial endpoint return static/fake data. Ignore amz for now.

Design

Patron should see the affiliate/vendors table but show static "tomb stone" grey placeholders until the amz + bwb data becomes available.

Describe the problem that you'd like solved

Justification: Currently BWB + amazon are expensive network calls which result in slow books page load times in sentry

We want to load this data async to speed up the perf.

Proposal & Constraints

Additional context

Stakeholders

mekarpeles avatar Nov 30 '23 20:11 mekarpeles

can i work on this?

nick2432 avatar Dec 17 '23 02:12 nick2432

@mekarpeles Please assign me this issue.

tusharv01 avatar Dec 20 '23 07:12 tusharv01

@nick2432 I've assigned you to work on this (sorry for the delay).

Here's some more information about this:

The template for our affiliate links is here. I don't think that this file will need to be modified at all, but it will need to be rendered server-side and returned to the client.

The template for our book page is here. Currently, we store the links as affiliate_links and render this value twice on the book page. This will need to be updated to instead store the HTML for the "tomb stone" placeholder. You can define a new template function that takes isbn, asin, prices, and edition_key as parameters and returns a placeholder element that includes data attributes for each parameter. These values will be used to fetch the HTML for the AffiliateLinks macro.

You'll have to create a new endpoint which will be used to fetch the rendered AffiliateLinks. This can be added to this file for now. You can model the GET handler on this handler, which returns partials for another feature. Since AffiliateLinks.html is macro, the render_template call will need to be replaced with something like macro.macrostore['AffiliateLinks'](doc, opts), where doc is the edition, and opts is a dict containing asin, isbn, and prices. You can get a reference to the edition by calling web.ctx.site.get(edition_key).

The path for the new endpoint can be something like /affiliate-links/partials.

Finally, you'll need to replace the placeholder with the HTML for the affiliate links. Make sure to only fetch the links once per book page, and replace both placeholders with the results.

jimchamp avatar Dec 21 '23 20:12 jimchamp

Please assign me to this issue

spurjhattam2207 avatar Jan 12 '24 16:01 spurjhattam2207

@jimchamp Please assign me this issue on a first come basis I am the one whom you assign sir.

tusharv01 avatar Jan 14 '24 12:01 tusharv01

@tusharv01 happy to assign you, before starting work, can you please post a comment here describing how you will implement the solution and make sure you tag @jimchamp so he can review your approach. Thank you!

mekarpeles avatar Jan 16 '24 20:01 mekarpeles

@mekarpeles @jimchamp Proposed Implementation Approach:

1. Update AffiliateLinks Template:

- Ensure the AffiliateLinks.html template includes necessary placeholders for BWB and Amazon prices.

2. Update Book Page Template:

- Replace the current rendering of 'affiliate_links' with a placeholder element.

- Create a new template function, render_affiliate_links_placeholder, to generate the placeholder.

3. Create a New Endpoint for AffiliateLinks:

- Add a new endpoint '/affiliate-links/partials' to fetch rendered AffiliateLinks.

- Model the GET handler on an existing handler, using the macro.macrostore['AffiliateLinks'](doc, opts) syntax.

4. Replace Placeholders with HTML:

- In the JavaScript part of book_page_template.html, fetch affiliate links using the new endpoint.

- Replace placeholders with the fetched HTML.

tusharv01 avatar Jan 20 '24 07:01 tusharv01

@tusharv01 are you still working on this issue?

QuantuM410 avatar Jan 31 '24 16:01 QuantuM410

@tusharv01 are you still working on this issue?

Yes

tusharv01 avatar Jan 31 '24 16:01 tusharv01

@tusharv01, this issue is rising in priority, and is something that we'd like finished relatively soon. When do you expect to open a PR with your solution?

Edit: Just noticed that you don't have the repo forked, so I'm assuming that you are not working on this. Assigning myself.

jimchamp avatar Feb 14 '24 18:02 jimchamp