update implementation and tests to match new API

terraso_backend/apps/graphql/schema/commons.py

@@ -15,11 +15,11 @@
 
 import enum
 import json
-from typing import Optional
+from typing import Any, Optional
 
 import structlog
 from django.core.exceptions import NON_FIELD_ERRORS, ValidationError
-from django.db import IntegrityError
+from django.db import IntegrityError, models
 from graphene import Connection, Int, relay
 from graphene.types.generic import GenericScalar
 from graphql import get_nullable_type
@@ -191,7 +191,7 @@ def get_logger(cls):
 
 
 class BaseWriteMutation(BaseAuthenticatedMutation, LoggerMixin):
-    skip_field_validation: Optional[str] = None
+    skip_field_validation: Optional[list[str]] = None
 
     @classmethod
     def mutate_and_get_payload(cls, root, info, **kwargs):
@@ -215,16 +215,12 @@ def mutate_and_get_payload(cls, root, info, **kwargs):
         result_class = cls.result_class or cls.model_class
         result_instance = kwargs.pop("result_instance", model_instance)
 
-        for attr, value in kwargs.items():
-            if isinstance(value, enum.Enum):
-                value = value.value
-            setattr(model_instance, attr, value)
-
         try:
-            kwargs = {}
-            if cls.skip_field_validation is not None:
-                kwargs["exclude"] = cls.skip_field_validation
-            model_instance.full_clean(**kwargs)
+            BaseWriteMutation.assign_graphql_fields_to_model(
+                model_instance=model_instance,
+                fields=kwargs,
+                skip_field_validation=cls.skip_field_validation,
+            )
         except ValidationError as exc:
             logger.info(
                 "Attempt to mutate an model, but it's invalid",
@@ -233,9 +229,6 @@ def mutate_and_get_payload(cls, root, info, **kwargs):
             raise GraphQLValidationException.from_validation_error(
                 exc, model_name=cls.model_class.__name__
             )
-
-        try:
-            model_instance.save()
         except IntegrityError as exc:
             logger.info(
                 "Attempt to mutate an model, but it's not unique",
@@ -264,6 +257,20 @@ def mutate_and_get_payload(cls, root, info, **kwargs):
     def is_update(cls, data):
         return "id" in data
 
+    @staticmethod
+    def assign_graphql_fields_to_model_instance(
+        model_instance: models.Model,
+        fields: dict[str, Any],
+        skip_field_validation: Optional[list[str]] = None,
+    ):
+        for attr, value in fields.items():
+            if isinstance(value, enum.Enum):
+                value = value.value
+            setattr(model_instance, attr, value)
+
+        model_instance.full_clean(exclude=skip_field_validation)
+        model_instance.save()
+
     @staticmethod
     def remove_null_fields(kwargs, options=[str]):
         """It seems like for some fields, if the frontend does not pass an argument, the

terraso_backend/apps/graphql/schema/schema.graphql

@@ -2366,7 +2366,7 @@ enum SoilDataPushFailureReason {
 }
 
 input SoilDataPushInput {
-  soilData: [SoilDataPushInputEntry!]!
+  soilDataEntries: [SoilDataPushInputEntry!]!
   clientMutationId: String
 }
 

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

@@ -1,9 +1,12 @@
+import copy
 import enum
 
 import graphene
 import structlog
-from django.db import transaction
+from django.db import IntegrityError, transaction
+from django.forms import ValidationError
 
+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
@@ -29,7 +32,7 @@ class SoilDataPushEntrySuccess(graphene.ObjectType):
 class SoilDataPushFailureReason(graphene.Enum):
     DOES_NOT_EXIST = "DOES_NOT_EXIST"
     NOT_ALLOWED = "NOT_ALLOWED"
-    INTEGRITY_ERROR = "INTEGRITY_ERROR"
+    INVALID_DATA = "INVALID_DATA"
 
 
 class SoilDataPushEntryFailure(graphene.ObjectType):
@@ -75,79 +78,106 @@ class SoilDataPush(BaseWriteMutation):
     results = graphene.Field(graphene.List(graphene.NonNull(SoilDataPushEntry)), required=True)
 
     class Input:
-        soil_data = graphene.Field(
+        soil_data_entries = graphene.Field(
             graphene.List(graphene.NonNull(SoilDataPushInputEntry)), required=True
         )
 
-    @classmethod
-    def mutate_and_get_payload(cls, root, info, soil_data):
-        # TODO: refactor spaghetti mutation logic re: history, split into smaller functions
-        results = []
+    @staticmethod
+    def record_update(user: User, soil_data_entries: list[dict]) -> list[SoilDataHistory]:
+        history_entries = []
 
-        with transaction.atomic():
-            for entry in soil_data:
-                site_id = entry.pop("site_id")
-                depth_intervals = entry["depth_dependent_data"]
-
-                site = Site.objects.filter(id=site_id).first()
-
-                if site is None:
-                    results.append(
-                        SoilDataPushEntry(
-                            site_id=site_id,
-                            result=SoilDataPushEntryFailure(
-                                reason=SoilDataPushFailureReason.DOES_NOT_EXIST
-                            ),
-                        )
-                    )
-                    continue
-
-                user = info.context.user
-                if not check_site_permission(user, SiteAction.ENTER_DATA, Context(site=site)):
-                    results.append(
-                        SoilDataPushEntry(
-                            site_id=site_id,
-                            result=SoilDataPushEntryFailure(
-                                site=site, reason=SoilDataPushFailureReason.NOT_ALLOWED
-                            ),
-                        )
-                    )
-                    continue
+        for entry in soil_data_entries:
+            changes = copy.deepcopy(entry)
+            site_id = changes.pop("site_id")
+            site = Site.objects.filter(id=site_id).first()
 
-                if not hasattr(site, "soil_data"):
-                    site.soil_data = SoilData()
+            history_entry = SoilDataHistory(site=site, changed_by=user, soil_data_changes=changes)
+            history_entry.save()
+            history_entries.append(history_entry)
 
-                for attr, value in entry["soil_data"].items():
-                    if isinstance(value, enum.Enum):
-                        value = value.value
-                    setattr(site.soil_data, attr, value)
+        return history_entries
 
-                site.soil_data.save()
+    @staticmethod
+    def record_update_failure(
+        history_entry: SoilDataHistory, reason: SoilDataPushFailureReason, site_id: str
+    ):
+        history_entry.update_failure_reason = reason
+        history_entry.save()
+        return SoilDataPushEntry(site_id=site_id, result=SoilDataPushEntryFailure(reason=reason))
 
-                for depth_interval_input in depth_intervals:
-                    interval = depth_interval_input.pop("depth_interval")
-                    depth_interval, _ = site.soil_data.depth_dependent_data.get_or_create(
-                        depth_interval_start=interval["start"],
-                        depth_interval_end=interval["end"],
-                    )
+    @staticmethod
+    def get_valid_site_for_soil_update(user: User, site_id: str):
+        site = Site.objects.filter(id=site_id).first()
+
+        if site is None:
+            return None, SoilDataPushFailureReason.DOES_NOT_EXIST
 
-                    for attr, value in depth_interval_input.items():
-                        if isinstance(value, enum.Enum):
-                            value = value.value
-                        setattr(depth_interval, attr, value)
+        if not check_site_permission(user, SiteAction.ENTER_DATA, Context(site=site)):
+            return None, SoilDataPushFailureReason.NOT_ALLOWED
 
-                    depth_interval_input["depth_interval"] = interval
+        if not hasattr(site, "soil_data"):
+            site.soil_data = SoilData()
 
-                    depth_interval.save()
+        return site, None
 
+    @staticmethod
+    def 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"]
+
+        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")
+
+        try:
+            site, reason = SoilDataPush.get_valid_site_for_soil_update(user=user, site_id=site_id)
+            if site is None:
+                return SoilDataPush.record_update_failure(
+                    history_entry=history_entry,
+                    site_id=site_id,
+                    reason=reason,
+                )
+
+            BaseWriteMutation.assign_graphql_fields_to_model_instance(
+                model_instance=site.soil_data, fields=soil_data
+            )
+
+            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_start=interval["start"],
+                    depth_interval_end=interval["end"],
+                )
+
+                BaseWriteMutation.assign_graphql_fields_to_model_instance(
+                    model_instance=depth_interval, fields=depth_dependent_entry
+                )
+
+            history_entry.update_succeeded = True
+            history_entry.save()
+            return SoilDataPushEntry(site_id=site_id, result=SoilDataPushEntrySuccess(site=site))
+
+        except (ValidationError, IntegrityError):
+            return SoilDataPush.record_update_failure(
+                history_entry=history_entry,
+                site_id=site_id,
+                reason=SoilDataPushFailureReason.INVALID_DATA,
+            )
+
+    @classmethod
+    def mutate_and_get_payload(cls, root, info, soil_data_entries: list[dict]):
+        results = []
+        user = info.context.user
+
+        with transaction.atomic():
+            history_entries = SoilDataPush.record_update(user, soil_data_entries)
+
+        with transaction.atomic():
+            for entry, history_entry in zip(soil_data_entries, history_entries):
                 results.append(
-                    SoilDataPushEntry(
-                        site_id=site_id,
-                        result=SoilDataPushEntrySuccess(site=site),
+                    SoilDataPush.get_entry_result(
+                        user=user, soil_data_entry=entry, history_entry=history_entry
                     )
                 )
 
-                history_entry = SoilDataHistory(site=site, changed_by=user, soil_data_changes=entry)
-                history_entry.save()
-
         return cls(results=results)

terraso_backend/apps/soil_id/models/soil_data_history.py

@@ -21,17 +21,32 @@
 
 
 class SoilDataHistory(BaseModel):
-    site = models.ForeignKey(Site, on_delete=models.CASCADE)
+    site = models.ForeignKey(Site, null=True, on_delete=models.CASCADE)
     changed_by = models.ForeignKey(User, on_delete=models.CASCADE)
+    update_succeeded = models.BooleanField(null=False, blank=False, default=False)
+    update_failure_reason = models.TextField(null=True)
 
     # intended JSON schema: {
     #   ...soilDataInputs,
+    #   "depth_dependent_data": [{
+    #     "depth_interval": {
+    #       "start": number,
+    #       "end": number
+    #     },
+    #     ...depthDependentInputs
+    #   }],
     #   "depth_intervals": [{
     #     "depth_interval": {
     #       "start": number,
     #       "end": number
     #     },
-    #     ...depthIntervalInputs,
+    #     ...depthIntervalConfig
+    #   }],
+    #   "deleted_depth_intervals": [{
+    #     "depth_interval": {
+    #       "start": number,
+    #       "end": number
+    #     }
     #   }]
     # }
     soil_data_changes = models.JSONField()

terraso_backend/tests/graphql/mutations/test_soil_data.py

@@ -783,17 +783,17 @@ def test_apply_to_all(client, project_site, project_manager):
         assert interval.soil_texture_enabled
 
 
-BULK_UPDATE_QUERY = """
-    mutation BulkSoilDataUpdateMutation($input: SoilDataBulkUpdateInput!) {
-        bulkUpdateSoilData(input: $input) {
+PUSH_SOIL_DATA_QUERY = """
+    mutation PushSoilDataMutation($input: SoilDataPushInput!) {
+        pushSoilData(input: $input) {
             results {
                 siteId
                 result {
                     __typename
-                    ... on SoilDataBulkUpdateFailure {
+                    ... on SoilDataPushEntryFailure {
                         reason
                     }
-                    ... on SoilDataBulkUpdateSuccess {
+                    ... on SoilDataPushEntrySuccess {
                         site {
                             soilData {
                                 slopeAspect
@@ -815,38 +815,48 @@ def test_apply_to_all(client, project_site, project_manager):
 """
 
 
-def test_bulk_update(client, user):
+def test_push_soil_data(client, user):
     sites = mixer.cycle(2).blend(Site, owner=user)
 
     client.force_login(user)
     response = graphql_query(
-        BULK_UPDATE_QUERY,
+        PUSH_SOIL_DATA_QUERY,
         input_data={
-            "soilData": [
+            "soilDataEntries": [
                 {
                     "siteId": str(sites[0].id),
-                    "soilData": {"slopeAspect": 10},
-                    "depthDependentData": [],
+                    "soilData": {
+                        "slopeAspect": 10,
+                        "depthDependentData": [],
+                        "depthIntervals": [],
+                        "deletedDepthIntervals": [],
+                    },
                 },
                 {
                     "siteId": str(sites[1].id),
-                    "soilData": {},
-                    "depthDependentData": [
-                        {"depthInterval": {"start": 0, "end": 10}, "clayPercent": 5}
-                    ],
+                    "soilData": {
+                        "depthDependentData": [
+                            {"depthInterval": {"start": 0, "end": 10}, "clayPercent": 5}
+                        ],
+                        "depthIntervals": [],
+                        "deletedDepthIntervals": [],
+                    },
                 },
                 {
                     "siteId": str("c9df7deb-6b9d-4c55-8ba6-641acc47dbb2"),
-                    "soilData": {},
-                    "depthDependentData": [],
+                    "soilData": {
+                        "depthDependentData": [],
+                        "depthIntervals": [],
+                        "deletedDepthIntervals": [],
+                    },
                 },
             ]
         },
         client=client,
     )
 
     print(response.json())
-    result = response.json()["data"]["bulkUpdateSoilData"]
+    result = response.json()["data"]["pushSoilData"]
     assert result["errors"] is None
     assert result["results"][2]["result"]["reason"] == "DOES_NOT_EXIST"
 
@@ -867,4 +877,4 @@ def test_bulk_update(client, user):
     assert history_1.soil_data_changes["soil_data"]["slope_aspect"] == 10
 
     history_2 = SoilDataHistory.objects.get(site=sites[1])
-    assert history_2.soil_data_changes["depth_dependent_data"][0]["clay_percent"] == 5
+    assert history_2.soil_data_changes["soil_data"]["depth_dependent_data"][0]["clay_percent"] == 5