Merge branch 'main' into 147-radio-buttons

docs/adr/0002-cypress-end-to-end-testing.md

@@ -0,0 +1,25 @@
+# 1. Cypress for end-to-end testing
+
+Date: 2023-07-14
+
+## Status
+
+Accepted
+
+## Context
+
+We need to test the functionality of the application and manual testing was becoming tedious because of the need to accept/reject every output.
+
+## Decision
+
+Use Cypress for end-to-end testing, both as an aid to development (it can walk through the functionality in a browser window faster than a person and provide continuous feedback) and as part of our CI test suite to automatically detect problems.
+
+## Consequences
+
+In other areas we've used Playwright, which also has support for Python. However, we needed something that would help us in the short-term, and Cypress was faster to get started with. We've not evaluated Cypress for longer term use.
+
+With any kind of end-to-end testing there's a risk that the tests will be slow and that the time to run the test suite will significantly increase. We've put the Cypress tests into a separate GitHub Action step, called with a different just command, so that we can monitor the time it takes to run. At the time of writing, the Cypress tests take around 1 minute to run, compared to two minutes for the unit tests.
+
+There's also a risk that having tests coupled to the UI will make development slower and make it harder to change things. To avoid this, we will follow the [Cypress Best Practices](https://docs.cypress.io/guides/references/best-practices) when designing our tests. This includes using data attributes, e.g `data-sacro-el`, to avoid coupling specific UI characteristics to test behaviour. Following the best practices should also help us keep the tests fast and reliable.
+
+We believe that the advantages of having the Cypress tests there to support development by providing fast feedback, outweigh the cost of having to maintain them and the slowdown in the CI pipeline.

sacro/middleware.py

@@ -1,6 +1,9 @@
 from django.conf import settings
 from django.http import HttpResponseForbidden
 
+from sacro.errors import error
+from sacro.versioning import IncorrectVersionError
+
 
 class AppTokenMiddleware:
     def __init__(self, get_response):
@@ -13,3 +16,17 @@ def __call__(self, request):
 
         response = self.get_response(request)
         return response
+
+
+class ErrorHandlerMiddleware:
+    def __init__(self, get_response):
+        self.get_response = get_response
+
+    def __call__(self, request):
+        return self.get_response(request)
+
+    def process_exception(self, request, exception):
+        if not isinstance(exception, IncorrectVersionError):
+            raise
+
+        return error(request, message=str(exception))

sacro/settings.py

