v1.13.1

.github/workflows/linting-and-tests.yml

@@ -85,6 +85,59 @@ jobs:
           python manage.py makemigrations --check
           python manage.py lintmigrations
 
+  # the following CI check is to prevent developers from dropping columns in a way that could cause downtime
+  # (the proper way to drop columns is documented in dev/README.md)
+  #
+  # we've been bitten by this before (see https://raintank-corp.slack.com/archives/C081TNWM73N as an example)
+  ensure-database-migrations-drop-columns-the-correct-way:
+    name: "Ensure database migrations drop columns the correct way"
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout PR code
+        uses: actions/checkout@v3
+        with:
+          # Fetch all history so we can compare with the base branch
+          fetch-depth: 0
+          # Checkout the head commit of the PR
+          ref: ${{ github.event.pull_request.head.sha }}
+
+      - name: Fetch base branch
+        run: git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }}
+
+      - name: Check for RemoveField in Migrations
+        # yamllint disable rule:line-length
+        run: |
+          # Get the list of files changed in the PR
+          git diff --name-only ${{ github.event.pull_request.base.ref }}...${{ github.event.pull_request.head.sha }} > changed_files.txt
+
+          # Filter for migration files
+          grep -E '^.*/migrations/.*\.py$' changed_files.txt > migration_files.txt || true
+
+          # Initialize a flag
+          FAILED=0
+
+          # Check each migration file for 'migrations.RemoveField'
+          if [ -s migration_files.txt ]; then
+            while IFS= read -r file; do
+              echo "Checking $file for migrations.RemoveField..."
+              if grep -q 'migrations.RemoveField' "$file"; then
+                echo "❌ Error: Found migrations.RemoveField in $file"
+                FAILED=1
+              else
+                echo "✅ No RemoveField found in $file"
+              fi
+            done < migration_files.txt
+          else
+            echo "No migration files changed."
+          fi
+
+          # Fail the job if RemoveField was found
+          if [ "$FAILED" -eq 1 ]; then
+            echo "❌ Error: Found migrations.RemoveField in one or more migration files. Please check out our documentation at https://github.com/grafana/oncall/tree/dev/dev#removing-a-nullable-field-from-a-model on how to properly drop columns."
+            exit 1
+          fi
+        # yamllint enable rule:line-length
+
   unit-test-helm-chart:
     name: "Helm Chart Unit Tests"
     runs-on: ubuntu-latest-16-cores
@@ -193,7 +246,7 @@ jobs:
       ONCALL_TESTING_RBAC_ENABLED: ${{ matrix.rbac_enabled }}
     services:
       redis_test:
-        image: redis:7.0.5
+        image: redis:7.0.15
         ports:
           - 6379:6379
         options: >-

dev/scripts/generate-fake-data/requirements.txt

@@ -1,3 +1,3 @@
-aiohttp==3.10.2
+aiohttp==3.10.11
 Faker==16.4.0
 tqdm==4.66.3

docker-compose-developer.yml

@@ -172,7 +172,7 @@ services:
   redis:
     container_name: redis
     labels: *oncall-labels
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     ports:
       - "6379:6379"

docker-compose-mysql-rabbitmq.yml

@@ -87,7 +87,7 @@ services:
       retries: 10
 
   redis:
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     expose:
       - 6379

docker-compose.yml

@@ -54,7 +54,7 @@ services:
         condition: service_healthy
 
   redis:
-    image: redis:7.0.5
+    image: redis:7.0.15
     restart: always
     expose:
       - 6379

engine/apps/alerts/incident_log_builder/incident_log_builder.py

@@ -152,6 +152,15 @@ class IncidentLogBuilder:
                     Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_TRIGGERED)
                     & Q(notification_policy__step=UserNotificationPolicy.Step.WAIT)
                 )
+                # Exclude SUCCESS + ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED, these cause confusions as the user
+                # has already been notified by another path so this step should not be displayed, although it is kept
+                # for auditing.
+                | Q(
+                    Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS)
+                    & Q(
+                        notification_error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED
+                    )
+                )
             )
             .select_related("author")
             .distinct()

engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py

@@ -12,14 +12,6 @@ class Migration(migrations.Migration):
 
     operations = [
         linter.IgnoreMigration(),
-        migrations.RemoveField(
-            model_name='channelfilter',
-            name='_slack_channel_id',
-        ),
-        migrations.RemoveField(
-            model_name='resolutionnoteslackmessage',
-            name='_slack_channel_id',
-        ),
         migrations.DeleteModel(
             name='AlertGroupPostmortem',
         ),

engine/apps/alerts/migrations/0067_remove_channelfilter__slack_channel_id_state.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:21
+
+import common.migrations.remove_field
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0066_remove_channelfilter__slack_channel_id_and_more'),
+    ]
+
+    operations = [
+        common.migrations.remove_field.RemoveFieldState(
+            model_name='channelfilter',
+            name='_slack_channel_id',
+        ),
+    ]

engine/apps/alerts/migrations/0068_remove_resolutionnoteslackmessage__slack_channel_id_state.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.16 on 2024-11-20 20:23
+
+import common.migrations.remove_field
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('alerts', '0067_remove_channelfilter__slack_channel_id_state'),
+    ]
+
+    operations = [
+        common.migrations.remove_field.RemoveFieldState(
+            model_name='resolutionnoteslackmessage',
+            name='_slack_channel_id',
+        ),
+    ]

engine/apps/alerts/tasks/notify_user.py

