From 1766f12c8be5d12d501963b20c0111c5132a0847 Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Wed, 21 Feb 2024 14:03:02 +0000
Subject: [PATCH 1/5] Move selected path into container

Previously, it was in PathItem, which was awkward, especially when
making changes for file group tree view. This moves it to being an
ephemeral attribute set on the container, and thus available to all
PathIems.

I added it as an attribute, so it can be set, and to do that I switched
from a regular base class to `typing.Protocol`, which allows for
specifying attributes. The original motiviation for using a base class
(code reuse) had gone away previously
---
 airlock/api.py                      | 40 +++++++++++++++++++----------
 airlock/file_browser_api.py         | 17 +++++-------
 airlock/views.py                    | 10 +++++---
 tests/unit/test_file_browser_api.py | 15 ++++++-----
 4 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/airlock/api.py b/airlock/api.py
index 734d6821f..f380d50bf 100644
--- a/airlock/api.py
+++ b/airlock/api.py
@@ -3,7 +3,7 @@
 from datetime import datetime
 from enum import Enum
 from pathlib import Path
-from typing import Optional
+from typing import Optional, Protocol
 
 from django.conf import settings
 from django.shortcuts import reverse
@@ -25,27 +25,33 @@ class Status(Enum):
     RELEASED = "RELEASED"
 
 
-class AirlockContainer:
-    """Abstract class for a directory or workspace or request files.
+class AirlockContainer(Protocol):
+    """Structural typing class for a instance of a Workspace or ReleaseRequest
 
-    Provides a uniform interface for accessing information about this directory.
+    Provides a uniform interface for accessing information the paths and files
+    container within this instance.
     """
 
-    def root(self):
-        raise NotImplementedError()
+    # The currently selected path in this container.  Used to calculate if how
+    # a particular path relates to it, i.e. if path *is* the currently selected
+    # path, or is one of its parents.
+    selected_path: Path = None
 
-    def get_id(self):
-        raise NotImplementedError()
+    def root(self) -> Path:
+        """Absolute concrete Path to root dir for files in this container."""
 
-    def get_absolute_url(self):
-        raise NotImplementedError()
+    def get_id(self) -> str:
+        """Get the human name for this container."""
 
-    def get_url_for_path(self):
-        raise NotImplementedError()
+    def get_absolute_url(self) -> str:
+        """Get the url for the container object"""
+
+    def get_url_for_path(self, path: Path) -> str:
+        """Get the url for the container object with path"""
 
 
 @dataclass
-class Workspace(AirlockContainer):
+class Workspace:
     """Simple wrapper around a workspace directory on disk.
 
     Deliberately a dumb python object - the only operations are about accessing
@@ -54,6 +60,9 @@ class Workspace(AirlockContainer):
 
     name: str
 
+    # can be set to mark the currently selected path in this workspace
+    selected_path: Path = None
+
     def __post_init__(self):
         if not self.root().exists():
             raise ProviderAPI.WorkspaceNotFound(self.name)
@@ -113,7 +122,7 @@ class FileGroup:
 
 
 @dataclass
-class ReleaseRequest(AirlockContainer):
+class ReleaseRequest:
     """Represents a release request made by a user.
 
     Deliberately a dumb python object. Does not operate on the state of request,
@@ -129,6 +138,9 @@ class ReleaseRequest(AirlockContainer):
     status: Status = Status.PENDING
     filegroups: dict[FileGroup] = field(default_factory=dict)
 
+    # can be set to mark the currently selected path in this release request
+    selected_path: Path = None
+
     def __post_init__(self):
         self.root().mkdir(parents=True, exist_ok=True)
 
diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py
index 06478a6c9..d8868c3da 100644
--- a/airlock/file_browser_api.py
+++ b/airlock/file_browser_api.py
@@ -7,7 +7,7 @@
 @dataclass(frozen=True)
 class PathItem:
     """
-    This provides a thin abstraction over `pathlib.Path` objects with two goals:
+    This provides a thin abstraction over url paths and related filesystem objects with two goals:
 
         1. Paths should be enforced as being relative to a certain "container" directory
            and it should not be possible to traverse outside of this directory or to
@@ -21,11 +21,6 @@ class PathItem:
     container: AirlockContainer
     relpath: pathlib.Path
 
