From b27962c55bac6da725deffa4f91126514786aa50 Mon Sep 17 00:00:00 2001 From: Chris Smit Date: Mon, 16 Dec 2024 11:52:03 +0200 Subject: [PATCH 1/4] Disallow child bots that are versions --- apps/experiments/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/experiments/models.py b/apps/experiments/models.py index 3f843330e..c4db807be 100644 --- a/apps/experiments/models.py +++ b/apps/experiments/models.py @@ -1101,7 +1101,7 @@ def eligible_children(cls, team: Team, parent: Experiment | None = None): else: eligible_experiments = Experiment.objects.filter(team=team).exclude(id__in=parent_ids) - return eligible_experiments + return eligible_experiments.filter(working_version_id=None) @transaction.atomic() def create_new_version(self, new_parent: Experiment) -> "ExperimentRoute": From 42e31c16981c3f583e3ca20e3a041988377f961e Mon Sep 17 00:00:00 2001 From: Chris Smit Date: Mon, 16 Dec 2024 13:06:40 +0200 Subject: [PATCH 2/4] Use the correct versioned child (or router) to generate responses for scheduled messages in a versioned router setup --- apps/events/models.py | 24 ++++++++++-- apps/events/tests/test_scheduled_messages.py | 40 +++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/apps/events/models.py b/apps/events/models.py index 41e7605a9..c997fc7c5 100644 --- a/apps/events/models.py +++ b/apps/events/models.py @@ -352,14 +352,13 @@ def safe_trigger(self): logger.exception(f"An error occured while trying to send scheduled messsage {self.id}. Error: {e}") def _trigger(self): - experiment_id = self.params.get("experiment_id", self.experiment.id) experiment_session = self.participant.get_latest_session(experiment=self.experiment) if not experiment_session: # Schedules probably created by the API return - experiment_to_use = Experiment.objects.get(id=experiment_id) + experiment_session.ad_hoc_bot_message( - self.params["prompt_text"], fail_silently=False, use_experiment=experiment_to_use + self.params["prompt_text"], fail_silently=False, use_experiment=self._get_experiment_to_generate_response() ) utc_now = timezone.now() @@ -373,6 +372,25 @@ def _trigger(self): self.save() + def _get_experiment_to_generate_response(self) -> Experiment: + """ + - If no child bot was specified to generate the response, use the default experiment version + - If a child bot was specified to generate the response, we must find the version of the child bot that is + linked to the default router. + """ + default_router_experiment = self.experiment.default_version + experiment_id = self.params.get("experiment_id") + if experiment_id and int(experiment_id) != self.experiment.id: + if default_router_experiment.is_a_version and default_router_experiment.child_links.count() > 0: + # Find the child of this version that has the specified experiment as its working version + return ( + default_router_experiment.child_links.filter(child__working_version_id=experiment_id).first().child + ) + + return Experiment.objects.get(id=experiment_id) + + return default_router_experiment + def _should_mark_complete(self): return bool(not self.remaining_triggers or (self.end_date and self.end_date <= timezone.now())) diff --git a/apps/events/tests/test_scheduled_messages.py b/apps/events/tests/test_scheduled_messages.py index a22fde30f..12e9273b4 100644 --- a/apps/events/tests/test_scheduled_messages.py +++ b/apps/events/tests/test_scheduled_messages.py @@ -9,8 +9,9 @@ from apps.events.models import EventActionType, ScheduledMessage, TimePeriod from apps.events.tasks import _get_messages_to_fire, poll_scheduled_messages +from apps.experiments.models import ExperimentRoute from apps.utils.factories.events import EventActionFactory, ScheduledMessageFactory -from apps.utils.factories.experiment import ExperimentSessionFactory +from apps.utils.factories.experiment import ExperimentFactory, ExperimentSessionFactory from apps.utils.time import timedelta_to_relative_delta @@ -260,3 +261,40 @@ def test_schedule_update(): _assert_next_trigger_date(message3, message3_next_trigger_data) assert message1.next_trigger_date < message1_prev_trigger_date assert message2.next_trigger_date < message2_prev_trigger_date + + +@pytest.mark.django_db() +def test_schedule_trigger_for_versioned_routes(): + router = ExperimentFactory() + child = ExperimentFactory(team=router.team) + session = ExperimentSessionFactory(experiment=router) + + ExperimentRoute.objects.create( + team=router.team, + parent=router, + child=child, + keyword="keyword1", + ) + + event_action, params = _construct_event_action(frequency=1, time_period=TimePeriod.DAYS, experiment_id=None) + sm = ScheduledMessageFactory( + team=router.team, action=event_action, experiment=router, participant=session.participant + ) + # No experiment specified, so the router should be used + assert sm._get_experiment_to_generate_response() == router + + new_params = sm.action.params + new_params["experiment_id"] = child.id + sm.action.params = new_params + sm.action.save() + + # No versions yet, so the working version of the child should be used + assert sm._get_experiment_to_generate_response() == child + + default_router = router.create_new_version(make_default=True) + sm.refresh_from_db() + child_version = default_router.child_links.first().child + assert new_params["experiment_id"] == child_version.working_version_id + # The router is versioned and the deployed version is not the working version, so the child of the deployed version + # should be used + assert sm._get_experiment_to_generate_response() == child_version From 54bb6bcd34dcfad7b2f8dd517c29f01cf8a5cc87 Mon Sep 17 00:00:00 2001 From: Chris Smit Date: Mon, 16 Dec 2024 14:52:22 +0200 Subject: [PATCH 3/4] Update test --- apps/experiments/tests/test_models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/experiments/tests/test_models.py b/apps/experiments/tests/test_models.py index 34e2901f2..3a71375c4 100644 --- a/apps/experiments/tests/test_models.py +++ b/apps/experiments/tests/test_models.py @@ -591,7 +591,7 @@ def test_eligible_children(self): assert len(queryset) == 2 queryset = ExperimentRoute.eligible_children(team=parent.team) - assert len(queryset) == 4 + assert len(queryset) == 3 def test_compare_with_model_testcase_1(self): """ From 1b2891c28c3ac176d8e8a993ed421c208ff82b5f Mon Sep 17 00:00:00 2001 From: Chris Smit Date: Tue, 17 Dec 2024 07:54:57 +0200 Subject: [PATCH 4/4] Update test --- apps/events/tests/test_scheduled_messages.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/apps/events/tests/test_scheduled_messages.py b/apps/events/tests/test_scheduled_messages.py index 12e9273b4..3156c34b3 100644 --- a/apps/events/tests/test_scheduled_messages.py +++ b/apps/events/tests/test_scheduled_messages.py @@ -280,7 +280,7 @@ def test_schedule_trigger_for_versioned_routes(): sm = ScheduledMessageFactory( team=router.team, action=event_action, experiment=router, participant=session.participant ) - # No experiment specified, so the router should be used + # No experiment specified, so the router should be used (no version yet, so router == default version) assert sm._get_experiment_to_generate_response() == router new_params = sm.action.params @@ -292,9 +292,20 @@ def test_schedule_trigger_for_versioned_routes(): assert sm._get_experiment_to_generate_response() == child default_router = router.create_new_version(make_default=True) + router.refresh_from_db() + del router.default_version sm.refresh_from_db() child_version = default_router.child_links.first().child assert new_params["experiment_id"] == child_version.working_version_id # The router is versioned and the deployed version is not the working version, so the child of the deployed version # should be used assert sm._get_experiment_to_generate_response() == child_version + + new_params = sm.action.params + new_params["experiment_id"] = None + sm.action.params = new_params + sm.action.save() + sm.refresh_from_db() + # Clear the `params` cached property on ScheduledMessage + del sm.params + assert sm._get_experiment_to_generate_response() == router.default_version