Merge pull request #770 from opensafely-core/rw/return-whenever

Allow reviewers to return requests at any point
opensafely-core · Feb 13, 2025 · 3c332cd · 3c332cd
2 parents 9a40af3 + e89c2bf
commit 3c332cd
Show file tree

Hide file tree

Showing 68 changed files with 49 additions and 50 deletions.
diff --git a/DEVELOPERS.md b/DEVELOPERS.md
@@ -2,7 +2,7 @@
 
 ## Diagrams
 
- * [Request State Machine](docs/request-states.md) (auto-generated)
+ * [Request State Machine](docs/reference/request-states.md) (auto-generated)
 
 
 ## Prerequisites for local development

diff --git a/airlock/business_logic.py b/airlock/business_logic.py
@@ -406,9 +406,11 @@ def get_approved_requests(self, user: User):
         ],
         RequestStatus.SUBMITTED: [
             RequestStatus.PARTIALLY_REVIEWED,
+            RequestStatus.RETURNED,
         ],
         RequestStatus.PARTIALLY_REVIEWED: [
             RequestStatus.REVIEWED,
+            RequestStatus.RETURNED,
         ],
         RequestStatus.REVIEWED: [
             RequestStatus.APPROVED,

diff --git a/airlock/templates/file_browser/request/request.html b/airlock/templates/file_browser/request/request.html
@@ -111,17 +111,18 @@
       {% endif %}
     {% endif %}
     {% if content_buttons.return.show %}
-      {% if content_buttons.return.disabled %}
-        {% #button disabled=True class="relative group" small=True variant="secondary" id="return-request-button" %}
-          Return request
-          {% tooltip class="airlock-tooltip" content=content_buttons.return.tooltip %}
-        {% /button %}
-      {% else %}
-        <form action="{{ content_buttons.return.url }}" method="POST">
-          {% csrf_token %}
-          {% #button type="submit" small=True tooltip=content_buttons.return.tooltip variant="secondary" id="return-request-button" %}Return request{% /button %}
-        </form>
-      {% endif %}
+      {% #modal id="returnRequest" button_small=True button_text="Return request" button_variant="secondary" button_tooltip=content_buttons.return.tooltip %}
+        {% #card container=True title="Return this request" %}
+          <form action="{{ content_buttons.return.url }}" method="POST">
+            {% csrf_token %}
+            <div class="pb-8">
+              {{content_buttons.return.modal_confirm_message}}
+            </div>
+            {% #button type="submit" variant="warning" class="action-button" small=True id="return-request-button" %}Return request{% /button %}
+            {% #button variant="primary" type="cancel" small=True %}Cancel{% /button %}
+          </form>
+        {% /card %}
+      {% /modal %}
     {% endif %}
     {% if content_buttons.release_files.show %}
       {% if content_buttons.release_files.disabled %}

diff --git a/airlock/views/request.py b/airlock/views/request.py
@@ -133,6 +133,9 @@ def _get_request_button_context(user, release_request):
         ]:
             button.show = True
 
+        # We allow output checkers to review at any time
+        return_btn.disabled = False
+
         try:
             permissions.check_user_can_submit_review(user, release_request)
         except exceptions.RequestReviewDenied as err:
@@ -143,11 +146,16 @@ def _get_request_button_context(user, release_request):
 
         if release_request.status == RequestStatus.REVIEWED:
             reject_btn.disabled = False
-            return_btn.disabled = False
             return_btn.tooltip = "Return request for changes/clarification"
+            return_btn.modal_confirm_message = "All reviews have been submitted. Are you ready to return the request to the original author?"
         else:
             reject_btn.tooltip = "Rejecting a request is disabled until review has been submitted by two reviewers"
-            return_btn.tooltip = "Returning a request is disabled until review has been submitted by two reviewers"
+            return_btn.tooltip = "Return request before full review"
+            return_btn.modal_confirm_message = (
+                "Are you sure you want to return this request before both reviews "
+                "are submitted? Typically you would only do this if the request "
+                "author has asked for an early return to make changes."
+            )
 
         if not release_request.can_be_released():
             release_files_btn.tooltip = "Releasing to jobs.opensafely.org is disabled until all files have been approved by by two reviewers"

diff --git a/assets/templates/_components/modal/modal.html b/assets/templates/_components/modal/modal.html
@@ -2,7 +2,7 @@
   {% if custom_button %}
     {{ custom_button }}
   {% else %}
-    {% #button class=button_class|default:"flex-shrink-0" small=button_small|default:False variant=button_variant|default:"primary" data-modal=id %}
+    {% #button class=button_class|default:"flex-shrink-0" small=button_small|default:False variant=button_variant|default:"primary" tooltip=button_tooltip data-modal=id %}
     {{ button_text }}
     {% /button %}
   {% endif %}

diff --git a/docs/explanation/workflow-and-permissions.md b/docs/explanation/workflow-and-permissions.md
@@ -57,8 +57,10 @@ flowchart TD
     subgraph Release Request Workflow
         A([PENDING]):::author -- Adds files and submits --> B([SUBMITTED]):::checker
         B -- 1st independent review --> C([PARTIALLY_REVIEWED]):::checker
+        B -- Return without review --> E([RETURNED]):::author
         C -- 2nd independent review --> D([REVIEWED]):::checker
-        D -- Consolidation --> E([RETURNED]):::author
+        C -- Return prior to full review --> E
+        D -- Consolidation --> E
         E -- Updates and responds to questions --> B
         D -- All files approved ----> F(RELEASED)
         D -- Not approved ----> G(REJECTED)

diff --git a/docs/how-tos/create-and-submit-a-release-request.md b/docs/how-tos/create-and-submit-a-release-request.md
@@ -71,3 +71,5 @@ Your release request's status will transition to "Submitted", and you will no lo
 able to edit it. Output checkers will be [automatically notified](../explanation/notifications.md) that the request is ready for review.
 
 ![Submitted request](../screenshots/submitted_request.png)
+
+If you have made a mistake on a submitted request, you can contact the output checkers. They will be able to return the request so you can make changes.
diff --git a/docs/how-tos/review-a-request.md b/docs/how-tos/review-a-request.md
@@ -29,8 +29,8 @@ It also shows a log of recent activity.
 
 This page also contains buttons for various actions that can be performed
 on the request as a whole (submitting your review, returning the request to
-the researcher etc.).  In the example below, all buttons are disabled, as
-this request has not yet been reviewed.
+the researcher etc.).  In the example below, all buttons (except "Return request")
+are disabled, as this request has not yet been reviewed.
 
 ![Request overview](../screenshots/request_overview.png)
 

diff --git a/docs/reference/request-states.md b/docs/reference/request-states.md
@@ -9,7 +9,9 @@ stateDiagram-v2
     PENDING --> SUBMITTED
     PENDING --> WITHDRAWN
     SUBMITTED --> PARTIALLY_REVIEWED
+    SUBMITTED --> RETURNED
     PARTIALLY_REVIEWED --> REVIEWED
+    PARTIALLY_REVIEWED --> RETURNED
     REVIEWED --> APPROVED
     REVIEWED --> REJECTED
     REVIEWED --> RETURNED

diff --git a/docs/request-states.md b/docs/request-states.md
diff --git a/docs/screenshots/add_file_button.png b/docs/screenshots/add_file_button.png
diff --git a/docs/screenshots/add_file_modal.png b/docs/screenshots/add_file_modal.png
diff --git a/docs/screenshots/changed_tree_file.png b/docs/screenshots/changed_tree_file.png
diff --git a/docs/screenshots/context_and_controls.png b/docs/screenshots/context_and_controls.png
diff --git a/docs/screenshots/context_modal.png b/docs/screenshots/context_modal.png
diff --git a/docs/screenshots/file_approved.png b/docs/screenshots/file_approved.png
diff --git a/docs/screenshots/file_group.png b/docs/screenshots/file_group.png
diff --git a/docs/screenshots/file_review.png b/docs/screenshots/file_review.png
diff --git a/docs/screenshots/file_update.png b/docs/screenshots/file_update.png
diff --git a/docs/screenshots/file_update_modal.png b/docs/screenshots/file_update_modal.png
diff --git a/docs/screenshots/files_released.png b/docs/screenshots/files_released.png
diff --git a/docs/screenshots/markdown_comment_anchor_1.png b/docs/screenshots/markdown_comment_anchor_1.png
diff --git a/docs/screenshots/markdown_comment_anchor_2.png b/docs/screenshots/markdown_comment_anchor_2.png
diff --git a/docs/screenshots/markdown_comment_blockquote_1.png b/docs/screenshots/markdown_comment_blockquote_1.png
diff --git a/docs/screenshots/markdown_comment_blockquote_2.png b/docs/screenshots/markdown_comment_blockquote_2.png
diff --git a/docs/screenshots/markdown_comment_bold_1.png b/docs/screenshots/markdown_comment_bold_1.png
diff --git a/docs/screenshots/markdown_comment_bold_2.png b/docs/screenshots/markdown_comment_bold_2.png
diff --git a/docs/screenshots/markdown_comment_code_1.png b/docs/screenshots/markdown_comment_code_1.png
diff --git a/docs/screenshots/markdown_comment_code_2.png b/docs/screenshots/markdown_comment_code_2.png
diff --git a/docs/screenshots/markdown_comment_italics_1.png b/docs/screenshots/markdown_comment_italics_1.png
diff --git a/docs/screenshots/markdown_comment_italics_2.png b/docs/screenshots/markdown_comment_italics_2.png
diff --git a/docs/screenshots/markdown_comment_ordered_list_1.png b/docs/screenshots/markdown_comment_ordered_list_1.png
diff --git a/docs/screenshots/markdown_comment_ordered_list_2.png b/docs/screenshots/markdown_comment_ordered_list_2.png
diff --git a/docs/screenshots/markdown_comment_unordered_list_1.png b/docs/screenshots/markdown_comment_unordered_list_1.png
diff --git a/docs/screenshots/markdown_comment_unordered_list_2.png b/docs/screenshots/markdown_comment_unordered_list_2.png
diff --git a/docs/screenshots/more_dropdown.png b/docs/screenshots/more_dropdown.png
diff --git a/docs/screenshots/more_dropdown_el.png b/docs/screenshots/more_dropdown_el.png
diff --git a/docs/screenshots/more_dropdown_el_request_file.png b/docs/screenshots/more_dropdown_el_request_file.png
diff --git a/docs/screenshots/multiselect_update.png b/docs/screenshots/multiselect_update.png
diff --git a/docs/screenshots/ready_to_release.png b/docs/screenshots/ready_to_release.png
diff --git a/docs/screenshots/request_independent_review_file_icons.png b/docs/screenshots/request_independent_review_file_icons.png
diff --git a/docs/screenshots/request_independent_review_researcher_file_icons.png b/docs/screenshots/request_independent_review_researcher_file_icons.png
diff --git a/docs/screenshots/request_overview.png b/docs/screenshots/request_overview.png
diff --git a/docs/screenshots/request_reviewed_file_icons.png b/docs/screenshots/request_reviewed_file_icons.png
diff --git a/docs/screenshots/request_tree.png b/docs/screenshots/request_tree.png
diff --git a/docs/screenshots/request_tree_post_voting.png b/docs/screenshots/request_tree_post_voting.png
diff --git a/docs/screenshots/returned_request_comments.png b/docs/screenshots/returned_request_comments.png
diff --git a/docs/screenshots/returned_tree.png b/docs/screenshots/returned_tree.png
diff --git a/docs/screenshots/reviewed_request_comments.png b/docs/screenshots/reviewed_request_comments.png
diff --git a/docs/screenshots/reviews_index.png b/docs/screenshots/reviews_index.png
diff --git a/docs/screenshots/submit_request.png b/docs/screenshots/submit_request.png
diff --git a/docs/screenshots/submit_review.png b/docs/screenshots/submit_review.png
diff --git a/docs/screenshots/submitted_request.png b/docs/screenshots/submitted_request.png
diff --git a/docs/screenshots/submitted_review.png b/docs/screenshots/submitted_review.png
diff --git a/docs/screenshots/withdraw_request.png b/docs/screenshots/withdraw_request.png
diff --git a/docs/screenshots/withdraw_request_modal.png b/docs/screenshots/withdraw_request_modal.png
diff --git a/docs/screenshots/withdrawn_file.png b/docs/screenshots/withdrawn_file.png
diff --git a/docs/screenshots/workspace_directory_content.png b/docs/screenshots/workspace_directory_content.png
diff --git a/docs/screenshots/workspace_directory_view.png b/docs/screenshots/workspace_directory_view.png
diff --git a/docs/screenshots/workspace_file_icons.png b/docs/screenshots/workspace_file_icons.png
diff --git a/docs/screenshots/workspace_file_view.png b/docs/screenshots/workspace_file_view.png
diff --git a/docs/screenshots/workspace_view.png b/docs/screenshots/workspace_view.png
diff --git a/docs/screenshots/workspaces_index.png b/docs/screenshots/workspaces_index.png
diff --git a/justfile b/justfile
@@ -219,7 +219,7 @@ manifests:
 
 
 # generate the automated state diagrams from code
-state-diagram file="docs/request-states.md":
+state-diagram file="docs/reference/request-states.md":
     cat scripts/statemachine.py | {{ just_executable() }} manage shell > {{ file }}
 
 # Run the documentation server: to configure the port, append: ---dev-addr localhost:<port>

diff --git a/tests/functional/test_docs_screenshots.py b/tests/functional/test_docs_screenshots.py
@@ -280,6 +280,7 @@ def do_review(screenshot=True):
 
     # Return to researcher
     page.goto(live_server.url + release_request.get_url())
+    page.locator("button[data-modal=returnRequest]").click()
     page.locator("#return-request-button").click()
 
     # Responding to returned request

diff --git a/tests/functional/test_request_pages.py b/tests/functional/test_request_pages.py
@@ -316,7 +316,7 @@ def test_request_buttons(
     page.goto(live_server.url + release_request.get_url())
 
     reviewer_buttons = [
-        "#return-request-button",
+        "button[data-modal=returnRequest]",
         "button[data-modal=rejectRequest]",
         "#submit-review-button",
     ]
@@ -463,8 +463,8 @@ def test_resubmit_button_visibility(
     "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.PARTIALLY_REVIEWED, ["checker1"], True),
+        ("checker2", RequestStatus.PARTIALLY_REVIEWED, ["checker1"], True),
         ("checker1", RequestStatus.REVIEWED, ["checker1", "checker2"], True),
     ],
 )
@@ -506,18 +506,18 @@ def test_request_returnable(
     )
 
     login_as_user(live_server, context, user_data[login_as])
-    return_request_button = page.locator("#return-request-button")
+    return_request_button = page.locator("button[data-modal=returnRequest]")
+    modal_return_request_button = page.locator("#return-request-button")
     page.goto(live_server.url + release_request.get_url())
 
     if can_return:
         expect(return_request_button).to_be_enabled()
+        expect(modal_return_request_button).not_to_be_visible()
         return_request_button.click()
-        expect(return_request_button).not_to_be_visible()
+        expect(modal_return_request_button).to_be_visible()
 
-    elif login_as == "author":
-        expect(return_request_button).not_to_be_visible()
     else:
-        expect(return_request_button).to_be_disabled()
+        expect(return_request_button).not_to_be_visible()
 
 
 def test_returned_request(live_server, context, page, bll):
@@ -597,7 +597,7 @@ def test_request_releaseable(live_server, context, page, bll):
 
     release_files_button = page.locator("#release-files-button")
     reject_request_button = page.locator("button[data-modal=rejectRequest]")
-    return_request_button = page.locator("#return-request-button")
+    return_request_button = page.locator("button[data-modal=returnRequest]")
 
     # Request is currently reviewed twice
     # output checker can release, return or reject

diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py
@@ -196,7 +196,7 @@ def test_request_view_with_submitted_request(airlock_client):
     response = airlock_client.get(f"/requests/view/{release_request.id}", follow=True)
     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 "Return request before full review" in response.rendered_content
     assert "Submit review" in response.rendered_content
 
 

diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py
@@ -833,14 +833,15 @@ def test_provider_get_current_request_for_user_output_checker(bll):
         (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.RETURNED, False, True, 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.RETURNED, False, True, None),
         (RequestStatus.PARTIALLY_REVIEWED, RequestStatus.WITHDRAWN, False, False, None),
         (RequestStatus.REVIEWED, RequestStatus.PENDING, False, False, None),
         (RequestStatus.REVIEWED, RequestStatus.SUBMITTED, False, False, None),
Original file line number	Diff line number	Diff line change
Expand Up		@@ -71,3 +71,5 @@ Your release request's status will transition to "Submitted", and you will no lo
		able to edit it. Output checkers will be [automatically notified](../explanation/notifications.md) that the request is ready for review.

		![Submitted request](../screenshots/submitted_request.png)

		If you have made a mistake on a submitted request, you can contact the output checkers. They will be able to return the request so you can make changes.