Merge pull request #95 from opensafely-core/filegroups-ui

Add support for adding a file to a new or existing file group on a request

Author: rebkwok

airlock/forms.py

@@ -0,0 +1,30 @@
+from django import forms
+
+
+class AddFileForm(forms.Form):
+    filegroup = forms.ChoiceField(required=False)
+    new_filegroup = forms.CharField(required=False)
+
+    def __init__(self, *args, **kwargs):
+        release_request = kwargs.pop("release_request")
+        super().__init__(*args, **kwargs)
+        if release_request:
+            self.filegroup_names = {group.name for group in release_request.filegroups}
+        else:
+            self.filegroup_names = set()
+        group_choices = {
+            (name, name) for name in self.filegroup_names if name != "default"
+        }
+        group_choices = [("default", "default"), *sorted(group_choices)]
+        self.fields["filegroup"].choices = group_choices
+        self.fields["new_filegroup"]
+
+    def clean_new_filegroup(self):
+        new_filegroup = self.cleaned_data.get("new_filegroup").lower()
+        if new_filegroup in [fg.lower() for fg in self.filegroup_names]:
+            self.add_error(
+                "new_filegroup",
+                f"File group with name '{new_filegroup}' already exists",
+            )
+        else:
+            return new_filegroup

airlock/templates/file_browser/index.html

@@ -1,5 +1,7 @@
 {% extends "base.html" %}
 
+{% load django_vite %}
+
 {% block full_width_content %}
 
 {% fragment as action_button %}
@@ -65,7 +67,33 @@
           {% endfor %}
         {% endif %}
       {% /list_group %}
