Merge pull request #607 from opensafely-core/evansd/withdraw-from-list

Allow withdrawing files from the file list view

airlock/templates/file_browser/_includes/directory_table/request.html

@@ -15,6 +15,11 @@
       <th>
         <div class="flex flex-row gap-2">Modified{% datatable_sort_icon %}</div>
       </th>
+      {% if multiselect_withdraw %}
+        <th data-searchable="false" data-sortable="false">
+          <input class="selectall" type="checkbox" onchange="toggleSelectAll(this)" />
+        </th>
+      {% endif %}
     </tr>
   {% /table_head %}
   <tbody>
@@ -24,6 +29,13 @@
         <td>{{ path.request_filetype.name|title }}</td>
         <td data-order="{{ path.size }}">{{ path.size_mb }}</td>
         <td>{{ path.modified_at|date:"Y-m-d H:i" }}</td>
+        {% if multiselect_withdraw %}
+          <td>
+            {% if not path.is_directory %}
+              {% form_checkbox name="selected" value=path.relpath custom_field=True %}
+            {% endif %}
+          </td>
+        {% endif %}
       </tr>
     {% endfor %}
   </tbody>

airlock/templates/file_browser/directory.html

@@ -40,6 +40,13 @@
         {% /button %}
       {% endif %}
     {% endif %}
+  {% elif context == "request" and multiselect_withdraw %}
+    <div class="div flex flex-col items-start gap-4">
+      <div class="flex items-center gap-2">
+        {% #button type="submit" name="action" value="withdraw_files" variant="warning" form="multiselect_form"%}Withdraw Files from Request{% /button %}
+      </div>
+      <div id="multiselect_modal"></div>
+    </div>
   {% endif %}
 {% endfragment %}
 

airlock/urls.py

@@ -103,6 +103,11 @@
         airlock.views.file_withdraw,
         name="file_withdraw",
     ),
