Let search inside work if availability is down #11521
Conversation
cdrini
added
the
Patch Deployed
There was a problem hiding this comment.
Pull request overview
This pull request addresses issue #11519 by allowing the search inside functionality to continue working even when the availability service is down. Previously, when get_availability returned an error, the function would return empty search results. Now, it continues with an empty availability dictionary and fetches editions directly from the database.
Key changes:
- Modified error handling to set
availability = {}instead of returning empty hits when availability service fails - Changed edition lookup from using availability data to directly querying by ocaid
- Simplified edition processing to use
.get()for safe access to availability data
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| format_book_data(ed, fetch_availability=False) if js else ed | ||
| ) | ||
| ia_results['hits']['hits'][idx]['availability'] = availability[ed.ocaid] | ||
| idx = ocaids.index(ed.ocaid) |
There was a problem hiding this comment.
The call to ocaids.index(ed.ocaid) on line 70 will raise a ValueError if ed.ocaid is not found in the ocaids list. This can occur if:
- An edition's ocaid field has been updated/changed between when the search index was built and this query
- There are data inconsistencies between the search results and the database
Consider either:
- Adding a check:
if ed.ocaid in ocaids:before line 70 - Using a try-except block to catch and handle ValueError gracefully
- Creating a dict mapping ocaid to index upfront:
ocaid_to_idx = {ocaid: idx for idx, ocaid in enumerate(ocaids)}
| if 'error' in availability: | ||
| return {"hits": {"hits": []}} | ||
| editions = web.ctx.site.get_many( | ||
| [ | ||
| '/books/%s' % availability[ocaid].get('openlibrary_edition') | ||
| for ocaid in availability | ||
| if availability[ocaid].get('openlibrary_edition') | ||
| ] | ||
| availability = {} | ||
|
|
||
| edition_keys = list( | ||
| web.ctx.site.things({'type': '/type/edition', 'ocaid': ocaids}) | ||
| ) | ||
| editions = web.ctx.site.get_many(edition_keys) | ||
| for ed in editions: | ||
| if ed.ocaid in ocaids: | ||
| idx = ocaids.index(ed.ocaid) | ||
| ia_results['hits']['hits'][idx]['edition'] = ( | ||
| format_book_data(ed, fetch_availability=False) if js else ed | ||
| ) | ||
| ia_results['hits']['hits'][idx]['availability'] = availability[ed.ocaid] | ||
| idx = ocaids.index(ed.ocaid) | ||
| hit = ia_results['hits']['hits'][idx] | ||
| hit['edition'] = ( | ||
| format_book_data(ed, fetch_availability=False) if js else ed | ||
| ) | ||
| hit['availability'] = availability.get(ed.ocaid, {}) |
There was a problem hiding this comment.
The new error handling path (when get_availability returns an error) and the modified edition lookup logic lack test coverage. Consider adding tests to verify:
- The function still returns results when availability service fails
- Editions are correctly matched to hits when availability is unavailable
- The
availability.get(ed.ocaid, {})fallback works correctly
This is important since this is a critical bug fix that changes the behavior when external services fail.
2 participants
Closes #11519 . Note it fixes it so that the page isn't empty, but the borrow buttons again say "Locate" instead of the correct availability if the availability service is erroring.
Technical
Testing
Tested on testing and confirmed it still works, but testing isn't exhibiting the bug described in the issue. Going to patch deploy to see if it fixes it there.
Screenshot
Stakeholders