-    {% /card %}
+
+    {% #card_footer no_container=False %}
+      {% if context == "request" %}
+        <div class="px-4">
+          {% #button variant="primary-outline" type="button" data-modal="viewFileGroups" %}
+            View File Groups
+          {% /button %}
+        </div>
+        {% #modal id="viewFileGroups" %}
+          {% #card container=True title="File Groups for this request" %}
+            {% for group in release_request.filegroups %}
+              {{ group.name}}
+              {% #list_group %}
+              {% for file in group.files %}
+                {% #list_group_item %}
+                  {{ file.relpath }}
+                {% /list_group_item %}
+              {% endfor %}
+              {% /list_group %}
+            {% endfor %}
+            {% #button variant="secondary" type="cancel" %}Close{% /button %}
+          {% /card %}
+        {% /modal %}
+        {% endif %}
+      {% /card_footer %}
+
+  {% /card %}
 
   </div>
 
@@ -91,14 +119,25 @@
       {% /card %}
 
     {% else %}
-
       {% fragment as add_button %}
         {% if context == "workspace" %}
-          <form action="{{ request_file_url }}" method="POST">
-            {% csrf_token %}
-            <input type=hidden name="path" value="{{ path_item.relpath }}"/>
-            {% #button type="submit" tooltip="Add this file to the current request, starting a new one if needed" variant="success" %}Add File to Request{% /button %}
-          </form>
+            {% #button variant="success" type="button" tooltip="Add this file to the current request, starting a new one if needed" data-modal="addRequestFile" id="add-file-button"%}
+              Add File to Request
+            {% /button %}
+            {% #modal id="addRequestFile" %}
+              {% #card container=True title="Add a file" %}
+                <form action="{{ request_file_url }}" method="POST">
+                  {% csrf_token %}
+                  {% form_select class="w-full max-w-lg mx-auto" label="Select a file group" field=form.filegroup choices=form.filegroup.field.choices %}
+                  {% form_input class="w-full max-w-lg mx-auto" label="Or create a new file group" field=form.new_filegroup %}
+                  <input type=hidden name="path" value="{{ path_item.relpath }}"/>
+                  <div class="mt-2">
+                    {% #button type="submit" variant="success" %}Add File to Request{% /button %}
+                    {% #button variant="danger" type="cancel" %}Cancel{% /button %}
+                  </div>
+                </form>
+                {% /card %}
+            {% /modal %}
         {% elif is_author %}
           <form action="" method="POST">
             {% csrf_token %}
@@ -133,3 +172,7 @@
 </div>
 
 {% endblock full_width_content %}
+
+{% block extra_js %}
+  {% vite_asset "templates/_components/modal/modal.js" %}
+{% endblock %}

airlock/views.py

@@ -14,6 +14,7 @@
 from airlock import login_api
 from airlock.api import Status
 from airlock.file_browser_api import PathItem
+from airlock.forms import AddFileForm
 from local_db.api import LocalDBProvider
 
 
@@ -141,6 +142,9 @@ def workspace_view(request, workspace_name: str, path: str = ""):
                 "workspace_add_file",
                 kwargs={"workspace_name": workspace_name},
             ),
+            "form": AddFileForm(
+                release_request=api.get_current_request(workspace_name, request.user)
+            ),
         },
     )
 
@@ -155,14 +159,23 @@ def workspace_add_file_to_request(request, workspace_name):
         raise Http404()
 
     release_request = api.get_current_request(workspace_name, request.user, create=True)
+    form = AddFileForm(request.POST, release_request=release_request)
+    if not form.is_valid():
+        for error in form.errors.values():
+            messages.error(request, error)
+        return redirect(workspace.get_url_for_path(relpath))
+
+    group_name = request.POST.get("new_filegroup") or request.POST.get("filegroup")
     try:
-        api.add_file_to_request(release_request, relpath, request.user)
+        api.add_file_to_request(release_request, relpath, request.user, group_name)
     except api.APIException as err:
         # This exception is raised if the file has already been added
         # (to any group on the request)
         messages.error(request, str(err))
     else:
-        messages.success(request, "File has been added to request")
+        messages.success(
+            request, f"File has been added to request (file group '{group_name}')"
+        )
     # redirect to this just added file
     return redirect(release_request.get_url_for_path(relpath))
 

tests/factories.py

@@ -49,3 +49,7 @@ def write_request_file(request, path, contents=""):
 
 def create_filegroup(release_request, group_name):
     return api._get_or_create_filegroupmetadata(release_request.id, group_name)
+
+
+def refresh_release_request(release_request):
+    return api.get_release_request(release_request.id)

tests/integration/test_views.py

@@ -1,5 +1,6 @@
 import pytest
 import requests
+from django.contrib import messages
 
 from airlock.api import Status
 from airlock.users import User
@@ -130,9 +131,9 @@ def test_workspace_request_file_creates(client_with_user, api):
     factories.write_workspace_file(workspace, "test/path.txt")
 
     assert api.get_current_request(workspace.name, user) is None
-
     response = client.post(
-        "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt"}
+        "/workspaces/add-file-to-request/test1",
+        data={"path": "test/path.txt", "filegroup": "default"},
     )
     assert response.status_code == 302
 
@@ -153,7 +154,8 @@ def test_workspace_request_file_request_already_exists(client_with_user, api):
     assert release_request.filegroups == []
 
     response = client.post(
-        "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt"}
+        "/workspaces/add-file-to-request/test1",
+        data={"path": "test/path.txt", "filegroup": "default"},
     )
     assert response.status_code == 302
     current_release_request = api.get_current_request(workspace.name, user)
@@ -164,6 +166,30 @@ def test_workspace_request_file_request_already_exists(client_with_user, api):
     assert str(filegroup.files[0].relpath) == "test/path.txt"
 
 
+def test_workspace_request_file_with_new_filegroup(client_with_user, api):
+    client = client_with_user({"workspaces": ["test1"]})
+    user = User.from_session(client.session)
+
+    workspace = factories.create_workspace("test1")
+    factories.write_workspace_file(workspace, "test/path.txt")
+
+    assert api.get_current_request(workspace.name, user) is None
+    response = client.post(
+        "/workspaces/add-file-to-request/test1",
+        data={
+            "path": "test/path.txt",
+            # new filegroup overrides a selected existing one (or the default)
+            "filegroup": "default",
+            "new_filegroup": "new_group",
+        },
+    )
+    assert response.status_code == 302
+
+    release_request = api.get_current_request(workspace.name, user)
+    filegroup = release_request.filegroups[0]
+    assert filegroup.name == "new_group"
+
+
 def test_workspace_request_file_filegroup_already_exists(client_with_user, api):
     client = client_with_user({"workspaces": ["test1"]})
     user = User.from_session(client.session)
@@ -175,14 +201,18 @@ def test_workspace_request_file_filegroup_already_exists(client_with_user, api):
     filegroupmetadata = factories.create_filegroup(release_request, "default")
     assert not filegroupmetadata.request_files.exists()
 
-    client.post("/workspaces/add-file-to-request/test1", data={"path": "test/path.txt"})
+    client.post(
+        "/workspaces/add-file-to-request/test1",
+        data={"path": "test/path.txt", "filegroup": "default"},
+    )
 
     assert filegroupmetadata.request_files.count() == 1
     assert str(filegroupmetadata.request_files.first().relpath) == "test/path.txt"
 
     # Attempt to add the same file again
     response = client.post(
-        "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt"}
+        "/workspaces/add-file-to-request/test1",
+        data={"path": "test/path.txt", "filegroup": "default"},
     )
     assert response.status_code == 302
     # No new file created
@@ -195,12 +225,44 @@ def test_workspace_request_file_request_path_does_not_exist(client_with_user):
     factories.create_workspace("test1")
 
     response = client.post(
-        "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt"}
+        "/workspaces/add-file-to-request/test1",
+        data={"path": "test/path.txt", "filegroup": "default"},
     )
 
     assert response.status_code == 404
 
 
+def test_workspace_request_file_invalid_new_filegroup(client_with_user, api):
+    client = client_with_user({"workspaces": ["test1"]})
+    user = User.from_session(client.session)
+
+    workspace = factories.create_workspace("test1")
+    factories.write_workspace_file(workspace, "test/path.txt")
+
+    release_request = factories.create_release_request(workspace, user)
+    filegroupmetadata = factories.create_filegroup(release_request, "test_group")
+
+    response = client.post(
+        "/workspaces/add-file-to-request/test1",
+        data={
+            "path": "test/path.txt",
+            "filegroup": "default",
+            "new_filegroup": "test_group",
+        },
+        follow=True,
+    )
+
+    assert not filegroupmetadata.request_files.exists()
+    # redirects to the workspace file again, with error messages
+    assert response.request["PATH_INFO"] == workspace.get_url_for_path("test/path.txt")
+
+    all_messages = [msg for msg in response.context["messages"]]
+    assert len(all_messages) == 1
+    message = all_messages[0]
+    assert message.level == messages.ERROR
+    assert "already exists" in message.message
+
+
 def test_request_index_no_user(client):
     release_request = factories.create_release_request("workspace")
     response = client.get(f"/requests/view/{release_request.id}/")

tests/unit/test_forms.py

@@ -0,0 +1,67 @@
+import pytest
+
+from airlock.forms import AddFileForm
+from tests import factories
+
+
+pytestmark = pytest.mark.django_db
+
+
+def test_add_file_form_no_release_request():
+    form = AddFileForm(release_request=None)
+    assert form.fields["filegroup"].choices == [("default", "default")]
+
+
+def test_add_file_form_empty_release_request():
+    release_request = factories.create_release_request("workspace")
+    form = AddFileForm(release_request=release_request)
+    assert form.fields["filegroup"].choices == [("default", "default")]
+
+
+def test_add_file_form_filegroup_choices():
+    release_request = factories.create_release_request("workspace")
+    for group in ["b_group", "a_group"]:
+        factories.create_filegroup(release_request, group)
+    release_request = factories.refresh_release_request(release_request)
+
+    other_release_request = factories.create_release_request("workspace1")
+    factories.create_filegroup(other_release_request, "other_group")
+
+    form = AddFileForm(release_request=release_request)
+    # default group is always first, other choices are sorted
+    assert form.fields["filegroup"].choices == [
+        ("default", "default"),
+        ("a_group", "a_group"),
+        ("b_group", "b_group"),
+    ]
+
+
+@pytest.mark.parametrize(
+    "new_group_name,is_valid",
+    [
+        # Can create a new default group if one doesn't already exist
+        ("default", True),
+        # Can't create a duplicate group
+        ("test", False),
+        # Can't create a duplicate group, case insensitive
+        ("Test", False),
+        # Can create a group with the same name as a group on another request
+        ("test 1", True),
+    ],
+)
+def test_add_file_form_new_filegroup(new_group_name, is_valid):
+    release_request = factories.create_release_request("workspace")
+    factories.create_filegroup(release_request, "test")
+    release_request = factories.refresh_release_request(release_request)
+
+    other_release_request = factories.create_release_request("workspace1")
+    factories.create_filegroup(other_release_request, "test 1")
+
+    data = {
+        # a filegroup is always in the POST data, ignored if
+        # new_filegroup also present
+        "filegroup": "default",
+        "new_filegroup": new_group_name,
+    }
+    form = AddFileForm(data, release_request=release_request)
+    assert form.is_valid() == is_valid