From 5e38222b50fc0d005f9bf1da2d11018257114acc Mon Sep 17 00:00:00 2001 From: David Evans Date: Thu, 7 Mar 2024 16:45:07 +0000 Subject: [PATCH 1/9] Make API provider configurable in one place Rather than instantiating an API object each time we need one, we create a single shared instance which can be configured in settings. This, in theory, would allow us to an Airlock app shared between two different projects with differently configured data providers. --- airlock/api.py | 12 ++++++++++++ airlock/settings.py | 2 ++ airlock/views/helpers.py | 5 +---- airlock/views/request.py | 6 +----- airlock/views/workspace.py | 6 +----- tests/conftest.py | 7 +++---- tests/factories.py | 5 +---- tests/functional/test_e2e.py | 6 +----- 8 files changed, 22 insertions(+), 27 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index 208c90ee..eafa1eba 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -10,6 +10,8 @@ from django.conf import settings from django.shortcuts import reverse +from django.utils.functional import SimpleLazyObject +from django.utils.module_loading import import_string import old_api from airlock.users import User @@ -422,3 +424,13 @@ def release_files(self, request: ReleaseRequest, user: User): ) self.set_status(request, Status.RELEASED, user) + + +def _get_configured_api(): + ProviderImplementation = import_string(settings.AIRLOCK_API_PROVIDER) + return ProviderImplementation() + + +# We follow the Django pattern of using a lazy object which configures itself on first +# access so as to avoid reading `settings` during import +api = SimpleLazyObject(_get_configured_api) diff --git a/airlock/settings.py b/airlock/settings.py index 185cca7c..61fd8571 100644 --- a/airlock/settings.py +++ b/airlock/settings.py @@ -268,6 +268,8 @@ def run_connection_init_queries(*, connection, **kwargs): f"variables must be set.\n\n{_missing_env_var_hint}" ) +AIRLOCK_API_PROVIDER = "local_db.api.LocalDBProvider" + # BACKEND is global env var on backends BACKEND = os.environ.get("BACKEND", "test") diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py index 91300c12..911202d4 100644 --- a/airlock/views/helpers.py +++ b/airlock/views/helpers.py @@ -3,10 +3,7 @@ from django.core.exceptions import PermissionDenied from django.http import FileResponse, Http404 -from local_db.api import LocalDBProvider - - -api = LocalDBProvider() +from airlock.api import api def login_exempt(view): diff --git a/airlock/views/request.py b/airlock/views/request.py index d14a5691..7a95ffa7 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -9,16 +9,12 @@ from django.views.decorators.http import require_http_methods from django.views.decorators.vary import vary_on_headers -from airlock.api import Status +from airlock.api import Status, api from airlock.file_browser_api import get_request_tree -from local_db.api import LocalDBProvider from .helpers import serve_file, validate_release_request -api = LocalDBProvider() - - def request_index(request): authored_requests = api.get_requests_authored_by_user(request.user) diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index 665db26f..584a6b3a 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -6,17 +6,13 @@ from django.views.decorators.http import require_http_methods from django.views.decorators.vary import vary_on_headers -from airlock.api import UrlPath +from airlock.api import UrlPath, api from airlock.file_browser_api import get_workspace_tree from airlock.forms import AddFileForm -from local_db.api import LocalDBProvider from .helpers import serve_file, validate_workspace -api = LocalDBProvider() - - def workspace_index(request): workspaces = api.get_workspaces_for_user(request.user) return TemplateResponse(request, "workspaces.html", {"workspaces": workspaces}) diff --git a/tests/conftest.py b/tests/conftest.py index 127ad102..7ea062e8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,9 +2,9 @@ import responses as _responses from django.conf import settings +import airlock.api import old_api import tests.factories -from local_db.api import LocalDBProvider # Fail the test run if we see any warnings @@ -34,9 +34,8 @@ def responses(): # we could parameterise this fixture to run tests over all api implementations in future @pytest.fixture def api(monkeypatch): - api = LocalDBProvider() - monkeypatch.setattr(tests.factories, "api", api) - return api + monkeypatch.setattr(tests.factories, "api", airlock.api.api) + return airlock.api.api @pytest.fixture diff --git a/tests/factories.py b/tests/factories.py index 478a3c70..bb001c67 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,14 +1,11 @@ from django.conf import settings -from airlock.api import Workspace +from airlock.api import Workspace, api from airlock.users import User -from local_db.api import LocalDBProvider default_user = User(1, "testuser") -api = LocalDBProvider() - def get_user(*, username="testuser", workspaces=[], output_checker=False): return User(1, username, workspaces, output_checker) diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index 6f538c3b..cd862a66 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -4,14 +4,10 @@ import pytest from playwright.sync_api import expect -from airlock.api import Status -from local_db.api import LocalDBProvider +from airlock.api import Status, api from tests import factories -api = LocalDBProvider() - - @pytest.fixture def dev_users(tmp_path, settings): settings.AIRLOCK_DEV_USERS_FILE = tmp_path / "dev_users.json" From e331afc3d56cd90fe2824cf4324eea1fb75ebb2e Mon Sep 17 00:00:00 2001 From: David Evans Date: Wed, 6 Mar 2024 16:38:34 +0000 Subject: [PATCH 2/9] Add `from_dict` methods to application objects We're going to want to be able to build them from nested dicts of simple values. --- airlock/api.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/airlock/api.py b/airlock/api.py index eafa1eba..4eb1a184 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -123,6 +123,10 @@ class RequestFile: relpath: UrlPath + @classmethod + def from_dict(cls, attrs): + return cls(**attrs) + @dataclass(frozen=True) class FileGroup: @@ -133,6 +137,13 @@ class FileGroup: name: str files: list[RequestFile] + @classmethod + def from_dict(cls, attrs): + return cls( + **{k: v for k, v in attrs.items() if k != "files"}, + files=[RequestFile.from_dict(value) for value in attrs.get("files", ())], + ) + @dataclass class ReleaseRequest: @@ -154,6 +165,17 @@ class ReleaseRequest: # can be set to mark the currently selected path in this release request selected_path: UrlPath = ROOT_PATH + @classmethod + def from_dict(cls, attrs): + return cls( + **{k: v for k, v in attrs.items() if k != "filegroups"}, + filegroups=cls._filegroups_from_dict(attrs.get("filegroups", {})), + ) + + @staticmethod + def _filegroups_from_dict(attrs): + return {key: FileGroup.from_dict(value) for key, value in attrs.items()} + def __post_init__(self): self.root().mkdir(parents=True, exist_ok=True) @@ -213,6 +235,9 @@ def file_set(self): for request_file in filegroup.files } + def set_filegroups_from_dict(self, attrs): + self.filegroups = self._filegroups_from_dict(attrs) + class ProviderAPI: class APIException(Exception): From 653c31ce7c3297c15cc850c6c667e7960dfdd914 Mon Sep 17 00:00:00 2001 From: David Evans Date: Wed, 6 Mar 2024 16:40:17 +0000 Subject: [PATCH 3/9] Split business logic and data access layers For now, this cleaves very closely to the existing logic but splits it across two classes which are composed together, rather than using inheritance. The intention is that the two layers communicate using simple values rather than application specific objects. --- airlock/api.py | 66 ++++++++++++++++++++++++------- airlock/settings.py | 2 +- local_db/api.py | 89 ++++++++++++++++++------------------------ tests/factories.py | 2 +- tests/unit/test_api.py | 8 ++-- 5 files changed, 94 insertions(+), 73 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index 4eb1a184..27f4078c 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -239,7 +239,25 @@ def set_filegroups_from_dict(self, attrs): self.filegroups = self._filegroups_from_dict(attrs) +class DataAccessLayerProtocol: + """ + Placeholder for a structural type class we can use to define what a data access + layer should look like, once we've settled what that is. + """ + + class ProviderAPI: + """ + Business Logic Layer (class will be renamed later) + + The mechanism via which the rest of the codebase should read and write application + state. Interacts with a Data Access Layer purely by exchanging simple values + (dictionaries, strings etc). + """ + + def __init__(self, data_access_layer: DataAccessLayerProtocol): + self._dal = data_access_layer + class APIException(Exception): pass @@ -292,11 +310,11 @@ def _create_release_request(self, **kwargs): Is private because it is mean to only be used by our test factories to set up state - it is not part of the public API. """ - raise NotImplementedError() + return ReleaseRequest.from_dict(self._dal.create_release_request(**kwargs)) def get_release_request(self, request_id: str) -> ReleaseRequest: """Get a ReleaseRequest object for an id.""" - raise NotImplementedError() + return ReleaseRequest.from_dict(self._dal.get_release_request(request_id)) def get_current_request( self, workspace_name: str, user: User, create: bool = False @@ -305,15 +323,32 @@ def get_current_request( If create is True, create one. """ - raise NotImplementedError() + current_request_data = self._dal.get_current_request( + workspace=workspace_name, + username=user.username, + user_workspaces=user.workspaces, + create=create, + ) + if current_request_data is not None: + return ReleaseRequest.from_dict(current_request_data) def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]: """Get all current requests authored by user.""" - raise NotImplementedError() + return [ + ReleaseRequest.from_dict(attrs) + for attrs in self._dal.get_requests_authored_by_user( + username=user.username, user_workspaces=user.workspaces + ) + ] def get_outstanding_requests_for_review(self, user: User): """Get all request that need review.""" - raise NotImplementedError() + return [ + ReleaseRequest.from_dict(attrs) + for attrs in self._dal.get_outstanding_requests_for_review( + username=user.username, user_is_output_checker=user.output_checker + ) + ] VALID_STATE_TRANSITIONS = { Status.PENDING: [ @@ -391,6 +426,7 @@ def set_status( # validate first self.check_status(release_request, to_status, user) + self._dal.set_status(release_request.id, to_status) release_request.status = to_status def add_file_to_request( @@ -400,14 +436,6 @@ def add_file_to_request( user: User, group_name: Optional[str] = "default", ): - """Add a file to a request. - - Subclasses should do what they need to create the filegroup and - record the file metadata as needed and THEN - call super().add_file_to_request(...) to do the - copying. If the copying fails (e.g. due to permission errors raised - below), the subclasses should roll back any changes. - """ if user.username != release_request.author: raise self.RequestPermissionDenied( f"only author {release_request.author} can add files to this request" @@ -425,6 +453,14 @@ def add_file_to_request( dst.parent.mkdir(exist_ok=True, parents=True) shutil.copy(src, dst) + # TODO: This is not currently safe in that we modify the filesytem before + # calling out to the DAL which could fail. We will deal with this later by + # switching to a content-addressed storage model which avoids having mutable + # state on the filesystem. + filegroup_data = self._dal.add_file_to_request( + request_id=release_request.id, group_name=group_name, relpath=relpath + ) + release_request.set_filegroups_from_dict(filegroup_data) return release_request def release_files(self, request: ReleaseRequest, user: User): @@ -452,8 +488,8 @@ def release_files(self, request: ReleaseRequest, user: User): def _get_configured_api(): - ProviderImplementation = import_string(settings.AIRLOCK_API_PROVIDER) - return ProviderImplementation() + DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) + return ProviderAPI(DataAccessLayer()) # We follow the Django pattern of using a lazy object which configures itself on first diff --git a/airlock/settings.py b/airlock/settings.py index 61fd8571..0eb841fd 100644 --- a/airlock/settings.py +++ b/airlock/settings.py @@ -268,7 +268,7 @@ def run_connection_init_queries(*, connection, **kwargs): f"variables must be set.\n\n{_missing_env_var_hint}" ) -AIRLOCK_API_PROVIDER = "local_db.api.LocalDBProvider" +AIRLOCK_DATA_ACCESS_LAYER = "local_db.api.LocalDBDataAccessLayer" # BACKEND is global env var on backends BACKEND = os.environ.get("BACKEND", "test") diff --git a/local_db/api.py b/local_db/api.py index 7a0cab9d..d8e4b1eb 100644 --- a/local_db/api.py +++ b/local_db/api.py @@ -1,27 +1,23 @@ -from dataclasses import dataclass from pathlib import Path -from typing import Optional from django.db import transaction from airlock.api import ( - FileGroup, + DataAccessLayerProtocol, ProviderAPI, - ReleaseRequest, - RequestFile, Status, - User, ) from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata -@dataclass -class LocalDBProvider(ProviderAPI): - """Implementation of ProviderAPI using local_db models to store data.""" +class LocalDBDataAccessLayer(DataAccessLayerProtocol): + """ + Implementation of DataAccessLayerProtocol using local_db models to store data + """ def _request(self, metadata: RequestMetadata = None): """Unpack the db data into the Request object.""" - return ReleaseRequest( + return dict( id=metadata.id, workspace=metadata.workspace, status=metadata.status, @@ -30,7 +26,7 @@ def _request(self, metadata: RequestMetadata = None): filegroups=self._get_filegroups(metadata), ) - def _create_release_request(self, **kwargs): + def create_release_request(self, **kwargs): metadata = RequestMetadata.objects.create(**kwargs) return self._request(metadata) @@ -38,14 +34,14 @@ def _find_metadata(self, request_id: str): try: return RequestMetadata.objects.get(id=request_id) except RequestMetadata.DoesNotExist: - raise self.ReleaseRequestNotFound(request_id) + raise ProviderAPI.ReleaseRequestNotFound(request_id) def _filegroup(self, filegroup_metadata: FileGroupMetadata): """Unpack file group db data into FileGroup and RequestFile objects.""" - return FileGroup( + return dict( name=filegroup_metadata.name, files=[ - RequestFile(relpath=Path(file_metadata.relpath)) + dict(relpath=Path(file_metadata.relpath)) for file_metadata in filegroup_metadata.request_files.all() ], ) @@ -66,18 +62,20 @@ def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): def get_release_request(self, request_id: str): return self._request(self._find_metadata(request_id)) - def get_current_request(self, workspace: str, user: User, create=False): + def get_current_request( + self, workspace: str, username: str, user_workspaces: list[str], create=False + ): requests = list( RequestMetadata.objects.filter( workspace=workspace, - author=user.username, + author=username, status__in=[Status.PENDING, Status.SUBMITTED], ) ) n = len(requests) if n > 1: raise Exception( - f"Multiple active release requests for user {user.username} in workspace {workspace}" + f"Multiple active release requests for user {username} in workspace {workspace}" ) elif n == 1: return self._request(requests[0]) @@ -85,66 +83,59 @@ def get_current_request(self, workspace: str, user: User, create=False): # To create a request, you must have explicit workspace permissions. # Output checkers can view all workspaces, but are not allowed to # create requests for all workspaces. - if workspace not in user.workspaces: - raise self.RequestPermissionDenied(workspace) + if workspace not in user_workspaces: + raise ProviderAPI.RequestPermissionDenied(workspace) - return self._create_release_request( + return self.create_release_request( workspace=workspace, - author=user.username, + author=username, ) - def get_requests_authored_by_user(self, user: User): + def get_requests_authored_by_user(self, username: str, user_workspaces: list[str]): requests = [] - for metadata in RequestMetadata.objects.filter(author=user.username).order_by( + for metadata in RequestMetadata.objects.filter(author=username).order_by( "status" ): # to create a request, user *must* have explicit workspace # permissions - being an output checker is not enough - if metadata.workspace in user.workspaces: + if metadata.workspace in user_workspaces: requests.append(self._request(metadata)) return requests - def get_outstanding_requests_for_review(self, user: User): + def get_outstanding_requests_for_review( + self, username: str, user_is_output_checker: bool + ): requests = [] - if not user.output_checker: + if not user_is_output_checker: return [] for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED): # do not show output_checker their own requests - if metadata.author != user.username: + if metadata.author != username: requests.append(self._request(metadata)) return requests - def set_status(self, request: ReleaseRequest, status: Status, user: User): + def set_status(self, request_id: str, status: Status): with transaction.atomic(): - # validate transition/permissions ahead of time - self.check_status(request, status, user) # persist state change - metadata = self._find_metadata(request.id) + metadata = self._find_metadata(request_id) metadata.status = status metadata.save() - super().set_status(request, status, user) - - def add_file_to_request( - self, - release_request: ReleaseRequest, - relpath: Path, - user: User, - group_name: Optional[str] = "default", - ): + + def add_file_to_request(self, request_id, relpath: Path, group_name: str): with transaction.atomic(): # Get/create the FileGroupMetadata if it doesn't already exist filegroupmetadata = self._get_or_create_filegroupmetadata( - release_request.id, group_name + request_id, group_name ) # Check if this file is already on the request, in any group try: existing_file = RequestFileMetadata.objects.get( - filegroup__request_id=release_request.id, relpath=relpath + filegroup__request_id=request_id, relpath=relpath ) except RequestFileMetadata.DoesNotExist: # create the RequestFile @@ -152,17 +143,11 @@ def add_file_to_request( relpath=str(relpath), filegroup=filegroupmetadata ) else: - raise self.APIException( + raise ProviderAPI.APIException( "File has already been added to request " f"(in file group '{existing_file.filegroup.name}')" ) - # Now that the db objects have been successfully creates, - # call super() to copy the file. This will also validate - # that the user can add the file, and that the request is in a - # state that allows adding files. - super().add_file_to_request(release_request, relpath, user, group_name) - - # update instance - metadata = self._find_metadata(release_request.id) - release_request.filegroups = self._get_filegroups(metadata) + # Return updated FileGroups data + metadata = self._find_metadata(request_id) + return self._get_filegroups(metadata) diff --git a/tests/factories.py b/tests/factories.py index bb001c67..0bc9d08d 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -66,7 +66,7 @@ def create_filegroup(release_request, group_name, filepaths=None): api.add_file_to_request( release_request, filepath, User(1, release_request.author), group_name ) - return api._get_or_create_filegroupmetadata(release_request.id, group_name) + return api._dal._get_or_create_filegroupmetadata(release_request.id, group_name) def refresh_release_request(release_request): diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index 21f30ab7..0dac4900 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_api.py @@ -1,6 +1,6 @@ import json from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, Mock import pytest from django.conf import settings @@ -56,7 +56,7 @@ def test_provider_get_workspaces_for_user(user_workspaces, output_checker, expec factories.create_workspace("not-allowed") user = User(1, "test", user_workspaces, output_checker) - api = ProviderAPI() + api = ProviderAPI(data_access_layer=None) assert set(api.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) @@ -79,7 +79,7 @@ def test_provider_request_release_files_not_approved(): status=Status.SUBMITTED, ) - api = ProviderAPI() + api = ProviderAPI(data_access_layer=None) with pytest.raises(api.InvalidStateTransition): api.release_files(release_request, checker) @@ -100,7 +100,7 @@ def test_provider_request_release_files(mock_old_api): abspath = release_request.abspath("group" / relpath) - api = ProviderAPI() + api = ProviderAPI(data_access_layer=Mock()) api.release_files(release_request, checker) expected_json = { From 0396e2eadef76d76486b1ec6a0f938ad76656599 Mon Sep 17 00:00:00 2001 From: David Evans Date: Thu, 7 Mar 2024 11:24:06 +0000 Subject: [PATCH 4/9] Remove workspace filtering logic Have confirmed with Simon that this is vestigial code from an earlier iteration and is no longer needed. https://bennettoxford.slack.com/archives/C069YDR4NCA/p1709803744875949 --- airlock/api.py | 4 +--- local_db/api.py | 19 +++++++------------ tests/unit/test_api.py | 31 +++++++------------------------ 3 files changed, 15 insertions(+), 39 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index 27f4078c..6461659c 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -336,9 +336,7 @@ def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]: """Get all current requests authored by user.""" return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_requests_authored_by_user( - username=user.username, user_workspaces=user.workspaces - ) + for attrs in self._dal.get_requests_authored_by_user(username=user.username) ] def get_outstanding_requests_for_review(self, user: User): diff --git a/local_db/api.py b/local_db/api.py index d8e4b1eb..2fc83db3 100644 --- a/local_db/api.py +++ b/local_db/api.py @@ -91,18 +91,13 @@ def get_current_request( author=username, ) - def get_requests_authored_by_user(self, username: str, user_workspaces: list[str]): - requests = [] - - for metadata in RequestMetadata.objects.filter(author=username).order_by( - "status" - ): - # to create a request, user *must* have explicit workspace - # permissions - being an output checker is not enough - if metadata.workspace in user_workspaces: - requests.append(self._request(metadata)) - - return requests + def get_requests_authored_by_user(self, username: str): + return [ + self._request(request) + for request in RequestMetadata.objects.filter(author=username).order_by( + "status" + ) + ] def get_outstanding_requests_for_review( self, username: str, user_is_output_checker: bool diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index 0dac4900..7bd21f9f 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_api.py @@ -127,30 +127,13 @@ def test_provider_request_release_files(mock_old_api): ) -@pytest.mark.parametrize( - "workspaces, output_checker, expected", - [ - ([], False, []), - (["allowed"], False, ["r1"]), - # output checkers can't create requests unless explit permission for workspace - ([], True, []), - (["allowed"], True, ["r1"]), - (["allowed", "notexist"], False, ["r1"]), - (["notexist"], False, []), - (["no-request-dir", "notexist"], False, []), - ], -) -def test_provider_get_requests_authored_by_user( - workspaces, output_checker, expected, api -): - user = User(1, "test", workspaces, output_checker) - other_user = User(1, "other", [], False) - factories.create_release_request("allowed", user, id="r1") - factories.create_release_request("allowed", other_user, id="r2") - factories.create_release_request("not-allowed", user, id="r3") - factories.create_workspace("no-request-dir") - - assert set(r.id for r in api.get_requests_authored_by_user(user)) == set(expected) +def test_provider_get_requests_authored_by_user(api): + user = User(1, "test", [], True) + other_user = User(1, "other", [], True) + factories.create_release_request("workspace", user, id="r1") + factories.create_release_request("workspace", other_user, id="r2") + + assert [r.id for r in api.get_requests_authored_by_user(user)] == ["r1"] @pytest.mark.parametrize( From d3e622f31cbd3bfe5da5ffad804d21c29060e6f8 Mon Sep 17 00:00:00 2001 From: David Evans Date: Wed, 6 Mar 2024 17:16:10 +0000 Subject: [PATCH 5/9] Move business logic out of the DAL, part 1 --- airlock/api.py | 28 +++++++++++++++++++++++----- local_db/api.py | 29 +++++------------------------ 2 files changed, 28 insertions(+), 29 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index 6461659c..b90435ba 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -323,14 +323,32 @@ def get_current_request( If create is True, create one. """ - current_request_data = self._dal.get_current_request( + active_requests = self._dal.get_active_requests_for_workspace_by_user( workspace=workspace_name, username=user.username, - user_workspaces=user.workspaces, - create=create, ) - if current_request_data is not None: - return ReleaseRequest.from_dict(current_request_data) + + n = len(active_requests) + if n > 1: + raise Exception( + f"Multiple active release requests for user {user.username} in workspace {workspace_name}" + ) + elif n == 1: + return ReleaseRequest.from_dict(active_requests[0]) + elif create: + # To create a request, you must have explicit workspace permissions. + # Output checkers can view all workspaces, but are not allowed to + # create requests for all workspaces. + if workspace_name not in user.workspaces: + raise ProviderAPI.RequestPermissionDenied(workspace_name) + + new_request = self._dal.create_release_request( + workspace=workspace_name, + author=user.username, + ) + return ReleaseRequest.from_dict(new_request) + else: + return None def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]: """Get all current requests authored by user.""" diff --git a/local_db/api.py b/local_db/api.py index 2fc83db3..f9ea5340 100644 --- a/local_db/api.py +++ b/local_db/api.py @@ -62,34 +62,15 @@ def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): def get_release_request(self, request_id: str): return self._request(self._find_metadata(request_id)) - def get_current_request( - self, workspace: str, username: str, user_workspaces: list[str], create=False - ): - requests = list( - RequestMetadata.objects.filter( + def get_active_requests_for_workspace_by_user(self, workspace: str, username: str): + return [ + self._request(request) + for request in RequestMetadata.objects.filter( workspace=workspace, author=username, status__in=[Status.PENDING, Status.SUBMITTED], ) - ) - n = len(requests) - if n > 1: - raise Exception( - f"Multiple active release requests for user {username} in workspace {workspace}" - ) - elif n == 1: - return self._request(requests[0]) - elif create: - # To create a request, you must have explicit workspace permissions. - # Output checkers can view all workspaces, but are not allowed to - # create requests for all workspaces. - if workspace not in user_workspaces: - raise ProviderAPI.RequestPermissionDenied(workspace) - - return self.create_release_request( - workspace=workspace, - author=username, - ) + ] def get_requests_authored_by_user(self, username: str): return [ From 27eb6c53a3d434b9552bf412c96989cc29a6e463 Mon Sep 17 00:00:00 2001 From: David Evans Date: Thu, 7 Mar 2024 12:34:33 +0000 Subject: [PATCH 6/9] Move business logic out of the DAL, part 2 --- airlock/api.py | 10 +++++++--- local_db/api.py | 19 +++++-------------- 2 files changed, 12 insertions(+), 17 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index b90435ba..dbaccb2f 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -359,11 +359,15 @@ def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]: def get_outstanding_requests_for_review(self, user: User): """Get all request that need review.""" + # Only output checkers can see these + if not user.output_checker: + return [] + return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_outstanding_requests_for_review( - username=user.username, user_is_output_checker=user.output_checker - ) + for attrs in self._dal.get_outstanding_requests_for_review() + # Do not show output_checker their own requests + if attrs["author"] != user.username ] VALID_STATE_TRANSITIONS = { diff --git a/local_db/api.py b/local_db/api.py index f9ea5340..32677563 100644 --- a/local_db/api.py +++ b/local_db/api.py @@ -80,20 +80,11 @@ def get_requests_authored_by_user(self, username: str): ) ] - def get_outstanding_requests_for_review( - self, username: str, user_is_output_checker: bool - ): - requests = [] - - if not user_is_output_checker: - return [] - - for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED): - # do not show output_checker their own requests - if metadata.author != username: - requests.append(self._request(metadata)) - - return requests + def get_outstanding_requests_for_review(self): + return [ + self._request(metadata) + for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED) + ] def set_status(self, request_id: str, status: Status): with transaction.atomic(): From 6a6458d5d49979891c665c81a99086d4c7061699 Mon Sep 17 00:00:00 2001 From: David Evans Date: Fri, 8 Mar 2024 14:52:08 +0000 Subject: [PATCH 7/9] Rename `local_db.api` to `local_db.data_access` --- airlock/settings.py | 2 +- local_db/{api.py => data_access.py} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename local_db/{api.py => data_access.py} (100%) diff --git a/airlock/settings.py b/airlock/settings.py index 0eb841fd..a4ad8411 100644 --- a/airlock/settings.py +++ b/airlock/settings.py @@ -268,7 +268,7 @@ def run_connection_init_queries(*, connection, **kwargs): f"variables must be set.\n\n{_missing_env_var_hint}" ) -AIRLOCK_DATA_ACCESS_LAYER = "local_db.api.LocalDBDataAccessLayer" +AIRLOCK_DATA_ACCESS_LAYER = "local_db.data_access.LocalDBDataAccessLayer" # BACKEND is global env var on backends BACKEND = os.environ.get("BACKEND", "test") diff --git a/local_db/api.py b/local_db/data_access.py similarity index 100% rename from local_db/api.py rename to local_db/data_access.py From 26d45a2855878c91336b2c628615159f8de49d95 Mon Sep 17 00:00:00 2001 From: David Evans Date: Fri, 8 Mar 2024 16:24:17 +0000 Subject: [PATCH 8/9] Rename `ProviderAPI` to `BusinessLogicLayer` --- airlock/api.py | 16 +++++++--------- local_db/data_access.py | 6 +++--- tests/unit/test_api.py | 8 ++++---- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/airlock/api.py b/airlock/api.py index dbaccb2f..27e2cede 100644 --- a/airlock/api.py +++ b/airlock/api.py @@ -75,7 +75,7 @@ class Workspace: def __post_init__(self): if not self.root().exists(): - raise ProviderAPI.WorkspaceNotFound(self.name) + raise BusinessLogicLayer.WorkspaceNotFound(self.name) def root(self): return settings.WORKSPACE_DIR / self.name @@ -107,7 +107,7 @@ def abspath(self, relpath): # validate path exists if not path.exists(): - raise ProviderAPI.FileNotFound(path) + raise BusinessLogicLayer.FileNotFound(path) return path @@ -214,7 +214,7 @@ def abspath(self, relpath): group = relpath.parts[0] if group not in self.filegroups: - raise ProviderAPI.FileNotFound(f"bad group {group} in url {relpath}") + raise BusinessLogicLayer.FileNotFound(f"bad group {group} in url {relpath}") filepath = relpath.relative_to(group) path = root / filepath @@ -224,7 +224,7 @@ def abspath(self, relpath): # validate path exists if not path.exists(): - raise ProviderAPI.FileNotFound(path) + raise BusinessLogicLayer.FileNotFound(path) return path @@ -246,10 +246,8 @@ class DataAccessLayerProtocol: """ -class ProviderAPI: +class BusinessLogicLayer: """ - Business Logic Layer (class will be renamed later) - The mechanism via which the rest of the codebase should read and write application state. Interacts with a Data Access Layer purely by exchanging simple values (dictionaries, strings etc). @@ -340,7 +338,7 @@ def get_current_request( # Output checkers can view all workspaces, but are not allowed to # create requests for all workspaces. if workspace_name not in user.workspaces: - raise ProviderAPI.RequestPermissionDenied(workspace_name) + raise BusinessLogicLayer.RequestPermissionDenied(workspace_name) new_request = self._dal.create_release_request( workspace=workspace_name, @@ -509,7 +507,7 @@ def release_files(self, request: ReleaseRequest, user: User): def _get_configured_api(): DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) - return ProviderAPI(DataAccessLayer()) + return BusinessLogicLayer(DataAccessLayer()) # We follow the Django pattern of using a lazy object which configures itself on first diff --git a/local_db/data_access.py b/local_db/data_access.py index 32677563..83745b3d 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -3,8 +3,8 @@ from django.db import transaction from airlock.api import ( + BusinessLogicLayer, DataAccessLayerProtocol, - ProviderAPI, Status, ) from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata @@ -34,7 +34,7 @@ def _find_metadata(self, request_id: str): try: return RequestMetadata.objects.get(id=request_id) except RequestMetadata.DoesNotExist: - raise ProviderAPI.ReleaseRequestNotFound(request_id) + raise BusinessLogicLayer.ReleaseRequestNotFound(request_id) def _filegroup(self, filegroup_metadata: FileGroupMetadata): """Unpack file group db data into FileGroup and RequestFile objects.""" @@ -110,7 +110,7 @@ def add_file_to_request(self, request_id, relpath: Path, group_name: str): relpath=str(relpath), filegroup=filegroupmetadata ) else: - raise ProviderAPI.APIException( + raise BusinessLogicLayer.APIException( "File has already been added to request " f"(in file group '{existing_file.filegroup.name}')" ) diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index 7bd21f9f..ad47eafc 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_api.py @@ -6,7 +6,7 @@ from django.conf import settings import old_api -from airlock.api import ProviderAPI, Status, UrlPath, Workspace +from airlock.api import BusinessLogicLayer, Status, UrlPath, Workspace from airlock.users import User from tests import factories @@ -56,7 +56,7 @@ def test_provider_get_workspaces_for_user(user_workspaces, output_checker, expec factories.create_workspace("not-allowed") user = User(1, "test", user_workspaces, output_checker) - api = ProviderAPI(data_access_layer=None) + api = BusinessLogicLayer(data_access_layer=None) assert set(api.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) @@ -79,7 +79,7 @@ def test_provider_request_release_files_not_approved(): status=Status.SUBMITTED, ) - api = ProviderAPI(data_access_layer=None) + api = BusinessLogicLayer(data_access_layer=None) with pytest.raises(api.InvalidStateTransition): api.release_files(release_request, checker) @@ -100,7 +100,7 @@ def test_provider_request_release_files(mock_old_api): abspath = release_request.abspath("group" / relpath) - api = ProviderAPI(data_access_layer=Mock()) + api = BusinessLogicLayer(data_access_layer=Mock()) api.release_files(release_request, checker) expected_json = { From 32ebad85eedf1547a2577158fcc35a2be6d0d4f5 Mon Sep 17 00:00:00 2001 From: David Evans Date: Fri, 8 Mar 2024 16:41:06 +0000 Subject: [PATCH 9/9] Rename `api` module and `api` singleton --- airlock/{api.py => business_logic.py} | 4 +- airlock/file_browser_api.py | 2 +- airlock/views/helpers.py | 10 +- airlock/views/request.py | 24 ++-- airlock/views/workspace.py | 16 +-- local_db/data_access.py | 2 +- .../migrations/0002_requestmetadata_status.py | 6 +- local_db/models.py | 2 +- tests/conftest.py | 11 +- tests/factories.py | 16 +-- tests/functional/test_e2e.py | 4 +- tests/integration/views/test_request.py | 10 +- tests/integration/views/test_workspace.py | 20 +-- .../{test_api.py => test_business_logic.py} | 128 +++++++++--------- 14 files changed, 129 insertions(+), 126 deletions(-) rename airlock/{api.py => business_logic.py} (99%) rename tests/unit/{test_api.py => test_business_logic.py} (76%) diff --git a/airlock/api.py b/airlock/business_logic.py similarity index 99% rename from airlock/api.py rename to airlock/business_logic.py index 27e2cede..ddeb9276 100644 --- a/airlock/api.py +++ b/airlock/business_logic.py @@ -505,11 +505,11 @@ def release_files(self, request: ReleaseRequest, user: User): self.set_status(request, Status.RELEASED, user) -def _get_configured_api(): +def _get_configured_bll(): DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) return BusinessLogicLayer(DataAccessLayer()) # We follow the Django pattern of using a lazy object which configures itself on first # access so as to avoid reading `settings` during import -api = SimpleLazyObject(_get_configured_api) +bll = SimpleLazyObject(_get_configured_bll) diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 9ca4e4c4..c65046d5 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -1,7 +1,7 @@ from dataclasses import dataclass, field from enum import Enum -from airlock.api import ROOT_PATH, AirlockContainer, UrlPath +from airlock.business_logic import ROOT_PATH, AirlockContainer, UrlPath class PathType(Enum): diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py index 911202d4..a69998c7 100644 --- a/airlock/views/helpers.py +++ b/airlock/views/helpers.py @@ -3,7 +3,7 @@ from django.core.exceptions import PermissionDenied from django.http import FileResponse, Http404 -from airlock.api import api +from airlock.business_logic import bll def login_exempt(view): @@ -14,8 +14,8 @@ def login_exempt(view): def validate_workspace(user, workspace_name): """Ensure the workspace exists and the current user has permissions to access it.""" try: - workspace = api.get_workspace(workspace_name) - except api.WorkspaceNotFound: + workspace = bll.get_workspace(workspace_name) + except bll.WorkspaceNotFound: raise Http404() if user is None or not user.has_permission(workspace_name): @@ -27,8 +27,8 @@ def validate_workspace(user, workspace_name): def validate_release_request(user, request_id): """Ensure the release request exists for this workspace.""" try: - release_request = api.get_release_request(request_id) - except api.ReleaseRequestNotFound: + release_request = bll.get_release_request(request_id) + except bll.ReleaseRequestNotFound: raise Http404() # check user permissions for this workspace diff --git a/airlock/views/request.py b/airlock/views/request.py index 7a95ffa7..c1713cdc 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -9,18 +9,18 @@ from django.views.decorators.http import require_http_methods from django.views.decorators.vary import vary_on_headers -from airlock.api import Status, api +from airlock.business_logic import Status, bll from airlock.file_browser_api import get_request_tree from .helpers import serve_file, validate_release_request def request_index(request): - authored_requests = api.get_requests_authored_by_user(request.user) + authored_requests = bll.get_requests_authored_by_user(request.user) outstanding_requests = [] if request.user.output_checker: - outstanding_requests = api.get_outstanding_requests_for_review(request.user) + outstanding_requests = bll.get_outstanding_requests_for_review(request.user) return TemplateResponse( request, @@ -74,7 +74,7 @@ def request_view(request, request_id: str, path: str = ""): ) context = { - "workspace": api.get_workspace(release_request.workspace), + "workspace": bll.get_workspace(release_request.workspace), "release_request": release_request, "root": tree, "path_item": path_item, @@ -97,7 +97,7 @@ def request_contents(request, request_id: str, path: str): try: abspath = release_request.abspath(path) - except api.FileNotFound: + except bll.FileNotFound: raise Http404() if not abspath.is_file(): @@ -121,8 +121,8 @@ def request_submit(request, request_id): release_request = validate_release_request(request.user, request_id) try: - api.set_status(release_request, Status.SUBMITTED, request.user) - except api.RequestPermissionDenied as exc: + bll.set_status(release_request, Status.SUBMITTED, request.user) + except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) messages.success(request, "Request has been submitted") @@ -134,8 +134,8 @@ def request_reject(request, request_id): release_request = validate_release_request(request.user, request_id) try: - api.set_status(release_request, Status.REJECTED, request.user) - except api.RequestPermissionDenied as exc: + bll.set_status(release_request, Status.REJECTED, request.user) + except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) messages.error(request, "Request has been rejected") @@ -148,9 +148,9 @@ def request_release_files(request, request_id): try: # For now, we just implicitly approve when release files is requested - api.set_status(release_request, Status.APPROVED, request.user) - api.release_files(release_request, request.user) - except api.RequestPermissionDenied as exc: + bll.set_status(release_request, Status.APPROVED, request.user) + bll.release_files(release_request, request.user) + except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) except requests.HTTPError as err: if settings.DEBUG: diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index 584a6b3a..ffc129c6 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -6,7 +6,7 @@ from django.views.decorators.http import require_http_methods from django.views.decorators.vary import vary_on_headers -from airlock.api import UrlPath, api +from airlock.business_logic import UrlPath, bll from airlock.file_browser_api import get_workspace_tree from airlock.forms import AddFileForm @@ -14,7 +14,7 @@ def workspace_index(request): - workspaces = api.get_workspaces_for_user(request.user) + workspaces = bll.get_workspaces_for_user(request.user) return TemplateResponse(request, "workspaces.html", {"workspaces": workspaces}) @@ -41,7 +41,7 @@ def workspace_view(request, workspace_name: str, path: str = ""): if path_item.is_directory() != is_directory_url: return redirect(path_item.url()) - current_request = api.get_current_request(workspace_name, request.user) + current_request = bll.get_current_request(workspace_name, request.user) # Only include the AddFileForm if this pathitem is a file that # can be added to a request - i.e. it is a file and it's not @@ -88,7 +88,7 @@ def workspace_contents(request, workspace_name: str, path: str): try: abspath = workspace.abspath(path) - except api.FileNotFound: + except bll.FileNotFound: raise Http404() if not abspath.is_file(): @@ -103,16 +103,16 @@ def workspace_add_file_to_request(request, workspace_name): relpath = UrlPath(request.POST["path"]) try: workspace.abspath(relpath) - except api.FileNotFound: + except bll.FileNotFound: raise Http404() - release_request = api.get_current_request(workspace_name, request.user, create=True) + release_request = bll.get_current_request(workspace_name, request.user, create=True) form = AddFileForm(request.POST, release_request=release_request) if form.is_valid(): group_name = request.POST.get("new_filegroup") or request.POST.get("filegroup") try: - api.add_file_to_request(release_request, relpath, request.user, group_name) - except api.APIException as err: + bll.add_file_to_request(release_request, relpath, request.user, group_name) + except bll.APIException as err: # This exception is raised if the file has already been added # (to any group on the request) messages.error(request, str(err)) diff --git a/local_db/data_access.py b/local_db/data_access.py index 83745b3d..2aed2908 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -2,7 +2,7 @@ from django.db import transaction -from airlock.api import ( +from airlock.business_logic import ( BusinessLogicLayer, DataAccessLayerProtocol, Status, diff --git a/local_db/migrations/0002_requestmetadata_status.py b/local_db/migrations/0002_requestmetadata_status.py index 239f4730..b5b14c1f 100644 --- a/local_db/migrations/0002_requestmetadata_status.py +++ b/local_db/migrations/0002_requestmetadata_status.py @@ -2,7 +2,7 @@ from django.db import migrations -import airlock.api +import airlock.business_logic import local_db.models @@ -15,6 +15,8 @@ class Migration(migrations.Migration): migrations.AddField( model_name="requestmetadata", name="status", - field=local_db.models.StatusField(default=airlock.api.Status["PENDING"]), + field=local_db.models.StatusField( + default=airlock.business_logic.Status["PENDING"] + ), ), ] diff --git a/local_db/models.py b/local_db/models.py index 4f846487..2e8c8149 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -2,7 +2,7 @@ from django.utils import timezone from ulid import ulid -from airlock.api import Status +from airlock.business_logic import Status def local_request_id(): diff --git a/tests/conftest.py b/tests/conftest.py index 7ea062e8..2de6899a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,7 @@ import responses as _responses from django.conf import settings -import airlock.api +import airlock.business_logic import old_api import tests.factories @@ -31,11 +31,12 @@ def responses(): yield rsps -# we could parameterise this fixture to run tests over all api implementations in future +# We could parameterise this fixture to run tests over all Data Access Layer +# implementations in future @pytest.fixture -def api(monkeypatch): - monkeypatch.setattr(tests.factories, "api", airlock.api.api) - return airlock.api.api +def bll(monkeypatch): + monkeypatch.setattr(tests.factories, "bll", airlock.business_logic.bll) + return airlock.business_logic.bll @pytest.fixture diff --git a/tests/factories.py b/tests/factories.py index 0bc9d08d..40ffe874 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,6 +1,6 @@ from django.conf import settings -from airlock.api import Workspace, api +from airlock.business_logic import Workspace, bll from airlock.users import User @@ -23,7 +23,7 @@ def ensure_workspace(workspace_or_name): def create_workspace(name): workspace_dir = settings.WORKSPACE_DIR / name workspace_dir.mkdir(exist_ok=True, parents=True) - return api.get_workspace(name) + return bll.get_workspace(name) def write_workspace_file(workspace, path, contents=""): @@ -40,7 +40,7 @@ def create_release_request(workspace, user=None, **kwargs): if user is None: user = get_user(workspaces=[workspace.name]) - release_request = api._create_release_request( + release_request = bll._create_release_request( workspace=workspace.name, author=user.username, **kwargs ) release_request.root().mkdir(parents=True, exist_ok=True) @@ -51,23 +51,23 @@ def write_request_file(request, group, path, contents="", user=None): workspace = ensure_workspace(request.workspace) try: workspace.abspath(path) - except api.FileNotFound: + except bll.FileNotFound: write_workspace_file(workspace, path, contents) # create a default user with permission on workspace if user is None: # pragma: nocover user = get_user(username=request.author, workspaces=[request.workspace]) - api.add_file_to_request(request, relpath=path, user=user, group_name=group) + bll.add_file_to_request(request, relpath=path, user=user, group_name=group) def create_filegroup(release_request, group_name, filepaths=None): for filepath in filepaths or []: # pragma: nocover - api.add_file_to_request( + bll.add_file_to_request( release_request, filepath, User(1, release_request.author), group_name ) - return api._dal._get_or_create_filegroupmetadata(release_request.id, group_name) + return bll._dal._get_or_create_filegroupmetadata(release_request.id, group_name) def refresh_release_request(release_request): - return api.get_release_request(release_request.id) + return bll.get_release_request(release_request.id) diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index cd862a66..a0120c4c 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -4,7 +4,7 @@ import pytest from playwright.sync_api import expect -from airlock.api import Status, api +from airlock.business_logic import Status, bll from tests import factories @@ -192,7 +192,7 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber): assert download.suggested_filename == "file.txt" # Mock the responses from job-server - release_request = api.get_release_request(request_id) + release_request = bll.get_release_request(request_id) release_files_stubber(release_request) # Release the files diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index e31d4f98..73c218a6 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -3,7 +3,7 @@ import pytest import requests -from airlock.api import Status +from airlock.business_logic import Status from airlock.users import User from tests import factories @@ -304,7 +304,7 @@ def test_request_submit_author(client_with_user): response = permitted_client.post(f"/requests/submit/{release_request.id}") assert response.status_code == 302 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.SUBMITTED @@ -319,7 +319,7 @@ def test_request_submit_not_author(client_with_user): response = permitted_client.post(f"/requests/submit/{release_request.id}") assert response.status_code == 403 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.PENDING @@ -335,7 +335,7 @@ def test_request_reject_output_checker(client_with_permission): response = client_with_permission.post(f"/requests/reject/{release_request.id}") assert response.status_code == 302 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.REJECTED @@ -352,7 +352,7 @@ def test_request_reject_not_output_checker(client_with_user): response = client.post(f"/requests/reject/{release_request.id}") assert response.status_code == 403 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.SUBMITTED diff --git a/tests/integration/views/test_workspace.py b/tests/integration/views/test_workspace.py index 4ee98369..12e53d4f 100644 --- a/tests/integration/views/test_workspace.py +++ b/tests/integration/views/test_workspace.py @@ -233,28 +233,28 @@ def test_workspaces_index_user_permitted_workspaces(client_with_user): assert "test2" not in response.rendered_content -def test_workspace_request_file_creates(client_with_user, api): +def test_workspace_request_file_creates(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) workspace = factories.create_workspace("test1") factories.write_workspace_file(workspace, "test/path.txt") - assert api.get_current_request(workspace.name, user) is None + assert bll.get_current_request(workspace.name, user) is None response = client.post( "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt", "filegroup": "default"}, ) assert response.status_code == 302 - release_request = api.get_current_request(workspace.name, user) + release_request = bll.get_current_request(workspace.name, user) filegroup = release_request.filegroups["default"] assert filegroup.name == "default" assert str(filegroup.files[0].relpath) == "test/path.txt" assert release_request.abspath("default/test/path.txt").exists() -def test_workspace_request_file_request_already_exists(client_with_user, api): +def test_workspace_request_file_request_already_exists(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) @@ -268,7 +268,7 @@ def test_workspace_request_file_request_already_exists(client_with_user, api): data={"path": "test/path.txt", "filegroup": "default"}, ) assert response.status_code == 302 - current_release_request = api.get_current_request(workspace.name, user) + current_release_request = bll.get_current_request(workspace.name, user) assert current_release_request.id == release_request.id assert current_release_request.abspath("default/test/path.txt").exists() filegroup = current_release_request.filegroups["default"] @@ -276,14 +276,14 @@ def test_workspace_request_file_request_already_exists(client_with_user, api): assert str(filegroup.files[0].relpath) == "test/path.txt" -def test_workspace_request_file_with_new_filegroup(client_with_user, api): +def test_workspace_request_file_with_new_filegroup(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) workspace = factories.create_workspace("test1") factories.write_workspace_file(workspace, "test/path.txt") - assert api.get_current_request(workspace.name, user) is None + assert bll.get_current_request(workspace.name, user) is None response = client.post( "/workspaces/add-file-to-request/test1", data={ @@ -295,12 +295,12 @@ def test_workspace_request_file_with_new_filegroup(client_with_user, api): ) assert response.status_code == 302 - release_request = api.get_current_request(workspace.name, user) + release_request = bll.get_current_request(workspace.name, user) filegroup = release_request.filegroups["new_group"] assert filegroup.name == "new_group" -def test_workspace_request_file_filegroup_already_exists(client_with_user, api): +def test_workspace_request_file_filegroup_already_exists(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) @@ -342,7 +342,7 @@ def test_workspace_request_file_request_path_does_not_exist(client_with_user): assert response.status_code == 404 -def test_workspace_request_file_invalid_new_filegroup(client_with_user, api): +def test_workspace_request_file_invalid_new_filegroup(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) diff --git a/tests/unit/test_api.py b/tests/unit/test_business_logic.py similarity index 76% rename from tests/unit/test_api.py rename to tests/unit/test_business_logic.py index ad47eafc..91651643 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_business_logic.py @@ -6,7 +6,7 @@ from django.conf import settings import old_api -from airlock.api import BusinessLogicLayer, Status, UrlPath, Workspace +from airlock.business_logic import BusinessLogicLayer, Status, UrlPath, Workspace from airlock.users import User from tests import factories @@ -56,9 +56,9 @@ def test_provider_get_workspaces_for_user(user_workspaces, output_checker, expec factories.create_workspace("not-allowed") user = User(1, "test", user_workspaces, output_checker) - api = BusinessLogicLayer(data_access_layer=None) + bll = BusinessLogicLayer(data_access_layer=None) - assert set(api.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) + assert set(bll.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) @pytest.fixture @@ -79,9 +79,9 @@ def test_provider_request_release_files_not_approved(): status=Status.SUBMITTED, ) - api = BusinessLogicLayer(data_access_layer=None) - with pytest.raises(api.InvalidStateTransition): - api.release_files(release_request, checker) + bll = BusinessLogicLayer(data_access_layer=None) + with pytest.raises(bll.InvalidStateTransition): + bll.release_files(release_request, checker) def test_provider_request_release_files(mock_old_api): @@ -96,12 +96,12 @@ def test_provider_request_release_files(mock_old_api): ) relpath = Path("test/file.txt") factories.write_request_file(release_request, "group", relpath, "test") - factories.api.set_status(release_request, Status.APPROVED, checker) + factories.bll.set_status(release_request, Status.APPROVED, checker) abspath = release_request.abspath("group" / relpath) - api = BusinessLogicLayer(data_access_layer=Mock()) - api.release_files(release_request, checker) + bll = BusinessLogicLayer(data_access_layer=Mock()) + bll.release_files(release_request, checker) expected_json = { "files": [ @@ -127,13 +127,13 @@ def test_provider_request_release_files(mock_old_api): ) -def test_provider_get_requests_authored_by_user(api): +def test_provider_get_requests_authored_by_user(bll): user = User(1, "test", [], True) other_user = User(1, "other", [], True) factories.create_release_request("workspace", user, id="r1") factories.create_release_request("workspace", other_user, id="r2") - assert [r.id for r in api.get_requests_authored_by_user(user)] == ["r1"] + assert [r.id for r in bll.get_requests_authored_by_user(user)] == ["r1"] @pytest.mark.parametrize( @@ -146,7 +146,7 @@ def test_provider_get_requests_authored_by_user(api): (True, ["r1"]), ], ) -def test_provider_get_outstanding_requests_for_review(output_checker, expected, api): +def test_provider_get_outstanding_requests_for_review(output_checker, expected, bll): user = User(1, "test", ["workspace"], output_checker) other_user = User(1, "other", ["workspace"], False) # request created by another user, status submitted @@ -172,39 +172,39 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, ws = f"workspace{i}" factories.create_release_request(ws, User(1, f"test_{i}", [ws]), status=status) - assert set(r.id for r in api.get_outstanding_requests_for_review(user)) == set( + assert set(r.id for r in bll.get_outstanding_requests_for_review(user)) == set( expected ) -def test_provider_get_current_request_for_user(api): +def test_provider_get_current_request_for_user(bll): workspace = factories.create_workspace("workspace") user = User(1, "testuser", ["workspace"], False) other_user = User(2, "otheruser", ["workspace"], False) - assert api.get_current_request("workspace", user) is None + assert bll.get_current_request("workspace", user) is None factories.create_release_request(workspace, other_user) - assert api.get_current_request("workspace", user) is None + assert bll.get_current_request("workspace", user) is None - release_request = api.get_current_request("workspace", user, create=True) + release_request = bll.get_current_request("workspace", user, create=True) assert release_request.workspace == "workspace" assert release_request.author == user.username # reach around an simulate 2 active requests for same user - api._create_release_request(author=user.username, workspace="workspace") + bll._create_release_request(author=user.username, workspace="workspace") with pytest.raises(Exception): - api.get_current_request("workspace", user) + bll.get_current_request("workspace", user) -def test_provider_get_current_request_for_user_output_checker(api): +def test_provider_get_current_request_for_user_output_checker(bll): """Output checker must have explict workspace permissions to create requests.""" factories.create_workspace("workspace") user = User(1, "output_checker", [], True) - with pytest.raises(api.RequestPermissionDenied): - api.get_current_request("workspace", user, create=True) + with pytest.raises(bll.RequestPermissionDenied): + bll.get_current_request("workspace", user, create=True) @pytest.mark.parametrize( @@ -235,7 +235,7 @@ def test_provider_get_current_request_for_user_output_checker(api): (Status.RELEASED, Status.WITHDRAWN, False, False), ], ) -def test_set_status(current, future, valid_author, valid_checker, api): +def test_set_status(current, future, valid_author, valid_checker, bll): author = User(1, "author", ["workspace"], False) checker = User(2, "checker", [], True) release_request1 = factories.create_release_request( @@ -246,30 +246,30 @@ def test_set_status(current, future, valid_author, valid_checker, api): ) if valid_author: - api.set_status(release_request1, future, user=author) + bll.set_status(release_request1, future, user=author) assert release_request1.status == future else: - with pytest.raises((api.InvalidStateTransition, api.RequestPermissionDenied)): - api.set_status(release_request1, future, user=author) + with pytest.raises((bll.InvalidStateTransition, bll.RequestPermissionDenied)): + bll.set_status(release_request1, future, user=author) if valid_checker: - api.set_status(release_request2, future, user=checker) + bll.set_status(release_request2, future, user=checker) assert release_request2.status == future else: - with pytest.raises((api.InvalidStateTransition, api.RequestPermissionDenied)): - api.set_status(release_request2, future, user=checker) + with pytest.raises((bll.InvalidStateTransition, bll.RequestPermissionDenied)): + bll.set_status(release_request2, future, user=checker) -def test_set_status_cannot_action_own_request(api): +def test_set_status_cannot_action_own_request(bll): user = User(2, "checker", [], True) release_request1 = factories.create_release_request( "workspace", user=user, status=Status.SUBMITTED ) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request1, Status.APPROVED, user=user) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request1, Status.REJECTED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request1, Status.APPROVED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request1, Status.REJECTED, user=user) release_request2 = factories.create_release_request( "workspace", @@ -277,11 +277,11 @@ def test_set_status_cannot_action_own_request(api): status=Status.APPROVED, ) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request2, Status.RELEASED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request2, Status.RELEASED, user=user) -def test_add_file_to_request_not_author(api): +def test_add_file_to_request_not_author(bll): author = User(1, "author", ["workspace"], False) other = User(1, "other", ["workspace"], True) @@ -293,8 +293,8 @@ def test_add_file_to_request_not_author(api): user=author, ) - with pytest.raises(api.RequestPermissionDenied): - api.add_file_to_request(release_request, path, other) + with pytest.raises(bll.RequestPermissionDenied): + bll.add_file_to_request(release_request, path, other) @pytest.mark.parametrize( @@ -308,7 +308,7 @@ def test_add_file_to_request_not_author(api): (Status.WITHDRAWN, False), ], ) -def test_add_file_to_request_states(status, success, api): +def test_add_file_to_request_states(status, success, bll): author = User(1, "author", ["workspace"], False) path = Path("path/file.txt") @@ -321,11 +321,11 @@ def test_add_file_to_request_states(status, success, api): ) if success: - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert release_request.abspath("default" / path).exists() else: - with pytest.raises(api.RequestPermissionDenied): - api.add_file_to_request(release_request, path, author) + with pytest.raises(bll.RequestPermissionDenied): + bll.add_file_to_request(release_request, path, author) def test_request_release_invalid_state(): @@ -337,15 +337,15 @@ def test_request_release_invalid_state(): ) -def test_request_release_abspath(api): +def test_request_release_abspath(bll): path = UrlPath("foo/bar.txt") release_request = factories.create_release_request("id") factories.write_request_file(release_request, "default", path) - with pytest.raises(api.FileNotFound): + with pytest.raises(bll.FileNotFound): release_request.abspath("badgroup" / path) - with pytest.raises(api.FileNotFound): + with pytest.raises(bll.FileNotFound): release_request.abspath("default/does/not/exist") assert release_request.abspath("default" / path).exists() @@ -363,15 +363,15 @@ def setup_empty_release_request(): return release_request, path, author -def test_release_request_filegroups_with_no_files(api): +def test_release_request_filegroups_with_no_files(bll): release_request, _, _ = setup_empty_release_request() assert release_request.filegroups == {} -def test_release_request_filegroups_default_filegroup(api): +def test_release_request_filegroups_default_filegroup(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert len(release_request.filegroups) == 1 filegroup = release_request.filegroups["default"] assert filegroup.name == "default" @@ -379,10 +379,10 @@ def test_release_request_filegroups_default_filegroup(api): assert filegroup.files[0].relpath == path -def test_release_request_filegroups_named_filegroup(api): +def test_release_request_filegroups_named_filegroup(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author, "test_group") + bll.add_file_to_request(release_request, path, author, "test_group") assert len(release_request.filegroups) == 1 filegroup = release_request.filegroups["test_group"] assert filegroup.name == "test_group" @@ -390,20 +390,20 @@ def test_release_request_filegroups_named_filegroup(api): assert filegroup.files[0].relpath == path -def test_release_request_filegroups_multiple_filegroups(api): +def test_release_request_filegroups_multiple_filegroups(bll): release_request, path, author = setup_empty_release_request() - api.add_file_to_request(release_request, path, author, "test_group") + bll.add_file_to_request(release_request, path, author, "test_group") assert len(release_request.filegroups) == 1 - workspace = api.get_workspace("workspace") + workspace = bll.get_workspace("workspace") path1 = Path("path/file1.txt") path2 = Path("path/file2.txt") factories.write_workspace_file(workspace, path1) factories.write_workspace_file(workspace, path2) - api.add_file_to_request(release_request, path1, author, "test_group") - api.add_file_to_request(release_request, path2, author, "test_group1") + bll.add_file_to_request(release_request, path1, author, "test_group") + bll.add_file_to_request(release_request, path2, author, "test_group1") - release_request = api.get_release_request(release_request.id) + release_request = bll.get_release_request(release_request.id) assert len(release_request.filegroups) == 2 release_request_files = { @@ -417,22 +417,22 @@ def test_release_request_filegroups_multiple_filegroups(api): } -def test_release_request_add_same_file(api): +def test_release_request_add_same_file(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert len(release_request.filegroups) == 1 assert len(release_request.filegroups["default"].files) == 1 # Adding the same file again should not create a new RequestFile - with pytest.raises(api.APIException): - api.add_file_to_request(release_request, path, author) + with pytest.raises(bll.APIException): + bll.add_file_to_request(release_request, path, author) # We also can't add the same file to a different group - with pytest.raises(api.APIException): - api.add_file_to_request(release_request, path, author, "new_group") + with pytest.raises(bll.APIException): + bll.add_file_to_request(release_request, path, author, "new_group") - release_request = api.get_release_request(release_request.id) + release_request = bll.get_release_request(release_request.id) # No additional files or groups have been created assert len(release_request.filegroups) == 1 assert len(release_request.filegroups["default"].files) == 1