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

Add initial support for displaying active BorrowDirect requests in requests tab #289

Merged
merged 2 commits into from
Jul 29, 2019

Conversation

jkeck
Copy link
Contributor

@jkeck jkeck commented Jul 26, 2019

Closes #212

Mobile

Screen Shot 2019-07-26 at 4 40 47 PM

Desktop

Screen Shot 2019-07-26 at 4 38 19 PM

@camillevilla camillevilla self-requested a review July 27, 2019 00:22
@jkeck
Copy link
Contributor Author

jkeck commented Jul 27, 2019

FWIW, I don't believe test users can have BorrowDirect requests (and they don't have a real test system that we can use), so we kind of have to use library IDs in order to test this. I would be happy to help anybody review w/ some library IDs (or @saseestone can too)


def requests
request_client.requests('open', true).map { |request| BorrowDirectRequests::Request.new(request) }.select(&:active?)
rescue BorrowDirect::Error
Copy link
Member

Choose a reason for hiding this comment

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

Should we report these errors to honeybadger, or are they just expected?

Copy link
Contributor Author

@jkeck jkeck Jul 27, 2019

Choose a reason for hiding this comment

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

Unfortunately expected. They return an error when the user has no requests (PUBQR004: No result for query) and always in any non-production mode since their default server always returns an error (PUBSC003: Invalid parameter - Invalid PartnershipId), which I presume is because they never added us to their test system.

The BorrowDirect gem doesn't make any distinction between these and returns the top level BorrowDirect::Error. The gem does have some facility for setting expected error codes (although does not include this case), but the RequestQuery class has some assumptions about what is returned from the API and doesn't handle this case well. I would be happy to submit a PR upstream to address that, which would allow us to report the real exceptions (if folks thought that it would be worth doing)

Copy link
Member

Choose a reason for hiding this comment

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

👍 ; I'm happy to punt for now.

@@ -151,6 +151,16 @@ def to_partial_path

private

def borrow_direct_requests
return [] if proxy_borrower? # Proxies can't submit borrow direct requests, so don't check.
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming that fee borrowers can do BD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would guess probably not. @saseestone, can you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Extracted to #326

@jkeck jkeck changed the title Add initial support for displaying active BorrodDirect requests in requests tab Add initial support for displaying active BorrowDirect requests in requests tab Jul 27, 2019
@cbeer cbeer merged commit d3e6fd6 into master Jul 29, 2019
@cbeer cbeer deleted the 212-bd-spike branch July 29, 2019 22:11
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.

Add support for displaying BorrowDirect requests
2 participants