diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 1b7fc883..6aa900a3 100644 --- a/airlock/business_logic.py +++ b/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 ) diff --git a/airlock/forms.py b/airlock/forms.py index 80d7f038..5dee9d6e 100644 --- a/airlock/forms.py +++ b/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): diff --git a/airlock/permissions.py b/airlock/permissions.py index 0697fa44..47445e5f 100644 --- a/airlock/permissions.py +++ b/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 ): diff --git a/airlock/policies.py b/airlock/policies.py index a30d644e..e7ab9118 100644 --- a/airlock/policies.py +++ b/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" ) diff --git a/airlock/templates/file_browser/file.html b/airlock/templates/file_browser/file.html index 3082bf03..02d7e454 100644 --- a/airlock/templates/file_browser/file.html +++ b/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 %} {% csrf_token %} diff --git a/airlock/templates/update_files.html b/airlock/templates/update_files.html new file mode 100644 index 00000000..e65f2da5 --- /dev/null +++ b/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" %} +
+ {% csrf_token %} + {{ form.next_url }} + {{ formset.management_form }} + {% #list_group %} + {% for name, reason in files_ignored.items %} + {% #list_group_item %} +
+ {{ name }} + {{ reason }} +
+ {% /list_group_item %} + {% endfor %} + {% for formset_form in formset %} + {% #list_group_item %} +
+ {{ formset_form.file.value }} + {{ formset_form.file }} +
+ {% /list_group_item %} + {% endfor %} + {% /list_group %} + +
+ {% #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 %} +
+
+ {% /card %} +{% /modal %} diff --git a/airlock/urls.py b/airlock/urls.py index 48bfbb89..6ed8d0df 100644 --- a/airlock/urls.py +++ b/airlock/urls.py @@ -61,6 +61,11 @@ airlock.views.workspace_add_file_to_request, name="workspace_add_file", ), + path( + "workspaces/update-file-in-request/", + airlock.views.workspace_update_file_in_request, + name="workspace_update_file", + ), # requests path( "requests/", diff --git a/airlock/views/__init__.py b/airlock/views/__init__.py index b867a33a..d90b3f95 100644 --- a/airlock/views/__init__.py +++ b/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", ] diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index da68027d..b74ae803 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -15,6 +15,7 @@ from airlock.file_browser_api import get_workspace_tree from airlock.forms import ( AddFileForm, + FileFormSet, FileTypeFormSet, MultiselectForm, ) @@ -90,8 +91,9 @@ def workspace_view(request, workspace_name: str, path: str = ""): # Only show the add file form button if the multiselect_add condition is true, # and also this pathitem is a file that can be added to a request - i.e. it is a # valid file and it's not already on the current request for the user - add_file = multiselect_add and policies.can_add_file_to_request( - workspace, path_item.relpath + add_file = multiselect_add and ( + policies.can_add_file_to_request(workspace, path_item.relpath) + or policies.can_replace_file_in_request(workspace, path_item.relpath) ) project = workspace.project() @@ -183,6 +185,8 @@ def workspace_multiselect(request, workspace_name: str): if action == "add_files": return multiselect_add_files(request, multiform, workspace) + if action == "update_files": + return multiselect_update_files(request, multiform, workspace) # TODO: withdraw action else: raise Http404(f"Invalid action {action}") @@ -213,6 +217,8 @@ def multiselect_add_files(request, multiform, workspace): state = workspace.get_workspace_file_status(relpath) if policies.can_add_file_to_request(workspace, relpath): files_to_add.append(f) + elif state == WorkspaceFileStatus.CONTENT_UPDATED: + files_ignored[f] = "cannot update using the add dialogue" elif state == WorkspaceFileStatus.RELEASED: files_ignored[f] = "already released" else: @@ -244,25 +250,53 @@ def multiselect_add_files(request, multiform, workspace): ) -@instrument(func_attributes={"workspace": "workspace_name"}) -@require_http_methods(["POST"]) -def workspace_add_file_to_request(request, workspace_name): - workspace = get_workspace_or_raise(request.user, workspace_name) - release_request = bll.get_or_create_current_request(workspace_name, request.user) - form = AddFileForm(request.POST, release_request=release_request) - formset = FileTypeFormSet(request.POST) +def multiselect_update_files(request, multiform, workspace): + files_to_add = [] + files_ignored = {} - # default redirect in case of error - next_url = workspace.get_url() + # validate which files can be added + for f in multiform.cleaned_data["selected"]: + workspace.abspath(f) # validate path + + if policies.can_update_file_on_request(workspace, UrlPath(f)): + files_to_add.append(f) + else: + files_ignored[f] = "file cannot be updated" + + add_file_form = AddFileForm( + release_request=workspace.current_request, + initial={"next_url": multiform.cleaned_data["next_url"]}, + ) + + filetype_formset = FileFormSet( + initial=[{"file": f} for f in files_to_add], + ) + + return TemplateResponse( + request, + template="update_files.html", + context={ + "form": add_file_form, + "formset": filetype_formset, + "files_ignored": files_ignored, + "no_valid_files": len(files_to_add) == 0, + # "update": True, + "add_file_url": reverse( + "workspace_update_file", + kwargs={"workspace_name": workspace.name}, + ), + }, + ) + + +# also displays errors if present +def add_or_update_form_is_valid(request, form, formset): errors = False if not form.is_valid(): display_form_errors(request, form.errors) errors = True - if "next_url" not in form.errors: - next_url = form.cleaned_data["next_url"] - if not formset.is_valid(): form_errors = [f.errors for f in formset] non_form_errors = formset.non_form_errors() @@ -271,11 +305,18 @@ def workspace_add_file_to_request(request, workspace_name): display_form_errors(request, *form_errors) errors = True - if errors: - # redirect back where we came from with errors - return redirect(next_url) + return errors + + +def get_next_url(workspace, form): + if "next_url" not in form.errors: + return form.cleaned_data["next_url"] - # check the files all exist + # default redirect in case of error + return workspace.get_url() + + +def check_all_files_exist(workspace, formset): try: for formset_form in formset: relpath = formset_form.cleaned_data["file"] @@ -283,6 +324,24 @@ def workspace_add_file_to_request(request, workspace_name): except exceptions.FileNotFound: raise Http404(f"file {relpath} does not exist") + +@instrument(func_attributes={"workspace": "workspace_name"}) +@require_http_methods(["POST"]) +def workspace_add_file_to_request(request, workspace_name): + workspace = get_workspace_or_raise(request.user, workspace_name) + release_request = bll.get_or_create_current_request(workspace_name, request.user) + form = AddFileForm(request.POST, release_request=release_request) + formset = FileTypeFormSet(request.POST) + + errors = add_or_update_form_is_valid(request, form, formset) + next_url = get_next_url(workspace, form) + + if errors: + # redirect back where we came from with errors + return redirect(next_url) + + check_all_files_exist(workspace, formset) + group_name = ( form.cleaned_data.get("new_filegroup") or form.cleaned_data.get("filegroup") @@ -296,10 +355,7 @@ def workspace_add_file_to_request(request, workspace_name): status = workspace.get_workspace_file_status(UrlPath(relpath)) try: - if status == WorkspaceFileStatus.CONTENT_UPDATED: - bll.update_file_in_request(release_request, relpath, request.user) - success_msg = "updated in request" - elif status == WorkspaceFileStatus.WITHDRAWN: + if status == WorkspaceFileStatus.WITHDRAWN: bll.add_withdrawn_file_to_request( release_request, relpath, request.user, group_name, filetype ) @@ -323,3 +379,39 @@ def workspace_add_file_to_request(request, workspace_name): # redirect back where we came from return redirect(next_url) + + +@instrument(func_attributes={"workspace": "workspace_name"}) +@require_http_methods(["POST"]) +def workspace_update_file_in_request(request, workspace_name): + workspace = get_workspace_or_raise(request.user, workspace_name) + release_request = bll.get_or_create_current_request(workspace_name, request.user) + form = AddFileForm(request.POST, release_request=release_request) + formset = FileFormSet(request.POST) + + errors = add_or_update_form_is_valid(request, form, formset) + next_url = get_next_url(workspace, form) + + if errors: + # redirect back where we came from with errors + return redirect(next_url) + + check_all_files_exist(workspace, formset) + + error_msgs = [] + success_msgs = [] + for formset_form in formset: + relpath = formset_form.cleaned_data["file"] + + try: + bll.update_file_in_request(release_request, relpath, request.user) + except exceptions.APIException as err: + error_msgs.append(f"{relpath}: {err}") + else: + success_msgs.append(f"{relpath}: file has been updated in request") + + display_multiple_messages(request, success_msgs, "success") + display_multiple_messages(request, error_msgs, "error") + + # redirect back where we came from + return redirect(next_url) diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index 43a43ed8..2a45440e 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -482,20 +482,12 @@ def test_e2e_update_file(page, live_server, dev_users): # Update file in request # Find the add file button and click on it to open the modal - find_and_click(page.locator("button[value=add_files]")) - - # By default, the selected filetype is OUTPUT - expect(page.locator("input[name=form-0-filetype][value=OUTPUT]")).to_be_checked() - expect( - page.locator("input[name=form-0-filetype][value=SUPPORTING]") - ).not_to_be_checked() + find_and_click(page.locator("button[value=update_files]")) # Click the button to add the file to a release request - find_and_click(page.get_by_role("form").locator("#add-file-button")) + find_and_click(page.get_by_role("form").locator("#update-file-button")) - expect(page.locator("body")).to_contain_text( - "Output file has been updated in request" - ) + expect(page.locator("body")).to_contain_text("file has been updated in request") def test_e2e_withdraw_and_readd_file(page, live_server, dev_users): diff --git a/tests/integration/views/test_workspace.py b/tests/integration/views/test_workspace.py index a029d683..22aca44c 100644 --- a/tests/integration/views/test_workspace.py +++ b/tests/integration/views/test_workspace.py @@ -3,8 +3,9 @@ from django.contrib.messages.api import get_messages from django.urls import reverse +from airlock import policies from airlock.business_logic import Project -from airlock.enums import RequestFileType, RequestStatus +from airlock.enums import RequestFileType, RequestStatus, WorkspaceFileStatus from airlock.types import UrlPath from tests import factories from tests.conftest import get_trace @@ -605,6 +606,103 @@ def test_workspace_multiselect_add_files_none_valid(airlock_client, bll): assert 'name="filegroup"' not in response.rendered_content +def test_workspace_multiselect_add_files_updated_file(airlock_client, bll): + airlock_client.login(workspaces=["test1"]) + workspace = factories.create_workspace("test1") + factories.write_workspace_file(workspace, "test/path1.txt") + factories.write_workspace_file(workspace, "test/path2.txt") + release_request = factories.create_release_request( + workspace, user=airlock_client.user + ) + factories.add_request_file(release_request, "group1", "test/path1.txt") + factories.add_request_file(release_request, "group1", "test/path2.txt") + + factories.write_workspace_file(workspace, "test/path1.txt", "changed1") + # refresh workspace + workspace = bll.get_workspace("test1", airlock_client.user) + policies.check_can_update_file_on_request(workspace, "test/path1.txt") + + response = airlock_client.post( + "/workspaces/multiselect/test1", + data={ + "action": "add_files", + "selected": [ + "test/path1.txt", + "test/path2.txt", + ], + "next_url": workspace.get_url("test/path.txt"), + }, + ) + + assert response.status_code == 200 + assert "test/path1.txt" in response.rendered_content + assert "test/path2.txt" in response.rendered_content + assert response.rendered_content.count("cannot update using the add dialogue") == 1 + assert response.rendered_content.count("already in group") == 1 + assert 'name="filegroup"' not in response.rendered_content + + +@pytest.mark.parametrize( + "path1_updated,path2_updated,ignored_count", + [ + (True, True, 0), + (True, False, 1), + (False, False, 2), + ], +) +def test_workspace_multiselect_update_files( + airlock_client, bll, path1_updated, path2_updated, ignored_count +): + author = factories.create_user("author", workspaces=["test1"]) + airlock_client.login_with_user(author) + + workspace = factories.create_workspace("test1") + path1 = "test/path1.txt" + path2 = "test/path2.txt" + + factories.create_request_at_status( + "test1", + author=airlock_client.user, + status=RequestStatus.RETURNED, + files=[ + factories.request_file(path=path1, group="default", changes_requested=True), + factories.request_file(path=path2, group="default", changes_requested=True), + ], + ) + + assert workspace.get_workspace_file_status(path1) == WorkspaceFileStatus.UNRELEASED + assert workspace.get_workspace_file_status(path2) == WorkspaceFileStatus.UNRELEASED + + if path1_updated: + factories.write_workspace_file(workspace, path1, "changed1") + # refresh workspace + workspace = bll.get_workspace("test1", airlock_client.user) + policies.check_can_update_file_on_request(workspace, path1) + + if path2_updated: + factories.write_workspace_file(workspace, path2, "changed1") + # refresh workspace + workspace = bll.get_workspace("test1", airlock_client.user) + policies.check_can_update_file_on_request(workspace, path2) + + response = airlock_client.post( + "/workspaces/multiselect/test1", + data={ + "action": "update_files", + "selected": [ + "test/path1.txt", + "test/path2.txt", + ], + "next_url": workspace.get_url("test/path1.txt"), + }, + ) + + assert response.status_code == 200 + assert "test/path1.txt" in response.rendered_content + assert "test/path2.txt" in response.rendered_content + assert response.rendered_content.count("file cannot be updated") == ignored_count + + def test_workspace_multiselect_add_released_file_not_valid(airlock_client, bll): airlock_client.login(workspaces=["test1"]) workspace = factories.create_workspace("test1") @@ -930,6 +1028,96 @@ def test_workspace_request_file_invalid_formset(airlock_client, bll): assert "At least one form must be completed" in message.message +def test_workspace_request_update_file_invalid_status(airlock_client, bll): + airlock_client.login(workspaces=["test1"]) + workspace = factories.create_workspace("test1") + factories.write_workspace_file(workspace, "test/path.txt") + + assert bll.get_current_request(workspace.name, airlock_client.user) is None + response = airlock_client.post( + "/workspaces/update-file-in-request/test1", + data={ + "form-TOTAL_FORMS": "1", + "form-INITIAL_FORMS": "1", + "form-0-file": "test/path.txt", + "next_url": workspace.get_url("test/path.txt"), + }, + follow=True, + ) + assert response.status_code == 200 + + all_messages = [msg for msg in response.context["messages"]] + assert len(all_messages) == 1 + message = all_messages[0] + assert message.level == messages.ERROR + assert ( + "Cannot update file in request if it is not updated on disk" in message.message + ) + + +def test_workspace_request_update_file_request_path_does_not_exist(airlock_client): + airlock_client.login(workspaces=["test1"]) + workspace = factories.create_workspace("test1") + + response = airlock_client.post( + "/workspaces/update-file-in-request/test1", + data={ + "form-TOTAL_FORMS": "1", + "form-INITIAL_FORMS": "1", + "form-0-file": "test/path.txt", + "next_url": workspace.get_url("test/path.txt"), + }, + ) + + assert response.status_code == 404 + + +def test_workspace_request_update_file_invalid_formset(airlock_client, bll): + airlock_client.login(workspaces=["test1"]) + + workspace = factories.create_workspace("test1") + factories.write_workspace_file(workspace, "test/path.txt") + + response = airlock_client.post( + "/workspaces/update-file-in-request/test1", + data={ + "form-TOTAL_FORMS": "1", + "form-INITIAL_FORMS": "1", + "next_url": workspace.get_url("test/path.txt"), + }, + follow=True, + ) + + all_messages = [msg for msg in response.context["messages"]] + assert len(all_messages) == 1 + message = all_messages[0] + assert message.level == messages.ERROR + assert "file: This field is required" in message.message + + +def test_workspace_request_update_file_empty_formset(airlock_client, bll): + airlock_client.login(workspaces=["test1"]) + + workspace = factories.create_workspace("test1") + factories.write_workspace_file(workspace, "test/path.txt") + + response = airlock_client.post( + "/workspaces/update-file-in-request/test1", + data={ + "form-TOTAL_FORMS": "0", + "form-INITIAL_FORMS": "0", + "next_url": workspace.get_url("test/path.txt"), + }, + follow=True, + ) + + all_messages = [msg for msg in response.context["messages"]] + assert len(all_messages) == 1 + message = all_messages[0] + assert message.level == messages.ERROR + assert "At least one form must be completed." in message.message + + @pytest.mark.parametrize( "urlpath,post_data", [