Merge lp:~mterry/update-manager/continue-on-error into lp:update-manager

[Michael Terry]

When an update error occurs, currently Software Updater just stops completely with the error. This branch implements the part of the spec [1] that covers errors during update. Specifically:

* Show a dialog with the error, and Settings, Try Again, and OK buttons.
* If OK is pressed with updates, show a sub-header that notes there were some errors.
* If OK is pressed with no updates, show a slightly different message than normal (one less confident that system is up to date).

Tests added too for most of the code paths.

[1] Spec not quite updated yet, but see mpt's comments: https://bugs.launchpad.net/ubuntu/+source/update-manager/+bug/1049046/comments/5

[Michael Vogt (mvo)]

This looks good, +1. Nice tests as well.

review: Approve

=== modified file 'UpdateManager/Dialogs.py'
--- UpdateManager/Dialogs.py	2012-12-13 19:56:07 +0000
+++ UpdateManager/Dialogs.py	2012-12-13 20:47:25 +0000
@@ -112,9 +112,12 @@
 
 
 class NoUpdatesDialog(Dialog):
-    def __init__(self, window_main):
+    def __init__(self, window_main, error_occurred=False):
         Dialog.__init__(self, window_main)
-        self.set_header(_("The software on this computer is up to date."))
+        if error_occurred:
+            self.set_header(_("No software updates are available."))
+        else:
+            self.set_header(_("The software on this computer is up to date."))
         self.add_settings_button()
         self.focus_button = self.add_button(Gtk.STOCK_OK, self.close)
 
@@ -198,15 +201,23 @@
     def main(self):
         Dialog.main(self)
         # The label likes to start selecting everything (b/c it got focus
-        # before
-        # we switched to our default button).
+        # before we switched to our default button).
         self.label_desc.select_region(0, 0)
-        # Since errors usually are outside the normal flow, we'll guarantee
-        # that
-        # we don't continue with normal code flow by running our own loop here.
-        # We won't screw anything up because the only thing this dialog will do
-        # is exit.
-        Gtk.main()
+
+
+class UpdateErrorDialog(ErrorDialog):
+    def __init__(self, window_main, header, desc=None):
+        ErrorDialog.__init__(self, window_main, header, desc)
+        # Get rid of normal error dialog button before adding our own
+        self.focus_button.destroy()
+        self.add_button(_("_Try Again"), self.update)
+        self.focus_button = self.add_button(Gtk.STOCK_OK, self.available)
+
+    def update(self):
+        self.window_main.start_update()
+
+    def available(self):
+        self.window_main.start_available(error_occurred=True)
 
 
 class NeedRestartDialog(Dialog):

=== modified file 'UpdateManager/InstallProgress.py'
--- UpdateManager/InstallProgress.py	2012-07-06 19:25:03 +0000
+++ UpdateManager/InstallProgress.py	2012-12-13 20:47:25 +0000
@@ -81,7 +81,8 @@
                 pkgs_upgrade.append(pkg.name)
         self.install_backend.commit(pkgs_install, pkgs_upgrade, close_on_done)
 
-    def _on_backend_done(self, backend, action, authorized, success):
+    def _on_backend_done(self, backend, action, authorized, success,
+                         error_string, error_desc):
         # Allow suspend after synaptic is finished
         if self.sleep_cookie:
             allow_sleep(self.sleep_dev, self.sleep_cookie)
@@ -90,6 +91,8 @@
         # Either launch main dialog and continue or quit altogether
         if success:
             self.window_main.start_available()
+        elif error_string:
+            self.window_main.start_error(False, error_string, error_desc)
         else:
             sys.exit(0)
 

=== modified file 'UpdateManager/UpdateManager.py'
--- UpdateManager/UpdateManager.py	2012-11-20 13:09:01 +0000
+++ UpdateManager/UpdateManager.py	2012-12-13 20:47:25 +0000
@@ -48,7 +48,8 @@
                       NoUpdatesDialog,
                       StoppedUpdatesDialog,
                       PartialUpgradeDialog,
-                      UnsupportedDialog)
+                      UnsupportedDialog,
+                      UpdateErrorDialog)
 from .InstallProgress import InstallProgress
 from .MetaReleaseGObject import MetaRelease
 from .UpdateProgress import UpdateProgress
@@ -163,17 +164,17 @@
 
         self._start_pane(UpdateProgress(self))
 
