Show a sensible dialogue when updating a file (#605)

* Add FileForm
* we don't need the filetype for updating a file (we want to keep
  the old one)
* Split up workspace_add_file_to_request()
* Refactor add/update permissions checks for more granularity
* Create replace file endpoints to complement the add file endpoints
* Add tests for multiselect update
* Refactor functions away from workspace & into policies

airlock/business_logic.py

@@ -458,22 +458,6 @@ def abspath(self, relpath):
     def request_filetype(self, relpath: UrlPath) -> None:
         return None
 
-    def file_has_been_released(self, relpath: UrlPath) -> bool:
-        return self.get_workspace_file_status(relpath) == WorkspaceFileStatus.RELEASED
-
-    def file_can_be_updated(self, relpath: UrlPath) -> bool:
-        return self.get_workspace_file_status(relpath) in [
-            WorkspaceFileStatus.CONTENT_UPDATED,
-            WorkspaceFileStatus.WITHDRAWN,
-        ]
-
-    def file_can_be_added(self, relpath: UrlPath) -> bool:
-        return self.get_workspace_file_status(relpath) in [
-            WorkspaceFileStatus.UNRELEASED,
-            WorkspaceFileStatus.CONTENT_UPDATED,
-            WorkspaceFileStatus.WITHDRAWN,
-        ]
-
 
 @dataclass(frozen=True)
 class CodeRepo:
@@ -1640,6 +1624,12 @@ def update_file_in_request(
         relpath: UrlPath,
         user: User,
     ) -> ReleaseRequest:
