7311/feature/loan history code for rendering loan_history page

[anujamerwade]

Closes #7311

Feature: This is a draft PR to create a borrow history page on openlibrary.org instead of being redirected to the internetarchive.org currently.

Technical

Testing

Screenshot

Stakeholders

[mekarpeles]

See also: https://github.com/internetarchive/openlibrary/wiki/Developing-The-Reading-Log

The goal is to:

• create a route /account/loan-history which gets caught by a new route in plugins/upstream/mybooks.py
  • This route then calls the MyBooksTemplate renderer (the same controller which builds all my books pages). In each case where MyBooksTemplate is used to render one of a patron's "my books" pages (e.g. their main dashboard, their lists, their reading log, their loans, etc), corresponding models are used to fetch data for that view and is then get passed in to account/books.html which is a base template that will render the sidebar and, to the right of it, the main content template.
• We'll update the MyBooksTemplate controller to be able to respond to a new key called loan_history which will use a patron's s3 keys to fetch their loan history and then pass these books into a new template called account/loan_history.html which will share a similar design to account/readinglog.html
  • The MyBooksTemplate controller will need to define new logic to get the patron's loan_history of archive.org loans and then use this list of archive.org identifiers to fetch the corresponding items on Open Library. The availability API can be used to achieve this. The code is shown below:

First, get the loan_history and extract a list of archive.org identifiers (we call ocaids)

loan_history = s3_loan_api(
    s3_keys=s3_keys, action='user_borrow_history').json()['history']['items']
ocaids = [loan_record['identifier'] for loan_record in loan_history]

Next, get the availability of these ocaids, which will also give us the openlibrary edition keys that we can use to fetch the records on Open Library:

• availability = get_availability_of_ocaids(ocaids)

Next, fetch the editions corresponding to these books:

editions = web.ctx.site.get_many(
            [
                '/books/%s' % availability[ocaid].get('openlibrary_edition')
                for ocaid in availability
                if availability[ocaid].get('openlibrary_edition')
            ]
        )

editions is now what we pass in to our template as docs. The one remaining problem is that the availability documents and the loan_history documents we've computed are not attached to the editions we just fetched... So we need to shuffle/weave availability and loan_history into editions:

availability is already a dictionary that maps ocaid to its availability so it's easy to loop over each edition, lookup its availability by ocaid, and attach the availability to the edition.
But loan_history is a list of objects that look like {'userid': '@username', 'listname': 'loan_history', 'identifier': 'archiveorg_book_identifier', 'updatedate': '2023-09-17 23:16:06', 'type': 'SESSION_LOAN'} and we need to instead transform it into a dictionary that similarly maps ocaid to loan_history record. Let's do that first...

loan_history_map = {}
for loan_record in loan_history_map:
ocaid = loan_record['identifier']
loan_history_map[ocaid] = loan_record

Now, we loop over each edition, and we use the edition's ocaid (if it exists) to look up the availability from the availability map and the loan_record from the loan_history_map:
for ed, i in enumerate(editions):

• loan_history_map = {loan_record['identifier']: loan_record for loan_record in loan_history}

All together, including the fetch of patron s3 credentials, the code looks something like:

from openlibrary.accounts.model import OpenLibraryAccount
user = accounts.get_current_user()
account = user and OpenLibraryAccount.get_by_username(user.username)
s3_keys = web.ctx.site.store.get(account._key).get('s3_keys')
loan_history = s3_loan_api(
    s3_keys=s3_keys, action='user_borrow_history').json()['history']['items']
ocaids = [loan_record['identifier'] for loan_record in loan_history]
availability = get_availability_of_ocaids(ocaids)
loan_history_map = {loan_record['identifier']: loan_record for loan_record in loan_history}
editions = web.ctx.site.get_many([
    '/books/%s' % availability[ocaid].get('openlibrary_edition')
    for ocaid in availability
    if availability[ocaid].get('openlibrary_edition')
])
for ed, i in enumerate(editions):
    if ed.ocaid in ocaids:
        # attach availability and loan to edition
        editions[i].availability = availability.get(ed.ocaid)        
        editions[i].loan = loan_history_map[ed.ocaid]