-    # The currently selected view path in this container.  Used to calculate if
-    # how relpath relates to it, i.e. if relpath *is* the currently selected
-    # path, or is one of its parents.
-    selected: pathlib.Path = None
-
     def __post_init__(self):
         # ensure relpath is a Path
         object.__setattr__(self, "relpath", pathlib.Path(self.relpath))
@@ -50,14 +45,14 @@ def url(self):
 
     def parent(self):
         if self.relpath.parents:
-            return PathItem(self.container, self.relpath.parent, selected=self.selected)
+            return PathItem(self.container, self.relpath.parent)
 
     def children(self):
         if not self.is_directory():
             return []
         root = self.container.root()
         children = [
-            PathItem(self.container, child.relative_to(root), selected=self.selected)
+            PathItem(self.container, child.relative_to(root))
             for child in self._absolute_path().iterdir()
         ]
         # directories first, then alphabetical, aks what windows does
@@ -85,7 +80,7 @@ def breadcrumbs(self):
 
         parent = item.parent()
         while parent:
-            if parent.relpath != pathlib.Path("."):
+            if parent.relpath != pathlib.Path(""):
                 crumbs.append(parent)
             parent = parent.parent()
 
@@ -113,10 +108,10 @@ def html_classes(self):
         return " ".join(classes)
 
     def is_selected(self):
-        return self.relpath == self.selected
+        return self.relpath == self.container.selected_path
 
     def is_on_selected_path(self):
-        return self.relpath in self.selected.parents
+        return self.relpath in self.container.selected_path.parents
 
     def is_open(self):
         return self.is_selected() or self.is_on_selected_path()
diff --git a/airlock/views.py b/airlock/views.py
index 36739f5d0..f024b3cd2 100644
--- a/airlock/views.py
+++ b/airlock/views.py
@@ -133,8 +133,9 @@ def workspace_view(request, workspace_name: str, path: str = ""):
     workspace = validate_workspace(request.user, workspace_name)
 
     relpath = Path(path)
-    root = PathItem(workspace, Path("."), selected=relpath)
-    path_item = PathItem(workspace, path, selected=relpath)
+    workspace.selected_path = relpath
+    root = PathItem(workspace, Path())
+    path_item = PathItem(workspace, relpath)
 
     if not path_item.exists():
         raise Http404()
@@ -216,8 +217,9 @@ def request_view(request, request_id: str, path: str = ""):
     release_request = validate_release_request(request.user, request_id)
 
     relpath = Path(path)
-    root = PathItem(release_request, Path("."), selected=relpath)
-    path_item = PathItem(release_request, path, selected=relpath)
+    release_request.selected_path = relpath
+    root = PathItem(release_request, Path())
+    path_item = PathItem(release_request, relpath)
 
     if not path_item.exists():
         raise Http404()
diff --git a/tests/unit/test_file_browser_api.py b/tests/unit/test_file_browser_api.py
index 326f60c66..e5cdd3927 100644
--- a/tests/unit/test_file_browser_api.py
+++ b/tests/unit/test_file_browser_api.py
@@ -3,13 +3,13 @@
 
 import pytest
 
-from airlock.api import AirlockContainer
 from airlock.file_browser_api import PathItem
 
 
 @dataclasses.dataclass(frozen=True)
-class DummyContainer(AirlockContainer):
+class DummyContainer:
     path: Path
+    selected_path: Path = Path()
 
     def root(self):
         return self.path
@@ -157,19 +157,20 @@ def test_breadcrumbs(container):
     ]
 
 
-def test_selection_logic(container):
-    selected = Path("some_dir/file_a.txt")
+def test_selection_logic(tmp_files):
+    selected_path = Path("some_dir/file_a.txt")
+    container = DummyContainer(tmp_files, selected_path=selected_path)
 
-    selected_item = PathItem(container, selected, selected)
+    selected_item = PathItem(container, selected_path)
     assert selected_item.is_selected()
     assert selected_item.is_open()
 
-    parent_item = PathItem(container, "some_dir", selected)
+    parent_item = PathItem(container, "some_dir")
     assert not parent_item.is_selected()
     assert parent_item.is_on_selected_path()
     assert parent_item.is_open()
 
