diff --git a/debian/copyright b/debian/copyright index 6e8cf2899..c6cbdcb28 100644 --- a/debian/copyright +++ b/debian/copyright @@ -132,8 +132,8 @@ Files: plinth/modules/janus/static/icons/janus.png Copyright: 2014-2022 Meetecho License: GPL-3 with OpenSSL exception -Files: static/themes/default/icons/kiwix.png - static/themes/default/icons/kiwix.svg +Files: plinth/modules/kiwix/static/icons/kiwix.png + plinth/modules/kiwix/static/icons/kiwix.svg Copyright: 2020 The other Kiwix guy Comment: https://commons.wikimedia.org/wiki/File:Kiwix_logo_v3.svg License: CC-BY-SA-4.0 diff --git a/plinth/modules/kiwix/__init__.py b/plinth/modules/kiwix/__init__.py index 7243ddcb2..af21083f1 100644 --- a/plinth/modules/kiwix/__init__.py +++ b/plinth/modules/kiwix/__init__.py @@ -5,13 +5,14 @@ FreedomBox app for Kiwix content server. from django.utils.translation import gettext_lazy as _ -from plinth import app as app_module, frontpage, menu, package +from plinth import app as app_module +from plinth import frontpage, menu, package from plinth.config import DropinConfigs from plinth.daemon import Daemon from plinth.modules.apache.components import Webserver from plinth.modules.backups.components import BackupRestore -from plinth.modules.kiwix import manifest -from plinth.modules.firewall.components import Firewall, FirewallLocalProtection +from plinth.modules.firewall.components import (Firewall, + FirewallLocalProtection) from plinth.modules.users.components import UsersAndGroups from . import manifest, privileged @@ -28,11 +29,13 @@ _description = [
  • Educational materials: PHET, TED Ed, Vikidia
  • eBooks: Project Gutenberg
  • Magazines: Low-tech Magazine
    • - ''') + '''), + _('You can download content packages from the Kiwix ' + 'project or create your own.'), ] -SYSTEM_USER = 'kiwix' - class KiwixApp(app_module.App): """FreedomBox app for Kiwix.""" @@ -116,6 +119,6 @@ class KiwixApp(app_module.App): def validate_file_name(file_name: str): - """Check if the content archive file has a valid extension.""" - if not file_name.endswith(".zim"): - raise ValueError(f"Expected a ZIM file. Found {file_name}") + """Check if the content package file has a valid extension.""" + if not file_name.endswith('.zim'): + raise ValueError(f'Expected a ZIM file. Found {file_name}') diff --git a/plinth/modules/kiwix/data/usr/lib/systemd/system/kiwix-server-freedombox.service b/plinth/modules/kiwix/data/usr/lib/systemd/system/kiwix-server-freedombox.service index 7abe49c60..a1ceaa2e9 100644 --- a/plinth/modules/kiwix/data/usr/lib/systemd/system/kiwix-server-freedombox.service +++ b/plinth/modules/kiwix/data/usr/lib/systemd/system/kiwix-server-freedombox.service @@ -13,7 +13,7 @@ Environment=HOME="/var/lib/kiwix-server-freedombox" Environment=LIBRARY_PATH="/var/lib/kiwix-server-freedombox/library_zim.xml" Environment=ARGS="--library --port=4201 --urlRootLocation=kiwix" ExecStartPre=sh -e -c "mkdir -p $HOME/content; library=$$(ls ${LIBRARY_PATH} 2>/dev/null || true); [ \"x$${library}\" = \"x\" ] && (mkdir -p \"${HOME}\" && echo '\n\n' > \"${LIBRARY_PATH}\") || true" -ExecStart=sh -e -c "exec /usr/bin/kiwix-serve $ARGS $LIBRARY_PATH" +ExecStart=/usr/bin/kiwix-serve $ARGS $LIBRARY_PATH Restart=on-failure ExecReload=/bin/kill -HUP $MAINPID DynamicUser=yes diff --git a/plinth/modules/kiwix/forms.py b/plinth/modules/kiwix/forms.py index c926c36ca..d41629d24 100644 --- a/plinth/modules/kiwix/forms.py +++ b/plinth/modules/kiwix/forms.py @@ -8,18 +8,21 @@ from django.core import validators from django.utils.translation import gettext_lazy as _ from plinth import cfg +from plinth.utils import format_lazy from .privileged import KIWIX_HOME -class AddContentForm(forms.Form): - """Form to create an empty library.""" +class AddPackageForm(forms.Form): + """Form to upload a content package to a library.""" # Would be nice to have a progress bar when uploading large files. file = forms.FileField( label=_('Upload File'), required=True, validators=[ validators.FileExtensionValidator( ['zim'], _('Content packages have to be in .zim format')) - ], help_text=_(f'''Uploaded ZIM files will be stored under - {KIWIX_HOME}/content on your {cfg.box_name}. If Kiwix fails to add the file, - it will be deleted immediately to save disk space.''')) + ], help_text=format_lazy( + _('Uploaded ZIM files will be stored under {kiwix_home}/content ' + 'on your {box_name}. If Kiwix fails to add the file, it will be ' + 'deleted immediately to save disk space.'), + box_name=_(cfg.box_name), kiwix_home=KIWIX_HOME)) diff --git a/plinth/modules/kiwix/manifest.py b/plinth/modules/kiwix/manifest.py index e7e3bc3d8..bf45ebe8d 100644 --- a/plinth/modules/kiwix/manifest.py +++ b/plinth/modules/kiwix/manifest.py @@ -5,7 +5,7 @@ from django.utils.translation import gettext_lazy as _ from plinth.clients import validate clients = validate([{ - 'name': _('kiwix'), + 'name': _('Kiwix'), 'platforms': [{ 'type': 'web', 'url': '/kiwix' diff --git a/plinth/modules/kiwix/privileged.py b/plinth/modules/kiwix/privileged.py index 5a47db19b..43cabd5fd 100644 --- a/plinth/modules/kiwix/privileged.py +++ b/plinth/modules/kiwix/privileged.py @@ -3,10 +3,11 @@ Privileged actions for Kiwix content server. """ -import subprocess +import os import pathlib import shutil -import xml.etree.ElementTree as ET +import subprocess +from xml.etree import ElementTree from plinth import action_utils from plinth.actions import privileged @@ -19,19 +20,19 @@ CONTENT_DIR = KIWIX_HOME / 'content' @privileged -def add_content(file_name: str): +def add_package(file_name: str): """Adds a content package to Kiwix. Adding packages is idempotent. - Users can add content to Kiwix in multiple ways: + Users can add content to Kiwix in multiple ways: - Upload a ZIM file - Provide a link to the ZIM file - Provide a magnet link to the ZIM file - The commandline download manager aria2c is a dependency of kiwix-tools. - aria2c is used for both HTTP and Magnet downloads. - """ + The commandline download manager aria2c is a dependency of kiwix-tools. + aria2c is used for both HTTP and Magnet downloads. + """ kiwix.validate_file_name(file_name) # Moving files to the Kiwix library path ensures that @@ -39,6 +40,8 @@ def add_content(file_name: str): zim_file_name = pathlib.Path(file_name).name CONTENT_DIR.mkdir(exist_ok=True) zim_file_dest = str(CONTENT_DIR / zim_file_name) + shutil.chown(file_name, 'root', 'root') + os.chmod(file_name, 0o644) shutil.move(file_name, zim_file_dest) _kiwix_manage_add(zim_file_dest) @@ -48,40 +51,51 @@ def _kiwix_manage_add(zim_file: str): subprocess.check_call(['kiwix-manage', LIBRARY_FILE, 'add', zim_file]) # kiwix-serve doesn't read the library file unless it is restarted. - action_utils.service_restart('kiwix-server-freedombox') + action_utils.service_try_restart('kiwix-server-freedombox') @privileged -def uninstall(): +def uninstall() -> None: """Remove all content during uninstall.""" - shutil.rmtree(str(CONTENT_DIR)) - LIBRARY_FILE.unlink() + shutil.rmtree(str(CONTENT_DIR), ignore_errors=True) + LIBRARY_FILE.unlink(missing_ok=True) @privileged -def list_content_packages() -> dict[str, dict]: - library = ET.parse(LIBRARY_FILE).getroot() +def list_packages() -> dict[str, dict[str, str]]: + """Return the list of content packages configured in library file.""" + library = ElementTree.parse(LIBRARY_FILE).getroot() - # Relying on the fact that Python dictionaries maintain order of insertion. - return { - book.attrib['id']: { - 'title': book.attrib['title'], - 'description': book.attrib['description'], - # strip '.zim' from the path - 'path': book.attrib['path'].split('/')[-1][:-4].lower() - } - for book in library - } + books = {} + for book in library: + path = book.attrib['path'].split('/')[-1] + path = path.removesuffix('.zim').lower() # Strip '.zim' from the path + try: + books[book.attrib['id']] = { + 'title': book.attrib['title'], + 'description': book.attrib['description'], + 'path': path + } + except KeyError: + pass # Ignore entries that don't have expected properties + + return books @privileged -def delete_content_package(zim_id: str): - library = ET.parse(LIBRARY_FILE).getroot() +def delete_package(zim_id: str): + """Remove a content package from the library file.""" + library = ElementTree.parse(LIBRARY_FILE).getroot() for book in library: - if book.attrib['id'] == zim_id: + try: + if book.attrib['id'] != zim_id: + continue + subprocess.check_call( ['kiwix-manage', LIBRARY_FILE, 'remove', zim_id]) (KIWIX_HOME / book.attrib['path']).unlink() - action_utils.service_restart('kiwix-server-freedombox') + action_utils.service_try_restart('kiwix-server-freedombox') return + except KeyError: # Expected properties not found on elements + pass diff --git a/plinth/modules/kiwix/static/icons/kiwix.png b/plinth/modules/kiwix/static/icons/kiwix.png index 021762824..9abe720c8 100644 Binary files a/plinth/modules/kiwix/static/icons/kiwix.png and b/plinth/modules/kiwix/static/icons/kiwix.png differ diff --git a/plinth/modules/kiwix/static/icons/kiwix.svg b/plinth/modules/kiwix/static/icons/kiwix.svg index 566b1064b..f944fd9ff 100644 --- a/plinth/modules/kiwix/static/icons/kiwix.svg +++ b/plinth/modules/kiwix/static/icons/kiwix.svg @@ -1,20 +1,48 @@ - + - -

    {{ title }}

    - -

    - {% blocktrans %} - You can download - content packages from the Kiwix project or - create your own. - {% endblocktrans %} -

    - -

    - {% blocktrans %} - Content packages can be added in the following ways: -

    - - - - {% endblocktrans %} -

    - - {% if max_filesize %} - - {% endif %} - -
    - {% csrf_token %} - - {{ form|bootstrap }} - - -
    - -{% endblock %} diff --git a/plinth/modules/kiwix/templates/kiwix-add-package.html b/plinth/modules/kiwix/templates/kiwix-add-package.html new file mode 100644 index 000000000..1e64d958e --- /dev/null +++ b/plinth/modules/kiwix/templates/kiwix-add-package.html @@ -0,0 +1,39 @@ +{% extends "base.html" %} +{% comment %} +# SPDX-License-Identifier: AGPL-3.0-or-later +{% endcomment %} + +{% load bootstrap %} +{% load i18n %} + +{% block content %} + +

    {{ title }}

    + +

    + {% blocktrans trimmed %} + You can download content packages from the Kiwix + project or create your own. + {% endblocktrans %} +

    + + {% if max_filesize %} + + {% endif %} + +
    + {% csrf_token %} + + {{ form|bootstrap }} + + +
    + +{% endblock %} diff --git a/plinth/modules/kiwix/templates/delete-content-package.html b/plinth/modules/kiwix/templates/kiwix-delete-package.html similarity index 93% rename from plinth/modules/kiwix/templates/delete-content-package.html rename to plinth/modules/kiwix/templates/kiwix-delete-package.html index 70f209af7..44159849a 100644 --- a/plinth/modules/kiwix/templates/delete-content-package.html +++ b/plinth/modules/kiwix/templates/kiwix-delete-package.html @@ -15,7 +15,8 @@

    {% blocktrans trimmed %} - Delete this package permanently? You may add it back later if you have a copy of the ZIM file. + Delete this package permanently? You may add it back later if you have a + copy of the ZIM file. {% endblocktrans %}

    diff --git a/plinth/modules/kiwix/templates/kiwix.html b/plinth/modules/kiwix/templates/kiwix.html index 45fb03c7d..e3a30a822 100644 --- a/plinth/modules/kiwix/templates/kiwix.html +++ b/plinth/modules/kiwix/templates/kiwix.html @@ -8,32 +8,31 @@ {% block configuration %} {{ block.super }} -

    {% trans "Manage Content" %}

    +

    {% trans "Manage Content Packages" %}

    - - {% trans 'Add' %} + {% trans 'Add Package' %}
    {% if not packages %} -

    {% trans 'No content available.' %}

    +

    {% trans 'No content packages available.' %}

    {% else %}
    {% for id, package in packages.items %}
    - + title="{{ package.description }}"> {{ package.title }} - diff --git a/plinth/modules/kiwix/tests/data/invalid.zim b/plinth/modules/kiwix/tests/data/invalid.zim index bd7fab739..944c593a9 100644 --- a/plinth/modules/kiwix/tests/data/invalid.zim +++ b/plinth/modules/kiwix/tests/data/invalid.zim @@ -1 +1 @@ -Nothing to see here. \ No newline at end of file +Nothing to see here. diff --git a/plinth/modules/kiwix/tests/test_functional.py b/plinth/modules/kiwix/tests/test_functional.py index 02d364939..91c8ea682 100644 --- a/plinth/modules/kiwix/tests/test_functional.py +++ b/plinth/modules/kiwix/tests/test_functional.py @@ -3,94 +3,97 @@ Functional, browser based tests for Kiwix app. """ -import pkg_resources +import pathlib +from time import sleep + import pytest -from time import sleep -from plinth.modules.kiwix.tests.test_privileged import ZIM_ID - from plinth.tests import functional +from .test_privileged import ZIM_ID + pytestmark = [pytest.mark.apps, pytest.mark.sso, pytest.mark.kiwix] _default_url = functional.config['DEFAULT']['url'] -ZIM_ID = 'bc4f8cdf-5626-2b13-3860-0033deddfbea' +_data_dir = pathlib.Path(__file__).parent / 'data' class TestKiwixApp(functional.BaseAppTests): + """Basic functional tests for Kiwix app.""" + app_name = 'kiwix' has_service = True has_web = True - def test_add_delete_content_package(self, session_browser): + def test_add_delete_package(self, session_browser): """Test adding/deleting content package to the library.""" functional.app_enable(session_browser, 'kiwix') - zim_file = pkg_resources.resource_filename( - 'plinth.modules.kiwix.tests', 'data/FreedomBox.zim') - _add_content_package(session_browser, zim_file) - assert _is_content_package_listed(session_browser, 'freedombox') - assert _is_content_package_available(session_browser, 'FreedomBox') + zim_file = _data_dir / 'FreedomBox.zim' + _add_package(session_browser, str(zim_file)) + assert _is_package_listed(session_browser, 'freedombox') + assert _is_package_available(session_browser, 'FreedomBox') - _delete_content_package(session_browser, ZIM_ID) - assert not _is_content_package_listed(session_browser, 'freedombox') - assert not _is_content_package_available(session_browser, 'FreedomBox') + _delete_package(session_browser, ZIM_ID) + assert not _is_package_listed(session_browser, 'freedombox') + assert not _is_package_available(session_browser, 'FreedomBox') @pytest.mark.backups def test_backup_restore(self, session_browser): """Test backing up and restoring.""" functional.app_enable(session_browser, 'kiwix') - zim_file = pkg_resources.resource_filename( - 'plinth.modules.kiwix.tests', 'data/FreedomBox.zim') - _add_content_package(session_browser, zim_file) + zim_file = _data_dir / 'FreedomBox.zim' + _add_package(session_browser, str(zim_file)) functional.backup_create(session_browser, 'kiwix', 'test_kiwix') - _delete_content_package(session_browser, ZIM_ID) + _delete_package(session_browser, ZIM_ID) functional.backup_restore(session_browser, 'kiwix', 'test_kiwix') - assert _is_content_package_listed(session_browser, 'freedombox') - assert _is_content_package_available(session_browser, 'FreedomBox') + assert _is_package_listed(session_browser, 'freedombox') + assert _is_package_available(session_browser, 'FreedomBox') def test_add_invalid_zim_file(self, session_browser): """Test handling of invalid zim files.""" functional.app_enable(session_browser, 'kiwix') - zim_file = pkg_resources.resource_filename( - 'plinth.modules.kiwix.tests', 'data/invalid.zim') - _add_content_package(session_browser, zim_file) + zim_file = _data_dir / 'invalid.zim' + _add_package(session_browser, str(zim_file)) - assert not _is_content_package_listed(session_browser, 'invalid') + assert not _is_package_listed(session_browser, 'invalid') -def _add_content_package(browser, file_name): - browser.links.find_by_href('/plinth/apps/kiwix/content/add/').first.click() +def _add_package(browser, file_name): + """Add a package by uploading the ZIM file in kiwix app page.""" + browser.links.find_by_href('/plinth/apps/kiwix/package/add/').first.click() browser.attach_file('kiwix-file', file_name) functional.submit(browser, form_class='form-kiwix') -def _is_content_package_available(browser, title) -> bool: +def _is_package_available(browser, title) -> bool: + """Check whether a ZIM file is available in Kiwix web interface.""" browser.visit(f'{_default_url}/kiwix') sleep(1) # Allow time for the books to appear titles = browser.find_by_id('book__title') - print(len(titles)) - print([title.value for title in titles]) - return any(map(lambda e: e.value == title, titles)) + return any(element.value == title for element in titles) -def _is_content_package_listed(browser, name) -> bool: +def _is_package_listed(browser, name) -> bool: + """Return whether a content package is list in kiwix app page.""" functional.nav_to_module(browser, 'kiwix') links_found = browser.links.find_by_partial_href(f'/kiwix/viewer#{name}') return len(links_found) == 1 -def _delete_content_package(browser, zim_id): +def _delete_package(browser, zim_id): + """Delete a content package from the kiwix app page.""" functional.nav_to_module(browser, 'kiwix') link = browser.links.find_by_href( - f'/plinth/apps/kiwix/content/{zim_id}/delete/') + f'/plinth/apps/kiwix/package/{zim_id}/delete/') if not link: raise ValueError('ZIM file missing!') + link.first.click() functional.submit(browser, form_class='form-delete') diff --git a/plinth/modules/kiwix/tests/test_privileged.py b/plinth/modules/kiwix/tests/test_privileged.py index 3050c70b3..34b98707d 100644 --- a/plinth/modules/kiwix/tests/test_privileged.py +++ b/plinth/modules/kiwix/tests/test_privileged.py @@ -4,7 +4,7 @@ Test module for Kiwix actions. """ import pathlib -import pkg_resources +import shutil from unittest.mock import patch import pytest @@ -22,65 +22,57 @@ ZIM_ID = 'bc4f8cdf-5626-2b13-3860-0033deddfbea' @pytest.fixture(autouse=True) -def fixture_kiwix_home(tmpdir): - """Set Kiwix home to a new temporary directory - initialized with an empty library file.""" - privileged.KIWIX_HOME = pathlib.Path(str(tmpdir / 'kiwix')) +def fixture_kiwix_home(tmp_path): + """Create a new Kiwix home in a new temporary directory. + + Initialize with a sample, valid library file. + """ + privileged.KIWIX_HOME = tmp_path / 'kiwix' privileged.KIWIX_HOME.mkdir() privileged.CONTENT_DIR = privileged.KIWIX_HOME / 'content' privileged.CONTENT_DIR.mkdir() privileged.LIBRARY_FILE = privileged.KIWIX_HOME / 'library_zim.xml' - with open(privileged.LIBRARY_FILE, 'w', encoding='utf_8') as library_file: - library_file.write(EMPTY_LIBRARY_CONTENTS) + source_file = pathlib.Path(__file__).parent / 'data/sample_library_zim.xml' + shutil.copy(source_file, privileged.LIBRARY_FILE) @pytest.fixture(autouse=True) def fixture_patch(): """Patch some underlying methods.""" - with patch('subprocess.check_call'), patch('subprocess.run'): + with patch('subprocess.check_call'), patch('subprocess.run'), patch( + 'os.chown'): yield -def test_add_content(tmpdir): +def test_add_package(tmp_path): """Test adding a content package to Kiwix.""" - some_dir = tmpdir / 'some' / 'dir' - pathlib.Path(some_dir).mkdir(parents=True, exist_ok=True) + some_dir = tmp_path / 'some' / 'dir' + some_dir.mkdir(parents=True, exist_ok=True) zim_file_name = 'wikipedia_en_all_maxi_2022-05.zim' orig_file = some_dir / zim_file_name - pathlib.Path(orig_file).touch() + orig_file.touch() - privileged.add_content(str(orig_file)) + privileged.add_package(str(orig_file)) assert (privileged.KIWIX_HOME / 'content' / zim_file_name).exists() assert not orig_file.exists() -def test_list_content_packages(): +def test_list_packages(): """Test listing the content packages from a library file.""" - privileged.LIBRARY_FILE = pkg_resources.resource_filename( - 'plinth.modules.kiwix.tests', 'data/sample_library_zim.xml') - content_packages = privileged.list_content_packages() - assert content_packages[ZIM_ID] == { + content = privileged.list_packages() + assert content[ZIM_ID] == { 'title': 'FreedomBox', 'description': 'A sample content archive', 'path': 'freedombox' } -def test_delete_content_package(): +def test_delete_package(): """Test deleting one content package.""" - sample_library_file = pkg_resources.resource_filename( - 'plinth.modules.kiwix.tests', 'data/sample_library_zim.xml') - - with open(sample_library_file, 'r', - encoding='utf_8') as sample_library_file: - with open(privileged.LIBRARY_FILE, 'w', - encoding='utf_8') as library_file: - library_file.write(sample_library_file.read()) - zim_file = privileged.CONTENT_DIR / 'FreedomBox.zim' zim_file.touch() - privileged.delete_content_package(ZIM_ID) + privileged.delete_package(ZIM_ID) assert not zim_file.exists() # Cannot check that the book is removed from library_zim.xml diff --git a/plinth/modules/kiwix/tests/test_validations.py b/plinth/modules/kiwix/tests/test_validations.py index f4113e131..5be411a25 100644 --- a/plinth/modules/kiwix/tests/test_validations.py +++ b/plinth/modules/kiwix/tests/test_validations.py @@ -3,19 +3,18 @@ Test module for Kiwix validations. """ -import unittest +import pytest + from plinth.modules import kiwix -class TestValidations(unittest.TestCase): +def test_add_file_with_invalid_extension(): + """Test that adding a file with invalid fails as expected.""" + with pytest.raises(ValueError): + kiwix.validate_file_name('wikipedia.zip') - def test_add_file_with_invalid_extension(self): - self.assertRaises(ValueError, - lambda: kiwix.validate_file_name('wikipedia.zip')) + # We don't support the legacy format of split zim files. + with pytest.raises(ValueError): + kiwix.validate_file_name('wikipedia_en_all_maxi_2022-05.zima') - # We don't support the legacy format of split zim files. - self.assertRaises( - ValueError, lambda: kiwix.validate_file_name( - 'wikipedia_en_all_maxi_2022-05.zima')) - - kiwix.validate_file_name('wikipedia_en_all_maxi_2022-05.zim') + kiwix.validate_file_name('wikipedia_en_all_maxi_2022-05.zim') diff --git a/plinth/modules/kiwix/tests/test_views.py b/plinth/modules/kiwix/tests/test_views.py index 099ccf290..fc77cd22d 100644 --- a/plinth/modules/kiwix/tests/test_views.py +++ b/plinth/modules/kiwix/tests/test_views.py @@ -3,15 +3,15 @@ Test module for Kiwix views. """ -from plinth import module_loader -from django import urls +import pathlib from unittest.mock import call, patch -from django.contrib.messages.storage.fallback import FallbackStorage -from django.http.response import Http404 -from django.test.client import encode_multipart, RequestFactory import pytest +from django import urls +from django.contrib.messages.storage.fallback import FallbackStorage +from django.http.response import Http404 +from plinth import module_loader from plinth.modules.kiwix import views # For all tests, use plinth.urls instead of urls configured for testing @@ -19,6 +19,8 @@ pytestmark = pytest.mark.urls('plinth.urls') ZIM_ID = 'bc4f8cdf-5626-2b13-3860-0033deddfbea' +_data_dir = pathlib.Path(__file__).parent / 'data' + @pytest.fixture(autouse=True, scope='module') def fixture_kiwix_urls(): @@ -41,107 +43,88 @@ def make_request(request, view, **kwargs): @pytest.fixture(autouse=True) -def kiwix_patch(): +def fiture_kiwix_patch(): """Patch kiwix methods.""" - with patch('plinth.modules.kiwix.privileged.list_content_packages' - ) as list_libraries: + with patch( + 'plinth.modules.kiwix.privileged.list_packages') as list_libraries: list_libraries.return_value = { ZIM_ID: { - 'title': 'TestExistingContentPackage', + 'title': 'TestExistingPackage', 'description': 'A sample content package', - 'path': 'test_existing_content_package' + 'path': 'test_existing_package' } } yield -@pytest.fixture() -def storage_info_patch(): - """Patch storage info method.""" - with patch('plinth.modules.storage.get_mount_info') as get_mount_info: - get_mount_info.return_value = {'free_bytes': 1000000000000} - yield - - -@patch('plinth.modules.kiwix.privileged.add_content') -def test_add_content_package(add_content, rf): +@patch('tempfile.TemporaryDirectory') +@patch('plinth.modules.kiwix.privileged.add_package') +def test_add_package(add_package, temp_dir_class, rf, tmp_path): """Test that adding content view works.""" - with open('plinth/modules/kiwix/tests/data/FreedomBox.zim', - 'rb') as zim_file: - post_data = { - 'kiwix-file': zim_file, - } - post_data = encode_multipart('BoUnDaRyStRiNg', post_data) - request = rf.post( - '', data=post_data, content_type='multipart/form-data; ' - 'boundary=BoUnDaRyStRiNg') + temp_dir_class.return_value.__enter__.return_value = str(tmp_path) + with open(_data_dir / 'FreedomBox.zim', 'rb') as zim_file: + post_data = {'kiwix-file': zim_file} + request = rf.post('', data=post_data) response, messages = make_request(request, - views.AddContentView.as_view()) + views.AddPackageView.as_view()) assert response.status_code == 302 assert response.url == urls.reverse('kiwix:index') assert list(messages)[0].message == 'Content package added.' - add_content.assert_has_calls([call('/tmp/FreedomBox.zim')]) + add_package.assert_has_calls([call(f'{tmp_path}/FreedomBox.zim')]) -@patch('plinth.modules.kiwix.privileged.add_content') -def test_add_content_package_failed(add_content, rf): +@patch('plinth.modules.kiwix.privileged.add_package') +def test_add_package_failed(add_package, rf): """Test that adding content package fails in case of an error.""" - add_content.side_effect = RuntimeError('TestError') - with open('plinth/modules/kiwix/tests/data/FreedomBox.zim', - 'rb') as zim_file: - post_data = { - 'kiwix-file': zim_file, - } - post_data = encode_multipart('BoUnDaRyStRiNg', post_data) - request = rf.post( - '', data=post_data, content_type='multipart/form-data; ' - 'boundary=BoUnDaRyStRiNg') + add_package.side_effect = RuntimeError('TestError') + with open(_data_dir / 'FreedomBox.zim', 'rb') as zim_file: + post_data = {'kiwix-file': zim_file} + request = rf.post('', data=post_data) response, messages = make_request(request, - views.AddContentView.as_view()) + views.AddPackageView.as_view()) assert response.status_code == 302 assert response.url == urls.reverse('kiwix:index') - assert list(messages)[0].message == \ - 'Failed to add content package.' + assert list(messages)[0].message == 'Failed to add content package.' @patch('plinth.app.App.get') def test_delete_package_confirmation_view(_app, rf): - """Test that deleting package confirmation shows correct title.""" - response, _ = make_request(rf.get(''), views.delete_content, zim_id=ZIM_ID) + """Test that deleting content confirmation shows correct title.""" + response, _ = make_request(rf.get(''), views.delete_package, zim_id=ZIM_ID) assert response.status_code == 200 - assert response.context_data['name'] == 'TestExistingContentPackage' + assert response.context_data['name'] == 'TestExistingPackage' -@patch('plinth.modules.kiwix.privileged.delete_content_package') +@patch('plinth.modules.kiwix.privileged.delete_package') @patch('plinth.app.App.get') -def test_delete_content_package(_app, delete_content_package, rf): +def test_delete_package(_app, delete_package, rf): """Test that deleting a content package works.""" - response, messages = make_request(rf.post(''), views.delete_content, + response, messages = make_request(rf.post(''), views.delete_package, zim_id=ZIM_ID) assert response.status_code == 302 assert response.url == urls.reverse('kiwix:index') - assert list(messages)[0].message == 'TestExistingContentPackage deleted.' - delete_content_package.assert_has_calls([call(ZIM_ID)]) + assert list(messages)[0].message == 'TestExistingPackage deleted.' + delete_package.assert_has_calls([call(ZIM_ID)]) -@patch('plinth.modules.kiwix.privileged.delete_content_package') -def test_delete_content_package_error(delete_content_package, rf): - """Test that deleting a content package shows an error when operation fails.""" - delete_content_package.side_effect = ValueError('TestError') - response, messages = make_request(rf.post(''), views.delete_content, +@patch('plinth.modules.kiwix.privileged.delete_package') +def test_delete_package_error(delete_package, rf): + """Test that deleting content shows an error when operation fails.""" + delete_package.side_effect = ValueError('TestError') + response, messages = make_request(rf.post(''), views.delete_package, zim_id=ZIM_ID) assert response.status_code == 302 assert response.url == urls.reverse('kiwix:index') assert list(messages)[0].message == \ - 'Could not delete TestExistingContentPackage: TestError' + 'Could not delete TestExistingPackage: TestError' -def test_delete_content_package_non_existing(rf): - """Test that deleting a content package shows error when operation fails.""" +def test_delete_package_non_existing(rf): + """Test that deleting content shows error when operation fails.""" with pytest.raises(Http404): - make_request(rf.post(''), views.delete_content, + make_request(rf.post(''), views.delete_package, zim_id='NonExistentZimId') with pytest.raises(Http404): - make_request(rf.get(''), views.delete_content, + make_request(rf.get(''), views.delete_package, zim_id='NonExistentZimId') diff --git a/plinth/modules/kiwix/urls.py b/plinth/modules/kiwix/urls.py index 1170d12b5..892a46d00 100644 --- a/plinth/modules/kiwix/urls.py +++ b/plinth/modules/kiwix/urls.py @@ -9,8 +9,8 @@ from . import views urlpatterns = [ re_path(r'^apps/kiwix/$', views.KiwixAppView.as_view(), name='index'), - re_path(r'^apps/kiwix/content/add/$', views.AddContentView.as_view(), - name='add-content'), - re_path(r'^apps/kiwix/content/(?P[a-zA-Z0-9-]+)/delete/$', - views.delete_content, name='delete-content'), + re_path(r'^apps/kiwix/package/add/$', views.AddPackageView.as_view(), + name='add-package'), + re_path(r'^apps/kiwix/package/(?P[a-zA-Z0-9-]+)/delete/$', + views.delete_package, name='delete-package'), ] diff --git a/plinth/modules/kiwix/views.py b/plinth/modules/kiwix/views.py index 470a6f6f5..0f7a2dc40 100644 --- a/plinth/modules/kiwix/views.py +++ b/plinth/modules/kiwix/views.py @@ -4,6 +4,7 @@ Views for the Kiwix module. """ import logging +import tempfile from django.contrib import messages from django.contrib.messages.views import SuccessMessageMixin @@ -27,21 +28,23 @@ logger = logging.getLogger(__name__) class KiwixAppView(views.AppView): """Serve configuration form.""" + app_id = 'kiwix' template_name = 'kiwix.html' def get_context_data(self, **kwargs): """Return additional context for rendering the template.""" context = super().get_context_data(**kwargs) - context['packages'] = privileged.list_content_packages() + context['packages'] = privileged.list_packages() return context -class AddContentView(SuccessMessageMixin, FormView): - """View to add content in the form of ZIM files.""" - form_class = forms.AddContentForm +class AddPackageView(SuccessMessageMixin, FormView): + """View to add content package in the form of ZIM files.""" + + form_class = forms.AddPackageForm prefix = 'kiwix' - template_name = 'add-content-package.html' + template_name = 'kiwix-add-package.html' success_url = reverse_lazy('kiwix:index') success_message = _('Content package added.') @@ -66,23 +69,26 @@ class AddContentView(SuccessMessageMixin, FormView): def form_valid(self, form): """Store the uploaded file.""" multipart_file = self.request.FILES['kiwix-file'] - zim_file_name = '/tmp/' + multipart_file.name - with open(zim_file_name, 'wb+') as zim_file: - for chunk in multipart_file.chunks(): - zim_file.write(chunk) - try: - privileged.add_content(zim_file_name) - except Exception: - messages.error(self.request, _('Failed to add content package.')) - return redirect(reverse_lazy('kiwix:index')) + with tempfile.TemporaryDirectory() as temp_dir: + zim_file_name = temp_dir + '/' + multipart_file.name + with open(zim_file_name, 'wb+') as zim_file: + for chunk in multipart_file.chunks(): + zim_file.write(chunk) + + try: + privileged.add_package(zim_file_name) + except Exception: + messages.error(self.request, + _('Failed to add content package.')) + return redirect(reverse_lazy('kiwix:index')) return super().form_valid(form) -def delete_content(request, zim_id): +def delete_package(request, zim_id): """View to delete a library.""" - packages = privileged.list_content_packages() + packages = privileged.list_packages() if zim_id not in packages: raise Http404 @@ -90,8 +96,8 @@ def delete_content(request, zim_id): if request.method == 'POST': try: - privileged.delete_content_package(zim_id) - messages.success(request, _(f'{name} deleted.')) + privileged.delete_package(zim_id) + messages.success(request, _('{name} deleted.').format(name=name)) except Exception as error: messages.error( request, @@ -99,7 +105,7 @@ def delete_content(request, zim_id): name=name, error=error)) return redirect(reverse_lazy('kiwix:index')) - return TemplateResponse(request, 'delete-content-package.html', { + return TemplateResponse(request, 'kiwix-delete-package.html', { 'title': app_module.App.get('kiwix').info.name, 'name': name })