Merge pull request #419 from opensafely-core/rounds

Add support for tracking 2 independent reviews in a "round"

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

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" %}
           <form action="{{ request_submit_url }}" method="POST">
             {% 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 %}
           </form>
-        {% elif release_request.status.name == "SUBMITTED" %}
           {% #modal id="withdrawRequest" button_text="Withdraw this request" variant="warning" %}
             {% #card container=True title="Withdraw this request" %}
               <form action="{{ request_withdraw_url }}" method="POST">
@@ -38,39 +37,60 @@
           {% /modal %}
         {% endif %}
       {% elif is_output_checker %}
-        {% if release_request.status.name == "SUBMITTED" %}
-          <form action="{{ request_reject_url }}" method="POST">
-            {% csrf_token %}
-            {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %}
-          </form>
-          {% if release_request.all_files_reviewed %}
+        {% if release_request.status_owner.name == "REVIEWER" %}
+          {% comment %} A fully reviewed request can be returned or rejected {% endcomment %}
+          {% if release_request.status.name == "REVIEWED" %}
+            <form action="{{ request_reject_url }}" method="POST">
+              {% csrf_token %}
+              {% #button type="submit" variant="danger" class="action-button btn-sm" id="reject-request-button" %}Reject request{% /button %}
+            </form>
             <form action="{{ request_return_url }}" method="POST" hx-post="{{ request_return_url }}" hx-disabled-elt="button">
               {% csrf_token %}
               {% #button type="submit" class="btn-sm" tooltip="Return request for changes/clarification" variant="secondary" id="return-request-button" %}Return request{% /button %}
             </form>
           {% 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 the request is disabled until all files have been reviewed by two reviewers" %}
+              {% tooltip class="airlock-tooltip" content="Returning a request is disabled until review has been completed by two reviewers" %}
             {% /button %}
           {% endif %}
-        {% endif %}
-        {% if release_request.status.name in "SUBMITTED,APPROVED" %}
-          {% if release_request.all_files_approved %}
-            <form action="{{ release_files_url }}" method="POST" hx-post="{{ release_files_url }}" hx-disabled-elt="button">
+          {% 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 %}
+            <form action="{{ request_review_url }}" method="POST">
               {% csrf_token %}
-              {% #button class="btn-sm" type="submit" tooltip="Release files to jobs.opensafely.org" variant="warning" id="release-files-button" %}
-                Release Files
-                <img height="16" width="16" class="icon icon--green-700 animate-spin htmx-indicator" src="{% static 'icons/progress_activity.svg' %}" alt="">
-              {% /button %}
+              {% #button type="submit" class="btn-sm" tooltip="Complete Review" variant="secondary" id="complete-review-button" %}Complete review{% /button %}
             </form>
           {% 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 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.can_be_released %}
+          <form action="{{ release_files_url }}" method="POST" hx-post="{{ release_files_url }}" hx-disabled-elt="button">
+            {% csrf_token %}
+            {% #button class="btn-sm" type="submit" tooltip="Release files to jobs.opensafely.org" variant="warning" id="release-files-button" %}
+              Release files
+              <img height="16" width="16" class="icon icon--green-700 animate-spin htmx-indicator" src="{% static 'icons/progress_activity.svg' %}" alt="">
+            {% /button %}
+          </form>
+        {% 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 all files have been approved by by two reviewers" %}
+          {% /button %}
+        {% endif %}
       {% endif %}
     {% /airlock_header %}
   {% else %}

airlock/urls.py

@@ -113,6 +113,11 @@
         airlock.views.request_submit,
         name="request_submit",
     ),
+    path(
+        "requests/review/<str:request_id>",
+        airlock.views.request_review,
+        name="request_review",
+    ),
     path(
         "requests/reject/<str:request_id>",
         airlock.views.request_reject,

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

airlock/views/request.py

@@ -157,10 +157,26 @@ 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},
     )
+    request_review_url = reverse(
+        "request_review",
+        kwargs={"request_id": request_id},
+    )
     request_withdraw_url = reverse(
         "request_withdraw",
         kwargs={"request_id": request_id},
@@ -227,10 +243,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": 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,
@@ -294,6 +313,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 +480,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 +507,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:

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

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
@@ -114,6 +105,20 @@ 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, 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,
         request_id: str,

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

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