+        relpath = UrlPath(relpath)
+        workspace = self.get_workspace(release_request.workspace, user)
+        permissions.check_user_can_update_file_on_request(
+            user, release_request, workspace, relpath
+        )
+
         request_file = release_request.get_request_file_from_output_path(relpath)
         return self.replace_file_in_request(
             release_request, relpath, user, request_file.group, request_file.filetype
@@ -1653,6 +1643,12 @@ def add_withdrawn_file_to_request(
         group_name: str = "default",
         filetype: RequestFileType = RequestFileType.OUTPUT,
     ) -> ReleaseRequest:
+        relpath = UrlPath(relpath)
+        workspace = self.get_workspace(release_request.workspace, user)
+        permissions.check_user_can_add_file_to_request(
+            user, release_request, workspace, relpath
+        )
+
         return self.replace_file_in_request(
             release_request, relpath, user, group_name, filetype
         )
@@ -1667,7 +1663,7 @@ def replace_file_in_request(
     ) -> ReleaseRequest:
         relpath = UrlPath(relpath)
         workspace = self.get_workspace(release_request.workspace, user)
-        permissions.check_user_can_update_file_on_request(
+        permissions.check_user_can_replace_file_in_request(
             user, release_request, workspace, relpath
         )
 

airlock/forms.py

@@ -74,6 +74,13 @@ def clean_new_filegroup(self):
             return new_filegroup
 
 
+class FileForm(forms.Form):
+    file = forms.CharField(
+        required=True,
+        widget=forms.HiddenInput(),
+    )
+
+
 class FileTypeForm(forms.Form):
     FILETYPE_CHOICES = [
         (RequestFileType.OUTPUT.name, RequestFileType.OUTPUT.name.title()),
@@ -113,6 +120,7 @@ def clean(self):
 
 
 FileTypeFormSet = formset_factory(FileTypeForm, extra=0, formset=RequiredOneBaseFormSet)
+FileFormSet = formset_factory(FileForm, extra=0, formset=RequiredOneBaseFormSet)
 
 
 class GroupEditForm(forms.Form):

airlock/permissions.py

@@ -151,6 +151,23 @@ def user_can_add_file_to_request(
     return True
 
 
+def check_user_can_replace_file_in_request(
+    user: User, request: "ReleaseRequest", workspace: "Workspace", relpath: UrlPath
+):
+    check_user_can_edit_request(user, request)
+    policies.check_can_replace_file_in_request(workspace, relpath)
+
+
+def user_can_replace_file_in_request(
+    user: User, request: "ReleaseRequest", workspace: "Workspace", relpath: UrlPath
+):  # pragma: no cover; not currently used
+    try:
+        check_user_can_replace_file_in_request(user, request, workspace, relpath)
+    except exceptions.RequestPermissionDenied:
+        return False
+    return True
+
+
 def check_user_can_update_file_on_request(
     user: User, request: "ReleaseRequest", workspace: "Workspace", relpath: UrlPath
 ):

airlock/policies.py

@@ -16,7 +16,7 @@
 from typing import TYPE_CHECKING
 
 from airlock import exceptions
-from airlock.enums import RequestFileVote, RequestStatus
+from airlock.enums import RequestFileVote, RequestStatus, WorkspaceFileStatus
 from airlock.types import UrlPath
 from airlock.utils import is_valid_file_type
 
@@ -58,17 +58,68 @@ def check_can_add_file_to_request(workspace: "Workspace", relpath: UrlPath):
         raise exceptions.RequestPermissionDenied(
             f"Cannot add file of type {relpath.suffix} to request"
         )
+
+    status = workspace.get_workspace_file_status(relpath)
+
     # The file hasn't already been released
-    if workspace.file_has_been_released(relpath):
+    if status == WorkspaceFileStatus.RELEASED:
         raise exceptions.RequestPermissionDenied("Cannot add released file to request")
 
-    if not workspace.file_can_be_added(relpath):
-        status = workspace.get_workspace_file_status(relpath)
+    if status not in [
+        WorkspaceFileStatus.UNRELEASED,
+        WorkspaceFileStatus.WITHDRAWN,
+    ]:
         raise exceptions.RequestPermissionDenied(
             f"Cannot add file to request if it is in status {status}"
         )
 
 
+def can_replace_file_in_request(workspace: "Workspace", relpath: UrlPath):
+    try:
+        check_can_replace_file_in_request(workspace, relpath)
+    except exceptions.RequestPermissionDenied:
+        return False
+    return True
+
+
+def check_can_replace_file_in_request(workspace: "Workspace", relpath: UrlPath):
+    """
+    This file can replace an existing file in the request, which currently happens
+    in two scenarios:
+    * when a file on a request is updated;
+    * or when a file on a request in the withdrawn state is re-added.
+    We expect that check_can_edit_request has already been called.
+    """
+    # The file is an allowed type
+    if not is_valid_file_type(Path(relpath)):
+        raise exceptions.RequestPermissionDenied(
+            f"Cannot add file of type {relpath.suffix} to request"
+        )
+
+    status = workspace.get_workspace_file_status(relpath)
+
+    # The file hasn't already been released
+    if status == WorkspaceFileStatus.RELEASED:
+        raise exceptions.RequestPermissionDenied("Cannot add released file to request")
+
+    if status not in [
+        WorkspaceFileStatus.WITHDRAWN,
+        WorkspaceFileStatus.CONTENT_UPDATED,
+    ]:
+        status = workspace.get_workspace_file_status(relpath)
+        raise exceptions.RequestPermissionDenied(
+            f"Cannot add or update file in request if it is in status {status}"
+        )
+
+
+def can_update_file_on_request(workspace: "Workspace", relpath: UrlPath):
+    try:
+        check_can_update_file_on_request(workspace, relpath)
+    except exceptions.RequestPermissionDenied:
+        return False
+    return True
+
+
 def check_can_update_file_on_request(workspace: "Workspace", relpath: UrlPath):
     """
     This file can be updated on the request.
@@ -79,7 +130,9 @@ def check_can_update_file_on_request(workspace: "Workspace", relpath: UrlPath):
             f"Cannot update file of type {relpath.suffix} in request"
         )
 
-    if not workspace.file_can_be_updated(relpath):
+    status = workspace.get_workspace_file_status(relpath)
+
+    if status != WorkspaceFileStatus.CONTENT_UPDATED:
         raise exceptions.RequestPermissionDenied(
             "Cannot update file in request if it is not updated on disk"
         )

airlock/templates/file_browser/file.html

@@ -8,13 +8,15 @@
           hx-target="#multiselect_modal"
           hx-swap="outerHtml"
         >
-          {% #button type="submit" name="action" value="add_files" variant="success" %}
-            {% if path_item.workspace_status.value == "UPDATED" %}
+          {% if path_item.workspace_status.value == "UPDATED" %}
+            {% #button type="submit" name="action" value="update_files" variant="success" %}
               Update File in Request
-            {% else %}
+            {% /button %}
+          {% else %}
+            {% #button type="submit" name="action" value="add_files" variant="success" %}
               Add File to Request
-            {% endif %}
-          {% /button %}
+            {% /button %}
+          {% endif %}
           <input type=hidden name="selected" value="{{ path_item.relpath }}"/>
           <input type=hidden name="next_url" value="{{ request.path }}"/>
           {% csrf_token %}

airlock/templates/update_files.html

@@ -0,0 +1,37 @@
+{# hide default button, as we are going to autoshow the modal anyway #}
+{% fragment as button %}
+{% endfragment %}
+{% #modal id="updateRequestFile" custom_button=button autoshow=True %}
+  {% #card container=True title="Update Files in Request" %}
+    <form action="{{ add_file_url }}" method="POST" aria-label="add-file-form">
+      {% csrf_token %}
+      {{ form.next_url }}
+      {{ formset.management_form }}
+      {% #list_group %}
+        {% for name, reason in files_ignored.items %}
+          {% #list_group_item %}
+            <div class="flex items-center gap-2 justify-between">
+              <span>{{ name }}</span>
+              <span class="flex flex-row">{{ reason }}</span>
+            </div>
+          {% /list_group_item %}
+        {% endfor %}
+        {% for formset_form in formset %}
+          {% #list_group_item %}
+            <div class="flex items-center gap-2 justify-between">
+              <span>{{ formset_form.file.value }}</span>
+              {{ formset_form.file }}
+            </div>
+          {% /list_group_item %}
+        {% endfor %}
+      {% /list_group %}
+
+      <div class="mt-2">
+        {% #button type="submit" variant="success" id="update-file-button" disabled=no_valid_files %}
+          Update Files in Request
+        {% /button %}
+        {% #button variant="danger" type="cancel" %}Cancel{% /button %}
+      </div>
+    </form>
+  {% /card %}
+{% /modal %}

airlock/urls.py

@@ -61,6 +61,11 @@
         airlock.views.workspace_add_file_to_request,
         name="workspace_add_file",
     ),
+    path(
+        "workspaces/update-file-in-request/<str:workspace_name>",
+        airlock.views.workspace_update_file_in_request,
+        name="workspace_update_file",
+    ),
     # requests
     path(
         "requests/",

airlock/views/__init__.py

@@ -25,6 +25,7 @@
     workspace_contents,
     workspace_index,
     workspace_multiselect,
+    workspace_update_file_in_request,
     workspace_view,
 )
 
@@ -55,6 +56,7 @@
     "workspace_multiselect",
     "workspace_contents",
     "workspace_index",
+    "workspace_update_file_in_request",
     "workspace_view",
     "serve_docs",
 ]