Merge pull request #94 from opensafely-core/tree

Add tree base navigation UI

Author: bloodearnest

airlock/file_browser_api.py

@@ -21,6 +21,11 @@ class PathItem:
     container: AirlockContainer
     relpath: pathlib.Path
 
+    # The currently selected view path in this container.  Used to calculate if
+    # how relpath relates to it, i.e. if relpath *is* the currently selected
+    # path, or is one of its parents.
+    selected: pathlib.Path = None
+
     def __post_init__(self):
         # ensure relpath is a Path
         object.__setattr__(self, "relpath", pathlib.Path(self.relpath))
@@ -45,14 +50,19 @@ def url(self):
 
     def parent(self):
         if self.relpath.parents:
-            return PathItem(self.container, self.relpath.parent)
+            return PathItem(self.container, self.relpath.parent, selected=self.selected)
 
     def children(self):
+        if not self.is_directory():
+            return []
         root = self.container.root()
-        return [
-            PathItem(self.container, child.relative_to(root))
+        children = [
+            PathItem(self.container, child.relative_to(root), selected=self.selected)
             for child in self._absolute_path().iterdir()
         ]
+        # directories first, then alphabetical, aks what windows does
+        children.sort(key=lambda p: (not p.is_directory(), p.relpath.name))
+        return children
 
     def siblings(self):
         if not self.relpath.parents:
@@ -63,10 +73,12 @@ def siblings(self):
     def contents(self):
         return self._absolute_path().read_text()
 
-    @property
     def suffix(self):
         return self.relpath.suffix
 
+    def file_type(self):
+        return self.suffix().lstrip(".")
+
     def breadcrumbs(self):
         item = self
         crumbs = [item]
@@ -79,3 +91,32 @@ def breadcrumbs(self):
 
         crumbs.reverse()
         return crumbs
+
+    def html_classes(self):
+        """Semantic html classes for this PathItem.
+
+        Currently, only "selected" is used, but it made sense to be able to
+        distinguish file/dirs, and maybe even file types, in the UI, in case we
+        need to.
+        """
+        classes = []
+
+        if self.is_directory():
+            classes.append("dir")
+        else:
+            classes.append("file")
+            classes.append(self.file_type())
+
+        if self.is_selected():
+            classes.append("selected")
+
+        return " ".join(classes)
+
+    def is_selected(self):
+        return self.relpath == self.selected
+
+    def is_on_selected_path(self):
+        return self.relpath in self.selected.parents
+
+    def is_open(self):
+        return self.is_selected() or self.is_on_selected_path()

airlock/settings.py

@@ -244,3 +244,6 @@ def run_connection_init_queries(*, connection, **kwargs):
     messages.WARNING: "alert-warning",
     messages.ERROR: "alert-danger",
 }
+
+# Temporary UI featureflag
+TREE = True

airlock/templates/file_browser/index.html

@@ -2,6 +2,51 @@
 
 {% load django_vite %}
 
+{% block extra_styles %}
+<style>
+
+/* tree styles */
+ul.tree {
+  list-style: none;
+
+  details ul {
+    border-left: 1px dotted grey;
+    padding-left: 0.25rem;
+    margin-left: 0.25rem;
+  }
+}
+
+
+.tree summary {
+  cursor: pointer;
+}
+
+.tree .selected {
+    font-weight: bold;
+    color: green;
+    background: rgba(0, 255, 0, 0.1);
+    cursor: pointer;
+}
+
+
+/* ths attempt at a nice selectedhighlight doesn't quite work
+.tree li {
+  position: relative;
+}
+
+&::before {
+  background: rgba(0, 255, 0, 0.1);
+  content: "";
+  position: absolute;
+  left: -100vw;
+  height: 100%;
+  width: 100vw;
+}
+*/
+
+</style>
+{% endblock extra_styles %}
+
 {% block full_width_content %}
 
 {% fragment as action_button %}
