Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show editions in reading log #8821

Conversation

benbdeitch
Copy link
Contributor

Closes #2733

This PR alters how the 'already-read' and 'currently-read' bookshelves retrieve and display items. Rather than simply fetching the work data, they now retrieve the edition-specific data that is already stored in their reading log.

Currently, the 'want-to-read' bookshelf is untouched, as users might want to know when any edition becomes available to borrow.

Technical

The get_users_logged_books function in openlibrary/core/Bookshelves was edited to provide this functionality. Based on the bookshelf_id received, it switches on editions variable, that is used to later define behavior.

Both the get_sorted_reading_log_books and get_filtered_reading_log_books were altered to switch between fetching work data, or fetching work and edition data, based on the 'editions' variable. As well, a new function, link_editions_to_works was created to process the retrieved results, and properly tie them together, so that they are correctly interpreted by the SearchResultsWork macro.

Testing

To verify, simply add a few books to each shelf of your reading log. Preferably, editions with titles that differ from the work's should be chosen, or a brief edit in macro/SearchResultsWork can be used to display the doc_type variable. On each shelf, one should both check the shelf as a whole (which uses the 'get_sorted_reading_log_books' function, and attempt to search for an individual work (using the get_filtered_reading_log_books function).

Screenshot

image image

(As noted, the 'solr_edition' line is not a part of the pull-request; merely an easy way to display the doc_type variable for testing.)

image image

Stakeholders

@cdrini

@cdrini cdrini self-assigned this Feb 16, 2024
@Watt3r
Copy link

Watt3r commented Feb 26, 2024

I bump into this problem often and this would totally fix it for me. Thanks for implementing this @benbdeitch!

@mekarpeles mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Feb 26, 2024
…s' value for get_sorted_reading_log_books was false. As well, updated the error message to reflect the change in a variable's name.
…ored in the list comprehension for searching Solr's index through get_sorted_reading_log_books.
… for redirects to properly function when linking retrieved methods to works.
… for add_storage_items_for_redirects to query Solr for a work containing the edition, if it has the edition data, but the work is a redirect.
@RayBB
Copy link
Collaborator

RayBB commented Mar 14, 2024

@benbdeitch how is it going with this? Do you need any help?

@cdrini cdrini changed the title 2733/feature/show editions in reading log Show editions in reading log Mar 20, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great! A few code cleanups, and one potential error! Will take a look at the last method afterwards.

Testing:

This page seems to error: https://testing.openlibrary.org/people/bigdbag/books/already-read?page=10&debug=true

✅ sort | edition_id=null | https://testing.openlibrary.org/people/pyat/books/already-read
✅ filter | edition_id=null | https://testing.openlibrary.org/people/pyat/books/already-read?q=playground
✅ sort | large reading log | https://testing.openlibrary.org/people/bigdbag/books/already-read
✅ filter | large reading log | https://testing.openlibrary.org/people/bigdbag/books/already-read?q=harry+potter

openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Show resolved Hide resolved
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the finished review!

openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/plugins/upstream/mybooks.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
openlibrary/core/bookshelves.py Outdated Show resolved Hide resolved
@cdrini cdrini added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Mar 28, 2024
@cdrini cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
@cdrini
Copy link
Collaborator

cdrini commented Apr 2, 2024

Performance-wise, so far this is looking good!

image

Will testing the filtered case once the bug is fixed causing it to return 0 results!

@cdrini
Copy link
Collaborator

cdrini commented Apr 5, 2024

Ok the bug was with ol-solr1 , so not related to this code! Performance is looking good as well:
image

I'll do a few more tests later today, and then I think this is good to merge!!

@cdrini cdrini removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Apr 8, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok lgtm! I tested redirects; there is one edge case when you add two works and then the two works are merged... but I think it's very edge case and I'm not too sure how we should handle it. It does work correctly when you add a work/edition and then that work gets merged into another. Also works if you add just a work, and it gets merged into another -- displays the empty tile it currently displays.

@cdrini cdrini merged commit 76facc7 into internetarchive:master Apr 11, 2024
3 checks passed
Achorn pushed a commit to Achorn/openlibrary that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the reading log to log editions instead of works
6 participants