diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py
index 5e999d5c..8e4c30b9 100644
--- a/airlock/file_browser_api.py
+++ b/airlock/file_browser_api.py
@@ -47,18 +47,6 @@ class PathNotFound(Exception):
# but this allow it to be overridden.
display_text: str = None
- DISPLAY_TYPES = {
- "html": "iframe",
- "jpeg": "image",
- "jpg": "image",
- "png": "image",
- "svg": "image",
- "csv": "table",
- "tsv": "table",
- "txt": "preformatted",
- "log": "preformatted",
- }
-
def __post_init__(self):
# ensure is UrlPath
self.relpath = UrlPath(self.relpath)
@@ -121,9 +109,6 @@ def suffix(self):
def file_type(self):
return self.suffix().lstrip(".")
- def display_type(self):
- return self.DISPLAY_TYPES.get(self.file_type(), "preformatted")
-
def breadcrumbs(self):
item = self
crumbs = [item]
diff --git a/airlock/templates/file_browser/contents.html b/airlock/templates/file_browser/contents.html
index 96f7bced..6cfdad05 100644
--- a/airlock/templates/file_browser/contents.html
+++ b/airlock/templates/file_browser/contents.html
@@ -21,66 +21,53 @@
{% else %}
{% fragment as add_button %}
- {% if context == "workspace" %}
- {% if form %}
- {% #modal id="addRequestFile" button_text="Add File to Request" variant="success" %}
- {% #card container=True title="Add a file" %}
-
- {% /card %}
- {% /modal %}
- {% elif file_in_request %}
- {% #button type="button" disabled=True tooltip="This file has already been added to the current request" id="add-file-modal-button-disabled" %}
- Add File to Request
- {% /button %}
- {% else %}
- {% #button type="button" disabled=True tooltip="You do not have permission to add this file to a request" id="add-file-modal-button-disabled" %}
- Add File to Request
- {% /button %}
+
+ {% if context == "workspace" %}
+ {% if form %}
+ {% #modal id="addRequestFile" button_text="Add File to Request" variant="success" %}
+ {% #card container=True title="Add a file" %}
+
+ {% /card %}
+ {% /modal %}
+ {% elif file_in_request %}
+ {% #button type="button" disabled=True tooltip="This file has already been added to the current request" id="add-file-modal-button-disabled" %}
+ Add File to Request
+ {% /button %}
+ {% else %}
+ {% #button type="button" disabled=True tooltip="You do not have permission to add this file to a request" id="add-file-modal-button-disabled" %}
+ Add File to Request
+ {% /button %}
+ {% endif %}
+ {% elif is_author %}
+
+ {% elif is_output_checker %}
+ {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %}
{% endif %}
- {% elif is_author %}
-
- {% elif is_output_checker %}
- {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %}
- {% endif %}
+ {% #button variant="primary" type="link" href=path_item.contents_url external=True id="view-button" %}View ↗{% /button %}
+
{% endfragment %}
{% #card title=path_item.name container=True custom_button=add_button %}
- {% if path_item.display_type == "iframe" %}
-
- {% elif path_item.display_type == "image" %}
-
-
-
- {% elif path_item.display_type == "table" %}
-
- {% include "file_browser/csv.html" with contents=path_item.contents %}
-
- {% else %}
-
-
{{ path_item.contents }}
-
- {% endif %}
+
{% /card %}
diff --git a/airlock/templates/file_browser/csv.html b/airlock/templates/file_browser/csv.html
index 7ecc7d55..7fa3053d 100644
--- a/airlock/templates/file_browser/csv.html
+++ b/airlock/templates/file_browser/csv.html
@@ -1,17 +1,28 @@
-{% load airlocktags %}
-{% as_csv_data contents as csv_data %}
-
+
+
+
Loading table data...
-
+
- {% for header in csv_data.headers %}
+ {% for header in headers %}
{{ header }} |
{% endfor %}
- {% for row in csv_data.rows %}
+ {% for row in rows %}
{% for cell in row %}
{{ cell }} |
diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html
index 8d7feb98..f02622d7 100644
--- a/airlock/templates/file_browser/index.html
+++ b/airlock/templates/file_browser/index.html
@@ -61,17 +61,6 @@
overflow: scroll;
}
- .datatable thead {
- position: sticky;
- top: 0;
- text-align: left;
- }
-
- .datatable tbody tr:nth-child(even) {
- background-color: rgba(248,250,252);
- }
-
-
{% endblock extra_styles %}
diff --git a/airlock/templates/file_browser/text.html b/airlock/templates/file_browser/text.html
new file mode 100644
index 00000000..a169eaa0
--- /dev/null
+++ b/airlock/templates/file_browser/text.html
@@ -0,0 +1,3 @@
+
+{{ text }}
+
diff --git a/airlock/templatetags/__init__.py b/airlock/templatetags/__init__.py
deleted file mode 100644
index e69de29b..00000000
diff --git a/airlock/templatetags/airlocktags.py b/airlock/templatetags/airlocktags.py
deleted file mode 100644
index 1124d40c..00000000
--- a/airlock/templatetags/airlocktags.py
+++ /dev/null
@@ -1,15 +0,0 @@
-import csv
-
-from django import template
-
-
-register = template.Library()
-
-
-@register.simple_tag
-def as_csv_data(file_content):
- reader = csv.reader(file_content.splitlines(keepends=True))
- return {
- "headers": next(reader),
- "rows": list(reader),
- }
diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py
index b8446694..b7ad08c9 100644
--- a/airlock/views/helpers.py
+++ b/airlock/views/helpers.py
@@ -1,9 +1,18 @@
-from email.utils import formatdate
+import csv
+import functools
+from email.utils import formatdate, parsedate
+from pathlib import Path
from django.core.exceptions import PermissionDenied
-from django.http import FileResponse, Http404
+from django.http import FileResponse, Http404, HttpResponseNotModified
+from django.template import loader
+from django.template.response import SimpleTemplateResponse
-from airlock.business_logic import bll
+from airlock.business_logic import UrlPath, bll
+
+
+class ServeFileException(Exception):
+ pass
def login_exempt(view):
@@ -35,16 +44,104 @@ def get_release_request_or_raise(user, request_id):
return release_request
-def serve_file(abspath, download=False, filename=None):
- stat = abspath.stat()
- # use same ETag format as whitenoise
- headers = {
- "Last-Modified": formatdate(stat.st_mtime, usegmt=True),
- "ETag": f'"{int(stat.st_mtime):x}-{stat.st_size:x}"',
+def download_file(abspath, filename=None):
+ """Simple Helper to download file."""
+ return FileResponse(abspath.open("rb"), as_attachment=True, filename=filename)
+
+
+def render_with_template(template):
+ """Micro framework for rendering content.
+
+ The main purpose is to be able to check to see if the template has changed
+ ahead of calling the render function and doing the work.
+
+ It loads the template path used, and stores it on the wrapper function
+ object for later inspection.
+ """
+ django_template = loader.get_template(template)
+ template_path = Path(django_template.template.origin.name)
+
+ def decorator(func):
+ @functools.wraps(func)
+ def wrapper(abspath, suffix):
+ context = func(abspath, suffix)
+ return SimpleTemplateResponse(template, context)
+
+ wrapper.template_path = template_path
+
+ return wrapper
+
+ return decorator
+
+
+@render_with_template("file_browser/csv.html")
+def csv_renderer(abspath, suffix):
+ reader = csv.reader(abspath.open())
+ headers = next(reader)
+ return {"headers": headers, "rows": reader}
+
+
+@render_with_template("file_browser/text.html")
+def text_renderer(abspath, suffix):
+ return {
+ "text": abspath.read_text(),
+ "class": suffix.lstrip("."),
}
- return FileResponse(
- abspath.open("rb"),
- headers=headers,
- as_attachment=download,
- filename=filename,
- )
+
+
+FILE_RENDERERS = {
+ ".csv": csv_renderer,
+ ".log": text_renderer,
+ ".txt": text_renderer,
+ ".json": text_renderer,
+}
+
+
+def build_etag(content_stat, template=None):
+ # Like whitenoise, use filesystem metadata rather than hash as its faster
+ etag = f"{int(content_stat.st_mtime):x}-{content_stat.st_size:x}"
+ if template:
+ # add the renderer's template etag so cache is invalidated if we change it
+ template_stat = Path(template).stat()
+ etag = f"{etag}-{int(template_stat.st_mtime):x}-{template_stat.st_size:x}"
+
+ # quote as per spec
+ return f'"{etag}"'
+
+
+def serve_file(request, abspath, filename=None):
+ """Serve file contents in a form the browser can render.
+
+ For html and images, just serve directly.
+
+ For csv and text, render that to html then serve.
+ """
+ if filename:
+ suffix = UrlPath(filename).suffix
+ else:
+ suffix = abspath.suffix
+
+ if not suffix:
+ raise ServeFileException(
+ f"Cannot serve file {abspath}, filename {filename}, as there is no suffix on either"
+ )
+
+ renderer = FILE_RENDERERS.get(suffix)
+ stat = abspath.stat()
+ last_modified = formatdate(stat.st_mtime, usegmt=True)
+ etag = build_etag(stat, getattr(renderer, "template_path", None))
+ last_requested = request.headers.get("If-Modified-Since")
+
+ if request.headers.get("If-None-Match") == etag:
+ response = HttpResponseNotModified()
+ elif last_requested and parsedate(last_requested) >= parsedate(last_modified):
+ response = HttpResponseNotModified()
+ elif renderer:
+ response = renderer(abspath, suffix)
+ else:
+ response = FileResponse(abspath.open("rb"), filename=filename)
+
+ response.headers["Last-Modified"] = last_modified
+ response.headers["ETag"] = etag
+
+ return response
diff --git a/airlock/views/request.py b/airlock/views/request.py
index 71fa346b..41418d0d 100644
--- a/airlock/views/request.py
+++ b/airlock/views/request.py
@@ -12,7 +12,7 @@
from airlock.business_logic import Status, bll
from airlock.file_browser_api import get_request_tree
-from .helpers import get_release_request_or_raise, serve_file
+from .helpers import download_file, get_release_request_or_raise, serve_file
def request_index(request):
@@ -104,13 +104,15 @@ def request_contents(request, request_id: str, path: str):
# Downloads are only allowed for output checkers
# Downloads are not allowed for request authors (including those that are also
# output checkers)
- if download and (
- not request.user.output_checker
- or (release_request.author == request.user.username)
- ):
- raise PermissionDenied()
+ if download:
+ if not request.user.output_checker or (
+ release_request.author == request.user.username
+ ):
+ raise PermissionDenied()
- return serve_file(abspath, download, filename=path)
+ return download_file(abspath, filename=path)
+
+ return serve_file(request, abspath, filename=path)
@require_http_methods(["POST"])
diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py
index 353f7be9..9341ff96 100644
--- a/airlock/views/workspace.py
+++ b/airlock/views/workspace.py
@@ -107,7 +107,7 @@ def workspace_contents(request, workspace_name: str, path: str):
if not abspath.is_file():
return HttpResponseBadRequest()
- return serve_file(abspath)
+ return serve_file(request, abspath)
@require_http_methods(["POST"])
diff --git a/pyproject.toml b/pyproject.toml
index 76698d65..aa7cc48f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -31,7 +31,8 @@ branch = true
# Required to get full coverage when using Playwright
concurrency = ["greenlet", "thread"]
plugins = ["django_coverage_plugin"]
-omit = ["*/assets/*",]
+omit = ["*/assets/*"]
+
[tool.coverage.report]
fail_under = 100
diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py
index 0d329579..c549f428 100644
--- a/tests/functional/test_e2e.py
+++ b/tests/functional/test_e2e.py
@@ -69,8 +69,9 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
Test full Airlock process to create, submit and release files
"""
# set up a workspace file in a subdirectory
+ workspace = factories.create_workspace("test-workspace")
factories.write_workspace_file(
- "test-workspace", "subdir/file.txt", "I am the file content"
+ workspace, "subdir/file.txt", "I am the file content"
)
# Log in as a researcher
@@ -92,7 +93,10 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
# Click on the first link we find
find_and_click(page.get_by_role("link", name="subdir").first)
find_and_click(page.get_by_role("link", name="file.txt").first)
- expect(page.locator("body")).to_contain_text("I am the file content")
+
+ expect(page.locator("iframe")).to_have_attribute(
+ "src", workspace.get_contents_url("subdir/file.txt")
+ )
# Add file to request, with custom named group
# Find the add file button and click on it to open the modal
@@ -106,7 +110,9 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
expect(page).to_have_url(
f"{live_server.url}/workspaces/view/test-workspace/subdir/file.txt"
)
- expect(page.locator("body")).to_contain_text("I am the file content")
+ expect(page.locator("iframe")).to_have_attribute(
+ "src", workspace.get_contents_url("subdir/file.txt")
+ )
# The "Add file to request" button is disabled
add_file_button = page.locator("#add-file-modal-button-disabled")
@@ -119,6 +125,7 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
expect(page).to_have_url(url_regex)
# get the request ID for the just-created request, for later reference
request_id = url_regex.match(page.url).groups()[0]
+ release_request = bll.get_release_request(request_id, admin_user)
# Find the filegroup in the tree
# Note: `get_by_role`` gets all links on the page; `locator` searches
@@ -142,7 +149,10 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
# Tree opens fully expanded, so now the file (in its subdir) is visible
find_and_click(file_link)
- expect(page.locator("body")).to_contain_text("I am the file content")
+
+ expect(page.locator("iframe")).to_have_attribute(
+ "src", release_request.get_contents_url("my-new-group/subdir/file.txt")
+ )
# Submit request
submit_button = page.locator("#submit-for-review-button")
@@ -186,7 +196,9 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
# Click to open the filegroup tree
find_and_click(filegroup_link)
find_and_click(file_link)
- expect(page.locator("body")).to_contain_text("I am the file content")
+ expect(page.locator("iframe")).to_have_attribute(
+ "src", release_request.get_contents_url("my-new-group/subdir/file.txt")
+ )
# Download the file
with page.expect_download() as download_info:
diff --git a/tests/integration/views/test_helpers.py b/tests/integration/views/test_helpers.py
index eb355f1b..d07a11b7 100644
--- a/tests/integration/views/test_helpers.py
+++ b/tests/integration/views/test_helpers.py
@@ -1,16 +1,95 @@
import os
+import pytest
+
from airlock.views import helpers
-def test_serve_file(tmp_path):
- path = tmp_path / "test.txt"
- path.write_text("hi")
+@pytest.mark.parametrize(
+ "suffix,mimetype,template_path",
+ [
+ (".html", "text/html", None),
+ (".png", "image/png", None),
+ (".csv", "text/html", "airlock/templates/file_browser/csv.html"),
+ (".txt", "text/html", "airlock/templates/file_browser/text.html"),
+ ],
+)
+def test_serve_file_rendered(tmp_path, rf, suffix, mimetype, template_path):
+ path = tmp_path / ("test" + suffix)
+ # use a csv as test data, it works for other types too
+ path.write_text("a,b,c\n1,2,3")
+
+ time = 1709652904 # date this test was written
+ os.utime(path, (time, time))
+
+ etag = helpers.build_etag(path.stat(), template_path)
+
+ response = helpers.serve_file(rf.get("/"), path)
+ if hasattr(response, "render"):
+ # ensure template is actually rendered, for template coverage
+ response.render()
+
+ assert response.status_code == 200
+ assert response.headers["Last-Modified"] == "Tue, 05 Mar 2024 15:35:04 GMT"
+ assert response.headers["Content-Type"].split(";")[0] == mimetype
+ assert response.headers["Etag"] == etag
+
+
+def test_serve_file_rendered_with_filename(tmp_path, rf):
+ path = tmp_path / "hash"
+ path.write_text("data")
+
time = 1709652904 # date this test was written
os.utime(path, (time, time))
- response = helpers.serve_file(path)
+ response = helpers.serve_file(rf.get("/"), path, filename="test.html")
+ assert response.status_code == 200
assert response.headers["Last-Modified"] == "Tue, 05 Mar 2024 15:35:04 GMT"
- assert response.headers["Etag"] == '"65e73ba8-2"'
- assert response.headers["Content-Type"] == "text/plain"
- assert response.headers["Content-Length"] == "2"
+ assert response.headers["Content-Type"].split(";")[0] == "text/html"
+ assert response.headers["Etag"] == '"65e73ba8-4"'
+
+
+@pytest.mark.parametrize(
+ "suffix,template_path",
+ [
+ (".html", None),
+ (".png", None),
+ (".csv", "airlock/templates/file_browser/csv.html"),
+ (".txt", "airlock/templates/file_browser/text.html"),
+ ],
+)
+def test_serve_file_not_modified(tmp_path, rf, suffix, template_path):
+ path = tmp_path / ("test" + suffix)
+ # use a csv as test data, it renders fine as text
+ path.write_text("a,b,c\n1,2,3")
+ time = 1709652904 # date this test was written
+ os.utime(path, (time, time))
+
+ etag = helpers.build_etag(path.stat(), template_path)
+
+ request = rf.get("/", headers={"If-None-Match": etag})
+ response = helpers.serve_file(request, path)
+ assert response.status_code == 304
+
+ request = rf.get(
+ "/", headers={"If-Modified-Since": "Tue, 05 Mar 2024 15:35:04 GMT"}
+ )
+ response = helpers.serve_file(request, path)
+ assert response.status_code == 304
+
+ request = rf.get(
+ "/", headers={"If-Modified-Since": "Tue, 05 Mar 2023 15:35:04 GMT"}
+ )
+ response = helpers.serve_file(request, path)
+ assert response.status_code == 200
+
+
+def test_serve_file_no_suffix(tmp_path, rf):
+ path = tmp_path / "nosuffix"
+ path.touch()
+
+ with pytest.raises(helpers.ServeFileException):
+ helpers.serve_file(rf.get("/"), path)
+
+ with pytest.raises(helpers.ServeFileException):
+ helpers.serve_file(rf.get("/"), path, filename="nosuffix")
diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py
index 6fb19bfd..79b54c6f 100644
--- a/tests/integration/views/test_request.py
+++ b/tests/integration/views/test_request.py
@@ -53,7 +53,9 @@ def test_request_view_with_file(airlock_client):
factories.write_request_file(release_request, "group", "file.txt", "foobar")
response = airlock_client.get(f"/requests/view/{release_request.id}/group/file.txt")
assert response.status_code == 200
- assert "foobar" in response.rendered_content
+ assert (
+ release_request.get_contents_url("group/file.txt") in response.rendered_content
+ )
assert response.template_name == "file_browser/index.html"
@@ -66,7 +68,9 @@ def test_request_view_with_file_htmx(airlock_client):
headers={"HX-Request": "true"},
)
assert response.status_code == 200
- assert "foobar" in response.rendered_content
+ assert (
+ release_request.get_contents_url("group/file.txt") in response.rendered_content
+ )
assert response.template_name == "file_browser/contents.html"
assert '\ntest\n\n'
def test_request_contents_dir(airlock_client):
diff --git a/tests/integration/views/test_workspace.py b/tests/integration/views/test_workspace.py
index 3411a0cd..2e5d0429 100644
--- a/tests/integration/views/test_workspace.py
+++ b/tests/integration/views/test_workspace.py
@@ -50,22 +50,24 @@ def test_workspace_view_with_directory(airlock_client):
def test_workspace_view_with_file(airlock_client):
airlock_client.login(output_checker=True)
- factories.write_workspace_file("workspace", "file.txt", "foobar")
+ workspace = factories.create_workspace("workspace")
+ factories.write_workspace_file(workspace, "file.txt", "foobar")
response = airlock_client.get("/workspaces/view/workspace/file.txt")
assert response.status_code == 200
- assert "foobar" in response.rendered_content
+ assert workspace.get_contents_url("file.txt") in response.rendered_content
assert response.template_name == "file_browser/index.html"
assert "HX-Request" in response.headers["Vary"]
def test_workspace_view_with_file_htmx(airlock_client):
airlock_client.login(output_checker=True)
- factories.write_workspace_file("workspace", "file.txt", "foobar")
+ workspace = factories.create_workspace("workspace")
+ factories.write_workspace_file(workspace, "file.txt", "foobar")
response = airlock_client.get(
"/workspaces/view/workspace/file.txt", headers={"HX-Request": "true"}
)
assert response.status_code == 200
- assert "foobar" in response.rendered_content
+ assert workspace.get_contents_url("file.txt") in response.rendered_content
assert response.template_name == "file_browser/contents.html"
assert '\ntest\n\n'
def test_workspace_contents_dir(airlock_client):
diff --git a/tests/unit/test_templatetags.py b/tests/unit/test_templatetags.py
deleted file mode 100644
index 9cca262a..00000000
--- a/tests/unit/test_templatetags.py
+++ /dev/null
@@ -1,9 +0,0 @@
-from airlock.templatetags.airlocktags import as_csv_data
-
-
-def test_file_content_as_csv_data():
- content = "Header1,Header2,Header3\n" "One,Two,Three\n" "Four,Five,Six\n"
- assert as_csv_data(content) == {
- "headers": ["Header1", "Header2", "Header3"],
- "rows": [["One", "Two", "Three"], ["Four", "Five", "Six"]],
- }