From b12d994760e552a1e4d7a8922f0792406c68e458 Mon Sep 17 00:00:00 2001
From: Sunil Mohan Adapa <sunil@medhas.org>
Date: Tue, 8 Sep 2020 15:27:48 -0700
Subject: [PATCH] upgrades: Separate concepts for backports enabled vs.
 requested

It is confusing to combine the user's intent of wanting to have backports
activated with whether they have actually been configured in the system.

- Separate out checking for requested which is a key in the kvstore from enabled
which is about checking system configuration for backports.

- Implement convenience method for setting whether user requested backports.

- Do not base the status display (in security and upgrades modules) on the
configuration status and instead focus on user intent.

  - If user requested backports but they have not been enabled yet due to not
  being available, show as activated. System will keep trying the background and
  configure eventually.

  - If user requested backports but their configuration is outdated yet due to
  newer release, show as activated. System will keep trying in the background
  and configure latest settings eventually.

- In all places where backports enabling is being checked, split the logic for
'can be activated' from 'already activated' and 'user requested activation'
properly.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: James Valleroy <jvalleroy@mailbox.org>
---
 .../modules/security/templates/security.html  |  2 +-
 plinth/modules/security/views.py              |  4 +--
 plinth/modules/upgrades/__init__.py           | 29 +++++++++++++------
 plinth/modules/upgrades/forms.py              |  2 +-
 .../templates/upgrades_configure.html         |  8 ++---
 plinth/modules/upgrades/views.py              | 29 +++++--------------
 6 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/plinth/modules/security/templates/security.html b/plinth/modules/security/templates/security.html
index 56bd907d1..80ecd8646 100644
--- a/plinth/modules/security/templates/security.html
+++ b/plinth/modules/security/templates/security.html
@@ -13,7 +13,7 @@
     {% trans "Show security report" %}
   </a>
 
-  {% if is_backports_enabled %}
+  {% if is_backports_requested %}
     <h3>{% trans "Frequent Feature Updates" %}</h3>
     <p>
       {% blocktrans trimmed %}
diff --git a/plinth/modules/security/views.py b/plinth/modules/security/views.py
index 65bcee4de..2270fb746 100644
--- a/plinth/modules/security/views.py
+++ b/plinth/modules/security/views.py
@@ -9,7 +9,7 @@ from django.utils.translation import ugettext as _
 
 from plinth import action_utils, actions
 from plinth.modules import security
-from plinth.modules.upgrades import is_backports_enabled
+from plinth.modules.upgrades import is_backports_requested
 
 from .forms import SecurityForm
 
@@ -33,7 +33,7 @@ def index(request):
         request, 'security.html', {
             'app_info': security.app.info,
             'form': form,
-            'is_backports_enabled': is_backports_enabled(),
+            'is_backports_requested': is_backports_requested(),
         })
 
 
diff --git a/plinth/modules/upgrades/__init__.py b/plinth/modules/upgrades/__init__.py
index f5808f162..4d733a361 100644
--- a/plinth/modules/upgrades/__init__.py
+++ b/plinth/modules/upgrades/__init__.py
@@ -3,6 +3,7 @@
 FreedomBox app for upgrades.
 """
 
+import logging
 import os
 import subprocess
 
@@ -42,10 +43,12 @@ _description = [
 
 app = None
 
-BACKPORTS_ENABLED_KEY = 'upgrades_backports_enabled'
+BACKPORTS_REQUESTED_KEY = 'upgrades_backports_requested'
 
 SOURCES_LIST = '/etc/apt/sources.list.d/freedombox2.list'
 
+logger = logging.getLogger(__name__)
+
 
 class UpgradesApp(app_module.App):
     """FreedomBox app for software upgrades."""
@@ -139,7 +142,7 @@ def disable():
 
 def setup_repositories(data):
     """Setup apt backport repositories."""
-    if is_backports_enabled():
+    if is_backports_requested():
         command = ['setup-repositories']
         if cfg.develop:
             command += ['--develop']
@@ -147,11 +150,22 @@ def setup_repositories(data):
         actions.superuser_run('upgrades', command)
 
 
-def is_backports_enabled():
-    """Return whether backports are enabled."""
+def is_backports_requested():
+    """Return whether user has chosen to activate backports."""
     from plinth import kvstore
-    return kvstore.get_default(BACKPORTS_ENABLED_KEY,
-                               os.path.exists(SOURCES_LIST))
+    return kvstore.get_default(BACKPORTS_REQUESTED_KEY, False)
+
+
+def set_backports_requested(requested):
+    """Set whether user has chosen to activate backports."""
+    from plinth import kvstore
+    kvstore.set(BACKPORTS_REQUESTED_KEY, requested)
+    logger.info('Backports requested - %s', requested)
+
+
+def is_backports_enabled():
+    """Return whether backports are enabled in the system configuration."""
+    return os.path.exists(SOURCES_LIST)
 
 
 def get_current_release():
@@ -180,9 +194,6 @@ def is_backports_current():
 
 def can_activate_backports():
     """Return whether backports can be activated."""
-    if is_backports_current():
-        return False
-
     release, _ = get_current_release()
     if release == 'unstable' or (release == 'testing' and not cfg.develop):
         return False
diff --git a/plinth/modules/upgrades/forms.py b/plinth/modules/upgrades/forms.py
index 5a92aa70c..db4263ef4 100644
--- a/plinth/modules/upgrades/forms.py
+++ b/plinth/modules/upgrades/forms.py
@@ -18,4 +18,4 @@ class BackportsFirstbootForm(forms.Form):
     """Form to configure backports during first boot wizard."""
     backports_enabled = forms.BooleanField(
         label=_('Activate frequent feature updates (recommended)'),
-        required=False)
+        required=False, initial=True)
diff --git a/plinth/modules/upgrades/templates/upgrades_configure.html b/plinth/modules/upgrades/templates/upgrades_configure.html
index 94262751c..f8c7ecf3b 100644
--- a/plinth/modules/upgrades/templates/upgrades_configure.html
+++ b/plinth/modules/upgrades/templates/upgrades_configure.html
@@ -10,12 +10,12 @@
 {% block extra_content %}
   <h3>{% trans "Frequent Feature Updates" %}</h3>
   <p>
-    {% if can_activate_backports %}
+    {% if can_activate_backports and not is_backports_requested %}
       {% blocktrans trimmed %}
         Frequent feature updates can be activated. Activating them is
         recommended.
       {% endblocktrans %}
-    {% elif is_backports_enabled %}
+    {% elif can_activate_backports and is_backports_requested %}
       {% blocktrans trimmed %}
         Frequent feature updates are enabled.
       {% endblocktrans %}
@@ -27,7 +27,7 @@
     {% endif %}
   </p>
   <p>
-    {% if can_activate_backports or is_backports_enabled %}
+    {% if can_activate_backports %}
       {% blocktrans trimmed %}
         Frequent feature updates allow a very limited set of software, including
         {{box_name}} service, to be updated to receive newer features regularly
@@ -38,7 +38,7 @@
       {% endblocktrans %}
     {% endif %}
   </p>
-  {% if can_activate_backports %}
+  {% if can_activate_backports and not is_backports_requested %}
     <div class="alert alert-warning" role="alert">
       {% url 'snapshot:index' as snapshot_url %}
       {% blocktrans trimmed %}
diff --git a/plinth/modules/upgrades/views.py b/plinth/modules/upgrades/views.py
index 43648bf28..443227c34 100644
--- a/plinth/modules/upgrades/views.py
+++ b/plinth/modules/upgrades/views.py
@@ -3,8 +3,6 @@
 FreedomBox app for upgrades.
 """
 
