Backups: clean up forms, names and templates

Reviewed-by: James Valleroy <jvalleroy@mailbox.org>

actions/backups

@@ -56,15 +56,14 @@ def parse_arguments():
 
     export_tar = subparsers.add_parser('export-tar',
                                    help='Export archive contents as tarball')
-    export_tar.add_argument('--archive', help='Archive name',
+    export_tar.add_argument('--name', help='Archive name)', required=True)
-                            required=True)
     export_tar.add_argument('--filepath', help='Destination tarball file path',
                             required=True)
 
-    get_apps_of_exported_archive = subparsers.add_parser(
+    get_exported_archive_apps = subparsers.add_parser(
-        'get-apps-of-exported-archive',
+        'get-exported-archive-apps',
         help='Get list of apps included in exported archive file')
-    get_apps_of_exported_archive.add_argument(
+    get_exported_archive_apps.add_argument(
         '--path', help='Tarball file path', required=True)
 
     get_archive_apps = subparsers.add_parser(
@@ -166,7 +165,7 @@ def subcommand_export_tar(arguments):
         os.makedirs(directory)
 
     subprocess.run([
-        'borg', 'export-tar', REPOSITORY + '::' + arguments.archive,
+        'borg', 'export-tar', REPOSITORY + '::' + arguments.name,
         arguments.filepath
     ], check=True)
 
@@ -202,8 +201,8 @@ def subcommand_get_archive_apps(arguments):
             print(app['name'])
 
 
-def subcommand_get_apps_of_exported_archive(arguments):
+def subcommand_get_exported_archive_apps(arguments):
-    """Get list of apps included in exported archive file."""
+    """Get list of apps included in an exported archive file."""
     manifest = None
     with tarfile.open(arguments.path) as t:
         filenames = t.getnames()
@@ -225,8 +224,7 @@ def subcommand_restore_archive(arguments):
     _locations = json.loads(locations_data)
     locations = _locations['directories'] + _locations['files']
     locations = [os.path.relpath(location, '/') for location in locations]
-    _extract(arguments.path, arguments.destination,
+    _extract(arguments.path, arguments.destination, locations=locations)
-             locations=locations)
 
 
 def subcommand_restore_exported_archive(arguments):

plinth/modules/backups/__init__.py

@@ -43,10 +43,10 @@ service = None
 
 MANIFESTS_FOLDER = '/var/lib/plinth/backups-manifests/'
 REPOSITORY = '/var/lib/freedombox/borgbackup'
-SESSION_BACKUP_VARIABLE = 'fbx-backup-filestamp'
-# default backup path for temporary actions like imports or download
-TMP_BACKUP_PATH = '/tmp/freedombox-backup.tar.gz'
 # session variable name that stores when a backup file should be deleted
+SESSION_BACKUP_VARIABLE = 'fbx-backup-filestamp'
+# default backup path for temporary backup files during down- or upload
+UPLOAD_BACKUP_PATH = '/tmp/freedombox-backup.tar.gz'
 
 
 def init():
@@ -105,17 +105,20 @@ def create_archive(name, app_names):
 
 
 def delete_archive(name):
-    # TODO: is name actually a path?
     actions.superuser_run('backups', ['delete', '--name', name])
 
 
-def delete_tmp_backup_file():
+def delete_upload_backup_file():
-    if os.path.isfile(TMP_BACKUP_PATH):
+    if os.path.isfile(UPLOAD_BACKUP_PATH):
-        os.remove(TMP_BACKUP_PATH)
+        os.remove(UPLOAD_BACKUP_PATH)
 
 
-def export_archive(name, filepath=TMP_BACKUP_PATH):
+def export_archive(name, filepath=UPLOAD_BACKUP_PATH):
-    arguments = ['export-tar', '--archive', name, '--filepath', filepath]
+    """Export an archive as .tar.gz file
+
+    name: name of the repository (w/o path)
+    """
+    arguments = ['export-tar', '--name', name, '--filepath', filepath]
     actions.superuser_run('backups', arguments)
 
 
@@ -131,9 +134,9 @@ def get_archive_apps(path):
     return output.splitlines()
 
 
-def get_apps_of_exported_archive(path):
+def get_exported_archive_apps(path):
     """Get list of apps included in exported archive file."""
-    arguments = ['get-apps-of-exported-archive', '--path', path]
+    arguments = ['get-exported-archive-apps', '--path', path]
     output = actions.superuser_run('backups', arguments)
     return output.splitlines()
 
@@ -154,10 +157,10 @@ def _restore_archive_handler(packet):
                 packet.label, '--destination', '/'], input=locations_data.encode())
 
 
-def restore_from_tmp(apps=None):
+def restore_from_upload(apps=None):
-    """Restore files from temporary backup file"""
+    """Restore files from (uploaded) eported backup file"""
     api.restore_apps(_restore_exported_archive_handler, app_names=apps,
-                     create_subvolume=False, backup_file=TMP_BACKUP_PATH)
+                     create_subvolume=False, backup_file=UPLOAD_BACKUP_PATH)
 
 
 def restore(archive_path, apps=None):