@@ -52,6 +52,7 @@
 
 MIDDLEWARE = [
     "sacro.middleware.AppTokenMiddleware",
+    "sacro.middleware.ErrorHandlerMiddleware",
     "django.middleware.security.SecurityMiddleware",
     "whitenoise.middleware.WhiteNoiseMiddleware",
     "django.contrib.sessions.middleware.SessionMiddleware",
@@ -171,3 +172,7 @@
 
 # Insert Whitenoise Middleware.
 STATICFILES_STORAGE = "whitenoise.storage.CompressedStaticFilesStorage"
+
+
+# PROJECT SETTINGS
+ACRO_SUPPORTED_VERSION = "0.4.x"

sacro/templates/error.html

@@ -21,13 +21,15 @@ <h1 class="text-3xl font-bold leading-tight tracking-tight">
         The application encountered a problem
       </h1>
       <div class="flex flex-col gap-y-4 text-lg">
-        <p>
-          Please restart the application and try again.<br />
-          If this problem continues please contact support.
-        </p>
         <p class="break-words">
-          {{ message }}
+          {% if message %}
+            {{ message }}
+          {% else %}
+            Please restart the application and try again.<br />
+            If this problem continues please contact support.
+          {% endif %}
         </p>
+
       </div>
     </main>
   </body>

sacro/templates/index.html

@@ -93,7 +93,7 @@ <h2 id="outputListHeader" class="sr-only">Output list</h2>
   <main class="container overflow-y-scroll">
     {{ outputs|json_script:"outputData" }}
 
-    <details class="overflow-hidden bg-white shadow mb-3" id="riskProfile" open>
+    <details class="overflow-hidden bg-white shadow mb-3" id="riskProfile">
       <summary class="pl-4 pr-2 py-2">
         <h3 class="ml-1 text-base font-medium leading-7 text-gray-900">
           ACRO risk profile

sacro/versioning.py

@@ -0,0 +1,71 @@
+import functools
+
+from django.conf import settings
+
+
+class IncorrectVersionError(Exception):
+    def __init__(self, *args, used, supported, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.used = used
+        self.supported = supported
+
+    def __str__(self):
+        return f"Unsupported ACRO output. This viewer supports ACRO version {self.supported}, but your results were generated with version {self.used}."
+
+
+class UnsupportedVersionFormatError(Exception):
+    def __init__(self, *args, version, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.version = version
+
+
+@functools.total_ordering
+class Version:
+    """Utility class to parse and compare version strings"""
+
+    def __init__(self, version: str) -> None:
+        try:
+            major, minor, *_ = version.split(".")
+
+            # check major and minor are valid numbers
+            int(major)
+            int(minor)
+
+            self.major = major
+            self.minor = minor
+        except ValueError:
+            msg = f"Expected version to be in format 1.2.3, got {version}"
+            raise UnsupportedVersionFormatError(msg, version=version)
+
+        self.original = version
+
+    def __eq__(self, other: "Version") -> bool:
+        return self.major == other.major and self.minor == other.minor
+
+    def __gt__(self, other: "Version") -> bool:
+        if self.major > other.major:
+            return True
+
+        if self.major == other.major and self.minor > other.minor:
+            return True
+
+        return False
+
+    def __repr__(self):
+        return f"Version: {self.original}"
+
+    def __str__(self):
+        return self.original
+
+
+def check_version(version: str) -> None:
+    """
+    Check the given version against the supported version in settings
+
+    We don't care about bugfix versions so the Version class ignores them.
+    """
+    supported = Version(settings.ACRO_SUPPORTED_VERSION)
+    used = Version(version)
+
+    if used < supported:
+        raise IncorrectVersionError(used=used, supported=supported)

sacro/views.py

@@ -1,4 +1,5 @@
 import getpass
+import hashlib
 import html
 import json
 import logging
@@ -15,6 +16,7 @@
 from django.views.decorators.http import require_GET, require_POST
 
 from sacro.adapters import local_audit, zipfile
+from sacro.versioning import check_version
 
 
 logger = logging.getLogger(__name__)
@@ -44,6 +46,10 @@ def __post_init__(self):
 
         self.version = self.raw_metadata["version"]
         self.update(self.raw_metadata["results"])
+        self.annotate()
+
+    def annotate(self):
+        """Add various useful annotations to the JSON data"""
 
         # add urls to JSON data
         for output, metadata in self.items():
@@ -57,6 +63,26 @@ def __post_init__(self):
                     "contents",
                 )
 
+        # add and check checksum data
+        checksums_dir = self.path.parent / "checksums"
+        for output, metadata in self.items():
+            for filedata in metadata["files"]:
+                filedata["checksum_valid"] = False
+                filedata["checksum"] = None
+
+                path = checksums_dir / (filedata["name"] + ".txt")
+                if not path.exists():
+                    continue
+
+                filedata["checksum"] = path.read_text(encoding="utf8")
+                actual_file = self.get_file_path(output, filedata["name"])
+
+                if not actual_file.exists():  # pragma: nocover
+                    continue
+
+                checksum = hashlib.sha256(actual_file.read_bytes()).hexdigest()
+                filedata["checksum_valid"] = checksum == filedata["checksum"]
+
     def get_file_path(self, output, filename):
         """Return absolute path to output file"""
         if filename not in {
@@ -85,7 +111,11 @@ def get_outputs(data):
     if not path.exists():  # pragma: no cover
         raise Http404
 
-    return Outputs(path)
+    outputs = Outputs(path)
+
+    check_version(outputs.version)
+
+    return outputs
 
 
 @require_GET

tests/conftest.py

@@ -0,0 +1,17 @@
+import shutil
+from pathlib import Path
+
+import pytest
+
+from sacro import views
+
+
+@pytest.fixture
+def TEST_PATH():
+    return Path("outputs/results.json")
+
+
+@pytest.fixture
+def test_outputs(tmp_path, TEST_PATH):
+    shutil.copytree(TEST_PATH.parent, tmp_path, dirs_exist_ok=True)
+    return views.Outputs(tmp_path / TEST_PATH.name)

tests/test_middleware.py

@@ -0,0 +1,21 @@
+import json
+import shutil
+
+from django.test import Client
+
+
+def test_error_handling_middleware(client, tmp_path, TEST_PATH):
+    shutil.copytree(TEST_PATH.parent, tmp_path, dirs_exist_ok=True)
+    path = tmp_path / TEST_PATH.name
+
+    # change the version number
+    data = json.load(path.open())
+    data["version"] = "0.3.0"
+    json.dump(data, path.open("w"))
+
+    response = Client().get(f"/?path={path}")
+    assert response.status_code == 500
+    assert (
+        "Unsupported ACRO output. This viewer supports ACRO version 0.4.x, but your results were generated with version 0.3.0."
+        in response.rendered_content
+    )

tests/test_versioning.py

@@ -0,0 +1,74 @@
+import pytest
+from django.test import override_settings
+
+from sacro.versioning import (
+    IncorrectVersionError,
+    UnsupportedVersionFormatError,
+    Version,
+    check_version,
+)
+
+
+@override_settings(ACRO_SUPPORTED_VERSION="0.4.0")
+def test_check_version(monkeypatch):
+    assert check_version("0.4.0") is None
+    assert check_version("0.4.2") is None
+
+    with pytest.raises(IncorrectVersionError):
+        check_version("0.3.0")
+
+
+@pytest.mark.parametrize("version", ["test", "v0.4.0"])
+def test_version_init_with_unexpected_format(version):
+    with pytest.raises(UnsupportedVersionFormatError):
+        Version(version)
+
+
+@pytest.mark.parametrize("version", ["0.4.0", "0.4"])
+def test_version_init_success(version):
+    Version(version)
+
+
+def test_version_rich_comparison_eq():
+    assert Version("0.4.0") == Version("0.4.0")
+
+    # check bugfix numbers are ignored
+    assert Version("0.4.0") == Version("0.4.2")
+
+
+def test_version_rich_comparison_ge():
+    assert Version("0.4.0") >= Version("0.3.0")
+    assert Version("0.4.0") >= Version("0.4.0")
+    assert Version("1.0.0") >= Version("0.3.0")
+
+
+def test_version_rich_comparison_gt():
+    assert Version("0.4.0") > Version("0.3.0")
+    assert Version("1.0.0") > Version("0.3.0")
+
+
+def test_version_rich_comparison_le():
+    assert Version("0.3.0") <= Version("0.4.0")
+    assert Version("0.4.0") <= Version("0.4.0")
+    assert Version("0.3.0") <= Version("1.0.0")
+
+
+def test_version_rich_comparison_lt():
+    assert Version("0.3.0") < Version("0.4.0")
+    assert Version("0.3.0") < Version("1.0.0")
+
+
+def test_version_rich_comparison_ne():
+    assert Version("1.4.0") != Version("0.4.0")
+    assert Version("0.3.0") != Version("0.4.0")
+
+    # check bugfix numbers are ignored
+    assert Version("0.3.2") != Version("0.4.2")
+
+
+def test_version_repr():
+    assert repr(Version("0.7.0")) == "Version: 0.7.0"
+
+
+def test_version_str():
+    assert str(Version("0.7.0")) == "0.7.0"

tests/test_views.py

@@ -1,6 +1,5 @@
 import io
 import json
-import shutil
 import zipfile
 from pathlib import Path
 from urllib.parse import urlencode
@@ -13,13 +12,26 @@
 from sacro import views
 
 
-TEST_PATH = Path("outputs/results.json")
+def test_outputs_annotation(test_outputs):
+    assert test_outputs.version == "0.4.0"
+    for metadata in test_outputs.values():
+        for filedata in metadata["files"]:
+            assert filedata["checksum"] is not None
+            assert filedata["checksum_valid"] is True
+            assert filedata["url"].startswith("/contents/?path=")
 
 
-@pytest.fixture
-def test_outputs(tmp_path):
-    shutil.copytree(TEST_PATH.parent, tmp_path, dirs_exist_ok=True)
-    return views.Outputs(tmp_path / TEST_PATH.name)
+def test_outputs_annotation_checksum_failed(test_outputs):
+    first_output = list(test_outputs)[0]
+    first_file = test_outputs[first_output]["files"][0]["name"]
+    checksum = test_outputs.path.parent / "checksums" / (first_file + ".txt")
+    checksum.write_text("bad checksum")
+
+    # re-annotate
+    test_outputs.annotate()
+
+    assert test_outputs[first_output]["files"][0]["checksum"] == "bad checksum"
+    assert test_outputs[first_output]["files"][0]["checksum_valid"] is False
 
 
 def test_index(test_outputs):
@@ -34,7 +46,7 @@ def test_index(test_outputs):
 
 
 @override_settings(DEBUG=True)
-def test_index_no_path():
+def test_index_no_path(TEST_PATH):
     request = RequestFactory().get(path="/")
 
     response = views.index(request)