-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix cover on author page #9131
base: master
Are you sure you want to change the base?
Fix cover on author page #9131
Conversation
for more information, see https://pre-commit.ci
@jjessieyang seems your code editor changed all the quotes in that file. Can you please change then back? It should be a config in your editor/formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this, and sorry for the delay in the review, @jjessieyang.
I can't seem to leave the comment the way I want, but what about something like this, starting on line 239:
diff --git a/openlibrary/book_providers.py b/openlibrary/book_providers.py
index 758e498a6..6227e2c5f 100644
--- a/openlibrary/book_providers.py
+++ b/openlibrary/book_providers.py
@@ -241,22 +241,21 @@ def get_cover_url(ed_or_solr: Edition | dict) -> str | None:
return cover.url(size) if cover else None
# Solr edition
- elif ed_or_solr['key'].startswith('/books/'):
- if ed_or_solr.get('cover_i'):
- return (
- get_coverstore_public_url()
- + f'/b/id/{ed_or_solr["cover_i"]}-{size}.jpg'
- )
- else:
- # Solr document augmented with availability
- availability = ed_or_solr.get('availability', {}) or {}
-
- if availability.get('openlibrary_edition'):
- olid = availability.get('openlibrary_edition')
- return f"{get_coverstore_public_url()}/b/olid/{olid}-{size}.jpg"
- if availability.get('identifier'):
- ocaid = ed_or_solr['availability']['identifier']
- return f"https://archive.org/download/{ocaid}/page/cover_w180_h360.jpg"
+ elif ed_or_solr['key'].startswith('/books/') and ed_or_solr.get('cover_i'):
+ return (
+ get_coverstore_public_url()
+ + f'/b/id/{ed_or_solr["cover_i"]}-{size}.jpg'
+ )
+
+ # Solr document augmented with availability
+ availability = ed_or_solr.get('availability', {}) or {}
+
+ if availability.get('openlibrary_edition'):
+ olid = availability.get('openlibrary_edition')
+ return f"{get_coverstore_public_url()}/b/olid/{olid}-{size}.jpg"
+ if availability.get('identifier'):
+ ocaid = ed_or_solr['availability']['identifier']
+ return f"https://archive.org/download/{ocaid}/page/cover_w180_h360.jpg"
# Plain solr - we don't know which edition is which here, so this is most
# preferable
Both seem to work equally well, but I'm a bit wary of changing the original logic flow more than necessary. Originally, the "Solr document augmented with availability" code was running on everything that fell through the "Editions" if/elif/else beginning on line 239, and I am hesitant to move it under the "Solr edition" conditional on line 243, unless there is a compelling reason to do so -- and there may be, as I haven't looked super closely at this yet. :)
Also, have you had a chance yet to look at the other half of the issue, relating to the merge tool, as shown in this image from the issue:
That will likely involve editing utils/SelectionManager/SelectionManager.js
.
@jjessieyang need any help with the feedback here? |
Closes #9064
Technical
I found covers that were not appearing on the author page from the local-production dataset and found that their covers were from archive.org rather than covers.openlibrary.org/something. It seems that before, if there was no cover in openlibrary, the function would simply go to
return None
inreturn_cover_url
inopenlibrary/book_providers.py
and ignore the rest of the logic.Testing
I found a few examples of this happening when you look up "web design" books in my local version.
Screenshot
after changes:
Stakeholders
@scottbarnes