-    other_item = PathItem(container, "other_dir", selected)
+    other_item = PathItem(container, "other_dir")
     assert not other_item.is_selected()
     assert not other_item.is_on_selected_path()
     assert not other_item.is_open()

From 9b2ccb735121a8d0c97fab76bd8a8698bdbd3c0f Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Wed, 21 Feb 2024 14:24:19 +0000
Subject: [PATCH 2/5] Add UrlPath type to use for all relative url paths

Previously, url paths mapped directly on to file paths, so using a full
Path for both made a lot of sense.

However, filegroups introduces a separation betweent the two. e.g. the
absolute url path "/requests/view/REQUEST_ID/default/output/file" has
a url path of `default/output/file` relative to the url root, but the
file path of `output/file` relative to the directory root.

So we use a renamed PurePosixPath as a convenient implementation of
a UrlPath.  This helps distinguish between the two types of paths via
type annotations, but also prevents accidentally doing IO operations
on the Pure path.

Also added a convenient constant ROOT_PATH, which is the empty relative
path.
---
 airlock/api.py                      | 20 +++++++++++++-------
 airlock/file_browser_api.py         |  9 ++++-----
 airlock/views.py                    | 14 ++++++--------
 tests/unit/test_file_browser_api.py | 10 ++++++----
 4 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/airlock/api.py b/airlock/api.py
index f380d50bf..0f3664617 100644
--- a/airlock/api.py
+++ b/airlock/api.py
@@ -3,6 +3,9 @@
 from datetime import datetime
 from enum import Enum
 from pathlib import Path
+
+# we use PurePosixPath as a convenient url path representation
+from pathlib import PurePosixPath as UrlPath
 from typing import Optional, Protocol
 
 from django.conf import settings
@@ -12,6 +15,9 @@
 from airlock.users import User
 
 
+ROOT_PATH = UrlPath()
+
+
 class Status(Enum):
     """Status for release Requests"""
 
@@ -35,7 +41,7 @@ class AirlockContainer(Protocol):
     # The currently selected path in this container.  Used to calculate if how
     # a particular path relates to it, i.e. if path *is* the currently selected
     # path, or is one of its parents.
-    selected_path: Path = None
+    selected_path: UrlPath = ROOT_PATH
 
     def root(self) -> Path:
         """Absolute concrete Path to root dir for files in this container."""
@@ -46,7 +52,7 @@ def get_id(self) -> str:
     def get_absolute_url(self) -> str:
         """Get the url for the container object"""
 
-    def get_url_for_path(self, path: Path) -> str:
+    def get_url_for_path(self, path: UrlPath) -> str:
         """Get the url for the container object with path"""
 
 
@@ -61,7 +67,7 @@ class Workspace:
     name: str
 
     # can be set to mark the currently selected path in this workspace
-    selected_path: Path = None
+    selected_path: UrlPath = ROOT_PATH
 
     def __post_init__(self):
         if not self.root().exists():
@@ -108,7 +114,7 @@ class RequestFile:
     Represents a single file within a release request
     """
 
-    relpath: Path
+    relpath: UrlPath
 
 
 @dataclass(frozen=True)
@@ -139,7 +145,7 @@ class ReleaseRequest:
     filegroups: dict[FileGroup] = field(default_factory=dict)
 
     # can be set to mark the currently selected path in this release request
-    selected_path: Path = None
+    selected_path: UrlPath = ROOT_PATH
 
     def __post_init__(self):
         self.root().mkdir(parents=True, exist_ok=True)
@@ -336,7 +342,7 @@ def set_status(
     def add_file_to_request(
         self,
         release_request: ReleaseRequest,
-        relpath: Path,
+        relpath: UrlPath,
         user: User,
         group_name: Optional[str] = "default",
     ):
@@ -382,7 +388,7 @@ def release_files(self, request: ReleaseRequest, user: User):
         )
 
         for f in filelist.files:
-            relpath = Path(f.name)
+            relpath = UrlPath(f.name)
             old_api.upload_file(
                 jobserver_release_id, relpath, request.root() / relpath, user.username
             )
diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py
index d8868c3da..5c8024c7e 100644
--- a/airlock/file_browser_api.py
+++ b/airlock/file_browser_api.py
@@ -1,7 +1,6 @@
-import pathlib
 from dataclasses import dataclass
 
-from airlock.api import AirlockContainer
+from airlock.api import ROOT_PATH, AirlockContainer, UrlPath
 
 
 @dataclass(frozen=True)
@@ -19,11 +18,11 @@ class PathItem:
     """
 
     container: AirlockContainer
