diff --git a/airlock/api.py b/airlock/api.py index 734d6821f..f421cfc34 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -3,7 +3,10 @@ from datetime import datetime from enum import Enum from pathlib import Path -from typing import Optional + +# 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 from django.shortcuts import reverse @@ -12,6 +15,9 @@ from airlock.users import User +ROOT_PATH = UrlPath() # empty path + + class Status(Enum): """Status for release Requests""" @@ -25,27 +31,30 @@ 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 about the paths and files + contained within this instance. """ - def root(self): - raise NotImplementedError() + # 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 - 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_url(self, path: UrlPath = ROOT_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 +63,9 @@ class Workspace(AirlockContainer): name: str + # can be set to mark the currently selected path in this workspace + selected_path: UrlPath = ROOT_PATH + def __post_init__(self): if not self.root().exists(): raise ProviderAPI.WorkspaceNotFound(self.name) @@ -64,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}, @@ -99,7 +108,7 @@ class RequestFile: Represents a single file within a release request """ - relpath: Path + relpath: UrlPath @dataclass(frozen=True) @@ -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: UrlPath = ROOT_PATH + def __post_init__(self): self.root().mkdir(parents=True, exist_ok=True) @@ -138,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={ @@ -324,7 +328,7 @@ def set_status( def add_file_to_request( self, release_request: ReleaseRequest, - relpath: Path, + relpath: UrlPath, user: User, group_name: Optional[str] = "default", ): @@ -370,7 +374,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 06478a6c9..24df50f40 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -1,13 +1,12 @@ -import pathlib from dataclasses import dataclass -from airlock.api import AirlockContainer +from airlock.api import ROOT_PATH, AirlockContainer, UrlPath @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 @@ -19,16 +18,11 @@ 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 + 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()) @@ -42,22 +36,25 @@ 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): 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: - 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,8 +82,7 @@ def breadcrumbs(self): parent = item.parent() while parent: - if parent.relpath != pathlib.Path("."): - crumbs.append(parent) + crumbs.append(parent) parent = parent.parent() crumbs.reverse() @@ -113,10 +109,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/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index 3ae2312cc..1d1284fdd 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -71,7 +71,7 @@ {% 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 %} {% endfragment %} @@ -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/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//", airlock.views.workspace_view, - name="workspace_home", + kwargs={"path": ""}, + name="workspace_view", ), path( "workspaces/view//", @@ -55,7 +56,8 @@ path( "requests/view//", airlock.views.request_view, - name="request_home", + name="request_view", + kwargs={"path": ""}, ), path( "requests/view//", diff --git a/airlock/views.py b/airlock/views.py index 36739f5d0..ff283f12b 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,10 @@ def use_tree_ui(request): 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) + relpath = UrlPath(path) + workspace.selected_path = relpath + root = PathItem(workspace) + path_item = PathItem(workspace, relpath) if not path_item.exists(): raise Http404() @@ -167,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: @@ -178,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: @@ -192,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): @@ -215,9 +214,10 @@ 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) - root = PathItem(release_request, Path("."), selected=relpath) - path_item = PathItem(release_request, path, selected=relpath) + relpath = UrlPath(path) + release_request.selected_path = relpath + root = PathItem(release_request) + path_item = PathItem(release_request, relpath) if not path_item.exists(): raise Http404() @@ -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 326f60c66..ee5436a17 100644 --- a/tests/unit/test_file_browser_api.py +++ b/tests/unit/test_file_browser_api.py @@ -3,18 +3,21 @@ import pytest -from airlock.api import AirlockContainer -from airlock.file_browser_api import PathItem +from airlock.file_browser_api import ROOT_PATH, PathItem, UrlPath @dataclasses.dataclass(frozen=True) -class DummyContainer(AirlockContainer): +class DummyContainer: path: Path + selected_path: UrlPath = ROOT_PATH def root(self): return self.path - def get_url_for_path(self, relpath): + def get_id(self): + return "DummyContainer" + + def get_url(self, relpath): return f"/test/{relpath}" @@ -111,7 +114,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( @@ -150,26 +155,29 @@ 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 [c.name() for c in path.breadcrumbs()] == [ + "DummyContainer", + "foo", + "bar", + "baz", ] -def test_selection_logic(container): - selected = Path("some_dir/file_a.txt") +def test_selection_logic(tmp_files): + selected_path = UrlPath("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()