Merge lp:~nataliabidart/ubuntu-sso-client/use-ui-no-matter-what-stable-3-0 into lp:ubuntu-sso-client/stable-3-0

[Natalia Bidart]

- Made the service use the Qt UI if available, and if no UI is installed, propagate a decent error instead of AssertionError (LP: #994632).

[dobey (dobey)]

review: Approve

[Roberto Alsina (ralsina)]

+1

review: Approve

=== modified file 'ubuntu_sso/credentials.py'
--- ubuntu_sso/credentials.py	2012-04-09 17:38:24 +0000
+++ ubuntu_sso/credentials.py	2012-05-04 19:12:19 +0000
@@ -49,7 +49,12 @@
 
 from twisted.internet import defer
 
-from ubuntu_sso import UI_EXECUTABLE_GTK, USER_CANCELLATION, USER_SUCCESS
+from ubuntu_sso import (
+    UI_EXECUTABLE_GTK,
+    UI_EXECUTABLE_QT,
+    USER_CANCELLATION,
+    USER_SUCCESS,
+)
 from ubuntu_sso.keyring import Keyring
 from ubuntu_sso.logger import setup_logging
 from ubuntu_sso.utils import get_bin_dir, runner
@@ -79,6 +84,10 @@
     """The user is not validated."""
 
 
+class GUINotAvailableError(CredentialsError):
+    """No user graphical interface is available."""
+
+
 def handle_failures(msg):
     """Handle failures using 'msg' as error message."""
 
@@ -153,14 +162,32 @@
 
     @defer.inlineCallbacks
     def _show_ui(self, login_only):
-        """Shows the UI, connect outcome signals."""
-        ui_exe_path = os.path.join(get_bin_dir(), self.ui_executable)
-        if not os.path.exists(ui_exe_path):
-            logger.debug('Falling back to the GTK+ UI since the given %r '
-                         'does not exist', ui_exe_path)
-            ui_exe_path = os.path.join(get_bin_dir(), UI_EXECUTABLE_GTK)
-
-        assert os.path.exists(ui_exe_path)
+        """Show the UI and wait for it to finish.
+
+        Upon analyzing returning code from the UI process, emit proper signals
+        to the caller.
+
+        The caller can specify a preference for the UI (so far either GTK+ or
+        Qt UIs are available), but if the preferred one is not available, the
+        service will try to open any available UI.
+
+        If no GUI is available, GUINotAvailableError will be raised.
+
+        """
+        guis = (self.ui_executable, UI_EXECUTABLE_GTK, UI_EXECUTABLE_QT)
+        for gui_exe_path in guis:
+            ui_exe_path = os.path.join(get_bin_dir(), gui_exe_path)
+            ui_exe_exists = os.path.exists(ui_exe_path)
+            logger.debug('Attempting to use the following executable as GUI: '
+                '%r (exists? %r).', ui_exe_path, ui_exe_exists)
+            if not ui_exe_exists:
+                logger.error('The given UI path %r does not exist.',
+                    ui_exe_path)
+            else:
+                break
+        else:
+            raise GUINotAvailableError('Can not find a GUI to present to the '
+                'user (tried with %r). Aborting.' % repr(guis))
 
         args = [ui_exe_path]
         for arg in ('app_name', 'help_text', 'ping_url', 'policy_url',

=== modified file 'ubuntu_sso/tests/test_credentials.py'
--- ubuntu_sso/tests/test_credentials.py	2012-04-09 17:38:24 +0000
+++ ubuntu_sso/tests/test_credentials.py	2012-05-04 19:12:19 +0000
@@ -37,7 +37,7 @@
 
 import ubuntu_sso.main
 
-from ubuntu_sso import credentials, utils
+from ubuntu_sso import credentials
 from ubuntu_sso.credentials import (
     APP_NAME_KEY,
     HELP_TEXT_KEY,
@@ -111,6 +111,8 @@
 class BasicTestCase(TestCase):
     """Test case with a helper tracker."""
 
+    bin_dir = 'some/bin/dir'
+
     @defer.inlineCallbacks
     def setUp(self):
         """Init."""
@@ -121,6 +123,8 @@
         self.memento.setLevel(logging.DEBUG)
         credentials.logger.addHandler(self.memento)
 
+        self.patch(credentials, 'get_bin_dir', lambda: self.bin_dir)
+
     def _set_called(self, *args, **kwargs):
         """Set _called to True."""
         self._called = (args, kwargs)
@@ -285,8 +289,7 @@
         yield super(RegisterTestCase, self).setUp()
         self.deferred = defer.Deferred()
         self.patch_inner(self.fake_inner)
-        self.exe_path = os.path.join(utils.get_bin_dir(),
-                                     KWARGS[UI_EXECUTABLE_KEY])
+        self.exe_path = os.path.join(self.bin_dir, KWARGS[UI_EXECUTABLE_KEY])
         self.inner_args = [
             self.exe_path,
             '--app_name', APP_NAME,
@@ -347,11 +350,9 @@
                    lambda kr, app: defer.succeed(None))
         self._next_inner_result = credentials.USER_SUCCESS
 
-        # patch os.path.exists so it also returns True when queried for the
-        # faked exe_path
-        really_exists = os.path.exists
+        # patch os.path.exists so it returns True for the faked exe_path
         self.patch(credentials.os.path, 'exists',
-                   lambda path: path == self.exe_path or really_exists(path))
+                   lambda path: path == self.exe_path)
 
         result = yield self.method_call(**self.kwargs)
         self.assertEqual(result, TOKEN)
@@ -368,34 +369,71 @@
         self._next_inner_result = credentials.USER_SUCCESS
 
         assert not os.path.exists(self.exe_path)
+        self.patch(credentials.os.path, 'exists',
+                   lambda path: path.endswith(credentials.UI_EXECUTABLE_GTK))
 
         result = yield self.method_call(**self.kwargs)
         self.assertEqual(result, TOKEN)
 
         # the ui was opened and proper params were passed
         args = yield self.deferred
-        self.inner_args[0] = os.path.join(utils.get_bin_dir(),
+        self.inner_args[0] = os.path.join(self.bin_dir,
                                           credentials.UI_EXECUTABLE_GTK)
         self.assertEqual(self.inner_args, args)
 
     @defer.inlineCallbacks
+    def test_ui_executable_uses_qt_ui_if_gtk_not_available(self):
+        """The executable loads the QT UI if the GTK+ does not exist."""
+        self.patch(credentials.Keyring, 'get_credentials',
+                   lambda kr, app: defer.succeed(None))
+        self._next_inner_result = credentials.USER_SUCCESS
+
+        self.patch(credentials.os.path, 'exists',
+                   lambda path: path.endswith(credentials.UI_EXECUTABLE_QT))
+
+        result = yield self.method_call(**self.kwargs)
+        self.assertEqual(result, TOKEN)
+
+        # the ui was opened and proper params were passed
+        args = yield self.deferred
+        self.inner_args[0] = os.path.join(self.bin_dir,
+                                          credentials.UI_EXECUTABLE_QT)
+        self.assertEqual(self.inner_args, args)
+
+    @defer.inlineCallbacks
+    def test_raises_exception_if_no_ui_available(self):
+        """If no GUI is available, raise GUINotAvailableError."""
+        self.patch(credentials.Keyring, 'get_credentials',
+                   lambda kr, app: defer.succeed(None))
+        self.patch(credentials.os.path, 'exists', lambda path: False)
+
+        f = self.method_call(**self.kwargs)
+        yield self.assertFailure(f, credentials.GUINotAvailableError)
+
+    @defer.inlineCallbacks
     def test_without_existent_token_and_return_code_cancel(self, exc=None):
         """The operation returns exc if defined, else UserCancellationError."""
         self.patch(credentials.Keyring, 'get_credentials',
                    lambda kr, app: defer.succeed(None))
         self._next_inner_result = credentials.USER_CANCELLATION
 
+        self.patch(credentials.os.path, 'exists',
+                   lambda path: path == self.exe_path)
+
         if exc is None:
             exc = credentials.UserCancellationError
         yield self.assertFailure(self.method_call(**self.kwargs), exc)
 
     @defer.inlineCallbacks
-    def test_without_existent_token_and_return_other(self, result=None):
+    def test_without_existent_token_and_return_other_code(self, result=None):
         """The operation returns CredentialsError with 'result'."""
         self.patch(credentials.Keyring, 'get_credentials',
                    lambda kr, app: defer.succeed(None))
         self._next_inner_result = credentials.USER_CANCELLATION * 2  # other
 
+        self.patch(credentials.os.path, 'exists',
+                   lambda path: path == self.exe_path)
+
         exc = yield self.assertFailure(self.method_call(**self.kwargs),
                                        credentials.CredentialsError)
         if result is None:
@@ -422,6 +460,9 @@
                    lambda kr, app: defer.succeed(None))
         self.patch_inner(self.fail_inner)
 
+        self.patch(credentials.os.path, 'exists',
+                   lambda path: path == self.exe_path)
+
         yield self.assertFailure(self.method_call(**self.kwargs),
                                  SampleMiscException)
 
@@ -478,14 +519,20 @@
     def test_ui_executable_falls_back_to_gtk(self):
         """This check does not apply for this test case."""
 
+    def test_ui_executable_uses_qt_ui_if_gtk_not_available(self):
+        """This check does not apply for this test case."""
+
+    def test_raises_exception_if_no_ui_available(self):
+        """This check does not apply for this test case."""
+
     def test_without_existent_token_and_return_code_cancel(self, exc=None):
         """The operation returns UserNotValidatedError."""
         exc = credentials.UserNotValidatedError
         return super(LoginEmailPasswordTestCase, self).\
                test_without_existent_token_and_return_code_cancel(exc=exc)
 
-    def test_without_existent_token_and_return_other(self, result=None):
+    def test_without_existent_token_and_return_other_code(self, result=None):
         """The operation returns CredentialsError with 'result'."""
         result = 'foo'
         return super(LoginEmailPasswordTestCase, self).\
-               test_without_existent_token_and_return_other(result=result)
+               test_without_existent_token_and_return_other_code(result=result)