cleanup TODOs and related minor changes

server/benchmark/serializers.py

@@ -41,19 +41,18 @@ class Meta:
         fields = "__all__"
 
     def update(self, instance, validated_data):
+        if "approval_status" in validated_data:
+            if validated_data["approval_status"] != instance.approval_status:
+                instance.approval_status = validated_data["approval_status"]
+                if instance.approval_status != "PENDING":
+                    instance.approved_at = timezone.now()
+        validated_data.pop("approval_status", None)
         for k, v in validated_data.items():
             setattr(instance, k, v)
-        # TODO: the condition below will run even
-        #       if a user edits the benchmark after it gets approved
-        if instance.approval_status != "PENDING":
-            instance.approved_at = timezone.now()
         instance.save()
         return instance
 
     def validate(self, data):
-        # TODO: define what should happen to existing assets when a benchmark
-        #       is rejected after being approved (associations? results? note also
-        #       that results submission doesn't check benchmark's approval status)
         if "approval_status" in data:
             if data["approval_status"] == "PENDING":
                 raise serializers.ValidationError(

server/benchmark/tests/test_pk.py

@@ -446,9 +446,6 @@ def test_approval_status_change_in_operation(
 class BenchmarkDeleteTest(BenchmarkTest):
     """Test module for DELETE /benchmarks/<pk>"""
 
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(BenchmarkDeleteTest, self).setUp()
         self.generic_setup()

server/benchmarkdataset/serializers.py

@@ -13,9 +13,6 @@ class Meta:
         fields = "__all__"
 
     def validate(self, data):
-        # TODO: define what should happen to existing assets when an association
-        #       is rejected after being approved (results)
-
         bid = self.context["request"].data.get("benchmark")
         dataset = self.context["request"].data.get("dataset")
         approval_status = self.context["request"].data.get("approval_status", "PENDING")

server/benchmarkmodel/serializers.py

@@ -13,8 +13,6 @@ class Meta:
         fields = "__all__"
 
     def validate(self, data):
-        # TODO: define what should happen to existing assets when an association
-        #       is rejected after being approved (results)
         bid = self.context["request"].data.get("benchmark")
         mlcube = self.context["request"].data.get("model_mlcube")
         approval_status = self.context["request"].data.get("approval_status", "PENDING")

server/dataset/tests/test_pk.py

@@ -236,9 +236,6 @@ def test_put_respects_unique_generated_uid(self):
     ]
 )
 class DatasetDeleteTest(DatasetTest):
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(DatasetDeleteTest, self).setUp()
         self.generic_setup()

server/dataset/tests/test_pk_benchmarks_bid.py

@@ -303,9 +303,6 @@ def test_put_works_on_latest_association(self):
     ]
 )
 class DatasetDeleteTest(DatasetTest):
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(DatasetDeleteTest, self).setUp()
         self.generic_setup()

server/dataset/urls.py

@@ -10,5 +10,5 @@
     path("benchmarks/", bviews.BenchmarkDatasetList.as_view()),
     path("<int:pk>/benchmarks/<int:bid>/", bviews.DatasetApproval.as_view()),
     # path("<int:pk>/benchmarks/", bviews.DatasetBenchmarksList.as_view()),
-    # TODO: when activating this endpoint later, check permissions and write tests
+    # NOTE: when activating this endpoint later, check permissions and write tests
 ]

server/medperf/tests.py

@@ -19,6 +19,8 @@
 class MedPerfTest(TestCase):
     """Common settings module for MedPerf APIs"""
 