+    path(
+        "requests/multiselect/<str:request_id>",
+        airlock.views.request_multiselect,
+        name="request_multiselect",
+    ),
     path(
         "requests/release/<str:request_id>",
         airlock.views.request_release_files,

airlock/views/__init__.py

@@ -10,6 +10,7 @@
     group_edit,
     request_contents,
     request_index,
+    request_multiselect,
     request_reject,
     request_release_files,
     request_return,
@@ -37,6 +38,7 @@
     "file_withdraw",
     "request_contents",
     "request_index",
+    "request_multiselect",
     "request_reject",
     "file_reset_review",
     "request_release_files",

airlock/views/request.py

@@ -18,12 +18,18 @@
 from airlock.business_logic import ROOT_PATH, bll
 from airlock.enums import RequestFileType, RequestFileVote, RequestStatus, Visibility
 from airlock.file_browser_api import get_request_tree
-from airlock.forms import GroupCommentDeleteForm, GroupCommentForm, GroupEditForm
+from airlock.forms import (
+    GroupCommentDeleteForm,
+    GroupCommentForm,
+    GroupEditForm,
+    MultiselectForm,
+)
 from airlock.types import UrlPath
 from services.tracing import instrument
 
 from .helpers import (
     display_form_errors,
+    display_multiple_messages,
     download_file,
     get_path_item_from_tree_or_404,
     get_release_request_or_raise,
@@ -335,8 +341,10 @@ def request_view(request, request_id: str, path: str = ""):
         "group_activity": group_activity,
         "show_c3": settings.SHOW_C3,
         "request_action_required": request_action_required,
-        # TODO, but for now stops template variable errors
-        "multiselect_url": "",
+        "multiselect_url": reverse(
+            "request_multiselect", kwargs={"request_id": request_id}
+        ),
+        "multiselect_withdraw": release_request.is_editing(),
         "code_url": code_url,
         "return_url": "",
         "group_comment_delete_url": group_comment_delete_url,
@@ -475,6 +483,39 @@ def file_withdraw(request, request_id, path: str):
     return redirect(redirect_url)
 
 
+@instrument(func_attributes={"release_request": "request_id"})
+@require_http_methods(["POST"])
+def request_multiselect(request, request_id: str):
+    release_request = get_release_request_or_raise(request.user, request_id)
+
+    multiform = MultiselectForm(request.POST)
+
+    if not multiform.is_valid():
+        display_form_errors(request, multiform.errors)
+    else:
+        action = multiform.cleaned_data["action"]
+        if action != "withdraw_files":
+            raise Http404(f"Invalid action {action}")
+
+        errors = []
+        successes = []
+        for path_str in multiform.cleaned_data["selected"]:
+            path = UrlPath(path_str)
+            try:
+                bll.withdraw_file_from_request(release_request, path, request.user)
+                successes.append(f"The file {path} has been withdrawn from the request")
+            except exceptions.RequestPermissionDenied as exc:
+                errors.append(str(exc))
+
+        if errors:
+            display_multiple_messages(request, errors, "error")
+        if successes:
+            display_multiple_messages(request, successes, "success")
+
+    url = multiform.cleaned_data["next_url"]
+    return HttpResponse(headers={"HX-Redirect": url})
+
+
 @instrument
 def requests_for_workspace(request, workspace_name: str):
     requests_for_workspace = bll.get_requests_for_workspace(

tests/functional/test_e2e.py

@@ -509,11 +509,19 @@ def test_e2e_withdraw_and_readd_file(page, live_server, dev_users):
 
     login_as(live_server, page, "researcher")
 
-    # Withdraw files after they've been reviewd
+    # Withdraw file1 from the file page
     page.goto(live_server.url + release_request.get_url("default/subdir/file1.txt"))
     find_and_click(page.locator("#withdraw-file-button"))
-    page.goto(live_server.url + release_request.get_url("default/subdir/file2.txt"))
-    find_and_click(page.locator("#withdraw-file-button"))
+    expect(page.locator("body")).to_contain_text("has been withdrawn from the request")
+
+    # Withdraw file2 from the directory page
+    page.goto(live_server.url + release_request.get_url("default/subdir"))
+    expect(page.locator("#customTable.datatable-table")).to_be_visible()
+    find_and_click(
+        page.locator('input[name="selected"][value="default/subdir/file2.txt"]')
+    )
+    find_and_click(page.locator("button[value=withdraw_files]"))
+    expect(page.locator("body")).to_contain_text("has been withdrawn from the request")
 
     # Change our mind on file 2: go to file page and re-add it
     page.goto(live_server.url + workspace.get_url(path2))

tests/integration/views/test_request.py

@@ -2,6 +2,7 @@
 
 import pytest
 import requests
+from django.contrib.messages import get_messages
 
 from airlock import exceptions
 from airlock.business_logic import bll
@@ -21,6 +22,10 @@
 pytestmark = pytest.mark.django_db
 
 
+def get_messages_text(response):
+    return "\n".join(m.message for m in get_messages(response.wsgi_request))
+
+
 def test_request_index_no_user(airlock_client):
     release_request = factories.create_release_request("workspace")
     response = airlock_client.get(f"/requests/view/{release_request.id}/")
@@ -1321,6 +1326,118 @@ def test_file_withdraw_file_bad_request(airlock_client):
     assert response.status_code == 404
 
 
+def test_request_multiselect_withdraw_files(airlock_client):
+    user = factories.create_user(workspaces=["workspace"])
+    release_request = factories.create_request_at_status(
+        list(user.workspaces)[0],
+        author=user,
+        status=RequestStatus.RETURNED,
+        files=[
+            factories.request_file(group="group", path="file1.txt", approved=True),
+            factories.request_file(group="group", path="file2.txt", approved=True),
+        ],
+    )
+
+    airlock_client.login_with_user(user)
+    response = airlock_client.post(
+        f"/requests/multiselect/{release_request.id}",
+        data={
+            "action": "withdraw_files",
+            "selected": [
+                "group/file1.txt",
+                "group/file2.txt",
+            ],
+            "next_url": release_request.get_url(),
+        },
+    )
+    persisted_request = factories.refresh_release_request(release_request)
+    messages = get_messages_text(response)
+
+    for path in ["group/file1.txt", "group/file2.txt"]:
+        request_file = persisted_request.get_request_file_from_urlpath(path)
+        assert request_file.filetype == RequestFileType.WITHDRAWN
+        assert f"{path} has been withdrawn from the request" in messages
+
+
+def test_request_multiselect_withdraw_files_not_permitted(airlock_client):
+    user = factories.create_user(workspaces=["workspace"])
+    release_request = factories.create_request_at_status(
+        list(user.workspaces)[0],
+        author=user,
+        status=RequestStatus.SUBMITTED,
+        files=[
+            factories.request_file(group="group", path="file1.txt", approved=True),
+            factories.request_file(group="group", path="file2.txt", approved=True),
+        ],
+    )
+
+    airlock_client.login_with_user(user)
+    response = airlock_client.post(
+        f"/requests/multiselect/{release_request.id}",
+        data={
+            "action": "withdraw_files",
+            "selected": [
+                "group/file1.txt",
+                "group/file2.txt",
+            ],
+            "next_url": release_request.get_url(),
+        },
+    )
+    persisted_request = factories.refresh_release_request(release_request)
+    messages = get_messages_text(response)
+    assert "Cannot withdraw file" in messages
+
+    for path in ["group/file1.txt", "group/file2.txt"]:
+        request_file = persisted_request.get_request_file_from_urlpath(path)
+        assert request_file.filetype != RequestFileType.WITHDRAWN
+
+
+def test_request_multiselect_withdraw_files_none_selected(airlock_client):
+    user = factories.create_user(workspaces=["workspace"])
+    release_request = factories.create_request_at_status(
+        list(user.workspaces)[0],
+        author=user,
+        status=RequestStatus.RETURNED,
+        files=[
+            factories.request_file(group="group", path="file1.txt", approved=True),
+        ],
+    )
+
+    airlock_client.login_with_user(user)
+    response = airlock_client.post(
+        f"/requests/multiselect/{release_request.id}",
+        data={
+            "action": "withdraw_files",
+            "selected": [],
+            "next_url": release_request.get_url(),
+        },
+    )
+    assert "You must select at least one file" in get_messages_text(response)
+
+
+def test_request_multiselect_withdraw_files_invalid_action(airlock_client):
+    user = factories.create_user(workspaces=["workspace"])
+    release_request = factories.create_request_at_status(
+        list(user.workspaces)[0],
+        author=user,
+        status=RequestStatus.RETURNED,
+        files=[
+            factories.request_file(group="group", path="file1.txt", approved=True),
+        ],
+    )
+
+    airlock_client.login_with_user(user)
+    response = airlock_client.post(
+        f"/requests/multiselect/{release_request.id}",
+        data={
+            "action": "prorogate_files",
+            "selected": ["group/file1.txt"],
+            "next_url": release_request.get_url(),
+        },
+    )
+    assert response.status_code == 404
+
+
 @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)