diff --git a/CHANGELOG.md b/CHANGELOG.md index 513d1b34..65182e34 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py b/engine/apps/alerts/tasks/notify_ical_schedule_shift.py index a23f2a4b..c8bfec12 100644 --- a/engine/apps/alerts/tasks/notify_ical_schedule_shift.py +++ b/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) diff --git a/engine/apps/schedules/tasks/refresh_ical_files.py b/engine/apps/schedules/tasks/refresh_ical_files.py index 918564fb..20204359 100644 --- a/engine/apps/schedules/tasks/refresh_ical_files.py +++ b/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, diff --git a/engine/apps/schedules/tests/test_tasks_refresh_ical_files.py b/engine/apps/schedules/tests/test_tasks_refresh_ical_files.py new file mode 100644 index 00000000..0a7ed738 --- /dev/null +++ b/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