plinth/modules/backups/api.py

@@ -110,13 +110,15 @@ class Packet:
         All paths populated are relative to the 'root' path. The root path
         itself must not be stored in the backup.
 
+        label is either an archive name (w/o path), or the full path of an
+        exported archive.
+        TODO: create two variables out of it as it's distinct information.
+
         """
         self.operation = operation
         self.scope = scope
-        # TODO: do we need root if we have the path?
         self.root = root
         self.apps = apps
-        # TODO: label is an archive path -- rename
         self.label = label
         self.errors = []
 

plinth/modules/backups/forms.py

@@ -61,8 +61,7 @@ class CreateArchiveForm(forms.Form):
 
 class RestoreForm(forms.Form):
     selected_apps = forms.MultipleChoiceField(
-        label=_('Restore apps'),
+        label=_('Select the apps you want to restore'),
-        help_text=_('Apps data to restore from the backup'),
         widget=forms.CheckboxSelectMultiple)
 
     def __init__(self, *args, **kwargs):
@@ -73,20 +72,7 @@ class RestoreForm(forms.Form):
         self.fields['selected_apps'].initial = [app.name for app in apps]
 
 
-class RestoreFromTmpForm(forms.Form):
+class UploadForm(forms.Form):
-    selected_apps = forms.MultipleChoiceField(
-        label=_('Restore apps'),
-        widget=forms.CheckboxSelectMultiple)
-
-    def __init__(self, *args, **kwargs):
-        """Initialize the form with selectable apps."""
-        apps = kwargs.pop('apps')
-        super().__init__(*args, **kwargs)
-        self.fields['selected_apps'].choices = _get_app_choices(apps)
-        self.fields['selected_apps'].initial = [app.name for app in apps]
-
-
-class UploadToTmpForm(forms.Form):
     file = forms.FileField(label=_('Upload File'), required=True,
             validators=[FileExtensionValidator(['gz'],
                 'Backup files have to be in .tar.gz format')],

plinth/modules/backups/middleware.py

@@ -16,8 +16,7 @@
 #
 
 """
