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

Fixing search to rm flickering, availability check moved to backend #995

Merged
merged 3 commits into from
Jun 26, 2018

Conversation

mekarpeles
Copy link
Member

@mekarpeles mekarpeles commented Jun 25, 2018

Closes #733

Description:

TL;DR: Fixing search to rm flickering, availability check moved to backend

When ebooks mode was selected, we used to rely on availability.js to filter out all books which didn't have ebooks. This is because our openlibrary solr things anything with an ocaid (i.e. archive.org ID) is an ebook, whereas many of these are printdisabled and can't be ready by the user. This availability check process was incorporated into the backend over our search results to prevent the interface from visually pruning records.

Also, fixes search results so they correctly show the user's waitlist + loan status on relevant titles.

$ waiting_loan = ctx.user and ctx.user.get_waiting_loan_for(page)
$ user_loan = None
$ my_turn_to_borrow = waiting_loan and waiting_loan['status'] == 'available' and waiting_loan['position'] == 1
$ wlsize = availability.get('num_waitlist', 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

QUESTION: Same question as in the other PR. Is it misleading to say a failed waitlist number get should be treated as an empty waitlist?

Copy link
Member Author

Choose a reason for hiding this comment

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

One problem is that it's inconsistent how its represented on IA's side. In most cases where there hasn't been a waitlist before, I believe the availability API reports None for the waitlist whereas if there was a waitlist and it has been decremented, the value shows as 0.

This being the case, I think the right answer is to keep the default as 0. This value is only used if there is a waitlist for the book. And if the waitlist value is somehow fundamentally wrong, that's a data problem.

data-userid="$(user_loan['userid'])" id="read_ebook"
class="borrow-btn borrow-link">Read eBook</a>

$ return_url = page.url.rsplit('/', 1)[0] + '/do_return/borrow'
Copy link
Collaborator

Choose a reason for hiding this comment

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

QUESTION: How fragile is this text parsing, i.e. when using different kinds of base URLS, e.g. with subdomains, localhost, with/without protocol, etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Investigating.

@mekarpeles mekarpeles changed the title WIP: Fixing search to rm flickering, availability check moved to backend Fixing search to rm flickering, availability check moved to backend Jun 26, 2018
@mekarpeles mekarpeles merged commit 0fd5964 into master Jun 26, 2018
@mekarpeles mekarpeles deleted the 733/hotfix/rm-fe-search-flicker branch June 26, 2018 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Open Library Search Experience for ebooks mode
2 participants