From f06f2a2b8d45e9b010694cd67f7b7b49dde1ccff Mon Sep 17 00:00:00 2001 From: Farmer Paul Date: Wed, 27 Nov 2024 10:28:11 -0500 Subject: [PATCH] fix: Incomplete Take Now flow prevents multi-informant assignment (M2-8214) (#1667) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: improve assignment existence CRUD function Renamed function to `exist` to align with naming convention for existence functions. Added missing soft existence check and made corrections to docs. * fix: repurposed unused private function to public * fix: only validate temp relation if no assignment * chore: reduce Pylance errors in tests * test: add test for expired relation w/ assignment * fix: delete inappropriate temp relations in tests This temporary relation is not normally created for Take Now: ❌ source subject ⇔ target subject Only these two temporary relations should be created: ✅ inputting subject ⇔ source subject ✅ inputting subject ⇔ target subject So deleted superfluous calls to `create_relation` where found in tests. --- .../activity_assignments/crud/assignments.py | 28 ++-- src/apps/activity_assignments/service.py | 22 +++- src/apps/answers/domain/answer_items.py | 8 +- src/apps/answers/domain/answers.py | 10 +- src/apps/answers/service.py | 16 ++- src/apps/answers/tests/test_answers.py | 123 +++++++++++++----- src/apps/subjects/domain.py | 10 +- 7 files changed, 156 insertions(+), 61 deletions(-) diff --git a/src/apps/activity_assignments/crud/assignments.py b/src/apps/activity_assignments/crud/assignments.py index 2cb7b1baa14..1ab2f9ac69c 100644 --- a/src/apps/activity_assignments/crud/assignments.py +++ b/src/apps/activity_assignments/crud/assignments.py @@ -7,6 +7,7 @@ from apps.activities.db.schemas import ActivitySchema from apps.activity_assignments.db.schemas import ActivityAssigmentSchema +from apps.activity_assignments.domain.assignments import ActivityAssignmentCreate from apps.activity_flows.db.schemas import ActivityFlowSchema from apps.shared.filtering import FilterField, Filtering from apps.shared.query_params import QueryParams @@ -78,9 +79,9 @@ async def create_many(self, schemas: list[ActivityAssigmentSchema]) -> list[Acti return await self._create_many(schemas) - async def already_exists(self, schema: ActivityAssigmentSchema) -> ActivityAssigmentSchema: + async def exist(self, assignment: ActivityAssignmentCreate) -> ActivityAssigmentSchema | None: """ - Checks if an activity assignment already exists in the database. + Checks if an activity assignment exists in the database. This method builds a query to check for the existence of an assignment with the same `activity_id`, `activity_flow_id`, `respondent_subject_id`, and `target_subject_id`, @@ -88,28 +89,29 @@ async def already_exists(self, schema: ActivityAssigmentSchema) -> ActivityAssig Parameters: ----------- - schema : ActivityAssigmentSchema + assignment : ActivityAssignmentCreate The activity assignment schema to check for existence. Returns: -------- - bool - `True` if the assignment already exists, otherwise `False`. + ActivityAssigmentSchema | None + The value of the first matching record, if it exists. Otherwise, returns None. Notes: ------ - - This method uses the `_execute` method from the `BaseCRUD` class to run the query and - check for the existence of the assignment. - - The existence check is based on the combination of IDs and considers soft-deleted records. + - The existence check excludes soft-deleted records. """ query: Query = select(ActivityAssigmentSchema) - query = query.where(ActivityAssigmentSchema.activity_id == schema.activity_id) - query = query.where(ActivityAssigmentSchema.respondent_subject_id == schema.respondent_subject_id) - query = query.where(ActivityAssigmentSchema.target_subject_id == schema.target_subject_id) - query = query.where(ActivityAssigmentSchema.activity_flow_id == schema.activity_flow_id) + query = query.where( + ActivityAssigmentSchema.activity_id == assignment.activity_id, + ActivityAssigmentSchema.activity_flow_id == assignment.activity_flow_id, + ActivityAssigmentSchema.respondent_subject_id == assignment.respondent_subject_id, + ActivityAssigmentSchema.target_subject_id == assignment.target_subject_id, + ActivityAssigmentSchema.soft_exists(), + ) db_result = await self._execute(query) - return db_result.scalars().first() + return db_result.scalars().one_or_none() async def get_target_subject_ids_by_activity_or_flow_ids( self, diff --git a/src/apps/activity_assignments/service.py b/src/apps/activity_assignments/service.py index f1db62b9c3f..9ac2a5bfce4 100644 --- a/src/apps/activity_assignments/service.py +++ b/src/apps/activity_assignments/service.py @@ -190,8 +190,26 @@ async def send_email_notification( ) _ = asyncio.create_task(service.send(message)) - async def _check_for_already_existing_assignment(self, schema: ActivityAssigmentSchema) -> bool: - return await ActivityAssigmentCRUD(self.session).already_exists(schema) + async def exist(self, assignment: ActivityAssignmentCreate) -> ActivityAssigmentSchema | None: + """ + Returns whether an assignment matching the given assignment schema values exists and has not been soft-deleted. + + Parameters: + ---------- + assignment : ActivityAssignmentCreate + The assignment object containing the details of the assignment, + including `activity_id`, `activity_flow_id`, `respondent_subject_id`, and `target_subject_id`. + + Returns: + ------- + ActivityAssigmentSchema | None + The the first matching assignment if it exists, None otherwise. + + Notes: + ------ + - The existence check excludes soft-deleted assignments. + """ + return await ActivityAssigmentCRUD(self.session).exist(assignment) @staticmethod def _validate_assignment_and_get_activity_or_flow_name( diff --git a/src/apps/answers/domain/answer_items.py b/src/apps/answers/domain/answer_items.py index 39a40b4a207..e66dfba535b 100644 --- a/src/apps/answers/domain/answer_items.py +++ b/src/apps/answers/domain/answer_items.py @@ -59,11 +59,11 @@ class AssessmentItem(InternalModel): class ItemAnswerCreate(InternalModel): - answer: str | None - events: str | None + answer: str | None = None + events: str | None = None item_ids: list[uuid.UUID] - identifier: str | None - scheduled_time: datetime.datetime | None + identifier: str | None = None + scheduled_time: datetime.datetime | None = None start_time: datetime.datetime end_time: datetime.datetime user_public_key: str | None diff --git a/src/apps/answers/domain/answers.py b/src/apps/answers/domain/answers.py index 0a0309d9491..374cd14e63c 100644 --- a/src/apps/answers/domain/answers.py +++ b/src/apps/answers/domain/answers.py @@ -106,12 +106,12 @@ class AppletAnswerCreate(InternalModel): is_flow_completed: bool | None = None activity_id: uuid.UUID answer: ItemAnswerCreate - created_at: datetime.datetime | None + created_at: datetime.datetime | None = None alerts: list[AnswerAlert] = Field(default_factory=list) client: ClientMeta - target_subject_id: uuid.UUID | None - source_subject_id: uuid.UUID | None - input_subject_id: uuid.UUID | None + target_subject_id: uuid.UUID | None = None + source_subject_id: uuid.UUID | None = None + input_subject_id: uuid.UUID | None = None _dates_from_ms = validator("created_at", pre=True, allow_reuse=True)(datetime_from_ms) @@ -121,7 +121,7 @@ class AssessmentAnswerCreate(InternalModel): item_ids: list[uuid.UUID] reviewer_public_key: str assessment_version_id: str - reviewed_flow_submit_id: uuid.UUID | None + reviewed_flow_submit_id: uuid.UUID | None = None class AnswerDate(InternalModel): diff --git a/src/apps/answers/service.py b/src/apps/answers/service.py index 238a00a9540..c3334da241f 100644 --- a/src/apps/answers/service.py +++ b/src/apps/answers/service.py @@ -23,6 +23,8 @@ from apps.activities.db.schemas import ActivityItemHistorySchema from apps.activities.domain.activity_history import ActivityHistoryFull from apps.activities.errors import ActivityDoeNotExist, ActivityHistoryDoeNotExist, FlowDoesNotExist +from apps.activity_assignments.domain.assignments import ActivityAssignmentCreate +from apps.activity_assignments.service import ActivityAssignmentService from apps.activity_flows.crud import FlowsCRUD, FlowsHistoryCRUD from apps.alerts.crud.alert import AlertCRUD from apps.alerts.db.schemas import AlertSchema @@ -345,9 +347,19 @@ async def _create_answer(self, applet_answer: AppletAnswerCreate) -> AnswerSchem else: source_subject = respondent_subject - await self._validate_temp_take_now_relation_between_subjects( - respondent_subject.id, source_subject.id, target_subject.id + # Check if source subject is manually assigned to target subject. + assignment = ActivityAssignmentCreate( + activity_id=applet_answer.activity_id if applet_answer.flow_id is None else None, + activity_flow_id=applet_answer.flow_id, + respondent_subject_id=source_subject.id, + target_subject_id=target_subject.id, ) + assignment_exists = await ActivityAssignmentService(self.session).exist(assignment) + # If no assignment exists, ensure valid temp take now relation between the subjects. + if not assignment_exists: + await self._validate_temp_take_now_relation_between_subjects( + respondent_subject.id, source_subject.id, target_subject.id + ) relation = await self._get_answer_relation(respondent_subject, source_subject, target_subject) answer = await AnswersCRUD(self.answer_session).create( diff --git a/src/apps/answers/tests/test_answers.py b/src/apps/answers/tests/test_answers.py index 0e3928e37d6..2958929f790 100644 --- a/src/apps/answers/tests/test_answers.py +++ b/src/apps/answers/tests/test_answers.py @@ -13,6 +13,8 @@ from sqlalchemy.ext.asyncio import AsyncSession from apps.activities.domain.activity_update import ActivityUpdate +from apps.activity_assignments.domain.assignments import ActivityAssignmentCreate +from apps.activity_assignments.service import ActivityAssignmentService from apps.answers.crud import AnswerItemsCRUD from apps.answers.crud.answers import AnswersCRUD from apps.answers.db.schemas import AnswerItemSchema, AnswerNoteSchema, AnswerSchema @@ -1116,29 +1118,21 @@ async def test_answer_activity_items_create_temporary_relation_success( }, ) - await subject_service.create_relation( - relation="take-now", - source_subject_id=source_subject.id, - subject_id=target_subject.id, - meta={ - "expiresAt": (datetime.datetime.now() + datetime.timedelta(days=1)).isoformat(), - }, - ) - data.source_subject_id = source_subject.id data.target_subject_id = target_subject.id data.input_subject_id = applet_one_sam_subject.id - # before posting the request, make sure that there is a temporary relation - existing_relation = await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) - assert existing_relation + # before posting the request, make sure that there are temporary relations + assert subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) response = await client.post(self.answer_url, data=data) assert response.status_code == http.HTTPStatus.CREATED, response.json() - # after submitting make sure that the relation has been deleted - relation_exists = await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) - assert not relation_exists + + # after submitting make sure that the relations have been deleted + assert not await subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert not await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) async def test_answer_activity_items_relation_equal_other_when_relation_is_temp( self, @@ -1278,24 +1272,21 @@ async def test_answer_activity_items_create_permanent_relation_success( relation="parent", source_subject_id=applet_one_sam_subject.id, subject_id=target_subject.id ) - await subject_service.create_relation( - relation="parent", source_subject_id=source_subject.id, subject_id=target_subject.id - ) - data.source_subject_id = source_subject.id data.target_subject_id = target_subject.id data.input_subject_id = applet_one_sam_subject.id - # before posting the request, make sure that there is a temporary relation - existing_relation = await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) - assert existing_relation + # before posting the request, make sure that there are temporary relations + assert await subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) response = await client.post(self.answer_url, data=data) assert response.status_code == http.HTTPStatus.CREATED, response.json() - # after submitting make sure that the relation has not been deleted - relation_exists = await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) - assert relation_exists + + # after submitting make sure that the relations have not been deleted + assert await subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) async def test_answer_activity_items_create_expired_temporary_relation_fail( self, @@ -1352,9 +1343,81 @@ async def test_answer_activity_items_create_expired_temporary_relation_fail( }, ) + data.source_subject_id = source_subject.id + data.target_subject_id = target_subject.id + data.input_subject_id = applet_one_sam_subject.id + + # before posting the request, make sure that there are temporary relations + assert await subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) + + response = await client.post(self.answer_url, data=data) + assert response.status_code == http.HTTPStatus.BAD_REQUEST, response.json() + + async def test_answer_activity_items_create_expired_temporary_relation_with_assignment_success( + self, + tom: User, + answer_create_applet_one: AppletAnswerCreate, + client: TestClient, + session: AsyncSession, + sam: User, + applet_one: AppletFull, + applet_one_sam_respondent, + applet_one_sam_subject, + ) -> None: + client.login(tom) + subject_service = SubjectsService(session, tom.id) + + data = answer_create_applet_one.copy(deep=True) + + client.login(sam) + subject_service = SubjectsService(session, sam.id) + source_subject = await subject_service.create( + SubjectCreate( + applet_id=applet_one.id, + creator_id=tom.id, + first_name="source", + last_name="subject", + email=EmailStr("source_subject@mindlogger.com"), + secret_user_id=f"{uuid.uuid4()}", + ) + ) + target_subject = await subject_service.create( + SubjectCreate( + applet_id=applet_one.id, + creator_id=tom.id, + first_name="target", + last_name="subject", + secret_user_id=f"{uuid.uuid4()}", + ) + ) + + # Create an assignment by source subject about target subject + await ActivityAssignmentService(session).create_many( + applet_id=applet_one.id, + assignments_create=[ + ActivityAssignmentCreate( + activity_id=answer_create_applet_one.activity_id, + activity_flow_id=None, + respondent_subject_id=source_subject.id, + target_subject_id=target_subject.id, + ), + ], + ) + + # create a relation between respondent and source + await subject_service.create_relation( + relation="take-now", + source_subject_id=applet_one_sam_subject.id, + subject_id=source_subject.id, + meta={ + "expiresAt": (datetime.datetime.now() - datetime.timedelta(days=1)).isoformat(), + }, + ) + # create a relation between respondent and target await subject_service.create_relation( relation="take-now", - source_subject_id=source_subject.id, + source_subject_id=applet_one_sam_subject.id, subject_id=target_subject.id, meta={ "expiresAt": (datetime.datetime.now() - datetime.timedelta(days=1)).isoformat(), @@ -1365,12 +1428,12 @@ async def test_answer_activity_items_create_expired_temporary_relation_fail( data.target_subject_id = target_subject.id data.input_subject_id = applet_one_sam_subject.id - # before posting the request, make sure that there is a temporary relation - existing_relation = await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) - assert existing_relation + # before posting the request, make sure that there are temporary relations + assert await subject_service.get_relation(applet_one_sam_subject.id, source_subject.id) + assert await subject_service.get_relation(applet_one_sam_subject.id, target_subject.id) response = await client.post(self.answer_url, data=data) - assert response.status_code == http.HTTPStatus.BAD_REQUEST, response.json() + assert response.status_code == http.HTTPStatus.CREATED, response.json() async def test_answer_get_export_data__answer_from_respondent( self, diff --git a/src/apps/subjects/domain.py b/src/apps/subjects/domain.py index f6d30739790..73fc0efa7f6 100644 --- a/src/apps/subjects/domain.py +++ b/src/apps/subjects/domain.py @@ -9,16 +9,16 @@ class SubjectCreate(InternalModel): applet_id: uuid.UUID - email: EmailStr | None + email: EmailStr | None = None creator_id: uuid.UUID - user_id: uuid.UUID | None - language: str | None + user_id: uuid.UUID | None = None + language: str | None = None first_name: str last_name: str secret_user_id: str - nickname: str | None + nickname: str | None = None is_deleted: bool = False - tag: str | None + tag: str | None = None class Subject(SubjectCreate):