packages: refresh package list before install and handle errors

- Refresh package list before trying to install packages.  Also lookup
  the ID of the package, including the version, using the newly fetched
  package list.

- Call on_install() callback requested by modules only if the package
  installation was successful.  Handle any exception raised in the
  callback itself.

- Handle exceptions raised during packagekit operations.  Also check the
  returned results for error code.

- Capture error/success status during a transaction.  Don't destroy the
  transaction object until the error/success status has been collected.
  When a view is refreshed after completion of a transaction collect the
  result of the transaction and show it to the user.

- Handle cases where package searches in the package list fails.  Simply
  show the package name without its description.

plinth/package.py

@@ -19,7 +19,10 @@
 Framework for installing and updating distribution packages
 """
 
+from django.contrib import messages
 import functools
+from gettext import gettext as _
+from gi.repository import GLib as glib
 from gi.repository import PackageKitGlib as packagekit
 import logging
 import threading
@@ -32,6 +35,17 @@ transactions = {}
 packages_resolved = {}
 
 
+class PackageException(Exception):
+    """A package operation has failed."""
+
+    def __init__(self, error_string=None, error_details=None, *args, **kwargs):
+        """Store packagekit error string and details."""
+        super(PackageException, self).__init__(*args, **kwargs)
+
+        self.error_string = error_string
+        self.error_details = error_details
+
+
 class Transaction(object):
     """Information about an ongoing transaction."""
 
@@ -57,6 +71,10 @@ class Transaction(object):
         self.caller_active = None
         self.download_size_remaining = None
 
+        # Completion
+        self.is_finished = False
+        self.exception = None
+
     def get_id(self):
         """Return a identifier to use as a key in a map of transactions."""
         return frozenset(self.package_names)
@@ -78,13 +96,70 @@ class Transaction(object):
 
     def _install(self):
         """Run a PackageKit transaction to install given packages."""
-        package_ids = [packages_resolved[package_name].get_id()
-                       for package_name in self.package_names]
+        try:
+            self._do_install()
+        except PackageException as exception:
+            self.finish(exception)
+            return
+        except glib.Error as exception:
+            self.finish(PackageException(exception.message))
+            return
+
+        try:
+            if self.on_install:
+                self.on_install()
+        except Exception as exception:
+            logger.exception('Error during setup - %s', exception)
+            self.finish(exception)
+            return
+
+        self.finish()
+
+    def _do_install(self):
+        """Run a PackageKit transaction to install given packages.
+
+        Raise exception in case of error.
+        """
         client = packagekit.Client()
         client.set_interactive(False)
-        client.install_packages(packagekit.TransactionFlagEnum.ONLY_TRUSTED,
-                                package_ids + [None], None,
-                                self.progress_callback, self)
+
+        # Refresh package cache from all enabled repositories
+        results = client.refresh_cache(
+            False, None, self.progress_callback, self)
+        self._assert_success(results)
+
+        # Resolve packages again to get the latest versions after refresh
+        results = client.resolve(packagekit.FilterEnum.INSTALLED,
+                                 tuple(self.package_names) + (None, ),
+                                 None, self.progress_callback, self)
+        self._assert_success(results)
+
+        for package in results.get_package_array():
+            packages_resolved[package.get_name()] = package
+
+        package_ids = []
+        for package_name in self.package_names:
+            if package_name not in packages_resolved or \
+               not packages_resolved[package_name]:
+                raise PackageException(_('packages not found'))
+
+            package_ids.append(packages_resolved[package_name].get_id())
+
+        # Start package installation
+        results = client.install_packages(
+            packagekit.TransactionFlagEnum.ONLY_TRUSTED, package_ids + [None],
+            None, self.progress_callback, self)
+        self._assert_success(results)
+
+    def _assert_success(self, results):
+        """Check that the most recent operation was a success."""
+        # XXX: Untested code
+        if results and results.get_error_code() is not None:
+            error = results.get_error_code()
+            error_code = error.get_code() if error else None
+            error_string = error_code.to_string() if error_code else None
+            error_details = error.get_details() if error else None
+            raise PackageException(error_string, error_details)
 
     def progress_callback(self, progress, progress_type, user_data):
         """Process progress updates on package resolve operation"""
@@ -102,8 +177,6 @@ class Transaction(object):
             self.status = progress.props.status
             self.status_string = \
                 packagekit.StatusEnum.to_string(progress.props.status)
-            if self.status == packagekit.StatusEnum.FINISHED:
-                self.finish()
         elif progress_type == packagekit.ProgressType.TRANSACTION_FLAGS:
             self.flags = progress.props.transaction_flags
         elif progress_type == packagekit.ProgressType.ROLE:
@@ -117,15 +190,20 @@ class Transaction(object):
             logger.info('Unhandle packagekit progress callback - %s, %s',
                         progress, progress_type)
 
-    def finish(self):
-        """Perform clean up operations on the transaction.
+    def finish(self, exception=None):
+        """Mark transaction as complected and store exception if any."""
+        self.is_finished = True
+        self.exception = exception
 