-    def start_available(self, cancelled_update=False):
+    def start_available(self, cancelled_update=False, error_occurred=True):
         self._look_busy()
         self.refresh_cache()
 
         pane = self._make_available_pane(self.cache.install_count,
                                          os.path.exists(REBOOT_REQUIRED_FILE),
-                                         cancelled_update)
+                                         cancelled_update, error_occurred)
         self._start_pane(pane)
 
-    def _make_available_pane(self, install_count, need_reboot,
-                             cancelled_update):
+    def _make_available_pane(self, install_count, need_reboot=False,
+                             cancelled_update=False, error_occurred=False):
         if install_count == 0:
             # Need Restart > New Release > No Updates
             if need_reboot:
@@ -184,11 +185,13 @@
             elif cancelled_update:
                 return StoppedUpdatesDialog(self)
             else:
-                return NoUpdatesDialog(self)
+                return NoUpdatesDialog(self, error_occurred=error_occurred)
         else:
             header = None
             desc = None
-            if cancelled_update:
+            if error_occurred:
+                desc = _("Some software couldn’t be checked for updates.")
+            elif cancelled_update:
                 header = _("You stopped the check for updates.")
                 desc = _("Updated software is available from "
                          "a previous check.")
@@ -197,8 +200,11 @@
     def start_install(self):
         self._start_pane(InstallProgress(self))
 
-    def start_error(self, header, desc):
-        self._start_pane(ErrorDialog(self, header, desc))
+    def start_error(self, is_update_error, header, desc):
+        if is_update_error:
+            self._start_pane(UpdateErrorDialog(self, header, desc))
+        else:
+            self._start_pane(ErrorDialog(self, header, desc))
 
     def _start_pane(self, pane):
         self._look_busy()

=== modified file 'UpdateManager/UpdateProgress.py'
--- UpdateManager/UpdateProgress.py	2012-08-24 20:29:12 +0000
+++ UpdateManager/UpdateProgress.py	2012-12-13 20:47:25 +0000
@@ -65,13 +65,17 @@
 
         self.install_backend.update()
 
-    def _on_backend_done(self, backend, action, authorized, success):
+    def _on_backend_done(self, backend, action, authorized, success,
+                         error_string, error_desc):
         # Allow suspend after synaptic is finished
         if self.sleep_cookie:
             allow_sleep(self.sleep_dev, self.sleep_cookie)
             self.sleep_cookie = self.sleep_dev = None
 
-        self.window_main.start_available(not success)
+        if error_string:
+            self.window_main.start_error(True, error_string, error_desc)
+        else:
+            self.window_main.start_available(not success)
 
     def main(self):
         self.invoke_manager()

=== modified file 'UpdateManager/UpdatesAvailable.py'
--- UpdateManager/UpdatesAvailable.py	2012-11-19 16:33:28 +0000
+++ UpdateManager/UpdatesAvailable.py	2012-12-13 20:47:25 +0000
@@ -468,7 +468,6 @@
 
         if self.custom_header is not None:
             text_header = self.custom_header
-            text_desc = self.custom_desc
         # show different text on first run (UX team suggestion)
         elif self.settings.get_boolean("first-run"):
             flavor = self.window_main.meta_release.flavor_name
@@ -481,6 +480,9 @@
             text_header = _("Updated software is available for this "
                             "computer. Do you want to install it now?")
 
+        if self.custom_desc is not None:
+            text_desc = self.custom_desc
+
         self.notebook_details.set_sensitive(True)
         self.treeview_update.set_sensitive(True)
         self.button_install.grab_default()
@@ -556,7 +558,7 @@
             self.cache.checkFreeSpace()
         except NotEnoughFreeSpaceError as e:
             for req in e.free_space_required_list:
