From 5afacfb2ef4c32c0b43d10fe46763516c6b6d12c Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Tue, 16 Apr 2024 16:43:10 +0200 Subject: [PATCH 1/8] Fix subscriptions sending twice --- ee/tasks/subscriptions/__init__.py | 2 +- posthog/models/subscription.py | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/ee/tasks/subscriptions/__init__.py b/ee/tasks/subscriptions/__init__.py index c4a87f515abc2..2f6e393ab3b2e 100644 --- a/ee/tasks/subscriptions/__init__.py +++ b/ee/tasks/subscriptions/__init__.py @@ -117,7 +117,7 @@ def _deliver_subscription_report( raise NotImplementedError(f"{subscription.target_type} is not supported") if not is_new_subscription_target: - subscription.set_next_delivery_date() + subscription.set_next_delivery_date(subscription.next_delivery_date) subscription.save() diff --git a/posthog/models/subscription.py b/posthog/models/subscription.py index 3680155f7df27..a2292658e208f 100644 --- a/posthog/models/subscription.py +++ b/posthog/models/subscription.py @@ -130,6 +130,8 @@ def save(self, *args, **kwargs) -> None: # Only if the schedule has changed do we update the next delivery date if not self.id or str(self._rrule) != str(self.rrule): self.set_next_delivery_date() + if "update_fields" in kwargs: + kwargs["update_fields"].append("next_delivery_date") super(Subscription, self).save(*args, **kwargs) @property From 2c0c2519d3cb1953105895be6e0b0dd42b9799c1 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Tue, 16 Apr 2024 17:08:38 +0200 Subject: [PATCH 2/8] Adjust calculation, add tests --- ee/tasks/subscriptions/__init__.py | 2 +- posthog/models/subscription.py | 4 +++- posthog/models/test/test_subscription_model.py | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/ee/tasks/subscriptions/__init__.py b/ee/tasks/subscriptions/__init__.py index 2f6e393ab3b2e..7ca0e06e6d529 100644 --- a/ee/tasks/subscriptions/__init__.py +++ b/ee/tasks/subscriptions/__init__.py @@ -118,7 +118,7 @@ def _deliver_subscription_report( if not is_new_subscription_target: subscription.set_next_delivery_date(subscription.next_delivery_date) - subscription.save() + subscription.save(update_fields=["next_delivery_date"]) @shared_task(queue=CeleryQueue.SUBSCRIPTION_DELIVERY.value) diff --git a/posthog/models/subscription.py b/posthog/models/subscription.py index a2292658e208f..6df12cab1edb2 100644 --- a/posthog/models/subscription.py +++ b/posthog/models/subscription.py @@ -124,7 +124,9 @@ def rrule(self): ) def set_next_delivery_date(self, from_dt=None): - self.next_delivery_date = self.rrule.after(dt=from_dt or timezone.now(), inc=False) + # We never want next_delivery_date to be in the past + now = timezone.now() + self.next_delivery_date = self.rrule.after(dt=max(from_dt or now, now), inc=False) def save(self, *args, **kwargs) -> None: # Only if the schedule has changed do we update the next delivery date diff --git a/posthog/models/test/test_subscription_model.py b/posthog/models/test/test_subscription_model.py index 0369c7d3c590e..14335e9d43d53 100644 --- a/posthog/models/test/test_subscription_model.py +++ b/posthog/models/test/test_subscription_model.py @@ -71,6 +71,21 @@ def test_only_updates_next_delivery_date_if_rrule_changes(self): subscription.save() assert old_date == subscription.next_delivery_date + @freeze_time("2022-01-01 09:55:00") + def test_set_next_delivery_date(self): + subscription = Subscription.objects.create( + team=self.team, + title="Daily Subscription", + target_type="email", + target_value="tests@posthog.com", + frequency="daily", + start_date=datetime(2022, 1, 1, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")), + ) + + subscription.set_next_delivery_date(subscription.next_delivery_date) + + assert subscription.next_delivery_date == datetime(2022, 1, 2, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")) + def test_generating_token(self): subscription = self._create_insight_subscription( target_value="test1@posthog.com,test2@posthog.com,test3@posthog.com" From bf1f0482c79021d1970c71afcdb462b3dff52539 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Tue, 16 Apr 2024 17:18:55 +0200 Subject: [PATCH 3/8] Add another test --- .../models/test/test_subscription_model.py | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/posthog/models/test/test_subscription_model.py b/posthog/models/test/test_subscription_model.py index 14335e9d43d53..e63b69856067e 100644 --- a/posthog/models/test/test_subscription_model.py +++ b/posthog/models/test/test_subscription_model.py @@ -71,20 +71,39 @@ def test_only_updates_next_delivery_date_if_rrule_changes(self): subscription.save() assert old_date == subscription.next_delivery_date - @freeze_time("2022-01-01 09:55:00") - def test_set_next_delivery_date(self): + @freeze_time("2022-01-11 09:55:00") + def test_set_next_delivery_date_when_in_upcoming_delta(self): subscription = Subscription.objects.create( + id=1, team=self.team, title="Daily Subscription", target_type="email", target_value="tests@posthog.com", frequency="daily", start_date=datetime(2022, 1, 1, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")), + next_delivery_date=datetime(2022, 1, 11, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")), ) subscription.set_next_delivery_date(subscription.next_delivery_date) - assert subscription.next_delivery_date == datetime(2022, 1, 2, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")) + assert subscription.next_delivery_date == datetime(2022, 1, 12, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")) + + @freeze_time("2022-01-11 09:55:00") + def test_set_next_delivery_date_when_days_behind(self): + subscription = Subscription.objects.create( + id=1, + team=self.team, + title="Daily Subscription", + target_type="email", + target_value="tests@posthog.com", + frequency="daily", + start_date=datetime(2022, 1, 1, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")), + next_delivery_date=datetime(2022, 1, 2, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")), + ) + + subscription.set_next_delivery_date(subscription.next_delivery_date) + + assert subscription.next_delivery_date == datetime(2022, 1, 12, 10, 0, 0, 0).replace(tzinfo=ZoneInfo("UTC")) def test_generating_token(self): subscription = self._create_insight_subscription( From 4181cb73a1e683dffbf14c8f421e4995caab7bf7 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 17 Apr 2024 17:46:38 +0200 Subject: [PATCH 4/8] Add buffer as well --- posthog/models/subscription.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/models/subscription.py b/posthog/models/subscription.py index 6df12cab1edb2..f7b8a90a7e492 100644 --- a/posthog/models/subscription.py +++ b/posthog/models/subscription.py @@ -125,7 +125,7 @@ def rrule(self): def set_next_delivery_date(self, from_dt=None): # We never want next_delivery_date to be in the past - now = timezone.now() + now = timezone.now() + timedelta(minutes=15) # Buffer of 15 minutes since we might run a bit early self.next_delivery_date = self.rrule.after(dt=max(from_dt or now, now), inc=False) def save(self, *args, **kwargs) -> None: From 39aadc654583e7cf991f94d6218b08fc81758ad7 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 17 Apr 2024 15:56:28 +0000 Subject: [PATCH 5/8] Update query snapshots --- .../api/test/__snapshots__/test_decide.ambr | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/posthog/api/test/__snapshots__/test_decide.ambr b/posthog/api/test/__snapshots__/test_decide.ambr index b60f0660121b9..ae8f9966d1136 100644 --- a/posthog/api/test/__snapshots__/test_decide.ambr +++ b/posthog/api/test/__snapshots__/test_decide.ambr @@ -142,6 +142,17 @@ ''' # --- # name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.3 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:RATE_LIMIT_ENABLED' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 + ''' +# --- +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.4 ''' SELECT 1 AS "a" FROM "posthog_grouptypemapping" @@ -149,7 +160,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.4 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.5 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -160,7 +171,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.5 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.6 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -171,7 +182,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.6 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.7 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -182,7 +193,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.7 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.8 ''' SELECT "posthog_user"."id", "posthog_user"."password", @@ -213,7 +224,7 @@ LIMIT 21 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.8 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.9 ''' SELECT "posthog_featureflag"."id", "posthog_featureflag"."key", @@ -236,22 +247,6 @@ AND "posthog_featureflag"."team_id" = 2) ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.9 - ''' - SELECT "posthog_pluginconfig"."id", - "posthog_pluginconfig"."web_token", - "posthog_pluginsourcefile"."updated_at", - "posthog_plugin"."updated_at", - "posthog_pluginconfig"."updated_at" - FROM "posthog_pluginconfig" - INNER JOIN "posthog_plugin" ON ("posthog_pluginconfig"."plugin_id" = "posthog_plugin"."id") - INNER JOIN "posthog_pluginsourcefile" ON ("posthog_plugin"."id" = "posthog_pluginsourcefile"."plugin_id") - WHERE ("posthog_pluginconfig"."enabled" - AND "posthog_pluginsourcefile"."filename" = 'site.ts' - AND "posthog_pluginsourcefile"."status" = 'TRANSPILED' - AND "posthog_pluginconfig"."team_id" = 2) - ''' -# --- # name: TestDecide.test_flag_with_behavioural_cohorts ''' SELECT "posthog_user"."id", From 3dfd3c40f568572d86a04d8d256b2b8bcce7a915 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 17 Apr 2024 16:17:44 +0000 Subject: [PATCH 6/8] Update query snapshots --- .../api/test/__snapshots__/test_decide.ambr | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/posthog/api/test/__snapshots__/test_decide.ambr b/posthog/api/test/__snapshots__/test_decide.ambr index ae8f9966d1136..b60f0660121b9 100644 --- a/posthog/api/test/__snapshots__/test_decide.ambr +++ b/posthog/api/test/__snapshots__/test_decide.ambr @@ -142,17 +142,6 @@ ''' # --- # name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.3 - ''' - SELECT "posthog_instancesetting"."id", - "posthog_instancesetting"."key", - "posthog_instancesetting"."raw_value" - FROM "posthog_instancesetting" - WHERE "posthog_instancesetting"."key" = 'constance:posthog:RATE_LIMIT_ENABLED' - ORDER BY "posthog_instancesetting"."id" ASC - LIMIT 1 - ''' -# --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.4 ''' SELECT 1 AS "a" FROM "posthog_grouptypemapping" @@ -160,7 +149,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.5 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.4 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -171,7 +160,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.6 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.5 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -182,7 +171,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.7 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.6 ''' SELECT "posthog_instancesetting"."id", "posthog_instancesetting"."key", @@ -193,7 +182,7 @@ LIMIT 1 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.8 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.7 ''' SELECT "posthog_user"."id", "posthog_user"."password", @@ -224,7 +213,7 @@ LIMIT 21 ''' # --- -# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.9 +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.8 ''' SELECT "posthog_featureflag"."id", "posthog_featureflag"."key", @@ -247,6 +236,22 @@ AND "posthog_featureflag"."team_id" = 2) ''' # --- +# name: TestDecide.test_decide_doesnt_error_out_when_database_is_down.9 + ''' + SELECT "posthog_pluginconfig"."id", + "posthog_pluginconfig"."web_token", + "posthog_pluginsourcefile"."updated_at", + "posthog_plugin"."updated_at", + "posthog_pluginconfig"."updated_at" + FROM "posthog_pluginconfig" + INNER JOIN "posthog_plugin" ON ("posthog_pluginconfig"."plugin_id" = "posthog_plugin"."id") + INNER JOIN "posthog_pluginsourcefile" ON ("posthog_plugin"."id" = "posthog_pluginsourcefile"."plugin_id") + WHERE ("posthog_pluginconfig"."enabled" + AND "posthog_pluginsourcefile"."filename" = 'site.ts' + AND "posthog_pluginsourcefile"."status" = 'TRANSPILED' + AND "posthog_pluginconfig"."team_id" = 2) + ''' +# --- # name: TestDecide.test_flag_with_behavioural_cohorts ''' SELECT "posthog_user"."id", From 03c9a02150855d4d3474c9281309710aef0e9ad2 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 18 Apr 2024 08:45:43 +0200 Subject: [PATCH 7/8] Adjust one other test --- ee/tasks/test/subscriptions/test_slack_subscriptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/tasks/test/subscriptions/test_slack_subscriptions.py b/ee/tasks/test/subscriptions/test_slack_subscriptions.py index 9770127e73778..758f74cd3b2d1 100644 --- a/ee/tasks/test/subscriptions/test_slack_subscriptions.py +++ b/ee/tasks/test/subscriptions/test_slack_subscriptions.py @@ -13,7 +13,7 @@ @patch("ee.tasks.subscriptions.slack_subscriptions.SlackIntegration") -@freeze_time("2022-02-02T08:55:00.000Z") +@freeze_time("2022-02-02T08:30:00.000Z") class TestSlackSubscriptionsTasks(APIBaseTest): subscription: Subscription dashboard: Dashboard From e412b28b7e32c5b311c85626e4a035e40dbb2e14 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Thu, 18 Apr 2024 11:39:26 +0200 Subject: [PATCH 8/8] Adjust one other test If we create subscriptions in the buffer that we have before the full hour we shouldn't expect them to run right in this hour. This is getting wonky. --- .../test/subscriptions/test_subscriptions.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/ee/tasks/test/subscriptions/test_subscriptions.py b/ee/tasks/test/subscriptions/test_subscriptions.py index 3a63e27ee6ff0..d6afe50b68f7f 100644 --- a/ee/tasks/test/subscriptions/test_subscriptions.py +++ b/ee/tasks/test/subscriptions/test_subscriptions.py @@ -51,17 +51,18 @@ def test_subscription_delivery_scheduling( mock_send_email: MagicMock, mock_send_slack: MagicMock, ) -> None: - subscriptions = [ - create_subscription(team=self.team, insight=self.insight, created_by=self.user), - create_subscription(team=self.team, insight=self.insight, created_by=self.user), - create_subscription(team=self.team, dashboard=self.dashboard, created_by=self.user), - create_subscription( - team=self.team, - dashboard=self.dashboard, - created_by=self.user, - deleted=True, - ), - ] + with freeze_time("2022-02-02T08:30:00.000Z"): # Create outside of buffer before running + subscriptions = [ + create_subscription(team=self.team, insight=self.insight, created_by=self.user), + create_subscription(team=self.team, insight=self.insight, created_by=self.user), + create_subscription(team=self.team, dashboard=self.dashboard, created_by=self.user), + create_subscription( + team=self.team, + dashboard=self.dashboard, + created_by=self.user, + deleted=True, + ), + ] # Modify a subscription to have its target time at least an hour ahead subscriptions[2].start_date = datetime(2022, 1, 1, 10, 0).replace(tzinfo=ZoneInfo("UTC")) subscriptions[2].save()