Merge lp:~ralsina/ubuntu-sso-client/error_message_key into lp:ubuntu-sso-client

[Roberto Alsina]

Added a function to convert errordicts to strings, used that function to make on_login_error work better.

Added a function to convert errordicts to strings:

* Based on the one in the gtk implementation
* Made more robust (we are getting errordicts with "error_message" and no "message" and no "__all__")
* Made it more readable (IMHO)
* Done with TDD ;-)

I suspect that this version is an improvement over the gtk one and both should be moved to a "neutral" place.

Used that function to make on_login_error work better.

[lp:~ralsina/ubuntu-sso-client/error_message_key]

762. By Roberto Alsina on 2011-08-18

style fixes

[Natalia Bidart (nataliabidart)]

The branch looks great!

Only a couple of tests are missing:
 - when both msg1 and msg2 are not None (for two cases: when msg2 is 'message' and when is 'error_message').
 - when msg1 is not None (ie __all__ is present)

Can you please add those test cases? Thanks!

review: Needs Fixing

[lp:~ralsina/ubuntu-sso-client/error_message_key]

763. By Roberto Alsina on 2011-08-18

added suggested tests

[Roberto Alsina (ralsina)]

> The branch looks great!
>
> Only a couple of tests are missing:
> - when both msg1 and msg2 are not None (for two cases: when msg2 is 'message'
> and when is 'error_message').
> - when msg1 is not None (ie __all__ is present)
>
> Can you please add those test cases? Thanks!

Added!

[lp:~ralsina/ubuntu-sso-client/error_message_key]

764. By Roberto Alsina on 2011-08-18

use the new function everywhere, test it everywhere

[Natalia Bidart (nataliabidart)]

Can you please fix these lint issues?

ubuntu_sso/qt/controllers.py:
    614: [W0311] Bad indentation. Found 16 spaces, expected 12

ubuntu_sso/qt/tests/test_windows.py:
    219: [W0231, FakeCurrentUserPage.__init__] __init__ method from base class 'FakePage' is not called

review: Needs Fixing

[lp:~ralsina/ubuntu-sso-client/error_message_key]

765. By Roberto Alsina on 2011-08-18

lint

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'ubuntu_sso/qt/controllers.py'
--- ubuntu_sso/qt/controllers.py	2011-08-16 20:34:28 +0000
+++ ubuntu_sso/qt/controllers.py	2011-08-18 15:41:42 +0000
@@ -79,6 +79,28 @@
 # disabled warnings about TODO comments
 
 
+# Based on the gtk implementation, but added error_message
+# support, and rewritten.
+def _build_general_error_message(errordict):
+    """Concatenate __all__ and message from the errordict."""
+    result = None
+    msg1 = errordict.get('__all__')
+    msg2 = errordict.get('message')
+    if msg2 is None:
+        # See the errordict in LP: 828417
+        msg2 = errordict.get('error_message')
+    if msg1 is not None and msg2 is not None:
+        result = '\n'.join((msg1, msg2))
+    elif msg1 is not None:
+        result = msg1
+    elif msg2 is not None:
+        result = msg2
+    else:
+        # I give up.
+        result = "Error: %r" % errordict
+    return result
+
+
 class BackendController(object):
     """Represent a controller that talks with the sso self.backend."""
 
@@ -222,7 +244,7 @@
         # let the user know
         logger.error('Got error when login %s, error: %s',
                      self.view.wizard().app_name, error)
-        self.message_box.critical(error['message'])
+        self.message_box.critical(_build_general_error_message(error))
 
     def on_logged_in(self, app_name, result):
         """We managed to log in."""
@@ -405,8 +427,7 @@
         logger.debug('SetUpAccountController.on_user_registration_error')
         # errors are returned as a dict with the data we want to show.
         self._refresh_captcha()
-        errors = [v for _, v in sorted(error.iteritems())]
-        self.message_box.critical('\n'.join(errors))
+        self.message_box.critical(_build_general_error_message(error))
 
     def on_user_registered(self, app_name, result):
         """Execute when the user did register."""
@@ -590,7 +611,7 @@
         """Signal thrown when there's a problem validating the email."""
         msg = error.get('email_token')
         if msg is None:
-            msg = '\n'.join([v for _, v in sorted(error.iteritems())])
+            msg = _build_general_error_message(error)
         self.message_box.critical(msg)
 
     #pylint: disable=C0103
@@ -720,8 +741,11 @@
 class ResetPasswordController(BackendController):
     """Controller used to deal with reseintg the password."""
 
-    def __init__(self, title='', subtitle=''):
+    def __init__(self, title='', subtitle='', message_box=None):
         """Create a new instance."""
+        if message_box is None:
+            message_box = QMessageBox
+        self.message_box = message_box
         super(ResetPasswordController, self).__init__(title, subtitle)
 
     def _set_translated_strings(self):
@@ -777,6 +801,9 @@
 
     def on_password_change_error(self, app_name, error):
         """Let the user know that there was an error."""
+        logger.error('Got error changing password for %s, error: %s',
+                     self.view.wizard().app_name, error)
+        self.message_box.critical(_build_general_error_message(error))
 
     def set_new_password(self):
         """Request a new password to be set."""