-                self.window_main.start_error(err_sum,
+                self.window_main.start_error(False, err_sum,
                                              err_long % (req.size_total,
                                                          req.dir,
                                                          req.size_needed,

=== modified file 'UpdateManager/backend/InstallBackendAptdaemon.py'
--- UpdateManager/backend/InstallBackendAptdaemon.py	2012-08-20 02:39:19 +0000
+++ UpdateManager/backend/InstallBackendAptdaemon.py	2012-12-13 20:47:25 +0000
@@ -13,12 +13,13 @@
 from aptdaemon.gtk3widgets import (AptCancelButton,
                                    AptConfigFileConflictDialog,
                                    AptDetailsExpander,
-                                   AptErrorDialog,
                                    AptMediumRequiredDialog,
                                    AptProgressBar)
 from aptdaemon.enums import (EXIT_SUCCESS,
                              EXIT_FAILED,
                              STATUS_COMMITTING,
+                             get_error_description_from_enum,
+                             get_error_string_from_enum,
                              get_status_string_from_enum)
 
 from UpdateManager.backend import InstallBackend
@@ -62,9 +63,9 @@
                                       _("Checking for updates…"),
                                       False, False)
         except errors.NotAuthorizedError:
-            self.emit("action-done", self.UPDATE, False, False)
+            self.emit("action-done", self.UPDATE, False, False, None, None)
         except:
-            self.emit("action-done", self.UPDATE, True, False)
+            self.emit("action-done", self.UPDATE, True, False, None, None)
             raise
 
     @inline_callbacks
@@ -84,14 +85,14 @@
                                       _("Installing updates…"),
                                       True, close_on_done)
         except errors.NotAuthorizedError as e:
-            self.emit("action-done", self.INSTALL, False, False)
+            self.emit("action-done", self.INSTALL, False, False, None, None)
         except dbus.DBusException as e:
             #print(e, e.get_dbus_name())
             if e.get_dbus_name() != "org.freedesktop.DBus.Error.NoReply":
                 raise
-            self.emit("action-done", self.INSTALL, False, False)
+            self.emit("action-done", self.INSTALL, False, False, None, None)
         except Exception as e:
-            self.emit("action-done", self.INSTALL, True, False)
+            self.emit("action-done", self.INSTALL, True, False, None, None)
             raise
 
     def _on_progress_changed(self, trans, progress):
@@ -223,16 +224,17 @@
             transaction.resolve_config_file_conflict(old, "keep")
 
     def _on_finished(self, trans, status, action, close_on_done):
+        error_string = None
+        error_desc = None
         if status == EXIT_FAILED:
-            err_dia = AptErrorDialog(trans.error, self.window_main)
-            err_dia.run()
-            err_dia.hide()
-            sys.exit(0)
+            error_string = get_error_string_from_enum(trans.error.code)
+            error_desc = get_error_description_from_enum(trans.error.code)
         elif status == EXIT_SUCCESS and close_on_done:
             sys.exit(0)
         # tell unity to hide the progress again
         self.unity.set_progress(-1)
-        self.emit("action-done", action, True, status == EXIT_SUCCESS)
+        self.emit("action-done", action, True, status == EXIT_SUCCESS,
+                  error_string, error_desc)
 
 if __name__ == "__main__":
     b = InstallBackendAptdaemon(None)

=== modified file 'UpdateManager/backend/InstallBackendSynaptic.py'
--- UpdateManager/backend/InstallBackendSynaptic.py	2012-06-28 19:29:15 +0000
+++ UpdateManager/backend/InstallBackendSynaptic.py	2012-12-13 20:47:25 +0000
@@ -42,7 +42,8 @@
         action, tempf = data
         if tempf:
             tempf.close()
-        self.emit("action-done", action, True, os.WEXITSTATUS(condition) == 0)
+        self.emit("action-done", action, True, os.WEXITSTATUS(condition) == 0,
+                  None, None)
 
     def update(self):
         opt = ["--update-at-startup"]

=== modified file 'UpdateManager/backend/__init__.py'
--- UpdateManager/backend/__init__.py	2012-06-28 00:10:23 +0000
+++ UpdateManager/backend/__init__.py	2012-12-13 20:47:25 +0000
@@ -18,7 +18,9 @@
                                     None,
                                     (GObject.TYPE_INT,      # action id
                                      GObject.TYPE_BOOLEAN,  # authorized
-                                     GObject.TYPE_BOOLEAN)  # success
+                                     GObject.TYPE_BOOLEAN,  # success
+                                     GObject.TYPE_STRING,   # error string
+                                     GObject.TYPE_STRING)   # error desc
                                     ),
                     }
 

=== modified file 'tests/test_stop_update.py'
--- tests/test_stop_update.py	2012-08-24 23:04:09 +0000
+++ tests/test_stop_update.py	2012-12-13 20:47:25 +0000
@@ -5,6 +5,7 @@
 import sys
 import unittest
 from mock import patch
+from gettext import gettext as _
 
 from UpdateManager.UpdateManager import UpdateManager
 from UpdateManager.UpdatesAvailable import UpdatesAvailable
@@ -24,23 +25,25 @@
         self.manager.datadir = os.path.join(CURDIR, '..', 'data')
 
     def test_stop_no_updates(self):