+    # TODO: for all DELETE tests, we should revisit when we allow users
+    # to delete. We should test the effects of model.CASCADE and model.PROTECT
     def setUp(self):
         SIMPLE_JWT = {
             "ALGORITHM": "RS256",

server/mlcube/serializers.py

@@ -3,16 +3,16 @@
 
 
 def validate_optional_mlcube_components(data):
-    git_parameters_url = data["git_parameters_url"]
-    parameters_hash = data["parameters_hash"]
+    git_parameters_url = data.get("git_parameters_url", "")
+    parameters_hash = data.get("parameters_hash", "")
 
-    additional_files_tarball_url = data["additional_files_tarball_url"]
-    additional_files_tarball_hash = data["additional_files_tarball_hash"]
+    additional_files_tarball_url = data.get("additional_files_tarball_url", "")
+    additional_files_tarball_hash = data.get("additional_files_tarball_hash", "")
 
-    image_hash = data["image_hash"]
+    image_hash = data.get("image_hash", "")
 
-    image_tarball_url = data["image_tarball_url"]
-    image_tarball_hash = data["image_tarball_hash"]
+    image_tarball_url = data.get("image_tarball_url", "")
+    image_tarball_hash = data.get("image_tarball_hash", "")
 
     # validate nonblank parameters file hash
     if git_parameters_url and not parameters_hash:

server/mlcube/tests/test_.py

@@ -142,21 +142,29 @@ def test_creation_of_duplicate_mlcubes_with_image_tarball(self, field):
 
     def test_default_values_are_as_expected(self):
         """Testing the model fields rules"""
-        # TODO?: when fixing the unique_together constraint,
-        # add default values of URLs here
-
         # Arrange
         default_values = {
             "state": "DEVELOPMENT",
             "is_valid": True,
             "metadata": {},
             "user_metadata": {},
+            "git_parameters_url": "",
+            "image_tarball_url": "",
+            "additional_files_tarball_url": "",
         }
         testmlcube = self.mock_mlcube()
         for key in default_values:
             if key in testmlcube:
                 del testmlcube[key]
 
+        # in order to allow empty urls
+        testmlcube.update(
+            {
+                "parameters_hash": "",
+                "image_tarball_hash": "",
+                "additional_files_tarball_hash": "",
+            }
+        )
         # Act
         response = self.client.post(self.url, testmlcube, format="json")
 

server/mlcube/tests/test_benchmarks.py

@@ -152,7 +152,7 @@ def setUp(self):
 
     @parameterized.expand([("DEVELOPMENT",), ("OPERATION",)])
     def test_association_with_unapproved_benchmark(self, state):
-        # TODO: the serializer checks also if benchmark is operation
+        # NOTE: the serializer checks also if benchmark is operation
         #       however, an approved benchmark cannot be in development
         #       (i.e. there is a redundant check that we can't test)
 

server/mlcube/tests/test_pk.py

@@ -473,9 +473,6 @@ def test_put_doesnot_allow_clearing_image_tarball_hash_without_url(self, state):
     ]
 )
 class MlCubeDeleteTest(MlCubeTest):
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(MlCubeDeleteTest, self).setUp()
         self.generic_setup()

server/mlcube/tests/test_pk_benchmarks_bid.py

@@ -302,9 +302,6 @@ def test_put_works_on_latest_association(self):
     ]
 )
 class MlCubeDeleteTest(MlCubeTest):
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(MlCubeDeleteTest, self).setUp()
         self.generic_setup()

server/mlcube/urls.py

@@ -10,5 +10,5 @@
     path("benchmarks/", bviews.BenchmarkModelList.as_view()),
     path("<int:pk>/benchmarks/<int:bid>/", bviews.ModelApproval.as_view()),
     # path("<int:pk>/benchmarks/", bviews.ModelBenchmarksList.as_view()),
-    # TODO: when activating this endpoint later, check permissions and write tests
+    # NOTE: when activating this endpoint later, check permissions and write tests
 ]

server/result/serializers.py

@@ -53,7 +53,6 @@ def validate(self, data):
         return data
 
 
-# TODO: define what should be editable, how, and WHO can do that
 class ModelResultDetailSerializer(serializers.ModelSerializer):
     class Meta:
         model = ModelResult

server/result/tests/test_pk.py

@@ -173,9 +173,6 @@ def test_put_does_not_modify_readonly_fields(self):
 class ResultDeleteTest(ResultsTest):
     """Test module for DELETE /results/<pk>"""
 
-    # TODO: for all DELETE tests, we should revisit when we allow users
-    # to delete. We should test the effects of model.CASCADE and model.PROTECT
-
     def setUp(self):
         super(ResultDeleteTest, self).setUp()
         self.generic_setup()

server/result/views.py

@@ -45,7 +45,6 @@ class ModelResultDetail(GenericAPIView):
     serializer_class = ModelResultDetailSerializer
     queryset = ""
 
-    # TODO: fix delete permissions?
     def get_permissions(self):
         if self.request.method == "PUT" or self.request.method == "DELETE":
             self.permission_classes = [IsAdmin]

server/user/serializers.py

@@ -4,8 +4,6 @@
 User = get_user_model()
 
 
-# TODO: define what can be edited, and how. Keep in mind
-#       that some of these information is stored in Auth0
 class UserSerializer(serializers.ModelSerializer):
     class Meta:
         model = User