From e375813d269ac387b1ffba206c34756b0c555dac Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Wed, 13 Mar 2024 16:19:42 +0000 Subject: [PATCH 01/15] Rename Status to RequestStatus --- airlock/business_logic.py | 46 +++++----- airlock/views/request.py | 8 +- local_db/data_access.py | 8 +- .../migrations/0002_requestmetadata_status.py | 2 +- local_db/models.py | 8 +- tests/functional/test_e2e.py | 4 +- tests/integration/views/test_request.py | 36 ++++---- tests/unit/test_business_logic.py | 90 +++++++++---------- 8 files changed, 101 insertions(+), 101 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 4d7d9b32..4b7af013 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -23,7 +23,7 @@ ROOT_PATH = UrlPath() # empty path -class Status(Enum): +class RequestStatus(Enum): """Status for release Requests""" # author set statuses @@ -198,7 +198,7 @@ class ReleaseRequest: workspace: str author: str created_at: datetime - status: Status = Status.PENDING + status: RequestStatus = RequestStatus.PENDING filegroups: dict[FileGroup] = field(default_factory=dict) # can be set to mark the currently selected path in this release request @@ -455,28 +455,28 @@ def get_outstanding_requests_for_review(self, user: User): ] VALID_STATE_TRANSITIONS = { - Status.PENDING: [ - Status.SUBMITTED, - Status.WITHDRAWN, + RequestStatus.PENDING: [ + RequestStatus.SUBMITTED, + RequestStatus.WITHDRAWN, ], - Status.SUBMITTED: [ - Status.APPROVED, - Status.REJECTED, - Status.PENDING, # allow un-submission - Status.WITHDRAWN, + RequestStatus.SUBMITTED: [ + RequestStatus.APPROVED, + RequestStatus.REJECTED, + RequestStatus.PENDING, # allow un-submission + RequestStatus.WITHDRAWN, ], - Status.APPROVED: [ - Status.RELEASED, - Status.REJECTED, # allow fixing mistake *before* release - Status.WITHDRAWN, # allow user to withdraw before released + RequestStatus.APPROVED: [ + RequestStatus.RELEASED, + RequestStatus.REJECTED, # allow fixing mistake *before* release + RequestStatus.WITHDRAWN, # allow user to withdraw before released ], - Status.REJECTED: [ - Status.APPROVED, # allow mind changed + RequestStatus.REJECTED: [ + RequestStatus.APPROVED, # allow mind changed ], } def check_status( - self, release_request: ReleaseRequest, to_status: Status, user: User + self, release_request: ReleaseRequest, to_status: RequestStatus, user: User ): """Check that a given status transtion is valid for this request and this user. @@ -493,14 +493,14 @@ def check_status( # check permissions # author transitions - if to_status in [Status.PENDING, Status.SUBMITTED, Status.WITHDRAWN]: + if to_status in [RequestStatus.PENDING, RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN]: if user.username != release_request.author: raise self.RequestPermissionDenied( f"only {user.username} can set status to {to_status.name}" ) # output checker transitions - if to_status in [Status.APPROVED, Status.REJECTED, Status.RELEASED]: + if to_status in [RequestStatus.APPROVED, RequestStatus.REJECTED, RequestStatus.RELEASED]: if not user.output_checker: raise self.RequestPermissionDenied( f"only an output checker can set status to {to_status.name}" @@ -512,7 +512,7 @@ def check_status( ) def set_status( - self, release_request: ReleaseRequest, to_status: Status, user: User + self, release_request: ReleaseRequest, to_status: RequestStatus, user: User ): """Set the status of the request. @@ -546,7 +546,7 @@ def add_file_to_request( f"only author {release_request.author} can add files to this request" ) - if release_request.status not in [Status.PENDING, Status.SUBMITTED]: + if release_request.status not in [RequestStatus.PENDING, RequestStatus.SUBMITTED]: raise self.RequestPermissionDenied( f"cannot add file to request in state {release_request.status.name}" ) @@ -573,7 +573,7 @@ def release_files(self, request: ReleaseRequest, user: User): """ # we check this is valid status transition *before* releasing the files - self.check_status(request, Status.RELEASED, user) + self.check_status(request, RequestStatus.RELEASED, user) file_paths = request.get_output_file_paths() filelist = old_api.create_filelist(file_paths) @@ -584,7 +584,7 @@ def release_files(self, request: ReleaseRequest, user: User): for relpath, abspath in file_paths: old_api.upload_file(jobserver_release_id, relpath, abspath, user.username) - self.set_status(request, Status.RELEASED, user) + self.set_status(request, RequestStatus.RELEASED, user) def _get_configured_bll(): diff --git a/airlock/views/request.py b/airlock/views/request.py index 7044c5b8..f8458de1 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -10,7 +10,7 @@ from django.views.decorators.vary import vary_on_headers from opentelemetry import trace -from airlock.business_logic import Status, bll +from airlock.business_logic import RequestStatus, bll from airlock.file_browser_api import get_request_tree from services.tracing import instrument @@ -130,7 +130,7 @@ def request_submit(request, request_id): release_request = get_release_request_or_raise(request.user, request_id) try: - bll.set_status(release_request, Status.SUBMITTED, request.user) + bll.set_status(release_request, RequestStatus.SUBMITTED, request.user) except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) @@ -144,7 +144,7 @@ def request_reject(request, request_id): release_request = get_release_request_or_raise(request.user, request_id) try: - bll.set_status(release_request, Status.REJECTED, request.user) + bll.set_status(release_request, RequestStatus.REJECTED, request.user) except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) @@ -159,7 +159,7 @@ def request_release_files(request, request_id): try: # For now, we just implicitly approve when release files is requested - bll.set_status(release_request, Status.APPROVED, request.user) + bll.set_status(release_request, RequestStatus.APPROVED, request.user) bll.release_files(release_request, request.user) except bll.RequestPermissionDenied as exc: raise PermissionDenied(str(exc)) diff --git a/local_db/data_access.py b/local_db/data_access.py index a110f55f..c435453e 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -6,7 +6,7 @@ BusinessLogicLayer, DataAccessLayerProtocol, RequestFileType, - Status, + RequestStatus, ) from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata @@ -76,7 +76,7 @@ def get_active_requests_for_workspace_by_user(self, workspace: str, username: st for request in RequestMetadata.objects.filter( workspace=workspace, author=username, - status__in=[Status.PENDING, Status.SUBMITTED], + status__in=[RequestStatus.PENDING, RequestStatus.SUBMITTED], ) ] @@ -91,10 +91,10 @@ def get_requests_authored_by_user(self, username: str): def get_outstanding_requests_for_review(self): return [ self._request(metadata) - for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED) + for metadata in RequestMetadata.objects.filter(status=RequestStatus.SUBMITTED) ] - def set_status(self, request_id: str, status: Status): + def set_status(self, request_id: str, status: RequestStatus): with transaction.atomic(): # persist state change metadata = self._find_metadata(request_id) diff --git a/local_db/migrations/0002_requestmetadata_status.py b/local_db/migrations/0002_requestmetadata_status.py index 202e7844..3703dabd 100644 --- a/local_db/migrations/0002_requestmetadata_status.py +++ b/local_db/migrations/0002_requestmetadata_status.py @@ -16,7 +16,7 @@ class Migration(migrations.Migration): model_name="requestmetadata", name="status", field=local_db.models.EnumField( - default=airlock.business_logic.Status["PENDING"] + default=airlock.business_logic.RequestStatus["PENDING"] ), ), ] diff --git a/local_db/models.py b/local_db/models.py index 7ffa4638..9f459470 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.business_logic import RequestFileType, Status +from airlock.business_logic import RequestFileType, RequestStatus def local_request_id(): @@ -14,10 +14,10 @@ class EnumField(models.TextField): Specifically, data is stored in the db as the string name, e.g. "PENDING"`, and when loaded from the db, is deserialized into the correct - enum instance e.g. `Status.PENDING`. + enum instance e.g. `RequestStatus.PENDING`. """ - def __init__(self, *args, enum=Status, **kwargs): + def __init__(self, *args, enum=RequestStatus, **kwargs): self.enum = enum self.choices = [(i.value, i.name) for i in self.enum] super().__init__(*args, **kwargs) @@ -43,7 +43,7 @@ class RequestMetadata(models.Model): ) workspace = models.TextField() - status = EnumField(default=Status.PENDING, enum=Status) + status = EnumField(default=RequestStatus.PENDING, enum=Status) author = models.TextField() # just username, as we have no User model created_at = models.DateTimeField(default=timezone.now) diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index 81c5c75c..d128ca1a 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.business_logic import Status, bll +from airlock.business_logic import RequestStatus, bll from tests import factories @@ -335,7 +335,7 @@ def test_e2e_reject_request(page, live_server, dev_users): factories.write_workspace_file("test-workspace", "file.txt") release_request = factories.create_release_request( "test-workspace", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.create_filegroup( release_request, group_name="default", filepaths=["file.txt"] diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index ca7bf4d5..71c89b99 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.business_logic import RequestFileType, Status +from airlock.business_logic import RequestFileType, RequestStatus from tests import factories from tests.conftest import get_trace @@ -84,7 +84,7 @@ def test_request_view_with_file_htmx(airlock_client): def test_request_view_with_submitted_request(airlock_client): airlock_client.login(output_checker=True) release_request = factories.create_release_request( - "workspace", status=Status.SUBMITTED + "workspace", status=RequestStatus.SUBMITTED ) response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) assert "Reject Request" in response.rendered_content @@ -96,7 +96,7 @@ def test_request_view_with_authored_request_file(airlock_client): release_request = factories.create_release_request( "workspace", user=airlock_client.user, - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "file.txt", "foobar") response = airlock_client.get( @@ -303,10 +303,10 @@ def test_request_index_user_output_checker(airlock_client): airlock_client.login(workspaces=["test_workspace"], output_checker=True) other = factories.create_user("other") r1 = factories.create_release_request( - "test_workspace", user=airlock_client.user, status=Status.SUBMITTED + "test_workspace", user=airlock_client.user, status=RequestStatus.SUBMITTED ) r2 = factories.create_release_request( - "other_workspace", user=other, status=Status.SUBMITTED + "other_workspace", user=other, status=RequestStatus.SUBMITTED ) response = airlock_client.get("/requests/") @@ -329,14 +329,14 @@ def test_request_submit_author(airlock_client): persisted_request = factories.bll.get_release_request( release_request.id, airlock_client.user ) - assert persisted_request.status == Status.SUBMITTED + assert persisted_request.status == RequestStatus.SUBMITTED def test_request_submit_not_author(airlock_client): airlock_client.login(workspaces=["test1"]) other_user = factories.create_user("other", [], False) release_request = factories.create_release_request( - "test1", user=other_user, status=Status.PENDING + "test1", user=other_user, status=RequestStatus.PENDING ) factories.write_request_file(release_request, "group", "path/test.txt") @@ -346,7 +346,7 @@ def test_request_submit_not_author(airlock_client): persisted_request = factories.bll.get_release_request( release_request.id, airlock_client.user ) - assert persisted_request.status == Status.PENDING + assert persisted_request.status == RequestStatus.PENDING def test_request_reject_output_checker(airlock_client): @@ -355,7 +355,7 @@ def test_request_reject_output_checker(airlock_client): release_request = factories.create_release_request( "test1", user=author, - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "path/test.txt") @@ -365,13 +365,13 @@ def test_request_reject_output_checker(airlock_client): persisted_request = factories.bll.get_release_request( release_request.id, airlock_client.user ) - assert persisted_request.status == Status.REJECTED + assert persisted_request.status == RequestStatus.REJECTED def test_request_reject_not_output_checker(airlock_client): release_request = factories.create_release_request( "test1", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) airlock_client.login(workspaces=[release_request.workspace], output_checker=False) factories.write_request_file(release_request, "group", "path/test.txt") @@ -382,7 +382,7 @@ def test_request_reject_not_output_checker(airlock_client): persisted_request = factories.bll.get_release_request( release_request.id, airlock_client.user ) - assert persisted_request.status == Status.SUBMITTED + assert persisted_request.status == RequestStatus.SUBMITTED def test_request_release_files_success(airlock_client, release_files_stubber): @@ -390,7 +390,7 @@ def test_request_release_files_success(airlock_client, release_files_stubber): release_request = factories.create_release_request( "workspace", id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "test/file1.txt", "test1") factories.write_request_file(release_request, "group", "test/file2.txt", "test2") @@ -409,7 +409,7 @@ def test_requests_release_workspace_403(airlock_client): release_request = factories.create_release_request( "workspace", id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "test/file1.txt", "test1") response = airlock_client.post("/requests/release/request_id") @@ -422,7 +422,7 @@ def test_requests_release_author_403(airlock_client): "workspace", id="request_id", user=airlock_client.user, - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "test/file1.txt", "test1") response = airlock_client.post("/requests/release/request_id") @@ -434,7 +434,7 @@ def test_requests_release_jobserver_403(airlock_client, release_files_stubber): release_request = factories.create_release_request( "workspace", id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "test/file.txt", "test") @@ -469,7 +469,7 @@ def test_requests_release_jobserver_403_with_debug( release_request = factories.create_release_request( "workspace", id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "default", "test/file.txt", "test") @@ -494,7 +494,7 @@ def test_requests_release_files_404(airlock_client, release_files_stubber): release_request = factories.create_release_request( "workspace", id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) factories.write_request_file(release_request, "group", "test/file.txt", "test") diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 692cc819..bb67fcf4 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -9,7 +9,7 @@ from airlock.business_logic import ( BusinessLogicLayer, RequestFileType, - Status, + RequestStatus, UrlPath, Workspace, ) @@ -90,7 +90,7 @@ def test_provider_request_release_files_not_approved(): "workspace", user=author, id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) bll = BusinessLogicLayer(data_access_layer=None) @@ -106,7 +106,7 @@ def test_provider_request_release_files(mock_old_api): "workspace", user=author, id="request_id", - status=Status.SUBMITTED, + status=RequestStatus.SUBMITTED, ) relpath = Path("test/file.txt") factories.write_request_file(release_request, "group", relpath, "test") @@ -119,7 +119,7 @@ def test_provider_request_release_files(mock_old_api): "test", filetype=RequestFileType.SUPPORTING, ) - factories.bll.set_status(release_request, Status.APPROVED, checker) + factories.bll.set_status(release_request, RequestStatus.APPROVED, checker) abspath = release_request.abspath("group" / relpath) @@ -174,22 +174,22 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, other_user = factories.create_user("other", ["workspace"], False) # request created by another user, status submitted factories.create_release_request( - "workspace", other_user, id="r1", status=Status.SUBMITTED + "workspace", other_user, id="r1", status=RequestStatus.SUBMITTED ) # requests not visible to output checker # status submitted, but authored by output checker factories.create_release_request( - "workspace", user, id="r2", status=Status.SUBMITTED + "workspace", user, id="r2", status=RequestStatus.SUBMITTED ) # requests authored by other users, status other than pending for i, status in enumerate( [ - Status.PENDING, - Status.WITHDRAWN, - Status.APPROVED, - Status.REJECTED, - Status.RELEASED, + RequestStatus.PENDING, + RequestStatus.WITHDRAWN, + RequestStatus.APPROVED, + RequestStatus.REJECTED, + RequestStatus.RELEASED, ] ): ws = f"workspace{i}" @@ -234,29 +234,29 @@ def test_provider_get_current_request_for_user_output_checker(bll): @pytest.mark.parametrize( "current,future,valid_author,valid_checker", [ - (Status.PENDING, Status.SUBMITTED, True, False), - (Status.PENDING, Status.WITHDRAWN, True, False), - (Status.PENDING, Status.APPROVED, False, False), - (Status.PENDING, Status.REJECTED, False, False), - (Status.PENDING, Status.RELEASED, False, False), - (Status.SUBMITTED, Status.APPROVED, False, True), - (Status.SUBMITTED, Status.REJECTED, False, True), - (Status.SUBMITTED, Status.WITHDRAWN, True, False), - (Status.SUBMITTED, Status.PENDING, True, False), - (Status.SUBMITTED, Status.RELEASED, False, False), - (Status.APPROVED, Status.RELEASED, False, True), - (Status.APPROVED, Status.REJECTED, False, True), - (Status.APPROVED, Status.WITHDRAWN, True, False), - (Status.REJECTED, Status.PENDING, False, False), - (Status.REJECTED, Status.SUBMITTED, False, False), - (Status.REJECTED, Status.APPROVED, False, True), - (Status.REJECTED, Status.WITHDRAWN, False, False), - (Status.RELEASED, Status.REJECTED, False, False), - (Status.RELEASED, Status.PENDING, False, False), - (Status.RELEASED, Status.SUBMITTED, False, False), - (Status.RELEASED, Status.APPROVED, False, False), - (Status.RELEASED, Status.REJECTED, False, False), - (Status.RELEASED, Status.WITHDRAWN, False, False), + (RequestStatus.PENDING, RequestStatus.SUBMITTED, True, False), + (RequestStatus.PENDING, RequestStatus.WITHDRAWN, True, False), + (RequestStatus.PENDING, RequestStatus.APPROVED, False, False), + (RequestStatus.PENDING, RequestStatus.REJECTED, False, False), + (RequestStatus.PENDING, RequestStatus.RELEASED, False, False), + (RequestStatus.SUBMITTED, RequestStatus.APPROVED, False, True), + (RequestStatus.SUBMITTED, RequestStatus.REJECTED, False, True), + (RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN, True, False), + (RequestStatus.SUBMITTED, RequestStatus.PENDING, True, False), + (RequestStatus.SUBMITTED, RequestStatus.RELEASED, False, False), + (RequestStatus.APPROVED, RequestStatus.RELEASED, False, True), + (RequestStatus.APPROVED, RequestStatus.REJECTED, False, True), + (RequestStatus.APPROVED, RequestStatus.WITHDRAWN, True, False), + (RequestStatus.REJECTED, RequestStatus.PENDING, False, False), + (RequestStatus.REJECTED, RequestStatus.SUBMITTED, False, False), + (RequestStatus.REJECTED, RequestStatus.APPROVED, False, True), + (RequestStatus.REJECTED, RequestStatus.WITHDRAWN, False, False), + (RequestStatus.RELEASED, RequestStatus.REJECTED, False, False), + (RequestStatus.RELEASED, RequestStatus.PENDING, False, False), + (RequestStatus.RELEASED, RequestStatus.SUBMITTED, False, False), + (RequestStatus.RELEASED, RequestStatus.APPROVED, False, False), + (RequestStatus.RELEASED, RequestStatus.REJECTED, False, False), + (RequestStatus.RELEASED, RequestStatus.WITHDRAWN, False, False), ], ) def test_set_status(current, future, valid_author, valid_checker, bll): @@ -287,22 +287,22 @@ def test_set_status(current, future, valid_author, valid_checker, bll): def test_set_status_cannot_action_own_request(bll): user = factories.create_user("checker", [], True) release_request1 = factories.create_release_request( - "workspace", user=user, status=Status.SUBMITTED + "workspace", user=user, status=RequestStatus.SUBMITTED ) with pytest.raises(bll.RequestPermissionDenied): - bll.set_status(release_request1, Status.APPROVED, user=user) + bll.set_status(release_request1, RequestStatus.APPROVED, user=user) with pytest.raises(bll.RequestPermissionDenied): - bll.set_status(release_request1, Status.REJECTED, user=user) + bll.set_status(release_request1, RequestStatus.REJECTED, user=user) release_request2 = factories.create_release_request( "workspace", user=user, - status=Status.APPROVED, + status=RequestStatus.APPROVED, ) with pytest.raises(bll.RequestPermissionDenied): - bll.set_status(release_request2, Status.RELEASED, user=user) + bll.set_status(release_request2, RequestStatus.RELEASED, user=user) def test_add_file_to_request_not_author(bll): @@ -324,12 +324,12 @@ def test_add_file_to_request_not_author(bll): @pytest.mark.parametrize( "status,success", [ - (Status.PENDING, True), - (Status.SUBMITTED, True), - (Status.APPROVED, False), - (Status.REJECTED, False), - (Status.RELEASED, False), - (Status.WITHDRAWN, False), + (RequestStatus.PENDING, True), + (RequestStatus.SUBMITTED, True), + (RequestStatus.APPROVED, False), + (RequestStatus.REJECTED, False), + (RequestStatus.RELEASED, False), + (RequestStatus.WITHDRAWN, False), ], ) def test_add_file_to_request_states(status, success, bll): From 64de32aeef4e8999afe8cbbfb1144fd44d41ce98 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Thu, 14 Mar 2024 15:42:47 +0000 Subject: [PATCH 02/15] add FileApprovalStatus --- airlock/business_logic.py | 5 +++++ local_db/models.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 4b7af013..de3e67d0 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -41,6 +41,11 @@ class RequestFileType(Enum): SUPPORTING = "supporting" +class FileApprovalStatus(Enum): + APPROVED = "APPROVED" + REJECTED = "REJECTED" + + class AirlockContainer(Protocol): """Structural typing class for a instance of a Workspace or ReleaseRequest diff --git a/local_db/models.py b/local_db/models.py index 9f459470..fc28d44e 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -43,7 +43,7 @@ class RequestMetadata(models.Model): ) workspace = models.TextField() - status = EnumField(default=RequestStatus.PENDING, enum=Status) + status = EnumField(default=RequestStatus.PENDING, enum=RequestStatus) author = models.TextField() # just username, as we have no User model created_at = models.DateTimeField(default=timezone.now) From bac48bd5cd69662a830c0c9452a7e970035b6156 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Thu, 14 Mar 2024 15:51:24 +0000 Subject: [PATCH 03/15] Create FileReview model --- local_db/migrations/0006_filereview.py | 31 ++++++++++++++++++++++++++ local_db/models.py | 18 ++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 local_db/migrations/0006_filereview.py diff --git a/local_db/migrations/0006_filereview.py b/local_db/migrations/0006_filereview.py new file mode 100644 index 00000000..abe1bd87 --- /dev/null +++ b/local_db/migrations/0006_filereview.py @@ -0,0 +1,31 @@ +# Generated by Django 5.0.2 on 2024-03-19 09:50 + +import airlock.business_logic +import django.db.models.deletion +import django.utils.timezone +import local_db.models +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('local_db', '0005_requestfilemetadata_filetype'), + ] + + operations = [ + migrations.CreateModel( + name='FileReview', + fields=[ + ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('reviewer', models.TextField()), + ('status', local_db.models.EnumField(default=airlock.business_logic.FileApprovalStatus['REJECTED'])), + ('created_at', models.DateTimeField(default=django.utils.timezone.now)), + ('updated_at', models.DateTimeField(default=django.utils.timezone.now)), + ('file', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='reviews', to='local_db.requestfilemetadata')), + ], + options={ + 'unique_together': {('file', 'reviewer')}, + }, + ), + ] diff --git a/local_db/models.py b/local_db/models.py index fc28d44e..ce42325f 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.business_logic import RequestFileType, RequestStatus +from airlock.business_logic import FileApprovalStatus, RequestFileType, RequestStatus def local_request_id(): @@ -74,3 +74,19 @@ class RequestFileMetadata(models.Model): class Meta: unique_together = ("relpath", "filegroup") + +class FileReview(models.Model): + """An output checker's review of a file""" + + file = models.ForeignKey( + RequestFileMetadata, related_name="reviews", on_delete=models.CASCADE + ) + reviewer = models.TextField() + status = EnumField( + default=FileApprovalStatus.REJECTED, enum=FileApprovalStatus + ) + created_at = models.DateTimeField(default=timezone.now) + updated_at = models.DateTimeField(default=timezone.now) + + class Meta: + unique_together = ("file", "reviewer") From ba38dc87ef5bbebfe4ca0ed633d085098168a8c3 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Thu, 14 Mar 2024 17:06:58 +0000 Subject: [PATCH 04/15] Create approve_file methods --- airlock/business_logic.py | 38 +++++++++++++++++ local_db/data_access.py | 27 +++++++++++- tests/unit/test_business_logic.py | 70 +++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 1 deletion(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index de3e67d0..b4f9894f 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -356,6 +356,9 @@ class WorkspacePermissionDenied(APIException): class RequestPermissionDenied(APIException): pass + class ApprovalPermissionDenied(APIException): + pass + def get_workspace(self, name: str, user: User) -> Workspace: """Get a workspace object.""" @@ -591,6 +594,41 @@ def release_files(self, request: ReleaseRequest, user: User): self.set_status(request, RequestStatus.RELEASED, user) + def get_file_approvals(self, release_request: ReleaseRequest): + # TODO: return BusinessLogicLayer.FileReview not LocalDB.FileReview + return bll._dal.get_file_approvals(release_request.id) + + + def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): + """"Approve a file""" + + if release_request.status != RequestStatus.SUBMITTED: + raise self.ApprovalPermissionDenied( + f"cannot approve file from request in state {release_request.status.name}" + ) + + if user.username == release_request.author: + raise self.ApprovalPermissionDenied( + f"cannot approve files in your own request" + ) + + if not user.output_checker: + raise self.ApprovalPermissionDenied( + "only an output checker can approve a file" + ) + + if path not in release_request.file_set(): + raise self.ApprovalPermissionDenied( + f"file is not part of the request" + ) + + bll._dal.approve_file(release_request.id, user, path) + + def reject_file(self): + """Reject a file""" + + pass + def _get_configured_bll(): DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) diff --git a/local_db/data_access.py b/local_db/data_access.py index c435453e..f82f3ddc 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -8,7 +8,7 @@ RequestFileType, RequestStatus, ) -from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata +from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata, FileReview, FileApprovalStatus class LocalDBDataAccessLayer(DataAccessLayerProtocol): @@ -140,3 +140,28 @@ def add_file_to_request( # Return updated FileGroups data metadata = self._find_metadata(request_id) return self._get_filegroups(metadata) + + + def get_file_approvals(self, request_id): + return FileReview.objects.filter(file__filegroup__request_id=request_id).all() + + + def approve_file(self, request_id, user, relpath): + with transaction.atomic(): + try: + request_file = RequestFileMetadata.objects.get( + filegroup__request_id=request_id, relpath=relpath + ) + except RequestFileMetadata.DoesNotExist: + raise BusinessLogicLayer.FileNotFound( + f"file {relpath} not part of a request {request_id}" + ) + review, _ = FileReview.objects.get_or_create( + file=request_file, reviewer=user + ) + review.status = FileApprovalStatus.APPROVED + review.save() + + + def reject_file(self, request_id, user, relpath): + pass diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index bb67fcf4..bed45c23 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -8,6 +8,7 @@ import old_api from airlock.business_logic import ( BusinessLogicLayer, + FileApprovalStatus, RequestFileType, RequestStatus, UrlPath, @@ -559,3 +560,72 @@ def test_release_request_add_same_file(bll): # No additional files or groups have been created assert len(release_request.filegroups) == 1 assert len(release_request.filegroups["default"].files) == 1 + + +def test_approve_file_not_submitted(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request(release_request, path, author) + + with pytest.raises(bll.ApprovalPermissionDenied): + bll.approve_file(release_request, checker, path) + + +def test_approve_file_not_your_own(bll): + release_request, path, author = setup_empty_release_request() + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author + ) + + with pytest.raises(bll.ApprovalPermissionDenied): + bll.approve_file(release_request, author, path) + + +def test_approve_file_not_checker(bll): + release_request, path, author = setup_empty_release_request() + author2 = factories.create_user("author2", [], False) + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author + ) + + with pytest.raises(bll.ApprovalPermissionDenied): + bll.approve_file(release_request, author2, path) + + +def test_approve_file_not_part_of_request(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, + to_status=RequestStatus.SUBMITTED, + user=author + ) + + bad_path = Path("path/file2.txt") + with pytest.raises(bll.ApprovalPermissionDenied): + bll.approve_file(release_request, checker, bad_path) + + +def test_approve_file(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, + to_status=RequestStatus.SUBMITTED, + user=author + ) + + assert len(bll.get_file_approvals(release_request)) == 0 + bll.approve_file(release_request, checker, path) + assert len(bll.get_file_approvals(release_request)) == 1 + assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + From 3f80007f8662fa6fe0b082460227590a723d5746 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Fri, 15 Mar 2024 13:17:37 +0000 Subject: [PATCH 05/15] implement reject_file --- airlock/business_logic.py | 14 ++++++---- local_db/data_access.py | 17 +++++++++++- tests/unit/test_business_logic.py | 45 +++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 6 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index b4f9894f..b2b7638b 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -599,9 +599,7 @@ def get_file_approvals(self, release_request: ReleaseRequest): return bll._dal.get_file_approvals(release_request.id) - def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): - """"Approve a file""" - + def _verify_permission_to_review_file(self, release_request: ReleaseRequest, user: User, path: Path): if release_request.status != RequestStatus.SUBMITTED: raise self.ApprovalPermissionDenied( f"cannot approve file from request in state {release_request.status.name}" @@ -622,13 +620,19 @@ def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): f"file is not part of the request" ) + def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): + """"Approve a file""" + + self._verify_permission_to_review_file(release_request, user, path) + bll._dal.approve_file(release_request.id, user, path) - def reject_file(self): + def reject_file(self, release_request: ReleaseRequest, user: User, path: Path): """Reject a file""" - pass + self._verify_permission_to_review_file(release_request, user, path) + bll._dal.reject_file(release_request.id, user, path) def _get_configured_bll(): DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) diff --git a/local_db/data_access.py b/local_db/data_access.py index f82f3ddc..8a93c1ae 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -143,6 +143,7 @@ def add_file_to_request( def get_file_approvals(self, request_id): + # TODO: return dict return FileReview.objects.filter(file__filegroup__request_id=request_id).all() @@ -164,4 +165,18 @@ def approve_file(self, request_id, user, relpath): def reject_file(self, request_id, user, relpath): - pass + with transaction.atomic(): + try: + request_file = RequestFileMetadata.objects.get( + filegroup__request_id=request_id, relpath=relpath + ) + except RequestFileMetadata.DoesNotExist: + raise BusinessLogicLayer.FileNotFound( + f"file {relpath} not part of a request {request_id}" + ) + review, _ = FileReview.objects.get_or_create( + file=request_file, reviewer=user + ) + review.status = FileApprovalStatus.REJECTED + review.save() + diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index bed45c23..8f259c0d 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -625,7 +625,52 @@ def test_approve_file(bll): ) assert len(bll.get_file_approvals(release_request)) == 0 + bll.approve_file(release_request, checker, path) + assert len(bll.get_file_approvals(release_request)) == 1 assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + +def test_reject_file(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, + to_status=RequestStatus.SUBMITTED, + user=author + ) + + assert len(bll.get_file_approvals(release_request)) == 0 + + bll.reject_file(release_request, checker, path) + + assert len(bll.get_file_approvals(release_request)) == 1 + assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + + +def test_approve_then_reject_file(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request(release_request, path, author) + bll.set_status( + release_request=release_request, + to_status=RequestStatus.SUBMITTED, + user=author + ) + + assert len(bll.get_file_approvals(release_request)) == 0 + + bll.approve_file(release_request, checker, path) + + assert len(bll.get_file_approvals(release_request)) == 1 + assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + + bll.reject_file(release_request, checker, path) + + assert len(bll.get_file_approvals(release_request)) == 1 + assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + From 7d7d3d77606b06a3f8cadd7518035cc6f4e6e842 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Fri, 15 Mar 2024 15:53:33 +0000 Subject: [PATCH 06/15] Add business logic layer FileReview --- airlock/business_logic.py | 30 +++++++++++++++++++++++++++--- local_db/data_access.py | 17 +++++++++++++++-- tests/unit/test_business_logic.py | 2 +- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index b2b7638b..30e43034 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -318,6 +318,29 @@ def store_file(release_request: ReleaseRequest, abspath: Path) -> str: return digest +@dataclass(frozen=True) +class FileReview: + """ + Represents a review of a file in the context of a release request + """ + + file: RequestFile + reviewer: User + status: FileApprovalStatus + created_at: datetime + updated_at: datetime + release_request: ReleaseRequest + + @classmethod + def from_dict(cls, attrs): + # TODO implement + return cls( + **{k: v for k, v in attrs.items() if k != "release_request"}, + release_request=ReleaseRequest.from_dict(attrs.get("release_request", ())), + # files=[RequestFile.from_dict(value) for value in attrs.get("files", ())], + ) + + class DataAccessLayerProtocol: """ Placeholder for a structural type class we can use to define what a data access @@ -595,9 +618,10 @@ def release_files(self, request: ReleaseRequest, user: User): self.set_status(request, RequestStatus.RELEASED, user) def get_file_approvals(self, release_request: ReleaseRequest): - # TODO: return BusinessLogicLayer.FileReview not LocalDB.FileReview - return bll._dal.get_file_approvals(release_request.id) - + return [ + FileReview.from_dict(r) + for r in bll._dal.get_file_approvals(release_request.id) + ] def _verify_permission_to_review_file(self, release_request: ReleaseRequest, user: User, path: Path): if release_request.status != RequestStatus.SUBMITTED: diff --git a/local_db/data_access.py b/local_db/data_access.py index 8a93c1ae..ea23ff3d 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -60,6 +60,17 @@ def _get_filegroups(self, metadata: RequestMetadata): for group_metadata in metadata.filegroups.all() } + def _filereview(self, file_review: FileReview): + """Convert a FileReview object into a dict""" + return dict( + release_request=self._request(file_review.file.filegroup.request), + file=self._request_file(file_review.file), + reviewer=file_review.reviewer, + status=file_review.status, + created_at=file_review.created_at, + updated_at=file_review.updated_at, + ) + def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): metadata = self._find_metadata(request_id) groupmetadata, _ = FileGroupMetadata.objects.get_or_create( @@ -143,8 +154,10 @@ def add_file_to_request( def get_file_approvals(self, request_id): - # TODO: return dict - return FileReview.objects.filter(file__filegroup__request_id=request_id).all() + return [ + self._filereview(r) + for r in FileReview.objects.filter(file__filegroup__request_id=request_id).all() + ] def approve_file(self, request_id, user, relpath): diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 8f259c0d..c72c0c26 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -630,6 +630,7 @@ def test_approve_file(bll): assert len(bll.get_file_approvals(release_request)) == 1 assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + assert type(bll.get_file_approvals(release_request)[0]) == FileReview def test_reject_file(bll): @@ -673,4 +674,3 @@ def test_approve_then_reject_file(bll): assert len(bll.get_file_approvals(release_request)) == 1 assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED - From a461e489ebdf4ef9bf4dd258057ddd2b2c958a8c Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Fri, 15 Mar 2024 16:09:49 +0000 Subject: [PATCH 07/15] fix ruff --- airlock/business_logic.py | 30 ++++++++++++------ local_db/data_access.py | 22 ++++++++----- local_db/migrations/0006_filereview.py | 44 +++++++++++++++++++------- local_db/models.py | 5 ++- tests/unit/test_business_logic.py | 33 +++++++++---------- 5 files changed, 86 insertions(+), 48 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 30e43034..3d68fa41 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -524,14 +524,22 @@ def check_status( # check permissions # author transitions - if to_status in [RequestStatus.PENDING, RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN]: + if to_status in [ + RequestStatus.PENDING, + RequestStatus.SUBMITTED, + RequestStatus.WITHDRAWN, + ]: if user.username != release_request.author: raise self.RequestPermissionDenied( f"only {user.username} can set status to {to_status.name}" ) # output checker transitions - if to_status in [RequestStatus.APPROVED, RequestStatus.REJECTED, RequestStatus.RELEASED]: + if to_status in [ + RequestStatus.APPROVED, + RequestStatus.REJECTED, + RequestStatus.RELEASED, + ]: if not user.output_checker: raise self.RequestPermissionDenied( f"only an output checker can set status to {to_status.name}" @@ -577,7 +585,10 @@ def add_file_to_request( f"only author {release_request.author} can add files to this request" ) - if release_request.status not in [RequestStatus.PENDING, RequestStatus.SUBMITTED]: + if release_request.status not in [ + RequestStatus.PENDING, + RequestStatus.SUBMITTED, + ]: raise self.RequestPermissionDenied( f"cannot add file to request in state {release_request.status.name}" ) @@ -623,7 +634,9 @@ def get_file_approvals(self, release_request: ReleaseRequest): for r in bll._dal.get_file_approvals(release_request.id) ] - def _verify_permission_to_review_file(self, release_request: ReleaseRequest, user: User, path: Path): + def _verify_permission_to_review_file( + self, release_request: ReleaseRequest, user: User, path: Path + ): if release_request.status != RequestStatus.SUBMITTED: raise self.ApprovalPermissionDenied( f"cannot approve file from request in state {release_request.status.name}" @@ -631,7 +644,7 @@ def _verify_permission_to_review_file(self, release_request: ReleaseRequest, use if user.username == release_request.author: raise self.ApprovalPermissionDenied( - f"cannot approve files in your own request" + "cannot approve files in your own request" ) if not user.output_checker: @@ -640,12 +653,10 @@ def _verify_permission_to_review_file(self, release_request: ReleaseRequest, use ) if path not in release_request.file_set(): - raise self.ApprovalPermissionDenied( - f"file is not part of the request" - ) + raise self.ApprovalPermissionDenied("file is not part of the request") def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): - """"Approve a file""" + """ "Approve a file""" self._verify_permission_to_review_file(release_request, user, path) @@ -658,6 +669,7 @@ def reject_file(self, release_request: ReleaseRequest, user: User, path: Path): bll._dal.reject_file(release_request.id, user, path) + def _get_configured_bll(): DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER) return BusinessLogicLayer(DataAccessLayer()) diff --git a/local_db/data_access.py b/local_db/data_access.py index ea23ff3d..7068c6a9 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -8,7 +8,13 @@ RequestFileType, RequestStatus, ) -from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata, FileReview, FileApprovalStatus +from local_db.models import ( + FileApprovalStatus, + FileGroupMetadata, + FileReview, + RequestFileMetadata, + RequestMetadata, +) class LocalDBDataAccessLayer(DataAccessLayerProtocol): @@ -68,7 +74,7 @@ def _filereview(self, file_review: FileReview): reviewer=file_review.reviewer, status=file_review.status, created_at=file_review.created_at, - updated_at=file_review.updated_at, + updated_at=file_review.updated_at, ) def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): @@ -102,7 +108,9 @@ def get_requests_authored_by_user(self, username: str): def get_outstanding_requests_for_review(self): return [ self._request(metadata) - for metadata in RequestMetadata.objects.filter(status=RequestStatus.SUBMITTED) + for metadata in RequestMetadata.objects.filter( + status=RequestStatus.SUBMITTED + ) ] def set_status(self, request_id: str, status: RequestStatus): @@ -152,14 +160,14 @@ def add_file_to_request( metadata = self._find_metadata(request_id) return self._get_filegroups(metadata) - def get_file_approvals(self, request_id): return [ self._filereview(r) - for r in FileReview.objects.filter(file__filegroup__request_id=request_id).all() + for r in FileReview.objects.filter( + file__filegroup__request_id=request_id + ).all() ] - def approve_file(self, request_id, user, relpath): with transaction.atomic(): try: @@ -176,7 +184,6 @@ def approve_file(self, request_id, user, relpath): review.status = FileApprovalStatus.APPROVED review.save() - def reject_file(self, request_id, user, relpath): with transaction.atomic(): try: @@ -192,4 +199,3 @@ def reject_file(self, request_id, user, relpath): ) review.status = FileApprovalStatus.REJECTED review.save() - diff --git a/local_db/migrations/0006_filereview.py b/local_db/migrations/0006_filereview.py index abe1bd87..8a042f29 100644 --- a/local_db/migrations/0006_filereview.py +++ b/local_db/migrations/0006_filereview.py @@ -1,31 +1,51 @@ # Generated by Django 5.0.2 on 2024-03-19 09:50 -import airlock.business_logic import django.db.models.deletion import django.utils.timezone -import local_db.models from django.db import migrations, models +import airlock.business_logic +import local_db.models + class Migration(migrations.Migration): - dependencies = [ - ('local_db', '0005_requestfilemetadata_filetype'), + ("local_db", "0005_requestfilemetadata_filetype"), ] operations = [ migrations.CreateModel( - name='FileReview', + name="FileReview", fields=[ - ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), - ('reviewer', models.TextField()), - ('status', local_db.models.EnumField(default=airlock.business_logic.FileApprovalStatus['REJECTED'])), - ('created_at', models.DateTimeField(default=django.utils.timezone.now)), - ('updated_at', models.DateTimeField(default=django.utils.timezone.now)), - ('file', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='reviews', to='local_db.requestfilemetadata')), + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("reviewer", models.TextField()), + ( + "status", + local_db.models.EnumField( + default=airlock.business_logic.FileApprovalStatus["REJECTED"] + ), + ), + ("created_at", models.DateTimeField(default=django.utils.timezone.now)), + ("updated_at", models.DateTimeField(default=django.utils.timezone.now)), + ( + "file", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="reviews", + to="local_db.requestfilemetadata", + ), + ), ], options={ - 'unique_together': {('file', 'reviewer')}, + "unique_together": {("file", "reviewer")}, }, ), ] diff --git a/local_db/models.py b/local_db/models.py index ce42325f..8438def2 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -75,6 +75,7 @@ class RequestFileMetadata(models.Model): class Meta: unique_together = ("relpath", "filegroup") + class FileReview(models.Model): """An output checker's review of a file""" @@ -82,9 +83,7 @@ class FileReview(models.Model): RequestFileMetadata, related_name="reviews", on_delete=models.CASCADE ) reviewer = models.TextField() - status = EnumField( - default=FileApprovalStatus.REJECTED, enum=FileApprovalStatus - ) + status = EnumField(default=FileApprovalStatus.REJECTED, enum=FileApprovalStatus) created_at = models.DateTimeField(default=timezone.now) updated_at = models.DateTimeField(default=timezone.now) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index c72c0c26..07129a05 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -9,6 +9,7 @@ from airlock.business_logic import ( BusinessLogicLayer, FileApprovalStatus, + FileReview, RequestFileType, RequestStatus, UrlPath, @@ -603,9 +604,7 @@ def test_approve_file_not_part_of_request(bll): bll.add_file_to_request(release_request, path, author) bll.set_status( - release_request=release_request, - to_status=RequestStatus.SUBMITTED, - user=author + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) bad_path = Path("path/file2.txt") @@ -619,9 +618,7 @@ def test_approve_file(bll): bll.add_file_to_request(release_request, path, author) bll.set_status( - release_request=release_request, - to_status=RequestStatus.SUBMITTED, - user=author + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) assert len(bll.get_file_approvals(release_request)) == 0 @@ -629,7 +626,9 @@ def test_approve_file(bll): bll.approve_file(release_request, checker, path) assert len(bll.get_file_approvals(release_request)) == 1 - assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + assert ( + bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + ) assert type(bll.get_file_approvals(release_request)[0]) == FileReview @@ -639,9 +638,7 @@ def test_reject_file(bll): bll.add_file_to_request(release_request, path, author) bll.set_status( - release_request=release_request, - to_status=RequestStatus.SUBMITTED, - user=author + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) assert len(bll.get_file_approvals(release_request)) == 0 @@ -649,7 +646,9 @@ def test_reject_file(bll): bll.reject_file(release_request, checker, path) assert len(bll.get_file_approvals(release_request)) == 1 - assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + assert ( + bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + ) def test_approve_then_reject_file(bll): @@ -658,9 +657,7 @@ def test_approve_then_reject_file(bll): bll.add_file_to_request(release_request, path, author) bll.set_status( - release_request=release_request, - to_status=RequestStatus.SUBMITTED, - user=author + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) assert len(bll.get_file_approvals(release_request)) == 0 @@ -668,9 +665,13 @@ def test_approve_then_reject_file(bll): bll.approve_file(release_request, checker, path) assert len(bll.get_file_approvals(release_request)) == 1 - assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + assert ( + bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED + ) bll.reject_file(release_request, checker, path) assert len(bll.get_file_approvals(release_request)) == 1 - assert bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + assert ( + bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED + ) From 30d2c36fa6be3b2cd187745e74230c7df9427711 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Fri, 15 Mar 2024 16:49:37 +0000 Subject: [PATCH 08/15] Remove error handlers * because we are only unit testing the BLL and not the DAL currently, these handlers cause test coverage to dip from 100% to 99.88%. --- local_db/data_access.py | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/local_db/data_access.py b/local_db/data_access.py index 7068c6a9..628913aa 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -170,14 +170,12 @@ def get_file_approvals(self, request_id): def approve_file(self, request_id, user, relpath): with transaction.atomic(): - try: - request_file = RequestFileMetadata.objects.get( - filegroup__request_id=request_id, relpath=relpath - ) - except RequestFileMetadata.DoesNotExist: - raise BusinessLogicLayer.FileNotFound( - f"file {relpath} not part of a request {request_id}" - ) + # nb. the business logic layer approve_file() should confirm that this path + # is part of the request before calling this method + request_file = RequestFileMetadata.objects.get( + filegroup__request_id=request_id, relpath=relpath + ) + review, _ = FileReview.objects.get_or_create( file=request_file, reviewer=user ) @@ -186,14 +184,10 @@ def approve_file(self, request_id, user, relpath): def reject_file(self, request_id, user, relpath): with transaction.atomic(): - try: - request_file = RequestFileMetadata.objects.get( - filegroup__request_id=request_id, relpath=relpath - ) - except RequestFileMetadata.DoesNotExist: - raise BusinessLogicLayer.FileNotFound( - f"file {relpath} not part of a request {request_id}" - ) + request_file = RequestFileMetadata.objects.get( + filegroup__request_id=request_id, relpath=relpath + ) + review, _ = FileReview.objects.get_or_create( file=request_file, reviewer=user ) From 2a9116ce2e28b421f0e77d46508e44b59723b621 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 19 Mar 2024 10:15:44 +0000 Subject: [PATCH 09/15] Clarify is_supporting_file argument is_supporting_file identifies whether a file is a supporting file from a UrlPath. This is different to a ReleaseFile's relpath (it includes the filegroup, which is part of the url for a release file, but is not part of the relpath, which comes from its location in the workspace). --- airlock/business_logic.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 3d68fa41..de83594d 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -285,9 +285,9 @@ def all_files_set(self): for request_file in filegroup.files.values() } - def is_supporting_file(self, relpath): + def is_supporting_file(self, urlpath: UrlPath): try: - return self.get_request_file(relpath).filetype == RequestFileType.SUPPORTING + return self.get_request_file(urlpath).filetype == RequestFileType.SUPPORTING except BusinessLogicLayer.FileNotFound: return False From 3e9ca97c60d59068c84d621989ba3f54254b25a8 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 19 Mar 2024 10:19:25 +0000 Subject: [PATCH 10/15] Raise ApprovalPermissionDenied for supporting files --- airlock/business_logic.py | 14 ++++++++++++-- tests/unit/test_business_logic.py | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index de83594d..8403f5be 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -285,6 +285,14 @@ def all_files_set(self): for request_file in filegroup.files.values() } + def output_files_set(self): + """Return the relpaths for output files on the request""" + return { + request_file.relpath + for filegroup in self.filegroups.values() + for request_file in filegroup.output_files + } + def is_supporting_file(self, urlpath: UrlPath): try: return self.get_request_file(urlpath).filetype == RequestFileType.SUPPORTING @@ -652,8 +660,10 @@ def _verify_permission_to_review_file( "only an output checker can approve a file" ) - if path not in release_request.file_set(): - raise self.ApprovalPermissionDenied("file is not part of the request") + if path not in release_request.output_files_set(): + raise self.ApprovalPermissionDenied( + "file is not an output file on this request" + ) def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): """ "Approve a file""" diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 07129a05..c8731046 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -612,6 +612,20 @@ def test_approve_file_not_part_of_request(bll): bll.approve_file(release_request, checker, bad_path) +def test_approve_supporting_file(bll): + release_request, path, author = setup_empty_release_request() + checker = factories.create_user("checker", [], True) + + bll.add_file_to_request( + release_request, path, author, filetype=RequestFileType.SUPPORTING + ) + bll.set_status( + release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author + ) + with pytest.raises(bll.ApprovalPermissionDenied): + bll.approve_file(release_request, checker, path) + + def test_approve_file(bll): release_request, path, author = setup_empty_release_request() checker = factories.create_user("checker", [], True) From cfbcbb58f85d534787309bcb2a9bd0ccae0e9a79 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Wed, 20 Mar 2024 15:33:48 +0000 Subject: [PATCH 11/15] Use convention of user as last arg * as per PR review --- airlock/business_logic.py | 20 ++++++++++++-------- local_db/data_access.py | 4 ++-- tests/unit/test_business_logic.py | 18 +++++++++--------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 8403f5be..47aa0538 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -643,7 +643,7 @@ def get_file_approvals(self, release_request: ReleaseRequest): ] def _verify_permission_to_review_file( - self, release_request: ReleaseRequest, user: User, path: Path + self, release_request: ReleaseRequest, relpath: UrlPath, user: User ): if release_request.status != RequestStatus.SUBMITTED: raise self.ApprovalPermissionDenied( @@ -660,24 +660,28 @@ def _verify_permission_to_review_file( "only an output checker can approve a file" ) - if path not in release_request.output_files_set(): + if relpath not in release_request.output_files_set(): raise self.ApprovalPermissionDenied( "file is not an output file on this request" ) - def approve_file(self, release_request: ReleaseRequest, user: User, path: Path): + def approve_file( + self, release_request: ReleaseRequest, relpath: UrlPath, user: User + ): """ "Approve a file""" - self._verify_permission_to_review_file(release_request, user, path) + self._verify_permission_to_review_file(release_request, relpath, user) - bll._dal.approve_file(release_request.id, user, path) + bll._dal.approve_file(release_request.id, relpath, user) - def reject_file(self, release_request: ReleaseRequest, user: User, path: Path): + def reject_file( + self, release_request: ReleaseRequest, relpath: UrlPath, user: User + ): """Reject a file""" - self._verify_permission_to_review_file(release_request, user, path) + self._verify_permission_to_review_file(release_request, relpath, user) - bll._dal.reject_file(release_request.id, user, path) + bll._dal.reject_file(release_request.id, relpath, user) def _get_configured_bll(): diff --git a/local_db/data_access.py b/local_db/data_access.py index 628913aa..33ecd09a 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -168,7 +168,7 @@ def get_file_approvals(self, request_id): ).all() ] - def approve_file(self, request_id, user, relpath): + def approve_file(self, request_id, relpath, user): with transaction.atomic(): # nb. the business logic layer approve_file() should confirm that this path # is part of the request before calling this method @@ -182,7 +182,7 @@ def approve_file(self, request_id, user, relpath): review.status = FileApprovalStatus.APPROVED review.save() - def reject_file(self, request_id, user, relpath): + def reject_file(self, request_id, relpath, user): with transaction.atomic(): request_file = RequestFileMetadata.objects.get( filegroup__request_id=request_id, relpath=relpath diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index c8731046..fcaa5b80 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -570,7 +570,7 @@ def test_approve_file_not_submitted(bll): bll.add_file_to_request(release_request, path, author) with pytest.raises(bll.ApprovalPermissionDenied): - bll.approve_file(release_request, checker, path) + bll.approve_file(release_request, path, checker) def test_approve_file_not_your_own(bll): @@ -582,7 +582,7 @@ def test_approve_file_not_your_own(bll): ) with pytest.raises(bll.ApprovalPermissionDenied): - bll.approve_file(release_request, author, path) + bll.approve_file(release_request, path, author) def test_approve_file_not_checker(bll): @@ -595,7 +595,7 @@ def test_approve_file_not_checker(bll): ) with pytest.raises(bll.ApprovalPermissionDenied): - bll.approve_file(release_request, author2, path) + bll.approve_file(release_request, path, author2) def test_approve_file_not_part_of_request(bll): @@ -609,7 +609,7 @@ def test_approve_file_not_part_of_request(bll): bad_path = Path("path/file2.txt") with pytest.raises(bll.ApprovalPermissionDenied): - bll.approve_file(release_request, checker, bad_path) + bll.approve_file(release_request, bad_path, checker) def test_approve_supporting_file(bll): @@ -623,7 +623,7 @@ def test_approve_supporting_file(bll): release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) with pytest.raises(bll.ApprovalPermissionDenied): - bll.approve_file(release_request, checker, path) + bll.approve_file(release_request, path, checker) def test_approve_file(bll): @@ -637,7 +637,7 @@ def test_approve_file(bll): assert len(bll.get_file_approvals(release_request)) == 0 - bll.approve_file(release_request, checker, path) + bll.approve_file(release_request, path, checker) assert len(bll.get_file_approvals(release_request)) == 1 assert ( @@ -657,7 +657,7 @@ def test_reject_file(bll): assert len(bll.get_file_approvals(release_request)) == 0 - bll.reject_file(release_request, checker, path) + bll.reject_file(release_request, path, checker) assert len(bll.get_file_approvals(release_request)) == 1 assert ( @@ -676,14 +676,14 @@ def test_approve_then_reject_file(bll): assert len(bll.get_file_approvals(release_request)) == 0 - bll.approve_file(release_request, checker, path) + bll.approve_file(release_request, path, checker) assert len(bll.get_file_approvals(release_request)) == 1 assert ( bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED ) - bll.reject_file(release_request, checker, path) + bll.reject_file(release_request, path, checker) assert len(bll.get_file_approvals(release_request)) == 1 assert ( From 0fb748875158e74cf34ef76862a4d2bed8c61b46 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Wed, 20 Mar 2024 15:35:51 +0000 Subject: [PATCH 12/15] auto-update the FileReview updated_at field --- local_db/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/local_db/models.py b/local_db/models.py index 8438def2..0f7599fb 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -85,7 +85,7 @@ class FileReview(models.Model): reviewer = models.TextField() status = EnumField(default=FileApprovalStatus.REJECTED, enum=FileApprovalStatus) created_at = models.DateTimeField(default=timezone.now) - updated_at = models.DateTimeField(default=timezone.now) + updated_at = models.DateTimeField(auto_now=True) class Meta: unique_together = ("file", "reviewer") From 33034dc820ca8e4a19962b98faa41048dc05bcf4 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Thu, 21 Mar 2024 14:54:38 +0000 Subject: [PATCH 13/15] remove dal.get_file_approvals in favour of RequestFile.reviews * as per PR review --- airlock/business_logic.py | 51 +++++++++++-------------- local_db/data_access.py | 18 +++------ tests/unit/test_business_logic.py | 63 +++++++++++++++++++++---------- 3 files changed, 70 insertions(+), 62 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 47aa0538..d23b2b2f 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -144,6 +144,22 @@ def is_supporting_file(self, relpath): return False +@dataclass(frozen=True) +class FileReview: + """ + Represents a review of a file in the context of a release request + """ + + reviewer: str + status: FileApprovalStatus + created_at: datetime + updated_at: datetime + + @classmethod + def from_dict(cls, attrs): + return cls(**attrs) + + @dataclass(frozen=True) class RequestFile: """ @@ -152,11 +168,15 @@ class RequestFile: relpath: UrlPath file_id: str + reviews: list[FileReview] filetype: RequestFileType = RequestFileType.OUTPUT @classmethod def from_dict(cls, attrs): - return cls(**attrs) + return cls( + **{k: v for k, v in attrs.items() if k != "reviews"}, + reviews=[FileReview.from_dict(value) for value in attrs.get("reviews", ())], + ) @dataclass(frozen=True) @@ -326,29 +346,6 @@ def store_file(release_request: ReleaseRequest, abspath: Path) -> str: return digest -@dataclass(frozen=True) -class FileReview: - """ - Represents a review of a file in the context of a release request - """ - - file: RequestFile - reviewer: User - status: FileApprovalStatus - created_at: datetime - updated_at: datetime - release_request: ReleaseRequest - - @classmethod - def from_dict(cls, attrs): - # TODO implement - return cls( - **{k: v for k, v in attrs.items() if k != "release_request"}, - release_request=ReleaseRequest.from_dict(attrs.get("release_request", ())), - # files=[RequestFile.from_dict(value) for value in attrs.get("files", ())], - ) - - class DataAccessLayerProtocol: """ Placeholder for a structural type class we can use to define what a data access @@ -636,12 +633,6 @@ def release_files(self, request: ReleaseRequest, user: User): self.set_status(request, RequestStatus.RELEASED, user) - def get_file_approvals(self, release_request: ReleaseRequest): - return [ - FileReview.from_dict(r) - for r in bll._dal.get_file_approvals(release_request.id) - ] - def _verify_permission_to_review_file( self, release_request: ReleaseRequest, relpath: UrlPath, user: User ): diff --git a/local_db/data_access.py b/local_db/data_access.py index 33ecd09a..78467010 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -48,6 +48,10 @@ def _request_file(self, file_metadata: RequestFileMetadata): relpath=Path(file_metadata.relpath), file_id=file_metadata.file_id, filetype=file_metadata.filetype, + reviews=[ + self._filereview(file_review) + for file_review in file_metadata.reviews.all() + ], ) def _filegroup(self, filegroup_metadata: FileGroupMetadata): @@ -69,8 +73,6 @@ def _get_filegroups(self, metadata: RequestMetadata): def _filereview(self, file_review: FileReview): """Convert a FileReview object into a dict""" return dict( - release_request=self._request(file_review.file.filegroup.request), - file=self._request_file(file_review.file), reviewer=file_review.reviewer, status=file_review.status, created_at=file_review.created_at, @@ -160,14 +162,6 @@ def add_file_to_request( metadata = self._find_metadata(request_id) return self._get_filegroups(metadata) - def get_file_approvals(self, request_id): - return [ - self._filereview(r) - for r in FileReview.objects.filter( - file__filegroup__request_id=request_id - ).all() - ] - def approve_file(self, request_id, relpath, user): with transaction.atomic(): # nb. the business logic layer approve_file() should confirm that this path @@ -177,7 +171,7 @@ def approve_file(self, request_id, relpath, user): ) review, _ = FileReview.objects.get_or_create( - file=request_file, reviewer=user + file=request_file, reviewer=user.username ) review.status = FileApprovalStatus.APPROVED review.save() @@ -189,7 +183,7 @@ def reject_file(self, request_id, relpath, user): ) review, _ = FileReview.objects.get_or_create( - file=request_file, reviewer=user + file=request_file, reviewer=user.username ) review.status = FileApprovalStatus.REJECTED review.save() diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index fcaa5b80..6c576c5b 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -563,6 +563,13 @@ def test_release_request_add_same_file(bll): assert len(release_request.filegroups["default"].files) == 1 +def _get_current_file_reviews(bll, release_request, path, author): + """Syntactic sugar to make the tests a little more readable""" + return bll.get_release_request(release_request.id, author).filegroups["default"].files[ + path + ].reviews + + def test_approve_file_not_submitted(bll): release_request, path, author = setup_empty_release_request() checker = factories.create_user("checker", [], True) @@ -572,6 +579,8 @@ def test_approve_file_not_submitted(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, checker) + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 + def test_approve_file_not_your_own(bll): release_request, path, author = setup_empty_release_request() @@ -584,6 +593,8 @@ def test_approve_file_not_your_own(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, author) + assert len(_get_current_file_reviews(bll, release_request, path, author)) == 0 + def test_approve_file_not_checker(bll): release_request, path, author = setup_empty_release_request() @@ -597,6 +608,8 @@ def test_approve_file_not_checker(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, author2) + assert len(_get_current_file_reviews(bll, release_request, path, author)) == 0 + def test_approve_file_not_part_of_request(bll): release_request, path, author = setup_empty_release_request() @@ -611,6 +624,8 @@ def test_approve_file_not_part_of_request(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, bad_path, checker) + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 + def test_approve_supporting_file(bll): release_request, path, author = setup_empty_release_request() @@ -625,6 +640,8 @@ def test_approve_supporting_file(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, checker) + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 + def test_approve_file(bll): release_request, path, author = setup_empty_release_request() @@ -635,15 +652,15 @@ def test_approve_file(bll): release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) - assert len(bll.get_file_approvals(release_request)) == 0 + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 bll.approve_file(release_request, path, checker) - assert len(bll.get_file_approvals(release_request)) == 1 - assert ( - bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED - ) - assert type(bll.get_file_approvals(release_request)[0]) == FileReview + current_reviews = _get_current_file_reviews(bll, release_request, path, checker) + assert len(current_reviews) == 1 + assert current_reviews[0].reviewer == "checker" + assert current_reviews[0].status == FileApprovalStatus.APPROVED + assert type(current_reviews[0]) == FileReview def test_reject_file(bll): @@ -655,14 +672,16 @@ def test_reject_file(bll): release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) - assert len(bll.get_file_approvals(release_request)) == 0 + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 bll.reject_file(release_request, path, checker) - assert len(bll.get_file_approvals(release_request)) == 1 - assert ( - bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED - ) + current_reviews = _get_current_file_reviews(bll, release_request, path, checker) + assert len(current_reviews) == 1 + assert current_reviews[0].reviewer == "checker" + assert current_reviews[0].status == FileApprovalStatus.REJECTED + assert type(current_reviews[0]) == FileReview + assert len(current_reviews) == 1 def test_approve_then_reject_file(bll): @@ -674,18 +693,22 @@ def test_approve_then_reject_file(bll): release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author ) - assert len(bll.get_file_approvals(release_request)) == 0 + assert len(_get_current_file_reviews(bll, release_request, path, checker)) == 0 bll.approve_file(release_request, path, checker) - assert len(bll.get_file_approvals(release_request)) == 1 - assert ( - bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.APPROVED - ) + current_reviews = _get_current_file_reviews(bll, release_request, path, checker) + print(current_reviews) + assert len(current_reviews) == 1 + assert current_reviews[0].reviewer == "checker" + assert current_reviews[0].status == FileApprovalStatus.APPROVED + assert type(current_reviews[0]) == FileReview bll.reject_file(release_request, path, checker) - assert len(bll.get_file_approvals(release_request)) == 1 - assert ( - bll.get_file_approvals(release_request)[0].status == FileApprovalStatus.REJECTED - ) + current_reviews = _get_current_file_reviews(bll, release_request, path, checker) + assert len(current_reviews) == 1 + assert current_reviews[0].reviewer == "checker" + assert current_reviews[0].status == FileApprovalStatus.REJECTED + assert type(current_reviews[0]) == FileReview + assert len(current_reviews) == 1 From 86eaf6720741af18f57d3da71581d89744f9d2ee Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Thu, 21 Mar 2024 14:58:31 +0000 Subject: [PATCH 14/15] Rename more Status to RequestStatus --- tests/integration/views/test_request.py | 24 ++++++++++++++++++------ tests/unit/test_business_logic.py | 9 ++++++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 71c89b99..023fb831 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -515,26 +515,38 @@ def test_requests_release_files_404(airlock_client, release_files_stubber): "/requests/view/request-id/default/", None, "output_checker", - Status.PENDING, + RequestStatus.PENDING, False, ), ( "/requests/view/request-id/default/file.txt", None, "output_checker", - Status.PENDING, + RequestStatus.PENDING, False, ), ( "/requests/content/request-id/default/file.txt", None, "output_checker", - Status.PENDING, + RequestStatus.PENDING, False, ), - ("/requests/submit/request-id", {}, "author", Status.PENDING, False), - ("/requests/reject/request-id", {}, "output_checker", Status.SUBMITTED, False), - ("/requests/release/request-id", {}, "output_checker", Status.SUBMITTED, True), + ("/requests/submit/request-id", {}, "author", RequestStatus.PENDING, False), + ( + "/requests/reject/request-id", + {}, + "output_checker", + RequestStatus.SUBMITTED, + False, + ), + ( + "/requests/release/request-id", + {}, + "output_checker", + RequestStatus.SUBMITTED, + True, + ), ], ) def test_request_view_tracing_with_request_attribute( diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 6c576c5b..477b26b3 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -565,9 +565,12 @@ def test_release_request_add_same_file(bll): def _get_current_file_reviews(bll, release_request, path, author): """Syntactic sugar to make the tests a little more readable""" - return bll.get_release_request(release_request.id, author).filegroups["default"].files[ - path - ].reviews + return ( + bll.get_release_request(release_request.id, author) + .filegroups["default"] + .files[path] + .reviews + ) def test_approve_file_not_submitted(bll): From 841f2a9e593ba7904306c013dab460aa34b8cff8 Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Fri, 22 Mar 2024 09:56:44 +0000 Subject: [PATCH 15/15] Rename FileApprovalStatus -> FileReviewStatus --- airlock/business_logic.py | 4 ++-- local_db/data_access.py | 6 +++--- local_db/migrations/0006_filereview.py | 2 +- local_db/models.py | 4 ++-- tests/unit/test_business_logic.py | 10 +++++----- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index d23b2b2f..1969b8cf 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -41,7 +41,7 @@ class RequestFileType(Enum): SUPPORTING = "supporting" -class FileApprovalStatus(Enum): +class FileReviewStatus(Enum): APPROVED = "APPROVED" REJECTED = "REJECTED" @@ -151,7 +151,7 @@ class FileReview: """ reviewer: str - status: FileApprovalStatus + status: FileReviewStatus created_at: datetime updated_at: datetime diff --git a/local_db/data_access.py b/local_db/data_access.py index 78467010..838441d5 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -9,9 +9,9 @@ RequestStatus, ) from local_db.models import ( - FileApprovalStatus, FileGroupMetadata, FileReview, + FileReviewStatus, RequestFileMetadata, RequestMetadata, ) @@ -173,7 +173,7 @@ def approve_file(self, request_id, relpath, user): review, _ = FileReview.objects.get_or_create( file=request_file, reviewer=user.username ) - review.status = FileApprovalStatus.APPROVED + review.status = FileReviewStatus.APPROVED review.save() def reject_file(self, request_id, relpath, user): @@ -185,5 +185,5 @@ def reject_file(self, request_id, relpath, user): review, _ = FileReview.objects.get_or_create( file=request_file, reviewer=user.username ) - review.status = FileApprovalStatus.REJECTED + review.status = FileReviewStatus.REJECTED review.save() diff --git a/local_db/migrations/0006_filereview.py b/local_db/migrations/0006_filereview.py index 8a042f29..95565691 100644 --- a/local_db/migrations/0006_filereview.py +++ b/local_db/migrations/0006_filereview.py @@ -30,7 +30,7 @@ class Migration(migrations.Migration): ( "status", local_db.models.EnumField( - default=airlock.business_logic.FileApprovalStatus["REJECTED"] + default=airlock.business_logic.FileReviewStatus["REJECTED"] ), ), ("created_at", models.DateTimeField(default=django.utils.timezone.now)), diff --git a/local_db/models.py b/local_db/models.py index 0f7599fb..bcba3c08 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.business_logic import FileApprovalStatus, RequestFileType, RequestStatus +from airlock.business_logic import FileReviewStatus, RequestFileType, RequestStatus def local_request_id(): @@ -83,7 +83,7 @@ class FileReview(models.Model): RequestFileMetadata, related_name="reviews", on_delete=models.CASCADE ) reviewer = models.TextField() - status = EnumField(default=FileApprovalStatus.REJECTED, enum=FileApprovalStatus) + status = EnumField(default=FileReviewStatus.REJECTED, enum=FileReviewStatus) created_at = models.DateTimeField(default=timezone.now) updated_at = models.DateTimeField(auto_now=True) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 477b26b3..bcafcf4d 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -8,8 +8,8 @@ import old_api from airlock.business_logic import ( BusinessLogicLayer, - FileApprovalStatus, FileReview, + FileReviewStatus, RequestFileType, RequestStatus, UrlPath, @@ -662,7 +662,7 @@ def test_approve_file(bll): current_reviews = _get_current_file_reviews(bll, release_request, path, checker) assert len(current_reviews) == 1 assert current_reviews[0].reviewer == "checker" - assert current_reviews[0].status == FileApprovalStatus.APPROVED + assert current_reviews[0].status == FileReviewStatus.APPROVED assert type(current_reviews[0]) == FileReview @@ -682,7 +682,7 @@ def test_reject_file(bll): current_reviews = _get_current_file_reviews(bll, release_request, path, checker) assert len(current_reviews) == 1 assert current_reviews[0].reviewer == "checker" - assert current_reviews[0].status == FileApprovalStatus.REJECTED + assert current_reviews[0].status == FileReviewStatus.REJECTED assert type(current_reviews[0]) == FileReview assert len(current_reviews) == 1 @@ -704,7 +704,7 @@ def test_approve_then_reject_file(bll): print(current_reviews) assert len(current_reviews) == 1 assert current_reviews[0].reviewer == "checker" - assert current_reviews[0].status == FileApprovalStatus.APPROVED + assert current_reviews[0].status == FileReviewStatus.APPROVED assert type(current_reviews[0]) == FileReview bll.reject_file(release_request, path, checker) @@ -712,6 +712,6 @@ def test_approve_then_reject_file(bll): current_reviews = _get_current_file_reviews(bll, release_request, path, checker) assert len(current_reviews) == 1 assert current_reviews[0].reviewer == "checker" - assert current_reviews[0].status == FileApprovalStatus.REJECTED + assert current_reviews[0].status == FileReviewStatus.REJECTED assert type(current_reviews[0]) == FileReview assert len(current_reviews) == 1