From 983f4d13e86bef85c85822d8845fcca043f6c82d Mon Sep 17 00:00:00 2001
From: Sunil Mohan Adapa <sunil@medhas.org>
Date: Fri, 25 Jan 2019 17:56:36 -0800
Subject: [PATCH] backups: Fix showing not-installed apps in create backup page

- Also minor styling.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
---
 plinth/modules/backups/api.py                   | 17 +++++++++++++----
 plinth/modules/backups/static/loading_button.js |  2 ++
 .../backups/templates/backups_restore.html      |  4 ++--
 plinth/modules/backups/tests/test_api.py        |  4 ++--
 plinth/modules/backups/views.py                 |  9 +++------
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/plinth/modules/backups/api.py b/plinth/modules/backups/api.py
index 92851a661..f359c35ae 100644
--- a/plinth/modules/backups/api.py
+++ b/plinth/modules/backups/api.py
@@ -169,7 +169,7 @@ def backup_apps(backup_handler, path, app_names=None,
     if not app_names:
         apps = get_all_apps_for_backup()
     else:
-        apps = _get_apps_in_order(app_names)
+        apps = get_apps_in_order(app_names)
 
     if _is_snapshot_available():
         snapshot = _take_snapshot()
@@ -198,7 +198,7 @@ def restore_apps(restore_handler, app_names=None, create_subvolume=True,
     if not app_names:
         apps = get_all_apps_for_backup()
     else:
-        apps = _get_apps_in_order(app_names)
+        apps = get_apps_in_order(app_names)
 
     _install_apps_before_restore(apps)
 
@@ -261,6 +261,13 @@ class BackupApp:
             self.manifest == other_app.manifest and \
             self.has_data == other_app.has_data
 
+    def is_installed(self):
+        """Return whether app is installed.
+
+        Return true even if the app needs update.
+        """
+        return self.app.setup_helper.get_state() != 'needs-setup'
+
     def run_hook(self, hook, packet):
         """Run a hook inside an application."""
         if not hasattr(self.app, hook):
@@ -280,14 +287,16 @@ def get_all_apps_for_backup():
     apps = []
     for module_name, module in module_loader.loaded_modules.items():
         try:
-            apps.append(BackupApp(module_name, module))
+            backup_app = BackupApp(module_name, module)
+            if backup_app.is_installed():
+                apps.append(backup_app)
         except TypeError:  # Application not available for backup/restore
             pass
 
     return apps
 
 
-def _get_apps_in_order(app_names):
+def get_apps_in_order(app_names):
     """Return a list of app modules in order of dependency."""
     apps = []
     for module_name, module in module_loader.loaded_modules.items():
diff --git a/plinth/modules/backups/static/loading_button.js b/plinth/modules/backups/static/loading_button.js
index bc87c4ca1..d5e90d24f 100644
--- a/plinth/modules/backups/static/loading_button.js
+++ b/plinth/modules/backups/static/loading_button.js
@@ -22,6 +22,8 @@
 */
 
 
+// XXX: This is misuse of sr-only class. This is problematic for people using
+// screen readers.
 function swapWithLoadingButton() {
     $("#restore_btn").addClass("sr-only");
     $("#loading_btn").removeClass("sr-only");
diff --git a/plinth/modules/backups/templates/backups_restore.html b/plinth/modules/backups/templates/backups_restore.html
index e940b691d..4893911d7 100644
--- a/plinth/modules/backups/templates/backups_restore.html
+++ b/plinth/modules/backups/templates/backups_restore.html
@@ -59,6 +59,6 @@
 {% endblock %}
 
 {% block page_js %}
-  <script type="text/javascript" src="{% static 'backups/loading_button.js' %}"></script>
+  <script type="text/javascript"
+          src="{% static 'backups/loading_button.js' %}"></script>
 {% endblock %}
-
diff --git a/plinth/modules/backups/tests/test_api.py b/plinth/modules/backups/tests/test_api.py
index b1b31048c..8f256c8e0 100644
--- a/plinth/modules/backups/tests/test_api.py
+++ b/plinth/modules/backups/tests/test_api.py
@@ -140,7 +140,7 @@ class TestBackupProcesses(unittest.TestCase):
 
     @staticmethod
     @patch('plinth.module_loader.loaded_modules.items')
-    def test__get_apps_in_order(modules):
+    def test_get_apps_in_order(modules):
         """Test that apps are listed in correct dependency order."""
         apps = [
             ('names', MagicMock(backup=_get_test_manifest('names'))),
@@ -150,7 +150,7 @@ class TestBackupProcesses(unittest.TestCase):
 
         module_loader.load_modules()
         app_names = ['config', 'names']
-        apps = api._get_apps_in_order(app_names)
+        apps = api.get_apps_in_order(app_names)
         assert apps[0].name == 'names'
         assert apps[1].name == 'config'
 
diff --git a/plinth/modules/backups/views.py b/plinth/modules/backups/views.py
index 03080d58d..f176d6627 100644
--- a/plinth/modules/backups/views.py
+++ b/plinth/modules/backups/views.py
@@ -179,10 +179,7 @@ class BaseRestoreView(SuccessMessageMixin, FormView):
         """Pass additional keyword args for instantiating the form."""
         kwargs = super().get_form_kwargs()
         included_apps = self._get_included_apps()
-        installed_apps = api.get_all_apps_for_backup()
-        kwargs['apps'] = [
-            app for app in installed_apps if app.name in included_apps
-        ]
+        kwargs['apps'] = api.get_apps_in_order(included_apps)
         return kwargs
 
     def get_context_data(self, **kwargs):
@@ -202,8 +199,8 @@ class RestoreFromUploadView(BaseRestoreView):
         if not os.path.isfile(path):
             messages.error(self.request, _('No backup file found.'))
             return redirect(reverse_lazy('backups:index'))
-        else:
-            return super().get(*args, **kwargs)
+
+        return super().get(*args, **kwargs)
 
     def get_context_data(self, **kwargs):
         """Return additional context for rendering the template."""