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

Alert patron on lending error #8501

Merged

Conversation

jimchamp
Copy link
Collaborator

@jimchamp jimchamp commented Nov 7, 2023

Closes #8372

Displays a toast message when clicking a "Borrow" or "Read" button results in a 404 or 409 from the server. Otherwise, redirects to the location returned by the server. A json handler for ocaid checkouts has been added in support of this feature.

Technical

The borrow logic was moved into its own method, which, instead of raising exceptions, instead returns a two-tuple containing the HTTP status code and optional redirect location. This method is called by both the original ocaid checkout handler, and the new json handler.

Testing

Screenshot

Stakeholders

@jimchamp jimchamp changed the title [WIP] Alert patron on lending error Alert patron on lending error Nov 7, 2023
Comment on lines 125 to 133
def prepare_json_response(self, status_code, data=None):
d = json.dumps(data or {})
return web.HTTPError(
status_code,
headers = {
"Content-Type": "application/json",
},
data=d
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An improved version of this function would be useful throughout the codebase.

@mekarpeles mekarpeles self-assigned this Nov 9, 2023
@mekarpeles
Copy link
Member

I think there's a lot here that's in the right direction, my main reservation is having the borrow process itself require js -- my preference would be that we attempt a borrow and if it fails, we return the patron to the book page with a GET parameter like error=limit which then displays a toast message on page load. I think this would be much simpler code-wise, less brittle in the event of js errors, and not require js

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 9, 2023
@jimchamp jimchamp force-pushed the 8372/feature/toast-on-loan-failure branch from 98e24a2 to 1c4fba7 Compare November 15, 2023 18:30
@@ -189,6 +190,12 @@ def POST(self, key):
except lending.PatronAccessException as e:
stats.increment('ol.loans.blocked')

add_flash_message(
'error',
_('Unable to complete borrow action. Please try again later.'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mekarpeles you'll probably want to update this copy. I think that you suggested something specific last week, but I was unable to find the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@bfalling your input required

@jimchamp jimchamp added Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] and removed Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] labels Nov 15, 2023
@jimchamp
Copy link
Collaborator Author

jimchamp commented Nov 15, 2023

There is no need for an error query parameter:

Screenshot from 2023-11-15 10-27-50

@mekarpeles
Copy link
Member

LGTM, simple!

@mekarpeles mekarpeles added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Nov 22, 2023
@mekarpeles mekarpeles merged commit 6b1fdeb into internetarchive:master Dec 4, 2023
3 checks passed
@jimchamp jimchamp deleted the 8372/feature/toast-on-loan-failure branch January 31, 2024 00:06
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Upstream Lending & API exceptions
2 participants