-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixes CTA + lending v2 fields #6303
Conversation
data-iframe-src="https://archive.org/details/$ocaid?view=theater&wrapper=false" | ||
data-iframe-link="https://archive.org/details/$ocaid" | ||
data-ol-link-track="CTAClick|Preview" href="#bookPreview">$_('Preview')</a> | ||
<div class="cta-button-group"> |
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.
just adds outer div to fix the width of preview btn
openlibrary/macros/LoanStatus.html
Outdated
@@ -103,6 +115,8 @@ | |||
<a href="$work_key" class="cta-btn cta-btn--missing" title="$_('This book is currently checked out, please check back later.')" | |||
data-ol-link-track="CTAClick|CheckedOut">$_('Checked Out')</a> | |||
</div> | |||
$if not secondary_action: |
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.
this shows editions table preview button if checked out
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.
These 2 lines and the next 2 lines are now very confusing. It's meant to show one button by default. secondary_action
means show a second button with a less favourable option. This is like using it to do the exact opposite :P
v1_resp['identifier'] = ocaid | ||
v1_resp['is_restricted'] = v1_resp['status'] != 'open' | ||
v1_resp['is_printdisabled'] = v1_resp.get('is_printdisabled', False) | ||
v1_resp['is_lendable'] = v1_resp['status'] == 'borrow_available' |
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.
This line is flat out wrong -- borrow_unavailable
can also imply is_lendable
. And besides, the value is already coming from the Bulk Availability API now (w/ latest schema) so this adapter is no longer necessary. Same for most the removed fields.
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.
Tested around and things look good. I think we're showing "Borrow" in some places we should be showing "Special Access", but small regression. Blocker: we're now showing two buttons in carousels when things are checked out:
I think we want to roll back the commented bit and only show the "checked out" button.
openlibrary/macros/LoanStatus.html
Outdated
@@ -103,6 +115,8 @@ | |||
<a href="$work_key" class="cta-btn cta-btn--missing" title="$_('This book is currently checked out, please check back later.')" | |||
data-ol-link-track="CTAClick|CheckedOut">$_('Checked Out')</a> | |||
</div> | |||
$if not secondary_action: |
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.
These 2 lines and the next 2 lines are now very confusing. It's meant to show one button by default. secondary_action
means show a second button with a less favourable option. This is like using it to do the exact opposite :P
81c6dd4
to
89598cd
Compare
Closes #6302
update_availability_schema_to_v2
adapter had an incorrect/incomplete definition foris_lendable
which did not consider theborrow_unavailable
cases, which resulted in us not showing the Checked Outpi
'd and so some books were returningerror
because Bulk Availability didn't have access toprintdisabled
scope and thus returnederror
status, causing Ground Truth API to be called. On testing, we used the deploy app from archive.org to confirm that values are now correctTechnical
Testing
Screenshot
Stakeholders