From d840da4454a7471cda6eb0687db7484955562565 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Fri, 8 Mar 2024 14:32:25 +0000 Subject: [PATCH 1/7] Tell pytest where our tests are It would scan the entire tree otherwise, which can be slow. Also, tweak coverage to ignore all assets files --- pyproject.toml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 6d48f296..76698d65 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,13 +22,16 @@ isort.lines-after-imports = 2 [tool.pytest.ini_options] DJANGO_SETTINGS_MODULE = "airlock.settings" +testpaths = [ + "tests" +] [tool.coverage.run] branch = true # Required to get full coverage when using Playwright concurrency = ["greenlet", "thread"] plugins = ["django_coverage_plugin"] -omit = ["*/assets/templates/*",] +omit = ["*/assets/*",] [tool.coverage.report] fail_under = 100 From 69332b019e84b1959372bcd53e6ad21be72b7208 Mon Sep 17 00:00:00 2001 From: bloodearnest Date: Fri, 8 Mar 2024 14:33:54 +0000 Subject: [PATCH 2/7] Optimise the partial rendering of contents. This has two parts. Firstly, we avoid loading the whole tree, and just load a partial tree containing just the nodes on the selected path. This was actually fairly simple to do with our path walking code, by passing a list containing just the selected_path. Secondly, we only render the contents we need, i.e. not the tree, headers or other parts of the UI. To do this we split out the contents part into its own template, `file_browser/contents.html`, and just render that. With a large 45k node file tree, whilst the initial page load is still slow, the subsequent partial loads are much faster. --- airlock/file_browser_api.py | 38 +++++++-- airlock/templates/file_browser/contents.html | 89 +++++++++++++++++++ airlock/templates/file_browser/index.html | 90 +------------------- airlock/views/request.py | 12 ++- airlock/views/workspace.py | 11 ++- tests/integration/views/test_request.py | 16 +++- tests/integration/views/test_workspace.py | 12 +++ tests/unit/test_file_browser_api.py | 36 ++++++++ 8 files changed, 203 insertions(+), 101 deletions(-) create mode 100644 airlock/templates/file_browser/contents.html diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index dce3db3e..bc68223e 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -208,13 +208,20 @@ def build_string(node, indent): return "\n".join(build_string(self, "")) -def get_workspace_tree(workspace, selected_path=ROOT_PATH): - """Recursively build a workspace tree from files on disk.""" +def get_workspace_tree(workspace, selected_path=ROOT_PATH, selected_only=False): + """Recursively build workspace tree from the root dir. + + If selected_only==True, only build the selected path, not the entire tree.""" selected_path = UrlPath(selected_path) root = workspace.root() - # list all files in one go is much faster than walking the tree - pathlist = [p.relative_to(root) for p in root.glob("**/*")] + + if selected_only: + pathlist = [selected_path] + else: + # listing all files in one go is much faster than walking the tree + pathlist = [p.relative_to(root) for p in root.glob("**/*")] + root_node = PathItem( container=workspace, relpath=ROOT_PATH, @@ -230,7 +237,7 @@ def get_workspace_tree(workspace, selected_path=ROOT_PATH): return root_node -def get_request_tree(release_request, selected_path=ROOT_PATH): +def get_request_tree(release_request, selected_path=ROOT_PATH, selected_only=False): """Build a tree recursively for a ReleaseRequest For each group, we create a node for that group, and then build a sub-tree @@ -261,9 +268,19 @@ def get_request_tree(release_request, selected_path=ROOT_PATH): expanded=selected or expanded, ) + pathlist = [f.relpath for f in group.files] + + if selected_only: + if expanded: + # remove group + relpath = selected_path.relative_to(selected_path.parts[0]) + pathlist = [relpath] + else: + pathlist = [] + group_node.children = get_path_tree( release_request, - pathlist=[f.relpath for f in group.files], + pathlist=pathlist, parent=group_node, selected_path=selected_path, expanded=expanded, @@ -274,7 +291,14 @@ def get_request_tree(release_request, selected_path=ROOT_PATH): return root_node -def get_path_tree(container, pathlist, parent, selected_path=ROOT_PATH, expanded=False): +def get_path_tree( + container, + pathlist, + parent, + selected_path=ROOT_PATH, + expanded=False, + selected_only=False, +): """Walk a flat list of paths and create a tree from them.""" def build_path_tree(path_parts, parent): diff --git a/airlock/templates/file_browser/contents.html b/airlock/templates/file_browser/contents.html new file mode 100644 index 00000000..96f7bced --- /dev/null +++ b/airlock/templates/file_browser/contents.html @@ -0,0 +1,89 @@ +
+ + {% if path_item.is_directory %} + + {% #card title=path_item.name %} + {% #list_group %} + {% if not path_item.children %} + {% list_group_empty icon=True title="Empty Directory" %} + {% else %} + {% for entry in path_item.children %} + {% #list_group_item href=entry.url %} + {{ entry.name}} + {% if entry.is_directory %} + {% icon_folder_outline class="h-6 w-6 text-slate-600 inline" %} + {% endif %} + {% /list_group_item %} + {% endfor %} + {% endif %} + {% /list_group %} + {% /card %} + + {% else %} + {% fragment as add_button %} + {% if context == "workspace" %} + {% if form %} + {% #modal id="addRequestFile" button_text="Add File to Request" variant="success" %} + {% #card container=True title="Add a file" %} +
+ {% 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 %} + +
+ {% #button type="submit" variant="success" id="add-file-button" %}Add File to Request{% /button %} + {% #button variant="danger" type="cancel" %}Cancel{% /button %} +
+
+ {% /card %} + {% /modal %} + {% elif file_in_request %} + {% #button type="button" disabled=True tooltip="This file has already been added to the current request" 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 + {% /button %} + {% endif %} + {% elif is_author %} +
+ {% csrf_token %} + {% #button type="submit" tooltip="Remove this file from this request" variant="warning" %}Remove File from Request{% /button %} +
+ {% elif is_output_checker %} + {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %} + {% endif %} + {% endfragment %} + + {% #card title=path_item.name container=True custom_button=add_button %} + +
+ {% if path_item.display_type == "iframe" %} + + {% elif path_item.display_type == "image" %} +
+ +
+ {% elif path_item.display_type == "table" %} +
+ {% include "file_browser/csv.html" with contents=path_item.contents %} +
+ {% else %} +
+ 
{{ path_item.contents }}
+
+ {% endif %} +
+ {% /card %} + + {% endif %} + +
diff --git a/airlock/templates/file_browser/index.html b/airlock/templates/file_browser/index.html index 9c6a944e..dd6b93cc 100644 --- a/airlock/templates/file_browser/index.html +++ b/airlock/templates/file_browser/index.html @@ -132,95 +132,7 @@
-
- - {% if path_item.is_directory %} - - {% #card title=path_item.name %} - {% #list_group %} - {% if not path_item.children %} - {% list_group_empty icon=True title="Empty Directory" %} - {% else %} - {% for entry in path_item.children %} - {% #list_group_item href=entry.url %} - {{ entry.name}} - {% if entry.is_directory %} - {% icon_folder_outline class="h-6 w-6 text-slate-600 inline" %} - {% endif %} - {% /list_group_item %} - {% endfor %} - {% endif %} - {% /list_group %} - {% /card %} - - {% else %} - {% fragment as add_button %} - {% if context == "workspace" %} - {% if form %} - {% #modal id="addRequestFile" button_text="Add File to Request" variant="success" %} - {% #card container=True title="Add a file" %} -
- {% 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 %} - -
- {% #button type="submit" variant="success" id="add-file-button" %}Add File to Request{% /button %} - {% #button variant="danger" type="cancel" %}Cancel{% /button %} -
-
- {% /card %} - {% /modal %} - {% elif file_in_request %} - {% #button type="button" disabled=True tooltip="This file has already been added to the current request" 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 - {% /button %} - {% endif %} - {% elif is_author %} -
- {% csrf_token %} - {% #button type="submit" tooltip="Remove this file from this request" variant="warning" %}Remove File from Request{% /button %} -
- {% elif is_output_checker %} - {% #button variant="primary" type="link" href=path_item.download_url id="download-button" %}Download file{% /button %} - {% endif %} - {% endfragment %} - - {% #card title=path_item.name container=True custom_button=add_button %} - -
- {% if path_item.display_type == "iframe" %} - - {% elif path_item.display_type == "image" %} -
- -
- {% elif path_item.display_type == "table" %} -
- {% include "file_browser/csv.html" with contents=path_item.contents %} -
- {% else %} -
- 
{{ path_item.contents }}
-
- {% endif %} -
- {% /card %} - - {% endif %} - -
+ {% include "file_browser/contents.html" %}
diff --git a/airlock/views/request.py b/airlock/views/request.py index f8b11a7f..960607e1 100644 --- a/airlock/views/request.py +++ b/airlock/views/request.py @@ -38,7 +38,15 @@ def request_index(request): def request_view(request, request_id: str, path: str = ""): release_request = validate_release_request(request.user, request_id) - tree = get_request_tree(release_request, path) + template = "file_browser/index.html" + selected_only = False + + if request.htmx: + template = "file_browser/contents.html" + selected_only = True + + tree = get_request_tree(release_request, path, selected_only) + try: path_item = tree.get_path(path) except tree.PathNotFound: @@ -81,7 +89,7 @@ def request_view(request, request_id: str, path: str = ""): "release_files_url": release_files_url, } - return TemplateResponse(request, "file_browser/index.html", context) + return TemplateResponse(request, template, context) @require_http_methods(["GET"]) diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index 1ca19bf8..17e1ee95 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -24,7 +24,14 @@ def workspace_index(request): def workspace_view(request, workspace_name: str, path: str = ""): workspace = validate_workspace(request.user, workspace_name) - tree = get_workspace_tree(workspace, path) + template = "file_browser/index.html" + selected_only = False + + if request.htmx: + template = "file_browser/contents.html" + selected_only = True + + tree = get_workspace_tree(workspace, path, selected_only) try: path_item = tree.get_path(path) @@ -58,7 +65,7 @@ def workspace_view(request, workspace_name: str, path: str = ""): return TemplateResponse( request, - "file_browser/index.html", + template, { "workspace": workspace, "root": tree, diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index 012748aa..e31d4f98 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -53,7 +53,21 @@ def test_request_view_with_file(client_with_permission): f"/requests/view/{release_request.id}/group/file.txt" ) assert response.status_code == 200 - assert "group" in response.rendered_content + assert "foobar" in response.rendered_content + assert response.template_name == "file_browser/index.html" + + +def test_request_view_with_file_htmx(client_with_permission): + release_request = factories.create_release_request("workspace") + factories.write_request_file(release_request, "group", "file.txt", "foobar") + response = client_with_permission.get( + f"/requests/view/{release_request.id}/group/file.txt", + headers={"HX-Request": "true"}, + ) + assert response.status_code == 200 + assert "foobar" in response.rendered_content + assert response.template_name == "file_browser/contents.html" + assert '