=== modified file 'ubuntu_sso/qt/tests/test_windows.py'
--- ubuntu_sso/qt/tests/test_windows.py	2011-08-16 20:34:28 +0000
+++ ubuntu_sso/qt/tests/test_windows.py	2011-08-18 15:41:42 +0000
@@ -201,12 +201,24 @@
         self._text = text
 
 
-class FakeCurrentUserPage(object):
+class FakePage(object):
+
+    """A fake wizard page."""
+
+    app_name = "APP"
+
+    def wizard(self):
+        """Fake wizard."""
+        return self
+
+
+class FakeCurrentUserPage(FakePage):
 
     """Fake CurrentuserPage."""
 
     def __init__(self, *args):
         """Initialize."""
+        super(FakeCurrentUserPage, self).__init__(*args)
         self.ui = self
         self.ui.email_edit = FakeLineEdit()
         self.ui.password_edit = FakeLineEdit()
@@ -250,6 +262,101 @@
         self.controller.setupUi(self.view)
 
 
+class CurrentUserControllerErrorTestCase(TestCase):
+
+    """Tests for CurrentUserController's error handler."""
+
+    on_error_method_name = "on_login_error"
+    controller_class = CurrentUserController
+
+    def setUp(self):
+        """Setup test."""
+        super(CurrentUserControllerErrorTestCase, self).setUp()
+        self.message_box = FakeMessageBox()
+        self.controller = self.controller_class(
+            message_box=self.message_box)
+        self.controller.view = FakePage()
+        self._called = False
+        self.on_error_method = getattr(
+            self.controller, self.on_error_method_name)
+
+    def test_error_message_key(self):
+        """Test that on_login_error reacts to errors with "error_message"."""
+        self.on_error_method({"error_message": "WORRY!"})
+        self.assertEqual(self.message_box.critical_args, (('WORRY!',), {}))
+
+    def test_message_key(self):
+        """Test that on_login_error reacts to errors with "message"."""
+        self.on_error_method({"message": "WORRY!"})
+        self.assertEqual(self.message_box.critical_args, (('WORRY!',), {}))
+
+    def test_broken_error(self):
+        """Test that on_login_error reacts to broken errors."""
+        self.on_error_method({"boo!": "WORRY!"})
+        self.assertEqual(self.message_box.critical_args,
+            (("Error: {'boo!': 'WORRY!'}",), {}))
+
+    def test_all_and_message(self):
+        """Test that on_login_error reacts to broken errors."""
+        self.on_error_method(
+            {"message": "WORRY!", "__all__": "MORE!"})
+        self.assertEqual(self.message_box.critical_args,
+            (('MORE!\nWORRY!',), {}))
+
+    def test_all_and_error_message(self):
+        """Test that on_login_error reacts to broken errors."""
+        self.on_error_method(
+            {"error_message": "WORRY!", "__all__": "MORE!"})
+        self.assertEqual(self.message_box.critical_args,
+            (('MORE!\nWORRY!',), {}))
+
+    def test_only_all(self):
+        """Test that on_login_error reacts to broken errors."""
+        self.on_error_method(
+            {"__all__": "MORE!"})
+        self.assertEqual(self.message_box.critical_args,
+            (('MORE!',), {}))
+
+
+class EmailVerificationControllerErrorTestCase(
+    CurrentUserControllerErrorTestCase):
+
+    """Tests for EmailVerificationController's error handler."""
+
+    on_error_method_name = "on_email_validation_error"
+    controller_class = EmailVerificationController
+
+    def setUp(self):
+        """Setup test."""
+        super(EmailVerificationControllerErrorTestCase, self).setUp()
+        # This error handler takes one extra argument.
+        self.on_error_method = lambda error: getattr(
+            self.controller, self.on_error_method_name)('APP', error)
+
+
+class SetUpAccountControllerErrorTestCase(
+    EmailVerificationControllerErrorTestCase):
+
+    """Tests for SetUpAccountController's error handler."""
+
+    on_error_method_name = "on_user_registration_error"
+    controller_class = SetUpAccountController
+
+    def setUp(self):
+        """Setup test."""
+        super(SetUpAccountControllerErrorTestCase, self).setUp()
+        self.patch(self.controller, "_refresh_captcha", lambda *args: None)
+
+
+class ResetPasswordControllerErrorTestCase(
+    EmailVerificationControllerErrorTestCase):
+
+    """Tests for ResetPasswordController's error handler."""
+
+    on_error_method_name = "on_password_change_error"
+    controller_class = ResetPasswordController
+
+
 class CurrentUserControllerValidationTest(TestCase):
 
     """Tests for CurrentUserController, but without Mocker."""
@@ -613,9 +720,7 @@
              'unknownfield': "Error in unknown",
             })
         self.assertEqual(self.message_box.critical_args, ((
-            "Error in All\nError in email\n"
-            "Error in password\nError in unknown",
-            ), {}))
+            "Error in All", ), {}))
 
 
 class TosControllerTestCase(MockerTestCase):
@@ -725,8 +830,8 @@
         error = dict(errtype='BadTokenError')
         app_name = 'app_name'
         self.controller.on_email_validation_error(app_name, error)
-        self.assertEqual(self.message_box.critical_args, ((
-            'BadTokenError',), {}))
+        self.assertEqual(self.message_box.critical_args,
+            (("Error: {'errtype': 'BadTokenError'}",), {}))
 
 
 class ErrorControllerTestCase(MockerTestCase):