apps: Fix regression in enabling install button

Signed-off-by: Joseph Nuthalapati <njoseph@riseup.net>

plinth/app.py

@@ -140,6 +140,14 @@ class App:
         """
         return self.get_component(self.app_id + '-info')
 
+    def is_available(self) -> bool:
+        """Return whether the app is available to install."""
+        for component in self.components.values():
+            if not component.is_available():
+                return False
+
+        return True
+
     def setup(self, old_version):
         """Install and configure the app and its components."""
         for component in self.components.values():
@@ -330,6 +338,10 @@ class Component:
         """
         return App.get(self.app_id)
 
+    def is_available(self) -> bool:
+        """Return whether the app is available to install."""
+        return True
+
     def setup(self, old_version):
         """Run operations to install and configure the component."""
 

plinth/modules/deluge/__init__.py

@@ -34,11 +34,11 @@ class DelugePackages(Packages):
     https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1031593
     """
 
-    def has_unavailable_packages(self) -> bool | None:
+    def is_available(self) -> bool:
         if get_current_release()[1] == 'bookworm':
-            return True
+            return False
 
-        return super().has_unavailable_packages()
+        return super().is_available()
 
 
 class DelugeApp(app_module.App):

plinth/package.py

@@ -249,27 +249,38 @@ class Packages(app_module.FollowerComponent):
 
         return packages_installed(self.conflicts)
 
-    def has_unavailable_packages(self) -> bool | None:
-        """Return whether any of the packages are not available.
+    def is_available(self) -> bool:
+        """Return whether all of the packages are available.
 
-        Returns True if one or more of the packages is not available in the
-        user's Debian distribution or False otherwise. Returns None if it
-        cannot be reliably determined whether the packages are available or
-        not.
+        Returns True if all of the packages are available in the user's Debian
+        distribution or False otherwise. Returns True if it cannot be reliably
+        determined whether the packages are available or not.
+
+        This operation can run for a long time as package cache may be
+        refreshed.
         """
-        apt_lists_dir = pathlib.Path('/var/lib/apt/lists/')
-        num_files = len(
-            [child for child in apt_lists_dir.iterdir() if child.is_file()])
-        if num_files < 2:  # not counting the lock file
-            return None
+        try:
+            self.get_actual_packages()
+            return True  # All packages were found in the cache
+        except MissingPackageError:
+            pass  # We will retry after refreshing package list
 
-        # List of all packages from all Package components
+        # If the package cache is new, then package is really not available.
+        package_cache = pathlib.Path('/var/cache/apt/pkgcache.bin')
+        if (package_cache.exists()
+                and time.time() - package_cache.stat().st_mtime < 3600):
+            return False
+
+        # Perform 'apt-get update'
+        refresh_package_lists()
+
+        # Check again for all the packages
         try:
             self.get_actual_packages()
         except MissingPackageError:
-            return True
+            return False
 
