From 65391c7825a1cc6b6f60971c07e4451080fc5b6e Mon Sep 17 00:00:00 2001 From: shrouxm Date: Thu, 31 Oct 2024 10:53:32 -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 | 41 +++++++++++-------- .../apps/soil_id/models/soil_data.py | 24 ++--------- .../apps/soil_id/models/soil_data_history.py | 6 +++ .../tests/graphql/mutations/test_soil_data.py | 1 - 6 files changed, 51 insertions(+), 47 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..f421cbb63 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: @@ -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,17 +185,17 @@ 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 @@ -194,7 +203,7 @@ def get_entry_result(user: User, soil_data_entry: dict, history_entry: SoilDataH 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 ) ) diff --git a/terraso_backend/apps/soil_id/models/soil_data.py b/terraso_backend/apps/soil_id/models/soil_data.py index c77b79e40..0c7541670 100644 --- a/terraso_backend/apps/soil_id/models/soil_data.py +++ b/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): 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..79ddb07e7 100644 --- a/terraso_backend/tests/graphql/mutations/test_soil_data.py +++ b/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)