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:
@@ -119,25 +126,25 @@ def get_valid_site_for_soil_update(user: User, site_id: str):
         if not hasattr(site, "soil_data"):
             site.soil_data = SoilData()
 
-        return site, None
+        return site.soil_data, None
 
     @staticmethod
-    def save_soil_data(site: Site, soil_data: dict):
+    def update_soil_data(soil_data: SoilData, update_data: dict):
         if (
-            "depth_interval_preset" in soil_data
-            and soil_data["depth_interval_preset"] != site.soil_data.depth_interval_preset
+            "depth_interval_preset" in update_data
+            and update_data["depth_interval_preset"] != soil_data.depth_interval_preset
         ):
-            site.soil_data.depth_intervals.all().delete()
+            soil_data.depth_intervals.all().delete()
 
         BaseWriteMutation.assign_graphql_fields_to_model_instance(
-            model_instance=site.soil_data, fields=soil_data
+            model_instance=soil_data, fields=update_data
         )
 
     @staticmethod
-    def save_depth_dependent_data(site: Site, depth_dependent_data: list[dict]):
+    def update_depth_dependent_data(soil_data: SoilData, 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(
+            depth_interval, _ = soil_data.depth_dependent_data.get_or_create(
                 depth_interval_start=interval["start"],
                 depth_interval_end=interval["end"],
             )
@@ -147,10 +154,10 @@ def save_depth_dependent_data(site: Site, depth_dependent_data: list[dict]):
             )
 
     @staticmethod
-    def update_depth_intervals(site: Site, depth_intervals: list[dict]):
+    def update_depth_intervals(soil_data: SoilData, depth_intervals: list[dict]):
         for depth_interval_input in depth_intervals:
             interval_input = depth_interval_input.pop("depth_interval")
-            depth_interval, _ = site.soil_data.depth_intervals.get_or_create(
+            depth_interval, _ = soil_data.depth_intervals.get_or_create(
                 depth_interval_start=interval_input["start"],
                 depth_interval_end=interval_input["end"],
             )
@@ -160,41 +167,47 @@ def update_depth_intervals(site: Site, depth_intervals: list[dict]):
             )
 
     @staticmethod
-    def delete_depth_intervals(site: Site, deleted_depth_intervals: list[dict]):
+    def delete_depth_intervals(soil_data: SoilData, deleted_depth_intervals: list[dict]):
         for interval in deleted_depth_intervals:
-            site.soil_data.depth_intervals.filter(
+            soil_data.depth_intervals.filter(
                 depth_interval_start=interval["start"], depth_interval_end=interval["end"]
             ).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"]
+        update_data = soil_data_entry["soil_data"]
 
-        depth_dependent_data = soil_data.pop("depth_dependent_data")
-        depth_intervals = soil_data.pop("depth_intervals")
-        deleted_depth_intervals = soil_data.pop("deleted_depth_intervals")
+        depth_dependent_data = update_data.pop("depth_dependent_data")
+        depth_intervals = update_data.pop("depth_intervals")
+        deleted_depth_intervals = update_data.pop("deleted_depth_intervals")
 
         try:
-            site, reason = SoilDataPush.get_valid_site_for_soil_update(user=user, site_id=site_id)
-            if site is None:
-                return SoilDataPush.record_entry_failure(
+            soil_data, reason = SoilDataPush.validate_site_for_soil_update(
+                user=user, site_id=site_id
+            )
+            if soil_data is None:
+                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_depth_intervals(site, depth_intervals)
-            SoilDataPush.save_depth_dependent_data(site, depth_dependent_data)
-            SoilDataPush.delete_depth_intervals(site, deleted_depth_intervals)
+            SoilDataPush.update_soil_data(soil_data, update_data)
+            SoilDataPush.update_depth_intervals(soil_data, depth_intervals)
+            SoilDataPush.update_depth_dependent_data(soil_data, depth_dependent_data)
+            SoilDataPush.delete_depth_intervals(soil_data, deleted_depth_intervals)
 
             history_entry.update_succeeded = True
             history_entry.save()
-            return SoilDataPushEntry(site_id=site_id, result=SoilDataPushEntrySuccess(site=site))
+            return SoilDataPushEntry(
+                site_id=site_id, result=SoilDataPushEntrySuccess(soil_data=soil_data)
+            )
 
         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 +219,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_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)