From d679c5b5dab496112f53c6c15a2ba678a06ea0c2 Mon Sep 17 00:00:00 2001 From: shrouxm Date: Tue, 15 Oct 2024 17:00:13 -0700 Subject: [PATCH] update implementation and tests to match new API --- .../apps/graphql/schema/commons.py | 37 +++-- .../apps/graphql/schema/schema.graphql | 2 +- .../graphql/soil_id/soil_data/types.py | 149 +++++++++++------- .../apps/soil_id/models/soil_data_history.py | 19 ++- .../tests/graphql/mutations/test_soil_data.py | 46 +++--- 5 files changed, 158 insertions(+), 95 deletions(-) diff --git a/terraso_backend/apps/graphql/schema/commons.py b/terraso_backend/apps/graphql/schema/commons.py index c3a9a5988..714206425 100644 --- a/terraso_backend/apps/graphql/schema/commons.py +++ b/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 diff --git a/terraso_backend/apps/graphql/schema/schema.graphql b/terraso_backend/apps/graphql/schema/schema.graphql index 40c151a80..d5fcbae0c 100644 --- a/terraso_backend/apps/graphql/schema/schema.graphql +++ b/terraso_backend/apps/graphql/schema/schema.graphql @@ -2366,7 +2366,7 @@ enum SoilDataPushFailureReason { } input SoilDataPushInput { - soilData: [SoilDataPushInputEntry!]! + soilDataEntries: [SoilDataPushInputEntry!]! clientMutationId: String } diff --git a/terraso_backend/apps/soil_id/graphql/soil_id/soil_data/types.py b/terraso_backend/apps/soil_id/graphql/soil_id/soil_data/types.py index 77651a08a..400d64274 100644 --- a/terraso_backend/apps/soil_id/graphql/soil_id/soil_data/types.py +++ b/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,107 @@ 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"] - + 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 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 + history_entry = SoilDataHistory( + site=site, changed_by=user, soil_data_changes=changes + ) + history_entry.save() + history_entries.append(history_entry) - if not hasattr(site, "soil_data"): - site.soil_data = SoilData() + return history_entries - for attr, value in entry["soil_data"].items(): - if isinstance(value, enum.Enum): - value = value.value - setattr(site.soil_data, attr, value) + @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)) - site.soil_data.save() + @staticmethod + def get_valid_site_for_soil_update(user: User, site_id: str): + site = Site.objects.filter(id=site_id).first() - 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"], - ) + if site is None: + return None, SoilDataPushFailureReason.DOES_NOT_EXIST + + if not check_site_permission(user, SiteAction.ENTER_DATA, Context(site=site)): + return None, SoilDataPushFailureReason.NOT_ALLOWED + + if not hasattr(site, "soil_data"): + site.soil_data = SoilData() - for attr, value in depth_interval_input.items(): - if isinstance(value, enum.Enum): - value = value.value - setattr(depth_interval, attr, value) + return site, None - depth_interval_input["depth_interval"] = interval + @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, + ) - depth_interval.save() + 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 + + 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) diff --git a/terraso_backend/apps/soil_id/models/soil_data_history.py b/terraso_backend/apps/soil_id/models/soil_data_history.py index 3f9fc15bb..cf42981bd 100644 --- a/terraso_backend/apps/soil_id/models/soil_data_history.py +++ b/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() diff --git a/terraso_backend/tests/graphql/mutations/test_soil_data.py b/terraso_backend/tests/graphql/mutations/test_soil_data.py index 83b6f2b75..fb588c3d5 100644 --- a/terraso_backend/tests/graphql/mutations/test_soil_data.py +++ b/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,30 +815,40 @@ 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": [], + }, }, ] }, @@ -846,7 +856,7 @@ def test_bulk_update(client, user): ) 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