From 7ba5fcbb714cb3a4dd45a357169f88c22693f7a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Mon, 15 Mar 2021 12:34:39 +0200 Subject: [PATCH] Fix sendalerts to clear Profile.next_nag_date if all checks up Profile.next_nag_date tracks when the next hourly/daily reminder should be sent. Normally, sendalerts sets this field when a check goes down, and sendreports clears it out whenever it is about to send a reminder but realizes all checks are up. The problem: sendalerts can set next_nag_date to a non-null value, but it does not clear it out when all checks are up. This can result in a hourly/daily reminder being sent out at the wrong time. Specific example, assuming hourly reminders: 13:00: Check A goes down. next_nag_date gets set to 14:00. 13:05: Check A goes up. next_nag_date remains set to 14:00. 13:55: Check B goes down. next_nag_date remains set to 14:00. 14:00: Healthchecks sends a hourly reminder, just 5 minutes after Check B going down. It should have sent the reminder at 13:55 + 1 hour = 14:55 The fix: sendalerts can now both set and clear the next_nag_date field. The main changes are in Project.update_next_nag_dates() and in Profile.update_next_nag_date(). With the fix: 13:00: Check A goes down. next_nag_date gets set to 14:00. 13:05: Check A goes up. next_nag_date gets set to null. 13:55: Check B goes down. next_nag_date gets set to 14:55. 14:55: Healthchecks sends a hourly reminder. --- CHANGELOG.md | 1 + hc/accounts/models.py | 27 +++++++++++------- hc/accounts/tests/test_profile_model.py | 36 ++++++++++++++++++++++++ hc/api/management/commands/sendalerts.py | 5 ++-- hc/api/tests/test_sendalerts.py | 32 ++++++++++++++++----- 5 files changed, 81 insertions(+), 20 deletions(-) create mode 100644 hc/accounts/tests/test_profile_model.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c4122b47..d9131aab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ All notable changes to this project will be documented in this file. ## Bug Fixes - Fix downtime summary to handle months when the check didn't exist yet (#472) - Relax cron expression validation: accept all expressions that croniter accepts +- Fix sendalerts to clear Profile.next_nag_date if all checks up ## v1.19.0 - 2021-02-03 diff --git a/hc/accounts/models.py b/hc/accounts/models.py index 878944d3..f87d2290 100644 --- a/hc/accounts/models.py +++ b/hc/accounts/models.py @@ -135,8 +135,8 @@ class Profile(models.Model): def projects(self): """ Return a queryset of all projects we have access to. """ - is_owner = Q(owner=self.user) - is_member = Q(member__user=self.user) + is_owner = Q(owner_id=self.user_id) + is_member = Q(member__user_id=self.user_id) q = Project.objects.filter(is_owner | is_member) return q.distinct().order_by("name") @@ -267,6 +267,15 @@ class Profile(models.Model): def can_accept(self, project): return project.num_checks() <= self.num_checks_available() + def update_next_nag_date(self): + any_down = self.checks_from_all_projects().filter(status="down").exists() + if any_down and self.next_nag_date is None and self.nag_period: + self.next_nag_date = timezone.now() + self.nag_period + self.save(update_fields=["next_nag_date"]) + elif not any_down and self.next_nag_date: + self.next_nag_date = None + self.save(update_fields=["next_nag_date"]) + class Project(models.Model): code = models.UUIDField(default=uuid.uuid4, unique=True) @@ -319,17 +328,15 @@ class Project(models.Model): user.profile.send_instant_login_link(self, redirect_url=checks_url) return True - def set_next_nag_date(self): - """ Set next_nag_date on profiles of all members of this project. """ + def update_next_nag_dates(self): + """ Update next_nag_date on profiles of all members of this project. """ - is_owner = Q(user=self.owner) + is_owner = Q(user_id=self.owner_id) is_member = Q(user__memberships__project=self) - q = Profile.objects.filter(is_owner | is_member) - q = q.exclude(nag_period=NO_NAG) - # Exclude profiles with next_nag_date already set - q = q.filter(next_nag_date__isnull=True) + q = Profile.objects.filter(is_owner | is_member).exclude(nag_period=NO_NAG) - q.update(next_nag_date=timezone.now() + models.F("nag_period")) + for profile in q: + profile.update_next_nag_date() def overall_status(self): status = "up" diff --git a/hc/accounts/tests/test_profile_model.py b/hc/accounts/tests/test_profile_model.py new file mode 100644 index 00000000..14cc0f22 --- /dev/null +++ b/hc/accounts/tests/test_profile_model.py @@ -0,0 +1,36 @@ +from datetime import timedelta as td + +from django.utils.timezone import now +from hc.test import BaseTestCase +from hc.api.models import Check + + +class ProfileModelTestCase(BaseTestCase): + def test_it_sets_next_nag_date(self): + Check.objects.create(project=self.project, status="down") + + self.profile.nag_period = td(hours=1) + self.profile.update_next_nag_date() + + self.assertTrue(self.profile.next_nag_date) + + def test_it_does_not_set_next_nag_date_if_no_nag_period(self): + Check.objects.create(project=self.project, status="down") + self.profile.update_next_nag_date() + self.assertIsNone(self.profile.next_nag_date) + + def test_it_does_not_update_existing_next_nag_date(self): + Check.objects.create(project=self.project, status="down") + + original_nag_date = now() - td(minutes=30) + + self.profile.next_nag_date = original_nag_date + self.profile.nag_period = td(hours=1) + self.profile.update_next_nag_date() + + self.assertEqual(self.profile.next_nag_date, original_nag_date) + + def test_it_clears_next_nag_date(self): + self.profile.next_nag_date = now() + self.profile.update_next_nag_date() + self.assertIsNone(self.profile.next_nag_date) diff --git a/hc/api/management/commands/sendalerts.py b/hc/api/management/commands/sendalerts.py index 54a9818c..b3624ea2 100644 --- a/hc/api/management/commands/sendalerts.py +++ b/hc/api/management/commands/sendalerts.py @@ -23,9 +23,8 @@ def notify(flip_id, stdout): stdout.write(SENDING_TMPL % (flip.new_status, check.code)) - # Set dates for followup nags - if flip.new_status == "down": - check.project.set_next_nag_date() + # Set or clear dates for followup nags + check.project.update_next_nag_dates() # Send notifications send_start = timezone.now() diff --git a/hc/api/tests/test_sendalerts.py b/hc/api/tests/test_sendalerts.py index b8d560a0..a0a5c3a6 100644 --- a/hc/api/tests/test_sendalerts.py +++ b/hc/api/tests/test_sendalerts.py @@ -101,10 +101,13 @@ class SendAlertsTestCase(BaseTestCase): # It should call `notify` instead of `notify_on_thread` self.assertTrue(mock_notify.called) - def test_it_updates_owners_next_nag_date(self): + def test_it_sets_next_nag_date(self): self.profile.nag_period = td(hours=1) self.profile.save() + self.bobs_profile.nag_period = td(hours=1) + self.bobs_profile.save() + check = Check(project=self.project, status="down") check.last_ping = now() - td(days=2) check.save() @@ -116,26 +119,41 @@ class SendAlertsTestCase(BaseTestCase): notify(flip.id, Mock()) + # next_nag_gate should now be set for the project's owner self.profile.refresh_from_db() self.assertIsNotNone(self.profile.next_nag_date) - def test_it_updates_members_next_nag_date(self): + # next_nag_gate should now be set for the project's members + self.bobs_profile.refresh_from_db() + self.assertIsNotNone(self.bobs_profile.next_nag_date) + + def test_it_clears_next_nag_date(self): + self.profile.nag_period = td(hours=1) + self.profile.next_nag_date = now() - td(minutes=30) + self.profile.save() + self.bobs_profile.nag_period = td(hours=1) + self.bobs_profile.next_nag_date = now() - td(minutes=30) self.bobs_profile.save() - check = Check(project=self.project, status="down") - check.last_ping = now() - td(days=2) + check = Check(project=self.project, status="up") + check.last_ping = now() check.save() flip = Flip(owner=check, created=check.last_ping) - flip.old_status = "up" - flip.new_status = "down" + flip.old_status = "down" + flip.new_status = "up" flip.save() notify(flip.id, Mock()) + # next_nag_gate should now be cleared out for the project's owner + self.profile.refresh_from_db() + self.assertIsNone(self.profile.next_nag_date) + + # next_nag_gate should now be cleared out for the project's members self.bobs_profile.refresh_from_db() - self.assertIsNotNone(self.bobs_profile.next_nag_date) + self.assertIsNone(self.bobs_profile.next_nag_date) def test_it_does_not_touch_already_set_next_nag_dates(self): original_nag_date = now() - td(minutes=30)