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

[Karl Williams]

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.

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

[Maximiliano Bertacchini (maxiberta)]

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

[Daniel Manrique (roadmr)]

======================================================================
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)

[Daniel Manrique (roadmr)]

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.

[Maximiliano Bertacchini (maxiberta)]

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

[Karl Williams (deadlight)]

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

[Otto Co-Pilot (otto-copilot)]

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

[Otto Co-Pilot (otto-copilot)]

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

[Daniel Manrique (roadmr)]

oh haha :)

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

[Anthony Dillon (ya-bo-ng)]

LGTM sorry I was late +1

review: Approve

[Karl Williams (deadlight)]

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

... arrgh! Fixed.

=== 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 @@
 
     def __init__(self, *args, **kwargs):
         placeholder = kwargs.pop('placeholder', default_password_placeholder)
+        html_class = "disableAutoComplete textType"
+        extra_class = kwargs.pop('extra_class', '')
+        if extra_class:
+            html_class += ' ' + extra_class
+
         attrs = {
-            'class': 'disableAutoComplete textType',
+            'class': html_class,
             'autocomplete': 'off',
             'placeholder': placeholder,
         }

=== 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 @@
 
         validate_policy = get_password_validator(self.account)
         if self.require_confirmation:
-            self.fields['oldpassword'] = PasswordField()
+            self.fields['oldpassword'] = PasswordField(
+                extra_class="p-form-validation__input")
         self.fields['password'] = PasswordField(
             placeholder=validate_policy.PASSWORD_POLICY_HELP_TEXT,
             help_text=validate_policy.PASSWORD_POLICY_HELP_TEXT,
             validators=[validate_policy],
+            extra_class="p-form-validation__input"
         )
         self.fields['passwordconfirm'] = PasswordField(
-            placeholder=_('Retype password'))
+            placeholder=_('Retype password'),
+            extra_class="p-form-validation__input",
+        )
 
     def clean_oldpassword(self):
         old_password = self.cleaned_data.get('oldpassword')

=== 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 @@
+# encoding: utf-8
+
 from identityprovider.fields import FIELD_REQUIRED
 from identityprovider.models.openidmodels import OpenIDRPSummary
 from identityprovider.tests.helpers import FunctionalTestCase
@@ -19,7 +21,7 @@
         response = self.client.get(self.base_url)
         self.assertContains(
             response,
-            '<span class="trusted-rp-name">Ubuntu One</span> '
+            '<span class="trusted-rp-name">Ubuntu One</span> › '
             '<span class="action-title">log in</span>')
 
         # However, if the user is logged in, they are presented with some

=== 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 @@
+# encoding: utf-8
 from django.urls import reverse
 
 from identityprovider.models.account import Account
@@ -19,7 +20,7 @@
         response = self.client.get(self.base_url)
         self.assertContains(
             response,
-            '<span class="trusted-rp-name">Ubuntu One</span> '
+            '<span class="trusted-rp-name">Ubuntu One</span> › '
             '<span class="action-title">log in</span>')
         self.assertIn("user_intention_create", response.content)
 

=== 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 @@
 
     <div class="row">
       <div class="col-8">
-        {% include "widgets/passwords.html" with edit_account_labels=1 fields=form %}
+        {% include "vanilla/widgets/passwords.html" with edit_account_labels=1 fields=form %}
       </div>
     </div>
     {% endif %}

=== 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 @@
     {{ create_form.username }}
   </div>
   {% endif %}
-  {% include "widgets/passwords.html" with fields=create_form %}
+  {% include "vanilla/widgets/passwords.html" with fields=create_form %}
 
   <div class="p-form-validation{% if create_form.accept_tos.errors %} is-error{% endif %} accept-tos-input">
     {% if create_form.accept_tos.errors %}

=== 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 @@
     {% endif %}
     <span class="action-title">{% blocktrans %}log in with Ubuntu One{% endblocktrans %}
   {% else %}
-    <span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">{% trans "log in" %}</span>
+    <span class="trusted-rp-name">Ubuntu One</span> › <span class="action-title">{% trans "log in" %}</span>
   {% endif %}
 </h2>
 <h2 class="p-heading--four title create-title u-no-margin--top">
@@ -20,7 +20,7 @@
       <span class="action-title">{% blocktrans %}create Ubuntu One account{% endblocktrans %}</span>
     {% endif %}
   {% else %}
-    <span class="trusted-rp-name">Ubuntu One</span> <span class="action-title">{% trans "create account" %}</span>
+    <span class="trusted-rp-name">Ubuntu One</span> › <span class="action-title">{% trans "create account" %}</span>
   {% endif %}
 </h2>
 <hr>

=== 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 @@
             {% endblock %}
             {% block messages %}
             {% if messages %}
-            <section class="p-strip is-shallow">
-              {% for message in messages %}
-              <div class="row">
-                <div class="col-12">
-                  <div class="p-notification">
-                    <p class="p-notification__response">
-                      {{ message }}
-                    </p>
-                  </div>
+            {% for message in messages %}
+            <div class="row">
+              <div class="col-12">
+                <div class="p-notification">
+                  <p class="p-notification__response">
+                    {{ message }}
+                  </p>
                 </div>
               </div>
-              {% endfor %}
-            </section>
+            </div>
+            {% endfor %}
             {% endif %}
             {% endblock %}
             {% block content %}{% endblock %}

=== 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 @@
 <p>{% trans "To edit any details on this page, you must confirm your current password." %}</p>
 
 {% if edit_account_labels %}
-<div class="p-form-validation{% if fields.oldpassword.errors or fields.non_field_errors %} is-error{% endif %}">
+<div class="p-form-validation{% if fields.oldpassword.errors %} is-error{% endif %}">
   <label for="id_oldpassword">{% trans "Current password" %}</label>
   {{ fields.oldpassword }}
   {% if fields.oldpassword.errors %}
@@ -15,9 +15,10 @@
 {% endif %}
 {% endif %}
 
-{% if edit_account_labels %}
 <div class="p-form-validation{% if fields.password.errors or fields.non_field_errors %} is-error{% endif %}">
+  {% if edit_account_labels %}
   <label for="id_password">{% trans "Choose password" %}</label>
+  {% endif %}
   {{ fields.password }}
   {% if fields.password.errors %}
   <p class="p-form-validation__message">
@@ -25,16 +26,15 @@
   </p>
   {% endif %}
 </div>
-{% endif %}
 
-{% if edit_account_labels %}
 <div class="p-form-validation{% if fields.passwordconfirm.errors or fields.non_field_errors %} is-error{% endif %}">
+  {% if edit_account_labels %}
   <label for="id_passwordconfirm">{% trans "Re-type password" %}</label>
+  {% endif %}
   {{ fields.passwordconfirm }}
-  {% if fields.passwordconfirm.errors %}
+  {% if fields.passwordconfirm.errors or fields.non_field_errors %}
   <p class="p-form-validation__message">
-    <strong>Error:</strong> {{ fields.passwordconfirm.errors|first }}
+    <strong>Error:</strong> {{ fields.passwordconfirm.errors|first }} {{ fields.non_field_errors|first }}
   </p>
   {% endif %}
 </div>
-{% endif %}