Matiasb/fix task refresh ical when empty value (#1401)

This should fix task error as seen in logs, trying to parse an empty
string as ical value:
```
Task apps.schedules.tasks.refresh_ical_files.refresh_ical_file[] raised unexpected: ValueError("Found no components where exactly one is required: ''")
```

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixed importing of global grafana styles ([672](https://github.com/grafana/oncall/issues/672))
 - Fixed UI permission related bug where Editors could not export their user iCal link
 - Fixed error when a shift is created using Etc/UTC as timezone
+- Fixed issue with refresh ical file task not considering empty string values
 
 ### Changed
 

engine/apps/alerts/tasks/notify_ical_schedule_shift.py

@@ -254,9 +254,7 @@ def notify_ical_schedule_shift(schedule_pk):
     prev_and_current_ical_files = schedule.get_prev_and_current_ical_files()
 
     for prev_ical_file, current_ical_file in prev_and_current_ical_files:
-        if prev_ical_file is not None and (
-            current_ical_file is None or not is_icals_equal(current_ical_file, prev_ical_file)
-        ):
+        if prev_ical_file and (not current_ical_file or not is_icals_equal(current_ical_file, prev_ical_file)):
             # If icals are not equal then compare current_events from them
             is_prev_ical_diff = True
             prev_calendar = icalendar.Calendar.from_ical(prev_ical_file)

engine/apps/schedules/tasks/refresh_ical_files.py

@@ -41,22 +41,29 @@ def refresh_ical_file(schedule_pk):
         notify_ical_schedule_shift.apply_async((schedule.pk,))
 
     run_task_primary = False
-    if schedule.cached_ical_file_primary is not None:
-        if schedule.prev_ical_file_primary is None:
+    if schedule.cached_ical_file_primary:
+        # ie. primary schedule is not empty (None -> no ical, "" -> empty cached value)
+        if not schedule.prev_ical_file_primary:
+            # prev value is empty
             run_task_primary = True
             task_logger.info(f"run_task_primary {schedule_pk} {run_task_primary} prev_ical_file_primary is None")
         else:
+            # prev value is not empty, we need to compare
             run_task_primary = not is_icals_equal(
                 schedule.cached_ical_file_primary,
                 schedule.prev_ical_file_primary,
             )
             task_logger.info(f"run_task_primary {schedule_pk} {run_task_primary} icals not equal")
+
     run_task_overrides = False
-    if schedule.cached_ical_file_overrides is not None:
-        if schedule.prev_ical_file_overrides is None:
+    if schedule.cached_ical_file_overrides:
+        # ie. overrides schedule is not empty (None -> no ical, "" -> empty cached value)
+        if not schedule.prev_ical_file_overrides:
+            # prev value is empty
             run_task_overrides = True
             task_logger.info(f"run_task_overrides {schedule_pk} {run_task_primary} prev_ical_file_overrides is None")
         else:
+            # prev value is not empty, we need to compare
             run_task_overrides = not is_icals_equal(
                 schedule.cached_ical_file_overrides,
                 schedule.prev_ical_file_overrides,

engine/apps/schedules/tests/test_tasks_refresh_ical_files.py

@@ -0,0 +1,58 @@
+from unittest.mock import patch
+
+import pytest
+
+from apps.schedules.models import OnCallScheduleICal
+from apps.schedules.tasks.refresh_ical_files import refresh_ical_file
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "cached_ical_primary,prev_ical_primary,cached_ical_overrides,prev_ical_overrides,run_task",
+    [
+        ("ical data", "", None, None, True),
+        (None, None, "ical data", "", True),
+        ("", "ical data", None, None, False),
+        (None, None, "", "ical data", False),
+        ("ical data", "diff data", None, None, True),
+        ("ical data", "ical data", None, None, False),
+        (None, None, "ical data", "diff data", True),
+        (None, None, "ical data", "ical data", False),
+    ],
+)
+def test_refresh_ical_file_trigger_run(
+    cached_ical_primary,
+    prev_ical_primary,
+    cached_ical_overrides,
+    prev_ical_overrides,
+    run_task,
+    make_organization,
+    make_schedule,
+):
+    organization = make_organization()
+    # set schedule ical file status *after* refresh
+    schedule = make_schedule(
+        organization,
+        schedule_class=OnCallScheduleICal,
+        cached_ical_file_primary=cached_ical_primary,
+        prev_ical_file_primary=prev_ical_primary,
+        cached_ical_file_overrides=cached_ical_overrides,
+        prev_ical_file_overrides=prev_ical_overrides,
+    )
+
+    # patch ical comparison to string compare
+    with patch("apps.schedules.tasks.refresh_ical_files.is_icals_equal", side_effect=lambda a, b: a == b):
+        # patch schedule refresh to avoid changing schedule status (keep as defined above)
+        with patch("apps.schedules.models.OnCallSchedule.refresh_ical_file", return_value=None):
+            # do not trigger tasks for real
+            with patch("apps.schedules.tasks.refresh_ical_files.notify_ical_schedule_shift"):
+                with patch(
+                    "apps.schedules.tasks.refresh_ical_files.notify_about_empty_shifts_in_schedule"
+                ) as mock_notify_empty:
+                    with patch(
+                        "apps.schedules.tasks.refresh_ical_files.notify_about_gaps_in_schedule"
+                    ) as mock_notify_gaps:
+                        refresh_ical_file(schedule.pk)
+
+        assert mock_notify_empty.apply_async.called == run_task
+        assert mock_notify_gaps.apply_async.called == run_task