[scottbarnes]

Hi, @anujamerwade. Thanks for this!

I tested it out, and there are a few things we should probably still address prior to merging this PR.

1. pagination shows the same results on each page;
2. each page shows all the results, even if the results_per_page value causes pagination to display (e.g. if there are 55 results, 3 pages will display by default, with a theoretical 25 per page, but each page will have all 55 results displayed);
3. book availability not working properly -- books that should say 'borrow' say 'read' for some reason;
4. the IA loan API is returning all loans, even those that aren't in OL. So the API might return 100 results, but, say, 93 might show up in OL, because we account for the fact that not every result from the IA loan API will be in OL. But this means the "only 100 results" message won't display, as it will see only 93 results, rather than the 100 returned.

Possible (partial) solutions

• to address the pagination issue, $:macros.Pager(current_page, doc_count, results_per_page=results_per_page) could be commented out in loan_history.html until we deal with pagination properly.
• a non-technical response to the difference between the number of OCAIDs returned from the IA loan-history endpoint and the results that actually show up in loan_history_map might be to change the language about the result limit in loan_history.html to say Loan history currently limited to at most 100 records, and then edit loan_history.htmlto show that message when ifdoc_count == 100, (as opposed to comparing len(docs)` to 100.)

I am not sure why the borrow buttons (almost) all show read, but that is probably also something we should fix prior to merging. I looked a bit and didn't immediately see a solution, but let me know if you'd like more help. :)

Finally, there is some commented out code that we should probably remove, e.g., in openlibrary/plugins/upstream/mybooks.py.

[anujamerwade]

Hi @scottbarnes , thank you for pointing out the errors. I will work on them this week and get back to you in case of any issues.

[scottbarnes]

I looked at this a bit more and the buttons showing read rather than borrow seem to be down to editions[i].loan = loan_history_map[ed.ocaid] in mybooks.py, because this makes user_loan True in openlibrary/macros/LoanStatus.html, and if that's true, the ReadButton macro is called like so:

$if user_loan:
  $:macros.ReadButton(ocaid, loan=user_loan, listen=listen)
  $if secondary_action:
    $:macros.ReturnForm(ocaid)
    $:macros.FormatExpiry(user_loan['expiry'])

rather than

$elif availability.get('is_lendable'):
  $if secondary_action:
    $:macros.BookPreview(ocaid)
  $if availability.get("available_to_borrow") or availability.get("available_to_browse"):
    $:macros.ReadButton(ocaid, borrow=True, listen=listen)
  $elif availability.get('available_to_waitlist'):
  ... more here ...

That said, at least in the development environment, these buttons don't appear to work correctly on the reading log or in the loan history, insofar as ensuring user_loan is False and thereby enabling both the reading log and the loan history to render the LoanStatus macro with the same code still makes it so that if an item is checked out and not currently borrowable, the button still shows 'borrow', which would probably annoy a patron when they find out the book can't be borrowed.

[anujamerwade]

@scottbarnes and I have tried debugging the reason why we face challenges while moving through pages on the loan-history page. The issues we have encountered are as follows:

1. We have observed that passing limit and offset params in the s3_loan_api yield different, randomized order of books than without passing those parameters.
2. For consecutive pages, the books are repeated.
3. editions does not return 1:1 mapping for the openlibrary identifiers due to which some books are missing in the history list.
4. The order in which identifiers are returned by the s3_loan_api is random(even though default is to return in reverse chronological order). Due to this, we have sorted the result of the API in this PR.

@mekarpeles your suggestion would be helpful.

[scottbarnes]

Just a few suggestions.

[scottbarnes]

I updated this to handle the case where there are no loan history results from Internet Archive that are in in Open Library for a given page of loans -- this logic will keep advancing pages until there is at least 1 loan from Internet Archive that is in Open Library.

However, there are at least two issues with this approach.

First, the code isn't currently tracking the state, so when the page renders the next and previous buttons, the template has no idea whether the previous page had no results. Therefore, for example, if the first three pages have no results, then, when viewing the fourth page, the patron will see a previous button, as they're on page 4, even though the three previous pages are blank.

This leads to the second limitation. Advancing through 'empty' pages where no item in the Internet Archive history is in Open Library is forward-looking only. This means that if someone clicks previous after having been forwarded, behind the scenes, past completely empty pages devoid of books in Open Library, the loan-history code will see the page is 'empty', and forward right back to the page the patron was on when they clicked previous.

I am unsure of the best way to address this, but I was thinking of adding another URL parameter that is something like previous_click=1, and this can be used to tell the loan-history Python code that the skip-pages-of-books-loans-not-in-Open-Library should run in reverse.

As for the case where the final page in the loan history is one that has no items in Open Library, the page will simply display No loans found in your borrow history., and not have any previous button, again because of the lack of tracking any sort of state. I can add more tracking if this seems like it's worth doing, but I wonder if, for this edge case at least, the logic might clutter the code up more than it's worth.

Thoughts?

Granted, the right way to handle this would be modify the IA API to return only items with OL editions, as Drini suggested. I'll investigate this more today.

Finally, when @mekarpeles's mybooks.py PR is merged, I will rebase this and refactor the code a bit to be more in line with the updated format.

@mekarpeles, @cdrini, @jimchamp, @anujamerwade.

[mekarpeles]

@mekarpeles to review https://git.archive.org/ia/petabox/-/merge_requests/3878

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7927be4) 16.62% compared to head (939a621) 16.62%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8375   +/-   ##
=======================================
  Coverage   16.62%   16.62%           
=======================================
  Files          88       88           
  Lines        4698     4698           
  Branches      838      838           
=======================================
  Hits          781      781           
  Misses       3399     3399           
  Partials      518      518           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[scottbarnes]

How this looks now, on the first page.

Desktop:

Tablet:

Mobile:

[mekarpeles]

1. Loan History breadcrumb/nav title

2. Accuracy of button availability statuses?

[mekarpeles]

Let's check against the add_availability function to see if this may diverge (and if there's a chance to DRY this code so we don't have to re-implement the add_availability logic)

[scottbarnes]

One shortcoming here is that if Internet Archive has scanned a single edition multiple times, it's possible both copies of the Internet Archive item/edition will point to a single Open Library edition, and that Open Library edition will only point to one of the Internet Archive items.

This creates a problem when a patron has borrowed the Internet Archive item that Open Library doesn't point to. See, e.g., climbersguidetoh0000rope and climbersguidetot00rope. They both point to OL5214872M, quite rightly. However, OL5214872M only points back to climbersguidetot00rope.

As I checked out the former, but Open Library points to the latter, it causes the loan history to show Not in Library, which is a bit odd given this is a list of books I've borrowed:

However, when I follow the link to the Open Library item, I see the book is borrowable (as climbersguidetot00rope, rather than the one I borrowed.:

Not sure there is a good solution to this, as currently Open Library can only be associated with one ocaid.

Thoughts?

@mekarpeles

[mekarpeles]

We could possibly modify get_availability_of_ocaids to pass in the patron's loans (or have a boolean flag to use the currently logged in patron's s3 keys if we want to know what a book is actively being borrowed by this patron.

[mekarpeles]

This was presumably causing a problem because in LoanStatus, we check a book method called ed.loan to fetch any loan that may be attached to the book and what we're doing here (which is attaching a loan history record to the book) was technically conflating two different loan types.

https://github.com/internetarchive/openlibrary/blob/master/openlibrary/macros/LoanStatus.html#L37-L46C38

The possible solutions are either to:

1. not attach the loan to the edition (if it's not being used)
2. attach the loan to the edition under a different name like loan_history
3. if the loan_history record has enough info associated to determine if it's active (e.g. an expiration date) and also assuming the ed.loan_history is in the same format as ed.loan else where on openlibrary.org. then consider modifying LoanStatus.html's check ~L37 to consider the expiry when determining if a book should show up as read.

[mekarpeles]

@anujamerwade + @scottbarnes congratulations on getting this feature over the finish line