-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow empty string for username and password in ODL settings. #2245
Conversation
715a94b
to
99fa507
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2245 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 363 363
Lines 41296 41296
Branches 8840 8840
=======================================
Hits 37628 37628
Misses 2406 2406
Partials 1262 1262 ☔ View full report in Codecov by Sentry. |
f83be0e
to
296df19
Compare
296df19
to
a70fc49
Compare
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.
Looks fine. I would have thought that changing the type of password to be str | None
would be a better solution for consistency since the base class for the OPDS settings defines them like that, since authentication is optional there:
circulation/src/palace/manager/core/opds_import.py
Lines 157 to 177 in b4d189b
username: str | None = FormField( | |
None, | |
form=ConfigurationFormItem( | |
label=_("Username"), | |
description=_( | |
"If HTTP Basic authentication is required to access the OPDS feed (it usually isn't), enter the username here." | |
), | |
weight=-1, | |
), | |
) | |
password: str | None = FormField( | |
None, | |
form=ConfigurationFormItem( | |
label=_("Password"), | |
description=_( | |
"If HTTP Basic authentication is required to access the OPDS feed (it usually isn't), enter the password here." | |
), | |
weight=-1, | |
), | |
) |
But I haven't tried making this type change, so maybe there are some consequences I'm not thinking about.
Description
A follow-on PR for https://ebce-lyrasis.atlassian.net/browse/PP-1834 to address validation error in ODL 2.0 collection config when no specifying username and password. Validation requires a string albeit empty for a string type control. Changing the return type had undesirable ripple effects. This changes the default value from None to an empty string which seemed to me the more straightforward solution.
Motivation and Context
How Has This Been Tested?
I tested it manually on my local instance.
Checklist