From b6a97686afbfaa87f45e822636354feb1adaad5e Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Thu, 14 Mar 2024 19:04:29 +0000
Subject: [PATCH 1/5] Use iframe to display all file contents.

This means changing the contents url for files to render csv/txt files
as html, and just serve up other files.

Also, I split the download parts of serve_file into its own function, as
serve_file is more complex, and the caching logic doesn't apply to
downloads.

Changed some test assertions to assert that the url to the content is
present, rather than the content itself.
---
 airlock/file_browser_api.py                  | 15 ----
 airlock/templates/file_browser/contents.html | 28 ++-----
 airlock/templates/file_browser/csv.html      | 23 ++++--
 airlock/templates/file_browser/index.html    | 11 ---
 airlock/views/helpers.py                     | 79 +++++++++++++++++---
 airlock/views/request.py                     | 16 ++--
 airlock/views/workspace.py                   |  2 +-
 pyproject.toml                               |  3 +-
 tests/functional/test_e2e.py                 | 22 ++++--
 tests/integration/views/test_helpers.py      | 68 +++++++++++++++--
 tests/integration/views/test_request.py      | 11 ++-
 tests/integration/views/test_workspace.py    | 18 +++--
 12 files changed, 199 insertions(+), 97 deletions(-)

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..3b279758 100644
--- a/airlock/templates/file_browser/contents.html
+++ b/airlock/templates/file_browser/contents.html
@@ -59,28 +59,12 @@
     {% #card title=path_item.name container=True custom_button=add_button %}
 
       <div class="content">
-        {% if path_item.display_type == "iframe" %}
-          <iframe
-            src="{{ path_item.contents_url }}"
-            title="{{ path_item.relpath }}"
-            frameborder=0
-            height=1000
-            style="width: 100%;"
-          >
-          </iframe>
-        {% elif path_item.display_type == "image" %}
-          <div class="content-scroller">
-            <img src="{{ path_item.contents_url }}">
-          </div>
-        {% elif path_item.display_type == "table" %}
-          <div class="content-scroller">
-            {% include "file_browser/csv.html" with contents=path_item.contents %}
-          </div>
-        {% else %}
-          <div class="content-scroller">
-                <pre>{{ path_item.contents }}</pre>
-          </div>
-        {% endif %}
+        <iframe src="{{ path_item.contents_url }}"
+                title="{{ path_item.relpath }}"
+                frameborder=0
+                height=1000
+                style="width: 100%;"
+        />
       </div>
     {% /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 %}
-<div id="csvtable" class="inline-block min-w-full align-middle max-w-full">
+<style>
+  .datatable thead {
+    position: sticky;
+    top: 0;
+    text-align: left;
+    background-color: rgba(248,250,252);
+  }
+
+  .datatable tbody tr:nth-child(even) {
+    background-color: rgba(248,250,252);
+  }
+</style>
+
+<div id="csvtable">
   <p class="spinner">Loading table data...</p>
   <table class="datatable" style="display: none">
-    <thead class="bg-slate-200" id="csvTable">
+    <thead>
       <tr>
-        {% for header in csv_data.headers %}
+        {% for header in headers %}
           <th>{{ header }}</th>
         {% endfor %}
       </tr>
     </thead>
     <tbody>
-      {% for row in csv_data.rows %}
+      {% for row in rows %}
         <tr>
           {% for cell in row %}
             <td>{{ cell }}</td>
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);
-    }
-
-
   </style>
 {% endblock extra_styles %}
 
diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py
index b8446694..c75b1728 100644
--- a/airlock/views/helpers.py
+++ b/airlock/views/helpers.py
@@ -1,9 +1,11 @@
+import csv
 from email.utils import formatdate
 
 from django.core.exceptions import PermissionDenied
-from django.http import FileResponse, Http404
+from django.http import FileResponse, Http404, HttpResponse
+from django.template.response import TemplateResponse
 
-from airlock.business_logic import bll
+from airlock.business_logic import UrlPath, bll
 
 
 def login_exempt(view):