-        Remove self from global transactions list.
+    def collect_result(self):
+        """Retrieve the result of this transaction.
+
+        Also remove self from global transactions list.
         """
-        if self.status == packagekit.StatusEnum.FINISHED:
-            self.on_install()
+        assert self.is_finished
 
         del transactions[self.get_id()]
+        return self.exception
 
 
 def required(package_names, on_install=None):
@@ -137,8 +215,7 @@ def required(package_names, on_install=None):
         @functools.wraps(func)
         def wrapper(request, *args, **kwargs):
             """Check and install packages required by a view."""
-            if not is_installing(package_names) and \
-               check_installed(package_names):
+            if not _should_show_install_view(request, package_names):
                 return func(request, *args, **kwargs)
 
             view = plinth.views.PackageInstallView.as_view()
@@ -150,6 +227,32 @@ def required(package_names, on_install=None):
     return wrapper2
 
 
+def _should_show_install_view(request, package_names):
+    """Return whether the installation view should be shown."""
+    transaction_id = frozenset(package_names)
+
+    # No transaction in progress
+    if transaction_id not in transactions:
+        is_installed = check_installed(package_names)
+        return not is_installed
+
+    # Installing
+    transaction = transactions[transaction_id]
+    if not transaction.is_finished:
+        return True
+
+    # Transaction finished, waiting to show the result
+    exception = transaction.collect_result()
+    if not exception:
+        messages.success(request,
+                         _('Installed and configured packages successfully'))
+        return False
+    else:
+        messages.error(request, _('Error installing packages: {details}')
+                       .format(details=exception.error_string))
+        return True
+
+
 def check_installed(package_names):
     """Return a boolean installed status of package.
 
@@ -171,6 +274,11 @@ def check_installed(package_names):
 
         packages_resolved[package.get_name()] = package
 
+    # When package names could not be resolved
+    for package_name in package_names:
+        if package_name not in packages_resolved:
+            packages_resolved[package_name] = None
+
     return set(installed_package_names) == set(package_names)
 
 

plinth/templates/package_install.html

@@ -44,9 +44,9 @@
         <tr><th>Package</th><th>Summary</th></tr>
       </thead>
       <tbody>
-        {% for package in packages %}
+        {% for package_name, package in packages.items %}
           <tr>
-            <td>{{ package.get_name }}</td>
+            <td>{{ package_name }}</td>
             <td>{{ package.get_summary }}</td>
           </tr>
         {% endfor %}

plinth/views.py

@@ -44,8 +44,9 @@ class PackageInstallView(TemplateView):
 
         if 'packages_names' not in context:
             context['package_names'] = self.kwargs.get('package_names', [])
-        context['packages'] = [package_module.packages_resolved[package_name]
-                               for package_name in context['package_names']]
+        context['packages'] = {
+            package_name: package_module.packages_resolved[package_name]
+            for package_name in context['package_names']}
         context['is_installing'] = \
             package_module.is_installing(context['package_names'])
         context['transactions'] = package_module.transactions