remove dal.get_file_approvals in favour of RequestFile.reviews

* as per PR review

airlock/business_logic.py

@@ -141,6 +141,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:
     """
@@ -149,11 +165,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)
@@ -320,29 +340,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
@@ -630,12 +627,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
     ):

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

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