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

@@ -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
 

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"

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)

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()

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)