-        return False
+        return True
 
     def _filter_packages_to_keep(self, packages: list[str]) -> list[str]:
         """Filter out the list of packages to keep from given list.

plinth/templates/setup.html

@@ -33,25 +33,36 @@
       {% endif %}
     </p>
 
+    <div class="app-unavailable d-none">
+      <div class="alert alert-warning d-flex align-items-center" role="alert">
+        <div class="me-2">
+          <span class="fa fa-exclamation-triangle" aria-hidden="true"></span>
+          <span class="visually-hidden">{% trans "Caution:" %}</span>
+        </div>
+        <div>
+          {% blocktrans trimmed %}
+            This application is currently not available in your distribution.
+          {% endblocktrans %}
+        </div>
+      </div>
+    </div>
+    <p class="app-checking-availability d-none" data-app-id="{{ app_id }}"
+       data-setup-state="{{ setup_state.value }}">
+      {% blocktrans trimmed %}
+        Checking app availability...
+      {% endblocktrans %}
+      <span class="spinner-border spinner-border-sm" role="status"></span>
+    </p>
+    <p class="app-checking-availability-error d-none">
+      {% blocktrans trimmed %}
+        Error checking app availability. Please refresh page.
+      {% endblocktrans %}
+    </p>
+
     <form class="form-install" action="" method="post">
       {% csrf_token %}
 
-      {% if has_unavailable_packages %}
-        <div class="alert alert-warning d-flex align-items-center" role="alert">
-          <div class="me-2">
-            <span class="fa fa-exclamation-triangle" aria-hidden="true"></span>
-            <span class="visually-hidden">{% trans "Caution:" %}</span>
-          </div>
-          <div>
-            {% blocktrans trimmed %}
-              This application is currently not available in your distribution.
-            {% endblocktrans %}
-            <button type="submit" class="btn btn-default btn-sm" name="refresh-packages">
-              <span class="fa fa-refresh"></span> {% trans "Check again" %}
-            </button>
-          </div>
-        </div>
-      {% elif package_conflicts and package_conflicts_action.value != 'ignore' %}
+      {% if package_conflicts and package_conflicts_action.value != 'ignore' %}
         <div class="alert alert-warning d-flex align-items-center" role="alert">
           <div class="me-2">
             <span class="fa fa-exclamation-triangle" aria-hidden="true"></span>
@@ -70,10 +81,8 @@
         </div>
       {% endif %}
 
-      <input type="submit" class="btn btn-md btn-primary" name="install"
-             {% if has_unavailable_packages %}
-             disabled="disabled"
-             {% endif %}
+      <input type="submit" class="btn btn-md btn-primary install-button"
+             name="install"
              {% if setup_state.value == 'needs-setup' %}
              value="{% trans "Install" %}"
              {% elif setup_state.value == 'needs-update' %}

plinth/tests/test_app.py

@@ -132,6 +132,20 @@ def test_get_components_of_type(app_with_components):
     assert list(components) == leader_components
 
 
+def test_app_is_available(app_with_components):
+    """Test checking if an app is available for setup."""
+    for component in app_with_components.components.values():
+        component.is_available = Mock(return_value=True)
+
+    assert app_with_components.is_available()
+    for component in app_with_components.components.values():
+        component.is_available.assert_has_calls([call()])
+
+    component = list(app_with_components.components.values())[0]
+    component.is_available.return_value = False
+    assert not app_with_components.is_available()
+
+
 def test_app_setup(app_with_components):
     """Test that running setup on an app runs setup on components."""
     for component in app_with_components.components.values():
@@ -333,6 +347,12 @@ def test_component_app_property():
     assert component.app == app
 
 
+def test_component_is_available():
+    """Test checking if a component is available for setup."""
+    component = Component('test-component')
+    assert component.is_available()
+
+
 def test_component_setup():
     """Test running setup on component."""
     component = Component('test-component')

plinth/tests/test_package.py

@@ -3,6 +3,7 @@
 Test module for package module.
 """
 
+import time
 import unittest
 from unittest.mock import Mock, call, patch
 
@@ -338,23 +339,46 @@ def test_packages_find_conflicts(packages_installed_):
     assert component.find_conflicts() == ['package1', 'package2']
 
 
+@patch('plinth.package.refresh_package_lists')
 @patch('apt.Cache')
 @patch('pathlib.Path')
-def test_packages_has_unavailable_packages(path_class, cache):
-    """Test checking for unavailable packages."""
+def test_packages_is_available(path_class, cache, refresh_package_lists):
+    """Test checking for available packages."""
     path = Mock()
     path_class.return_value = path
-    path.iterdir.return_value = [Mock()]
 
+    # Packages found in cache
     component = Packages('test-component', ['package1', 'package2'])
-    assert component.has_unavailable_packages() is None
-
-    path.iterdir.return_value = [Mock(), Mock()]
     cache.return_value = ['package1', 'package2']
-    assert not component.has_unavailable_packages()
+    assert component.is_available()
+    path_class.assert_not_called()
+    refresh_package_lists.assert_not_called()
 
+    # Packages not found, cache exists and is fresh
     cache.return_value = ['package1']
