diff --git a/airlock/business_logic.py b/airlock/business_logic.py index c62aa9d56..c7546a6a4 100644 --- a/airlock/business_logic.py +++ b/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 ): diff --git a/local_db/data_access.py b/local_db/data_access.py index 33ecd09a8..784670107 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 fcaa5b800..6c576c5bf 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