@@ -35,16 +37,73 @@ def get_release_request_or_raise(user, request_id):
     return release_request
 
 
-def serve_file(abspath, download=False, filename=None):
-    stat = abspath.stat()
+def render_csv(request, abspath):
+    reader = csv.reader(abspath.open())
+    headers = next(reader)
+
+    return TemplateResponse(
+        request,
+        "file_browser/csv.html",
+        {
+            "headers": headers,
+            "rows": list(reader),
+        },
+    )
+
+
+def render_text(request, abspath):
+    # TODO: synatax highlighting?
+    content = f"<pre>{abspath.read_text()}</pre>"
+    return HttpResponse(
+        content,
+        content_type="text/html",
+        headers={"Content-Length": len(content)},
+    )
+
+
+FILE_RENDERERS = {
+    ".csv": render_csv,
+    ".log": render_text,
+    ".txt": render_text,
+    ".json": render_text,
+}
+
+
+def download_file(abspath, filename=None):
+    return FileResponse(abspath.open("rb"), as_attachment=True, filename=filename)
+
+
+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.
+    """
     # use same ETag format as whitenoise
+    stat = abspath.stat()
     headers = {
         "Last-Modified": formatdate(stat.st_mtime, usegmt=True),
         "ETag": f'"{int(stat.st_mtime):x}-{stat.st_size:x}"',
     }
-    return FileResponse(
-        abspath.open("rb"),
-        headers=headers,
-        as_attachment=download,
-        filename=filename,
-    )
+
+    if filename:
+        suffix = UrlPath(filename).suffix
+    else:
+        suffix = abspath.suffix
+
+    if not suffix:
+        raise Exception(
+            f"Cannot serve file {abspath}, filename {filename}, as there is no suffix on either"
+        )
+
+    renderer = FILE_RENDERERS.get(suffix)
+    if renderer:
+        response = renderer(request, abspath)
+    else:
+        response = FileResponse(abspath.open("rb"), filename=filename)
+
+    for k, v in headers.items():
+        response.headers[k] = v
+
+    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..8edbf68a 100644
--- a/tests/integration/views/test_helpers.py
+++ b/tests/integration/views/test_helpers.py
@@ -1,16 +1,70 @@
 import os
 
+import pytest
+from django.test import RequestFactory
+from django.template.response import TemplateResponse
+
 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",
+    [
+        (".html", "text/html"),
+        (".csv", "text/html"),
+        (".png", "image/png"),
+        (".txt", "text/html"),
+    ],
+)
+def test_serve_file_rendered(tmp_path, suffix, mimetype):
+    rf = RequestFactory()
+    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))
 
-    response = helpers.serve_file(path)
+    # test rendered content for iframe
+    response = helpers.serve_file(rf.get("/"), path)
+    if isinstance(response, TemplateResponse):
+        # ensure template is actually rendered, for template coverage
+        response.render()
+
     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["Etag"] == '"65e73ba8-b"'
+    assert response.headers["Content-Type"].split(";")[0] == mimetype
+
+
+@pytest.mark.parametrize(
+    "filename,mimetype",
+    [
+        ("foo.html", "text/html"),
+        ("foo.csv", "text/html"),
+        ("foo.png", "image/png"),
+        ("foo.txt", "text/html"),
+    ],
+)
+def test_serve_file_filename(tmp_path, filename, mimetype):
+    rf = RequestFactory()
+    path = tmp_path / "hashed_file"
+    path.write_text("data")
+    time = 1709652904  # date this test was written
+    os.utime(path, (time, time))
+
+    response = helpers.serve_file(rf.get("/"), path, filename=filename)
+    assert response.headers["Last-Modified"] == "Tue, 05 Mar 2024 15:35:04 GMT"
+    assert response.headers["Etag"] == '"65e73ba8-4"'
+    assert response.headers["Content-Type"].split(";")[0] == mimetype
+
+
+def test_serve_file_no_suffix(tmp_path):
+    rf = RequestFactory()
+
+    path = tmp_path / "nosuffix"
+    path.touch()
+
+    with pytest.raises(Exception):
+        helpers.serve_file(rf.get("/"), path)
+
+    with pytest.raises(Exception):
+        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..de7ca769 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 '<ul id="tree"' not in response.rendered_content
 
@@ -147,8 +151,7 @@ def test_request_contents_file(airlock_client):
     )
     response = airlock_client.get("/requests/content/id/default/file.txt")
     assert response.status_code == 200
-    assert not response.as_attachment
-    assert list(response.streaming_content) == [b"test"]
+    assert response.content == b"<pre>test</pre>"
 
 
 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..a9707f98 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 '<ul id="tree"' not in response.rendered_content
     assert "HX-Request" in response.headers["Vary"]
@@ -126,10 +128,10 @@ def test_workspace_view_with_svg_file(airlock_client):
 
 def test_workspace_view_with_csv_file(airlock_client):
     airlock_client.login(output_checker=True)
-    factories.write_workspace_file("workspace", "file.csv", "header1,header2\nFoo,Bar")
+    workspace = factories.create_workspace("workspace")
+    factories.write_workspace_file(workspace, "file.csv", "header1,header2\nFoo,Bar")
     response = airlock_client.get("/workspaces/view/workspace/file.csv")
-    for content in ["<table", "Foo", "Bar"]:
-        assert content in response.rendered_content
+    assert workspace.get_contents_url("file.csv") in response.rendered_content
 
 
 def test_workspace_view_with_404(airlock_client):
@@ -214,7 +216,7 @@ def test_workspace_contents_file(airlock_client):
     factories.write_workspace_file("workspace", "file.txt", "test")
     response = airlock_client.get("/workspaces/content/workspace/file.txt")
     assert response.status_code == 200
-    assert list(response.streaming_content) == [b"test"]
+    assert response.content == b"<pre>test</pre>"
 
 
 def test_workspace_contents_dir(airlock_client):

From a01c60dca50a3ac090438fa1f6aa06107abc192b Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Thu, 14 Mar 2024 19:59:58 +0000
Subject: [PATCH 2/5] Implement conditional GETs for file contents

This means we will return 304s if the file contents have not changed,
which means the browser cache will be used, and we don't need to render
the content. For larger files, especially csv or text, this avoids a lot
of work.

For content that we render (text and csv atm), the etag is a bit more
involved. We want the etag value to capture both the original file and
the template file that was used. This is so that if either changes, the
cache is invalid and the new content is served.

To do this, I added a micro rendering framework that ties the templating
together with the rendering, so that we can inspect the template and
check *before* doing the potentially expensive rendering.
---
 airlock/templates/file_browser/text.html  |   3 +
 airlock/views/helpers.py                  | 116 ++++++++++++++--------
 tests/integration/views/test_helpers.py   |  85 +++++++++++-----
 tests/integration/views/test_request.py   |   2 +-
 tests/integration/views/test_workspace.py |   2 +-
 5 files changed, 141 insertions(+), 67 deletions(-)
 create mode 100644 airlock/templates/file_browser/text.html

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 @@
+<pre class="{{ class }}">
+{{ text }}
+</pre>
diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py
index c75b1728..b7ad08c9 100644
--- a/airlock/views/helpers.py
+++ b/airlock/views/helpers.py
@@ -1,13 +1,20 @@
 import csv
-from email.utils import formatdate
+import functools
+from email.utils import formatdate, parsedate
+from pathlib import Path
 
 from django.core.exceptions import PermissionDenied
-from django.http import FileResponse, Http404, HttpResponse
-from django.template.response import TemplateResponse
+from django.http import FileResponse, Http404, HttpResponseNotModified
+from django.template import loader
+from django.template.response import SimpleTemplateResponse
 
 from airlock.business_logic import UrlPath, bll
 
 
+class ServeFileException(Exception):
+    pass
+
+
 def login_exempt(view):
     view.login_exempt = True
     return view
@@ -37,40 +44,69 @@ def get_release_request_or_raise(user, request_id):
     return release_request
 
 
-def render_csv(request, abspath):
+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}
 
-    return TemplateResponse(
-        request,
-        "file_browser/csv.html",
-        {
-            "headers": headers,
-            "rows": list(reader),
-        },
-    )
 
-
-def render_text(request, abspath):
-    # TODO: synatax highlighting?
-    content = f"<pre>{abspath.read_text()}</pre>"
-    return HttpResponse(
-        content,
-        content_type="text/html",
-        headers={"Content-Length": len(content)},
-    )
+@render_with_template("file_browser/text.html")
+def text_renderer(abspath, suffix):
+    return {
+        "text": abspath.read_text(),
+        "class": suffix.lstrip("."),
+    }
 
 
 FILE_RENDERERS = {
-    ".csv": render_csv,
-    ".log": render_text,
-    ".txt": render_text,
-    ".json": render_text,
+    ".csv": csv_renderer,
+    ".log": text_renderer,
+    ".txt": text_renderer,
+    ".json": text_renderer,
 }
 
 
-def download_file(abspath, filename=None):
-    return FileResponse(abspath.open("rb"), as_attachment=True, filename=filename)
+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):
@@ -80,30 +116,32 @@ def serve_file(request, abspath, filename=None):
 
     For csv and text, render that to html then serve.
     """
