Merge pull request #108 from opensafely-core/expand-request-groups

Expand all directories withing a filegroup

airlock/file_browser_api.py

@@ -40,8 +40,8 @@ class PathNotFound(Exception):
 
     # is this the currently selected path?
     selected: bool = False
-    # is this a parent of the currently selected path?
-    on_selected_path: bool = False
+    # should this node be expanded in the tree?
+    expanded: bool = False
 
     # relative filepath on disk. defaults to path, but allows us to customise it.
     filepath: UrlPath = None
@@ -130,10 +130,6 @@ def html_classes(self):
 
         return " ".join(classes)
 
-    def is_open(self):
-        """Should this node be expanded"""
-        return self.selected or self.on_selected_path
-
     def get_path(self, relpath):
         """Walk the tree and return the PathItem for relpath.
 
@@ -171,7 +167,7 @@ def walk_selected(node):
                 if child.selected:
                     return child
 
-                if child.on_selected_path:
+                if child.expanded:
                     return walk_selected(child)
 
             raise self.PathNotFound("No selected path found")
@@ -182,7 +178,7 @@ def __str__(self, render_selected=False):
         """Debugging utility to inspect tree."""
 
         def build_string(node, indent):
-            yield f"{indent}{node.name()}{'*' if node.on_selected_path else ''}{'**' if node.selected else ''}"
+            yield f"{indent}{node.name()}{'*' if node.expanded else ''}{'**' if node.selected else ''}"
             for child in node.children:
                 yield from build_string(child, indent + "  ")
 
@@ -193,12 +189,13 @@ def get_workspace_tree(workspace, selected_path=ROOT_PATH):
     """Recursively build a workspace tree from files on disk."""
 
     def build_workspace_tree(path, parent):
+        selected = path == selected_path
         node = PathItem(
             container=workspace,
             relpath=path,
             parent=parent,
+            selected=selected,
         )
-        set_selected(node, selected_path)
 
         if node._absolute_path().is_dir():
             if path == ROOT_PATH:
@@ -215,6 +212,7 @@ def build_workspace_tree(path, parent):
                 for child in node._absolute_path().iterdir()
             ]
             node.children.sort(key=children_sort_key)
+            node.expanded = selected or (path in (selected_path.parents or []))
         else:
             node.type = PathType.FILE
 
@@ -238,33 +236,41 @@ def get_request_tree(release_request, selected_path=ROOT_PATH):
         relpath=ROOT_PATH,
         type=PathType.REQUEST,
         parent=None,
+        selected=(selected_path == ROOT_PATH),
+        expanded=True,
     )
-    set_selected(root_node, selected_path)
 
     for name, group in release_request.filegroups.items():
+        group_path = UrlPath(name)
+        selected = group_path == selected_path
+        expanded = selected or (group_path in (selected_path.parents or []))
         group_node = PathItem(
             container=release_request,
             relpath=UrlPath(name),
             type=PathType.FILEGROUP,
             parent=root_node,
             display_text=f"{name} ({len(group.files)} files)",
+            selected=selected,
+            expanded=selected or expanded,
         )
-        set_selected(group_node, selected_path)
 
         group_node.children = get_filegroup_tree(
             release_request,
             selected_path,
             group,
             group_node.relpath,
             parent=group_node,
+            expanded=expanded,
         )
 
         root_node.children.append(group_node)
 
     return root_node
 
 
-def get_filegroup_tree(container, selected_path, group_data, group_path, parent):
+def get_filegroup_tree(
+    container, selected_path, group_data, group_path, parent, expanded
+):
     """Get the tree for a filegroup's files.
 
     This is more than just a walk the disk. The FileGroup.files is a flat list of
@@ -290,16 +296,16 @@ def build_filegroup_tree(file_parts, path, parent):
         for child, descendants in grouped.items():
             child_path = path / child
 
+            selected = child_path == selected_path
             node = PathItem(
                 container=container,
                 relpath=child_path,
                 parent=parent,
                 # actual path on disk, striping the group part
                 filepath=child_path.relative_to(child_path.parts[0]),
+                selected=selected,
             )
 
-            set_selected(node, selected_path)
-
             abspath = node._absolute_path()
             assert abspath.exists()
 
@@ -310,6 +316,8 @@ def build_filegroup_tree(file_parts, path, parent):
                 node.children = build_filegroup_tree(
                     descendants, child_path, parent=node
                 )
+                node.expanded = expanded
+
             else:
                 assert abspath.is_file()
                 node.type = PathType.FILE
@@ -324,13 +332,6 @@ def build_filegroup_tree(file_parts, path, parent):
     return build_filegroup_tree(file_parts, group_path, parent)
 
 
-def set_selected(node, selected_path):
-    if node.relpath == selected_path:
-        node.selected = True
-    if node.relpath in (selected_path.parents or []):
-        node.on_selected_path = True
-
-
 def children_sort_key(node):
     """Sort children first by directory, then files."""
     # this works because True == 1 and False == 0

airlock/templates/file_browser/tree.html

@@ -1,7 +1,7 @@
 {% for child in path.children %}
 <li class="tree ">
   {% if child.is_directory %}
-  <details {% if child.is_open %}open{% endif %}>
+  <details {% if child.expanded %}open{% endif %}>
     <summary>
       <a class="{{ child.html_classes }}" href="{{ child.url }}">{{ child.display }}</a>
     </summary>

tests/unit/test_file_browser_api.py

@@ -201,24 +201,31 @@ def test_workspace_tree_selection_root(workspace):
     assert tree.get_selected() == tree
 
 
-def test_workspace_tree_selection_path(workspace):
+def test_workspace_tree_selection_path_file(workspace):
     selected_path = UrlPath("some_dir/file_a.txt")
     tree = get_workspace_tree(workspace, selected_path)
 
     selected_item = tree.get_path(selected_path)
     assert selected_item.selected
-    assert not selected_item.on_selected_path
-    assert selected_item.is_open()
+    assert not selected_item.expanded
 
     parent_item = tree.get_path("some_dir")
     assert not parent_item.selected
-    assert parent_item.on_selected_path
-    assert parent_item.is_open()
+    assert parent_item.expanded
 
     other_item = tree.get_path("some_dir/file_b.txt")
     assert not other_item.selected
-    assert not other_item.on_selected_path
-    assert not other_item.is_open()
+    assert not other_item.expanded
+
+
+def test_workspace_tree_selection_path_dir(workspace):
+    selected_path = UrlPath("some_dir")
+    tree = get_workspace_tree(workspace, selected_path)
+
+    selected_item = tree.get_path(selected_path)
+    assert selected_item.selected
+    # selected dir *is* expanded
+    assert selected_item.expanded
 
 
 def test_workspace_tree_selection_bad_path(workspace):
@@ -242,18 +249,15 @@ def test_request_tree_selection_path(release_request):
 
     selected_item = tree.get_path(selected_path)
     assert selected_item.selected
-    assert not selected_item.on_selected_path
-    assert selected_item.is_open()
+    assert not selected_item.expanded
 
     parent_item = tree.get_path("group1/some_dir")
     assert not parent_item.selected
-    assert parent_item.on_selected_path
-    assert parent_item.is_open()
+    assert parent_item.expanded
 
     other_item = tree.get_path("group2/some_dir/file_b.txt")
     assert not other_item.selected
-    assert not other_item.on_selected_path
-    assert not other_item.is_open()
+    assert not other_item.expanded
 
 
 @pytest.mark.django_db