-import logging
-
 from django.contrib import messages
 from django.http import HttpResponseRedirect
 from django.shortcuts import redirect
@@ -12,15 +10,13 @@ from django.urls import reverse_lazy
 from django.utils.translation import ugettext as _
 from django.views.generic.edit import FormView
 
-from plinth import actions, kvstore, package
+from plinth import actions, package
 from plinth.errors import ActionError
 from plinth.modules import first_boot, upgrades
 from plinth.views import AppView
 
 from .forms import BackportsFirstbootForm, ConfigureForm
 
-logger = logging.getLogger(__name__)
-
 
 class UpgradesConfigurationView(AppView):
     """Serve configuration page."""
@@ -35,7 +31,7 @@ class UpgradesConfigurationView(AppView):
     def get_context_data(self, *args, **kwargs):
         context = super().get_context_data(*args, **kwargs)
         context['can_activate_backports'] = upgrades.can_activate_backports()
-        context['is_backports_enabled'] = upgrades.is_backports_enabled()
+        context['is_backports_requested'] = upgrades.is_backports_requested()
         context['is_busy'] = package.is_package_manager_busy()
         context['log'] = get_log()
         context['refresh_page_sec'] = 3 if context['is_busy'] else None
@@ -90,7 +86,7 @@ def upgrade(request):
 def activate_backports(request):
     """Activate backports."""
     if request.method == 'POST':
-        kvstore.set(upgrades.BACKPORTS_ENABLED_KEY, True)
+        upgrades.set_backports_requested(True)
         upgrades.setup_repositories(None)
         messages.success(request, _('Frequent feature updates activated.'))
 
@@ -107,24 +103,18 @@ class BackportsFirstbootView(FormView):
         if upgrades.is_backports_enabled():
             # Backports is already enabled. Record this preference and
             # skip first boot step.
-            kvstore.set(upgrades.BACKPORTS_ENABLED_KEY, True)
+            upgrades.set_backports_requested(True)
             first_boot.mark_step_done('backports_wizard')
             return HttpResponseRedirect(reverse_lazy(first_boot.next_step()))
 
         if not upgrades.can_activate_backports():
             # Skip first boot step.
+            upgrades.set_backports_requested(False)
             first_boot.mark_step_done('backports_wizard')
             return HttpResponseRedirect(reverse_lazy(first_boot.next_step()))
 
         return super().dispatch(request, *args, *kwargs)
 
-    def get_initial(self):
-        """Get initial form data."""
-        return {
-            'backports_enabled':
-                kvstore.get_default(upgrades.BACKPORTS_ENABLED_KEY, True)
-        }
-
     def get_success_url(self):
         """Return next firstboot step."""
         return reverse_lazy(first_boot.next_step())
@@ -132,12 +122,7 @@ class BackportsFirstbootView(FormView):
     def form_valid(self, form):
         """Mark the first wizard step as done, save value and redirect."""
         enabled = form.cleaned_data['backports_enabled']
-        kvstore.set(upgrades.BACKPORTS_ENABLED_KEY, enabled)
-        if enabled:
-            upgrades.setup_repositories(None)
-            logger.info('Backports enabled.')
-        else:
-            logger.info('Backports not enabled.')
-
+        upgrades.set_backports_requested(enabled)
+        upgrades.setup_repositories(None)
         first_boot.mark_step_done('backports_wizard')
         return super().form_valid(form)