-    # use same ETag format as whitenoise
-    stat = abspath.stat()
-    headers = {
-        "Last-Modified": formatdate(stat.st_mtime, usegmt=True),
-        "ETag": f'"{int(stat.st_mtime):x}-{stat.st_size:x}"',
-    }
-
     if filename:
         suffix = UrlPath(filename).suffix
     else:
         suffix = abspath.suffix
 
     if not suffix:
-        raise Exception(
+        raise ServeFileException(
             f"Cannot serve file {abspath}, filename {filename}, as there is no suffix on either"
         )
 
     renderer = FILE_RENDERERS.get(suffix)
-    if renderer:
-        response = renderer(request, abspath)
+    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)
 
-    for k, v in headers.items():
-        response.headers[k] = v
+    response.headers["Last-Modified"] = last_modified
+    response.headers["ETag"] = etag
 
     return response
diff --git a/tests/integration/views/test_helpers.py b/tests/integration/views/test_helpers.py
index 8edbf68a..489c7f26 100644
--- a/tests/integration/views/test_helpers.py
+++ b/tests/integration/views/test_helpers.py
@@ -2,59 +2,92 @@
 
 import pytest
 from django.test import RequestFactory
-from django.template.response import TemplateResponse
 
 from airlock.views import helpers
 
 
 @pytest.mark.parametrize(
-    "suffix,mimetype",
+    "suffix,mimetype,template_path",
     [
-        (".html", "text/html"),
-        (".csv", "text/html"),
-        (".png", "image/png"),
-        (".txt", "text/html"),
+        (".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, suffix, mimetype):
+def test_serve_file_rendered(tmp_path, suffix, mimetype, template_path):
     rf = RequestFactory()
+
     path = tmp_path / ("test" + suffix)
-    # use a csv as test data, it renders fine as text
+    # 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))
 
-    # test rendered content for iframe
+    etag = helpers.build_etag(path.stat(), template_path)
+
     response = helpers.serve_file(rf.get("/"), path)
-    if isinstance(response, TemplateResponse):
+    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["Etag"] == '"65e73ba8-b"'
     assert response.headers["Content-Type"].split(";")[0] == mimetype
+    assert response.headers["Etag"] == etag
+
+
+def test_serve_file_rendered_with_filename(tmp_path):
+    rf = RequestFactory()
+
+    path = tmp_path / "hash"
+    path.write_text("data")
+
+    time = 1709652904  # date this test was written
+    os.utime(path, (time, time))
+
+    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["Content-Type"].split(";")[0] == "text/html"
+    assert response.headers["Etag"] == '"65e73ba8-4"'
 
 
 @pytest.mark.parametrize(
-    "filename,mimetype",
+    "suffix,template_path",
     [
-        ("foo.html", "text/html"),
-        ("foo.csv", "text/html"),
-        ("foo.png", "image/png"),
-        ("foo.txt", "text/html"),
+        (".html", None),
+        (".png", None),
+        (".csv", "airlock/templates/file_browser/csv.html"),
+        (".txt", "airlock/templates/file_browser/text.html"),
     ],
 )
-def test_serve_file_filename(tmp_path, filename, mimetype):
+def test_serve_file_not_modified(tmp_path, suffix, template_path):
     rf = RequestFactory()
-    path = tmp_path / "hashed_file"
-    path.write_text("data")
+    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))
 
-    response = helpers.serve_file(rf.get("/"), path, filename=filename)
-    assert response.headers["Last-Modified"] == "Tue, 05 Mar 2024 15:35:04 GMT"
-    assert response.headers["Etag"] == '"65e73ba8-4"'
-    assert response.headers["Content-Type"].split(";")[0] == mimetype
+    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):
@@ -63,8 +96,8 @@ def test_serve_file_no_suffix(tmp_path):
     path = tmp_path / "nosuffix"
     path.touch()
 