-    relpath: pathlib.Path
+    relpath: UrlPath = ROOT_PATH
 
     def __post_init__(self):
         # ensure relpath is a Path
-        object.__setattr__(self, "relpath", pathlib.Path(self.relpath))
+        object.__setattr__(self, "relpath", UrlPath(self.relpath))
         # ensure path is within container
         self._absolute_path().resolve().relative_to(self.container.root())
 
@@ -80,7 +79,7 @@ def breadcrumbs(self):
 
         parent = item.parent()
         while parent:
-            if parent.relpath != pathlib.Path(""):
+            if parent.relpath != ROOT_PATH:
                 crumbs.append(parent)
             parent = parent.parent()
 
diff --git a/airlock/views.py b/airlock/views.py
index f024b3cd2..08e02d3df 100644
--- a/airlock/views.py
+++ b/airlock/views.py
@@ -1,5 +1,3 @@
-from pathlib import Path
-
 import requests
 from django import forms
 from django.conf import settings
@@ -12,7 +10,7 @@
 from django.views.decorators.http import require_http_methods
 
 from airlock import login_api
-from airlock.api import Status
+from airlock.api import Status, UrlPath
 from airlock.file_browser_api import PathItem
 from airlock.forms import AddFileForm
 from local_db.api import LocalDBProvider
@@ -132,9 +130,9 @@ def use_tree_ui(request):
 def workspace_view(request, workspace_name: str, path: str = ""):
     workspace = validate_workspace(request.user, workspace_name)
 
-    relpath = Path(path)
+    relpath = UrlPath(path)
     workspace.selected_path = relpath
-    root = PathItem(workspace, Path())
+    root = PathItem(workspace)
     path_item = PathItem(workspace, relpath)
 
     if not path_item.exists():
@@ -168,7 +166,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
 @require_http_methods(["POST"])
 def workspace_add_file_to_request(request, workspace_name):
     workspace = validate_workspace(request.user, workspace_name)
-    relpath = Path(request.POST["path"])
+    relpath = UrlPath(request.POST["path"])
     try:
         workspace.abspath(relpath)
     except api.FileNotFound:
@@ -216,9 +214,9 @@ def request_index(request):
 def request_view(request, request_id: str, path: str = ""):
     release_request = validate_release_request(request.user, request_id)
 
-    relpath = Path(path)
+    relpath = UrlPath(path)
     release_request.selected_path = relpath
-    root = PathItem(release_request, Path())
+    root = PathItem(release_request)
     path_item = PathItem(release_request, relpath)
 
     if not path_item.exists():
diff --git a/tests/unit/test_file_browser_api.py b/tests/unit/test_file_browser_api.py
index e5cdd3927..0220a0d1f 100644
--- a/tests/unit/test_file_browser_api.py
+++ b/tests/unit/test_file_browser_api.py
@@ -3,13 +3,13 @@
 
 import pytest
 
-from airlock.file_browser_api import PathItem
+from airlock.file_browser_api import ROOT_PATH, PathItem, UrlPath
 
 
 @dataclasses.dataclass(frozen=True)
 class DummyContainer:
     path: Path
-    selected_path: Path = Path()
+    selected_path: UrlPath = ROOT_PATH
 
     def root(self):
         return self.path
@@ -111,7 +111,9 @@ def test_parent(container, path, parent_path):
 )
 def test_children(container, path, child_paths):
     children = PathItem(container, path).children()