-Django middleware to redirect to firstboot wizard if it has not be run
+Django middleware to occasionally delete temporary backup files
-yet.
 """
 
 import logging
@@ -41,8 +40,8 @@ class BackupsMiddleware(MiddlewareMixin):
             if request.session.has_key(backups.SESSION_BACKUP_VARIABLE):
                 now = time.time()
                 if now > request.session[backups.SESSION_BACKUP_VARIABLE]:
-                    backups.delete_tmp_backup_file()
+                    backups.delete_upload_backup_file()
                     del request.session[backups.SESSION_BACKUP_VARIABLE]
             else:
-                backups.delete_tmp_backup_file()
+                backups.delete_upload_backup_file()
         return

plinth/modules/backups/templates/backups_restore.html

@@ -24,24 +24,11 @@
 {% block content %}
   <h2>{{ title }}</h2>
 
-  <p>{% trans "Restore data from this archive?" %}</p>
+  {% if name %}
-
+    <p>
-  <div class="row">
+      {% trans 'Restore data from' %} {{ name }}
-    <div class="col-lg-12">
+    </p>
-      <table class="table table-bordered table-condensed table-striped">
+  {% endif %}
-        <thead>
-          <th>{% trans "Location" %}</th>
-          <th>{% trans "Name" %}</th>
-        </thead>
-        <tbody>
-          <tr>
-            <td>{{ label }}</td>
-            <td>{{ name }}</td>
-          </tr>
-        </tbody>
-      </table>
-    </div>
-  </div>
 
   <p>
     <form class="form" method="post">
@@ -49,10 +36,15 @@
 
       {{ form|bootstrap }}
 
-      <input type="submit" class="btn btn-danger"
+      <input type="submit" class="btn btn-primary"
              value="{% blocktrans trimmed %}
-                      Restore data from {{ name }}
+                      Restore
                     {% endblocktrans %}"/>
+      <a title="{% trans 'Abort' %}"
+         role="button" class="btn btn-warning"
+         href="{% url 'backups:index' %}">
+        {% trans 'Abort' %}
+      </a>
     </form>
   </p>
 

plinth/modules/backups/templates/backups_restore_from_tmp.html

@@ -1,45 +0,0 @@
-{% extends "base.html" %}
-{% comment %}
-#
-# This file is part of FreedomBox.
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as
-# published by the Free Software Foundation, either version 3 of the
-# License, or (at your option) any later version.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Affero General Public License for more details.
-#
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
-{% endcomment %}
-
-{% load bootstrap %}
-{% load i18n %}
-
-{% block content %}
-  <h2>{{ title }}</h2>
-
-  <p>
-    <form class="form" method="post">
-      {% csrf_token %}
-
-      {{ form|bootstrap }}
-
-      <input type="submit" class="btn btn-primary"
-             value="{% blocktrans trimmed %}
-                      Restore data
-                    {% endblocktrans %}"/>
-      <a title="{% trans 'Abort' %}"
-         role="button" class="btn btn-warning"
-         href="{% url 'backups:index' %}">
-        {% trans 'Abort' %}
-      </a>
-    </form>
-  </p>
-
-{% endblock %}

plinth/modules/backups/tests/test_api.py

@@ -24,9 +24,8 @@ from unittest.mock import MagicMock, call, patch
 from django.core.files.uploadedfile import SimpleUploadedFile
 
 from plinth import cfg, module_loader
-from plinth.errors import PlinthError
 
-from .. import api, forms, get_location_path
+from .. import api, forms
 
 # pylint: disable=protected-access
 
@@ -244,35 +243,16 @@ class TestBackupProcesses(unittest.TestCase):
 class TestBackupModule(unittest.TestCase):
     """Tests of the backups django module, like views or forms."""
 
-    def test_get_location_path(self):
-        """Test the 'get_location_path' method"""
-        locations = [{
-            'path': '/var/www',
-            'device': '/dummy/device'
-        }, {
-            'path': '/etc',
-            'device': '/dangerous'
-        }]
-        location_path = get_location_path('/dummy/device', locations)
-        self.assertEqual(location_path, locations[0]['path'])
-        # verify that an unknown location raises an error
-        with self.assertRaises(PlinthError):
-            get_location_path('/unknown/device', locations)
-
     def test_file_upload(self):
-        locations = get_export_locations()
-        location_name = locations[0]['device']
-        post_data = {'location': location_name}
-
         # posting a video should fail
         video_file = SimpleUploadedFile("video.mp4", b"file_content",
                                         content_type="video/mp4")
-        form = forms.UploadForm(post_data, {'file': video_file})
+        form = forms.UploadForm({}, {'file': video_file})
         self.assertFalse(form.is_valid())
 
         # posting an archive file should work
         archive_file = SimpleUploadedFile("backup.tar.gz", b"file_content",
                                           content_type="application/gzip")
-        form = forms.UploadForm(post_data, {'file': archive_file})
+        form = forms.UploadForm({}, {'file': archive_file})
         form.is_valid()
         self.assertTrue(form.is_valid())

plinth/modules/backups/urls.py

@@ -22,7 +22,7 @@ from django.conf.urls import url
 
 from .views import IndexView, CreateArchiveView, DeleteArchiveView, \
         UploadArchiveView, ExportAndDownloadView, RestoreArchiveView, \
-        RestoreFromTmpView
+        RestoreFromUploadView
 
 urlpatterns = [
     url(r'^sys/backups/$', IndexView.as_view(), name='index'),
@@ -34,6 +34,6 @@ urlpatterns = [
     url(r'^sys/backups/upload/$', UploadArchiveView.as_view(), name='upload'),
     url(r'^sys/backups/restore-archive/(?P<name>[^/]+)/$',
         RestoreArchiveView.as_view(), name='restore-archive'),
-    url(r'^sys/backups/restore-from-tmp/$',
+    url(r'^sys/backups/restore-from-upload/$',
-        RestoreFromTmpView.as_view(), name='restore-from-tmp'),
+        RestoreFromUploadView.as_view(), name='restore-from-upload'),
 ]

plinth/modules/backups/views.py

@@ -35,8 +35,8 @@ from django.views.generic import View, FormView, TemplateView
 
 from plinth.modules import backups
 
-from . import api, TMP_BACKUP_PATH, forms, \
+from . import api, UPLOAD_BACKUP_PATH, forms, \
-        SESSION_BACKUP_VARIABLE, delete_tmp_backup_file
+        SESSION_BACKUP_VARIABLE, delete_upload_backup_file
 
 # number of seconds an uploaded backup file should be kept/stored
 KEEP_UPLOADED_BACKUP_FOR = 60*10
@@ -145,32 +145,32 @@ class create_temporary_backup_file:
 
     def __init__(self, name):
         self.name = name
-        self.path = TMP_BACKUP_PATH
+        self.path = UPLOAD_BACKUP_PATH
 
     def __enter__(self):
         backups.export_archive(self.name, self.path)
         return self.path
 
     def __exit__(self, type, value, traceback):
-        delete_tmp_backup_file()
+        delete_upload_backup_file()
 
 
 class UploadArchiveView(SuccessMessageMixin, FormView):
-    form_class = forms.UploadToTmpForm
+    form_class = forms.UploadForm
     prefix = 'backups'
     template_name = 'backups_upload.html'
-    success_url = reverse_lazy('backups:restore-from-tmp')
+    success_url = reverse_lazy('backups:restore-from-upload')
 
     def get_context_data(self, **kwargs):
         """Return additional context for rendering the template."""
         context = super().get_context_data(**kwargs)
-        context['title'] = _('Upload and import a backup file')
+        context['title'] = _('Upload and restore a backup file')
         context['subsubmenu'] = subsubmenu
         return context
 
     def form_valid(self, form):
         """store uploaded file."""
-        with open(TMP_BACKUP_PATH, 'wb+') as destination:
+        with open(UPLOAD_BACKUP_PATH, 'wb+') as destination:
             for chunk in self.request.FILES['backups-file'].chunks():
                 destination.write(chunk)
         self.request.session[SESSION_BACKUP_VARIABLE] = time.time() + \
@@ -178,51 +178,7 @@ class UploadArchiveView(SuccessMessageMixin, FormView):
         return super().form_valid(form)
 
 
-class RestoreFromTmpView(SuccessMessageMixin, FormView):
+class BaseRestoreView(SuccessMessageMixin, FormView):
-    """View to restore files from an exported archive.
-    
-    TODO: combine with RestoreView"""
-    # TODO: display more information about the backup, like the date
-    form_class = forms.RestoreFromTmpForm
-    prefix = 'backups'
-    template_name = 'backups_restore_from_tmp.html'
-    success_url = reverse_lazy('backups:index')
-    success_message = _('Restored files from backup.')
-
-    def get(self, *args, **kwargs):
-        if not os.path.isfile(TMP_BACKUP_PATH):
-            messages.error(self.request, _('No backup file found.'))
-            return redirect(reverse_lazy('backups:index'))
-        else:
-            return super().get(*args, **kwargs)
-
-    def _get_included_apps(self):
-        """Save some data used to instantiate the form."""
-        return backups.get_apps_of_exported_archive(TMP_BACKUP_PATH)
-
-    def get_form_kwargs(self):
-        """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
-        ]
-        return kwargs
-
-    def get_context_data(self, **kwargs):
-        """Return additional context for rendering the template."""
-        context = super().get_context_data(**kwargs)
-        context['title'] = _('Restore data')
-        return context
-
-    def form_valid(self, form):
-        """Restore files from the archive on valid form submission."""
-        backups.restore_from_tmp(form.cleaned_data['selected_apps'])
-        return super().form_valid(form)
-
-
-class RestoreArchiveView(SuccessMessageMixin, FormView):
     """View to restore files from an archive."""
     form_class = forms.RestoreForm
     prefix = 'backups'
