Implement file approval backend

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
@@ -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
 
@@ -139,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:
     """
@@ -147,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)
@@ -198,7 +223,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
@@ -280,9 +305,17 @@ def all_files_set(self):
             for request_file in filegroup.files.values()
         }
 
-    def is_supporting_file(self, relpath):
+    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(relpath).filetype == RequestFileType.SUPPORTING
+            return self.get_request_file(urlpath).filetype == RequestFileType.SUPPORTING
         except BusinessLogicLayer.FileNotFound:
             return False
 
@@ -351,6 +384,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."""
 
@@ -455,28 +491,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 +529,22 @@ 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 +556,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 +590,10 @@ 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 +620,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 +631,48 @@ 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 _verify_permission_to_review_file(
+        self, release_request: ReleaseRequest, relpath: UrlPath, user: User
+    ):
+        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(
+                "cannot approve files in your own request"
+            )
+
+        if not user.output_checker:
+            raise self.ApprovalPermissionDenied(
+                "only an output checker can approve a file"
+            )
+
+        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, relpath: UrlPath, user: User
+    ):
+        """ "Approve a file"""
+
+        self._verify_permission_to_review_file(release_request, relpath, user)
+
+        bll._dal.approve_file(release_request.id, relpath, user)
+
+    def reject_file(
+        self, release_request: ReleaseRequest, relpath: UrlPath, user: User
+    ):
+        """Reject a file"""
+
+        self._verify_permission_to_review_file(release_request, relpath, user)
+
+        bll._dal.reject_file(release_request.id, relpath, user)
 
 
 def _get_configured_bll():

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

local_db/data_access.py

@@ -6,9 +6,15 @@
     BusinessLogicLayer,
     DataAccessLayerProtocol,
     RequestFileType,
-    Status,
+    RequestStatus,
+)
+from local_db.models import (
+    FileApprovalStatus,
+    FileGroupMetadata,
+    FileReview,
+    RequestFileMetadata,
+    RequestMetadata,
 )
-from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata
 
 
 class LocalDBDataAccessLayer(DataAccessLayerProtocol):
@@ -42,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):
@@ -60,6 +70,15 @@ 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(
+            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(
@@ -76,7 +95,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 +110,12 @@ 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)
@@ -140,3 +161,29 @@ def add_file_to_request(
         # Return updated FileGroups data
         metadata = self._find_metadata(request_id)
         return self._get_filegroups(metadata)
+
+    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
+            request_file = RequestFileMetadata.objects.get(
+                filegroup__request_id=request_id, relpath=relpath
+            )
+
+            review, _ = FileReview.objects.get_or_create(
+                file=request_file, reviewer=user.username
+            )
+            review.status = FileApprovalStatus.APPROVED
+            review.save()
+
+    def reject_file(self, request_id, relpath, user):
+        with transaction.atomic():
+            request_file = RequestFileMetadata.objects.get(
+                filegroup__request_id=request_id, relpath=relpath
+            )
+
+            review, _ = FileReview.objects.get_or_create(
+                file=request_file, reviewer=user.username
+            )
+            review.status = FileApprovalStatus.REJECTED
+            review.save()

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