-    assert component.has_unavailable_packages()
+    path.exists.return_value = True
+    path.stat.return_value.st_mtime = time.time()
+    assert not component.is_available()
+    refresh_package_lists.assert_not_called()
+
+    # Packages not found, cache does not exist
+    cache.return_value = ['package1']
+    path.exists.return_value = False
+    assert not component.is_available()
+    refresh_package_lists.assert_called_once()
+
+    # Packages not found, cache is stale
+    cache.return_value = ['package1']
+    refresh_package_lists.reset_mock()
+    path.exists.return_value = True
+    path.stat.return_value.st_mtime = time.time() - 7200
+    assert not component.is_available()
+    refresh_package_lists.assert_called_once()
+
+    # Packages not found, cache is stale, but packages found after refresh
+    cache.side_effect = [['package1'], ['package1', 'package2']]
+    refresh_package_lists.reset_mock()
+    assert component.is_available()
 
 
 def test_packages_installed():

plinth/urls.py

@@ -26,6 +26,8 @@ urlpatterns = [
     re_path(r'', include((system_urlpatterns, 'system'))),
     re_path(r'^uninstall/(?P<app_id>[1-9a-z\-_]+)/$',
             views.UninstallView.as_view(), name='uninstall'),
+    re_path(r'^is-available/(?P<app_id>[1-9a-z\-_]+)/$',
+            views.is_available_view, name='is-available'),
     re_path(r'^rerun-setup/(?P<app_id>[1-9a-z\-_]+)/$', views.rerun_setup_view,
             name='rerun-setup'),
 

plinth/views.py

@@ -12,7 +12,7 @@ from django.contrib import messages
 from django.core.exceptions import ImproperlyConfigured
 from django.forms import Form
 from django.http import (Http404, HttpRequest, HttpResponseBadRequest,
-                         HttpResponseRedirect)
+                         HttpResponseRedirect, JsonResponse)
 from django.shortcuts import redirect
 from django.template.response import TemplateResponse
 from django.urls import reverse
@@ -513,12 +513,6 @@ class SetupView(TemplateView):
         context['show_uninstall'] = (not app.info.is_essential and setup_state
                                      != app_module.App.SetupState.NEEDS_SETUP)
 
-        # Perform expensive operation only if needed.
-        if not context['operations']:
-            context[
-                'has_unavailable_packages'] = self._has_unavailable_packages(
-                    app)
-
         context['refresh_page_sec'] = None
         if context['setup_state'] == app_module.App.SetupState.UP_TO_DATE:
             context['refresh_page_sec'] = 0
@@ -565,12 +559,16 @@ class SetupView(TemplateView):
 
         return conflicts, conflicts_action
 
-    @staticmethod
-    def _has_unavailable_packages(app_):
-        """Return whether the app has unavailable packages."""
-        components = app_.get_components_of_type(Packages)
-        return any(component for component in components
-                   if component.has_unavailable_packages())
+
+def is_available_view(request, app_id):
+    """Return whether an app is available.
+
+    This check may take quite some time, so don't perform this check when
+    loading the app's setup page.
+    """
+    app = app_module.App.get(app_id)
+    data = {'is_available': app.is_available()}
+    return JsonResponse(data)
 
 
 @require_POST

pyproject.toml

@@ -142,6 +142,7 @@ markers = [
     "apps",
     "domain",
     "system",
+    "tags",
 ]
 
 # Useful when pylint is invoked separately instead of flake8

static/themes/default/js/main.js

@@ -27,7 +27,7 @@
  * create different rules when Javascript is available and when it is not. This
  * functionality was provided by the Modernizr library earlier.
  */