@@ -528,7 +528,7 @@ def perform_notification(log_record_pk, use_default_notification_policy_fallback
                 )
                 UserNotificationPolicyLogRecord(
                     author=user,
-                    type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED,
+                    type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS,
                     notification_policy=notification_policy,
                     reason="Prevented from posting in Slack",
                     alert_group=alert_group,

engine/apps/alerts/tests/test_notify_user.py

@@ -315,7 +315,7 @@ def test_perform_notification_slack_prevent_posting(
 
     mocked_send_slack_notification.assert_not_called()
     last_log_record = UserNotificationPolicyLogRecord.objects.last()
-    assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED
+    assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS
     assert last_log_record.reason == "Prevented from posting in Slack"
     assert (
         last_log_record.notification_error_code

engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py

@@ -20,6 +20,15 @@ EXTRA_LOOKUP_DAYS = 16
 
 
 class AmixrRecurringIcalEventsAdapter(IcalService):
+    def _normalize(self, dt: datetime.datetime) -> datetime.datetime:
+        tz = getattr(dt, "tzinfo", None)
+        if tz:
+            normalized = tz.normalize(dt)
+            if normalized.tzinfo != tz:
+                diff = dt.dst() - normalized.dst()
+                dt = normalized + diff
+        return dt
+
     def get_events_from_ical_between(
         self, calendar: Calendar, start_date: datetime.datetime, end_date: datetime.datetime
     ) -> typing.List[Event]:
@@ -38,6 +47,10 @@ class AmixrRecurringIcalEventsAdapter(IcalService):
             end_date + datetime.timedelta(days=EXTRA_LOOKUP_DAYS),
         )
 
+        for event in events:
+            # account for timezones not being properly calculated when DST changes.
+            event[ICAL_DATETIME_END].dt = self._normalize(event[ICAL_DATETIME_END].dt)
+
         def filter_extra_days(event):
             event_start, event_end = self.get_start_and_end_with_respect_to_event_type(event)
             if event_start > event_end:

engine/apps/schedules/migrations/0020_remove_oncallschedule_channel_state.py

@@ -1,7 +1,7 @@
-# Generated by Django 4.2.16 on 2024-11-06 21:13
+# Generated by Django 4.2.16 on 2024-11-20 20:12
 
+import common.migrations.remove_field
 from django.db import migrations
-import django_migration_linter as linter
 
 
 class Migration(migrations.Migration):
@@ -11,8 +11,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
-        linter.IgnoreMigration(),
-        migrations.RemoveField(
+        common.migrations.remove_field.RemoveFieldState(
             model_name='oncallschedule',
             name='channel',
         ),

engine/apps/schedules/tests/test_on_call_schedule.py

@@ -1818,6 +1818,63 @@ def test_refresh_ical_final_schedule_ok(
                 assert event in expected_events
 
 
+@pytest.mark.django_db
+def test_filter_events_during_dst_change(
+    make_organization,
+    make_user_for_organization,
+    make_schedule,
+    make_on_call_shift,
+):
+    organization = make_organization()
+    u1 = make_user_for_organization(organization)
+
+    schedule = make_schedule(
+        organization,
+        time_zone="America/Chicago",  # UTC-6 or UTC-5 depending on DST
+        schedule_class=OnCallScheduleCalendar,
+    )
+    start_datetime = timezone.datetime(2024, 10, 1, 0, 0, 0, tzinfo=timezone.utc)
+    duration = timezone.timedelta(seconds=60 * 60 * 24 * 7)  # 1 week
+    data = {
+        "start": start_datetime,
+        "rotation_start": start_datetime,
+        "duration": duration,
+        "frequency": CustomOnCallShift.FREQUENCY_WEEKLY,
+        "week_start": CustomOnCallShift.MONDAY,
+    }
+    on_call_shift = make_on_call_shift(
+        organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data
+    )
+    on_call_shift.add_rolling_users([[u1]])
+    schedule.custom_on_call_shifts.add(on_call_shift)
+
+    schedule.refresh_ical_file()
+
+    # week with DST change
+    start_date = timezone.datetime(2024, 11, 4, 0, 0, 0, tzinfo=timezone.utc)
+    end_date = start_date + timezone.timedelta(days=1)
+    events = schedule.filter_events(start_date, end_date)
+    expected = {
+        "start": timezone.datetime(2024, 10, 29, 5, 0, 0, tzinfo=timezone.utc),
+        "end": timezone.datetime(2024, 11, 5, 6, 0, 0, tzinfo=timezone.utc),
+    }
+    assert len(events) == 1
+    returned = {"start": events[0]["start"], "end": events[0]["end"]}
+    assert returned == expected
+
+    # week with DST change back
+    start_date = timezone.datetime(2025, 3, 10, 0, 0, 0, tzinfo=timezone.utc)
+    end_date = start_date + timezone.timedelta(days=1)
+    events = schedule.filter_events(start_date, end_date)
+    expected = {
+        "start": timezone.datetime(2025, 3, 4, 6, 0, 0, tzinfo=timezone.utc),
+        "end": timezone.datetime(2025, 3, 11, 5, 0, 0, tzinfo=timezone.utc),
+    }
+    assert len(events) == 1
+    returned = {"start": events[0]["start"], "end": events[0]["end"]}
+    assert returned == expected
+
+
 @pytest.mark.django_db
 def test_refresh_ical_final_schedule_cancel_deleted_events(
     make_organization,

engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id_state.py

@@ -1,7 +1,7 @@
-# Generated by Django 4.2.16 on 2024-11-06 21:11
+# Generated by Django 4.2.16 on 2024-11-20 17:58
 
+import common.migrations.remove_field
 from django.db import migrations
-import django_migration_linter as linter
 
 
 class Migration(migrations.Migration):
@@ -11,8 +11,7 @@ class Migration(migrations.Migration):
     ]
 
     operations = [
-        linter.IgnoreMigration(),
-        migrations.RemoveField(
+        common.migrations.remove_field.RemoveFieldState(
             model_name='organization',
             name='general_log_channel_id',
         ),