feat: final post-review cleanup

terraso_backend/apps/graphql/schema/schema.graphql

@@ -1642,11 +1642,11 @@ type Mutations {
   deleteUserFromProject(input: ProjectDeleteUserMutationInput!): ProjectDeleteUserMutationPayload!
   updateUserRoleInProject(input: ProjectUpdateUserRoleMutationInput!): ProjectUpdateUserRoleMutationPayload!
   markProjectSeen(input: ProjectMarkSeenMutationInput!): ProjectMarkSeenMutationPayload!
-  updateSoilData(input: SoilDataUpdateMutationInput!): SoilDataUpdateMutationPayload!
-  updateDepthDependentSoilData(input: DepthDependentSoilDataUpdateMutationInput!): DepthDependentSoilDataUpdateMutationPayload!
+  updateSoilData(input: SoilDataUpdateMutationInput!): SoilDataUpdateMutationPayload! @deprecated(reason: "Use push_soil_data instead.")
+  updateDepthDependentSoilData(input: DepthDependentSoilDataUpdateMutationInput!): DepthDependentSoilDataUpdateMutationPayload! @deprecated(reason: "Use push_soil_data instead.")
   pushSoilData(input: SoilDataPushInput!): SoilDataPushPayload!
-  updateSoilDataDepthInterval(input: SoilDataUpdateDepthIntervalMutationInput!): SoilDataUpdateDepthIntervalMutationPayload!
-  deleteSoilDataDepthInterval(input: SoilDataDeleteDepthIntervalMutationInput!): SoilDataDeleteDepthIntervalMutationPayload!
+  updateSoilDataDepthInterval(input: SoilDataUpdateDepthIntervalMutationInput!): SoilDataUpdateDepthIntervalMutationPayload! @deprecated(reason: "Use push_soil_data instead.")
+  deleteSoilDataDepthInterval(input: SoilDataDeleteDepthIntervalMutationInput!): SoilDataDeleteDepthIntervalMutationPayload! @deprecated(reason: "Use push_soil_data instead.")
   updateProjectSoilSettings(input: ProjectSoilSettingsUpdateMutationInput!): ProjectSoilSettingsUpdateMutationPayload!
   updateProjectSoilSettingsDepthInterval(input: ProjectSoilSettingsUpdateDepthIntervalMutationInput!): ProjectSoilSettingsUpdateDepthIntervalMutationPayload!
   deleteProjectSoilSettingsDepthInterval(input: ProjectSoilSettingsDeleteDepthIntervalMutationInput!): ProjectSoilSettingsDeleteDepthIntervalMutationPayload!
@@ -2291,7 +2291,7 @@ type SoilDataPushEntry {
 union SoilDataPushEntryResult = SoilDataPushEntrySuccess | SoilDataPushEntryFailure
 
 type SoilDataPushEntrySuccess {
-  site: SiteNode!
+  soilData: SoilDataNode!
 }
 
 type SoilDataPushEntryFailure {

terraso_backend/apps/graphql/schema/schema.py

@@ -190,11 +190,19 @@ class Mutations(graphene.ObjectType):
     delete_user_from_project = ProjectDeleteUserMutation.Field()
     update_user_role_in_project = ProjectUpdateUserRoleMutation.Field()
     mark_project_seen = ProjectMarkSeenMutation.Field()
-    update_soil_data = SoilDataUpdateMutation.Field()
-    update_depth_dependent_soil_data = DepthDependentSoilDataUpdateMutation.Field()
+    update_soil_data = SoilDataUpdateMutation.Field(
+        deprecation_reason="Use push_soil_data instead."
+    )
+    update_depth_dependent_soil_data = DepthDependentSoilDataUpdateMutation.Field(
+        deprecation_reason="Use push_soil_data instead."
+    )
     push_soil_data = SoilDataPush.Field()
-    update_soil_data_depth_interval = SoilDataUpdateDepthIntervalMutation.Field()
-    delete_soil_data_depth_interval = SoilDataDeleteDepthIntervalMutation.Field()
+    update_soil_data_depth_interval = SoilDataUpdateDepthIntervalMutation.Field(
+        deprecation_reason="Use push_soil_data instead."
+    )
+    delete_soil_data_depth_interval = SoilDataDeleteDepthIntervalMutation.Field(
+        deprecation_reason="Use push_soil_data instead."
+    )
     update_project_soil_settings = ProjectSoilSettingsUpdateMutation.Field()
     update_project_soil_settings_depth_interval = (
         ProjectSoilSettingsUpdateDepthIntervalMutation.Field()

terraso_backend/apps/soil_id/graphql/soil_data/push_mutation.py

@@ -7,10 +7,10 @@
 
 from apps.core.models.users import User
 from apps.graphql.schema.commons import BaseWriteMutation
-from apps.graphql.schema.sites import SiteNode
 from apps.project_management.models.sites import Site
 from apps.project_management.permission_rules import Context
 from apps.project_management.permission_table import SiteAction, check_site_permission
+from apps.soil_id.graphql.soil_data.queries import SoilDataNode
 from apps.soil_id.graphql.soil_data.types import (
     SoilDataDepthDependentInputs,
     SoilDataDepthIntervalInputs,
@@ -24,10 +24,9 @@
 
 
 class SoilDataPushEntrySuccess(graphene.ObjectType):
-    site = graphene.Field(SiteNode, required=True)
+    soil_data = graphene.Field(SoilDataNode, required=True)
 
 
-# TODO: just a generic "can't access" result?
 class SoilDataPushFailureReason(graphene.Enum):
     DOES_NOT_EXIST = "DOES_NOT_EXIST"
     NOT_ALLOWED = "NOT_ALLOWED"
@@ -73,6 +72,14 @@ class SoilDataPushInputEntry(graphene.InputObjectType):
     soil_data = graphene.Field(graphene.NonNull(SoilDataPushInputSoilData))
 
 
+# NOTE: we check if the user has all permissions required to edit a site,
+#       rather than checking individual permissions against which data has been modified
+# NOTE: we catch errors at the granularity of each site in the request.
+#       so one site's updates can succeed while another fails. but if any of
+#       an individual site's updates are invalid, we reject all of that site's updates
+# NOTE: changing a depth interval preset causes all depth intervals for that site to be
+#       deleted. we haven't yet thought through the implications of when/whether to apply
+#       that change in the context of this mutation
 class SoilDataPush(BaseWriteMutation):
     results = graphene.Field(graphene.List(graphene.NonNull(SoilDataPushEntry)), required=True)
 
@@ -82,7 +89,7 @@ class Input:
         )
 
     @staticmethod
-    def record_soil_data_push(user: User, soil_data_entries: list[dict]) -> list[SoilDataHistory]:
+    def log_soil_data_push(user: User, soil_data_entries: list[dict]) -> list[SoilDataHistory]:
         history_entries = []
 
         for entry in soil_data_entries:
@@ -96,15 +103,15 @@ def record_soil_data_push(user: User, soil_data_entries: list[dict]) -> list[Soi
         return history_entries
 
     @staticmethod
-    def record_entry_failure(
+    def log_soil_data_push_entry_failure(
         history_entry: SoilDataHistory, reason: SoilDataPushFailureReason, site_id: str
     ):
         history_entry.update_failure_reason = reason.value
         history_entry.save()
         return SoilDataPushEntry(site_id=site_id, result=SoilDataPushEntryFailure(reason=reason))
 
     @staticmethod
-    def get_valid_site_for_soil_update(user: User, site_id: str):
+    def validate_site_for_soil_update(user: User, site_id: str):
         site = Site.objects.filter(id=site_id).first()
 
         if site is None:
@@ -122,7 +129,7 @@ def get_valid_site_for_soil_update(user: User, site_id: str):
         return site, None
 
     @staticmethod
-    def save_soil_data(site: Site, soil_data: dict):
+    def update_soil_data(site: Site, soil_data: dict):
         if (
             "depth_interval_preset" in soil_data
             and soil_data["depth_interval_preset"] != site.soil_data.depth_interval_preset
@@ -134,7 +141,7 @@ def save_soil_data(site: Site, soil_data: dict):
         )
 
     @staticmethod
-    def save_depth_dependent_data(site: Site, depth_dependent_data: list[dict]):
+    def update_depth_dependent_data(site: Site, depth_dependent_data: list[dict]):
         for depth_dependent_entry in depth_dependent_data:
             interval = depth_dependent_entry.pop("depth_interval")
             depth_interval, _ = site.soil_data.depth_dependent_data.get_or_create(
@@ -167,7 +174,9 @@ def delete_depth_intervals(site: Site, deleted_depth_intervals: list[dict]):
             ).delete()
 
     @staticmethod
-    def get_entry_result(user: User, soil_data_entry: dict, history_entry: SoilDataHistory):
+    def mutate_and_get_entry_result(
+        user: User, soil_data_entry: dict, history_entry: SoilDataHistory
+    ):
         site_id = soil_data_entry["site_id"]
         soil_data = soil_data_entry["soil_data"]
 
@@ -176,25 +185,25 @@ def get_entry_result(user: User, soil_data_entry: dict, history_entry: SoilDataH
         deleted_depth_intervals = soil_data.pop("deleted_depth_intervals")
 
         try:
-            site, reason = SoilDataPush.get_valid_site_for_soil_update(user=user, site_id=site_id)
+            site, reason = SoilDataPush.validate_site_for_soil_update(user=user, site_id=site_id)
             if site is None:
-                return SoilDataPush.record_entry_failure(
+                return SoilDataPush.log_soil_data_push_entry_failure(
                     history_entry=history_entry,
                     site_id=site_id,
                     reason=reason,
                 )
 
-            SoilDataPush.save_soil_data(site, soil_data)
+            SoilDataPush.update_soil_data(site, soil_data)
             SoilDataPush.update_depth_intervals(site, depth_intervals)
-            SoilDataPush.save_depth_dependent_data(site, depth_dependent_data)
+            SoilDataPush.update_depth_dependent_data(site, depth_dependent_data)
             SoilDataPush.delete_depth_intervals(site, deleted_depth_intervals)
 
             history_entry.update_succeeded = True
             history_entry.save()
             return SoilDataPushEntry(site_id=site_id, result=SoilDataPushEntrySuccess(site=site))
 
         except (ValidationError, IntegrityError):
-            return SoilDataPush.record_entry_failure(
+            return SoilDataPush.log_soil_data_push_entry_failure(
                 history_entry=history_entry,
                 site_id=site_id,
                 reason=SoilDataPushFailureReason.INVALID_DATA,
@@ -206,12 +215,12 @@ def mutate_and_get_payload(cls, root, info, soil_data_entries: list[dict]):
         user = info.context.user
 
         with transaction.atomic():
-            history_entries = SoilDataPush.record_soil_data_push(user, soil_data_entries)
+            history_entries = SoilDataPush.log_soil_data_push(user, soil_data_entries)
 
         with transaction.atomic():
             for entry, history_entry in zip(soil_data_entries, history_entries):
                 results.append(
-                    SoilDataPush.get_entry_result(
+                    SoilDataPush.mutate_and_get_entry_result(
                         user=user, soil_data_entry=entry, history_entry=history_entry
                     )
                 )

terraso_backend/apps/soil_id/models/soil_data.py

@@ -22,32 +22,14 @@
 from apps.soil_id.models.depth_interval import BaseDepthInterval
 
 
+# THIS IS NO LONGER USED/MAINTAINED, just exists to satisfy migration 0004
 def default_depth_intervals():
     return []
 
 
+# THIS IS NO LONGER USED/MAINTAINED, just exists to satisfy migration 0004
 def validate_depth_intervals(intervals):
-    if not isinstance(intervals, list):
-        raise ValidationError(f"Depth intervals must be list, got {intervals}")
-    for index, interval in enumerate(intervals):
-        if not isinstance(interval, dict) or len(interval) != 2:
-            raise ValidationError(f"Depth interval must be two element dict, got {interval}")
-        for field in ["start", "end"]:
-            if field not in interval or not isinstance(interval[field], int):
-                raise ValidationError(
-                    f"Depth interval {field} must exist and be integer, got {interval[field]}"
-                )
-        if interval["start"] < 0 or interval["end"] > 200:
-            raise ValidationError(f"Depth interval must be between 0 and 200, got {interval}")
-        if interval["start"] >= interval["end"]:
-            raise ValidationError(f"Depth interval start must be less than end, got {interval}")
-        if index + 1 < len(intervals) and interval["end"] > intervals[index + 1]["start"]:
-            raise ValidationError(
-                f"""
-                Depth interval must end at or before next interval,
-                got {interval} followed by {intervals[index + 1]}
-                """
-            )
+    pass
 
 
 class SoilData(BaseModel):

terraso_backend/apps/soil_id/models/soil_data_history.py

@@ -20,6 +20,12 @@
 from apps.project_management.models.sites import Site
 
 
+# NOTE: this table may contain data associated with sites that was submitted
+#       by unauthorized (but still authenticated) users. such requests may have
+#       an update_failure_reason of null or "NOT_ALLOWED". unless a user is
+#       handcrafting malicious requests, this will be because a user had
+#       authorization to enter data for a site, then went offline and made
+#       changes simultaneous to losing authorization to enter data for that site
 class SoilDataHistory(BaseModel):
     site = models.ForeignKey(Site, null=True, on_delete=models.CASCADE)
     changed_by = models.ForeignKey(User, on_delete=models.CASCADE)

terraso_backend/tests/graphql/mutations/test_soil_data.py

@@ -961,7 +961,6 @@ def test_push_soil_data_can_process_mixed_results(client, user):
     assert history_3.soil_data_changes["slope_aspect"] == 15
 
 
-# TODO: fleshly out
 def test_push_soil_data_success(client, user):
     site = mixer.blend(Site, owner=user)