-        # install_count, need_reboot, cancelled_update
-        p = UpdateManager._make_available_pane(self.manager, 0, False, True)
+        p = UpdateManager._make_available_pane(self.manager, 0,
+                                               cancelled_update=True)
         self.assertIsInstance(p, Dialogs.StoppedUpdatesDialog)
 
     def test_no_stop_no_updates(self):
-        # install_count, need_reboot, cancelled_update
-        p = UpdateManager._make_available_pane(self.manager, 0, False, False)
+        p = UpdateManager._make_available_pane(self.manager, 0,
+                                               cancelled_update=False)
         self.assertNotIsInstance(p, Dialogs.StoppedUpdatesDialog)
 
     def test_stop_updates(self):
-        # install_count, need_reboot, cancelled_update
-        p = UpdateManager._make_available_pane(self.manager, 1, False, True)
+        p = UpdateManager._make_available_pane(self.manager, 1,
+                                               cancelled_update=True)
         self.assertIsInstance(p, UpdatesAvailable)
-        self.assertIsNotNone(p.custom_header)
+        self.assertEqual(p.custom_header,
+                         _("You stopped the check for updates."))
 
     def test_no_stop_updates(self):
-        p = UpdateManager._make_available_pane(self.manager, 1, False, False)
+        p = UpdateManager._make_available_pane(self.manager, 1,
+                                               cancelled_update=False)
         self.assertIsInstance(p, UpdatesAvailable)
         self.assertIsNone(p.custom_header)
 

=== added file 'tests/test_update_error.py'
--- tests/test_update_error.py	1970-01-01 00:00:00 +0000
+++ tests/test_update_error.py	2012-12-13 20:47:25 +0000
@@ -0,0 +1,71 @@
+#!/usr/bin/python3
+# -*- Mode: Python; indent-tabs-mode: nil; tab-width: 4; coding: utf-8 -*-
+
+import logging
+import mock
+import sys
+import unittest
+from gettext import gettext as _
+from mock import patch
+
+from UpdateManager.Dialogs import NoUpdatesDialog
+from UpdateManager.UpdateManager import UpdateManager
+from UpdateManager.UpdateProgress import UpdateProgress
+from UpdateManager.UpdatesAvailable import UpdatesAvailable
+
+import os
+CURDIR = os.path.dirname(os.path.abspath(__file__))
+
+
+class TestUpdateManagerError(unittest.TestCase):
+
+    def setUp(self):
+        patcher = patch('UpdateManager.UpdateManager.UpdateManager')
+        self.addCleanup(patcher.stop)
+        self.manager = patcher.start()
+        self.manager._check_meta_release.return_value = False
+        self.manager.datadir = os.path.join(CURDIR, '..', 'data')
+
+    def test_error_no_updates(self):
+        p = UpdateManager._make_available_pane(self.manager, 0,
+                                               error_occurred=True)
+        self.assertIsInstance(p, NoUpdatesDialog)
+        self.assertEqual(p.label_header.get_label(),
+                         _("No software updates are available."))
+
+    def test_error_with_updates(self):
+        p = UpdateManager._make_available_pane(self.manager, 1,
+                                               error_occurred=True)
+        self.assertIsInstance(p, UpdatesAvailable)
+        self.assertEqual(p.custom_desc,
+                         _("Some software couldn’t be checked for updates."))
+
+
+class TestBackendError(unittest.TestCase):
+
+    def setUp(self):
+        os.environ['UPDATE_MANAGER_FORCE_BACKEND_APTDAEMON'] = '1'
+
+        def clear_environ():
+            del os.environ['UPDATE_MANAGER_FORCE_BACKEND_APTDAEMON']
+
+        self.addCleanup(clear_environ)
+
+    @patch('UpdateManager.backend.InstallBackendAptdaemon.'
+           'InstallBackendAptdaemon.update')
+    def test_backend_error(self, update):
+        main = mock.MagicMock()
+        main.datadir = os.path.join(CURDIR, '..', 'data')
+
+        progress = UpdateProgress(main)
+        update.side_effect = lambda: progress.install_backend.emit(
+            "action-done", progress.install_backend.UPDATE, True, False,
+            "string", "desc")
+
+        progress.main()
+        main.start_error.assert_called_once_with(True, "string", "desc")
+
+if __name__ == '__main__':
+    if len(sys.argv) > 1 and sys.argv[1] == "-v":
+        logging.basicConfig(level=logging.DEBUG)
+    unittest.main()