Canonical SSO provider

Merge lp:~deadlight/canonical-identity-provider/form-error-states into lp:canonical-identity-provider/release

  1. form-error-states
  2. Merge into trunk
Proposed by Karl Williams
Status: Merged
Approved by: Maximiliano Bertacchini
Approved revision: no longer in the source branch.
Merged at revision: 1717
Proposed branch: lp:~deadlight/canonical-identity-provider/form-error-states
Merge into: lp:canonical-identity-provider/release
Diff against target: 207 lines (+37/-27)
9 files modified
src/identityprovider/fields.py (+6/-1)
src/identityprovider/forms.py (+6/-2)
src/identityprovider/tests/sso_server/test_home_page.py (+3/-1)
src/identityprovider/tests/sso_server/test_standalone_login.py (+2/-1)
src/webui/templates/account/edit.html (+1/-1)
src/webui/templates/registration/_create_account_form.html (+1/-1)
src/webui/templates/registration/_title.html (+2/-2)
src/webui/templates/vanilla/base.html (+9/-11)
src/webui/templates/vanilla/widgets/passwords.html (+7/-7)
To merge this branch: bzr merge lp:~deadlight/canonical-identity-provider/form-error-states
Reviewer Review Type Date Requested Status
Anthony Dillon (community) Approve
Maximiliano Bertacchini Approve
Review via email: mp+378530@code.launchpad.net

Commit message

Fixed the rendering of error states for the password fields (included modifying the password field generation code to take an optional HTML class parameter)

Changed the base template to move global error messages into the main strip, reducing vertical spacing.

Description of the change

Fixed the rendering of error states for the password fields (included modifying the password field generation code to take an optional HTML class parameter)

Changed the base template to move global error messages into the main strip, reducing vertical spacing.

QA:
Try and create an account but input non-matching passwords then also try a password which is too short

To post a comment you must log in.
Revision history for this message
Maximiliano Bertacchini (maxiberta) wrote :

Nice! Looks good to me. But there are 3 related failing tests, which fortunately look trivial to fix :)

Also, it'd be great if passwords field errors in "My account" had the same rendering style. Have you considered that?

Thanks!

review: Needs Fixing
Revision history for this message
Daniel Manrique (roadmr) wrote :

======================================================================
FAIL: identityprovider.tests.sso_server.test_standalone_login.StandaloneLoginTestCase.test_login
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/src/canonical-identity-provider/form-error-states/src/identityprovider/tests/sso_server/test_standalone_login.py", line 22, in test_login
    '<span class="trusted-rp-name">Ubuntu One</span> '
  File "/src/canonical-identity-provider/form-error-states/env/local/lib/python2.7/site-packages/django/test/testcases.py", line 393, in assertContains
    self.assertTrue(real_count != 0, msg_prefix + "Couldn't find %s in response" % text_repr)
  File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
    raise self.failureException(msg)
AssertionError: Couldn't find '<span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">log in</span>' in response
======================================================================
FAIL: identityprovider.tests.sso_server.test_home_page.HomePageTestCase.test
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/src/canonical-identity-provider/form-error-states/src/identityprovider/tests/sso_server/test_home_page.py", line 22, in test
    '<span class="trusted-rp-name">Ubuntu One</span> '
  File "/src/canonical-identity-provider/form-error-states/env/local/lib/python2.7/site-packages/django/test/testcases.py", line 393, in assertContains
    self.assertTrue(real_count != 0, msg_prefix + "Couldn't find %s in response" % text_repr)
  File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
    raise self.failureException(msg)
AssertionError: Couldn't find '<span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">log in</span>' in response
======================================================================
FAIL: identityprovider.tests.test_fields.PasswordFieldTestCase.test_widget_attrs
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/src/canonical-identity-provider/form-error-states/src/identityprovider/tests/test_fields.py", line 121, in test_widget_attrs
    self.assertEqual(self.field.widget.attrs, self.expected_attrs)
  File "/usr/lib/python2.7/unittest/case.py", line 513, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python2.7/unittest/case.py", line 835, in assertDictEqual
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python2.7/unittest/case.py", line 410, in fail
    raise self.failureException(msg)
