Merge pull request #97 from opensafely-core/filegroup-dict

Refactor filegroups to be a dict

Author: bloodearnest

airlock/api.py

@@ -124,7 +124,7 @@ class ReleaseRequest(AirlockContainer):
     author: str
     created_at: datetime
     status: Status = Status.PENDING
-    filegroups: list[FileGroup] = field(default_factory=list)
+    filegroups: dict[FileGroup] = field(default_factory=dict)
 
     def __post_init__(self):
         self.root().mkdir(parents=True, exist_ok=True)

airlock/forms.py

@@ -9,7 +9,7 @@ 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}
+            self.filegroup_names = release_request.filegroups.keys()
         else:
             self.filegroup_names = set()
         group_choices = {

airlock/templates/file_browser/index.html

@@ -129,7 +129,7 @@
         </div>
         {% #modal id="viewFileGroups" %}
           {% #card container=True title="File Groups for this request" %}
-            {% for group in release_request.filegroups %}
+            {% for group in release_request.filegroups.values %}
               {{ group.name}}
               {% #list_group %}
               {% for file in group.files %}

local_db/api.py

@@ -51,10 +51,10 @@ def _filegroup(self, filegroup_metadata: FileGroupMetadata):
         )
 
     def _get_filegroups(self, metadata: RequestMetadata):
-        return [
-            self._filegroup(group_metadata)
+        return {
+            group_metadata.name: self._filegroup(group_metadata)
             for group_metadata in metadata.filegroups.all()
-        ]
+        }
 
     def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str):
         metadata = self._find_metadata(request_id)

tests/integration/test_views.py

@@ -144,7 +144,7 @@ def test_workspace_request_file_creates(client_with_user, api):
     assert response.status_code == 302
 
     release_request = api.get_current_request(workspace.name, user)
-    filegroup = release_request.filegroups[0]
+    filegroup = release_request.filegroups["default"]
     assert filegroup.name == "default"
     assert str(filegroup.files[0].relpath) == "test/path.txt"
     assert release_request.abspath("test/path.txt").exists()
@@ -157,7 +157,7 @@ def test_workspace_request_file_request_already_exists(client_with_user, api):
     workspace = factories.create_workspace("test1")
     factories.write_workspace_file(workspace, "test/path.txt")
     release_request = factories.create_release_request(workspace, user)
-    assert release_request.filegroups == []
+    assert release_request.filegroups == {}
 
     response = client.post(
         "/workspaces/add-file-to-request/test1",
@@ -167,7 +167,7 @@ def test_workspace_request_file_request_already_exists(client_with_user, api):
     current_release_request = api.get_current_request(workspace.name, user)
     assert current_release_request.id == release_request.id
     assert release_request.abspath("test/path.txt").exists()
-    filegroup = current_release_request.filegroups[0]
+    filegroup = current_release_request.filegroups["default"]
     assert filegroup.name == "default"
     assert str(filegroup.files[0].relpath) == "test/path.txt"
 
@@ -192,7 +192,7 @@ def test_workspace_request_file_with_new_filegroup(client_with_user, api):
     assert response.status_code == 302
 
     release_request = api.get_current_request(workspace.name, user)
-    filegroup = release_request.filegroups[0]
+    filegroup = release_request.filegroups["new_group"]
     assert filegroup.name == "new_group"
 
 

tests/unit/test_api.py

@@ -288,28 +288,28 @@ def setup_empty_release_request():
 
 def test_release_request_filegroups_with_no_files(api):
     release_request, _, _ = setup_empty_release_request()
-    assert release_request.filegroups == []
+    assert release_request.filegroups == {}
 
 
 def test_release_request_filegroups_default_filegroup(api):
     release_request, path, author = setup_empty_release_request()
-    assert release_request.filegroups == []
+    assert release_request.filegroups == {}
     release_request = api.add_file_to_request(release_request, path, author)
     assert len(release_request.filegroups) == 1
-    filegroup = release_request.filegroups[0]
+    filegroup = release_request.filegroups["default"]
     assert filegroup.name == "default"
     assert len(filegroup.files) == 1
     assert filegroup.files[0].relpath == path
 
 
 def test_release_request_filegroups_named_filegroup(api):
     release_request, path, author = setup_empty_release_request()
-    assert release_request.filegroups == []
+    assert release_request.filegroups == {}
     release_request = api.add_file_to_request(
         release_request, path, author, "test_group"
     )
     assert len(release_request.filegroups) == 1
-    filegroup = release_request.filegroups[0]
+    filegroup = release_request.filegroups["test_group"]
     assert filegroup.name == "test_group"
     assert len(filegroup.files) == 1
     assert filegroup.files[0].relpath == path
@@ -335,7 +335,7 @@ def test_release_request_filegroups_multiple_filegroups(api):
 
     release_request_files = {
         filegroup.name: [file.relpath for file in filegroup.files]
-        for filegroup in release_request.filegroups
+        for filegroup in release_request.filegroups.values()
     }
 
     assert release_request_files == {
@@ -346,10 +346,10 @@ def test_release_request_filegroups_multiple_filegroups(api):
 
 def test_release_request_add_same_file(api):
     release_request, path, author = setup_empty_release_request()
-    assert release_request.filegroups == []
+    assert release_request.filegroups == {}
     release_request = api.add_file_to_request(release_request, path, author)
     assert len(release_request.filegroups) == 1
-    assert len(release_request.filegroups[0].files) == 1
+    assert len(release_request.filegroups["default"].files) == 1
 
     # Adding the same file again should not create a new RequestFile
     with pytest.raises(api.APIException):
@@ -362,4 +362,4 @@ def test_release_request_add_same_file(api):
     release_request = api.get_release_request(release_request.id)
     # No additional files or groups have been created
     assert len(release_request.filegroups) == 1
-    assert len(release_request.filegroups[0].files) == 1
+    assert len(release_request.filegroups["default"].files) == 1