@@ -50,25 +95,32 @@
   <div style="flex-basis: 25%">
 
     {% #card %}
-      {% #list_group %}
-        {% if not path_item.parent %}
-          {% list_group_empty icon=True title=context|title|add:" Root" %}   
-        {% else %}
-          {% #list_group_item href=path_item.parent.url %}
-            ↰ ..
-          {% /list_group_item %}
-          {% for entry in path_item.siblings %}
-            {% #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 %}
+      {% if tree %}
+        <ul class="tree root">
+        {% include "file_browser/tree.html" with path=root %}
+        </ul>
+      {% else %}
+        {% #list_group %}
+          {% if not path_item.parent %}
+            {% list_group_empty icon=True title=context|title|add:" Root" %}   
+          {% else %}
+            {% #list_group_item href=path_item.parent.url %}
+                ↰ ..
             {% /list_group_item %}
-          {% endfor %}
-        {% endif %}
-      {% /list_group %}
+            {% for entry in path_item.siblings %}
+                {% #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 %}
+      {% endif %}
+ 
 
-    {% #card_footer no_container=False %}
+      {% #card_footer no_container=False %}
       {% if context == "request" %}
         <div class="px-4">
           {% #button variant="primary-outline" type="button" data-modal="viewFileGroups" %}
@@ -92,7 +144,6 @@
         {% /modal %}
         {% endif %}
       {% /card_footer %}
-
   {% /card %}
 
   </div>

airlock/templates/file_browser/tree.html

@@ -0,0 +1,18 @@
+{% for child in path.children %}
+<li class="tree ">
+  {% if child.is_directory %}
+  <details {% if child.is_open %}open{% endif %}>
+    <summary>
+      <a class="{{ child.html_classes }}" href="{{ child.url }}">{{ child.name }}</a>
+    </summary>
+    {% if child.children %}
+      <ul class="tree">
+        {% include "file_browser/tree.html" with path=child %}
+      </ul>
+    {% endif %}
+  </details>
+  {% else %}
+  <a class="{{ child.html_classes }}" {% if not child.is_selected %}href="{{ child.url }}"{% endif %}>{{ child.name }}</a>
+  {% endif %}
+</li>
+{% endfor %}

airlock/views.py

@@ -118,10 +118,23 @@ def workspace_index(request):
     return TemplateResponse(request, "workspaces.html", {"workspaces": workspaces})
 
 
+def use_tree_ui(request):
+    """Quick hack to be able to dynamically switch ui options."""
+    tree = request.session.get("tree", settings.TREE)
+    # hack to switch UI dynamically
+    if "tree" in request.GET:  # pragma: nocover
+        tree = request.GET["tree"].lower() == "true"
+
+    request.session["tree"] = tree
+    return tree
+
+
 def workspace_view(request, workspace_name: str, path: str = ""):
     workspace = validate_workspace(request.user, workspace_name)
 
-    path_item = PathItem(workspace, path)
+    relpath = Path(path)
+    root = PathItem(workspace, Path("."), selected=relpath)
+    path_item = PathItem(workspace, path, selected=relpath)
 
     if not path_item.exists():
         raise Http404()
@@ -135,6 +148,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
         "file_browser/index.html",
         {
             "workspace": workspace,
+            "root": root,
             "path_item": path_item,
             "context": "workspace",
             "title": f"Files for workspace {workspace_name}",
@@ -145,6 +159,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
             "form": AddFileForm(
                 release_request=api.get_current_request(workspace_name, request.user)
             ),
+            "tree": use_tree_ui(request),
         },
     )
 
@@ -200,7 +215,9 @@ def request_index(request):
 def request_view(request, request_id: str, path: str = ""):
     release_request = validate_release_request(request.user, request_id)
 
-    path_item = PathItem(release_request, path)
+    relpath = Path(path)
+    root = PathItem(release_request, Path("."), selected=relpath)
+    path_item = PathItem(release_request, path, selected=relpath)
 
     if not path_item.exists():
         raise Http404()
@@ -230,6 +247,7 @@ def request_view(request, request_id: str, path: str = ""):
     context = {
         "workspace": api.get_workspace(release_request.workspace),
         "release_request": release_request,
+        "root": root,
         "path_item": path_item,
         "context": "request",
         "title": f"Request for {release_request.workspace} by {release_request.author}",
@@ -239,6 +257,7 @@ def request_view(request, request_id: str, path: str = ""):
         "request_submit_url": request_submit_url,
         "request_reject_url": request_reject_url,
         "release_files_url": release_files_url,
+        "tree": use_tree_ui(request),
     }
 
     return TemplateResponse(request, "file_browser/index.html", context)

tests/integration/test_views.py

@@ -34,7 +34,13 @@ def client_with_permission(client_with_user):
     yield client_with_user(output_checker)
 
 
-def test_workspace_view(client_with_permission):
+# temporary fixture to test both UI options until we decide on one.
+@pytest.fixture(params=[True, False])
+def ui_options(request, settings):
+    settings.TREE = request.param
+
+
+def test_workspace_view(client_with_permission, ui_options):
     factories.write_workspace_file("workspace", "file.txt")
 
     response = client_with_permission.get("/workspaces/view/workspace/")
@@ -46,13 +52,13 @@ def test_workspace_does_not_exist(client_with_permission):
     assert response.status_code == 404
 
 
-def test_workspace_view_with_directory(client_with_permission):
+def test_workspace_view_with_directory(client_with_permission, ui_options):
     factories.write_workspace_file("workspace", "some_dir/file.txt")
     response = client_with_permission.get("/workspaces/view/workspace/some_dir/")
     assert "file.txt" in response.rendered_content
 
 
-def test_workspace_view_with_file(client_with_permission):
+def test_workspace_view_with_file(client_with_permission, ui_options):
     factories.write_workspace_file("workspace", "file.txt", "foobar")
     response = client_with_permission.get("/workspaces/view/workspace/file.txt")
     assert "foobar" in response.rendered_content
@@ -269,7 +275,7 @@ def test_request_index_no_user(client):
     assert response.status_code == 302
 
 
-def test_request_view_index(client_with_permission):
+def test_request_view_index(client_with_permission, ui_options):
     release_request = factories.create_release_request(
         "workspace", client_with_permission.user
     )
@@ -288,7 +294,7 @@ def test_request_id_does_not_exist(client_with_permission):
     assert response.status_code == 404
 
 
-def test_request_view_with_directory(client_with_permission):
+def test_request_view_with_directory(client_with_permission, ui_options):
     release_request = factories.create_release_request("workspace")
     factories.write_request_file(release_request, "some_dir/file.txt")
     response = client_with_permission.get(
@@ -297,7 +303,7 @@ def test_request_view_with_directory(client_with_permission):
     assert "file.txt" in response.rendered_content
 
 
-def test_request_view_with_file(client_with_permission):
+def test_request_view_with_file(client_with_permission, ui_options):
     release_request = factories.create_release_request("workspace")
     factories.write_request_file(release_request, "file.txt", "foobar")
     response = client_with_permission.get(

tests/unit/test_file_browser_api.py

@@ -103,6 +103,10 @@ def test_parent(container, path, parent_path):
             "some_dir",
             ["some_dir/file_a.txt", "some_dir/file_b.txt"],
         ),
+        (
+            "some_dir/file_a.txt",
+            [],
+        ),
     ],
 )
 def test_children(container, path, child_paths):
@@ -151,3 +155,21 @@ def test_breadcrumbs(container):
         PathItem(container, "foo/bar"),
         PathItem(container, "foo/bar/baz"),
     ]
+
+
+def test_selection_logic(container):
+    selected = Path("some_dir/file_a.txt")
+
+    selected_item = PathItem(container, selected, selected)
+    assert selected_item.is_selected()
+    assert selected_item.is_open()
+
+    parent_item = PathItem(container, "some_dir", selected)
+    assert not parent_item.is_selected()
+    assert parent_item.is_on_selected_path()
+    assert parent_item.is_open()
+
+    other_item = PathItem(container, "other_dir", selected)
+    assert not other_item.is_selected()
+    assert not other_item.is_on_selected_path()
+    assert not other_item.is_open()