AssertionError: {'autocomplete': 'off', 'placeholder': u'Password', 'class': 'disableAutoComplet [truncated]... != {'autocomplete': 'off', 'placeholder': u'Password', 'class': 'disableAutoComplet [truncated]...
  {'autocomplete': 'off',
- 'class': 'disableAutoComplete textType ',
? -

+ 'class': 'disableAutoComplete textType',
   'placeholder': u'Password'}

Ran 3272 tests in 80.238s
FAILED (failures=3)

Revision history for this message
Daniel Manrique (roadmr) wrote :

This diff fixes the test failures.

https://pastebin.canonical.com/p/HfSGQ2z954/

It does not make the password field errors in My Account have consistent errors as suggested by Maxi.

Revision history for this message
Maximiliano Bertacchini (maxiberta) wrote :

Password fields with errors now look awesome :) Thanks!

Note there's still one test failure: identityprovider.tests.sso_server.test_home_page.HomePageTestCase.test.

+1 provided that tests pass (and an optional nitpick).

review: Approve
Revision history for this message
Karl Williams (deadlight) wrote :

Oh yes, my mistake. I was just running the webUI suite of tests.

Revision history for this message
Otto Co-Pilot (otto-copilot) wrote :

A commit message must be set
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/279/

Revision history for this message
Otto Co-Pilot (otto-copilot) wrote :

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/280/

Revision history for this message
Daniel Manrique (roadmr) wrote :

oh haha :)

src/identityprovider/forms.py:149:80: E501 line too long (94 > 79 characters)

Revision history for this message
Anthony Dillon (ya-bo-ng) wrote :

LGTM sorry I was late +1

review: Approve
Revision history for this message
Karl Williams (deadlight) wrote :

> oh haha :)
>
> src/identityprovider/forms.py:149:80: E501 line too long (94 > 79 characters)