@@ -230,12 +186,6 @@ class RestoreArchiveView(SuccessMessageMixin, FormView):
     success_url = reverse_lazy('backups:index')
     success_message = _('Restored files from backup.')
 
-    def _get_included_apps(self):
-        """Save some data used to instantiate the form."""
-        name = unquote(self.kwargs['name'])
-        archive_path = backups.get_archive_path(name)
-        return backups.get_archive_apps(archive_path)
-
     def get_form_kwargs(self):
         """Pass additional keyword args for instantiating the form."""
         kwargs = super().get_form_kwargs()
@@ -249,10 +199,46 @@ class RestoreArchiveView(SuccessMessageMixin, FormView):
     def get_context_data(self, **kwargs):
         """Return additional context for rendering the template."""
         context = super().get_context_data(**kwargs)
-        context['title'] = _('Restore from backup')
+        context['title'] = _('Restore')
-        context['name'] = self.kwargs['name']
+        context['name'] = self.kwargs.get('name', None)
         return context
 
+
+class RestoreFromUploadView(BaseRestoreView):
+    """View to restore files from an (uploaded) exported archive."""
+
+    def get(self, *args, **kwargs):
+        if not os.path.isfile(UPLOAD_BACKUP_PATH):
+            messages.error(self.request, _('No backup file found.'))
+            return redirect(reverse_lazy('backups:index'))
+        else:
+            return super().get(*args, **kwargs)
+
+    def get_context_data(self, **kwargs):
+        """Return additional context for rendering the template."""
+        context = super().get_context_data(**kwargs)
+        context['title'] = _('Restore from uploaded file')
+        return context
+
+    def _get_included_apps(self):
+        """Save some data used to instantiate the form."""
+        return backups.get_exported_archive_apps(UPLOAD_BACKUP_PATH)
+
+    def form_valid(self, form):
+        """Restore files from the archive on valid form submission."""
+        backups.restore_from_upload(form.cleaned_data['selected_apps'])
+        return super().form_valid(form)
+
+
+class RestoreArchiveView(BaseRestoreView):
+    """View to restore files from an archive."""
+
+    def _get_included_apps(self):
+        """Save some data used to instantiate the form."""
+        name = unquote(self.kwargs['name'])
+        archive_path = backups.get_archive_path(name)
+        return backups.get_archive_apps(archive_path)
+
     def form_valid(self, form):
         """Restore files from the archive on valid form submission."""
         archive_path = backups.get_archive_path(self.kwargs['name'])