-
Notifications
You must be signed in to change notification settings - Fork 938
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
fix: Add Trim leading and trailing whitespace from username in forms #15455
Conversation
@Mr-Sunglasses You'll want to update the translations, see https://warehouse.pypa.io/development/submitting-patches.html#translations |
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.
Nice work so far @Mr-Sunglasses !
Since we try to add tests for functionality and guarantee that the behavior is tested when someone else changes the code, please look at the existing tests for the Login form. Here's one:
warehouse/tests/unit/accounts/test_forms.py
Lines 81 to 92 in 26a3446
def test_validate_username_with_user(self): | |
request = pretend.stub() | |
user_service = pretend.stub(find_userid=pretend.call_recorder(lambda userid: 1)) | |
breach_service = pretend.stub() | |
form = forms.LoginForm( | |
request=request, user_service=user_service, breach_service=breach_service | |
) | |
field = pretend.stub(data="my_username") | |
form.validate_username(field) | |
assert user_service.find_userid.calls == [pretend.call("my_username")] |
You could either:
- add a new test like
test_validate_username_strips_whitespace()
and have the input have some whitespace and confirm that the submitted form passes validation and calls the user_service with the trimmed username, or - use
@pytest.mark.parametrize()
(documentation here) and update the existingtest_validate_username_with_user()
test function to accept the existingmy_username
and expect that in the service call, and add another entry with whitespace and expect the resulting call to not have the whitespace.
The second option is more advanced, so either test solution would be acceptable.
Done, Added tests Thanks a lot @miketheman 🙏🏻 |
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.
Nice work. Glad to see that the tests surfaced an even simpler solution.
Thanks for working through this issue!
Thanks a lot @miketheman for guiding looking forward to work on more issues 😃 |
fix: #15356
Screen.Recording.2024-02-23.at.4.27.35.AM.mov