Merge pull request #193 from opensafely-core/madwort/implement-file-a…

…pproval-frontend

create file approve & reject urls

airlock/business_logic.py

@@ -386,6 +386,16 @@ def output_files_set(self):
             for request_file in filegroup.output_files
         }
 
+    def get_file_review_for_reviewer(self, urlpath: UrlPath, reviewer: str):
+        return next(
+            (
+                r
+                for r in self.get_request_file(urlpath).reviews
+                if r.reviewer == reviewer
+            ),
+            None,
+        )
+
     def request_filetype(self, urlpath: UrlPath):
         try:
             return self.get_request_file(urlpath).filetype

airlock/templates/file_browser/contents.html

@@ -54,7 +54,7 @@
           {% endif %}
         {% else %}{# request #}
           {% if path_item.is_supporting  %}
-            {% #description_item title="Supporting file" %}This is a supporting file and will not be releaed.{% /description_item%}
+            {% #description_item title="Supporting file" %}This is a supporting file and will not be released.{% /description_item%}
           {% elif path_item.is_withdrawn %}
             {% #description_item title="Withdrawn file" %}This file has been withdrawn and will not be released.{% /description_item%}
           {% endif %}
@@ -68,6 +68,24 @@
               </form>
             {% endif %}
           {% elif is_output_checker %}
+            {% if file_approve_url %}
+              <form action="{{ file_approve_url }}" method="POST">
+                {% csrf_token %}
+                {% #button type="submit" variant="success" id="file-approve-button" %}Approve File{% /button %}
+              </form>
+            {% else %}
+              {% csrf_token %}
+              {% #button type="submit" variant="success" disabled=True id="file-approve-button" %}Approve File{% /button %}
+            {% endif %}
+            {% if file_reject_url %}
+              <form action="{{ file_reject_url }}" method="POST">
+                {% csrf_token %}
+                {% #button type="submit" variant="danger" href=file_reject_url id="file-reject-button" %}Reject File{% /button %}
+              </form>
+            {% else %}
+              {% csrf_token %}
+              {% #button type="submit" variant="danger" disabled=True href=file_reject_url id="file-reject-button" %}Reject File{% /button %}
+            {% endif %}
             {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %}
           {% endif %}
         {% endif %}

airlock/urls.py

@@ -77,6 +77,16 @@
         airlock.views.request_contents,
         name="request_contents",
     ),
+    path(
+        "requests/approve/<str:request_id>/<path:path>",
+        airlock.views.file_approve,
+        name="file_approve",
+    ),
+    path(
+        "requests/reject/<str:request_id>/<path:path>",
+        airlock.views.file_reject,
+        name="file_reject",
+    ),
     path(
         "requests/release/<str:request_id>",
         airlock.views.request_release_files,

airlock/views/__init__.py

@@ -1,5 +1,7 @@
 from .auth import login, logout
 from .request import (
+    file_approve,
+    file_reject,
     request_contents,
     request_index,
     request_reject,
@@ -20,6 +22,8 @@
     "login",
     "logout",
     "index",
+    "file_approve",
+    "file_reject",
     "request_contents",
     "request_index",
     "request_reject",

airlock/views/request.py

@@ -11,7 +11,13 @@
 from django.views.decorators.vary import vary_on_headers
 from opentelemetry import trace
 
-from airlock.business_logic import RequestStatus, UrlPath, bll
+from airlock.business_logic import (
+    FileReviewStatus,
+    RequestFileType,
+    RequestStatus,
+    UrlPath,
+    bll,
+)
 from airlock.file_browser_api import get_request_tree
 from services.tracing import instrument
 
@@ -87,6 +93,33 @@ def request_view(request, request_id: str, path: str = ""):
         kwargs={"request_id": request_id},
     )
 
+    if (
+        is_directory_url
+        or release_request.request_filetype(path) == RequestFileType.SUPPORTING
+    ):
+        file_approve_url = None
+        file_reject_url = None
+    else:
+        file_approve_url = reverse(
+            "file_approve",
+            kwargs={"request_id": request_id, "path": path},
+        )
+        file_reject_url = reverse(
+            "file_reject",
+            kwargs={"request_id": request_id, "path": path},
+        )
+
+        existing_review = release_request.get_file_review_for_reviewer(
+            path, request.user.username
+        )
+        if existing_review:
+            if existing_review.status == FileReviewStatus.APPROVED:
+                file_approve_url = None
+            elif existing_review.status == FileReviewStatus.REJECTED:
+                file_reject_url = None
+            else:
+                assert False, "Invalid FileReviewStatus value"
+
     context = {
         "workspace": bll.get_workspace(release_request.workspace, request.user),
         "release_request": release_request,
@@ -97,6 +130,8 @@ def request_view(request, request_id: str, path: str = ""):
         # TODO file these in from user/models
         "is_author": is_author,
         "is_output_checker": request.user.output_checker,
+        "file_approve_url": file_approve_url,
+        "file_reject_url": file_reject_url,
         "request_submit_url": request_submit_url,
         "request_reject_url": request_reject_url,
         "release_files_url": release_files_url,
@@ -191,6 +226,44 @@ def request_withdraw(request, request_id):
     return redirect(redirect_url)
 
 
+@instrument(func_attributes={"release_request": "request_id"})
+@require_http_methods(["POST"])
+def file_approve(request, request_id, path: str):
+    release_request = get_release_request_or_raise(request.user, request_id)
+
+    try:
+        relpath = release_request.get_request_file(path).relpath
+    except bll.FileNotFound:
+        raise Http404()
+
+    try:
+        bll.approve_file(release_request, relpath, request.user)
+    except bll.ApprovalPermissionDenied as exc:
+        raise PermissionDenied(str(exc))
+
+    messages.success(request, "File has been approved")
+    return redirect(release_request.get_url(path))
+
+
+@instrument(func_attributes={"release_request": "request_id"})
+@require_http_methods(["POST"])
+def file_reject(request, request_id, path: str):
+    release_request = get_release_request_or_raise(request.user, request_id)
+
+    try:
+        relpath = release_request.get_request_file(path).relpath
+    except bll.FileNotFound:
+        raise Http404()
+
+    try:
+        bll.reject_file(release_request, relpath, request.user)
+    except bll.ApprovalPermissionDenied as exc:
+        raise PermissionDenied(str(exc))
+
+    messages.success(request, "File has been rejected")
+    return redirect(release_request.get_url(path))
+
+
 @instrument(func_attributes={"release_request": "request_id"})
 @require_http_methods(["POST"])
 def request_release_files(request, request_id):

tests/functional/test_e2e.py

@@ -101,7 +101,10 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
     - View requests list
     - Click and view submitted request
     - View output file
+    - Reject output file
+    - Approve output file
     - Download output file
+    - View supporting file
     - Release files
     - View requests list again and confirm released request is not shown
     """
@@ -320,12 +323,33 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber):
         "src", release_request.get_contents_url("my-new-group/subdir/file.txt")
     )
 
+    # Reject the file
+    expect(page.locator("#file-reject-button")).not_to_have_attribute("disabled", "")
+    find_and_click(page.locator("#file-reject-button"))
+    expect(page.locator("#file-reject-button")).to_have_attribute("disabled", "")
+
+    # Change our minds & approve the file
+    expect(page.locator("#file-approve-button")).not_to_have_attribute("disabled", "")
+    find_and_click(page.locator("#file-approve-button"))
+    expect(page.locator("#file-approve-button")).to_have_attribute("disabled", "")
+
     # Download the file
     with page.expect_download() as download_info:
         find_and_click(page.locator("#download-button"))
     download = download_info.value
     assert download.suggested_filename == "file.txt"
 
+    # Look at a supporting file & verify it can't be approved
+    supporting_file_link = page.get_by_role("link", name="supporting.txt").locator(
+        ".file:scope"
+    )
+    find_and_click(supporting_file_link)
+    expect(page.locator("iframe")).to_have_attribute(
+        "src", release_request.get_contents_url("my-new-group/subdir/supporting.txt")
+    )
+    expect(page.locator("#file-approve-button")).to_have_attribute("disabled", "")
+    expect(page.locator("#file-reject-button")).to_have_attribute("disabled", "")
+
     # Mock the responses from job-server
     release_request = bll.get_release_request(request_id, admin_user)
     release_files_stubber(release_request)