From 8360648b999d0d9bdc72b5a9ebab273f94501232 Mon Sep 17 00:00:00 2001 From: shrouxm Date: Thu, 31 Oct 2024 11:26:08 -0700 Subject: [PATCH] feat: final post-review cleanup --- .../apps/graphql/schema/schema.graphql | 10 +- terraso_backend/apps/graphql/schema/schema.py | 16 ++- .../graphql/soil_data/push_mutation.py | 81 ++++++----- .../apps/soil_id/models/soil_data_history.py | 6 + .../tests/graphql/mutations/test_soil_data.py | 126 +++++++++--------- 5 files changed, 132 insertions(+), 107 deletions(-) diff --git a/terraso_backend/apps/graphql/schema/schema.graphql b/terraso_backend/apps/graphql/schema/schema.graphql index eeee271cf..d60394b17 100644 --- a/terraso_backend/apps/graphql/schema/schema.graphql +++ b/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 { diff --git a/terraso_backend/apps/graphql/schema/schema.py b/terraso_backend/apps/graphql/schema/schema.py index 850a6b78d..514216e5c 100644 --- a/terraso_backend/apps/graphql/schema/schema.py +++ b/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() diff --git a/terraso_backend/apps/soil_id/graphql/soil_data/push_mutation.py b/terraso_backend/apps/soil_id/graphql/soil_data/push_mutation.py index 9eb3b54ba..b34334f06 100644 --- a/terraso_backend/apps/soil_id/graphql/soil_data/push_mutation.py +++ b/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,7 +103,7 @@ 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 @@ -104,7 +111,7 @@ def record_entry_failure( 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 ) ) 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 cf42981bd..58919c162 100644 --- a/terraso_backend/apps/soil_id/models/soil_data_history.py +++ b/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) diff --git a/terraso_backend/tests/graphql/mutations/test_soil_data.py b/terraso_backend/tests/graphql/mutations/test_soil_data.py index 07d97f952..61be4b438 100644 --- a/terraso_backend/tests/graphql/mutations/test_soil_data.py +++ b/terraso_backend/tests/graphql/mutations/test_soil_data.py @@ -794,69 +794,67 @@ def test_apply_to_all(client, project_site, project_manager): reason } ... on SoilDataPushEntrySuccess { - site { - soilData { - downSlope - crossSlope - bedrock - depthIntervalPreset - slopeLandscapePosition - slopeAspect - slopeSteepnessSelect - slopeSteepnessPercent - slopeSteepnessDegree - surfaceCracksSelect - surfaceSaltSelect - floodingSelect - limeRequirementsSelect - surfaceStoninessSelect - waterTableDepthSelect - soilDepthSelect - landCoverSelect - grazingSelect - depthDependentData { - depthInterval { - start - end - } - texture - rockFragmentVolume - clayPercent - colorHue - colorValue - colorChroma - colorPhotoUsed - colorPhotoSoilCondition - colorPhotoLightingCondition - conductivity - conductivityTest - conductivityUnit - structure - ph - phTestingSolution - phTestingMethod - soilOrganicCarbon - soilOrganicMatter - soilOrganicCarbonTesting - soilOrganicMatterTesting - sodiumAbsorptionRatio - carbonates + soilData { + downSlope + crossSlope + bedrock + depthIntervalPreset + slopeLandscapePosition + slopeAspect + slopeSteepnessSelect + slopeSteepnessPercent + slopeSteepnessDegree + surfaceCracksSelect + surfaceSaltSelect + floodingSelect + limeRequirementsSelect + surfaceStoninessSelect + waterTableDepthSelect + soilDepthSelect + landCoverSelect + grazingSelect + depthDependentData { + depthInterval { + start + end } - depthIntervals { - label - depthInterval { - start - end - } - soilTextureEnabled - soilColorEnabled - carbonatesEnabled - phEnabled - soilOrganicCarbonMatterEnabled - electricalConductivityEnabled - sodiumAdsorptionRatioEnabled - soilStructureEnabled + texture + rockFragmentVolume + clayPercent + colorHue + colorValue + colorChroma + colorPhotoUsed + colorPhotoSoilCondition + colorPhotoLightingCondition + conductivity + conductivityTest + conductivityUnit + structure + ph + phTestingSolution + phTestingMethod + soilOrganicCarbon + soilOrganicMatter + soilOrganicCarbonTesting + soilOrganicMatterTesting + sodiumAbsorptionRatio + carbonates + } + depthIntervals { + label + depthInterval { + start + end } + soilTextureEnabled + soilColorEnabled + carbonatesEnabled + phEnabled + soilOrganicCarbonMatterEnabled + electricalConductivityEnabled + sodiumAdsorptionRatioEnabled + soilStructureEnabled } } } @@ -924,9 +922,10 @@ def test_push_soil_data_can_process_mixed_results(client, user): ) assert response.json() + assert "data" in response.json() result = response.json()["data"]["pushSoilData"] assert result["errors"] is None - assert result["results"][0]["result"]["site"]["soilData"]["slopeAspect"] == 10 + assert result["results"][0]["result"]["soilData"]["slopeAspect"] == 10 assert result["results"][1]["result"]["reason"] == "INVALID_DATA" assert result["results"][2]["result"]["reason"] == "NOT_ALLOWED" assert result["results"][3]["result"]["reason"] == "DOES_NOT_EXIST" @@ -961,7 +960,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) @@ -1000,7 +998,7 @@ def test_push_soil_data_success(client, user): assert response.json() result = response.json()["data"]["pushSoilData"] assert result["errors"] is None - assert result["results"][0]["result"]["site"]["soilData"]["slopeAspect"] == 10 + assert result["results"][0]["result"]["soilData"]["slopeAspect"] == 10 site.refresh_from_db()