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.

Author: bloodearnest

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={

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:

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 %}

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 %}

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 %}

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>",

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())

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

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",