From 6501b97eb7e5275d420ddcea2f668b1f4bc02514 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Fri, 21 Jun 2024 11:46:44 +0100 Subject: [PATCH 01/22] Expose factories.review_file helper And use it in the existing write_request_file code --- tests/factories.py | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index f977615a..933d261a 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -13,6 +13,8 @@ AuditEventType, CodeRepo, RequestFileType, + UrlPath, + UserFileReviewStatus, Workspace, bll, ) @@ -272,21 +274,38 @@ def write_request_file( request, relpath=path, user=user, group_name=group, filetype=filetype ) if approved: - for i in range(2): + review_file(request, path, UserFileReviewStatus.APPROVED) + elif rejected: + review_file(request, path, UserFileReviewStatus.REJECTED) + + +def review_file(request, relpath, status, *users): + if users: + usernames = [user.username for user in users] + else: + usernames = ["output-checker-0", "output-checker-1"] + + for username in usernames: + if status == UserFileReviewStatus.APPROVED: bll._dal.approve_file( request, - relpath=UrlPath(path), - username=f"output-checker-{i}", - audit=create_audit_event(AuditEventType.REQUEST_FILE_APPROVE), + relpath=UrlPath(relpath), + username=username, + audit=create_audit_event( + AuditEventType.REQUEST_FILE_APPROVE, user=username + ), ) - elif rejected: - for i in range(2): + elif status == UserFileReviewStatus.REJECTED: bll._dal.reject_file( request, - relpath=UrlPath(path), - username=f"output-checker-{i}", - audit=create_audit_event(AuditEventType.REQUEST_FILE_REJECT), + relpath=UrlPath(relpath), + username=username, + audit=create_audit_event( + AuditEventType.REQUEST_FILE_REJECT, user=username + ), ) + else: + raise AssertionError(f"unrecognised status; {status}") def create_filegroup(release_request, group_name, filepaths=None): From e949db93015589e8cbd33fef1440e4bee5743cd7 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 20 Jun 2024 15:54:21 +0100 Subject: [PATCH 02/22] Refactor DAL get_requests functions Both get get_outstanding_requests_for_review and get_returned_requests were wrappers around _get_requests_by_status. So, this change just exposes that as a formal DAL method, and then uses it in the corresponding business logic methods. It also takes multiple states, which will be used in a future commit. --- airlock/business_logic.py | 14 ++++---------- local_db/data_access.py | 13 ++----------- tests/unit/test_business_logic.py | 3 +-- 3 files changed, 7 insertions(+), 23 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 4626a869..1c6290d2 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -936,13 +936,7 @@ def get_requests_for_workspace(self, workspace: str): def get_requests_authored_by_user(self, username: str): raise NotImplementedError() - def get_outstanding_requests_for_review(self): - raise NotImplementedError() - - def get_returned_requests(self): - raise NotImplementedError() - - def get_approved_requests(self): + def get_requests_by_status(self, *states: RequestStatus): raise NotImplementedError() def set_status(self, request_id: str, status: RequestStatus, audit: AuditEvent): @@ -1238,7 +1232,7 @@ def get_outstanding_requests_for_review(self, user: User): return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_outstanding_requests_for_review() + for attrs in self._dal.get_requests_by_status(RequestStatus.SUBMITTED) # Do not show output_checker their own requests if attrs["author"] != user.username ] @@ -1251,7 +1245,7 @@ def get_returned_requests(self, user: User): return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_returned_requests() + for attrs in self._dal.get_requests_by_status(RequestStatus.RETURNED) # Do not show output_checker their own requests if attrs["author"] != user.username ] @@ -1264,7 +1258,7 @@ def get_approved_requests(self, user: User): return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_approved_requests() + for attrs in self._dal.get_requests_by_status(RequestStatus.APPROVED) # Do not show output_checker their own requests if attrs["author"] != user.username ] diff --git a/local_db/data_access.py b/local_db/data_access.py index b9451111..8a3931ab 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -91,21 +91,12 @@ def get_requests_for_workspace(self, workspace: str): ) ] - def _get_requests_by_status(self, status: RequestStatus): + def get_requests_by_status(self, *states: RequestStatus): return [ metadata.to_dict() - for metadata in RequestMetadata.objects.filter(status=status) + for metadata in RequestMetadata.objects.filter(status__in=states) ] - def get_outstanding_requests_for_review(self): - return self._get_requests_by_status(status=RequestStatus.SUBMITTED) - - def get_returned_requests(self): - return self._get_requests_by_status(status=RequestStatus.RETURNED) - - def get_approved_requests(self): - return self._get_requests_by_status(status=RequestStatus.APPROVED) - def set_status(self, request_id: str, status: RequestStatus, audit: AuditEvent): with transaction.atomic(): # persist state change diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 05d7a45b..3bc716b2 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -1794,8 +1794,7 @@ def test_mark_file_undecided_permission_errors(bll, request_status, file_status) "get_requests_for_workspace", "get_active_requests_for_workspace_by_user", "get_audit_log", - "get_outstanding_requests_for_review", - "get_returned_requests", + "get_requests_by_status", "get_requests_authored_by_user", "get_approved_requests", "delete_file_from_request", From dd0bed5d7d8b5b3e716f7d63f4741f25728165ba Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 20 Jun 2024 16:31:33 +0100 Subject: [PATCH 03/22] Formalise who owns what RequestStatus For now is fairly simple, but will be useful when we expand the states shortly. --- airlock/business_logic.py | 38 ++++++++++++++++++++++++++----- tests/unit/test_business_logic.py | 6 +++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 1c6290d2..0a4eaa1e 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -51,6 +51,14 @@ class RequestStatus(Enum): RELEASED = "RELEASED" +class RequestStatusOwner(Enum): + """Who can write to a request in this state.""" + + AUTHOR = "AUTHOR" + REVIEWER = "REVIEWER" + SYSTEM = "SYSTEM" + + class RequestFileType(Enum): OUTPUT = "output" SUPPORTING = "supporting" @@ -884,12 +892,14 @@ def all_files_reviewed(self): for request_file in filegroup.output_files ) + # helpers for using in template logic + def status_owner(self) -> RequestStatusOwner: + return BusinessLogicLayer.STATUS_OWNERS[self.status] + def is_final(self): - return self.status not in [ - RequestStatus.PENDING, - RequestStatus.SUBMITTED, - RequestStatus.RETURNED, - ] + return ( + BusinessLogicLayer.STATUS_OWNERS[self.status] == RequestStatusOwner.SYSTEM + ) def store_file(release_request: ReleaseRequest, abspath: Path) -> str: @@ -1286,6 +1296,22 @@ def get_approved_requests(self, user: User): ], } + # The following lists should a) include every status and b) be disjoint + # This is validated in tests + # + STATUS_OWNERS = { + # states where only the author can edit this request + RequestStatus.PENDING: RequestStatusOwner.AUTHOR, + RequestStatus.RETURNED: RequestStatusOwner.AUTHOR, + # states where only an output-checker can edit this request + RequestStatus.SUBMITTED: RequestStatusOwner.REVIEWER, + # states where no user can edit + RequestStatus.WITHDRAWN: RequestStatusOwner.SYSTEM, + RequestStatus.APPROVED: RequestStatusOwner.SYSTEM, + RequestStatus.REJECTED: RequestStatusOwner.SYSTEM, + RequestStatus.RELEASED: RequestStatusOwner.SYSTEM, + } + STATUS_AUDIT_EVENT = { RequestStatus.PENDING: AuditEventType.REQUEST_CREATE, RequestStatus.SUBMITTED: AuditEventType.REQUEST_SUBMIT, @@ -1595,7 +1621,7 @@ def submit_request(self, request: ReleaseRequest, user: User): def _verify_permission_to_review_file( self, release_request: ReleaseRequest, relpath: UrlPath, user: User ): - if release_request.status != RequestStatus.SUBMITTED: + if self.STATUS_OWNERS[release_request.status] != RequestStatusOwner.REVIEWER: raise self.ApprovalPermissionDenied( f"cannot approve file from request in state {release_request.status.name}" ) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 3bc716b2..13aac2d1 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -764,6 +764,12 @@ def test_set_status(current, future, valid_author, valid_checker, bll): bll.set_status(release_request2, future, user=checker) +def test_request_status_ownership(bll): + """Test every RequestStatus has been assigned an ownership""" + missing_states = set(RequestStatus) - BusinessLogicLayer.STATUS_OWNERS.keys() + assert missing_states == set() + + @pytest.mark.parametrize( "current,future,user,notification_event_type", [ From 4e7c4f33a720b674dd41c3b418bdd1609e14b07d Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Thu, 20 Jun 2024 16:45:35 +0100 Subject: [PATCH 04/22] Add a completed_reviews attribute to RequestMetadata This is a dict that will track which users have completed a review for the current round. We need to know who has, so we can make sure they cannot do two reviews. The plan is to reset completed_reviews to empty if set the status to RETURNED, so when it is resubmitted, any users can do the blind review again. --- airlock/business_logic.py | 1 + .../0017_requestmetadata_completed_reviews.py | 17 +++++++++++++++++ local_db/models.py | 2 ++ 3 files changed, 20 insertions(+) create mode 100644 local_db/migrations/0017_requestmetadata_completed_reviews.py diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 0a4eaa1e..e0dac375 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -724,6 +724,7 @@ class ReleaseRequest: created_at: datetime status: RequestStatus = RequestStatus.PENDING filegroups: dict[str, FileGroup] = field(default_factory=dict) + completed_reviews: dict[str, str] = field(default_factory=dict) @classmethod def from_dict(cls, attrs) -> Self: diff --git a/local_db/migrations/0017_requestmetadata_completed_reviews.py b/local_db/migrations/0017_requestmetadata_completed_reviews.py new file mode 100644 index 00000000..afbc0871 --- /dev/null +++ b/local_db/migrations/0017_requestmetadata_completed_reviews.py @@ -0,0 +1,17 @@ +# Generated by Django 5.0.6 on 2024-06-20 13:58 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("local_db", "0016_requestfilemetadata_released_at_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="requestmetadata", + name="completed_reviews", + field=models.JSONField(default=dict), + ), + ] diff --git a/local_db/models.py b/local_db/models.py index 15d1c598..bbca8546 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -89,6 +89,7 @@ class RequestMetadata(models.Model): 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) + completed_reviews = models.JSONField(default=dict) def get_filegroups_to_dict(self): return { @@ -105,6 +106,7 @@ def to_dict(self): author=self.author, created_at=self.created_at, filegroups=self.get_filegroups_to_dict(), + completed_reviews=self.completed_reviews, ) From 75f04803af3749b0ad6d0397d6840090ead3d817 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Mon, 24 Jun 2024 13:11:56 +0100 Subject: [PATCH 05/22] WIP commit of adding the REVIEWED state and UI This was hard to split up into smaller commits, as adding and enforcing new state transitions required new UI elements. It's mostly there, but is lacking some coverage. It has been tested manually. It adds two new states: PARTIALLY_REVIEWED and REVIEWED, and new view urls and UI for users to mark their review as completed, using the above states. It adds a new db column `completed_reviews` to track which output checkers have completed a review for this round, These are reset when a request is resubmitted (i.e. a new round). Existing tests are broken in this commit - next commit is WIP fix for them - but hopefully helpful to review. --- airlock/business_logic.py | 97 ++++++++++++++++++----- airlock/file_browser_api.py | 1 + airlock/templates/file_browser/index.html | 31 +++++--- airlock/urls.py | 5 ++ airlock/views/__init__.py | 2 + airlock/views/request.py | 27 +++++++ docs/request-states.md | 9 ++- local_db/data_access.py | 13 +++ 8 files changed, 150 insertions(+), 35 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index e0dac375..ce079ee2 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -47,6 +47,8 @@ class RequestStatus(Enum): # output checker set statuses APPROVED = "APPROVED" REJECTED = "REJECTED" + PARTIALLY_REVIEWED = "PARTIALLY_REVIEWED" + REVIEWED = "REVIEWED" RETURNED = "RETURNED" RELEASED = "RELEASED" @@ -97,6 +99,7 @@ class AuditEventType(Enum): REQUEST_CREATE = "REQUEST_CREATE" REQUEST_SUBMIT = "REQUEST_SUBMIT" REQUEST_WITHDRAW = "REQUEST_WITHDRAW" + REQUEST_REVIEW = "REQUEST_REVIEW" REQUEST_APPROVE = "REQUEST_APPROVE" REQUEST_REJECT = "REQUEST_REJECT" REQUEST_RETURN = "REQUEST_RETURN" @@ -150,6 +153,7 @@ class NotificationUpdateType(Enum): AuditEventType.REQUEST_CREATE: "Created request", AuditEventType.REQUEST_SUBMIT: "Submitted request", AuditEventType.REQUEST_WITHDRAW: "Withdrew request", + AuditEventType.REQUEST_REVIEW: "Reviewed request", AuditEventType.REQUEST_APPROVE: "Approved request", AuditEventType.REQUEST_REJECT: "Rejected request", AuditEventType.REQUEST_RETURN: "Returned request", @@ -886,6 +890,12 @@ def all_files_approved(self): for request_file in filegroup.output_files ) + def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: + return all( + rfile.get_status_for_user(reviewer) is not None + for rfile in self.output_files().values() + ) + def all_files_reviewed(self): return all( request_file.reviewed() @@ -953,6 +963,12 @@ def get_requests_by_status(self, *states: RequestStatus): def set_status(self, request_id: str, status: RequestStatus, audit: AuditEvent): raise NotImplementedError() + def record_review(self, request_id: str, reviewer: str): + raise NotImplementedError() + + def reset_reviews(self, request_id: str): + raise NotImplementedError() + def add_file_to_request( self, request_id: str, @@ -1091,6 +1107,9 @@ class WorkspacePermissionDenied(APIException): class RequestPermissionDenied(APIException): pass + class RequestReviewDenied(APIException): + pass + class ApprovalPermissionDenied(APIException): pass @@ -1243,7 +1262,11 @@ def get_outstanding_requests_for_review(self, user: User): return [ ReleaseRequest.from_dict(attrs) - for attrs in self._dal.get_requests_by_status(RequestStatus.SUBMITTED) + for attrs in self._dal.get_requests_by_status( + RequestStatus.SUBMITTED, + RequestStatus.PARTIALLY_REVIEWED, + RequestStatus.REVIEWED, + ) # Do not show output_checker their own requests if attrs["author"] != user.username ] @@ -1280,10 +1303,15 @@ def get_approved_requests(self, user: User): RequestStatus.WITHDRAWN, ], RequestStatus.SUBMITTED: [ + RequestStatus.PARTIALLY_REVIEWED, + ], + RequestStatus.PARTIALLY_REVIEWED: [ + RequestStatus.REVIEWED, + ], + RequestStatus.REVIEWED: [ RequestStatus.APPROVED, RequestStatus.REJECTED, RequestStatus.RETURNED, - RequestStatus.WITHDRAWN, ], RequestStatus.RETURNED: [ RequestStatus.SUBMITTED, @@ -1306,6 +1334,8 @@ def get_approved_requests(self, user: User): RequestStatus.RETURNED: RequestStatusOwner.AUTHOR, # states where only an output-checker can edit this request RequestStatus.SUBMITTED: RequestStatusOwner.REVIEWER, + RequestStatus.PARTIALLY_REVIEWED: RequestStatusOwner.REVIEWER, + RequestStatus.REVIEWED: RequestStatusOwner.REVIEWER, # states where no user can edit RequestStatus.WITHDRAWN: RequestStatusOwner.SYSTEM, RequestStatus.APPROVED: RequestStatusOwner.SYSTEM, @@ -1316,6 +1346,8 @@ def get_approved_requests(self, user: User): STATUS_AUDIT_EVENT = { RequestStatus.PENDING: AuditEventType.REQUEST_CREATE, RequestStatus.SUBMITTED: AuditEventType.REQUEST_SUBMIT, + RequestStatus.PARTIALLY_REVIEWED: AuditEventType.REQUEST_REVIEW, + RequestStatus.REVIEWED: AuditEventType.REQUEST_REVIEW, RequestStatus.APPROVED: AuditEventType.REQUEST_APPROVE, RequestStatus.REJECTED: AuditEventType.REQUEST_REJECT, RequestStatus.RETURNED: AuditEventType.REQUEST_RETURN, @@ -1350,23 +1382,16 @@ def check_status( # check permissions # author transitions - if to_status in [ - RequestStatus.PENDING, - RequestStatus.SUBMITTED, - RequestStatus.WITHDRAWN, - ]: - if user.username != release_request.author: - raise self.RequestPermissionDenied( - f"only {release_request.author} can set status to {to_status.name}" - ) + owner = release_request.status_owner() - # output checker transitions - if to_status in [ - RequestStatus.APPROVED, - RequestStatus.REJECTED, - RequestStatus.RETURNED, - RequestStatus.RELEASED, - ]: + if ( + owner == RequestStatusOwner.AUTHOR + and user.username != release_request.author + ): + raise self.RequestPermissionDenied( + f"only the request author {release_request.author} can set status from {release_request.status} to {to_status.name}" + ) + elif owner == RequestStatusOwner.REVIEWER: if not user.output_checker: raise self.RequestPermissionDenied( f"only an output checker can set status to {to_status.name}" @@ -1400,6 +1425,7 @@ def check_status( raise self.RequestPermissionDenied( f"Cannot set status to {to_status.name}; request has unreviewed files." ) + # TODO: enforce system def set_status( self, release_request: ReleaseRequest, to_status: RequestStatus, user: User @@ -1612,7 +1638,13 @@ def submit_request(self, request: ReleaseRequest, user: User): RETURNED status, mark any rejected reviews as undecided. """ self.check_status(request, RequestStatus.SUBMITTED, user) + + # reset any previous review data if request.status == RequestStatus.RETURNED: + # reset completed review tracking + self._dal.reset_reviews(request.id) + + # any files that have not been updated are set to UNDECIDED for rfile in request.output_files().values(): for review in rfile.rejected_reviews(): self.mark_file_undecided(request, review, rfile.relpath, user) @@ -1690,6 +1722,35 @@ def reset_review_file( self._dal.reset_review_file(release_request.id, relpath, user.username, audit) + def review_request(self, release_request: ReleaseRequest, user: User): + """ + Complete a review + + Marking the request as either PARTIALLY_REVIEWED or REVIEWED, depending on whether this is the first or second review. + """ + if not release_request.all_files_reviewed_by_reviewer(user): + raise self.RequestReviewDenied( + "You must review all files to complete your review" + ) + + if user.username in release_request.completed_reviews: + raise self.RequestReviewDenied( + "You have already completed your review of this request" + ) + + self._dal.record_review(release_request.id, user.username) + + release_request = self.get_release_request(release_request.id, user) + n_reviews = len(release_request.completed_reviews) + + # this method is called twice, by different users. It advances the + # state differently depending on whether its the 1st or 2nd review to + # be completed. + if n_reviews == 1: + self.set_status(release_request, RequestStatus.PARTIALLY_REVIEWED, user) + elif n_reviews == 2: + self.set_status(release_request, RequestStatus.REVIEWED, user) + def mark_file_undecided( self, release_request: ReleaseRequest, diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 373eb9ae..48897931 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -562,6 +562,7 @@ def build_path_tree( # calls node.workspace_status = container.get_workspace_status(path) node.request_status = container.get_request_status(path) + if user: node.user_request_status = container.get_user_request_status( path, user diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index fbede4af..09ecb388 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -19,7 +19,7 @@ {% csrf_token %} {% #button type="submit" tooltip="This request is ready to be reviewed" variant="success" class="action-button btn-sm" id="submit-for-review-button" %}Submit for review{% /button %} - {% elif release_request.status.name == "SUBMITTED" %} + {% elif release_request.status_owner.name == "AUTHOR" %} {% #modal id="withdrawRequest" button_text="Withdraw this request" variant="warning" %} {% #card container=True title="Withdraw this request" %}
@@ -38,22 +38,27 @@ {% /modal %} {% endif %} {% elif is_output_checker %} - {% if release_request.status.name == "SUBMITTED" %} + {% if release_request.status_owner.name == "REVIEWER" %} {% csrf_token %} {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %}
- {% if release_request.all_files_reviewed %} -
- {% csrf_token %} - {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %} -
- {% else %} - {% #button disabled=True class="relative group btn-sm" variant="secondary" id="return-request-button" %} - Return request - {% tooltip class="airlock-tooltip" content="Returning the request is disabled until all files have been reviewed by two reviewers" %} - {% /button %} - {% endif %} + {% endif %} + {% if release_request.status.name == "REVIEWED" %} +
+ {% csrf_token %} + {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %} +
+ {% elif user_has_completed_review %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} + Complete Review + {% tooltip class="airlock-tooltip" content="You have already completed your review" %} + {% /button %} + {% else %} +
+ {% csrf_token %} + {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="return-request-button" %}Complete Review{% /button %} +
{% endif %} {% if release_request.status.name in "SUBMITTED,APPROVED" %} {% if release_request.all_files_approved %} diff --git a/airlock/urls.py b/airlock/urls.py index 53b6a5e1..01162b2e 100644 --- a/airlock/urls.py +++ b/airlock/urls.py @@ -113,6 +113,11 @@ airlock.views.request_submit, name="request_submit", ), + path( + "requests/review/", + airlock.views.request_review, + name="request_review", + ), path( "requests/reject/", airlock.views.request_reject, diff --git a/airlock/views/__init__.py b/airlock/views/__init__.py index 2e23d2dc..74d7313c 100644 --- a/airlock/views/__init__.py +++ b/airlock/views/__init__.py @@ -13,6 +13,7 @@ request_reject, request_release_files, request_return, + request_review, request_submit, request_view, request_withdraw, @@ -41,6 +42,7 @@ "request_release_files", "request_return", "request_submit", + "request_review", "request_view", "request_withdraw", "requests_for_workspace", diff --git a/airlock/views/request.py b/airlock/views/request.py index 761151f0..02713b91 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -161,6 +161,10 @@ def request_view(request, request_id: str, path: str = ""): "request_submit", kwargs={"request_id": request_id}, ) + request_review_url = reverse( + "request_review", + kwargs={"request_id": request_id}, + ) request_withdraw_url = reverse( "request_withdraw", kwargs={"request_id": request_id}, @@ -227,10 +231,13 @@ def request_view(request, request_id: str, path: str = ""): "file_reset_review_url": file_reset_review_url, "file_withdraw_url": file_withdraw_url, "request_submit_url": request_submit_url, + "request_review_url": request_review_url, "request_reject_url": request_reject_url, "request_return_url": request_return_url, "request_withdraw_url": request_withdraw_url, "release_files_url": release_files_url, + "user_has_completed_review": request.user.username + in release_request.completed_reviews, "activity": activity, "group_edit_form": group_edit_form, "group_edit_url": group_edit_url, @@ -294,6 +301,23 @@ def request_submit(request, request_id): return redirect(release_request.get_url()) +@instrument(func_attributes={"release_request": "request_id"}) +@require_http_methods(["POST"]) +def request_review(request, request_id): + release_request = get_release_request_or_raise(request.user, request_id) + + try: + bll.review_request(release_request, request.user) + except bll.RequestPermissionDenied as exc: + raise PermissionDenied(str(exc)) + except bll.RequestReviewDenied as exc: + messages.error(request, str(exc)) + else: + messages.success(request, "Your review has been completed") + + return redirect(release_request.get_url()) + + def _action_request(request, request_id, new_status): release_request = get_release_request_or_raise(request.user, request_id) @@ -444,6 +468,8 @@ def request_release_files(request, request_id): bll.release_files(release_request, request.user) except bll.RequestPermissionDenied as exc: messages.error(request, f"Error releasing files: {str(exc)}") + except bll.InvalidStateTransition as exc: + messages.error(request, f"Error releasing files: {str(exc)}") except requests.HTTPError as err: if settings.DEBUG: response_type = err.response.headers["Content-Type"] @@ -469,6 +495,7 @@ def request_release_files(request, request_id): ) else: messages.success(request, "Files have been released to jobs.opensafely.org") + if request.htmx: return HttpResponse(headers={"HX-Redirect": release_request.get_url()}) else: diff --git a/docs/request-states.md b/docs/request-states.md index b1b540d3..2ec235b8 100644 --- a/docs/request-states.md +++ b/docs/request-states.md @@ -8,10 +8,11 @@ stateDiagram-v2 [*] --> PENDING PENDING --> SUBMITTED PENDING --> WITHDRAWN - SUBMITTED --> APPROVED - SUBMITTED --> REJECTED - SUBMITTED --> RETURNED - SUBMITTED --> WITHDRAWN + SUBMITTED --> PARTIALLY_REVIEWED + PARTIALLY_REVIEWED --> REVIEWED + REVIEWED --> APPROVED + REVIEWED --> REJECTED + REVIEWED --> RETURNED RETURNED --> SUBMITTED RETURNED --> WITHDRAWN APPROVED --> RELEASED diff --git a/local_db/data_access.py b/local_db/data_access.py index 8a3931ab..9bd0ae5d 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -105,6 +105,19 @@ def set_status(self, request_id: str, status: RequestStatus, audit: AuditEvent): metadata.save() self._create_audit_log(audit) + def record_review(self, request_id: str, reviewer: str): + with transaction.atomic(): + # persist reviewer state + metadata = self._find_metadata(request_id) + metadata.completed_reviews[reviewer] = timezone.now().isoformat() + metadata.save() + + def reset_reviews(self, request_id: str): + with transaction.atomic(): + metadata = self._find_metadata(request_id) + metadata.completed_reviews = {} + metadata.save() + def add_file_to_request( self, request_id: str, From 341826bbd718e573975cdac4c9ffb536b2e9af21 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Mon, 24 Jun 2024 13:18:25 +0100 Subject: [PATCH 06/22] WIP fix of all the existing tests The introduction of a new state means a lot of test setup needs changing. This commit is midway through this leg work. --- tests/factories.py | 134 ++++++++++++++++++++++-- tests/integration/views/test_request.py | 55 +++++++--- tests/unit/test_business_logic.py | 100 ++++++++++-------- 3 files changed, 221 insertions(+), 68 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index 933d261a..47709fb5 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -13,6 +13,7 @@ AuditEventType, CodeRepo, RequestFileType, + RequestStatus, UrlPath, UserFileReviewStatus, Workspace, @@ -245,6 +246,96 @@ def create_release_request(workspace, user=None, **kwargs): return release_request +def create_request_at_state( + workspace, author, status, files=[], checker=None, withdrawn_after=None, **kwargs +): + if status == RequestStatus.WITHDRAWN: + assert ( + withdrawn_after is not None + ), "pass withdrawn_after to decide when to withdraw" + + if checker is None: + checker = get_default_output_checkers()[0] + + request = create_release_request( + workspace, author, status=RequestStatus.PENDING, **kwargs + ) + + # add all files + for add_file in files: + add_file(request) + + request = refresh_release_request(request) + if status == RequestStatus.PENDING: + return request + + if status == RequestStatus.WITHDRAWN and withdrawn_after == RequestStatus.PENDING: + bll.set_status(request, RequestStatus.WITHDRAWN, author) + return refresh_release_request(request) + + bll.submit_request(request, author) + request = refresh_release_request(request) + + if status == RequestStatus.SUBMITTED: + return request + + if status == RequestStatus.WITHDRAWN and withdrawn_after == RequestStatus.SUBMITTED: + bll.set_status(request, RequestStatus.WITHDRAWN, author) + return refresh_release_request(request) + + if status == RequestStatus.PARTIALLY_REVIEWED: + complete_independent_review(request, checker) + return refresh_release_request(request) + + # all other state require completed reviews. + complete_independent_review(request) + request = refresh_release_request(request) + + if status == RequestStatus.REVIEWED: + return request + + if status in [RequestStatus.RETURNED, RequestStatus.WITHDRAWN]: + bll.set_status(request, RequestStatus.RETURNED, checker) + request = refresh_release_request(request) + + if ( + status == RequestStatus.WITHDRAWN + and withdrawn_after == RequestStatus.RETURNED + ): + bll.set_status(request, RequestStatus.WITHDRAWN, author) + return refresh_release_request(request) + else: + return request + + elif status == RequestStatus.REJECTED: + bll.set_status(request, RequestStatus.REJECTED, checker) + return refresh_release_request(request) + + bll.set_status(request, RequestStatus.APPROVED, checker) + request = refresh_release_request(request) + + if status == RequestStatus.APPROVED: + return request + + elif status == RequestStatus.RELEASED: + bll.set_status(request, RequestStatus.RELEASED, checker) + return refresh_release_request(request) + + raise Exception(f"invalid state: {status}") + + +def request_file( + group="group", path="test/file.txt", approved=False, rejected=False, **kwargs +): + def add_file(request): + request = refresh_release_request(request) + write_request_file( + request, group, path, approved=approved, rejected=rejected, **kwargs + ) + + return add_file + + def write_request_file( request, group, @@ -279,35 +370,56 @@ def write_request_file( review_file(request, path, UserFileReviewStatus.REJECTED) +def get_default_output_checkers(): + return [ + create_user("output-checker-0", output_checker=True), + create_user("output-checker-1", output_checker=True), + ] + + def review_file(request, relpath, status, *users): - if users: - usernames = [user.username for user in users] - else: - usernames = ["output-checker-0", "output-checker-1"] + if not users: + users = get_default_output_checkers() + + request = refresh_release_request(request) - for username in usernames: + relpath = UrlPath(relpath) + for user in users: + # use the DAL directly means we can add reviews regardless of request + # state, which is very useful in test setup. if status == UserFileReviewStatus.APPROVED: bll._dal.approve_file( request, - relpath=UrlPath(relpath), - username=username, + relpath=relpath, + username=user.username, audit=create_audit_event( - AuditEventType.REQUEST_FILE_APPROVE, user=username + AuditEventType.REQUEST_FILE_APPROVE, user=user.username ), ) elif status == UserFileReviewStatus.REJECTED: bll._dal.reject_file( request, - relpath=UrlPath(relpath), - username=username, + relpath=relpath, + username=user.username, audit=create_audit_event( - AuditEventType.REQUEST_FILE_REJECT, user=username + AuditEventType.REQUEST_FILE_REJECT, user=user.username ), ) else: raise AssertionError(f"unrecognised status; {status}") +def complete_independent_review(request, *users): + if not users: + users = get_default_output_checkers() + + request = refresh_release_request(request) + + # caller's job to make sure all files have been voted on + for user in users: + bll.review_request(request, user) + + def create_filegroup(release_request, group_name, filepaths=None): user = create_user(release_request.author, [release_request.workspace]) for filepath in filepaths or []: # pragma: nocover diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 0293cee3..a044cf2f 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -548,7 +548,7 @@ def test_request_withdraw_not_author(airlock_client): def test_request_return_author(airlock_client): airlock_client.login(workspaces=["test1"]) release_request = factories.create_release_request( - "test1", user=airlock_client.user + "test1", user=airlock_client.user, status=RequestStatus.SUBMITTED ) factories.write_request_file( release_request, "group", "path/test.txt", approved=True @@ -556,13 +556,15 @@ def test_request_return_author(airlock_client): factories.write_request_file( release_request, "group", "path/test1.txt", rejected=True ) - bll.set_status(release_request, RequestStatus.SUBMITTED, airlock_client.user) + factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/return/{release_request.id}") assert response.status_code == 403 - persisted_request = bll.get_release_request(release_request.id, airlock_client.user) - assert persisted_request.status == RequestStatus.SUBMITTED + persisted_request = factories.bll.get_release_request( + release_request.id, airlock_client.user + ) + assert persisted_request.status == RequestStatus.REVIEWED def test_request_return_output_checker(airlock_client): @@ -575,7 +577,8 @@ def test_request_return_output_checker(airlock_client): factories.write_request_file( release_request, "group", "path/test1.txt", rejected=True ) - bll.set_status(release_request, RequestStatus.SUBMITTED, other_author) + factories.bll.set_status(release_request, RequestStatus.SUBMITTED, other_author) + factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/return/{release_request.id}") assert response.status_code == 302 @@ -780,7 +783,10 @@ def test_request_reject_output_checker(airlock_client): user=author, status=RequestStatus.SUBMITTED, ) - factories.write_request_file(release_request, "group", "path/test.txt") + factories.write_request_file( + release_request, "group", "path/test.txt", rejected=True + ) + factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/reject/{release_request.id}") @@ -795,13 +801,18 @@ def test_request_reject_not_output_checker(airlock_client): status=RequestStatus.SUBMITTED, ) airlock_client.login(workspaces=[release_request.workspace], output_checker=False) - factories.write_request_file(release_request, "group", "path/test.txt") + factories.write_request_file( + release_request, "group", "path/test.txt", rejected=True + ) + factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/reject/{release_request.id}") assert response.status_code == 403 - persisted_request = bll.get_release_request(release_request.id, airlock_client.user) - assert persisted_request.status == RequestStatus.SUBMITTED + persisted_request = factories.bll.get_release_request( + release_request.id, airlock_client.user + ) + assert persisted_request.status == RequestStatus.REVIEWED def test_file_withdraw_file_pending(airlock_client): @@ -909,6 +920,7 @@ def test_request_release_files_success(airlock_client, release_files_stubber, st factories.write_request_file( release_request, "group", "test/file2.txt", "test2", approved=True ) + factories.complete_independent_review(release_request) if status == RequestStatus.APPROVED: release_request = factories.refresh_release_request(release_request) @@ -942,7 +954,9 @@ def test_request_release_files_success_htmx( ) if status == RequestStatus.APPROVED: release_request = factories.refresh_release_request(release_request) - bll.set_status(release_request, status, airlock_client.user) + factories.complete_independent_review(release_request) + release_request = factories.refresh_release_request(release_request) + factories.bll.set_status(release_request, status, airlock_client.user) api_responses = release_files_stubber(release_request) response = airlock_client.post( @@ -982,6 +996,7 @@ def test_requests_release_author_403(airlock_client): factories.write_request_file( release_request, "group", "test/file1.txt", "test1", approved=True ) + factories.complete_independent_review(release_request) response = airlock_client.post("/requests/release/request_id", follow=True) assert response.status_code == 200 assert ( @@ -1000,6 +1015,7 @@ def test_requests_release_jobserver_403(airlock_client, release_files_stubber): factories.write_request_file( release_request, "group", "test/file.txt", "test", approved=True ) + factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 403 @@ -1040,6 +1056,7 @@ def test_requests_release_jobserver_403_with_debug( factories.write_request_file( release_request, "default", "test/file.txt", "test", approved=True ) + factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 403 @@ -1065,7 +1082,7 @@ def test_requests_release_unapproved_files(airlock_client): status=RequestStatus.SUBMITTED, ) factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=False + release_request, "group", "test/file1.txt", "test1", approved=True ) response = airlock_client.post("/requests/release/request_id", follow=True) @@ -1085,6 +1102,7 @@ def test_requests_release_files_404(airlock_client, release_files_stubber): factories.write_request_file( release_request, "group", "test/file.txt", "test", approved=True ) + factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 404 @@ -1128,14 +1146,14 @@ def test_requests_release_files_404(airlock_client, release_files_stubber): "/requests/reject/request-id", {}, "output_checker", - RequestStatus.SUBMITTED, + RequestStatus.REVIEWED, False, ), ( "/requests/release/request-id", {}, "output_checker", - RequestStatus.SUBMITTED, + RequestStatus.REVIEWED, True, ), ], @@ -1146,16 +1164,23 @@ def test_request_view_tracing_with_request_attribute( author = factories.create_user("author", ["test-workspace"]) factories.create_user("output_checker", output_checker=True) airlock_client.login(username=login_as, output_checker=True) + + initial_status = ( + RequestStatus.SUBMITTED if status == RequestStatus.REVIEWED else status + ) release_request = factories.create_release_request( - "test-workspace", id="request-id", user=author, status=status + "test-workspace", id="request-id", user=author, status=initial_status ) + factories.write_request_file( release_request, "default", "file.txt", contents="test", - approved="/requests/release/" in urlpath, + approved=status == RequestStatus.REVIEWED, ) + if status == RequestStatus.REVIEWED: + factories.complete_independent_review(release_request) if stub: release_files_stubber(release_request) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 13aac2d1..b3f72785 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -343,6 +343,7 @@ def test_provider_request_release_files_invalid_file_type(bll, mock_notification factories.write_request_file( release_request, "group", relpath, "test", approved=True ) + factories.complete_independent_review(release_request) release_request = factories.refresh_release_request(release_request) bll.set_status(release_request, RequestStatus.APPROVED, checker) @@ -374,7 +375,9 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f "test", filetype=RequestFileType.SUPPORTING, ) - bll.set_status(release_request, RequestStatus.APPROVED, checker) + factories.complete_independent_review(release_request) + release_request = factories.refresh_release_request(release_request) + factories.bll.set_status(release_request, RequestStatus.APPROVED, checker) abspath = release_request.abspath("group" / relpath) @@ -425,22 +428,22 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f audit_log = bll.get_audit_log(request=release_request.id) - assert audit_log[3].type == AuditEventType.REQUEST_APPROVE - assert audit_log[3].user == checker.username - assert audit_log[3].request == release_request.id - assert audit_log[3].workspace == "workspace" - - assert audit_log[4].type == AuditEventType.REQUEST_FILE_RELEASE - assert audit_log[4].user == checker.username - assert audit_log[4].request == release_request.id - assert audit_log[4].workspace == "workspace" - assert audit_log[4].path == Path("test/file.txt") - - assert audit_log[5].type == AuditEventType.REQUEST_RELEASE + assert audit_log[5].type == AuditEventType.REQUEST_APPROVE assert audit_log[5].user == checker.username assert audit_log[5].request == release_request.id assert audit_log[5].workspace == "workspace" + assert audit_log[6].type == AuditEventType.REQUEST_FILE_RELEASE + assert audit_log[6].user == checker.username + assert audit_log[6].request == release_request.id + assert audit_log[6].workspace == "workspace" + assert audit_log[6].path == Path("test/file.txt") + + assert audit_log[7].type == AuditEventType.REQUEST_RELEASE + assert audit_log[7].user == checker.username + assert audit_log[7].request == release_request.id + assert audit_log[7].workspace == "workspace" + def test_provider_get_requests_for_workspace(bll): user = factories.create_user("test", ["workspace", "workspace2"]) @@ -544,14 +547,20 @@ def test_provider_get_returned_requests(output_checker, expected, bll): release_request1 = factories.create_release_request( "workspace", other_user, id="r1", status=RequestStatus.SUBMITTED ) - bll.set_status(release_request1, RequestStatus.RETURNED, output_checker) + factories.write_request_file(release_request1, "group", "file.txt", approved=True) + factories.complete_independent_review(release_request1) + release_request1 = factories.refresh_release_request(release_request1) + factories.bll.set_status(release_request1, RequestStatus.RETURNED, output_checker) # requests not visible to output checker # status returned, but authored by output checker release_request2 = factories.create_release_request( "workspace", user, id="r2", status=RequestStatus.SUBMITTED ) - bll.set_status(release_request2, RequestStatus.RETURNED, output_checker) + factories.write_request_file(release_request2, "group", "file.txt", approved=True) + factories.complete_independent_review(release_request2) + release_request2 = factories.refresh_release_request(release_request2) + factories.bll.set_status(release_request2, RequestStatus.RETURNED, output_checker) # requests authored by other users, status other than returned for i, status in enumerate( @@ -684,15 +693,33 @@ def test_provider_get_current_request_for_user_output_checker(bll): [ (RequestStatus.PENDING, RequestStatus.SUBMITTED, True, False), (RequestStatus.PENDING, RequestStatus.WITHDRAWN, True, False), + (RequestStatus.PENDING, RequestStatus.PARTIALLY_REVIEWED, False, False), + (RequestStatus.PENDING, RequestStatus.REVIEWED, False, 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.RETURNED, False, True), - (RequestStatus.SUBMITTED, RequestStatus.RELEASED, False, False), (RequestStatus.SUBMITTED, RequestStatus.PENDING, False, False), + (RequestStatus.SUBMITTED, RequestStatus.PARTIALLY_REVIEWED, False, True), + (RequestStatus.SUBMITTED, RequestStatus.REVIEWED, False, False), + (RequestStatus.SUBMITTED, RequestStatus.APPROVED, False, False), + (RequestStatus.SUBMITTED, RequestStatus.REJECTED, False, False), + (RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN, False, False), + (RequestStatus.SUBMITTED, RequestStatus.RETURNED, False, False), + (RequestStatus.SUBMITTED, RequestStatus.RELEASED, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.PENDING, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.SUBMITTED, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REVIEWED, False, True), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.APPROVED, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REJECTED, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.RELEASED, False, False), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.WITHDRAWN, False, False), + (RequestStatus.REVIEWED, RequestStatus.PENDING, False, False), + (RequestStatus.REVIEWED, RequestStatus.SUBMITTED, False, False), + (RequestStatus.REVIEWED, RequestStatus.PARTIALLY_REVIEWED, False, False), + (RequestStatus.REVIEWED, RequestStatus.RETURNED, False, True), + (RequestStatus.REVIEWED, RequestStatus.APPROVED, False, True), + (RequestStatus.REVIEWED, RequestStatus.REJECTED, False, True), + (RequestStatus.REVIEWED, RequestStatus.WITHDRAWN, False, False), (RequestStatus.RETURNED, RequestStatus.SUBMITTED, True, False), (RequestStatus.RETURNED, RequestStatus.WITHDRAWN, True, False), (RequestStatus.APPROVED, RequestStatus.RELEASED, False, True), @@ -714,11 +741,18 @@ def test_set_status(current, future, valid_author, valid_checker, bll): author = factories.create_user("author", ["workspace"], False) checker = factories.create_user("checker", [], True) audit_type = bll.STATUS_AUDIT_EVENT[future] - release_request1 = factories.create_release_request( - "workspace1", user=author, status=current + + release_request1 = factories.create_request_at_state( + "workspace1", + author, + current, + files=[factories.request_file(approved=True)], ) - release_request2 = factories.create_release_request( - "workspace2", user=author, status=current + release_request2 = factories.create_request_at_state( + "workspace2", + author, + current, + files=[factories.request_file(approved=True)], ) if valid_author: @@ -734,26 +768,8 @@ def test_set_status(current, future, valid_author, valid_checker, bll): bll.set_status(release_request1, future, user=author) if valid_checker: - if current == RequestStatus.SUBMITTED: - factories.write_request_file( - release_request2, "group", "test/file.txt", approved=True - ) - release_request2 = factories.refresh_release_request(release_request2) - - if current == RequestStatus.REJECTED: - # We cannot add files to a rejected request, so re-create the request - release_request2 = factories.create_release_request( - "workspace2", user=author, status=RequestStatus.SUBMITTED - ) - factories.write_request_file( - release_request2, "group", "test/file.txt", approved=True - ) - bll.set_status(release_request2, current, user=checker) - release_request2 = factories.refresh_release_request(release_request2) - bll.set_status(release_request2, future, user=checker) assert release_request2.status == future - audit_log = bll.get_audit_log(request=release_request2.id) assert audit_log[0].type == audit_type assert audit_log[0].user == checker.username From d57c7678b78086db5f20b60d3c841b1e48e10c6f Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 09:17:22 +0100 Subject: [PATCH 07/22] Add notification events for new statuses --- airlock/business_logic.py | 4 ++++ tests/unit/test_business_logic.py | 12 ++++++++---- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index ce079ee2..196dafd7 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -123,6 +123,8 @@ class AuditEventType(Enum): class NotificationEventType(Enum): REQUEST_SUBMITTED = "request_submitted" REQUEST_WITHDRAWN = "request_withdrawn" + REQUEST_PARTIALLY_REVIEWED = "request_partially_reviewed" + REQUEST_REVIEWED = "request_reviewed" REQUEST_APPROVED = "request_approved" REQUEST_RELEASED = "request_released" REQUEST_REJECTED = "request_rejected" @@ -1357,6 +1359,8 @@ def get_approved_requests(self, user: User): STATUS_EVENT_NOTIFICATION = { RequestStatus.SUBMITTED: NotificationEventType.REQUEST_SUBMITTED, + RequestStatus.PARTIALLY_REVIEWED: NotificationEventType.REQUEST_PARTIALLY_REVIEWED, + RequestStatus.REVIEWED: NotificationEventType.REQUEST_REVIEWED, RequestStatus.APPROVED: NotificationEventType.REQUEST_APPROVED, RequestStatus.REJECTED: NotificationEventType.REQUEST_REJECTED, RequestStatus.RETURNED: NotificationEventType.REQUEST_RETURNED, diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index b3f72785..91991a53 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -417,14 +417,18 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f notification_responses = parse_notification_responses(mock_notifications) # Notifications expected for: # - write file x2 - # - set status to test_set_status_approved + # - set status to partially reviewed + # - set status to reviewed + # - set status to approved # - set status to released - assert notification_responses["count"] == 4 + assert notification_responses["count"] == 6 request_json = notification_responses["request_json"] assert request_json[0]["event_type"] == "request_updated" assert request_json[1]["event_type"] == "request_updated" - assert request_json[2]["event_type"] == "request_approved" - assert request_json[3]["event_type"] == "request_released" + assert request_json[2]["event_type"] == "request_partially_reviewed" + assert request_json[3]["event_type"] == "request_reviewed" + assert request_json[4]["event_type"] == "request_approved" + assert request_json[5]["event_type"] == "request_released" audit_log = bll.get_audit_log(request=release_request.id) From 5d41a3eedb352ada1c3239a42834aae69ed0a7e6 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 10:09:52 +0100 Subject: [PATCH 08/22] Check status for system-owned statuses --- airlock/business_logic.py | 16 ++++++++++++---- tests/unit/test_business_logic.py | 2 ++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 196dafd7..ff7171e4 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -1385,9 +1385,8 @@ def check_status( ) # check permissions - # author transitions owner = release_request.status_owner() - + # author transitions if ( owner == RequestStatusOwner.AUTHOR and user.username != release_request.author @@ -1395,7 +1394,13 @@ def check_status( raise self.RequestPermissionDenied( f"only the request author {release_request.author} can set status from {release_request.status} to {to_status.name}" ) - elif owner == RequestStatusOwner.REVIEWER: + # reviewer transitions + elif owner == RequestStatusOwner.REVIEWER or ( + # APPROVED and REJECTED cannot be edited by any user, but can be + # moved to valid state transitions by a reviewer + owner == RequestStatusOwner.SYSTEM + and release_request.status in [RequestStatus.APPROVED, RequestStatus.REJECTED] + ): if not user.output_checker: raise self.RequestPermissionDenied( f"only an output checker can set status to {to_status.name}" @@ -1429,7 +1434,10 @@ def check_status( raise self.RequestPermissionDenied( f"Cannot set status to {to_status.name}; request has unreviewed files." ) - # TODO: enforce system + elif owner == RequestStatusOwner.SYSTEM: + raise self.RequestPermissionDenied( + f"only the system can set status to {to_status.name}" + ) def set_status( self, release_request: ReleaseRequest, to_status: RequestStatus, user: User diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 91991a53..2b614294 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -694,6 +694,8 @@ def test_provider_get_current_request_for_user_output_checker(bll): @pytest.mark.parametrize( "current,future,valid_author,valid_checker", + # valid_author: author can set status of their own request + # valid_checker: checker can set status of another author's request [ (RequestStatus.PENDING, RequestStatus.SUBMITTED, True, False), (RequestStatus.PENDING, RequestStatus.WITHDRAWN, True, False), From 44d87bce0b8e9a049c6ad517d958fb722c8a4367 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 12:54:44 +0100 Subject: [PATCH 09/22] Add an audit event for resetting reviews --- airlock/business_logic.py | 19 ++++++++++++++----- local_db/data_access.py | 3 ++- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index ff7171e4..7ea1aa28 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -104,6 +104,7 @@ class AuditEventType(Enum): REQUEST_REJECT = "REQUEST_REJECT" REQUEST_RETURN = "REQUEST_RETURN" REQUEST_RELEASE = "REQUEST_RELEASE" + REQUEST_REVIEW_RESET = "REQUEST_REVIEW_RESET" # request edits REQUEST_EDIT = "REQUEST_EDIT" @@ -145,6 +146,7 @@ class NotificationUpdateType(Enum): AuditEventType.WORKSPACE_FILE_VIEW, AuditEventType.REQUEST_FILE_VIEW, AuditEventType.REQUEST_FILE_UNDECIDED, + AuditEventType.REQUEST_REVIEW_RESET, } @@ -160,6 +162,7 @@ class NotificationUpdateType(Enum): AuditEventType.REQUEST_REJECT: "Rejected request", AuditEventType.REQUEST_RETURN: "Returned request", AuditEventType.REQUEST_RELEASE: "Released request", + AuditEventType.REQUEST_REVIEW_RESET: "Reviews on request reset", AuditEventType.REQUEST_EDIT: "Edited the Context/Controls", AuditEventType.REQUEST_COMMENT: "Commented", AuditEventType.REQUEST_COMMENT_DELETE: "Comment deleted", @@ -968,7 +971,7 @@ def set_status(self, request_id: str, status: RequestStatus, audit: AuditEvent): def record_review(self, request_id: str, reviewer: str): raise NotImplementedError() - def reset_reviews(self, request_id: str): + def reset_reviews(self, request_id: str, audit: AuditEvent): raise NotImplementedError() def add_file_to_request( @@ -1396,10 +1399,11 @@ def check_status( ) # reviewer transitions elif owner == RequestStatusOwner.REVIEWER or ( - # APPROVED and REJECTED cannot be edited by any user, but can be + # APPROVED and REJECTED cannot be edited by any user, but can be # moved to valid state transitions by a reviewer - owner == RequestStatusOwner.SYSTEM - and release_request.status in [RequestStatus.APPROVED, RequestStatus.REJECTED] + owner == RequestStatusOwner.SYSTEM + and release_request.status + in [RequestStatus.APPROVED, RequestStatus.REJECTED] ): if not user.output_checker: raise self.RequestPermissionDenied( @@ -1653,8 +1657,13 @@ def submit_request(self, request: ReleaseRequest, user: User): # reset any previous review data if request.status == RequestStatus.RETURNED: + audit = AuditEvent( + AuditEventType.REQUEST_REVIEW_RESET, + user=user.username, + request=request.id, + ) # reset completed review tracking - self._dal.reset_reviews(request.id) + self._dal.reset_reviews(request.id, audit) # any files that have not been updated are set to UNDECIDED for rfile in request.output_files().values(): diff --git a/local_db/data_access.py b/local_db/data_access.py index 9bd0ae5d..7e9c6db6 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -112,11 +112,12 @@ def record_review(self, request_id: str, reviewer: str): metadata.completed_reviews[reviewer] = timezone.now().isoformat() metadata.save() - def reset_reviews(self, request_id: str): + def reset_reviews(self, request_id: str, audit: AuditEvent): with transaction.atomic(): metadata = self._find_metadata(request_id) metadata.completed_reviews = {} metadata.save() + self._create_audit_log(audit) def add_file_to_request( self, From 875b8362f1c348ebec89d9b7d586eb9785d724e3 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 14:56:11 +0100 Subject: [PATCH 10/22] Fix logic for displaying review buttons and functional tests --- airlock/templates/file_browser/index.html | 58 ++++++++++-------- tests/functional/test_e2e.py | 50 +++++++-------- tests/functional/test_request_pages.py | 74 +++++++++-------------- tests/integration/views/test_request.py | 49 ++++++++++++++- 4 files changed, 133 insertions(+), 98 deletions(-) diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index 09ecb388..3466679b 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -14,12 +14,11 @@ {% if context == "request" %} {% #airlock_header context=context release_request=release_request title=title workspace=workspace %} {% if is_author %} - {% if release_request.status.name in "PENDING,RETURNED" %} + {% if release_request.status_owner.name == "AUTHOR" %}
{% csrf_token %} {% #button type="submit" tooltip="This request is ready to be reviewed" variant="success" class="action-button btn-sm" id="submit-for-review-button" %}Submit for review{% /button %}
- {% elif release_request.status_owner.name == "AUTHOR" %} {% #modal id="withdrawRequest" button_text="Withdraw this request" variant="warning" %} {% #card container=True title="Withdraw this request" %}
@@ -38,44 +37,55 @@ {% /modal %} {% endif %} {% elif is_output_checker %} - {% if release_request.status_owner.name == "REVIEWER" %} + {% comment %} A fully reviewed request can be returned or rejected {% endcomment %} + {% if release_request.status.name == "REVIEWED" %} {% csrf_token %} {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %}
- {% endif %} - {% if release_request.status.name == "REVIEWED" %}
{% csrf_token %} {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %}
- {% elif user_has_completed_review %} + {% else %} + {% #button disabled=True class="relative group btn-sm" variant="danger" id="reject-request-button" %} + Reject request + {% tooltip class="airlock-tooltip" content="Rejecting a request is disabled until review has been completed by two reviewers" %} + {% /button %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="return-request-button" %} + Return request + {% tooltip class="airlock-tooltip" content="Returning a request is disabled until review has been completed by two reviewers" %} + {% /button %} + {% endif %} + + {% comment %} A fully reviewed or approved request can be released if all its files are also approved {% endcomment %} + {% if release_request.status.name in "REVIEWED,APPROVED" and release_request.all_files_approved %} +
+ {% csrf_token %} + {% #button class="btn-sm" type="submit" tooltip="Release files to jobs.opensafely.org" variant="warning" id="release-files-button" %} + Release files + + {% /button %} +
+ {% else %} + {% #button disabled=True class="relative group btn-sm" variant="warning" id="release-files-button" %} + Release files + {% tooltip class="airlock-tooltip" content="Releasing to jobs.opensafely.org is disabled until review has been completed by by two reviewers" %} + {% /button %} + {% endif %} + + {% comment %} User can complete review if they haven't already {% endcomment %} + {% if user_has_completed_review %} {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} - Complete Review + Complete review {% tooltip class="airlock-tooltip" content="You have already completed your review" %} {% /button %} {% else %}
{% csrf_token %} - {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="return-request-button" %}Complete Review{% /button %} + {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="complete-review-button" %}Complete review{% /button %}
{% endif %} - {% if release_request.status.name in "SUBMITTED,APPROVED" %} - {% if release_request.all_files_approved %} -
- {% csrf_token %} - {% #button class="btn-sm" type="submit" tooltip="Release files to jobs.opensafely.org" variant="warning" id="release-files-button" %} - Release Files - - {% /button %} -
- {% else %} - {% #button disabled=True class="relative group btn-sm" variant="warning" id="release-files-button" %} - Release Files - {% tooltip class="airlock-tooltip" content="Releasing to jobs.opensafely.org is disabled until all files have been approved by two reviewers" %} - {% /button %} - {% endif %} - {% endif %} {% endif %} {% /airlock_header %} {% else %} diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index b3741121..3b7629f2 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -1,6 +1,5 @@ import json import re -import time import pytest from playwright.sync_api import expect @@ -121,10 +120,12 @@ def test_e2e_release_files( - View supporting file - Reject output file - Approve output file + - Complete review - Logout 3) Log in as second output checker - Approve output file + - Complete review - Release files - View requests list again and confirm released request is not shown """ @@ -397,6 +398,8 @@ def test_e2e_release_files( expect(page.locator("#file-reject-button")).not_to_be_visible() expect(page.locator("#file-reset-button")).not_to_be_visible() + # complete review for this output-checker + find_and_click(page.locator("#complete-review-button")) # Logout (by clearing cookies) and log in as second output-checker to do second approval # and release context.clear_cookies() @@ -404,7 +407,13 @@ def test_e2e_release_files( # Approve the file page.goto(live_server.url + release_request.get_url("my-new-group/subdir/file.txt")) find_and_click(page.locator("#file-approve-button")) - # Now the file has 2 approvals, the release files button is enabled + + # The file has 2 approvals, but the release files button is not yet enabled until this + # reviewer completes their review + expect(release_button).to_be_disabled() + + # complete review + find_and_click(page.locator("#complete-review-button")) expect(release_button).to_be_enabled() # Mock the responses from job-server @@ -426,24 +435,19 @@ def test_e2e_reject_request(page, live_server, dev_users): """ Test output-checker rejects a release request """ - # set up a submitted file - factories.write_workspace_file("test-workspace", "file.txt") - release_request = factories.create_release_request( + # set up a reviewed request + release_request = factories.create_request_at_state( "test-workspace", - status=RequestStatus.SUBMITTED, - ) - factories.create_filegroup( - release_request, group_name="default", filepaths=["file.txt"] + author=factories.create_user("author", workspaces=["test-workspace"]), + status=RequestStatus.REVIEWED, + files=[factories.request_file(rejected=True)], ) # Log in as output checker login_as(live_server, page, "output_checker") - # View requests - find_and_click(page.get_by_test_id("nav-requests")) - # View submitted request - find_and_click(page.get_by_role("link", name="test-workspace by author")) + page.goto(live_server.url + release_request.get_url()) # Reject request find_and_click(page.locator("#reject-request-button")) @@ -459,27 +463,19 @@ def test_e2e_withdraw_request(page, live_server, dev_users): Request author withdraws their request """ # set up a submitted request - factories.write_workspace_file("test-workspace", "file.txt") user = factories.create_user("researcher", ["test-workspace"], False) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "test-workspace", - user, - status=RequestStatus.SUBMITTED, - ) - factories.create_filegroup( - release_request, group_name="default", filepaths=["file.txt"] + author=user, + status=RequestStatus.RETURNED, + files=[factories.request_file(rejected=True)], ) # Log in as a researcher login_as(live_server, page, user.username) - # View requests - find_and_click(page.get_by_test_id("nav-requests")) - - page.get_by_role("link", name="test-workspace: SUBMITTED").click() - - # give the js time to set up the dialog - time.sleep(0.1) + # View submitted request + page.goto(live_server.url + release_request.get_url()) find_and_click(page.locator("[data-modal=withdrawRequest]")) find_and_click(page.locator("#withdraw-request-confirm")) diff --git a/tests/functional/test_request_pages.py b/tests/functional/test_request_pages.py index 422ebf29..51cb50ff 100644 --- a/tests/functional/test_request_pages.py +++ b/tests/functional/test_request_pages.py @@ -18,21 +18,14 @@ def test_request_file_withdraw(live_server, context, page, bll): }, ) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=author, - ) - factories.write_request_file( - release_request, - "group", - "file1.txt", - "file 1 content", - ) - factories.write_request_file( - release_request, - "group", - "file2.txt", - "file 2 content", + author=author, + status=RequestStatus.PENDING, + files=[ + factories.request_file(group="group", path="file1.txt"), + factories.request_file(group="group", path="file2.txt"), + ], ) page.goto(live_server.url + release_request.get_url("group/file1.txt")) @@ -70,15 +63,11 @@ def test_request_group_edit_comment(live_server, context, page, bll, settings): }, ) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=author, - ) - factories.write_request_file( - release_request, - "group", - "file1.txt", - "file 1 content", + author=author, + files=[factories.request_file(group="group")], + status=RequestStatus.SUBMITTED, ) page.goto(live_server.url + release_request.get_url("group")) @@ -123,23 +112,15 @@ def test_request_return(live_server, context, page, bll): }, ) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=author, - ) - factories.write_request_file( - release_request, - "group", - "file1.txt", - "file 1 content", - ) - factories.write_request_file( - release_request, - "group", - "file2.txt", - "file 2 content", + author=author, + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(group="group", path="file1.txt"), + factories.request_file(group="group", path="file2.txt"), + ], ) - bll.set_status(release_request, RequestStatus.SUBMITTED, author) return_request_button = page.locator("#return-request-button") page.goto(live_server.url + release_request.get_url()) @@ -169,6 +150,9 @@ def _review_files(username): page.goto(live_server.url + release_request.get_url("group/file2.txt")) page.locator("#file-reject-button").click() + # mark review as completed + page.locator("#complete-review-button").click() + # First output-checker reviews files _review_files("output-checker-1") @@ -222,13 +206,13 @@ def _review_files(username): def test_request_releaseable(live_server, context, page, bll): - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED - ) - factories.write_request_file( - release_request, "group", "file1.txt", "file 1 content", approved=True + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.REVIEWED, + files=[ + factories.request_file(group="group", path="file1.txt", approved=True), + ], ) - release_request = factories.refresh_release_request(release_request) output_checker = login_as_user( live_server, context, @@ -245,7 +229,7 @@ def test_request_releaseable(live_server, context, page, bll): return_request_button = page.locator("#reject-request-button") reject_request_button = page.locator("#return-request-button") - # Request is currently submitted and all files approved twice + # Request is currently reviewed twice # output checker can release, return or reject for locator in [release_files_button, return_request_button, reject_request_button]: expect(locator).to_be_visible() @@ -258,4 +242,4 @@ def test_request_releaseable(live_server, context, page, bll): # output checker cannot return or reject expect(release_files_button).to_be_enabled() for locator in [return_request_button, reject_request_button]: - expect(locator).not_to_be_visible() + expect(locator).not_to_be_enabled() diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index a044cf2f..904bdac7 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -146,8 +146,53 @@ def test_request_view_with_submitted_request(airlock_client): "workspace", status=RequestStatus.SUBMITTED ) response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) - assert "Reject request" in response.rendered_content - assert "Release files" in response.rendered_content + assert "Rejecting a request is disabled" in response.rendered_content + assert "Releasing to jobs.opensafely.org is disabled" in response.rendered_content + assert "Returning a request is disabled" in response.rendered_content + assert "Complete review" in response.rendered_content + + +def test_request_view_with_reviewed_request(airlock_client): + # Login as 1st default output-checker + airlock_client.login("output-checker-0", output_checker=True) + release_request = factories.create_request_at_state( + "workspace", status=RequestStatus.REVIEWED + ) + response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) + + expected_buttons = [ + ("Reject request", "Rejecting a request is disabled"), + ("Release files", "Releasing to jobs.opensafely.org is disabled"), + ("Return request", "Returning a request is disabled"), + ] + + for button_text, diabled_tooltip in expected_buttons: + assert button_text in response.rendered_content + assert diabled_tooltip not in response.rendered_content + + assert "Complete review" in response.rendered_content + assert "You have already completed your review" in response.rendered_content + + +def test_request_view_with_withdrawn_request(airlock_client): + # Login as 1st default output-checker + airlock_client.login("output-checker-0", output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.WITHDRAWN, + withdrawn_after=RequestStatus.RETURNED, + ) + response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) + + expected_buttons = [ + ("Reject request", "Rejecting a request is disabled"), + ("Release files", "Releasing to jobs.opensafely.org is disabled"), + ("Return request", "Returning a request is disabled"), + ] + + for button_text, diabled_tooltip in expected_buttons: + assert button_text in response.rendered_content + assert diabled_tooltip in response.rendered_content def test_request_view_with_authored_request_file(airlock_client): From 3c2330f4d15e10358a1ddcda1a3262409d3fc116 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Wed, 26 Jun 2024 11:37:53 +0100 Subject: [PATCH 11/22] Extend create_request_at_state to review files by specified output-checkers --- tests/factories.py | 83 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 16 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index 47709fb5..cc3800c1 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -247,15 +247,58 @@ def create_release_request(workspace, user=None, **kwargs): def create_request_at_state( - workspace, author, status, files=[], checker=None, withdrawn_after=None, **kwargs + workspace, + status, + author=None, + files=[], + checker=None, + withdrawn_after=None, + **kwargs, ): + """ + Create a valid request at the given status. + + Files must be provided for any status that needs them, in the appropriate reviewed + state (e.g. an approved/released status requires at least one fully approved output file). + + Files are provided using a factory method for creating them. e.g. + + create_request_at_state( + "workspace", + RequestStatus.RELEASED, + files=[ + request_file(approved=True), + request_file(approved=True, filetype=RequestFileType.SUPPORTING), + ] + ) + + `checker` is the user who will do any status changes or actions that require an + output-checker. If not provided, a default output checker is used. + + Optionally, `request_file` can be given `checkers`, a list of users who will + review (approve/reject) the file. If not provided, default output checkers + will be used. + """ + author = author or create_user( + "author", + workspaces=[workspace if isinstance(workspace, str) else workspace.name], + ) if status == RequestStatus.WITHDRAWN: assert ( withdrawn_after is not None ), "pass withdrawn_after to decide when to withdraw" - if checker is None: - checker = get_default_output_checkers()[0] + # Get a default checker if one was not provided + # This is the checker who does the state transitions (approved/released/returned/rejected) + # It is not necessarily the same checker who reviews files. + # `request_file()` can be called with an optional list of checkers; if it is + # None, the same default checkers will be used for reviewing files (and the first one will + # be the one that does the other state transitions.) + if checker: + file_reviewers = [checker, get_default_output_checkers()[1]] + else: + file_reviewers = get_default_output_checkers() + checker = file_reviewers[0] request = create_release_request( workspace, author, status=RequestStatus.PENDING, **kwargs @@ -279,16 +322,22 @@ def create_request_at_state( if status == RequestStatus.SUBMITTED: return request - if status == RequestStatus.WITHDRAWN and withdrawn_after == RequestStatus.SUBMITTED: - bll.set_status(request, RequestStatus.WITHDRAWN, author) - return refresh_release_request(request) + # If there are output files, get the usernames of all file reviewers + # so we can complete reviews with the correct checkers. + # Note that it is possible to review a request with no output files + # (potentially before returning it to the reviewer so they can add some). + # Approving or releasing requests with no output files is not allowed. + if request.output_files(): + file_reviewers = [ + User(username, output_checker=True) + for username in list(request.output_files().values())[0].reviews.keys() + ] if status == RequestStatus.PARTIALLY_REVIEWED: - complete_independent_review(request, checker) + complete_independent_review(request, file_reviewers[0]) return refresh_release_request(request) - # all other state require completed reviews. - complete_independent_review(request) + complete_independent_review(request, *file_reviewers) request = refresh_release_request(request) if status == RequestStatus.REVIEWED: @@ -321,7 +370,7 @@ def create_request_at_state( bll.set_status(request, RequestStatus.RELEASED, checker) return refresh_release_request(request) - raise Exception(f"invalid state: {status}") + raise Exception(f"invalid state: {status}") # pragma: no cover def request_file( @@ -346,6 +395,7 @@ def write_request_file( approved=False, workspace=None, rejected=False, + checkers=None, ): # if ensure_workspace is passed a string, it will always create a # new workspace. Optionally pass a workspace instance, which will @@ -361,13 +411,15 @@ def write_request_file( if user is None: # pragma: nocover user = create_user(request.author, workspaces=[workspace.name]) + checkers = checkers or get_default_output_checkers() + bll.add_file_to_request( request, relpath=path, user=user, group_name=group, filetype=filetype ) if approved: - review_file(request, path, UserFileReviewStatus.APPROVED) + review_file(request, path, UserFileReviewStatus.APPROVED, *checkers) elif rejected: - review_file(request, path, UserFileReviewStatus.REJECTED) + review_file(request, path, UserFileReviewStatus.REJECTED, *checkers) def get_default_output_checkers(): @@ -378,7 +430,7 @@ def get_default_output_checkers(): def review_file(request, relpath, status, *users): - if not users: + if not users: # pragma: no cover users = get_default_output_checkers() request = refresh_release_request(request) @@ -406,12 +458,11 @@ def review_file(request, relpath, status, *users): ), ) else: - raise AssertionError(f"unrecognised status; {status}") + raise AssertionError(f"unrecognised status; {status}") # pragma: no cover def complete_independent_review(request, *users): - if not users: - users = get_default_output_checkers() + users = users or get_default_output_checkers() request = refresh_release_request(request) From 0c4d3b155fdbf836307cea813ca9d43537963e7d Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Wed, 26 Jun 2024 11:38:19 +0100 Subject: [PATCH 12/22] Fix up tests, cont. Update many tests that used create_release_request to use create_request_at_state instead. The ones that are still left using create_release_request should only be ones created as pending, which don't do any more state-setting in the test itself. This is asserted in create_release_request, to ensure that any future tests always use create_request_at_state and have a correctly set up request for its state. Also adds some tests for coverage. --- tests/factories.py | 4 +- tests/functional/test_code_pages.py | 4 +- tests/integration/views/test_request.py | 557 +++++----- tests/local_db/test_data_access.py | 27 +- tests/unit/test_business_logic.py | 1305 ++++++++++++++--------- 5 files changed, 1111 insertions(+), 786 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index cc3800c1..152835eb 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -14,7 +14,6 @@ CodeRepo, RequestFileType, RequestStatus, - UrlPath, UserFileReviewStatus, Workspace, bll, @@ -233,6 +232,9 @@ def create_repo(workspace, files=None, temporary=True): def create_release_request(workspace, user=None, **kwargs): + assert ( + kwargs.get("status", RequestStatus.PENDING) == RequestStatus.PENDING + ), "Use create_request_at_state to create a release request with a state other than PENDING" workspace = ensure_workspace(workspace) # create a default user with permission on workspace diff --git a/tests/functional/test_code_pages.py b/tests/functional/test_code_pages.py index f6748908..294e1c12 100644 --- a/tests/functional/test_code_pages.py +++ b/tests/functional/test_code_pages.py @@ -12,8 +12,8 @@ def release_request(researcher_user): workspace = factories.create_workspace("test-dir1") factories.write_workspace_file(workspace, "foo.txt", "") factories.create_repo(workspace) - release_request = factories.create_release_request( - workspace, user=researcher_user, status=RequestStatus.SUBMITTED + release_request = factories.create_request_at_state( + workspace, author=researcher_user, status=RequestStatus.SUBMITTED ) # Ensure the request file is written using the workspace previously # created (so it's assigned the correct commit from the manifest.json associated diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 904bdac7..e99b44ac 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -47,20 +47,22 @@ def test_request_id_does_not_exist(airlock_client): def test_request_view_root_summary(airlock_client): airlock_client.login(output_checker=True) audit_user = factories.create_user("audit_user") - release_request = factories.create_release_request("workspace", user=audit_user) - factories.write_request_file(release_request, "group1", "some_dir/file1.txt") - factories.write_request_file( - release_request, - "group1", - "some_dir/file2.txt", - filetype=RequestFileType.SUPPORTING, - ) - factories.write_request_file(release_request, "group2", "some_dir/file3.txt") - factories.write_request_file( - release_request, - "group2", - "some_dir/file4.txt", - filetype=RequestFileType.WITHDRAWN, + release_request = factories.create_request_at_state( + "workspace", + author=audit_user, + status=RequestStatus.PENDING, + files=[ + factories.request_file("group1", "some_dir/file1.txt"), + factories.request_file( + "group1", + "some_dir/file2.txt", + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file("group2", "some_dir/file3.txt"), + factories.request_file( + "group2", "some_dir/file4.txt", filetype=RequestFileType.WITHDRAWN + ), + ], ) response = airlock_client.get(f"/requests/view/{release_request.id}/") @@ -78,13 +80,18 @@ def test_request_view_root_summary(airlock_client): def test_request_view_root_group(airlock_client): airlock_client.login(output_checker=True) audit_user = factories.create_user("audit_user") - release_request = factories.create_release_request("workspace", user=audit_user) - factories.write_request_file(release_request, "group1", "some_dir/file1.txt") - factories.write_request_file( - release_request, - "group1", - "some_dir/file2.txt", - filetype=RequestFileType.SUPPORTING, + release_request = factories.create_request_at_state( + "workspace", + author=audit_user, + status=RequestStatus.PENDING, + files=[ + factories.request_file("group1", "some_dir/file1.txt"), + factories.request_file( + "group1", + "some_dir/file2.txt", + filetype=RequestFileType.SUPPORTING, + ), + ], ) response = airlock_client.get(f"/requests/view/{release_request.id}/group1/") @@ -142,7 +149,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( + release_request = factories.create_request_at_state( "workspace", status=RequestStatus.SUBMITTED ) response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) @@ -197,12 +204,14 @@ def test_request_view_with_withdrawn_request(airlock_client): def test_request_view_with_authored_request_file(airlock_client): airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=airlock_client.user, + author=airlock_client.user, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "file.txt", contents="foobar"), + ], ) - factories.write_request_file(release_request, "group", "file.txt", "foobar") response = airlock_client.get( f"/requests/view/{release_request.id}/group/file.txt", follow=True ) @@ -210,12 +219,14 @@ def test_request_view_with_authored_request_file(airlock_client): def test_request_view_with_submitted_file(airlock_client): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "file.txt", contents="foobar"), + ], ) - factories.write_request_file(release_request, "group", "file.txt", "foobar") response = airlock_client.get( f"/requests/view/{release_request.id}/group/file.txt", follow=True ) @@ -225,16 +236,15 @@ def test_request_view_with_submitted_file(airlock_client): def test_request_view_with_submitted_supporting_file(airlock_client): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, - "group", - "supporting_file.txt", - filetype=RequestFileType.SUPPORTING, + files=[ + factories.request_file( + "group", "supporting_file.txt", filetype=RequestFileType.SUPPORTING + ), + ], ) response = airlock_client.get( f"/requests/view/{release_request.id}/group/supporting_file.txt", follow=True @@ -246,12 +256,14 @@ def test_request_view_with_submitted_supporting_file(airlock_client): def test_request_view_with_submitted_file_approved(airlock_client): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "file.txt", contents="foobar"), + ], ) - factories.write_request_file(release_request, "group", "file.txt", "foobar") airlock_client.post(f"/requests/approve/{release_request.id}/group/file.txt") response = airlock_client.get( f"/requests/view/{release_request.id}/group/file.txt", follow=True @@ -261,12 +273,14 @@ def test_request_view_with_submitted_file_approved(airlock_client): def test_request_view_with_submitted_file_rejected(airlock_client): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "file.txt", contents="foobar"), + ], ) - factories.write_request_file(release_request, "group", "file.txt", "foobar") airlock_client.post(f"/requests/reject/{release_request.id}/group/file.txt") response = airlock_client.get( f"/requests/view/{release_request.id}/group/file.txt", follow=True @@ -286,14 +300,16 @@ def test_request_view_with_404(airlock_client): def test_request_view_404_with_files(airlock_client): airlock_client.login(output_checker=True) - release_request = factories.create_release_request("workspace") # write a file and a supporting file to the group - factories.write_request_file(release_request, "group", "file.txt") - factories.write_request_file( - release_request, - "group", - "supporting_file.txt", - filetype=RequestFileType.SUPPORTING, + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.PENDING, + files=[ + factories.request_file("group", "file.txt"), + factories.request_file( + "group", "supporting_file.txt", filetype=RequestFileType.SUPPORTING + ), + ], ) response = airlock_client.get( f"/requests/view/{release_request.id}/group/no_such_file.txt" @@ -507,17 +523,23 @@ def test_request_index_user_permitted_requests(airlock_client): 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=RequestStatus.SUBMITTED + r1 = factories.create_request_at_state( + "test_workspace", author=airlock_client.user, status=RequestStatus.SUBMITTED ) - r2 = factories.create_release_request( - "other_workspace", user=other, status=RequestStatus.SUBMITTED + r2 = factories.create_request_at_state( + "other_workspace", author=other, status=RequestStatus.SUBMITTED ) - r3 = factories.create_release_request( - "other_other_workspace", user=other, status=RequestStatus.RETURNED + r3 = factories.create_request_at_state( + "other_other_workspace", + author=other, + status=RequestStatus.RETURNED, + files=[factories.request_file(rejected=True)], ) - r4 = factories.create_release_request( - "other_other1_workspace", user=other, status=RequestStatus.APPROVED + r4 = factories.create_request_at_state( + "other_other1_workspace", + author=other, + status=RequestStatus.APPROVED, + files=[factories.request_file(approved=True)], ) response = airlock_client.get("/requests/") @@ -592,16 +614,15 @@ def test_request_withdraw_not_author(airlock_client): def test_request_return_author(airlock_client): airlock_client.login(workspaces=["test1"]) - release_request = factories.create_release_request( - "test1", user=airlock_client.user, status=RequestStatus.SUBMITTED - ) - factories.write_request_file( - release_request, "group", "path/test.txt", approved=True - ) - factories.write_request_file( - release_request, "group", "path/test1.txt", rejected=True + release_request = factories.create_request_at_state( + "test1", + author=airlock_client.user, + status=RequestStatus.REVIEWED, + files=[ + factories.request_file("group", "path/test.txt", approved=True), + factories.request_file("group", "path/test1.txt", rejected=True), + ], ) - factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/return/{release_request.id}") @@ -615,15 +636,15 @@ def test_request_return_author(airlock_client): def test_request_return_output_checker(airlock_client): airlock_client.login(workspaces=["test1"], output_checker=True) other_author = factories.create_user("other", [], False) - release_request = factories.create_release_request("test1", user=other_author) - factories.write_request_file( - release_request, "group", "path/test.txt", approved=True - ) - factories.write_request_file( - release_request, "group", "path/test1.txt", rejected=True + release_request = factories.create_request_at_state( + "test1", + author=other_author, + status=RequestStatus.REVIEWED, + files=[ + factories.request_file("group", "path/test.txt", approved=True), + factories.request_file("group", "path/test1.txt", rejected=True), + ], ) - factories.bll.set_status(release_request, RequestStatus.SUBMITTED, other_author) - factories.complete_independent_review(release_request) response = airlock_client.post(f"/requests/return/{release_request.id}") assert response.status_code == 302 @@ -631,6 +652,74 @@ def test_request_return_output_checker(airlock_client): assert persisted_request.status == RequestStatus.RETURNED +def test_request_review_author(airlock_client): + airlock_client.login(workspaces=["test1"], output_checker=True) + release_request = factories.create_request_at_state( + "test1", + author=airlock_client.user, + status=RequestStatus.SUBMITTED, + files=[factories.request_file(approved=True)], + ) + response = airlock_client.post(f"/requests/review/{release_request.id}") + + assert response.status_code == 403 + persisted_request = factories.refresh_release_request(release_request) + assert persisted_request.status == RequestStatus.SUBMITTED + + +def test_request_review_output_checker(airlock_client): + airlock_client.login("checker", workspaces=["test1"], output_checker=True) + release_request = factories.create_request_at_state( + "test1", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(approved=True, checkers=[airlock_client.user])], + ) + response = airlock_client.post( + f"/requests/review/{release_request.id}", follow=True + ) + + assert response.status_code == 200 + persisted_request = factories.refresh_release_request(release_request) + assert persisted_request.status == RequestStatus.PARTIALLY_REVIEWED + assert "Your review has been completed" in response.rendered_content + + +def test_request_review_non_output_checker(airlock_client): + airlock_client.login(workspaces=["test1"]) + release_request = factories.create_request_at_state( + "test1", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(approved=True)], + ) + response = airlock_client.post(f"/requests/review/{release_request.id}") + + assert response.status_code == 403 + persisted_request = factories.refresh_release_request(release_request) + assert persisted_request.status == RequestStatus.SUBMITTED + + +def test_request_review_not_all_files_reviewed(airlock_client): + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( + "test1", + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(approved=True, checkers=[airlock_client.user]), + factories.request_file(path="foo.txt"), + ], + ) + response = airlock_client.post( + f"/requests/review/{release_request.id}", follow=True + ) + + assert response.status_code == 200 + persisted_request = factories.refresh_release_request(release_request) + assert persisted_request.status == RequestStatus.SUBMITTED + assert ( + "You must review all files to complete your review" in response.rendered_content + ) + + def test_empty_requests_for_workspace(airlock_client): airlock_client.login(workspaces=["test1"]) @@ -671,13 +760,15 @@ def test_file_review_bad_user(airlock_client, review): workspace = "test1" airlock_client.login(workspaces=[workspace], output_checker=False) author = factories.create_user("author", [workspace], False) - release_request = factories.create_release_request( - workspace, - user=author, + path = "path/test.txt" + release_request = factories.create_request_at_state( + "test1", + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", path), + ], ) - path = "path/test.txt" - factories.write_request_file(release_request, "group", path, contents="test") response = airlock_client.post( f"/requests/{review}/{release_request.id}/group/{path}" @@ -699,13 +790,15 @@ def test_file_review_bad_user(airlock_client, review): def test_file_review_bad_file(airlock_client, review): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_release_request( + path = "path/test.txt" + release_request = factories.create_request_at_state( "test1", - user=author, + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", path), + ], ) - path = "path/test.txt" - factories.write_request_file(release_request, "group", path, contents="test") bad_path = "path/bad.txt" response = airlock_client.post( @@ -727,13 +820,15 @@ def test_file_review_bad_file(airlock_client, review): def test_file_approve(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_release_request( + path = "path/test.txt" + release_request = factories.create_request_at_state( "test1", - user=author, + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", path), + ], ) - path = "path/test.txt" - factories.write_request_file(release_request, "group", path, contents="test") response = airlock_client.post( f"/requests/approve/{release_request.id}/group/{path}" @@ -752,13 +847,15 @@ def test_file_approve(airlock_client): def test_file_reject(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_release_request( + path = "path/test.txt" + release_request = factories.create_request_at_state( "test1", - user=author, + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", path), + ], ) - path = "path/test.txt" - factories.write_request_file(release_request, "group", path, contents="test") response = airlock_client.post( f"/requests/reject/{release_request.id}/group/{path}" @@ -777,25 +874,25 @@ def test_file_reject(airlock_client): def test_file_reset_review(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_release_request( + path = "path/test.txt" + release_request = factories.create_request_at_state( "test1", - user=author, + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", path), + ], ) - path = "path/test.txt" - factories.write_request_file(release_request, "group", path, contents="test") - # first reject a file response = airlock_client.post( f"/requests/reject/{release_request.id}/group/{path}" ) assert response.status_code == 302 relpath = UrlPath(path) - review = ( - bll.get_release_request(release_request.id, author) - .get_request_file_from_output_path(relpath) - .reviews[airlock_client.user.username] - ) + release_request = factories.refresh_release_request(release_request) + review = release_request.get_request_file_from_output_path(relpath).reviews[ + airlock_client.user.username + ] assert review.status == UserFileReviewStatus.REJECTED assert review.reviewer == "testuser" @@ -805,12 +902,8 @@ def test_file_reset_review(airlock_client): ) assert response.status_code == 302 relpath = UrlPath(path) - reviews = ( - bll.get_release_request(release_request.id, author) - .filegroups["group"] - .files[relpath] - .reviews - ) + release_request = factories.refresh_release_request(release_request) + reviews = release_request.filegroups["group"].files[relpath].reviews assert len(reviews) == 0 # verify a re-request @@ -823,16 +916,14 @@ def test_file_reset_review(airlock_client): def test_request_reject_output_checker(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "test1", - user=author, - status=RequestStatus.SUBMITTED, + author=author, + status=RequestStatus.REVIEWED, + files=[ + factories.request_file(rejected=True), + ], ) - factories.write_request_file( - release_request, "group", "path/test.txt", rejected=True - ) - factories.complete_independent_review(release_request) - response = airlock_client.post(f"/requests/reject/{release_request.id}") assert response.status_code == 302 @@ -841,16 +932,15 @@ def test_request_reject_output_checker(airlock_client): def test_request_reject_not_output_checker(airlock_client): - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "test1", - status=RequestStatus.SUBMITTED, + author=factories.create_user("author1", workspaces=["test1"]), + status=RequestStatus.REVIEWED, + files=[ + factories.request_file(rejected=True), + ], ) airlock_client.login(workspaces=[release_request.workspace], output_checker=False) - factories.write_request_file( - release_request, "group", "path/test.txt", rejected=True - ) - factories.complete_independent_review(release_request) - response = airlock_client.post(f"/requests/reject/{release_request.id}") assert response.status_code == 403 @@ -861,15 +951,15 @@ def test_request_reject_not_output_checker(airlock_client): def test_file_withdraw_file_pending(airlock_client): - author = factories.create_user("author", ["test1"], False) - airlock_client.login_with_user(author) - release_request = factories.create_release_request( + airlock_client.login("author", ["test1"], False) + release_request = factories.create_request_at_state( "test1", - user=author, + author=airlock_client.user, status=RequestStatus.PENDING, + files=[ + factories.request_file("group", "path/test.txt"), + ], ) - factories.write_request_file(release_request, "group", "path/test.txt") - release_request = factories.refresh_release_request(release_request) # ensure it does exist release_request.get_request_file_from_urlpath("group/path/test.txt") @@ -880,23 +970,22 @@ def test_file_withdraw_file_pending(airlock_client): assert response.status_code == 302 assert response.headers["location"] == release_request.get_url("group") - persisted_request = bll.get_release_request(release_request.id, author) + persisted_request = factories.refresh_release_request(release_request) with pytest.raises(bll.FileNotFound): persisted_request.get_request_file_from_urlpath("group/path/test.txt") def test_file_withdraw_file_submitted(airlock_client): - author = factories.create_user("author", ["test1"], False) - airlock_client.login_with_user(author) - release_request = factories.create_release_request( + airlock_client.login("author", ["test1"], False) + release_request = factories.create_request_at_state( "test1", - user=author, + author=airlock_client.user, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "path/test.txt", rejected=True), + ], ) - factories.write_request_file(release_request, "group", "path/test.txt") - release_request = factories.refresh_release_request(release_request) - # ensure it does exist release_request.get_request_file_from_urlpath("group/path/test.txt") @@ -909,7 +998,7 @@ def test_file_withdraw_file_submitted(airlock_client): assert response.status_code == 200 assert "This file has been withdrawn" in response.rendered_content - persisted_request = bll.get_release_request(release_request.id, author) + persisted_request = factories.refresh_release_request(release_request) request_file = persisted_request.get_request_file_from_urlpath( "group/path/test.txt" ) @@ -951,29 +1040,21 @@ def test_file_withdraw_file_bad_request(airlock_client): assert response.status_code == 404 -@pytest.mark.parametrize("status", [RequestStatus.SUBMITTED, RequestStatus.APPROVED]) +@pytest.mark.parametrize("status", [RequestStatus.REVIEWED, RequestStatus.APPROVED]) def test_request_release_files_success(airlock_client, release_files_stubber, status): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login(username="checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", id="request_id", - status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=True + status=status, + files=[ + factories.request_file(path="file.txt", contents="test1", approved=True), + factories.request_file(path="file1.txt", contents="test2", approved=True), + ], ) - factories.write_request_file( - release_request, "group", "test/file2.txt", "test2", approved=True - ) - factories.complete_independent_review(release_request) - - if status == RequestStatus.APPROVED: - release_request = factories.refresh_release_request(release_request) - bll.set_status(release_request, status, airlock_client.user) api_responses = release_files_stubber(release_request) response = airlock_client.post("/requests/release/request_id") - assert response.url == "/requests/view/request_id/" assert response.status_code == 302 @@ -981,28 +1062,20 @@ def test_request_release_files_success(airlock_client, release_files_stubber, st assert api_responses.calls[2].request.body.read() == b"test2" -@pytest.mark.parametrize("status", [RequestStatus.SUBMITTED, RequestStatus.APPROVED]) +@pytest.mark.parametrize("status", [RequestStatus.REVIEWED, RequestStatus.APPROVED]) def test_request_release_files_success_htmx( airlock_client, release_files_stubber, status ): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login(username="checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", id="request_id", - status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=True - ) - factories.write_request_file( - release_request, "group", "test/file2.txt", "test2", approved=True + status=status, + files=[ + factories.request_file(path="file.txt", contents="test1", approved=True), + factories.request_file(path="file1.txt", contents="test2", approved=True), + ], ) - if status == RequestStatus.APPROVED: - release_request = factories.refresh_release_request(release_request) - factories.complete_independent_review(release_request) - release_request = factories.refresh_release_request(release_request) - factories.bll.set_status(release_request, status, airlock_client.user) - api_responses = release_files_stubber(release_request) response = airlock_client.post( "/requests/release/request_id", @@ -1017,31 +1090,29 @@ def test_request_release_files_success_htmx( def test_requests_release_workspace_403(airlock_client): - airlock_client.login(output_checker=False) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=False) + factories.create_request_at_state( "workspace", id="request_id", status=RequestStatus.SUBMITTED, + files=[ + factories.request_file("group", "path/test.txt", approved=True), + ], ) - factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=True - ) + response = airlock_client.post("/requests/release/request_id") assert response.status_code == 403 def test_requests_release_author_403(airlock_client): airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + factories.create_request_at_state( "workspace", id="request_id", - user=airlock_client.user, - status=RequestStatus.SUBMITTED, + author=airlock_client.user, + status=RequestStatus.REVIEWED, + files=[factories.request_file(approved=True)], ) - factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=True - ) - factories.complete_independent_review(release_request) response = airlock_client.post("/requests/release/request_id", follow=True) assert response.status_code == 200 assert ( @@ -1050,17 +1121,30 @@ def test_requests_release_author_403(airlock_client): ) -def test_requests_release_jobserver_403(airlock_client, release_files_stubber): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( +def test_requests_release_invalid_state_transition_403(airlock_client): + airlock_client.login("checker", output_checker=True) + factories.create_request_at_state( "workspace", id="request_id", - status=RequestStatus.SUBMITTED, + status=RequestStatus.RETURNED, + files=[factories.request_file(approved=True)], ) - factories.write_request_file( - release_request, "group", "test/file.txt", "test", approved=True + response = airlock_client.post("/requests/release/request_id", follow=True) + assert response.status_code == 200 + assert ( + list(response.context["messages"])[0].message + == "Error releasing files: cannot change status from RETURNED to APPROVED" + ) + + +def test_requests_release_jobserver_403(airlock_client, release_files_stubber): + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + id="request_id", + status=RequestStatus.REVIEWED, + files=[factories.request_file(approved=True)], ) - factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 403 @@ -1091,17 +1175,14 @@ def test_requests_release_jobserver_403_with_debug( content_type, content, ): - airlock_client.login(output_checker=True) + airlock_client.login("checker", output_checker=True) settings.DEBUG = True - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", id="request_id", - status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, "default", "test/file.txt", "test", approved=True + status=RequestStatus.REVIEWED, + files=[factories.request_file(approved=True)], ) - factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 403 @@ -1119,35 +1200,14 @@ def test_requests_release_jobserver_403_with_debug( assert f"Type: {content_type}" in error_message -def test_requests_release_unapproved_files(airlock_client): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( - "workspace", - id="request_id", - status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, "group", "test/file1.txt", "test1", approved=True - ) - - response = airlock_client.post("/requests/release/request_id", follow=True) - assert response.status_code == 200 - assert ( - "request has unapproved files" in list(response.context["messages"])[0].message - ) - - def test_requests_release_files_404(airlock_client, release_files_stubber): - airlock_client.login(output_checker=True) - release_request = factories.create_release_request( + airlock_client.login("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", id="request_id", - status=RequestStatus.SUBMITTED, - ) - factories.write_request_file( - release_request, "group", "test/file.txt", "test", approved=True + status=RequestStatus.REVIEWED, + files=[factories.request_file(approved=True)], ) - factories.complete_independent_review(release_request) response = requests.Response() response.status_code = 404 @@ -1207,25 +1267,24 @@ def test_request_view_tracing_with_request_attribute( airlock_client, release_files_stubber, urlpath, post_data, login_as, status, stub ): author = factories.create_user("author", ["test-workspace"]) - factories.create_user("output_checker", output_checker=True) + checker = factories.create_user("output_checker", output_checker=True) airlock_client.login(username=login_as, output_checker=True) - initial_status = ( - RequestStatus.SUBMITTED if status == RequestStatus.REVIEWED else status - ) - release_request = factories.create_release_request( - "test-workspace", id="request-id", user=author, status=initial_status + release_request = factories.create_request_at_state( + "test-workspace", + status=status, + author=author, + id="request-id", + files=[ + factories.request_file( + "default", + "file.txt", + approved=status in [RequestStatus.REVIEWED, RequestStatus.RELEASED], + checkers=[checker, factories.create_user(output_checker=True)], + ), + ], ) - factories.write_request_file( - release_request, - "default", - "file.txt", - contents="test", - approved=status == RequestStatus.REVIEWED, - ) - if status == RequestStatus.REVIEWED: - factories.complete_independent_review(release_request) if stub: release_files_stubber(release_request) @@ -1239,13 +1298,13 @@ def test_request_view_tracing_with_request_attribute( def test_group_edit_success(airlock_client): - author = factories.create_user("author", ["workspace"], False) + airlock_client.login("author", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) + release_request = factories.create_release_request( + "workspace", user=airlock_client.user + ) factories.write_request_file(release_request, "group", "file.txt") - airlock_client.login_with_user(author) - response = airlock_client.post( f"/requests/edit/{release_request.id}/group", data={ @@ -1259,20 +1318,26 @@ def test_group_edit_success(airlock_client): messages = list(response.context.get("messages", [])) assert messages[0].message == "Updated group group" - release_request = bll.get_release_request(release_request.id, author) + release_request = factories.refresh_release_request(release_request) assert release_request.filegroups["group"].context == "foo" assert release_request.filegroups["group"].controls == "bar" def test_group_edit_no_change(airlock_client, bll): - author = factories.create_user("author", ["workspace"], False) + airlock_client.login("author", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) + release_request = factories.create_release_request( + "workspace", user=airlock_client.user + ) factories.write_request_file(release_request, "group", "file.txt") - bll.group_edit(release_request, "group", context="foo", controls="bar", user=author) - - airlock_client.login_with_user(author) + bll.group_edit( + release_request, + "group", + context="foo", + controls="bar", + user=airlock_client.user, + ) response = airlock_client.post( f"/requests/edit/{release_request.id}/group", @@ -1287,7 +1352,7 @@ def test_group_edit_no_change(airlock_client, bll): messages = list(response.context.get("messages", [])) assert messages[0].message == "No changes made to group group" - release_request = bll.get_release_request(release_request.id, author) + release_request = factories.refresh_release_request(release_request) assert release_request.filegroups["group"].context == "foo" assert release_request.filegroups["group"].controls == "bar" diff --git a/tests/local_db/test_data_access.py b/tests/local_db/test_data_access.py index 683dbe78..ef451312 100644 --- a/tests/local_db/test_data_access.py +++ b/tests/local_db/test_data_access.py @@ -83,8 +83,8 @@ def test_get_audit_log(test_audits, kwargs, expected_audits): def test_delete_file_from_request_bad_state(): author = factories.create_user() - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED, user=author + release_request = factories.create_request_at_state( + "workspace", status=RequestStatus.SUBMITTED, author=author ) audit = AuditEvent.from_request( release_request, @@ -97,7 +97,7 @@ def test_delete_file_from_request_bad_state(): @pytest.mark.parametrize( - "state", + "status", [ RequestStatus.PENDING, RequestStatus.WITHDRAWN, @@ -105,12 +105,14 @@ def test_delete_file_from_request_bad_state(): RequestStatus.APPROVED, ], ) -def test_withdraw_file_from_request_bad_state(state): +def test_withdraw_file_from_request_bad_state(status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=author, - status=state, + author=author, + status=status, + files=[factories.request_file(approved=status != RequestStatus.PENDING)], + withdrawn_after=RequestStatus.PENDING, ) with pytest.raises(AssertionError): @@ -145,13 +147,12 @@ def test_group_comment_delete_bad_params(): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["other-workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.PENDING, + files=[factories.request_file()], ) - release_request = factories.refresh_release_request(release_request) audit = AuditEvent.from_request( request=release_request, diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 2b614294..0aba1193 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -309,10 +309,10 @@ def mock_old_api(monkeypatch): def test_provider_request_release_files_request_not_approved(bll, mock_notifications): author = factories.create_user("author", ["workspace"]) - checker = factories.create_user("checker", [], output_checker=True) - release_request = factories.create_release_request( + checker = factories.create_user("checker", output_checker=True) + release_request = factories.create_request_at_state( "workspace", - user=author, + author=author, id="request_id", status=RequestStatus.SUBMITTED, ) @@ -320,33 +320,23 @@ def test_provider_request_release_files_request_not_approved(bll, mock_notificat with pytest.raises(bll.InvalidStateTransition): bll.release_files(release_request, checker) - # Note factories.create_release_request bypasses bll.set_status, so - # doesn't trigger notifications + # Notification for submitting request only # Failed release attempt does not notify - assert_no_notifications(mock_notifications) + assert_last_notification(mock_notifications, "request_submitted") def test_provider_request_release_files_invalid_file_type(bll, mock_notifications): - author = factories.create_user("author", ["workspace"]) - checker = factories.create_user("checker", [], output_checker=True) - release_request = factories.create_release_request( - "workspace", - user=author, - id="request_id", - status=RequestStatus.SUBMITTED, - ) - - # mock the LEVEL4_FILE_TYPES so that we can add this invalid file to the - # request - relpath = Path("test/file.foo") + # mock the LEVEL4_FILE_TYPES so that we can create this request with an + # invalid file with patch("airlock.utils.LEVEL4_FILE_TYPES", [".foo"]): - factories.write_request_file( - release_request, "group", relpath, "test", approved=True + release_request = factories.create_request_at_state( + "workspace", + id="request_id", + status=RequestStatus.APPROVED, + files=[factories.request_file(path="test/file.foo", approved=True)], ) - factories.complete_independent_review(release_request) - release_request = factories.refresh_release_request(release_request) - bll.set_status(release_request, RequestStatus.APPROVED, checker) + checker = factories.create_user("checker", [], output_checker=True) with pytest.raises(bll.RequestPermissionDenied): bll.release_files(release_request, checker) assert_last_notification(mock_notifications, "request_approved") @@ -354,39 +344,43 @@ def test_provider_request_release_files_invalid_file_type(bll, mock_notification def test_provider_request_release_files(mock_old_api, mock_notifications, bll, freezer): old_api.create_release.return_value = "jobserver_id" - author = factories.create_user("author", ["workspace"]) checker = factories.create_user("checker", [], output_checker=True) - release_request = factories.create_release_request( + checker1 = factories.create_user("checker1", [], output_checker=True) + release_request = factories.create_request_at_state( "workspace", - user=author, id="request_id", - status=RequestStatus.SUBMITTED, - ) - relpath = UrlPath("test/file.txt") - factories.write_request_file( - release_request, "group", relpath, "test", approved=True - ) - # Add a supporting file, which should NOT be released - supporting_relpath = Path("test/supporting_file.txt") - factories.write_request_file( - release_request, - "group", - supporting_relpath, - "test", - filetype=RequestFileType.SUPPORTING, + status=RequestStatus.APPROVED, + checker=checker, + files=[ + factories.request_file( + group="group", + path="test/file.txt", + contents="test", + approved=True, + checkers=[checker, checker1], + ), + # a supporting file, which should NOT be released + factories.request_file( + group="group", + path="test/supporting_file.txt", + filetype=RequestFileType.SUPPORTING, + ), + # An approved but withdrawn file, which should NOT be released + factories.request_file( + group="group", + path="test/withdrawn_file.txt", + filetype=RequestFileType.WITHDRAWN, + approved=True, + ), + ], ) - factories.complete_independent_review(release_request) - release_request = factories.refresh_release_request(release_request) - factories.bll.set_status(release_request, RequestStatus.APPROVED, checker) - + relpath = Path("test/file.txt") abspath = release_request.abspath("group" / relpath) freezer.move_to("2022-01-01T12:34:56") bll.release_files(release_request, checker) - # TODO: when we do this, it reverses the order of the audit log in the data struct?? release_request = factories.refresh_release_request(release_request) - request_file = release_request.filegroups["group"].files[relpath] assert request_file.released_by == checker.username assert request_file.released_at == parse_datetime("2022-01-01T12:34:56Z") @@ -416,37 +410,75 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f notification_responses = parse_notification_responses(mock_notifications) # Notifications expected for: - # - write file x2 + # - set status to submitted # - set status to partially reviewed # - set status to reviewed # - set status to approved # - set status to released - assert notification_responses["count"] == 6 + # (Note: files are added to the request when it is in pending status, so no notifications sent.) + assert notification_responses["count"] == 5 request_json = notification_responses["request_json"] - assert request_json[0]["event_type"] == "request_updated" - assert request_json[1]["event_type"] == "request_updated" - assert request_json[2]["event_type"] == "request_partially_reviewed" - assert request_json[3]["event_type"] == "request_reviewed" - assert request_json[4]["event_type"] == "request_approved" - assert request_json[5]["event_type"] == "request_released" + expected_notifications = [ + "request_submitted", + "request_partially_reviewed", + "request_reviewed", + "request_approved", + "request_released", + ] + assert [event["event_type"] for event in request_json] == expected_notifications audit_log = bll.get_audit_log(request=release_request.id) + expected_audit_logs = [ + # create request + AuditEventType.REQUEST_CREATE, + # add 3 files + AuditEventType.REQUEST_FILE_ADD, + AuditEventType.REQUEST_FILE_ADD, + AuditEventType.REQUEST_FILE_ADD, + # submit request + AuditEventType.REQUEST_SUBMIT, + # checker reviews + AuditEventType.REQUEST_REVIEW, + # checker1 reviews + AuditEventType.REQUEST_REVIEW, + # appprove, release 1 output file, change request to released + AuditEventType.REQUEST_APPROVE, + AuditEventType.REQUEST_FILE_RELEASE, + AuditEventType.REQUEST_RELEASE, + ] + assert [log.type for log in audit_log] == expected_audit_logs + + checker_review_log = audit_log[5] + checker1_review_log = audit_log[6] + approve_log = audit_log[7] + release_file_log = audit_log[8] + release_log = audit_log[9] + + assert checker_review_log.type == AuditEventType.REQUEST_REVIEW + assert checker_review_log.user == checker.username + assert checker_review_log.request == release_request.id + assert checker_review_log.workspace == "workspace" - assert audit_log[5].type == AuditEventType.REQUEST_APPROVE - assert audit_log[5].user == checker.username - assert audit_log[5].request == release_request.id - assert audit_log[5].workspace == "workspace" + assert checker1_review_log.type == AuditEventType.REQUEST_REVIEW + assert checker1_review_log.user == checker1.username + assert checker1_review_log.request == release_request.id + assert checker1_review_log.workspace == "workspace" - assert audit_log[6].type == AuditEventType.REQUEST_FILE_RELEASE - assert audit_log[6].user == checker.username - assert audit_log[6].request == release_request.id - assert audit_log[6].workspace == "workspace" - assert audit_log[6].path == Path("test/file.txt") + assert approve_log.type == AuditEventType.REQUEST_APPROVE + assert approve_log.user == checker.username + assert approve_log.request == release_request.id + assert approve_log.workspace == "workspace" - assert audit_log[7].type == AuditEventType.REQUEST_RELEASE - assert audit_log[7].user == checker.username - assert audit_log[7].request == release_request.id - assert audit_log[7].workspace == "workspace" + assert release_file_log.type == AuditEventType.REQUEST_FILE_RELEASE + assert release_file_log.user == checker.username + assert release_file_log.request == release_request.id + assert release_file_log.workspace == "workspace" + assert release_file_log.path == Path("test/file.txt") + + assert release_log.type == AuditEventType.REQUEST_RELEASE + assert release_log.user == checker.username + assert release_log.request == release_request.id + assert release_log.workspace == "workspace" def test_provider_get_requests_for_workspace(bll): @@ -505,14 +537,14 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, user = factories.create_user("test", ["workspace"], output_checker) 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=RequestStatus.SUBMITTED + factories.create_request_at_state( + "workspace", author=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=RequestStatus.SUBMITTED + factories.create_request_at_state( + "workspace", author=user, id="r2", status=RequestStatus.SUBMITTED ) # requests authored by other users, status other than pending for i, status in enumerate( @@ -526,7 +558,13 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_release_request(ws, user_n, status=status) + factories.create_request_at_state( + ws, + author=user_n, + status=status, + files=[factories.request_file(approved=status != RequestStatus.PENDING)], + withdrawn_after=RequestStatus.PENDING, + ) assert set(r.id for r in bll.get_outstanding_requests_for_review(user)) == set( expected @@ -548,23 +586,23 @@ def test_provider_get_returned_requests(output_checker, expected, bll): other_user = factories.create_user("other", ["workspace"], False) output_checker = factories.create_user("other-checker", ["workspace"], True) # request created by another user, status returned - release_request1 = factories.create_release_request( - "workspace", other_user, id="r1", status=RequestStatus.SUBMITTED + factories.create_request_at_state( + "workspace", + author=other_user, + id="r1", + status=RequestStatus.RETURNED, + files=[factories.request_file(path="file.txt", approved=True)], ) - factories.write_request_file(release_request1, "group", "file.txt", approved=True) - factories.complete_independent_review(release_request1) - release_request1 = factories.refresh_release_request(release_request1) - factories.bll.set_status(release_request1, RequestStatus.RETURNED, output_checker) # requests not visible to output checker # status returned, but authored by output checker - release_request2 = factories.create_release_request( - "workspace", user, id="r2", status=RequestStatus.SUBMITTED + factories.create_request_at_state( + "workspace", + author=user, + id="r2", + status=RequestStatus.RETURNED, + files=[factories.request_file(path="file.txt", approved=True)], ) - factories.write_request_file(release_request2, "group", "file.txt", approved=True) - factories.complete_independent_review(release_request2) - release_request2 = factories.refresh_release_request(release_request2) - factories.bll.set_status(release_request2, RequestStatus.RETURNED, output_checker) # requests authored by other users, status other than returned for i, status in enumerate( @@ -579,7 +617,13 @@ def test_provider_get_returned_requests(output_checker, expected, bll): ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_release_request(ws, user_n, status=status) + factories.create_request_at_state( + ws, + author=user_n, + status=status, + withdrawn_after=RequestStatus.PENDING, + files=[factories.request_file(approved=True)], + ) assert set(r.id for r in bll.get_returned_requests(user)) == set(expected) @@ -598,15 +642,24 @@ def test_provider_get_approved_requests(output_checker, expected, bll): user = factories.create_user("test", ["workspace"], output_checker) other_user = factories.create_user("other", ["workspace"], False) output_checker = factories.create_user("other-checker", ["workspace"], True) + # request created by another user, status approved - factories.create_release_request( - "workspace", other_user, id="r1", status=RequestStatus.APPROVED + factories.create_request_at_state( + "workspace", + author=other_user, + id="r1", + status=RequestStatus.APPROVED, + files=[factories.request_file(path="file.txt", approved=True)], ) # requests not visible to output checker - # status returned, but authored by output checker - factories.create_release_request( - "workspace", user, id="r2", status=RequestStatus.APPROVED + # status approved, but authored by output checker + factories.create_request_at_state( + "workspace", + author=user, + id="r2", + status=RequestStatus.APPROVED, + files=[factories.request_file(path="file.txt", approved=True)], ) # requests authored by other users, status other than approved @@ -622,8 +675,13 @@ def test_provider_get_approved_requests(output_checker, expected, bll): ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_release_request(ws, user_n, status=status) - + factories.create_request_at_state( + ws, + author=user_n, + status=status, + withdrawn_after=RequestStatus.PENDING, + files=[factories.request_file(approved=True)], + ) assert set(r.id for r in bll.get_approved_requests(user)) == set(expected) @@ -693,72 +751,187 @@ def test_provider_get_current_request_for_user_output_checker(bll): @pytest.mark.parametrize( - "current,future,valid_author,valid_checker", + "current,future,valid_author,valid_checker,withdrawn_after", # valid_author: author can set status of their own request # valid_checker: checker can set status of another author's request [ - (RequestStatus.PENDING, RequestStatus.SUBMITTED, True, False), - (RequestStatus.PENDING, RequestStatus.WITHDRAWN, True, False), - (RequestStatus.PENDING, RequestStatus.PARTIALLY_REVIEWED, False, False), - (RequestStatus.PENDING, RequestStatus.REVIEWED, False, False), - (RequestStatus.PENDING, RequestStatus.APPROVED, False, False), - (RequestStatus.PENDING, RequestStatus.REJECTED, False, False), - (RequestStatus.PENDING, RequestStatus.RELEASED, False, False), - (RequestStatus.SUBMITTED, RequestStatus.PENDING, False, False), - (RequestStatus.SUBMITTED, RequestStatus.PARTIALLY_REVIEWED, False, True), - (RequestStatus.SUBMITTED, RequestStatus.REVIEWED, False, False), - (RequestStatus.SUBMITTED, RequestStatus.APPROVED, False, False), - (RequestStatus.SUBMITTED, RequestStatus.REJECTED, False, False), - (RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN, False, False), - (RequestStatus.SUBMITTED, RequestStatus.RETURNED, False, False), - (RequestStatus.SUBMITTED, RequestStatus.RELEASED, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.PENDING, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.SUBMITTED, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REVIEWED, False, True), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.APPROVED, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REJECTED, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.RELEASED, False, False), - (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.WITHDRAWN, False, False), - (RequestStatus.REVIEWED, RequestStatus.PENDING, False, False), - (RequestStatus.REVIEWED, RequestStatus.SUBMITTED, False, False), - (RequestStatus.REVIEWED, RequestStatus.PARTIALLY_REVIEWED, False, False), - (RequestStatus.REVIEWED, RequestStatus.RETURNED, False, True), - (RequestStatus.REVIEWED, RequestStatus.APPROVED, False, True), - (RequestStatus.REVIEWED, RequestStatus.REJECTED, False, True), - (RequestStatus.REVIEWED, RequestStatus.WITHDRAWN, False, False), - (RequestStatus.RETURNED, RequestStatus.SUBMITTED, True, False), - (RequestStatus.RETURNED, RequestStatus.WITHDRAWN, True, False), - (RequestStatus.APPROVED, RequestStatus.RELEASED, False, True), - (RequestStatus.APPROVED, RequestStatus.REJECTED, False, False), - (RequestStatus.APPROVED, RequestStatus.WITHDRAWN, False, 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), + (RequestStatus.PENDING, RequestStatus.SUBMITTED, True, False, None), + (RequestStatus.PENDING, RequestStatus.WITHDRAWN, True, False, None), + (RequestStatus.PENDING, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.PENDING, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.PENDING, RequestStatus.APPROVED, False, False, None), + (RequestStatus.PENDING, RequestStatus.REJECTED, False, False, None), + (RequestStatus.PENDING, RequestStatus.RELEASED, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.PENDING, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.PARTIALLY_REVIEWED, False, True, None), + (RequestStatus.SUBMITTED, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.APPROVED, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.REJECTED, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.WITHDRAWN, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.RETURNED, False, False, None), + (RequestStatus.SUBMITTED, RequestStatus.RELEASED, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.PENDING, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.SUBMITTED, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REVIEWED, False, True, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.APPROVED, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.REJECTED, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.RELEASED, False, False, None), + (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.WITHDRAWN, False, False, None), + (RequestStatus.REVIEWED, RequestStatus.PENDING, False, False, None), + (RequestStatus.REVIEWED, RequestStatus.SUBMITTED, False, False, None), + (RequestStatus.REVIEWED, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.REVIEWED, RequestStatus.RETURNED, False, True, None), + (RequestStatus.REVIEWED, RequestStatus.APPROVED, False, True, None), + (RequestStatus.REVIEWED, RequestStatus.REJECTED, False, True, None), + (RequestStatus.REVIEWED, RequestStatus.WITHDRAWN, False, False, None), + (RequestStatus.RETURNED, RequestStatus.PENDING, False, False, None), + (RequestStatus.RETURNED, RequestStatus.SUBMITTED, True, False, None), + (RequestStatus.RETURNED, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.RETURNED, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.RETURNED, RequestStatus.APPROVED, False, False, None), + (RequestStatus.RETURNED, RequestStatus.REJECTED, False, False, None), + (RequestStatus.RETURNED, RequestStatus.WITHDRAWN, True, False, None), + (RequestStatus.APPROVED, RequestStatus.PENDING, False, False, None), + (RequestStatus.APPROVED, RequestStatus.SUBMITTED, False, False, None), + (RequestStatus.APPROVED, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.APPROVED, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.APPROVED, RequestStatus.RELEASED, False, True, None), + (RequestStatus.APPROVED, RequestStatus.REJECTED, False, False, None), + (RequestStatus.APPROVED, RequestStatus.WITHDRAWN, False, False, None), + (RequestStatus.REJECTED, RequestStatus.PENDING, False, False, None), + (RequestStatus.REJECTED, RequestStatus.SUBMITTED, False, False, None), + (RequestStatus.REJECTED, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.REJECTED, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.REJECTED, RequestStatus.APPROVED, False, True, None), + (RequestStatus.REJECTED, RequestStatus.WITHDRAWN, False, False, None), + (RequestStatus.RELEASED, RequestStatus.PENDING, False, False, None), + (RequestStatus.RELEASED, RequestStatus.SUBMITTED, False, False, None), + (RequestStatus.RELEASED, RequestStatus.PARTIALLY_REVIEWED, False, False, None), + (RequestStatus.RELEASED, RequestStatus.REVIEWED, False, False, None), + (RequestStatus.RELEASED, RequestStatus.APPROVED, False, False, None), + (RequestStatus.RELEASED, RequestStatus.REJECTED, False, False, None), + (RequestStatus.RELEASED, RequestStatus.WITHDRAWN, False, False, None), + ( + RequestStatus.WITHDRAWN, + RequestStatus.PENDING, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.SUBMITTED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.PARTIALLY_REVIEWED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.REVIEWED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.APPROVED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.REJECTED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.RETURNED, + False, + False, + RequestStatus.PENDING, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.PENDING, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.SUBMITTED, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.PARTIALLY_REVIEWED, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.REVIEWED, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.APPROVED, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.REJECTED, + False, + False, + RequestStatus.RETURNED, + ), + ( + RequestStatus.WITHDRAWN, + RequestStatus.RETURNED, + False, + False, + RequestStatus.RETURNED, + ), ], ) -def test_set_status(current, future, valid_author, valid_checker, bll): +def test_set_status(current, future, valid_author, valid_checker, withdrawn_after, bll): author = factories.create_user("author", ["workspace"], False) - checker = factories.create_user("checker", [], True) + checker = factories.create_user(output_checker=True) + file_reviewers = [checker, factories.create_user("checker1", [], True)] audit_type = bll.STATUS_AUDIT_EVENT[future] release_request1 = factories.create_request_at_state( "workspace1", - author, - current, - files=[factories.request_file(approved=True)], + status=current, + author=author, + checker=checker, + withdrawn_after=withdrawn_after, + files=[factories.request_file(approved=True, checkers=file_reviewers)], ) release_request2 = factories.create_request_at_state( "workspace2", - author, - current, - files=[factories.request_file(approved=True)], + status=current, + author=author, + checker=checker, + withdrawn_after=withdrawn_after, + files=[factories.request_file(approved=True, checkers=file_reviewers)], ) if valid_author: @@ -796,26 +969,33 @@ def test_request_status_ownership(bll): "current,future,user,notification_event_type", [ (RequestStatus.PENDING, RequestStatus.SUBMITTED, "author", "request_submitted"), + (RequestStatus.PENDING, RequestStatus.WITHDRAWN, "author", "request_withdrawn"), ( RequestStatus.SUBMITTED, - RequestStatus.APPROVED, + RequestStatus.PARTIALLY_REVIEWED, "checker", - "request_approved", + "request_partially_reviewed", ), ( - RequestStatus.SUBMITTED, + RequestStatus.PARTIALLY_REVIEWED, + RequestStatus.REVIEWED, + "checker", + "request_reviewed", + ), + ( + RequestStatus.REVIEWED, RequestStatus.REJECTED, "checker", "request_rejected", ), ( - RequestStatus.SUBMITTED, - RequestStatus.WITHDRAWN, - "author", - "request_withdrawn", + RequestStatus.REVIEWED, + RequestStatus.APPROVED, + "checker", + "request_approved", ), ( - RequestStatus.SUBMITTED, + RequestStatus.REVIEWED, RequestStatus.RETURNED, "checker", "request_returned", @@ -826,7 +1006,14 @@ def test_request_status_ownership(bll): "author", "request_resubmitted", ), + ( + RequestStatus.RETURNED, + RequestStatus.WITHDRAWN, + "author", + "request_withdrawn", + ), (RequestStatus.APPROVED, RequestStatus.RELEASED, "checker", "request_released"), + (RequestStatus.REJECTED, RequestStatus.APPROVED, "checker", "request_approved"), ], ) def test_set_status_notifications( @@ -834,21 +1021,14 @@ def test_set_status_notifications( ): users = { "author": factories.create_user("author", ["workspace"], False), - "checker": factories.create_user("checker", [], True), + "checker": factories.create_user(output_checker=True), } - release_request = factories.create_release_request( - "workspace", user=users["author"], status=RequestStatus.PENDING - ) - factories.write_request_file( - release_request, "group", "test/file.txt", approved=True + release_request = factories.create_request_at_state( + "workspace", + status=current, + files=[factories.request_file(approved=True)], + author=users["author"], ) - release_request = factories.refresh_release_request(release_request) - - if current != RequestStatus.PENDING: - bll.set_status(release_request, RequestStatus.SUBMITTED, user=users["author"]) - if current in [RequestStatus.APPROVED, RequestStatus.RETURNED]: - bll.set_status(release_request, current, user=users["checker"]) - bll.set_status(release_request, future, users[user]) assert_last_notification(mock_notifications, notification_event_type) @@ -873,43 +1053,45 @@ def test_notification_error(bll, notifications_stubber, caplog): assert caplog.records[-1].message == "something went wrong" -@pytest.mark.parametrize("files_approved", (True, False)) -def test_set_status_approved(files_approved, bll, mock_notifications): +@pytest.mark.parametrize("all_files_approved", (True, False)) +def test_set_status_approved(all_files_approved, bll, mock_notifications): author = factories.create_user("author", ["workspace"], False) - checker = factories.create_user("checker", [], True) - release_request = factories.create_release_request( - "workspace", user=author, status=RequestStatus.SUBMITTED - ) - factories.write_request_file( - release_request, "group", "test/file.txt", approved=files_approved + checker = factories.create_user(output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.REVIEWED, + files=[ + factories.request_file( + approved=all_files_approved, rejected=not all_files_approved + ) + ], ) - release_request = factories.refresh_release_request(release_request) - if files_approved: + if all_files_approved: bll.set_status(release_request, RequestStatus.APPROVED, user=checker) assert release_request.status == RequestStatus.APPROVED assert_last_notification(mock_notifications, "request_approved") else: - with pytest.raises((bll.InvalidStateTransition, bll.RequestPermissionDenied)): + with pytest.raises(bll.RequestPermissionDenied): bll.set_status(release_request, RequestStatus.APPROVED, user=checker) - assert_last_notification(mock_notifications, "request_updated") + assert_last_notification(mock_notifications, "request_reviewed") 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=RequestStatus.SUBMITTED + user = factories.create_user(output_checker=True) + release_request1 = factories.create_request_at_state( + "workspace", author=user, status=RequestStatus.SUBMITTED ) with pytest.raises(bll.RequestPermissionDenied): - bll.set_status(release_request1, RequestStatus.APPROVED, user=user) - with pytest.raises(bll.RequestPermissionDenied): - bll.set_status(release_request1, RequestStatus.REJECTED, user=user) + bll.set_status(release_request1, RequestStatus.PARTIALLY_REVIEWED, user=user) - release_request2 = factories.create_release_request( - "workspace", - user=user, + release_request2 = factories.create_request_at_state( + "workspace1", + author=user, status=RequestStatus.APPROVED, + files=[factories.request_file(approved=True)], ) with pytest.raises(bll.RequestPermissionDenied): @@ -917,9 +1099,9 @@ def test_set_status_cannot_action_own_request(bll): def test_set_status_approved_no_files_denied(bll): - user = factories.create_user("checker", [], True) - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED + user = factories.create_user(output_checker=True) + release_request = factories.create_request_at_state( + "workspace", status=RequestStatus.REVIEWED ) with pytest.raises(bll.RequestPermissionDenied): @@ -927,14 +1109,12 @@ def test_set_status_approved_no_files_denied(bll): def test_set_status_approved_only_supporting_file_denied(bll): - user = factories.create_user("checker", [], True) - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED - ) - factories.write_request_file( - release_request, "group", "test/file.txt", filetype=RequestFileType.SUPPORTING + user = factories.create_user(output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.REVIEWED, + files=[factories.request_file(filetype=RequestFileType.SUPPORTING)], ) - release_request = factories.refresh_release_request(release_request) with pytest.raises(bll.RequestPermissionDenied): bll.set_status(release_request, RequestStatus.APPROVED, user=user) @@ -954,74 +1134,46 @@ def test_submit_request(bll, mock_notifications): assert_last_notification(mock_notifications, "request_submitted") -def test_return_request_files_not_reviewed(bll, mock_notifications): - author = factories.create_user("author", ["workspace"], False) - checker = factories.create_user("checker", ["workspace"], True) - release_request = factories.create_release_request( - "workspace", user=author, status=RequestStatus.SUBMITTED - ) - # file approved twice - factories.write_request_file( - release_request, "group", "test/file.txt", approved=True - ) - # file rejected twice - factories.write_request_file( - release_request, "group", "test/file1.txt", rejected=True - ) - # file with one approval - factories.write_request_file(release_request, "group", "test/file2.txt") - release_request = factories.refresh_release_request(release_request) - bll.approve_file(release_request, UrlPath("test/file2.txt"), user=checker) - release_request = factories.refresh_release_request(release_request) - - with pytest.raises( - BusinessLogicLayer.RequestPermissionDenied, match="request has unreviewed files" - ): - bll.set_status(release_request, RequestStatus.RETURNED, checker) - - def test_resubmit_request(bll, mock_notifications): """ From returned Files with rejected status are moved to undecided """ author = factories.create_user("author", ["workspace"], False) - checker = factories.create_user("checker", [], True) - release_request = factories.create_release_request( - "workspace", user=author, status=RequestStatus.SUBMITTED - ) - # One file is approved by both, one is rejected - factories.write_request_file( - release_request, "group", "test/file.txt", approved=True - ) - factories.write_request_file( - release_request, "group", "test/file1.txt", rejected=True + # Returned request with two files, one is approved by both reviewers, one is rejected + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.RETURNED, + files=[ + factories.request_file(group="test", path="file.txt", approved=True), + factories.request_file(group="test", path="file1.txt", rejected=True), + ], ) - release_request = factories.refresh_release_request(release_request) - - # checker returns request - bll.set_status(release_request, RequestStatus.RETURNED, checker) + assert len(release_request.completed_reviews) == 2 # author re-submits with no changes to files bll.submit_request(release_request, author) release_request = factories.refresh_release_request(release_request) assert release_request.status == RequestStatus.SUBMITTED assert_last_notification(mock_notifications, "request_resubmitted") - for i in range(2): user = factories.create_user(f"output-checker-{i}", output_checker=True) # approved file review is still approved approved_file = release_request.get_request_file_from_output_path( - UrlPath("test/file.txt") + UrlPath("file.txt") ) assert approved_file.get_status_for_user(user) == UserFileReviewStatus.APPROVED # rejected file review is now undecided approved rejected_file = release_request.get_request_file_from_output_path( - UrlPath("test/file1.txt") + UrlPath("file1.txt") ) assert rejected_file.get_status_for_user(user) == UserFileReviewStatus.UNDECIDED + # completed reviews have been reset + assert release_request.completed_reviews == {} + def test_add_file_to_request_not_author(bll): author = factories.create_user("author", ["workspace"], False) @@ -1046,7 +1198,7 @@ def test_add_file_to_request_invalid_file_type(bll): workspace = factories.create_workspace("workspace") factories.write_workspace_file(workspace, path) release_request = factories.create_release_request( - "workspace", + workspace, user=author, ) @@ -1073,10 +1225,15 @@ def test_add_file_to_request_states( path = UrlPath("path/file.txt") workspace = factories.create_workspace("workspace") factories.write_workspace_file(workspace, path) - release_request = factories.create_release_request( - "workspace", - user=author, + + release_request = factories.create_request_at_state( + workspace, + author=author, status=status, + files=[factories.request_file(path="file.txt", approved=True)], + withdrawn_after=RequestStatus.PENDING + if status == RequestStatus.WITHDRAWN + else None, ) if success: @@ -1109,7 +1266,7 @@ def test_add_file_to_request_default_filetype(bll): workspace = factories.create_workspace("workspace") factories.write_workspace_file(workspace, path) release_request = factories.create_release_request( - "workspace", + workspace, user=author, ) bll.add_file_to_request(release_request, path, author) @@ -1131,7 +1288,7 @@ def test_add_file_to_request_with_filetype(bll, filetype, success): workspace = factories.create_workspace("workspace") factories.write_workspace_file(workspace, path) release_request = factories.create_release_request( - "workspace", + workspace, user=author, ) @@ -1146,21 +1303,21 @@ def test_add_file_to_request_with_filetype(bll, filetype, success): def test_withdraw_file_from_request_pending(bll, mock_notifications): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( + path1 = Path("path/file1.txt") + path2 = Path("path/file2.txt") + release_request = factories.create_request_at_state( "workspace", - user=author, + author=author, status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="group", path=path1, contents="1", user=author + ), + factories.request_file( + group="group", path=path2, contents="2", user=author + ), + ], ) - path1 = UrlPath("path/file1.txt") - path2 = UrlPath("path/file2.txt") - factories.write_request_file( - release_request, "group", path1, contents="1", user=author - ) - factories.write_request_file( - release_request, "group", path2, contents="2", user=author - ) - release_request = factories.refresh_release_request(release_request) - assert release_request.filegroups["group"].files.keys() == {path1, path2} bll.withdraw_file_from_request(release_request, "group" / path1, user=author) @@ -1193,20 +1350,22 @@ def test_withdraw_file_from_request_pending(bll, mock_notifications): def test_withdraw_file_from_request_submitted(bll, mock_notifications): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( + path1 = Path("path/file1.txt") + release_request = factories.create_request_at_state( "workspace", - user=author, + author=author, status=RequestStatus.SUBMITTED, + files=[ + factories.request_file( + group="group", path=path1, contents="1", user=author + ), + ], ) - path1 = UrlPath("path/file1.txt") - factories.write_request_file(release_request, "group", path1, user=author) - release_request = factories.refresh_release_request(release_request) - assert [f.filetype for f in release_request.filegroups["group"].files.values()] == [ RequestFileType.OUTPUT, ] - bll.withdraw_file_from_request(release_request, "group" / path1, user=author) + release_request = factories.refresh_release_request(release_request) assert [f.filetype for f in release_request.filegroups["group"].files.values()] == [ RequestFileType.WITHDRAWN, @@ -1225,7 +1384,7 @@ def test_withdraw_file_from_request_submitted(bll, mock_notifications): @pytest.mark.parametrize( - "state", + "status", [ RequestStatus.APPROVED, RequestStatus.REJECTED, @@ -1233,12 +1392,20 @@ def test_withdraw_file_from_request_submitted(bll, mock_notifications): RequestStatus.RELEASED, ], ) -def test_withdraw_file_from_request_not_editable_state(bll, state): +def test_withdraw_file_from_request_not_editable_state(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( + release_request = factories.create_request_at_state( "workspace", - user=author, - status=state, + author=author, + status=status, + files=[ + factories.request_file( + group="group", path="foo.txt", user=author, approved=True + ), + ], + withdrawn_after=RequestStatus.PENDING + if status == RequestStatus.WITHDRAWN + else None, ) with pytest.raises(bll.RequestPermissionDenied): @@ -1247,11 +1414,18 @@ def test_withdraw_file_from_request_not_editable_state(bll, state): ) -@pytest.mark.parametrize("state", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) -def test_withdraw_file_from_request_bad_file(bll, state): +@pytest.mark.parametrize("status", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) +def test_withdraw_file_from_request_bad_file(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( - "workspace", status=state, user=author + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=status, + files=[ + factories.request_file( + group="group", path="foo.txt", user=author, approved=True + ), + ], ) with pytest.raises(bll.FileNotFound): @@ -1260,38 +1434,47 @@ def test_withdraw_file_from_request_bad_file(bll, state): ) -@pytest.mark.parametrize("state", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) -def test_withdraw_file_from_request_not_author(bll, state): +@pytest.mark.parametrize("status", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) +def test_withdraw_file_from_request_not_author(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_release_request( - "workspace", status=state, user=author + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=status, + files=[ + factories.request_file( + group="group", path="foo.txt", user=author, approved=True + ), + ], ) other = factories.create_user(username="other", workspaces=["workspace"]) with pytest.raises(bll.RequestPermissionDenied): - bll.withdraw_file_from_request(release_request, UrlPath("bad/path"), user=other) + bll.withdraw_file_from_request( + release_request, UrlPath("group/foo.txt"), user=other + ) def test_request_all_files_by_name(bll): author = factories.create_user(username="author", workspaces=["workspace"]) - path = UrlPath("path/file.txt") - supporting_path = UrlPath("path/supporting_file.txt") - workspace = factories.create_workspace("workspace") - for fp in [path, supporting_path]: - factories.write_workspace_file(workspace, fp) - release_request = factories.create_release_request( + path = Path("path/file.txt") + supporting_path = Path("path/supporting_file.txt") + + release_request = factories.create_request_at_state( "workspace", - user=author, - ) - bll.add_file_to_request( - release_request, path, author, filetype=RequestFileType.OUTPUT - ) - bll.add_file_to_request( - release_request, supporting_path, author, filetype=RequestFileType.SUPPORTING + author=author, + status=RequestStatus.PENDING, + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], ) - release_request = factories.refresh_release_request(release_request) # all_files_by_name consists of output files and supporting files assert release_request.all_files_by_name.keys() == {path, supporting_path} @@ -1304,10 +1487,19 @@ def test_request_all_files_by_name(bll): def test_request_release_get_request_file_from_urlpath(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_release_request("id") - factories.write_request_file(release_request, "default", path) - factories.write_request_file( - release_request, "default", supporting_path, filetype=RequestFileType.SUPPORTING + + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.PENDING, + id="id", + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], ) with pytest.raises(bll.FileNotFound): @@ -1323,10 +1515,18 @@ def test_request_release_get_request_file_from_urlpath(bll): def test_request_release_abspath(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_release_request("id") - factories.write_request_file(release_request, "default", path) - factories.write_request_file( - release_request, "default", supporting_path, filetype=RequestFileType.SUPPORTING + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.PENDING, + id="id", + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], ) assert release_request.abspath("default" / path).exists() @@ -1336,10 +1536,18 @@ def test_request_release_abspath(bll): def test_request_release_request_filetype(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_release_request("id") - factories.write_request_file(release_request, "default", path) - factories.write_request_file( - release_request, "default", supporting_path, filetype=RequestFileType.SUPPORTING + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.PENDING, + id="id", + files=[ + factories.request_file( + group="default", + path=supporting_path, + filetype=RequestFileType.SUPPORTING, + ), + factories.request_file(group="default", path=path), + ], ) assert release_request.request_filetype("default" / path) == RequestFileType.OUTPUT @@ -1436,25 +1644,23 @@ def test_release_request_add_same_file(bll): assert len(release_request.filegroups["default"].files) == 1 -def _get_request_file(bll, release_request, path): +def _get_request_file(release_request, path): """Syntactic sugar to make the tests a little more readable""" # refresh - release_request = bll.get_release_request( - release_request.id, factories.create_user(output_checker=True) - ) + release_request = factories.refresh_release_request(release_request) return release_request.get_request_file_from_output_path(path) def test_approve_file_not_submitted(bll): release_request, path, author = setup_empty_release_request() - checker = factories.create_user("checker", [], True) + checker = factories.create_user(output_checker=True) bll.add_file_to_request(release_request, path, author) with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1470,7 +1676,7 @@ def test_approve_file_not_your_own(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, author) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(author) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1487,63 +1693,61 @@ def test_approve_file_not_checker(bll): with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, author2) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(author) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) - - bad_path = UrlPath("path/file2.txt") + checker = factories.create_user(output_checker=True) + bad_path = Path("path/file2.txt") with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, bad_path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path, filetype=RequestFileType.SUPPORTING)], ) + checker = factories.create_user(output_checker=True) + with pytest.raises(bll.ApprovalPermissionDenied): bll.approve_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) + checker = factories.create_user(output_checker=True) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE bll.approve_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == UserFileReviewStatus.APPROVED assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1557,43 +1761,43 @@ def test_approve_file(bll): def test_approve_file_requires_two_plus(bll): - release_request, path, author = setup_empty_release_request() + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], + ) checker1 = factories.create_user("checker1", [], True) checker2 = factories.create_user("checker2", [], True) checker3 = factories.create_user("checker3", [], True) - bll.add_file_to_request(release_request, path, author) - bll.set_status( - release_request=release_request, to_status=RequestStatus.SUBMITTED, user=author - ) - bll.approve_file(release_request, path, checker1) bll.reject_file(release_request, path, checker2) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status() == RequestFileReviewStatus.CONFLICTED bll.approve_file(release_request, path, checker3) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status() == RequestFileReviewStatus.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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) + checker = factories.create_user(output_checker=True) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE bll.reject_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == UserFileReviewStatus.REJECTED assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1607,27 +1811,27 @@ def test_reject_file(bll): 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) + checker = factories.create_user(output_checker=True) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE bll.approve_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == UserFileReviewStatus.APPROVED assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE bll.reject_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == UserFileReviewStatus.REJECTED assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1636,15 +1840,15 @@ def test_approve_then_reject_file(bll): "review", [UserFileReviewStatus.APPROVED, UserFileReviewStatus.REJECTED] ) def test_reviewreset_then_reset_review_file(bll, review): - 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) + checker = factories.create_user(output_checker=True) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1655,34 +1859,34 @@ def test_reviewreset_then_reset_review_file(bll, review): else: assert False - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == review assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE bll.reset_review_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE def test_reset_review_file_no_reviews(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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) + checker = factories.create_user(output_checker=True) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE with pytest.raises(bll.FileReviewNotFound): bll.reset_review_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) is None assert rfile.get_status() == RequestFileReviewStatus.INCOMPLETE @@ -1696,11 +1900,11 @@ def test_reset_review_file_no_reviews(bll): ], ) def test_request_file_status_approved(bll, reviews, final_review): - 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 + path = Path("path/file1.txt") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[factories.request_file(path=path)], ) for i, review in enumerate(reviews): @@ -1711,7 +1915,7 @@ def test_request_file_status_approved(bll, reviews, final_review): else: bll.reject_file(release_request, path, checker) - rfile = _get_request_file(bll, release_request, path) + rfile = _get_request_file(release_request, path) assert rfile.get_status_for_user(checker) == UserFileReviewStatus[review] if i == 0: @@ -1722,98 +1926,160 @@ def test_request_file_status_approved(bll, reviews, final_review): def test_mark_file_undecided(bll): # Set up submitted request - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED - ) - - # Write a request file that already has 2 rejected reviewsso that we can - # set the request to RETURNED - path = Path("path/file.txt") - factories.write_request_file( - release_request, - "default", - path, - rejected=True, + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.RETURNED, + files=[factories.request_file(path="file.txt", rejected=True)], ) - # setup the initial file status for our test checker - checker = factories.create_user("checker", [], True) - bll.reject_file(release_request, path, checker) - release_request = factories.refresh_release_request(release_request) - - # set the request to returned - bll.set_status( - release_request=release_request, to_status=RequestStatus.RETURNED, user=checker - ) + # first default output-checker + checker = factories.create_user("output-checker-0") # mark file review as undecided - review = release_request.get_request_file_from_output_path(path).reviews[ + review = release_request.get_request_file_from_output_path("file.txt").reviews[ checker.username ] - bll.mark_file_undecided(release_request, review, path, checker) + bll.mark_file_undecided(release_request, review, "file.txt", user=checker) release_request = factories.refresh_release_request(release_request) - review = release_request.get_request_file_from_output_path(path).reviews[ + review = release_request.get_request_file_from_output_path("file.txt").reviews[ checker.username ] assert review.status == UserFileReviewStatus.UNDECIDED @pytest.mark.parametrize( - "request_status,file_status", + "request_status,file_status,allowed", [ # can only mark undecided for a rejected file on a returned request - (RequestStatus.SUBMITTED, UserFileReviewStatus.REJECTED), - (RequestStatus.APPROVED, UserFileReviewStatus.REJECTED), - (RequestStatus.RELEASED, UserFileReviewStatus.REJECTED), - (RequestStatus.RETURNED, UserFileReviewStatus.APPROVED), + (RequestStatus.SUBMITTED, UserFileReviewStatus.REJECTED, False), + (RequestStatus.RETURNED, UserFileReviewStatus.APPROVED, False), + (RequestStatus.RETURNED, UserFileReviewStatus.REJECTED, True), ], ) -def test_mark_file_undecided_permission_errors(bll, request_status, file_status): - # Set up submitted request - release_request = factories.create_release_request( - "workspace", status=RequestStatus.SUBMITTED - ) - - # Write a request file that already has 2 reviews; these are both rejected for +def test_mark_file_undecided_permission_errors( + bll, request_status, file_status, allowed +): + # Set up that already has 2 reviews; these are both rejected for # requests that we want to be in RETURNED status, and approved # for SUBMITTED/APPROVED/RELEASED, so we can set the request status - path = Path("path/file.txt") - factories.write_request_file( - release_request, - "default", - path, - rejected=file_status == RequestStatus.RETURNED, - approved=file_status != RequestStatus.RETURNED, + path = "path/file.txt" + checkers = factories.get_default_output_checkers() + release_request = factories.create_request_at_state( + "workspace", + status=request_status, + files=[ + factories.request_file( + path=path, + rejected=file_status == UserFileReviewStatus.REJECTED, + approved=file_status == UserFileReviewStatus.APPROVED, + checkers=checkers, + ) + ], ) - # setup the initial file status for our test checker - checker = factories.create_user("checker", [], True) - if file_status == UserFileReviewStatus.APPROVED: - bll.approve_file(release_request, path, checker) + review = release_request.get_request_file_from_output_path(path).reviews[ + checkers[0].username + ] + assert review.status == file_status + if allowed: + bll.mark_file_undecided(release_request, review, path, checkers[0]) else: - bll.reject_file(release_request, path, checker) + with pytest.raises(bll.ApprovalPermissionDenied): + bll.mark_file_undecided(release_request, review, path, checkers[0]) + + +def test_review_request(bll): + checker = factories.create_user("checker", output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(path="test.txt", rejected=True, checkers=[checker]), + factories.request_file(path="test1.txt"), + ], + ) + # first file is already rejected, second file is not reviewed + with pytest.raises( + bll.RequestReviewDenied, + match="You must review all files to complete your review", + ): + bll.review_request(release_request, checker) + assert "checker" not in release_request.completed_reviews + assert release_request.status == RequestStatus.SUBMITTED + + # approved second file + factories.review_file( + release_request, "test1.txt", UserFileReviewStatus.APPROVED, checker + ) release_request = factories.refresh_release_request(release_request) + bll.review_request(release_request, checker) + release_request = factories.refresh_release_request(release_request) + assert "checker" in release_request.completed_reviews + assert release_request.status == RequestStatus.PARTIALLY_REVIEWED - # set the request status - # For released, first move the initially submitted request to approved - if request_status == RequestStatus.RELEASED: - bll.set_status( - release_request=release_request, - to_status=RequestStatus.APPROVED, - user=checker, - ) - # If necessary, move the initally submitted request to the final state for testing - if request_status != RequestStatus.SUBMITTED: - bll.set_status( - release_request=release_request, to_status=request_status, user=checker - ) + # re-review + with pytest.raises( + bll.RequestReviewDenied, match="You have already completed your review" + ): + bll.review_request(release_request, checker) - review = release_request.get_request_file_from_output_path(path).reviews[ - checker.username - ] - assert review.status == file_status - with pytest.raises(bll.ApprovalPermissionDenied): - bll.mark_file_undecided(release_request, review, path, checker) + +def test_review_request_non_submitted_status(bll): + checker = factories.create_user(output_checker=True) + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.WITHDRAWN, + withdrawn_after=RequestStatus.PENDING, + files=[ + factories.request_file( + path="test.txt", + checkers=[checker, factories.create_user(output_checker=True)], + approved=True, + ), + ], + ) + with pytest.raises( + bll.RequestPermissionDenied, match="Cannot review request in state WITHDRAWN" + ): + bll.review_request(release_request, checker) + + +def test_review_request_non_output_checker(bll): + user = factories.create_user("non-output-checker") + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(path="test.txt", approved=True), + ], + ) + with pytest.raises( + bll.RequestPermissionDenied, match="Only an output checker can review a request" + ): + bll.review_request(release_request, user) + + +def test_review_request_more_than_2_checkers(bll): + checkers = [factories.create_user(f"checker_{i}", [], True) for i in range(3)] + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(path="test.txt", approved=True, checkers=checkers), + ], + ) + bll.review_request(release_request, checkers[0]) + release_request = factories.refresh_release_request(release_request) + assert release_request.status == RequestStatus.PARTIALLY_REVIEWED + + bll.review_request(release_request, checkers[1]) + release_request = factories.refresh_release_request(release_request) + assert release_request.status == RequestStatus.REVIEWED + + bll.review_request(release_request, checkers[2]) + release_request = factories.refresh_release_request(release_request) + assert release_request.status == RequestStatus.REVIEWED + assert len(release_request.completed_reviews) == 3 # add DAL method names to this if they do not require auditing @@ -1826,6 +2092,7 @@ def test_mark_file_undecided_permission_errors(bll, request_status, file_status) "get_requests_authored_by_user", "get_approved_requests", "delete_file_from_request", + "record_review", } @@ -1913,14 +2180,12 @@ def test_group_edit_notifications( # Set the output checking org and repo to override any local settings settings.AIRLOCK_OUTPUT_CHECKING_ORG = settings.AIRLOCK_OUTPUT_CHECKING_REPO = None author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) - bll.set_status(release_request, RequestStatus.SUBMITTED, author) # group is always created with no context/controls initially assert release_request.filegroups["group"].context == "" @@ -1957,14 +2222,12 @@ def test_notifications_org_repo( settings.AIRLOCK_OUTPUT_CHECKING_ORG = org settings.AIRLOCK_OUTPUT_CHECKING_REPO = repo author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) - bll.set_status(release_request, RequestStatus.SUBMITTED, author) # notifications endpoint called when request submitted notification_responses = parse_notification_responses(mock_notifications) @@ -1986,11 +2249,11 @@ def test_notifications_org_repo( def test_group_edit_not_author(bll): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) with pytest.raises(bll.RequestPermissionDenied): @@ -1998,12 +2261,16 @@ def test_group_edit_not_author(bll): @pytest.mark.parametrize( - "state", [RequestStatus.APPROVED, RequestStatus.REJECTED, RequestStatus.WITHDRAWN] + "status", [RequestStatus.APPROVED, RequestStatus.REJECTED, RequestStatus.WITHDRAWN] ) -def test_group_edit_not_editable(bll, state): +def test_group_edit_not_editable(bll, status): author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_release_request( - "workspace", user=author, status=state + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=status, + files=[factories.request_file(approved=True)], + withdrawn_after=RequestStatus.PENDING, ) with pytest.raises(bll.RequestPermissionDenied): @@ -2012,11 +2279,11 @@ def test_group_edit_not_editable(bll, state): def test_group_edit_bad_group(bll): author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) with pytest.raises(bll.FileNotFound): @@ -2028,7 +2295,7 @@ def test_group_edit_bad_group(bll): [ (RequestStatus.PENDING, 0), # Currently no notifications are sent for comments. The only notification - # sent in this test is for adding a file to the submitted request + # sent in this test is for summitting request (RequestStatus.SUBMITTED, 1), ], ) @@ -2037,29 +2304,26 @@ def test_group_comment_create_success( ): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - release_request = factories.create_release_request( - "workspace", user=author, status=status - ) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=status, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) assert release_request.filegroups["group"].comments == [] bll.group_comment_create(release_request, "group", "question?", other) bll.group_comment_create(release_request, "group", "answer!", author) + release_request = factories.refresh_release_request(release_request) notification_responses = parse_notification_responses(mock_notifications) assert notification_responses["count"] == notification_count if notification_count > 0: - file_added = notification_responses["request_json"][0] - assert file_added["event_type"] == "request_updated" - assert file_added["updates"][0]["update_type"] == "file added" - - release_request = factories.refresh_release_request(release_request) + assert ( + notification_responses["request_json"][0]["event_type"] + == "request_submitted" + ) assert release_request.filegroups["group"].comments[0].comment == "question?" assert release_request.filegroups["group"].comments[0].author == "other" @@ -2082,17 +2346,16 @@ def test_group_comment_create_success( def test_group_comment_create_permissions(bll): author = factories.create_user("author", ["workspace"], False) - collaborator = factories.create_user("collaboratorr", ["workspace"], False) + collaborator = factories.create_user("collaborator", ["workspace"], False) other = factories.create_user("other", ["other"], False) checker = factories.create_user("checker", ["other"], True) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.PENDING, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) assert len(release_request.filegroups["group"].comments) == 0 @@ -2113,16 +2376,12 @@ def test_group_comment_create_permissions(bll): def test_group_comment_delete_success(bll): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - status = RequestStatus.SUBMITTED - release_request = factories.create_release_request( - "workspace", user=author, status=status - ) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) assert release_request.filegroups["group"].comments == [] @@ -2172,13 +2431,12 @@ def test_group_comment_delete_permissions(bll): collaborator = factories.create_user("collaborator", ["workspace"], False) other = factories.create_user("other", ["other"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) bll.group_comment_create(release_request, "group", "author comment", author) release_request = factories.refresh_release_request(release_request) @@ -2201,13 +2459,12 @@ def test_group_comment_create_invalid_params(bll): author = factories.create_user("author", ["workspace"], False) collaborator = factories.create_user("collaborator", ["workspace"], False) - release_request = factories.create_release_request("workspace", user=author) - factories.write_request_file( - release_request, - "group", - "test/file.txt", + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.SUBMITTED, + files=[factories.request_file("group", "test/file.txt")], ) - release_request = factories.refresh_release_request(release_request) with pytest.raises(bll.APIException): bll.group_comment_delete(release_request, "group", 1, author) From ca5ffee4399072b889008e589ea43c7f7fefe772 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 18:14:26 +0100 Subject: [PATCH 13/22] Add permission checks for review_request --- airlock/business_logic.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 7ea1aa28..ef518b22 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -1749,6 +1749,19 @@ def review_request(self, release_request: ReleaseRequest, user: User): Marking the request as either PARTIALLY_REVIEWED or REVIEWED, depending on whether this is the first or second review. """ + if self.STATUS_OWNERS[release_request.status] != RequestStatusOwner.REVIEWER: + raise self.RequestPermissionDenied( + f"Cannot review request in state {release_request.status.name}" + ) + + if user.username == release_request.author: + raise self.RequestPermissionDenied("You cannot review your own request") + + if not user.output_checker: + raise self.RequestPermissionDenied( + "Only an output checker can review a request" + ) + if not release_request.all_files_reviewed_by_reviewer(user): raise self.RequestReviewDenied( "You must review all files to complete your review" From 8be977883a16afd85dcbfe108c0e56496ab3c259 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Tue, 25 Jun 2024 18:15:16 +0100 Subject: [PATCH 14/22] Remove some unused methods and status checks --- airlock/business_logic.py | 37 +------------------------------------ 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index ef518b22..9650c08b 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -645,22 +645,6 @@ def rejected_reviews(self): if review.status == UserFileReviewStatus.REJECTED ] - def reviewed(self): - """ - A file is reviewed if it has been approved OR rejected by two reviewers - """ - return ( - len( - [ - review - for review in self.reviews.values() - if review.status - in [UserFileReviewStatus.APPROVED, UserFileReviewStatus.REJECTED] - ] - ) - >= 2 - ) - @dataclass(frozen=True) class FileGroup: @@ -901,13 +885,6 @@ def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: for rfile in self.output_files().values() ) - def all_files_reviewed(self): - return all( - request_file.reviewed() - for filegroup in self.filegroups.values() - for request_file in filegroup.output_files - ) - # helpers for using in template logic def status_owner(self) -> RequestStatusOwner: return BusinessLogicLayer.STATUS_OWNERS[self.status] @@ -1384,7 +1361,7 @@ def check_status( if to_status not in valid_transitions: raise self.InvalidStateTransition( - f"from {release_request.status.name} to {to_status.name}" + f"cannot change status from {release_request.status.name} to {to_status.name}" ) # check permissions @@ -1431,18 +1408,6 @@ def check_status( f"Cannot set status to {to_status.name}; request contains no output files." ) - if ( - to_status == RequestStatus.RETURNED - and not release_request.all_files_reviewed() - ): - raise self.RequestPermissionDenied( - f"Cannot set status to {to_status.name}; request has unreviewed files." - ) - elif owner == RequestStatusOwner.SYSTEM: - raise self.RequestPermissionDenied( - f"only the system can set status to {to_status.name}" - ) - def set_status( self, release_request: ReleaseRequest, to_status: RequestStatus, user: User ): From 3fd242cc76ced4f5335f69cfb3dca2f85bc897a8 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Wed, 26 Jun 2024 16:44:31 +0100 Subject: [PATCH 15/22] Disable complete button until ready, show messages A review can only be completed once all files have been reviewed; hide the Complete review button until then. Also show a reminder message if a user has reviewed all files but has not pressed the button to complete their review yet. --- airlock/templates/file_browser/index.html | 7 ++++++- airlock/views/request.py | 16 ++++++++++++++-- tests/functional/test_e2e.py | 13 +++++++++++++ tests/integration/views/test_request.py | 14 +++++++++++++- 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index 3466679b..1ece814c 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -80,11 +80,16 @@ Complete review {% tooltip class="airlock-tooltip" content="You have already completed your review" %} {% /button %} - {% else %} + {% elif user_has_reviewed_all_files %}
{% csrf_token %} {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="complete-review-button" %}Complete review{% /button %}
+ {% else %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} + Complete review + {% tooltip class="airlock-tooltip" content="You must review all files before you can complete your review" %} + {% /button %} {% endif %} {% endif %} {% /airlock_header %} diff --git a/airlock/views/request.py b/airlock/views/request.py index 02713b91..654ab5e3 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -157,6 +157,18 @@ def request_view(request, request_id: str, path: str = ""): request=release_request.id, group=group, exclude_readonly=True, size=20 ) + user_has_completed_review = ( + request.user.username in release_request.completed_reviews + ) + user_has_reviewed_all_files = release_request.all_files_reviewed_by_reviewer( + request.user + ) + + if user_has_reviewed_all_files and not user_has_completed_review: + messages.success( + request, "All files reviewed. Your review can now be completed." + ) + request_submit_url = reverse( "request_submit", kwargs={"request_id": request_id}, @@ -236,8 +248,8 @@ def request_view(request, request_id: str, path: str = ""): "request_return_url": request_return_url, "request_withdraw_url": request_withdraw_url, "release_files_url": release_files_url, - "user_has_completed_review": request.user.username - in release_request.completed_reviews, + "user_has_completed_review": user_has_completed_review, + "user_has_reviewed_all_files": user_has_reviewed_all_files, "activity": activity, "group_edit_form": group_edit_form, "group_edit_url": group_edit_url, diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index 3b7629f2..d7f8e4a5 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -341,6 +341,11 @@ def test_e2e_release_files( find_and_click(page.locator("#outstanding-requests").get_by_role("link")) expect(page.locator("body")).to_contain_text(request_id) + complete_review_button = page.locator("#complete-review-button") + # output checker hasn't reviewed files yet, complete review button visible but disabled + expect(complete_review_button).to_be_visible() + expect(complete_review_button).to_be_disabled() + # Reuse the locators from the workspace view to click on filegroup and then file # Click to open the filegroup tree find_and_click(filegroup_link) @@ -360,6 +365,10 @@ def test_e2e_release_files( find_and_click(page.locator("#file-reject-button")) expect(page.locator("#file-reset-button")).to_have_attribute("aria-pressed", "true") + # output checker has now reviewed all output files + expect(complete_review_button).to_be_visible() + expect(complete_review_button).to_be_enabled() + # Change our minds & remove the review expect(page.locator("#file-reset-button")).to_have_attribute("aria-pressed", "true") find_and_click(page.locator("#file-reset-button")) @@ -367,6 +376,10 @@ def test_e2e_release_files( "aria-pressed", "false" ) + # complete review button disabled again + expect(complete_review_button).to_be_visible() + expect(complete_review_button).to_be_disabled() + # Think some more & finally approve the file expect(page.locator("#file-approve-button")).to_have_attribute( "aria-pressed", "false" diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index e99b44ac..6f46bdb2 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -674,6 +674,11 @@ def test_request_review_output_checker(airlock_client): status=RequestStatus.SUBMITTED, files=[factories.request_file(approved=True, checkers=[airlock_client.user])], ) + + response = airlock_client.get(release_request.get_url()) + # Files have been reviewed but review has not been completed yet + assert "All files reviewed" in list(response.context["messages"])[0].message + response = airlock_client.post( f"/requests/review/{release_request.id}", follow=True ) @@ -681,7 +686,14 @@ def test_request_review_output_checker(airlock_client): assert response.status_code == 200 persisted_request = factories.refresh_release_request(release_request) assert persisted_request.status == RequestStatus.PARTIALLY_REVIEWED - assert "Your review has been completed" in response.rendered_content + assert ( + "Your review has been completed" + in list(response.context["messages"])[0].message + ) + + response = airlock_client.get(release_request.get_url()) + # Reminder message no longer shown now that review is complete + assert list(response.context["messages"]) == [] def test_request_review_non_output_checker(airlock_client): From dade2d0d68b1d8fc2a04f03ea47f7467a8d2210e Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 10:50:20 +0100 Subject: [PATCH 16/22] Refactor returned functional tests These have been a bit flakey, and don't need to be doing so many things. Refactored to just test whether the return button is visible/enabeld in various states, and file status after re-submission. --- tests/functional/test_request_pages.py | 158 +++++++++++++------------ 1 file changed, 83 insertions(+), 75 deletions(-) diff --git a/tests/functional/test_request_pages.py b/tests/functional/test_request_pages.py index 51cb50ff..5792b263 100644 --- a/tests/functional/test_request_pages.py +++ b/tests/functional/test_request_pages.py @@ -1,5 +1,6 @@ import re +import pytest from playwright.sync_api import expect from airlock.business_logic import RequestStatus @@ -101,100 +102,107 @@ def test_request_group_edit_comment(live_server, context, page, bll, settings): expect(comments_locator).to_contain_text("test comment") -def test_request_return(live_server, context, page, bll): - author = login_as_user( - live_server, - context, - user_dict={ - "username": "author", - "workspaces": ["workspace"], - "output_checker": False, - }, - ) - +@pytest.mark.parametrize( + "login_as,status,checkers, can_return", + [ + ("author", RequestStatus.SUBMITTED, None, False), + ("checker1", RequestStatus.PARTIALLY_REVIEWED, ["checker1"], False), + ("checker2", RequestStatus.PARTIALLY_REVIEWED, ["checker1"], False), + ("checker1", RequestStatus.REVIEWED, ["checker1", "checker2"], True), + ], +) +def test_request_returnable( + live_server, context, page, bll, login_as, status, checkers, can_return +): + user_data = { + "author": dict( + username="author", workspaces=["workspace"], output_checker=False + ), + "checker1": dict( + username="checker1", workspaces=["workspace"], output_checker=True + ), + "checker2": dict( + username="checker2", workspaces=["workspace"], output_checker=True + ), + } + author = factories.create_user(**user_data["author"]) + if checkers is not None: + checkers = [factories.create_user(**user_data[user]) for user in checkers] release_request = factories.create_request_at_state( "workspace", author=author, - status=RequestStatus.SUBMITTED, + status=status, files=[ - factories.request_file(group="group", path="file1.txt"), - factories.request_file(group="group", path="file2.txt"), + factories.request_file( + group="group", + path="file1.txt", + checkers=checkers, + approved=checkers is not None, + ), + factories.request_file( + group="group", + path="file2.txt", + checkers=checkers, + rejected=checkers is not None, + ), ], ) + login_as_user(live_server, context, user_data[login_as]) return_request_button = page.locator("#return-request-button") page.goto(live_server.url + release_request.get_url()) - def _logout(): - # logout by clearing cookies - context.clear_cookies() - - def _review_files(username): - # logout current user, login as username - _logout() - login_as_user( - live_server, - context, - user_dict={ - "username": username, - "workspaces": [], - "output_checker": True, - }, - ) - page.goto(live_server.url + release_request.get_url()) + if can_return: + expect(return_request_button).to_be_enabled() + return_request_button.click() + expect(return_request_button).to_be_disabled() + elif login_as == "author": + expect(return_request_button).not_to_be_visible() + else: expect(return_request_button).to_be_disabled() - page.goto(live_server.url + release_request.get_url("group/file1.txt")) - page.locator("#file-approve-button").click() - - page.goto(live_server.url + release_request.get_url("group/file2.txt")) - page.locator("#file-reject-button").click() - - # mark review as completed - page.locator("#complete-review-button").click() - - # First output-checker reviews files - _review_files("output-checker-1") - - # Return button is still disabled - expect(return_request_button).to_be_disabled() - - # Second output-checker reviews files - _review_files("output-checker-2") - - # Return button is now enabled - expect(return_request_button).to_be_enabled() - - # Return the request - return_request_button.click() - # logout, login as author again - _logout() - login_as_user( - live_server, - context, - user_dict={ - "username": "author", - "workspaces": ["workspace"], - "output_checker": False, - }, +def test_returned_request(live_server, context, page, bll): + user_data = { + "author": dict( + username="author", workspaces=["workspace"], output_checker=False + ), + "checker1": dict( + username="checker1", workspaces=["workspace"], output_checker=True + ), + "checker2": dict( + username="checker2", workspaces=["workspace"], output_checker=True + ), + } + author = factories.create_user(**user_data["author"]) + checkers = [ + factories.create_user(**user_data[user]) for user in ["checker1", "checker2"] + ] + release_request = factories.create_request_at_state( + "workspace", + author=author, + status=RequestStatus.RETURNED, + files=[ + factories.request_file( + group="group", path="file1.txt", checkers=checkers, approved=True + ), + factories.request_file( + group="group", path="file2.txt", checkers=checkers, rejected=True + ), + ], ) + + # author resubmits + login_as_user(live_server, context, user_data["author"]) page.goto(live_server.url + release_request.get_url()) # Can re-submit a returned request page.locator("#submit-for-review-button").click() - # logout, login as first output-checker - _logout() - login_as_user( - live_server, - context, - user_dict={ - "username": "output-checker-1", - "workspaces": [], - "output_checker": True, - }, - ) + # logout by clearing cookies + context.clear_cookies() + # checker looks at previously rejected/approved files + login_as_user(live_server, context, user_data["checker1"]) status_locator = page.locator(".file-status") # go to previously approved file; still shown as approved page.goto(live_server.url + release_request.get_url("group/file1.txt")) From b93e41676d79f51e40a8bf2d030c3ea38f560a9f Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 13:36:31 +0100 Subject: [PATCH 17/22] Fix template logic for request buttons And add some more functional tests to test it. --- airlock/business_logic.py | 6 ++ airlock/templates/file_browser/index.html | 82 +++++++++++------------ tests/functional/test_request_pages.py | 74 +++++++++++++++++++- tests/integration/views/test_request.py | 21 ------ 4 files changed, 120 insertions(+), 63 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 9650c08b..58cae73f 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -889,6 +889,12 @@ def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: def status_owner(self) -> RequestStatusOwner: return BusinessLogicLayer.STATUS_OWNERS[self.status] + def can_be_released(self) -> bool: + return ( + self.status in [RequestStatus.REVIEWED, RequestStatus.APPROVED] + and self.all_files_approved() + ) + def is_final(self): return ( BusinessLogicLayer.STATUS_OWNERS[self.status] == RequestStatusOwner.SYSTEM diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index 1ece814c..c7b00bf3 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -37,29 +37,47 @@ {% /modal %} {% endif %} {% elif is_output_checker %} - {% comment %} A fully reviewed request can be returned or rejected {% endcomment %} - {% if release_request.status.name == "REVIEWED" %} -
- {% csrf_token %} - {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %} -
-
- {% csrf_token %} - {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %} -
- {% else %} - {% #button disabled=True class="relative group btn-sm" variant="danger" id="reject-request-button" %} - Reject request - {% tooltip class="airlock-tooltip" content="Rejecting a request is disabled until review has been completed by two reviewers" %} - {% /button %} - {% #button disabled=True class="relative group btn-sm" variant="secondary" id="return-request-button" %} - Return request - {% tooltip class="airlock-tooltip" content="Returning a request is disabled until review has been completed by two reviewers" %} - {% /button %} + {% if release_request.status_owner.name == "REVIEWER" %} + {% comment %} A fully reviewed request can be returned or rejected {% endcomment %} + {% if release_request.status.name == "REVIEWED" %} +
+ {% csrf_token %} + {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %} +
+
+ {% csrf_token %} + {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %} +
+ {% else %} + {% #button disabled=True class="relative group btn-sm" variant="danger" id="reject-request-button" %} + Reject request + {% tooltip class="airlock-tooltip" content="Rejecting a request is disabled until review has been completed by two reviewers" %} + {% /button %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="return-request-button" %} + Return request + {% tooltip class="airlock-tooltip" content="Returning a request is disabled until review has been completed by two reviewers" %} + {% /button %} + {% endif %} + {% comment %} User can complete review if they haven't already {% endcomment %} + {% if user_has_completed_review %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} + Complete review + {% tooltip class="airlock-tooltip" content="You have already completed your review" %} + {% /button %} + {% elif user_has_reviewed_all_files %} +
+ {% csrf_token %} + {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="complete-review-button" %}Complete review{% /button %} +
+ {% else %} + {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} + Complete review + {% tooltip class="airlock-tooltip" content="You must review all files before you can complete your review" %} + {% /button %} + {% endif %} {% endif %} - {% comment %} A fully reviewed or approved request can be released if all its files are also approved {% endcomment %} - {% if release_request.status.name in "REVIEWED,APPROVED" and release_request.all_files_approved %} + {% if release_request.can_be_released %}
{% csrf_token %} {% #button class="btn-sm" type="submit" tooltip="Release files to jobs.opensafely.org" variant="warning" id="release-files-button" %} @@ -67,28 +85,10 @@ {% /button %}
- {% else %} + {% elif release_request.status_owner.name == "REVIEWER" %} {% #button disabled=True class="relative group btn-sm" variant="warning" id="release-files-button" %} Release files - {% tooltip class="airlock-tooltip" content="Releasing to jobs.opensafely.org is disabled until review has been completed by by two reviewers" %} - {% /button %} - {% endif %} - - {% comment %} User can complete review if they haven't already {% endcomment %} - {% if user_has_completed_review %} - {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} - Complete review - {% tooltip class="airlock-tooltip" content="You have already completed your review" %} - {% /button %} - {% elif user_has_reviewed_all_files %} -
- {% csrf_token %} - {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="complete-review-button" %}Complete review{% /button %} -
- {% else %} - {% #button disabled=True class="relative group btn-sm" variant="secondary" id="complete-review-button" %} - Complete review - {% tooltip class="airlock-tooltip" content="You must review all files before you can complete your review" %} + {% tooltip class="airlock-tooltip" content="Releasing to jobs.opensafely.org is disabled until all files have been approved by by two reviewers" %} {% /button %} {% endif %} {% endif %} diff --git a/tests/functional/test_request_pages.py b/tests/functional/test_request_pages.py index 5792b263..1cbd5078 100644 --- a/tests/functional/test_request_pages.py +++ b/tests/functional/test_request_pages.py @@ -102,6 +102,78 @@ def test_request_group_edit_comment(live_server, context, page, bll, settings): expect(comments_locator).to_contain_text("test comment") +@pytest.mark.parametrize( + "author,login_as,status,reviewer_buttons_visible,release_button_visible", + [ + ("researcher", "researcher", RequestStatus.SUBMITTED, False, False), + ("researcher", "checker", RequestStatus.SUBMITTED, True, True), + ("checker", "checker", RequestStatus.SUBMITTED, False, False), + ("researcher", "checker", RequestStatus.PARTIALLY_REVIEWED, True, True), + ("checker", "checker", RequestStatus.PARTIALLY_REVIEWED, False, False), + ("researcher", "checker", RequestStatus.REVIEWED, True, True), + # APPROVED status - can be released, but other review buttons are hidden + ("researcher", "checker", RequestStatus.APPROVED, False, True), + ("researcher", "checker", RequestStatus.RELEASED, False, False), + ("researcher", "checker", RequestStatus.REJECTED, False, False), + ("researcher", "checker", RequestStatus.WITHDRAWN, False, False), + ], +) +def test_request_buttons( + live_server, + context, + page, + bll, + author, + login_as, + status, + reviewer_buttons_visible, + release_button_visible, +): + user_data = { + "researcher": dict( + username="researcher", workspaces=["workspace"], output_checker=False + ), + "checker": dict( + username="checker", workspaces=["workspace"], output_checker=True + ), + } + + release_request = factories.create_request_at_state( + "workspace", + author=factories.create_user(**user_data[author]), + status=status, + withdrawn_after=RequestStatus.PENDING, + files=[ + factories.request_file( + group="group", + path="file1.txt", + approved=True, + ), + ], + ) + + login_as_user(live_server, context, user_data[login_as]) + page.goto(live_server.url + release_request.get_url()) + + reviewer_buttons = [ + "#return-request-button", + "#reject-request-button", + "#complete-review-button", + ] + + if reviewer_buttons_visible: + for button_id in reviewer_buttons: + expect(page.locator(button_id)).to_be_visible() + else: + for button_id in reviewer_buttons: + expect(page.locator(button_id)).not_to_be_visible() + + if release_button_visible: + expect(page.locator("#release-files-button")).to_be_visible() + else: + expect(page.locator("#release-files-button")).not_to_be_visible() + + @pytest.mark.parametrize( "login_as,status,checkers, can_return", [ @@ -250,4 +322,4 @@ def test_request_releaseable(live_server, context, page, bll): # output checker cannot return or reject expect(release_files_button).to_be_enabled() for locator in [return_request_button, reject_request_button]: - expect(locator).not_to_be_enabled() + expect(locator).not_to_be_visible() diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 6f46bdb2..26e32a19 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -181,27 +181,6 @@ def test_request_view_with_reviewed_request(airlock_client): assert "You have already completed your review" in response.rendered_content -def test_request_view_with_withdrawn_request(airlock_client): - # Login as 1st default output-checker - airlock_client.login("output-checker-0", output_checker=True) - release_request = factories.create_request_at_state( - "workspace", - status=RequestStatus.WITHDRAWN, - withdrawn_after=RequestStatus.RETURNED, - ) - response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) - - expected_buttons = [ - ("Reject request", "Rejecting a request is disabled"), - ("Release files", "Releasing to jobs.opensafely.org is disabled"), - ("Return request", "Returning a request is disabled"), - ] - - for button_text, diabled_tooltip in expected_buttons: - assert button_text in response.rendered_content - assert diabled_tooltip in response.rendered_content - - def test_request_view_with_authored_request_file(airlock_client): airlock_client.login(output_checker=True) release_request = factories.create_request_at_state( From 160357eb0127413651b396040b2f0e4229932012 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 15:52:15 +0100 Subject: [PATCH 18/22] Handle potential race condition in review_request --- airlock/business_logic.py | 32 ++++++++++++++--- tests/unit/test_business_logic.py | 58 +++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 5 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 58cae73f..f58f25f4 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -885,6 +885,9 @@ def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: for rfile in self.output_files().values() ) + def completed_reviews_count(self): + return len(self.completed_reviews) + # helpers for using in template logic def status_owner(self) -> RequestStatusOwner: return BusinessLogicLayer.STATUS_OWNERS[self.status] @@ -1746,15 +1749,34 @@ def review_request(self, release_request: ReleaseRequest, user: User): self._dal.record_review(release_request.id, user.username) release_request = self.get_release_request(release_request.id, user) - n_reviews = len(release_request.completed_reviews) + n_reviews = release_request.completed_reviews_count() # this method is called twice, by different users. It advances the # state differently depending on whether its the 1st or 2nd review to # be completed. - if n_reviews == 1: - self.set_status(release_request, RequestStatus.PARTIALLY_REVIEWED, user) - elif n_reviews == 2: - self.set_status(release_request, RequestStatus.REVIEWED, user) + try: + if n_reviews == 1: + self.set_status(release_request, RequestStatus.PARTIALLY_REVIEWED, user) + elif n_reviews == 2: + self.set_status(release_request, RequestStatus.REVIEWED, user) + except self.InvalidStateTransition: + # There is a potential race condition where two reviewers hit the Complete Review + # button at the same time, and both attempt to transition from SUBMITTED to + # PARTIALLY_REVIEWED, or from PARTIALLY_REVIEWED to REVIEWED + # Assuming that the request status is now either PARTIALLY_REVIEWED or REVIEWED, + # we can verify the status by refreshing the request, getting the number of reviews + # again, and advance it if necessary + if release_request.status not in [ + RequestStatus.PARTIALLY_REVIEWED, + RequestStatus.REVIEWED, + ]: + raise + release_request = self.get_release_request(release_request.id, user) + if ( + release_request.completed_reviews_count() > 1 + and release_request.status == RequestStatus.PARTIALLY_REVIEWED + ): + self.set_status(release_request, RequestStatus.REVIEWED, user) def mark_file_undecided( self, diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 0aba1193..99770711 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -2082,6 +2082,64 @@ def test_review_request_more_than_2_checkers(bll): assert len(release_request.completed_reviews) == 3 +def test_review_request_race_condition(bll): + """ + In a potential race condition, a + """ + checkers = [ + factories.create_user("checker", output_checker=True), + factories.create_user("checker1", output_checker=True), + factories.create_user("checker2", output_checker=True), + factories.create_user("checker3", output_checker=True), + ] + release_request = factories.create_request_at_state( + "workspace", + status=RequestStatus.SUBMITTED, + files=[ + factories.request_file(path="test.txt", approved=True, checkers=checkers), + factories.request_file(path="test1.txt", rejected=True, checkers=checkers), + ], + ) + # first checker completes review + bll.review_request(release_request, checkers[0]) + + # mock race condition by patching the number of completed reviews to return 1 initially + # This is called AFTER recording the review. If it's a first review, we expect there to + # be 1 completed reivew, if it's a second review we expect there to be 2. + # However in a race condition, this could be review 2, but the count is incorrectly + # retrieved as 1 + with patch( + "airlock.business_logic.ReleaseRequest.completed_reviews_count" + ) as completed_reviews: + completed_reviews.side_effect = [1, 2] + bll.review_request(release_request, checkers[1]) + + release_request = factories.refresh_release_request(release_request) + assert release_request.status == RequestStatus.REVIEWED + + # Similarly, there could be a race between reviewer 2 and 3. For reviewer 2, we want to move the + # status to REVIEWED, for review 3 we should do nothing + with patch( + "airlock.business_logic.ReleaseRequest.completed_reviews_count" + ) as completed_reviews: + completed_reviews.side_effect = [2, 3] + bll.review_request(release_request, checkers[2]) + release_request = factories.refresh_release_request(release_request) + assert release_request.status == RequestStatus.REVIEWED + + # The request must be in a reviewed state (part or fully) + # Set status to submitted (mock check status to allow the invalid transition) + with patch("airlock.business_logic.BusinessLogicLayer.check_status"): + bll.set_status(release_request, RequestStatus.SUBMITTED, checkers[0]) + + with pytest.raises(bll.InvalidStateTransition): + with patch( + "airlock.business_logic.ReleaseRequest.completed_reviews_count" + ) as completed_reviews: + completed_reviews.side_effect = [2, 4] + bll.review_request(release_request, checkers[3]) + + # add DAL method names to this if they do not require auditing DAL_AUDIT_EXCLUDED = { "get_release_request", From a1128ff388a45b957e0f4baec67ccdf02919e2ad Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 16:00:26 +0100 Subject: [PATCH 19/22] Omit UNDECIDED when calculating if all files have been reviewed --- airlock/business_logic.py | 6 +++--- tests/unit/test_business_logic.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index f58f25f4..b785460a 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -875,13 +875,13 @@ def get_output_file_paths(self): def all_files_approved(self): return all( request_file.get_status() == RequestFileReviewStatus.APPROVED - for filegroup in self.filegroups.values() - for request_file in filegroup.output_files + for request_file in self.output_files().values() ) def all_files_reviewed_by_reviewer(self, reviewer: User) -> bool: return all( - rfile.get_status_for_user(reviewer) is not None + rfile.get_status_for_user(reviewer) + not in [None, UserFileReviewStatus.UNDECIDED] for rfile in self.output_files().values() ) diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index 99770711..c312ea18 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -1150,7 +1150,7 @@ def test_resubmit_request(bll, mock_notifications): factories.request_file(group="test", path="file1.txt", rejected=True), ], ) - assert len(release_request.completed_reviews) == 2 + assert release_request.completed_reviews_count() == 2 # author re-submits with no changes to files bll.submit_request(release_request, author) @@ -1165,12 +1165,12 @@ def test_resubmit_request(bll, mock_notifications): ) assert approved_file.get_status_for_user(user) == UserFileReviewStatus.APPROVED - # rejected file review is now undecided approved + # rejected file review is now undecided rejected_file = release_request.get_request_file_from_output_path( UrlPath("file1.txt") ) assert rejected_file.get_status_for_user(user) == UserFileReviewStatus.UNDECIDED - + assert not release_request.all_files_reviewed_by_reviewer(user) # completed reviews have been reset assert release_request.completed_reviews == {} From ef8e3ae764b0c9e7a5aa8b211dba792352700218 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 16:04:52 +0100 Subject: [PATCH 20/22] Refactor if/else branches in create_request_at_state --- tests/factories.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index 152835eb..c071ce0a 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -349,16 +349,15 @@ def create_request_at_state( bll.set_status(request, RequestStatus.RETURNED, checker) request = refresh_release_request(request) - if ( + if not ( status == RequestStatus.WITHDRAWN and withdrawn_after == RequestStatus.RETURNED ): - bll.set_status(request, RequestStatus.WITHDRAWN, author) - return refresh_release_request(request) - else: return request + bll.set_status(request, RequestStatus.WITHDRAWN, author) + return refresh_release_request(request) - elif status == RequestStatus.REJECTED: + if status == RequestStatus.REJECTED: bll.set_status(request, RequestStatus.REJECTED, checker) return refresh_release_request(request) @@ -368,7 +367,7 @@ def create_request_at_state( if status == RequestStatus.APPROVED: return request - elif status == RequestStatus.RELEASED: + if status == RequestStatus.RELEASED: bll.set_status(request, RequestStatus.RELEASED, checker) return refresh_release_request(request) From 6c9f3e01d6a322766fbc9269569d80854a9737a4 Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 16:12:50 +0100 Subject: [PATCH 21/22] s/create_request_at_state/create_request_at_status --- tests/factories.py | 6 +- tests/functional/test_code_pages.py | 2 +- tests/functional/test_e2e.py | 4 +- tests/functional/test_request_pages.py | 12 +-- tests/integration/views/test_request.py | 76 ++++++++-------- tests/local_db/test_data_access.py | 6 +- tests/unit/test_business_logic.py | 114 ++++++++++++------------ 7 files changed, 110 insertions(+), 110 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index c071ce0a..40588567 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -234,7 +234,7 @@ def create_repo(workspace, files=None, temporary=True): def create_release_request(workspace, user=None, **kwargs): assert ( kwargs.get("status", RequestStatus.PENDING) == RequestStatus.PENDING - ), "Use create_request_at_state to create a release request with a state other than PENDING" + ), "Use create_request_at_status to create a release request with a state other than PENDING" workspace = ensure_workspace(workspace) # create a default user with permission on workspace @@ -248,7 +248,7 @@ def create_release_request(workspace, user=None, **kwargs): return release_request -def create_request_at_state( +def create_request_at_status( workspace, status, author=None, @@ -265,7 +265,7 @@ def create_request_at_state( Files are provided using a factory method for creating them. e.g. - create_request_at_state( + create_request_at_status( "workspace", RequestStatus.RELEASED, files=[ diff --git a/tests/functional/test_code_pages.py b/tests/functional/test_code_pages.py index 294e1c12..d52ff2f9 100644 --- a/tests/functional/test_code_pages.py +++ b/tests/functional/test_code_pages.py @@ -12,7 +12,7 @@ def release_request(researcher_user): workspace = factories.create_workspace("test-dir1") factories.write_workspace_file(workspace, "foo.txt", "") factories.create_repo(workspace) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( workspace, author=researcher_user, status=RequestStatus.SUBMITTED ) # Ensure the request file is written using the workspace previously diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index d7f8e4a5..cab5b1fc 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -449,7 +449,7 @@ def test_e2e_reject_request(page, live_server, dev_users): Test output-checker rejects a release request """ # set up a reviewed request - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test-workspace", author=factories.create_user("author", workspaces=["test-workspace"]), status=RequestStatus.REVIEWED, @@ -477,7 +477,7 @@ def test_e2e_withdraw_request(page, live_server, dev_users): """ # set up a submitted request user = factories.create_user("researcher", ["test-workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test-workspace", author=user, status=RequestStatus.RETURNED, diff --git a/tests/functional/test_request_pages.py b/tests/functional/test_request_pages.py index 1cbd5078..de7e702e 100644 --- a/tests/functional/test_request_pages.py +++ b/tests/functional/test_request_pages.py @@ -19,7 +19,7 @@ def test_request_file_withdraw(live_server, context, page, bll): }, ) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.PENDING, @@ -64,7 +64,7 @@ def test_request_group_edit_comment(live_server, context, page, bll, settings): }, ) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, files=[factories.request_file(group="group")], @@ -138,7 +138,7 @@ def test_request_buttons( ), } - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=factories.create_user(**user_data[author]), status=status, @@ -200,7 +200,7 @@ def test_request_returnable( author = factories.create_user(**user_data["author"]) if checkers is not None: checkers = [factories.create_user(**user_data[user]) for user in checkers] - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -250,7 +250,7 @@ def test_returned_request(live_server, context, page, bll): checkers = [ factories.create_user(**user_data[user]) for user in ["checker1", "checker2"] ] - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.RETURNED, @@ -286,7 +286,7 @@ def test_returned_request(live_server, context, page, bll): def test_request_releaseable(live_server, context, page, bll): - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.REVIEWED, files=[ diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 26e32a19..f5bd6b1a 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -47,7 +47,7 @@ def test_request_id_does_not_exist(airlock_client): def test_request_view_root_summary(airlock_client): airlock_client.login(output_checker=True) audit_user = factories.create_user("audit_user") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=audit_user, status=RequestStatus.PENDING, @@ -80,7 +80,7 @@ def test_request_view_root_summary(airlock_client): def test_request_view_root_group(airlock_client): airlock_client.login(output_checker=True) audit_user = factories.create_user("audit_user") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=audit_user, status=RequestStatus.PENDING, @@ -149,7 +149,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_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED ) response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) @@ -162,7 +162,7 @@ def test_request_view_with_submitted_request(airlock_client): def test_request_view_with_reviewed_request(airlock_client): # Login as 1st default output-checker airlock_client.login("output-checker-0", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.REVIEWED ) response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True) @@ -183,7 +183,7 @@ def test_request_view_with_reviewed_request(airlock_client): def test_request_view_with_authored_request_file(airlock_client): airlock_client.login(output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=airlock_client.user, status=RequestStatus.SUBMITTED, @@ -199,7 +199,7 @@ def test_request_view_with_authored_request_file(airlock_client): def test_request_view_with_submitted_file(airlock_client): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -216,7 +216,7 @@ def test_request_view_with_submitted_file(airlock_client): def test_request_view_with_submitted_supporting_file(airlock_client): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -236,7 +236,7 @@ def test_request_view_with_submitted_supporting_file(airlock_client): def test_request_view_with_submitted_file_approved(airlock_client): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -253,7 +253,7 @@ def test_request_view_with_submitted_file_approved(airlock_client): def test_request_view_with_submitted_file_rejected(airlock_client): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -280,7 +280,7 @@ def test_request_view_with_404(airlock_client): def test_request_view_404_with_files(airlock_client): airlock_client.login(output_checker=True) # write a file and a supporting file to the group - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.PENDING, files=[ @@ -502,19 +502,19 @@ def test_request_index_user_permitted_requests(airlock_client): 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_request_at_state( + r1 = factories.create_request_at_status( "test_workspace", author=airlock_client.user, status=RequestStatus.SUBMITTED ) - r2 = factories.create_request_at_state( + r2 = factories.create_request_at_status( "other_workspace", author=other, status=RequestStatus.SUBMITTED ) - r3 = factories.create_request_at_state( + r3 = factories.create_request_at_status( "other_other_workspace", author=other, status=RequestStatus.RETURNED, files=[factories.request_file(rejected=True)], ) - r4 = factories.create_request_at_state( + r4 = factories.create_request_at_status( "other_other1_workspace", author=other, status=RequestStatus.APPROVED, @@ -593,7 +593,7 @@ def test_request_withdraw_not_author(airlock_client): def test_request_return_author(airlock_client): airlock_client.login(workspaces=["test1"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=airlock_client.user, status=RequestStatus.REVIEWED, @@ -615,7 +615,7 @@ def test_request_return_author(airlock_client): def test_request_return_output_checker(airlock_client): airlock_client.login(workspaces=["test1"], output_checker=True) other_author = factories.create_user("other", [], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=other_author, status=RequestStatus.REVIEWED, @@ -633,7 +633,7 @@ def test_request_return_output_checker(airlock_client): def test_request_review_author(airlock_client): airlock_client.login(workspaces=["test1"], output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=airlock_client.user, status=RequestStatus.SUBMITTED, @@ -648,7 +648,7 @@ def test_request_review_author(airlock_client): def test_request_review_output_checker(airlock_client): airlock_client.login("checker", workspaces=["test1"], output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", status=RequestStatus.SUBMITTED, files=[factories.request_file(approved=True, checkers=[airlock_client.user])], @@ -677,7 +677,7 @@ def test_request_review_output_checker(airlock_client): def test_request_review_non_output_checker(airlock_client): airlock_client.login(workspaces=["test1"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", status=RequestStatus.SUBMITTED, files=[factories.request_file(approved=True)], @@ -691,7 +691,7 @@ def test_request_review_non_output_checker(airlock_client): def test_request_review_not_all_files_reviewed(airlock_client): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", status=RequestStatus.SUBMITTED, files=[ @@ -752,7 +752,7 @@ def test_file_review_bad_user(airlock_client, review): airlock_client.login(workspaces=[workspace], output_checker=False) author = factories.create_user("author", [workspace], False) path = "path/test.txt" - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.SUBMITTED, @@ -782,7 +782,7 @@ def test_file_review_bad_file(airlock_client, review): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) path = "path/test.txt" - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.SUBMITTED, @@ -812,7 +812,7 @@ def test_file_approve(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) path = "path/test.txt" - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.SUBMITTED, @@ -839,7 +839,7 @@ def test_file_reject(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) path = "path/test.txt" - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.SUBMITTED, @@ -866,7 +866,7 @@ def test_file_reset_review(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) path = "path/test.txt" - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.SUBMITTED, @@ -907,7 +907,7 @@ def test_file_reset_review(airlock_client): def test_request_reject_output_checker(airlock_client): airlock_client.login(output_checker=True) author = factories.create_user("author", ["test1"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=author, status=RequestStatus.REVIEWED, @@ -923,7 +923,7 @@ def test_request_reject_output_checker(airlock_client): def test_request_reject_not_output_checker(airlock_client): - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=factories.create_user("author1", workspaces=["test1"]), status=RequestStatus.REVIEWED, @@ -943,7 +943,7 @@ def test_request_reject_not_output_checker(airlock_client): def test_file_withdraw_file_pending(airlock_client): airlock_client.login("author", ["test1"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=airlock_client.user, status=RequestStatus.PENDING, @@ -969,7 +969,7 @@ def test_file_withdraw_file_pending(airlock_client): def test_file_withdraw_file_submitted(airlock_client): airlock_client.login("author", ["test1"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test1", author=airlock_client.user, status=RequestStatus.SUBMITTED, @@ -1034,7 +1034,7 @@ def test_file_withdraw_file_bad_request(airlock_client): @pytest.mark.parametrize("status", [RequestStatus.REVIEWED, RequestStatus.APPROVED]) def test_request_release_files_success(airlock_client, release_files_stubber, status): airlock_client.login(username="checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=status, @@ -1058,7 +1058,7 @@ def test_request_release_files_success_htmx( airlock_client, release_files_stubber, status ): airlock_client.login(username="checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=status, @@ -1082,7 +1082,7 @@ def test_request_release_files_success_htmx( def test_requests_release_workspace_403(airlock_client): airlock_client.login("checker", output_checker=False) - factories.create_request_at_state( + factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.SUBMITTED, @@ -1097,7 +1097,7 @@ def test_requests_release_workspace_403(airlock_client): def test_requests_release_author_403(airlock_client): airlock_client.login(output_checker=True) - factories.create_request_at_state( + factories.create_request_at_status( "workspace", id="request_id", author=airlock_client.user, @@ -1114,7 +1114,7 @@ def test_requests_release_author_403(airlock_client): def test_requests_release_invalid_state_transition_403(airlock_client): airlock_client.login("checker", output_checker=True) - factories.create_request_at_state( + factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.RETURNED, @@ -1130,7 +1130,7 @@ def test_requests_release_invalid_state_transition_403(airlock_client): def test_requests_release_jobserver_403(airlock_client, release_files_stubber): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.REVIEWED, @@ -1168,7 +1168,7 @@ def test_requests_release_jobserver_403_with_debug( ): airlock_client.login("checker", output_checker=True) settings.DEBUG = True - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.REVIEWED, @@ -1193,7 +1193,7 @@ def test_requests_release_jobserver_403_with_debug( def test_requests_release_files_404(airlock_client, release_files_stubber): airlock_client.login("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.REVIEWED, @@ -1261,7 +1261,7 @@ def test_request_view_tracing_with_request_attribute( checker = factories.create_user("output_checker", output_checker=True) airlock_client.login(username=login_as, output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "test-workspace", status=status, author=author, diff --git a/tests/local_db/test_data_access.py b/tests/local_db/test_data_access.py index ef451312..35853d06 100644 --- a/tests/local_db/test_data_access.py +++ b/tests/local_db/test_data_access.py @@ -83,7 +83,7 @@ def test_get_audit_log(test_audits, kwargs, expected_audits): def test_delete_file_from_request_bad_state(): author = factories.create_user() - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, author=author ) audit = AuditEvent.from_request( @@ -107,7 +107,7 @@ def test_delete_file_from_request_bad_state(): ) def test_withdraw_file_from_request_bad_state(status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -147,7 +147,7 @@ def test_group_comment_delete_bad_params(): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["other-workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.PENDING, diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index c312ea18..e59c156f 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -310,7 +310,7 @@ def mock_old_api(monkeypatch): def test_provider_request_release_files_request_not_approved(bll, mock_notifications): author = factories.create_user("author", ["workspace"]) checker = factories.create_user("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, id="request_id", @@ -329,7 +329,7 @@ def test_provider_request_release_files_invalid_file_type(bll, mock_notification # mock the LEVEL4_FILE_TYPES so that we can create this request with an # invalid file with patch("airlock.utils.LEVEL4_FILE_TYPES", [".foo"]): - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.APPROVED, @@ -346,7 +346,7 @@ def test_provider_request_release_files(mock_old_api, mock_notifications, bll, f old_api.create_release.return_value = "jobserver_id" checker = factories.create_user("checker", [], output_checker=True) checker1 = factories.create_user("checker1", [], output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", id="request_id", status=RequestStatus.APPROVED, @@ -537,13 +537,13 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, user = factories.create_user("test", ["workspace"], output_checker) other_user = factories.create_user("other", ["workspace"], False) # request created by another user, status submitted - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=other_user, id="r1", status=RequestStatus.SUBMITTED ) # requests not visible to output checker # status submitted, but authored by output checker - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=user, id="r2", status=RequestStatus.SUBMITTED ) # requests authored by other users, status other than pending @@ -558,7 +558,7 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_request_at_state( + factories.create_request_at_status( ws, author=user_n, status=status, @@ -586,7 +586,7 @@ def test_provider_get_returned_requests(output_checker, expected, bll): other_user = factories.create_user("other", ["workspace"], False) output_checker = factories.create_user("other-checker", ["workspace"], True) # request created by another user, status returned - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=other_user, id="r1", @@ -596,7 +596,7 @@ def test_provider_get_returned_requests(output_checker, expected, bll): # requests not visible to output checker # status returned, but authored by output checker - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=user, id="r2", @@ -617,7 +617,7 @@ def test_provider_get_returned_requests(output_checker, expected, bll): ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_request_at_state( + factories.create_request_at_status( ws, author=user_n, status=status, @@ -644,7 +644,7 @@ def test_provider_get_approved_requests(output_checker, expected, bll): output_checker = factories.create_user("other-checker", ["workspace"], True) # request created by another user, status approved - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=other_user, id="r1", @@ -654,7 +654,7 @@ def test_provider_get_approved_requests(output_checker, expected, bll): # requests not visible to output checker # status approved, but authored by output checker - factories.create_request_at_state( + factories.create_request_at_status( "workspace", author=user, id="r2", @@ -675,7 +675,7 @@ def test_provider_get_approved_requests(output_checker, expected, bll): ): ws = f"workspace{i}" user_n = factories.create_user(f"test_{i}", [ws]) - factories.create_request_at_state( + factories.create_request_at_status( ws, author=user_n, status=status, @@ -917,7 +917,7 @@ def test_set_status(current, future, valid_author, valid_checker, withdrawn_afte file_reviewers = [checker, factories.create_user("checker1", [], True)] audit_type = bll.STATUS_AUDIT_EVENT[future] - release_request1 = factories.create_request_at_state( + release_request1 = factories.create_request_at_status( "workspace1", status=current, author=author, @@ -925,7 +925,7 @@ def test_set_status(current, future, valid_author, valid_checker, withdrawn_afte withdrawn_after=withdrawn_after, files=[factories.request_file(approved=True, checkers=file_reviewers)], ) - release_request2 = factories.create_request_at_state( + release_request2 = factories.create_request_at_status( "workspace2", status=current, author=author, @@ -1023,7 +1023,7 @@ def test_set_status_notifications( "author": factories.create_user("author", ["workspace"], False), "checker": factories.create_user(output_checker=True), } - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=current, files=[factories.request_file(approved=True)], @@ -1057,7 +1057,7 @@ def test_notification_error(bll, notifications_stubber, caplog): def test_set_status_approved(all_files_approved, bll, mock_notifications): author = factories.create_user("author", ["workspace"], False) checker = factories.create_user(output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.REVIEWED, @@ -1080,14 +1080,14 @@ def test_set_status_approved(all_files_approved, bll, mock_notifications): def test_set_status_cannot_action_own_request(bll): user = factories.create_user(output_checker=True) - release_request1 = factories.create_request_at_state( + release_request1 = factories.create_request_at_status( "workspace", author=user, status=RequestStatus.SUBMITTED ) with pytest.raises(bll.RequestPermissionDenied): bll.set_status(release_request1, RequestStatus.PARTIALLY_REVIEWED, user=user) - release_request2 = factories.create_request_at_state( + release_request2 = factories.create_request_at_status( "workspace1", author=user, status=RequestStatus.APPROVED, @@ -1100,7 +1100,7 @@ def test_set_status_cannot_action_own_request(bll): def test_set_status_approved_no_files_denied(bll): user = factories.create_user(output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.REVIEWED ) @@ -1110,7 +1110,7 @@ def test_set_status_approved_no_files_denied(bll): def test_set_status_approved_only_supporting_file_denied(bll): user = factories.create_user(output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.REVIEWED, files=[factories.request_file(filetype=RequestFileType.SUPPORTING)], @@ -1141,7 +1141,7 @@ def test_resubmit_request(bll, mock_notifications): """ author = factories.create_user("author", ["workspace"], False) # Returned request with two files, one is approved by both reviewers, one is rejected - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.RETURNED, @@ -1226,7 +1226,7 @@ def test_add_file_to_request_states( workspace = factories.create_workspace("workspace") factories.write_workspace_file(workspace, path) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( workspace, author=author, status=status, @@ -1305,7 +1305,7 @@ def test_withdraw_file_from_request_pending(bll, mock_notifications): author = factories.create_user(username="author", workspaces=["workspace"]) path1 = Path("path/file1.txt") path2 = Path("path/file2.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.PENDING, @@ -1351,7 +1351,7 @@ def test_withdraw_file_from_request_pending(bll, mock_notifications): def test_withdraw_file_from_request_submitted(bll, mock_notifications): author = factories.create_user(username="author", workspaces=["workspace"]) path1 = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -1394,7 +1394,7 @@ def test_withdraw_file_from_request_submitted(bll, mock_notifications): ) def test_withdraw_file_from_request_not_editable_state(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -1417,7 +1417,7 @@ def test_withdraw_file_from_request_not_editable_state(bll, status): @pytest.mark.parametrize("status", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) def test_withdraw_file_from_request_bad_file(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -1437,7 +1437,7 @@ def test_withdraw_file_from_request_bad_file(bll, status): @pytest.mark.parametrize("status", [RequestStatus.PENDING, RequestStatus.SUBMITTED]) def test_withdraw_file_from_request_not_author(bll, status): author = factories.create_user(username="author", workspaces=["workspace"]) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -1461,7 +1461,7 @@ def test_request_all_files_by_name(bll): path = Path("path/file.txt") supporting_path = Path("path/supporting_file.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.PENDING, @@ -1488,7 +1488,7 @@ def test_request_release_get_request_file_from_urlpath(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.PENDING, id="id", @@ -1515,7 +1515,7 @@ def test_request_release_get_request_file_from_urlpath(bll): def test_request_release_abspath(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.PENDING, id="id", @@ -1536,7 +1536,7 @@ def test_request_release_abspath(bll): def test_request_release_request_filetype(bll): path = UrlPath("foo/bar.txt") supporting_path = UrlPath("foo/bar1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.PENDING, id="id", @@ -1700,7 +1700,7 @@ def test_approve_file_not_checker(bll): def test_approve_file_not_part_of_request(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1717,7 +1717,7 @@ def test_approve_file_not_part_of_request(bll): def test_approve_supporting_file(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path, filetype=RequestFileType.SUPPORTING)], @@ -1734,7 +1734,7 @@ def test_approve_supporting_file(bll): def test_approve_file(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1762,7 +1762,7 @@ def test_approve_file(bll): def test_approve_file_requires_two_plus(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1784,7 +1784,7 @@ def test_approve_file_requires_two_plus(bll): def test_reject_file(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1812,7 +1812,7 @@ def test_reject_file(bll): def test_approve_then_reject_file(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1841,7 +1841,7 @@ def test_approve_then_reject_file(bll): ) def test_reviewreset_then_reset_review_file(bll, review): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1872,7 +1872,7 @@ def test_reviewreset_then_reset_review_file(bll, review): def test_reset_review_file_no_reviews(bll): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1901,7 +1901,7 @@ def test_reset_review_file_no_reviews(bll): ) def test_request_file_status_approved(bll, reviews, final_review): path = Path("path/file1.txt") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[factories.request_file(path=path)], @@ -1926,7 +1926,7 @@ def test_request_file_status_approved(bll, reviews, final_review): def test_mark_file_undecided(bll): # Set up submitted request - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.RETURNED, files=[factories.request_file(path="file.txt", rejected=True)], @@ -1964,7 +1964,7 @@ def test_mark_file_undecided_permission_errors( # for SUBMITTED/APPROVED/RELEASED, so we can set the request status path = "path/file.txt" checkers = factories.get_default_output_checkers() - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=request_status, files=[ @@ -1990,7 +1990,7 @@ def test_mark_file_undecided_permission_errors( def test_review_request(bll): checker = factories.create_user("checker", output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -2026,7 +2026,7 @@ def test_review_request(bll): def test_review_request_non_submitted_status(bll): checker = factories.create_user(output_checker=True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.WITHDRAWN, withdrawn_after=RequestStatus.PENDING, @@ -2046,7 +2046,7 @@ def test_review_request_non_submitted_status(bll): def test_review_request_non_output_checker(bll): user = factories.create_user("non-output-checker") - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -2061,7 +2061,7 @@ def test_review_request_non_output_checker(bll): def test_review_request_more_than_2_checkers(bll): checkers = [factories.create_user(f"checker_{i}", [], True) for i in range(3)] - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -2092,7 +2092,7 @@ def test_review_request_race_condition(bll): factories.create_user("checker2", output_checker=True), factories.create_user("checker3", output_checker=True), ] - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", status=RequestStatus.SUBMITTED, files=[ @@ -2238,7 +2238,7 @@ def test_group_edit_notifications( # Set the output checking org and repo to override any local settings settings.AIRLOCK_OUTPUT_CHECKING_ORG = settings.AIRLOCK_OUTPUT_CHECKING_REPO = None author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2280,7 +2280,7 @@ def test_notifications_org_repo( settings.AIRLOCK_OUTPUT_CHECKING_ORG = org settings.AIRLOCK_OUTPUT_CHECKING_REPO = repo author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2307,7 +2307,7 @@ def test_notifications_org_repo( def test_group_edit_not_author(bll): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2323,7 +2323,7 @@ def test_group_edit_not_author(bll): ) def test_group_edit_not_editable(bll, status): author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -2337,7 +2337,7 @@ def test_group_edit_not_editable(bll, status): def test_group_edit_bad_group(bll): author = factories.create_user("author", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2362,7 +2362,7 @@ def test_group_comment_create_success( ): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=status, @@ -2408,7 +2408,7 @@ def test_group_comment_create_permissions(bll): other = factories.create_user("other", ["other"], False) checker = factories.create_user("checker", ["other"], True) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.PENDING, @@ -2434,7 +2434,7 @@ def test_group_comment_create_permissions(bll): def test_group_comment_delete_success(bll): author = factories.create_user("author", ["workspace"], False) other = factories.create_user("other", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2489,7 +2489,7 @@ def test_group_comment_delete_permissions(bll): collaborator = factories.create_user("collaborator", ["workspace"], False) other = factories.create_user("other", ["other"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, @@ -2517,7 +2517,7 @@ def test_group_comment_create_invalid_params(bll): author = factories.create_user("author", ["workspace"], False) collaborator = factories.create_user("collaborator", ["workspace"], False) - release_request = factories.create_request_at_state( + release_request = factories.create_request_at_status( "workspace", author=author, status=RequestStatus.SUBMITTED, From 66efe6c9737e5dd6c96c5de01cc8edfbe320436b Mon Sep 17 00:00:00 2001 From: Becky Smith Date: Thu, 27 Jun 2024 16:16:15 +0100 Subject: [PATCH 22/22] Update airlock/business_logic.py Co-authored-by: Tom Ward --- airlock/business_logic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airlock/business_logic.py b/airlock/business_logic.py index b785460a..f7488d5f 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -1639,7 +1639,7 @@ def submit_request(self, request: ReleaseRequest, user: User): # reset completed review tracking self._dal.reset_reviews(request.id, audit) - # any files that have not been updated are set to UNDECIDED + # any unapproved files that have not been updated are set to UNDECIDED for rfile in request.output_files().values(): for review in rfile.rejected_reviews(): self.mark_file_undecided(request, review, rfile.relpath, user)