From 8b7c0fb0bfc2b058ba54999a835e718f8fe1f37b Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Fri, 28 Apr 2023 11:55:09 -0700 Subject: [PATCH] uninstall: Fix issue with uninstall of apps that have no backup Closes: #2342. - Define a helper method to figure if app has backup. Use the helper method in main AppView. - Minor refactor of repeated code in uninstall view that gets the app and app_id. - Pass whether an app has backup or not into form. Delete backup related form fields when backup is not supported. UX when fields are disabled is not nice, it is not explained why fields are disabled. Better UX seems to be to remove the backup fields entirely. Tests: - Install wireguard. In the uninstall form backup fields don't appear. Uninstall is successful. - Install Bepasty. In the uninstall form back fields are shown. Uninstall is successful with and without a backup. In case backup is chosen, backup is created and can successfully be restored. - Run functional tests for bepasty. For wireguard uninstall test succeeds. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/forms.py | 8 ++++++++ plinth/views.py | 52 +++++++++++++++++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/plinth/forms.py b/plinth/forms.py index 504747110..269d233dc 100644 --- a/plinth/forms.py +++ b/plinth/forms.py @@ -39,6 +39,14 @@ class UninstallForm(forms.Form): repository = forms.ChoiceField(label=_('Repository to backup to'), choices=_get_repository_choices) + def __init__(self, **kwargs): + """Determine if backup fields must be used.""" + has_backup_restore = kwargs.pop('has_backup_restore', True) + super().__init__(**kwargs) + if not has_backup_restore: + del self.fields['should_backup'] + del self.fields['repository'] + class DomainSelectionForm(forms.Form): """Form for selecting a domain name to be used for diff --git a/plinth/views.py b/plinth/views.py index e103dd08f..6d6e295de 100644 --- a/plinth/views.py +++ b/plinth/views.py @@ -66,6 +66,15 @@ def _get_redirect_url_from_param(request): return reverse('index') +def _has_backup_restore(app): + """Return whether an app implements backup/restore.""" + from plinth.modules.backups.components import BackupRestore + return any([ + component.has_data + for component in app.get_components_of_type(BackupRestore) + ]) + + @public def index(request): """Serve the main index page.""" @@ -272,11 +281,7 @@ class AppView(FormView): from plinth.modules.firewall.components import Firewall context['firewall'] = self.app.get_components_of_type(Firewall) - from plinth.modules.backups.components import BackupRestore - context['has_backup_restore'] = any([ - component.has_data - for component in self.app.get_components_of_type(BackupRestore) - ]) + context['has_backup_restore'] = _has_backup_restore(self.app) return context @@ -400,21 +405,35 @@ class UninstallView(FormView): form_class = forms.UninstallForm template_name = 'uninstall.html' + def __init__(self, **kwargs): + """Initialize extra instance members.""" + super().__init__(**kwargs) + self.app = None + + def setup(self, request, *args, **kwargs): + """Initialize attributes shared by all view methods.""" + super().setup(request, *args, **kwargs) + app_id = self.kwargs['app_id'] + self.app = app_module.App.get(app_id) + self.has_backup_restore = _has_backup_restore(self.app) + def dispatch(self, request, *args, **kwargs): """Don't allow the view to be used on essential apps.""" - app_id = self.kwargs['app_id'] - app = app_module.App.get(app_id) - if app.info.is_essential: + if self.app.info.is_essential: raise Http404 return super().dispatch(request, *args, **kwargs) + def get_form_kwargs(self): + """Return the keyword arguments for instantiating the form.""" + kwargs = super().get_form_kwargs() + kwargs['has_backup_restore'] = self.has_backup_restore + return kwargs + def get_context_data(self, *args, **kwargs): """Add app information to the context data.""" context = super().get_context_data(*args, **kwargs) - app_id = self.kwargs['app_id'] - app = app_module.App.get(app_id) - context['app_info'] = app.info + context['app_info'] = self.app.info return context def get_success_url(self): @@ -423,10 +442,8 @@ class UninstallView(FormView): def form_valid(self, form): """Uninstall the app.""" - app_id = self.kwargs['app_id'] - # Backup the app - if form.cleaned_data['should_backup']: + if self.has_backup_restore and form.cleaned_data['should_backup']: repository_id = form.cleaned_data['repository'] import plinth.modules.backups.repository as repository_module @@ -436,11 +453,12 @@ class UninstallView(FormView): name = datetime.datetime.now().strftime( '%Y-%m-%d:%H:%M:%S') + ' ' + str( - _('before uninstall of {app_id}')).format(app_id=app_id) - repository.create_archive(name, [app_id]) + _('before uninstall of {app_id}')).format( + app_id=self.app.app_id) + repository.create_archive(name, [self.app.app_id]) # Uninstall - setup.run_uninstall_on_app(app_id) + setup.run_uninstall_on_app(self.app.app_id) return super().form_valid(form)