Only show "add file(s)" buttons when a request is in pending or returned statuses

airlock/templates/file_browser/directory.html

@@ -24,12 +24,22 @@
 {% fragment as buttons %}
   {% if context == "workspace" %}
     {% if path_item.children %}
-      <div class="div flex flex-col items-start gap-4">
-        <div class="flex items-center gap-2">
-          {% #button type="submit" name="action" value="add_files" variant="success" form="multiselect_form"%}Add Files to Request{% /button %}
+      {% if multiselect_add %}
+        <div class="div flex flex-col items-start gap-4">
+          <div class="flex items-center gap-2">
+            {% #button type="submit" name="action" value="add_files" variant="success" form="multiselect_form"%}Add Files to Request{% /button %}
+          </div>
+          <div id="multiselect_modal"></div>
         </div>
-        <div id="multiselect_modal"></div>
-      </div>
+      {% elif current_request and current_request.status_owner != "AUTHOR"  %}
+        {% #button type="button" disabled=True tooltip="The current request is under review and cannot be modified." id="add-file-modal-button-disabled" %}
+          Add Files to Request
+        {% /button %}
+      {% else %}
+        {% #button type="button" disabled=True tooltip="You do not have permission to add files to a request" id="add-file-modal-button-disabled" %}
+          Add Files to Request
+        {% /button %}
+      {% endif %}
     {% endif %}
   {% endif %}
 {% endfragment %}

airlock/templates/file_browser/file.html

@@ -22,6 +22,10 @@
         {% #button type="button" disabled=True tooltip="This file type cannot be added to a request" id="add-file-modal-button-disabled" %}
           Add File to Request
         {% /button %}
+      {% elif current_request and current_request.status_owner != "AUTHOR" %}
+        {% #button type="button" disabled=True tooltip="The current request is under review and cannot be modified." id="add-file-modal-button-disabled" %}
+          Add File to Request
+        {% /button %}
       {% else %}
         {% #button type="button" disabled=True tooltip="You do not have permission to add this file to a request" id="add-file-modal-button-disabled" %}
           Add File to Request

airlock/views/workspace.py

@@ -9,7 +9,11 @@
 from django.views.decorators.vary import vary_on_headers
 from opentelemetry import trace
 
-from airlock.business_logic import RequestFileType, bll
+from airlock.business_logic import (
+    RequestFileType,
+    RequestStatusOwner,
+    bll,
+)
 from airlock.file_browser_api import get_workspace_tree
 from airlock.forms import AddFileForm, FileTypeFormSet, MultiselectForm
 from airlock.types import UrlPath, WorkspaceFileStatus
@@ -63,23 +67,32 @@ def workspace_view(request, workspace_name: str, path: str = ""):
     if path_item.is_directory() != is_directory_url:
         return redirect(path_item.url())
 
-    # Only show the add form button this pathitem is a file that can be added
-    # to a request - i.e. it is a file and it's not already on the curent
-    # request for the user, and the user is allowed to add it to a request (if
-    # they are an output-checker they are allowed to view all workspaces, but
-    # not necessarily create requests for them.)
+    # Add file / add files buttons
+    #
+    # Only show the add files multiselect button if the user is allowed to
+    # create a request (if they are an output-checker they are allowed to
+    # view all workspaces, but not necessarily create requests for them)
+    # If there already is a current request, only show the multiselect add
+    # if the request is in an author-editable state (pending/returned)
+    #
+    # Only show the add file form button if the above is true, and also
+    # this pathitem is a file that can be added to a request - i.e. it is a
+    # file and it's not already on the curent request for the user
     # Currently we can just rely on checking the relpath against
     # the files on the request. In future we'll likely also need to
     # check file metadata to allow updating a file if the original has
     # changed.
+    multiselect_add = request.user.can_create_request(workspace_name) and (
+        workspace.current_request is None
+        or workspace.current_request.status_owner() == RequestStatusOwner.AUTHOR
+    )
     valid_states_to_add = [
         WorkspaceFileStatus.UNRELEASED,
         # TODO WorkspaceFileStatus.CONTENT_UPDATED,
     ]
-
     add_file = (
-        path_item.is_valid()
-        and request.user.can_create_request(workspace_name)
+        multiselect_add
+        and path_item.is_valid()
         and workspace.get_workspace_status(path_item.relpath) in valid_states_to_add
     )
 
@@ -125,6 +138,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
             "current_request": workspace.current_request,
             # for add file buttons
             "add_file": add_file,
+            "multiselect_add": multiselect_add,
             "multiselect_url": reverse(
                 "workspace_multiselect",
                 kwargs={"workspace_name": workspace_name},

tests/functional/test_e2e.py

@@ -75,12 +75,18 @@ def login_as(live_server, page, username):
     expect(page.locator("body")).to_contain_text(f"Logged in as: {username}")
 
 
-def tree_element_is_selected(page, locator):
+def assert_tree_element_is_not_selected(page, locator):
     # We need to wait for a tiny amount to ensure the js that swaps the
-    # selected classes around has run. An arbitrary 20ms seems to be enough.
+    # selected classes around has had time to run before we assert that the
+    # class isn't present. An arbitrary 20ms seems to be enough.
     page.wait_for_timeout(20)
-    classes = locator.get_attribute("class")
-    return "selected" in classes
+    expect(locator).not_to_have_class(re.compile("selected"))
+
+
+def assert_tree_element_is_selected(locator):
+    # expect waits for the default timeout period, so we don't need
+    # to rely on an explicit timeout
+    expect(locator).to_have_class(re.compile("selected"))
 
 
 def test_e2e_release_files(
@@ -273,35 +279,36 @@ def test_e2e_release_files(
 
     # Click on the output directory to ensure that renders correctly.
     subdir_link = page.get_by_role("link").locator(".directory:scope")
-    assert not tree_element_is_selected(page, subdir_link)
+
+    assert_tree_element_is_not_selected(page, subdir_link)
     find_and_click(subdir_link)
     # subdir link is shown as selected
-    assert tree_element_is_selected(page, subdir_link)
+    assert_tree_element_is_selected(subdir_link)
     expect(page.locator("#selected-contents")).to_contain_text("file.txt")
 
     # Tree opens fully expanded, so now the file (in its subdir) is visible
     find_and_click(file_link)
     # File is selected, subdir is now unselected
-    assert tree_element_is_selected(page, file_link)
-    assert not tree_element_is_selected(page, subdir_link)
+    assert_tree_element_is_selected(file_link)
+    assert_tree_element_is_not_selected(page, subdir_link)
     expect(page.locator("iframe")).to_have_attribute(
         "src", release_request.get_contents_url(UrlPath("my-new-group/subdir/file.txt"))
     )
 
     # Click on the supporting file link.
     supporting_file_link = page.get_by_role("link").locator(".supporting.file:scope")
     find_and_click(supporting_file_link)
-    assert tree_element_is_selected(page, supporting_file_link)
-    assert not tree_element_is_selected(page, file_link)
+    assert_tree_element_is_selected(supporting_file_link)
+    assert_tree_element_is_not_selected(page, file_link)
     expect(page.locator("iframe")).to_have_attribute(
         "src",
         release_request.get_contents_url(UrlPath("my-new-group/subdir/supporting.txt")),
     )
 
     # Click back to the output file link and ensure the selected classes are correctly applied
     find_and_click(file_link)
-    assert tree_element_is_selected(page, file_link)
-    assert not tree_element_is_selected(page, supporting_file_link)
+    assert_tree_element_is_selected(file_link)
+    assert_tree_element_is_not_selected(page, supporting_file_link)
     expect(page.locator("iframe")).to_have_attribute(
         "src", release_request.get_contents_url(UrlPath("my-new-group/subdir/file.txt"))
     )

tests/integration/views/test_workspace.py

@@ -3,7 +3,7 @@
 from django.contrib.messages.api import get_messages
 from django.urls import reverse
 
-from airlock.business_logic import AuditEventType, RequestFileType, bll
+from airlock.business_logic import AuditEventType, RequestFileType, RequestStatus, bll
 from airlock.types import UrlPath
 from tests import factories
 from tests.conftest import get_trace
@@ -66,6 +66,54 @@ def test_workspace_view_with_directory(airlock_client):
     assert "subdir" in response.rendered_content
 
 
+@pytest.mark.parametrize(
+    "login_as,status,can_multiselect_add",
+    [
+        # The request is pending, only author can add
+        ("author", RequestStatus.PENDING, True),
+        ("checker", RequestStatus.PENDING, False),
+        # The request is under reivew, no-one can add
+        ("author", RequestStatus.SUBMITTED, False),
+        ("checker", RequestStatus.SUBMITTED, False),
+        ("author", RequestStatus.PARTIALLY_REVIEWED, False),
+        ("checker", RequestStatus.PARTIALLY_REVIEWED, False),
+        ("author", RequestStatus.REVIEWED, False),
+        ("checker", RequestStatus.REVIEWED, False),
+        # The request is pending, only author can add
+        ("author", RequestStatus.RETURNED, True),
+        ("checker", RequestStatus.RETURNED, False),
+        # The request is not current, only author can add
+        ("author", RequestStatus.APPROVED, True),
+        ("checker", RequestStatus.APPROVED, False),
+        ("author", RequestStatus.RELEASED, True),
+        ("checker", RequestStatus.RELEASED, False),
+        ("author", RequestStatus.REJECTED, True),
+        ("checker", RequestStatus.REJECTED, False),
+        ("author", RequestStatus.WITHDRAWN, True),
+        ("checker", RequestStatus.WITHDRAWN, False),
+    ],
+)
+def test_workspace_directory_and_request_can_multiselect_add(
+    airlock_client, bll, login_as, status, can_multiselect_add
+):
+    users = {
+        "author": factories.create_user("author", workspaces=["workspace"]),
+        "checker": factories.create_user("checker", output_checker=True),
+    }
+    airlock_client.login_with_user(users[login_as])
+    factories.create_request_at_status(
+        "workspace",
+        status,
+        author=users["author"],
+        files=[factories.request_file(path="test/file.txt", approved=True)],
+        withdrawn_after=(
+            RequestStatus.PENDING if status == RequestStatus.WITHDRAWN else None
+        ),
+    )
+    response = airlock_client.get("/workspaces/view/workspace/test/")
+    assert response.context["multiselect_add"] == can_multiselect_add
+
+
 def test_workspace_view_with_empty_directory(airlock_client):
     airlock_client.login(output_checker=True)
     workspace = factories.create_workspace("workspace")
@@ -209,6 +257,96 @@ def test_workspace_view_file_add_to_request(airlock_client, user, can_see_form):
     assert response.context["add_file"] == can_see_form
 
 
+@pytest.mark.parametrize(
+    "status,is_current,files,can_see_form",
+    [
+        # author-editable
+        (RequestStatus.PENDING, True, [], True),
+        (RequestStatus.PENDING, True, [factories.request_file(path="file.txt")], False),
+        (RequestStatus.RETURNED, True, [], True),
+        (
+            RequestStatus.RETURNED,
+            True,
+            [factories.request_file(path="file.txt", rejected=True)],
+            False,
+        ),
+        # reviewer-editable
+        (RequestStatus.SUBMITTED, True, [], False),
+        (
+            RequestStatus.SUBMITTED,
+            True,
+            [factories.request_file(path="file.txt")],
+            False,
+        ),
+        (RequestStatus.PARTIALLY_REVIEWED, True, [], False),
+        (
+            RequestStatus.PARTIALLY_REVIEWED,
+            True,
+            [factories.request_file(path="file.txt", rejected=True)],
+            False,
+        ),
+        (RequestStatus.REVIEWED, True, [], False),
+        (
+            RequestStatus.REVIEWED,
+            True,
+            [factories.request_file(path="file.txt", rejected=True)],
+            False,
+        ),
+        # non-editable, can see form because there is no current request
+        (RequestStatus.WITHDRAWN, False, [], True),
+        (
+            RequestStatus.WITHDRAWN,
+            False,
+            [factories.request_file(path="file.txt")],
+            True,
+        ),
+        (RequestStatus.REJECTED, False, [], True),
+        (
+            RequestStatus.REJECTED,
+            False,
+            [factories.request_file(path="file.txt", rejected=True)],
+            True,
+        ),
+        (RequestStatus.APPROVED, False, [], True),
+        (
+            RequestStatus.APPROVED,
+            False,
+            [factories.request_file(path="file.txt", approved=True)],
+            True,
+        ),
+        (RequestStatus.RELEASED, False, [], True),
+        (
+            RequestStatus.RELEASED,
+            False,
+            [factories.request_file(path="file.txt", approved=True)],
+            True,
+        ),
+    ],
+)
+def test_workspace_view_file_add_to_current_request(
+    airlock_client, status, is_current, files, can_see_form
+):
+    user = factories.create_user(workspaces=["workspace"])
+    airlock_client.login_with_user(user)
+    workspace = factories.create_workspace("workspace")
+    factories.write_workspace_file("workspace", "file.txt", "foo")
+    release_request = factories.create_request_at_status(
+        workspace,
+        status,
+        author=user,
+        files=[factories.request_file(path="other_file.txt", approved=True), *files],
+        withdrawn_after=RequestStatus.PENDING
+        if status == RequestStatus.WITHDRAWN
+        else None,
+    )
+    response = airlock_client.get("/workspaces/view/workspace/file.txt")
+    if is_current:
+        assert response.context["current_request"] == release_request
+    else:
+        assert response.context["current_request"] is None
+    assert response.context["add_file"] == can_see_form
+
+
 def test_workspace_view_index_no_user(airlock_client):
     workspace = factories.create_workspace("workspace")
     (workspace.root() / "some_dir").mkdir(parents=True)