-document.addEventListener('DOMContentLoaded', function(event) {
+document.addEventListener('DOMContentLoaded', function (event) {
     const html = document.querySelector('html');
     html.classList.remove('no-js');
     html.classList.add('js');
@@ -36,7 +36,7 @@ document.addEventListener('DOMContentLoaded', function(event) {
 /*
  * Refresh page if marked for refresh.
  */
-document.addEventListener('DOMContentLoaded', function() {
+document.addEventListener('DOMContentLoaded', function () {
     const body = document.querySelector('body');
     if (body.hasAttribute('data-refresh-page-sec')) {
         let seconds = body.getAttribute('data-refresh-page-sec');
@@ -54,7 +54,7 @@ document.addEventListener('DOMContentLoaded', function() {
 /*
  * Return all submit buttons on the page
  */
-function getSubmitButtons(){
+function getSubmitButtons() {
     return document.querySelectorAll(
         "form input[type='submit'], form button[type='submit'].toggle-button");
 }
@@ -81,12 +81,12 @@ function onSubmitAddProgress(event) {
     // for the next event loop run which will happen after current event is
     // processed.
     window.setTimeout(() => {
-        if (button.tagName == "INPUT"){
+        if (button.tagName == "INPUT") {
             // For push buttons
             const beforeElement = document.createElement('div');
             beforeElement.classList.add('running-status-button-before');
             button.parentNode.insertBefore(beforeElement, button);
-        } else if (button.tagName == "BUTTON"){
+        } else if (button.tagName == "BUTTON") {
             // For toggle buttons
             button.classList.toggle('toggle-button--toggled');
         }
@@ -96,8 +96,8 @@ function onSubmitAddProgress(event) {
         // Disable all form submit buttons on the page
         for (const formbutton of getSubmitButtons()) {
             if (!(formbutton.classList.contains('btn-link') ||
-                  formbutton.classList.contains('no-running-status') ||
-                  formbutton.hasAttribute('disabled'))) {
+                formbutton.classList.contains('no-running-status') ||
+                formbutton.hasAttribute('disabled'))) {
                 formbutton.classList.add('temporarily-disabled');
                 formbutton.setAttribute('disabled', 'disabled');
             }
@@ -105,7 +105,7 @@ function onSubmitAddProgress(event) {
     }, 0);
 }
 
-document.addEventListener('DOMContentLoaded', function(event) {
+document.addEventListener('DOMContentLoaded', function (event) {
     for (const button of getSubmitButtons()) {
         // Don't listen for 'click' event on buttons as they are triggered
         // even when the form is invalid.
@@ -141,7 +141,7 @@ window.addEventListener('pageshow', clearButtonDisabling);
 /*
  * Select all option for multiple checkboxes.
  */
-document.addEventListener('DOMContentLoaded', function(event) {
+document.addEventListener('DOMContentLoaded', function (event) {
     // Django < 4.0 generates <ul> and <li> where as Django >= 4.0 generates <div>s
     let parents = document.querySelectorAll('ul.has-select-all,div.has-select-all');
     for (const parent of parents) {
@@ -206,3 +206,60 @@ function setSelectAllValue(parent) {
 
     parent.querySelector('.select-all').checked = enableSelectAll;
 }
+
+/*
+ * Check whether an app is available on its setup page.
+ */
+document.addEventListener('DOMContentLoaded', async () => {
+    const checkingElement = document.querySelector('.app-checking-availability');
+    if (!checkingElement)
+        return;
+
+    // App does not need setup, it likely needs upgrade
+    const setupState = checkingElement.getAttribute('data-setup-state');
+    if (setupState !== 'needs-setup')
+        return;
+
+    const appId = checkingElement.getAttribute('data-app-id');
+    checkingElement.classList.remove('d-none');
+
+    function setInstallButtonState(enable) {
+        const installButton = document.querySelector('.install-button');
+        if (enable)
+            installButton?.removeAttribute('disabled')
+        else
+            installButton?.setAttribute('disabled', 'disabled');
+    }
+
+    function error() {
+        const element = document.querySelector('.app-checking-availability-error');
+        element.classList.remove('d-none');
+        checkingElement.classList.add('d-none');
+        setInstallButtonState(true); // Allow trying installation
+    }
+
+    try {
+        setInstallButtonState(false);
+        const response = await fetch(`/plinth/is-available/${appId}/`, {
+            timeout: 2 * 60 * 1000  // 2 minutes
+        });
+
+        checkingElement.classList.add('d-none');
+
+        if (response.ok) {
+            const data = await response.json();
+            if (data.is_available === true) {
+                setInstallButtonState(true);
+            } else if (data.is_available === false) {
+                document.querySelector('.app-unavailable').classList.remove('d-none');
+                setInstallButtonState(false);
+            } else {
+                error();
+            }
+        } else {
+            error();
+        }
+    } catch {
+        error();
+    }
+});