-    with pytest.raises(Exception):
+    with pytest.raises(helpers.ServeFileException):
         helpers.serve_file(rf.get("/"), path)
 
-    with pytest.raises(Exception):
-        helpers.serve_file(rf.get(), path, filename="nosuffix")
+    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 de7ca769..79b54c6f 100644
--- a/tests/integration/views/test_request.py
+++ b/tests/integration/views/test_request.py
@@ -151,7 +151,7 @@ def test_request_contents_file(airlock_client):
     )
     response = airlock_client.get("/requests/content/id/default/file.txt")
     assert response.status_code == 200
-    assert response.content == b"<pre>test</pre>"
+    assert response.content == b'<pre class="txt">\ntest\n</pre>\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 a9707f98..2e5d0429 100644
--- a/tests/integration/views/test_workspace.py
+++ b/tests/integration/views/test_workspace.py
@@ -216,7 +216,7 @@ def test_workspace_contents_file(airlock_client):
     factories.write_workspace_file("workspace", "file.txt", "test")
     response = airlock_client.get("/workspaces/content/workspace/file.txt")
     assert response.status_code == 200
-    assert response.content == b"<pre>test</pre>"
+    assert response.content == b'<pre class="txt">\ntest\n</pre>\n'
 
 
 def test_workspace_contents_dir(airlock_client):