-    assert set(children) == {PathItem(container, Path(child)) for child in child_paths}
+    assert set(children) == {
+        PathItem(container, UrlPath(child)) for child in child_paths
+    }
 
 
 @pytest.mark.parametrize(
@@ -158,7 +160,7 @@ def test_breadcrumbs(container):
 
 
 def test_selection_logic(tmp_files):
-    selected_path = Path("some_dir/file_a.txt")
+    selected_path = UrlPath("some_dir/file_a.txt")
     container = DummyContainer(tmp_files, selected_path=selected_path)
 
     selected_item = PathItem(container, selected_path)

From 367aabb66fb35502b06a810174c796bf70391b01 Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Wed, 21 Feb 2024 14:53:17 +0000
Subject: [PATCH 3/5] Make breadcrumbs include root

Previously, this was done manually in the template. This was is self
contained and better.
---
 airlock/file_browser_api.py               |  5 +++--
 airlock/templates/file_browser/index.html |  1 -
 tests/unit/test_file_browser_api.py       | 13 +++++++++----
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py
index 5c8024c7e..3cbccfa98 100644
--- a/airlock/file_browser_api.py
+++ b/airlock/file_browser_api.py
@@ -36,6 +36,8 @@ def is_directory(self):
         return self._absolute_path().is_dir()
 
     def name(self):
+        if self.relpath == ROOT_PATH:
+            return self.container.get_id()
         return self.relpath.name
 
     def url(self):
@@ -79,8 +81,7 @@ def breadcrumbs(self):
 
         parent = item.parent()
         while parent:
-            if parent.relpath != ROOT_PATH:
-                crumbs.append(parent)
+            crumbs.append(parent)
             parent = parent.parent()
 
         crumbs.reverse()
diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html
index 3ae2312cc..6e135e1e8 100644
--- a/airlock/templates/file_browser/index.html
+++ b/airlock/templates/file_browser/index.html
@@ -84,7 +84,6 @@
 {% /card %}
 
 {% #breadcrumbs %}
-    {% breadcrumb title=path_item.container.get_id url=path_item.container.get_absolute_url %}
     {% for crumb in path_item.breadcrumbs %}
         {% breadcrumb title=crumb.name url=crumb.url active=forloop.last %}
     {% endfor %}
diff --git a/tests/unit/test_file_browser_api.py b/tests/unit/test_file_browser_api.py
index 0220a0d1f..0fee85d44 100644
--- a/tests/unit/test_file_browser_api.py
+++ b/tests/unit/test_file_browser_api.py
@@ -14,6 +14,9 @@ class DummyContainer:
     def root(self):
         return self.path
 
+    def get_id(self):
+        return "DummyContainer"
+
     def get_url_for_path(self, relpath):
         return f"/test/{relpath}"
 
@@ -152,10 +155,12 @@ def test_from_relative_path_rejects_path_escape(container, path):
 
 
 def test_breadcrumbs(container):
-    assert PathItem(container, "foo/bar/baz").breadcrumbs() == [
-        PathItem(container, "foo"),
-        PathItem(container, "foo/bar"),
-        PathItem(container, "foo/bar/baz"),
+    path = PathItem(container, "foo/bar/baz")
+    assert [b.name() for b in path.breadcrumbs()] == [
+        "DummyContainer",
+        "foo",
+        "bar",
+        "baz",
     ]
 
 

From bbab7841de41810c2a76194ece322c78726b69a9 Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Wed, 21 Feb 2024 15:02:32 +0000
Subject: [PATCH 4/5] Refactor url routing

Previously, we had two separately named path for workspace and requests
views.

This gives them the same name, and provides a default value for path, as
per recommend way in django docs.

This allows us to consolidate our url handling to just one reverse call,
that defaults to the root path if none is specified.
---
 airlock/api.py                            | 22 ++++------------------
 airlock/file_browser_api.py               |  3 ++-
 airlock/templates/file_browser/index.html |  2 +-
 airlock/templates/requests.html           |  4 ++--
 airlock/templates/workspaces.html         |  2 +-
 airlock/urls.py                           |  6 ++++--
 airlock/views.py                          | 10 +++++-----
 tests/integration/test_views.py           |  2 +-
 tests/unit/test_file_browser_api.py       |  4 ++--
 9 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/airlock/api.py b/airlock/api.py
index 0f3664617..5a63b0fef 100644
--- a/airlock/api.py
+++ b/airlock/api.py
@@ -15,7 +15,7 @@
 from airlock.users import User
 
 
-ROOT_PATH = UrlPath()
+ROOT_PATH = UrlPath()  # empty path
 
 
 class Status(Enum):
@@ -49,10 +49,7 @@ def root(self) -> Path:
     def get_id(self) -> str:
         """Get the human name for this container."""
 
-    def get_absolute_url(self) -> str:
-        """Get the url for the container object"""
-
-    def get_url_for_path(self, path: UrlPath) -> str:
+    def get_url(self, path: UrlPath = ROOT_PATH) -> str:
         """Get the url for the container object with path"""
 
 
@@ -79,10 +76,7 @@ def root(self):
     def get_id(self):
         return self.name
 
-    def get_absolute_url(self):
-        return reverse("workspace_home", kwargs={"workspace_name": self.name})
-
-    def get_url_for_path(self, relpath):
+    def get_url(self, relpath=ROOT_PATH):
         return reverse(
             "workspace_view",
             kwargs={"workspace_name": self.name, "path": relpath},
@@ -156,15 +150,7 @@ def root(self):
     def get_id(self):
         return self.id
 
-    def get_absolute_url(self):
-        return reverse(
-            "request_home",
-            kwargs={
-                "request_id": self.id,
-            },
-        )
-
-    def get_url_for_path(self, relpath):
+    def get_url(self, relpath=ROOT_PATH):
         return reverse(
             "request_view",
             kwargs={
diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py
index 3cbccfa98..24df50f40 100644
--- a/airlock/file_browser_api.py
+++ b/airlock/file_browser_api.py
@@ -38,11 +38,12 @@ def is_directory(self):
     def name(self):
         if self.relpath == ROOT_PATH:
             return self.container.get_id()
+
         return self.relpath.name
 
     def url(self):
         suffix = "/" if self.is_directory() else ""
-        return self.container.get_url_for_path(f"{self.relpath}{suffix}")
+        return self.container.get_url(f"{self.relpath}{suffix}")
 
     def parent(self):
         if self.relpath.parents:
diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html
index 6e135e1e8..1d1284fdd 100644
--- a/airlock/templates/file_browser/index.html
+++ b/airlock/templates/file_browser/index.html
@@ -71,7 +71,7 @@
             </form>
             {% endif %}
         {% endif %}
-        {% #button type="link" href=workspace.get_absolute_url variant="success" %}Workspace Home{% /button %}
+        {% #button type="link" href=workspace.get_url variant="success" %}Workspace Home{% /button %}
     {% endif %}
 </div>
 {% endfragment %}
diff --git a/airlock/templates/requests.html b/airlock/templates/requests.html
index a1e1030d7..34de023ae 100644
--- a/airlock/templates/requests.html
+++ b/airlock/templates/requests.html
@@ -5,7 +5,7 @@
 {% #card title="Requests by "|add:request.user.username %}
   {% #list_group %}
     {% for request in authored_requests %} 
-    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }}: {{ request.status.name }}{% /list_group_item %}
+    {% #list_group_item href=request.get_url %}{{ request.workspace }}: {{ request.status.name }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}
@@ -14,7 +14,7 @@
 {% #card title="Outstanding requests awaiting review" %}
   {% #list_group %}
     {% for request in outstanding_requests %} 
-    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }} by {{ request.author }}{% /list_group_item %}
+    {% #list_group_item href=request.get_url %}{{ request.workspace }} by {{ request.author }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}
diff --git a/airlock/templates/workspaces.html b/airlock/templates/workspaces.html
index 9f1c5126d..a0272a244 100644
--- a/airlock/templates/workspaces.html
+++ b/airlock/templates/workspaces.html
@@ -4,7 +4,7 @@
 {% #card title="Workspaces for "|add:request.user.username %}
   {% #list_group %}
     {% for workspace in workspaces %} 
-    {% #list_group_item href=workspace.get_absolute_url %}{{ workspace.name }}{% /list_group_item %}
+    {% #list_group_item href=workspace.get_url %}{{ workspace.name }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}
diff --git a/airlock/urls.py b/airlock/urls.py
index ac5a1be4f..6891458eb 100644
--- a/airlock/urls.py
+++ b/airlock/urls.py
@@ -34,7 +34,8 @@
     path(
         "workspaces/view/<str:workspace_name>/",
         airlock.views.workspace_view,
-        name="workspace_home",
+        kwargs={"path": ""},
+        name="workspace_view",
     ),
     path(
         "workspaces/view/<str:workspace_name>/<path:path>",
@@ -55,7 +56,8 @@
     path(
         "requests/view/<str:request_id>/",
         airlock.views.request_view,
-        name="request_home",
+        name="request_view",
+        kwargs={"path": ""},
     ),
     path(
         "requests/view/<str:request_id>/<path:path>",
diff --git a/airlock/views.py b/airlock/views.py
index 08e02d3df..ff283f12b 100644
--- a/airlock/views.py
+++ b/airlock/views.py
@@ -177,7 +177,7 @@ def workspace_add_file_to_request(request, workspace_name):
     if not form.is_valid():
         for error in form.errors.values():
             messages.error(request, error)
-        return redirect(workspace.get_url_for_path(relpath))
+        return redirect(workspace.get_url(relpath))
 
     group_name = request.POST.get("new_filegroup") or request.POST.get("filegroup")
     try:
@@ -191,7 +191,7 @@ def workspace_add_file_to_request(request, workspace_name):
             request, f"File has been added to request (file group '{group_name}')"
         )
     # redirect to this just added file
-    return redirect(release_request.get_url_for_path(relpath))
+    return redirect(release_request.get_url(relpath))
 
 
 def request_index(request):
@@ -273,7 +273,7 @@ def request_submit(request, request_id):
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been submitted")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())
 
 
 @require_http_methods(["POST"])
@@ -286,7 +286,7 @@ def request_reject(request, request_id):
         raise PermissionDenied(str(exc))
 
     messages.error(request, "Request has been rejected")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())
 
 
 @require_http_methods(["POST"])
@@ -315,4 +315,4 @@ def request_release_files(request, request_id):
         raise
 
     messages.success(request, "Files have been released to jobs.opensafely.org")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())
diff --git a/tests/integration/test_views.py b/tests/integration/test_views.py
index a20028282..db6f600c3 100644
--- a/tests/integration/test_views.py
+++ b/tests/integration/test_views.py
@@ -260,7 +260,7 @@ def test_workspace_request_file_invalid_new_filegroup(client_with_user, api):
 
     assert not filegroupmetadata.request_files.exists()
     # redirects to the workspace file again, with error messages
-    assert response.request["PATH_INFO"] == workspace.get_url_for_path("test/path.txt")
+    assert response.request["PATH_INFO"] == workspace.get_url("test/path.txt")
 
     all_messages = [msg for msg in response.context["messages"]]
     assert len(all_messages) == 1
diff --git a/tests/unit/test_file_browser_api.py b/tests/unit/test_file_browser_api.py
index 0fee85d44..ee5436a17 100644
--- a/tests/unit/test_file_browser_api.py
+++ b/tests/unit/test_file_browser_api.py
@@ -17,7 +17,7 @@ def root(self):
     def get_id(self):
         return "DummyContainer"
 
-    def get_url_for_path(self, relpath):
+    def get_url(self, relpath):
         return f"/test/{relpath}"
 
 
@@ -156,7 +156,7 @@ def test_from_relative_path_rejects_path_escape(container, path):
 
 def test_breadcrumbs(container):
     path = PathItem(container, "foo/bar/baz")
-    assert [b.name() for b in path.breadcrumbs()] == [
+    assert [c.name() for c in path.breadcrumbs()] == [
         "DummyContainer",
         "foo",
         "bar",

From e6f43a19dda55e92a354d30317455a04cb622a7c Mon Sep 17 00:00:00 2001
From: Simon Davy <bloodearnest@gmail.com>
Date: Thu, 22 Feb 2024 11:58:36 +0000
Subject: [PATCH 5/5] Apply suggestions from code review

Co-authored-by: Becky Smith <becky.smith@thedatalab.org>
---
 airlock/api.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/airlock/api.py b/airlock/api.py
index 5a63b0fef..f421cfc34 100644
--- a/airlock/api.py
+++ b/airlock/api.py
@@ -34,11 +34,11 @@ class Status(Enum):
 class AirlockContainer(Protocol):
     """Structural typing class for a instance of a Workspace or ReleaseRequest
 
-    Provides a uniform interface for accessing information the paths and files
-    container within this instance.
+    Provides a uniform interface for accessing information about the paths and files
+    contained within this instance.
     """
 
-    # The currently selected path in this container.  Used to calculate if how
+    # The currently selected path in this container.  Used to calculate how
     # a particular path relates to it, i.e. if path *is* the currently selected
     # path, or is one of its parents.
     selected_path: UrlPath = ROOT_PATH