Merge ~maxiberta/canonical-identity-provider:skip-username-validation-if-unchanged into canonical-identity-provider:master

Proposed by Maximiliano Bertacchini
Status: Merged
Approved by: Maximiliano Bertacchini
Approved revision: 6e67aed5a2cec86ad92628a9c74e97d4abddf214
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~maxiberta/canonical-identity-provider:skip-username-validation-if-unchanged
Merge into: canonical-identity-provider:master
Diff against target: 61 lines (+29/-4)
2 files modified
src/identityprovider/forms.py (+14/-4)
src/identityprovider/tests/test_forms.py (+15/-0)
Reviewer Review Type Date Requested Status
Daniel Manrique Approve
Review via email: mp+397386@code.launchpad.net

Commit message

Skip username validation in EditAccountForm if unchanged

Description of the change

The introduction of detailed username validation errors added form field validators on the username field. But, these validators run *before* the form's custom clean_username(), which contains the logic for skipping if unchanged.

https://docs.djangoproject.com/en/1.11/ref/forms/validation/#form-and-field-validation

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

+1, I think this was my fault right? :(

review: Approve
6e67aed... by Maximiliano Bertacchini on 2021-02-03

Fix new username validation test

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/identityprovider/forms.py b/src/identityprovider/forms.py
2index a23f2ba..61ab346 100644
3--- a/src/identityprovider/forms.py
4+++ b/src/identityprovider/forms.py
5@@ -374,8 +374,7 @@ class EditAccountForm(ResetPasswordFormMixin, forms.ModelForm):
6 placeholder = _('Verify your email address first')
7 self.fields['username'] = UsernameField(
8 required=False, placeholder=placeholder,
9- disabled=disabled, extra_class="p-form-validation__input",
10- validators=get_username_validators())
11+ disabled=disabled, extra_class="p-form-validation__input")
12 if self.instance.person is not None or require_username:
13 self.fields['username'].required = True
14
15@@ -426,8 +425,19 @@ class EditAccountForm(ResetPasswordFormMixin, forms.ModelForm):
16 # are now considered "distasteful".
17 if username == self.instance.person_name:
18 return username
19- if not valid_username(username):
20- raise forms.ValidationError(INVALID_USERNAME_MSG % username)
21+
22+ errors = []
23+ for validator in USERNAME_VALIDATORS:
24+ try:
25+ validator.__call__(username)
26+ except forms.ValidationError as ex:
27+ errors.append(ex)
28+ if errors:
29+ if gargoyle.is_active('USERNAME_DETAILED_ERRORS'):
30+ raise forms.ValidationError(errors)
31+ else:
32+ raise forms.ValidationError(INVALID_USERNAME_MSG % username)
33+
34 if username and username != self.instance.person_name:
35 # The username is going to change, so ask LP to do a dry
36 # run to see if it will fail.
37diff --git a/src/identityprovider/tests/test_forms.py b/src/identityprovider/tests/test_forms.py
38index 9ffa2e3..6b2c4a6 100644
39--- a/src/identityprovider/tests/test_forms.py
40+++ b/src/identityprovider/tests/test_forms.py
41@@ -740,6 +740,21 @@ class EditAccountFormTestCase(FormBaseTestCase):
42 ['Capital letters are not allowed.',
43 'Must contain only lowercase letters, hyphens and numbers.']})
44
45+ @switches(USERNAME_DETAILED_ERRORS=True)
46+ def test_username_validation_skipped_if_unchanged(self):
47+ self.factory.make_person(name='x', account=self.account)
48+ data = {
49+ 'username': 'x',
50+ 'oldpassword': DEFAULT_USER_PASSWORD,
51+ 'password': 'testingpass',
52+ 'passwordconfirm': 'testingpass',
53+ 'displayname': self.account.displayname,
54+ 'preferred_email': self.account.preferredemail.id,
55+ }
56+ form = EditAccountForm(
57+ data=data, instance=self.account, enable_username=True)
58+ self.assertTrue(form.is_valid())
59+
60
61 class EditAccountFormDevicePrefsEnabledTestCase(EditAccountFormTestCase):
62

Subscribers

People subscribed via source and target branches