Fix bug generating OPDS2 ODL notification URL #2126
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2126 +/- ##
=======================================
Coverage 90.73% 90.73%
=======================================
Files 351 351
Lines 40904 40905 +1
Branches 8874 8875 +1
=======================================
+ Hits 37115 37117 +2
+ Misses 2481 2480 -1
Partials 1308 1308 ☔ View full report in Codecov by Sentry. |
| @@ -421,6 +421,25 @@ def test_checkin_open_access( | |||
| opds2_with_odl_api_fixture.patron, "pin", pool | |||
| ) | |||
|
|
|||
| def test__notification_url(self): | |||
| short_name = "short_name" | |||
There was a problem hiding this comment.
Since we go to the trouble of setting up the app context here, could we set our short_name to "cm", pull in the fixture, and use this same test to verify that our fixture's _notification_url stays in sync with the mainline version?
tests/mocks/odl.py
Outdated
| """Get the notification URL that should be passed in the ODL checkout link | ||
|
|
||
| This is broken out into a separate function to make it easier to override | ||
| in tests. | ||
| """ |
There was a problem hiding this comment.
Did you mean to keep this comment from the mainline code here verbatim?
src/palace/manager/api/odl/api.py
Outdated
| def _notification_url( | ||
| short_name: str | None, patron_id: str, license_id: str | ||
| ) -> str: | ||
| """Get the notification URL that should be passed in the ODL checkout link |
There was a problem hiding this comment.
Very minor: Missing period at the end of the first sentence of the docstring.
Description
Fix parameters passed to
url_forto generate the URL for the OPDS2 + ODL notification endpoint. Previously I was passingpatron_idandlicense_id, but the actual parameters werepatron_identifierandlicense_identifier.I did a little bit of light refactoring as well so I could add a better test, where we test the actual URL generation.
Motivation and Context
Testing the changes from #2117
How Has This Been Tested?
Checklist