Fix acknowledge reminder (#3345)

# What this PR does Fix acknowledge reminder: - check if organization was deleted - improve logging ## Which issue(s) this PR fixes ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
grafana · Nov 14, 2023 · d7d5c3a · d7d5c3a
1 parent e8bd71b
commit d7d5c3a
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Forward headers for Amazon SNS when organizations are moved @mderynck ([#3326](https://github.com/grafana/oncall/pull/3326))
 - Fix styling when light theme is turned on via system preferences
   by excluding dark theme css vars in this case ([#3336](https://github.com/grafana/oncall/pull/3336))
+- Fix issue when acknowledge reminder works for deleted organizations @Ferril ([#3345](https://github.com/grafana/oncall/pull/3345))
 
 ## v1.3.57 (2023-11-10)
 

diff --git a/engine/apps/alerts/tasks/acknowledge_reminder.py b/engine/apps/alerts/tasks/acknowledge_reminder.py
@@ -26,13 +26,11 @@ def acknowledge_reminder_task(alert_group_pk: int, unacknowledge_process_id: str
         if unacknowledge_process_id != alert_group.last_unique_unacknowledge_process_id:
             return
 
+    organization = alert_group.channel.organization
+
     # Get timeout values
-    acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[
-        alert_group.channel.organization.acknowledge_remind_timeout
-    ]
-    unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[
-        alert_group.channel.organization.unacknowledge_timeout
-    ]
+    acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[organization.acknowledge_remind_timeout]
+    unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[organization.unacknowledge_timeout]
 
     # Don't proceed if the alert group is not in a state for acknowledgement reminder
     acknowledge_reminder_required = (
@@ -41,10 +39,18 @@ def acknowledge_reminder_task(alert_group_pk: int, unacknowledge_process_id: str
         and alert_group.acknowledged_by == AlertGroup.USER
         and acknowledge_reminder_timeout
     )
-    if not acknowledge_reminder_required:
-        task_logger.info("AlertGroup is not in a state for acknowledgement reminder")
+    is_organization_deleted = organization.deleted_at is not None
+    log_info = (
+        f"acknowledge_reminder_timeout option: {acknowledge_reminder_timeout},"
+        f"organization ppk: {organization.public_primary_key},"
+        f"organization is deleted: {is_organization_deleted}"
+    )
+    if not acknowledge_reminder_required or is_organization_deleted:
+        task_logger.info(f"alert group {alert_group_pk} is not in a state for acknowledgement reminder. {log_info}")
         return
 
+    task_logger.info(f"alert group {alert_group_pk} is in a state for acknowledgement reminder. {log_info}")
+
     # unacknowledge_timeout_task uses acknowledged_by_confirmed to check if acknowledgement reminder has been confirmed
     # by the user. Setting to None here to indicate that the user has not confirmed the acknowledgement reminder
     alert_group.acknowledged_by_confirmed = None
@@ -80,13 +86,11 @@ def unacknowledge_timeout_task(alert_group_pk: int, unacknowledge_process_id: st
         if unacknowledge_process_id != alert_group.last_unique_unacknowledge_process_id:
             return
 
+    organization = alert_group.channel.organization
+
     # Get timeout values
-    acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[
-        alert_group.channel.organization.acknowledge_remind_timeout
-    ]
-    unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[
-        alert_group.channel.organization.unacknowledge_timeout
-    ]
+    acknowledge_reminder_timeout = Organization.ACKNOWLEDGE_REMIND_DELAY[organization.acknowledge_remind_timeout]
+    unacknowledge_timeout = Organization.UNACKNOWLEDGE_TIMEOUT_DELAY[organization.unacknowledge_timeout]
 
     # Don't proceed if the alert group is not in a state for auto-unacknowledge
     unacknowledge_required = (
@@ -96,16 +100,28 @@ def unacknowledge_timeout_task(alert_group_pk: int, unacknowledge_process_id: st
         and acknowledge_reminder_timeout
         and unacknowledge_timeout
     )
-    if not unacknowledge_required:
-        task_logger.info("AlertGroup is not in a state for unacknowledge")
+    is_organization_deleted = organization.deleted_at is not None
+    log_info = (
+        f"acknowledge_reminder_timeout option: {acknowledge_reminder_timeout},"
+        f"unacknowledge_timeout option: {unacknowledge_timeout},"
+        f"organization ppk: {organization.public_primary_key},"
+        f"organization is deleted: {is_organization_deleted}"
+    )
+    if not unacknowledge_required or is_organization_deleted:
+        task_logger.info(f"alert group {alert_group_pk} is not in a state for unacknowledge by timeout. {log_info}")
         return
 
     if alert_group.acknowledged_by_confirmed:  # acknowledgement reminder was confirmed by the user
         acknowledge_reminder_task.apply_async(
             (alert_group_pk, unacknowledge_process_id), countdown=acknowledge_reminder_timeout - unacknowledge_timeout
         )
+        task_logger.info(
+            f"Acknowledgement reminder was confirmed by user. Rescheduling acknowledge_reminder_task..."
+            f"alert group: {alert_group_pk}, {log_info}"
+        )
         return
 
+    task_logger.info(f"alert group {alert_group_pk} is in a state for unacknowledge by timeout. {log_info}")
     # If acknowledgement reminder wasn't confirmed by the user, unacknowledge the alert group and start escalation again
     log_record = alert_group.log_records.create(
         type=AlertGroupLogRecord.TYPE_AUTO_UN_ACK, author=alert_group.acknowledged_by_user

diff --git a/engine/apps/alerts/tests/test_acknowledge_reminder.py b/engine/apps/alerts/tests/test_acknowledge_reminder.py
@@ -299,3 +299,43 @@ def test_unacknowledge_timeout_task_no_unacknowledge(
     )
 
     assert not alert_group.log_records.exists()
+
+
+@patch.object(acknowledge_reminder_task, "apply_async")
+@patch.object(unacknowledge_timeout_task, "apply_async")
+@pytest.mark.django_db
+def test_ack_reminder_skip_deleted_org(
+    mock_acknowledge_reminder_task,
+    mock_unacknowledge_timeout_task,
+    ack_reminder_test_setup,
+):
+    organization, alert_group, user = ack_reminder_test_setup()
+    organization.deleted_at = timezone.now()
+    organization.save()
+
+    acknowledge_reminder_task(alert_group.pk, TASK_ID)
+
+    mock_unacknowledge_timeout_task.assert_not_called()
+    mock_acknowledge_reminder_task.assert_not_called()
+
+    assert not alert_group.log_records.exists()
+
+
+@patch.object(acknowledge_reminder_task, "apply_async")
+@patch.object(unacknowledge_timeout_task, "apply_async")
+@pytest.mark.django_db
+def test_unacknowledge_timeout_task_skip_deleted_org(
+    mock_acknowledge_reminder_task,
+    mock_unacknowledge_timeout_task,
+    ack_reminder_test_setup,
+):
+    organization, alert_group, user = ack_reminder_test_setup()
+    organization.deleted_at = timezone.now()
+    organization.save()
+
+    unacknowledge_timeout_task(alert_group.pk, TASK_ID)
+
+    mock_unacknowledge_timeout_task.assert_not_called()
+    mock_acknowledge_reminder_task.assert_not_called()
+
+    assert not alert_group.log_records.exists()