... arrgh! Fixed.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/identityprovider/fields.py'
--- src/identityprovider/fields.py 2018-02-09 20:56:16 +0000
+++ src/identityprovider/fields.py 2020-02-07 08:47:17 +0000
@@ -90,8 +90,13 @@
9090
91 def __init__(self, *args, **kwargs):91 def __init__(self, *args, **kwargs):
92 placeholder = kwargs.pop('placeholder', default_password_placeholder)92 placeholder = kwargs.pop('placeholder', default_password_placeholder)
93 html_class = "disableAutoComplete textType"
94 extra_class = kwargs.pop('extra_class', '')
95 if extra_class:
96 html_class += ' ' + extra_class
97
93 attrs = {98 attrs = {
94 'class': 'disableAutoComplete textType',99 'class': html_class,
95 'autocomplete': 'off',100 'autocomplete': 'off',
96 'placeholder': placeholder,101 'placeholder': placeholder,
97 }102 }
98103
=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py 2019-10-22 14:18:28 +0000
+++ src/identityprovider/forms.py 2020-02-07 08:47:17 +0000
@@ -146,14 +146,18 @@
146146
147 validate_policy = get_password_validator(self.account)147 validate_policy = get_password_validator(self.account)
148 if self.require_confirmation:148 if self.require_confirmation:
149 self.fields['oldpassword'] = PasswordField()149 self.fields['oldpassword'] = PasswordField(
150 extra_class="p-form-validation__input")
150 self.fields['password'] = PasswordField(151 self.fields['password'] = PasswordField(
151 placeholder=validate_policy.PASSWORD_POLICY_HELP_TEXT,152 placeholder=validate_policy.PASSWORD_POLICY_HELP_TEXT,
152 help_text=validate_policy.PASSWORD_POLICY_HELP_TEXT,153 help_text=validate_policy.PASSWORD_POLICY_HELP_TEXT,
153 validators=[validate_policy],154 validators=[validate_policy],
155 extra_class="p-form-validation__input"
154 )156 )
155 self.fields['passwordconfirm'] = PasswordField(157 self.fields['passwordconfirm'] = PasswordField(
156 placeholder=_('Retype password'))158 placeholder=_('Retype password'),
159 extra_class="p-form-validation__input",
160 )
157161
158 def clean_oldpassword(self):162 def clean_oldpassword(self):
159 old_password = self.cleaned_data.get('oldpassword')163 old_password = self.cleaned_data.get('oldpassword')
160164
=== modified file 'src/identityprovider/tests/sso_server/test_home_page.py'
--- src/identityprovider/tests/sso_server/test_home_page.py 2018-12-12 16:18:18 +0000
+++ src/identityprovider/tests/sso_server/test_home_page.py 2020-02-07 08:47:17 +0000
@@ -1,3 +1,5 @@
1# encoding: utf-8
2
1from identityprovider.fields import FIELD_REQUIRED3from identityprovider.fields import FIELD_REQUIRED
2from identityprovider.models.openidmodels import OpenIDRPSummary4from identityprovider.models.openidmodels import OpenIDRPSummary
3from identityprovider.tests.helpers import FunctionalTestCase5from identityprovider.tests.helpers import FunctionalTestCase
@@ -19,7 +21,7 @@
19 response = self.client.get(self.base_url)21 response = self.client.get(self.base_url)
20 self.assertContains(22 self.assertContains(
21 response,23 response,
22 '<span class="trusted-rp-name">Ubuntu One</span> '24 '<span class="trusted-rp-name">Ubuntu One</span> › '
23 '<span class="action-title">log in</span>')25 '<span class="action-title">log in</span>')
2426
25 # However, if the user is logged in, they are presented with some27 # However, if the user is logged in, they are presented with some
2628
=== modified file 'src/identityprovider/tests/sso_server/test_standalone_login.py'
--- src/identityprovider/tests/sso_server/test_standalone_login.py 2019-12-19 20:48:07 +0000
+++ src/identityprovider/tests/sso_server/test_standalone_login.py 2020-02-07 08:47:17 +0000
@@ -1,3 +1,4 @@
1# encoding: utf-8
1from django.urls import reverse2from django.urls import reverse
23
3from identityprovider.models.account import Account4from identityprovider.models.account import Account
@@ -19,7 +20,7 @@
19 response = self.client.get(self.base_url)20 response = self.client.get(self.base_url)
20 self.assertContains(21 self.assertContains(
21 response,22 response,
22 '<span class="trusted-rp-name">Ubuntu One</span> '23 '<span class="trusted-rp-name">Ubuntu One</span> › '
23 '<span class="action-title">log in</span>')24 '<span class="action-title">log in</span>')
24 self.assertIn("user_intention_create", response.content)25 self.assertIn("user_intention_create", response.content)
2526
2627
=== modified file 'src/webui/templates/account/edit.html'
--- src/webui/templates/account/edit.html 2020-01-22 12:52:26 +0000
+++ src/webui/templates/account/edit.html 2020-02-07 08:47:17 +0000
@@ -98,7 +98,7 @@
9898
99 <div class="row">99 <div class="row">
100 <div class="col-8">100 <div class="col-8">
101 {% include "widgets/passwords.html" with edit_account_labels=1 fields=form %}101 {% include "vanilla/widgets/passwords.html" with edit_account_labels=1 fields=form %}
102 </div>102 </div>
103 </div>103 </div>
104 {% endif %}104 {% endif %}
105105
=== modified file 'src/webui/templates/registration/_create_account_form.html'
--- src/webui/templates/registration/_create_account_form.html 2019-12-03 11:05:18 +0000
+++ src/webui/templates/registration/_create_account_form.html 2020-02-07 08:47:17 +0000
@@ -41,7 +41,7 @@
41 {{ create_form.username }}41 {{ create_form.username }}
42 </div>42 </div>
43 {% endif %}43 {% endif %}
44 {% include "widgets/passwords.html" with fields=create_form %}44 {% include "vanilla/widgets/passwords.html" with fields=create_form %}
4545
46 <div class="p-form-validation{% if create_form.accept_tos.errors %} is-error{% endif %} accept-tos-input">46 <div class="p-form-validation{% if create_form.accept_tos.errors %} is-error{% endif %} accept-tos-input">
47 {% if create_form.accept_tos.errors %}47 {% if create_form.accept_tos.errors %}
4848
=== modified file 'src/webui/templates/registration/_title.html'
--- src/webui/templates/registration/_title.html 2019-12-03 11:05:18 +0000
+++ src/webui/templates/registration/_title.html 2020-02-07 08:47:17 +0000
@@ -8,7 +8,7 @@
8 {% endif %}8 {% endif %}
9 <span class="action-title">{% blocktrans %}log in with Ubuntu One{% endblocktrans %}9 <span class="action-title">{% blocktrans %}log in with Ubuntu One{% endblocktrans %}
10 {% else %}10 {% else %}
11 <span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">{% trans "log in" %}</span>11 <span class="trusted-rp-name">Ubuntu One</span> › <span class="action-title">{% trans "log in" %}</span>
12 {% endif %}12 {% endif %}
13</h2>13</h2>
14<h2 class="p-heading--four title create-title u-no-margin--top">14<h2 class="p-heading--four title create-title u-no-margin--top">
@@ -20,7 +20,7 @@
20 <span class="action-title">{% blocktrans %}create Ubuntu One account{% endblocktrans %}</span>20 <span class="action-title">{% blocktrans %}create Ubuntu One account{% endblocktrans %}</span>
21 {% endif %}21 {% endif %}
22 {% else %}22 {% else %}
23 <span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">{% trans "create account" %}</span>23 <span class="trusted-rp-name">Ubuntu One</span> › <span class="action-title">{% trans "create account" %}</span>
24 {% endif %}24 {% endif %}
25</h2>25</h2>
26<hr>26<hr>
2727
=== modified file 'src/webui/templates/vanilla/base.html'
--- src/webui/templates/vanilla/base.html 2020-01-22 12:52:26 +0000
+++ src/webui/templates/vanilla/base.html 2020-02-07 08:47:17 +0000
@@ -71,19 +71,17 @@
71 {% endblock %}71 {% endblock %}
72 {% block messages %}72 {% block messages %}
73 {% if messages %}73 {% if messages %}
74 <section class="p-strip is-shallow">74 {% for message in messages %}
75 {% for message in messages %}75 <div class="row">
76 <div class="row">76 <div class="col-12">
77 <div class="col-12">77 <div class="p-notification">
78 <div class="p-notification">78 <p class="p-notification__response">
79 <p class="p-notification__response">79 {{ message }}
80 {{ message }}80 </p>
81 </p>
82 </div>
83 </div>81 </div>
84 </div>82 </div>
85 {% endfor %}83 </div>
86 </section>84 {% endfor %}
87 {% endif %}85 {% endif %}
88 {% endblock %}86 {% endblock %}
89 {% block content %}{% endblock %}87 {% block content %}{% endblock %}
9088
=== modified file 'src/webui/templates/vanilla/widgets/passwords.html'
--- src/webui/templates/vanilla/widgets/passwords.html 2019-11-08 14:47:08 +0000
+++ src/webui/templates/vanilla/widgets/passwords.html 2020-02-07 08:47:17 +0000
@@ -3,7 +3,7 @@
3<p>{% trans "To edit any details on this page, you must confirm your current password." %}</p>3<p>{% trans "To edit any details on this page, you must confirm your current password." %}</p>
44
5{% if edit_account_labels %}5{% if edit_account_labels %}
6<div class="p-form-validation{% if fields.oldpassword.errors or fields.non_field_errors %} is-error{% endif %}">6<div class="p-form-validation{% if fields.oldpassword.errors %} is-error{% endif %}">
7 <label for="id_oldpassword">{% trans "Current password" %}</label>7 <label for="id_oldpassword">{% trans "Current password" %}</label>
8 {{ fields.oldpassword }}8 {{ fields.oldpassword }}
9 {% if fields.oldpassword.errors %}9 {% if fields.oldpassword.errors %}
@@ -15,9 +15,10 @@
15{% endif %}15{% endif %}
16{% endif %}16{% endif %}
1717
18{% if edit_account_labels %}
19<div class="p-form-validation{% if fields.password.errors or fields.non_field_errors %} is-error{% endif %}">18<div class="p-form-validation{% if fields.password.errors or fields.non_field_errors %} is-error{% endif %}">
19 {% if edit_account_labels %}
20 <label for="id_password">{% trans "Choose password" %}</label>20 <label for="id_password">{% trans "Choose password" %}</label>
21 {% endif %}
21 {{ fields.password }}22 {{ fields.password }}
22 {% if fields.password.errors %}23 {% if fields.password.errors %}
23 <p class="p-form-validation__message">24 <p class="p-form-validation__message">
@@ -25,16 +26,15 @@
25 </p>26 </p>
26 {% endif %}27 {% endif %}
27</div>28</div>
28{% endif %}
2929
30{% if edit_account_labels %}
31<div class="p-form-validation{% if fields.passwordconfirm.errors or fields.non_field_errors %} is-error{% endif %}">30<div class="p-form-validation{% if fields.passwordconfirm.errors or fields.non_field_errors %} is-error{% endif %}">
31 {% if edit_account_labels %}
32 <label for="id_passwordconfirm">{% trans "Re-type password" %}</label>32 <label for="id_passwordconfirm">{% trans "Re-type password" %}</label>
33 {% endif %}
33 {{ fields.passwordconfirm }}34 {{ fields.passwordconfirm }}
34 {% if fields.passwordconfirm.errors %}35 {% if fields.passwordconfirm.errors or fields.non_field_errors %}
35 <p class="p-form-validation__message">36 <p class="p-form-validation__message">
36 <strong>Error:</strong> {{ fields.passwordconfirm.errors|first }}37 <strong>Error:</strong> {{ fields.passwordconfirm.errors|first }} {{ fields.non_field_errors|first }}
37 </p>38 </p>
38 {% endif %}39 {% endif %}
39</div>40</div>
40{% endif %}