From e5a3a8e29cf9e0bf7149bd9dfba4ada616564fbc Mon Sep 17 00:00:00 2001
From: David Lougheed <david.lougheed@gmail.com>
Date: Mon, 30 Sep 2024 14:39:58 -0400
Subject: [PATCH] address review comments

---
 .../chord/tests/test_api.py                   | 26 +++++++++++++------
 chord_metadata_service/restapi/utils.py       |  6 +----
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/chord_metadata_service/chord/tests/test_api.py b/chord_metadata_service/chord/tests/test_api.py
index 212c826e0..e7c5676d7 100644
--- a/chord_metadata_service/chord/tests/test_api.py
+++ b/chord_metadata_service/chord/tests/test_api.py
@@ -192,15 +192,25 @@ def test_dats_as_attachment(self):
         self.assertEqual(r.status_code, status.HTTP_201_CREATED)
         dataset_id = Dataset.objects.first().identifier
 
-        response = self.client.get(f"/api/datasets/{dataset_id}/dats?attachment=true")
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertDictEqual(response.data, payload['dats_file'])
-        self.assertEqual(response.headers["Content-Disposition"], f"attachment; filename=\"{dataset_id}_dats.json\"")
+        subtest_params = [
+            ("?attachment=true", True),
+            ("?attachment=false", False),
+            ("?attachment=", False),
+            ("", False),
+        ]
 
-        response = self.client.get(f"/api/datasets/{dataset_id}/dats?attachment=false")
-        self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertDictEqual(response.data, payload['dats_file'])
-        self.assertNotIn("Content-Disposition", response.headers)
+        for params in subtest_params:
+            with self.subTest(params=params):
+                response = self.client.get(f"/api/datasets/{dataset_id}/dats{params[0]}")
+                self.assertEqual(response.status_code, status.HTTP_200_OK)
+                self.assertDictEqual(response.data, payload['dats_file'])
+                if params[1]:
+                    self.assertEqual(
+                        response.headers["Content-Disposition"],
+                        f"attachment; filename=\"{dataset_id}_dats.json\""
+                    )
+                else:
+                    self.assertNotIn("Content-Disposition", response.headers)
 
     def test_resources(self):
         resource = {
diff --git a/chord_metadata_service/restapi/utils.py b/chord_metadata_service/restapi/utils.py
index 29c63a823..442d3d303 100644
--- a/chord_metadata_service/restapi/utils.py
+++ b/chord_metadata_service/restapi/utils.py
@@ -11,8 +11,6 @@
 from chord_metadata_service.phenopackets.models import Biosample
 
 __all__ = [
-    "COMPUTED_PROPERTY_PREFIX",
-    "camel_case_field_names",
     "transform_keys",
     "computed_property",
     "remove_computed_properties",
@@ -25,6 +23,7 @@
 
 
 COMPUTED_PROPERTY_PREFIX = "__"
+FILENAME_REPLACE_PATTERN = re.compile(r"[\\/:*?\"<>|]")
 
 
 def camel_case_field_names(string) -> str:
@@ -117,9 +116,6 @@ def response_as_attachment(request: DrfRequest) -> bool:
     return request.query_params.get("attachment", "").strip().lower() in ("1", "true", "yes")
 
 
-FILENAME_REPLACE_PATTERN = re.compile(r"[\\/:*?\"<>|]")
-
-
 def attachment_content_disposition(filename: str) -> dict[str, str]:
     filename_safe = FILENAME_REPLACE_PATTERN.sub("_", filename)
     return {"Content-Disposition": f"attachment; filename=\"{filename_safe}\""}