From e4bbf4df5bf95046329e8f11cabd73ab8435e75a Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Fri, 15 Mar 2024 11:52:57 +0000
Subject: [PATCH 3/5] Remove templatetags as no longer used

---
 airlock/templatetags/__init__.py    |  0
 airlock/templatetags/airlocktags.py | 15 ---------------
 tests/unit/test_templatetags.py     |  9 ---------
 3 files changed, 24 deletions(-)
 delete mode 100644 airlock/templatetags/__init__.py
 delete mode 100644 airlock/templatetags/airlocktags.py
 delete mode 100644 tests/unit/test_templatetags.py

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/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"]],
-    }

From 688e9e7d407f694ce016ca7ae22fd1d416a22d15 Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Fri, 15 Mar 2024 16:12:26 +0000
Subject: [PATCH 4/5] Add a View link, that will open just the file in a new
 tab

This helps view larger files in detail
---
 airlock/templates/file_browser/contents.html | 67 ++++++++++----------
 1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/airlock/templates/file_browser/contents.html b/airlock/templates/file_browser/contents.html
index 3b279758..6cfdad05 100644
--- a/airlock/templates/file_browser/contents.html
+++ b/airlock/templates/file_browser/contents.html
@@ -21,39 +21,42 @@
 
   {% 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" %}
-              <form action="{{ request_file_url }}" method="POST" aria-label="add-file-form">
-                {% csrf_token %}
-                {% form_select class="w-full max-w-lg mx-auto" label="Select a file group" field=form.filegroup choices=form.filegroup.field.choices %}
-                {% form_input class="w-full max-w-lg mx-auto" label="Or create a new file group" field=form.new_filegroup %}
-                <input type=hidden name="path" value="{{ path_item.relpath }}"/>
-                <div class="mt-2">
-                  {% #button type="submit" variant="success" id="add-file-button" %}Add File to Request{% /button %}
-                  {% #button variant="danger" type="cancel" %}Cancel{% /button %}
-                </div>
-              </form>
-            {% /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 %}
+      <div class="flex items-center gap-2">
+        {% if context == "workspace" %}
+          {% if form %}
+            {% #modal id="addRequestFile" button_text="Add File to Request" variant="success" %}
+              {% #card container=True title="Add a file" %}
+                <form action="{{ request_file_url }}" method="POST" aria-label="add-file-form">
+                  {% csrf_token %}
+                  {% form_select class="w-full max-w-lg mx-auto" label="Select a file group" field=form.filegroup choices=form.filegroup.field.choices %}
+                  {% form_input class="w-full max-w-lg mx-auto" label="Or create a new file group" field=form.new_filegroup %}
+                  <input type=hidden name="path" value="{{ path_item.relpath }}"/>
+                  <div class="mt-2">
+                    {% #button type="submit" variant="success" id="add-file-button" %}Add File to Request{% /button %}
+                    {% #button variant="danger" type="cancel" %}Cancel{% /button %}
+                  </div>
+                </form>
+              {% /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 %}
+          <form action="" method="POST">
+            {% csrf_token %}
+            {% #button type="submit" tooltip="Remove this file from this request" variant="warning" %}Remove File from Request{% /button %}
+          </form>
+        {% elif is_output_checker %}
+          {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %}
         {% endif %}
-      {% elif is_author %}
-        <form action="" method="POST">
-          {% csrf_token %}
-          {% #button type="submit" tooltip="Remove this file from this request" variant="warning" %}Remove File from Request{% /button %}
-        </form>
-      {% 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 %}
+      </div>
     {% endfragment %}
 
     {% #card title=path_item.name container=True custom_button=add_button %}

From 59bc383ba7ae1854ba9e61409e1dfa8ebd7c7fef Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Mon, 18 Mar 2024 10:59:20 +0000
Subject: [PATCH 5/5] Use `rf` fixture instead of manual RequestFactory()

---
 tests/integration/views/test_helpers.py | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/tests/integration/views/test_helpers.py b/tests/integration/views/test_helpers.py
index 489c7f26..d07a11b7 100644
--- a/tests/integration/views/test_helpers.py
+++ b/tests/integration/views/test_helpers.py
@@ -1,7 +1,6 @@
 import os
 
 import pytest
-from django.test import RequestFactory
 
 from airlock.views import helpers
 
@@ -15,9 +14,7 @@
         (".txt", "text/html", "airlock/templates/file_browser/text.html"),
     ],
 )
-def test_serve_file_rendered(tmp_path, suffix, mimetype, template_path):
-    rf = RequestFactory()
-
+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")
@@ -38,9 +35,7 @@ def test_serve_file_rendered(tmp_path, suffix, mimetype, template_path):
     assert response.headers["Etag"] == etag
 
 
-def test_serve_file_rendered_with_filename(tmp_path):
-    rf = RequestFactory()
-
+def test_serve_file_rendered_with_filename(tmp_path, rf):
     path = tmp_path / "hash"
     path.write_text("data")
 
@@ -63,8 +58,7 @@ def test_serve_file_rendered_with_filename(tmp_path):
         (".txt", "airlock/templates/file_browser/text.html"),
     ],
 )
-def test_serve_file_not_modified(tmp_path, suffix, template_path):
-    rf = RequestFactory()
+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")
@@ -90,9 +84,7 @@ def test_serve_file_not_modified(tmp_path, suffix, template_path):
     assert response.status_code == 200
 
 
-def test_serve_file_no_suffix(tmp_path):
-    rf = RequestFactory()
-
+def test_serve_file_no_suffix(tmp_path, rf):
     path = tmp_path / "nosuffix"
     path.touch()