Improve Sirsi patron auth self-test failure feedback (PP-3899)#3155
Improve Sirsi patron auth self-test failure feedback (PP-3899)#3155jonathangreen merged 4 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3155 +/- ##
=======================================
Coverage 93.26% 93.26%
=======================================
Files 493 493
Lines 45599 45616 +17
Branches 6254 6254
=======================================
+ Hits 42530 42546 +16
- Misses 1982 1983 +1
Partials 1087 1087 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tdilauro
left a comment
tdilauro
left a comment
There was a problem hiding this comment.
Looks great! 🪵🚀
I couple small suggestions below, but no show stoppers.
| def __bool__(self) -> bool: | ||
| return False |
There was a problem hiding this comment.
Clever trick here, but it looks like you've updated all of the call sites to use isinstance. So, is this method still needed?
There was a problem hiding this comment.
No needed at all. Seemed like it could prevent a foot-gun in the future though, so I thought I'd leave it in.
There was a problem hiding this comment.
I updated the doc string to make the intent clear though, the old one made it sound like it was being actively used.
| debug_message=( | ||
| f"Made a {result.method} request to {result.url} " | ||
| f"and received HTTP {result.status_code}.\n\n" | ||
| f"Response body:\n{result.message}" | ||
| ), |
There was a problem hiding this comment.
Minor - The debug message in read_data below is almost identical. We could make this into a format string and use it in both places, if we want to ensure consistency. Or, better yet, we could add a method on SirsiError, since all the variables are coming from it anyway.
There was a problem hiding this comment.
I just started looking at #3158 and noticed that you're using it there, too, which makes a stronger argument for making this a method on SirsiError.
| """ | ||
|
|
||
| status_code: int | ||
| message: str |
There was a problem hiding this comment.
Minor - "body" or "response_body" instead of "message"?
Surface the HTTP status code and response body in self-test results via IntegrationException debug_message, so library admins can diagnose configuration issues from the admin UI without needing server logs.
Add method and url fields to SirsiError, populated via a new _make_error helper. Self-test debug messages now show the exact request that failed (e.g. "Made a POST request to ... and received HTTP 401") along with the response body.
- Rename `message` field to `response_body` for clarity. - Extract duplicated debug message formatting into a `debug_message` property on `SirsiError`.
jonathangreen
force-pushed
the
feature/sirsi-selftest-error-feedback
branch
from
7bc0032 to
4b44309
Compare
2 participants
Description
When SirsiDynix patron auth self-tests fail (login, read patron data, patron status info), the error messages were generic (e.g., "Could not authenticate test patron") with no indication of why the request failed. This PR surfaces the HTTP status code, request method, URL, and response body in the self-test
debug_message, which is displayed in the admin UI.Motivation and Context
Helps troubleshoot PP-3899. Library admins and the support/integrations team can now diagnose SirsiDynix configuration issues directly from the admin UI self-test results, without needing access to server logs.
How Has This Been Tested?
Checklist