diff --git a/CHANGELOG.md b/CHANGELOG.md index b01bb660..f8f0b7d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add helm chart support for twilio existing secrets by @atownsend247 ([#1435](https://github.com/grafana/oncall/pull/1435)) +### Changed + +- Update shift API to use a default interval value (`1`) when a `frequency` is set and no `interval` is given + ## v1.2.14 (2023-04-19) ### Fixed diff --git a/docs/sources/oncall-api-reference/on_call_shifts.md b/docs/sources/oncall-api-reference/on_call_shifts.md index dd108676..ff70f9c2 100644 --- a/docs/sources/oncall-api-reference/on_call_shifts.md +++ b/docs/sources/oncall-api-reference/on_call_shifts.md @@ -55,7 +55,7 @@ The above command returns JSON structured in the following way: | `start` | No | Yes | Start time of the on-call shift. This parameter takes a date format as `yyyy-MM-dd'T'HH:mm:ss` (for example "2020-09-05T08:00:00"). | | `duration` | No | Yes | Duration of the event. | | `frequency` | No | If type = `recurrent_event` or `rolling_users` | One of: `hourly`, `daily`, `weekly`, `monthly`. | -| `interval` | No | Optional | This parameter takes a positive integer that represents the intervals that the recurrence rule repeats. | +| `interval` | No | Optional | This parameter takes a positive integer that represents the intervals that the recurrence rule repeats. If `frequency` is set, the default assumed value for this will be `1`. | | `until` | No | Optional | When the recurrence rule ends (endless if None). This parameter takes a date format as `yyyy-MM-dd'T'HH:mm:ss` (for example "2020-09-05T08:00:00"). | | `week_start` | No | Optional | Start day of the week in iCal format. One of: `SU` (Sunday), `MO` (Monday), `TU` (Tuesday), `WE` (Wednesday), `TH` (Thursday), `FR` (Friday), `SA` (Saturday). Default: `SU`. | | `by_day` | No | Optional | List of days in iCal format. Valid values are: `SU`, `MO`, `TU`, `WE`, `TH`, `FR`, `SA`. | diff --git a/engine/apps/api/serializers/on_call_shifts.py b/engine/apps/api/serializers/on_call_shifts.py index cb1b1505..b4a49822 100644 --- a/engine/apps/api/serializers/on_call_shifts.py +++ b/engine/apps/api/serializers/on_call_shifts.py @@ -120,6 +120,10 @@ class OnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer): ) if frequency not in (CustomOnCallShift.FREQUENCY_WEEKLY, CustomOnCallShift.FREQUENCY_DAILY) and by_day: raise serializers.ValidationError({"by_day": ["Cannot set days value for this frequency type"]}) + if interval is None: + raise serializers.ValidationError( + {"interval": ["If frequency is set, interval must be a positive integer"]} + ) if frequency == CustomOnCallShift.FREQUENCY_DAILY and by_day and interval > len(by_day): raise serializers.ValidationError( {"interval": ["Interval must be less than or equal to the number of selected days"]} diff --git a/engine/apps/api/tests/test_oncall_shift.py b/engine/apps/api/tests/test_oncall_shift.py index 77ca0460..fab47f11 100644 --- a/engine/apps/api/tests/test_oncall_shift.py +++ b/engine/apps/api/tests/test_oncall_shift.py @@ -41,7 +41,7 @@ def test_create_on_call_shift_rotation(on_call_shift_internal_api_setup, make_us "rotation_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), "until": None, "frequency": 1, - "interval": None, + "interval": 1, "by_day": [ CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.MONDAY], CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.FRIDAY], @@ -451,7 +451,7 @@ def test_update_old_on_call_shift_with_future_version( "rotation_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), "until": None, "frequency": CustomOnCallShift.FREQUENCY_DAILY, - "interval": None, + "interval": 1, "by_day": None, "rolling_users": [[user1.public_primary_key]], } @@ -715,7 +715,7 @@ def test_create_on_call_shift_invalid_data_until(on_call_shift_internal_api_setu "rotation_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), "until": (start_date - timezone.timedelta(hours=2)).strftime("%Y-%m-%dT%H:%M:%SZ"), "frequency": 1, - "interval": None, + "interval": 1, "by_day": [ CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.MONDAY], CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.FRIDAY], @@ -828,26 +828,32 @@ def test_create_on_call_shift_invalid_data_interval(on_call_shift_internal_api_s assert response.status_code == status.HTTP_400_BAD_REQUEST assert response.data["interval"][0] == "Cannot set interval for non-recurrent shifts" - # by_day, daily, interval > len(by_day) - data = { - "title": "Test Shift 2", - "type": CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, - "schedule": schedule.public_primary_key, - "priority_level": 0, - "shift_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), - "shift_end": (start_date + timezone.timedelta(hours=2)).strftime("%Y-%m-%dT%H:%M:%SZ"), - "rotation_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), - "until": None, - "frequency": CustomOnCallShift.FREQUENCY_DAILY, - "interval": 2, - "by_day": [CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.MONDAY]], - "rolling_users": [[user1.public_primary_key]], - } + invalid_intervals = ( + (None, "If frequency is set, interval must be a positive integer"), + (2, "Interval must be less than or equal to the number of selected days"), + ) + for interval, expected_error in invalid_intervals: + # by_day, daily shift + data = { + "title": "Test Shift 2", + "type": CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, + "schedule": schedule.public_primary_key, + "priority_level": 0, + "shift_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), + "shift_end": (start_date + timezone.timedelta(hours=2)).strftime("%Y-%m-%dT%H:%M:%SZ"), + "rotation_start": start_date.strftime("%Y-%m-%dT%H:%M:%SZ"), + "until": None, + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "by_day": [CustomOnCallShift.ICAL_WEEKDAY_MAP[CustomOnCallShift.MONDAY]], + "rolling_users": [[user1.public_primary_key]], + } + if interval: + data["interval"] = interval - response = client.post(url, data, format="json", **make_user_auth_headers(user1, token)) + response = client.post(url, data, format="json", **make_user_auth_headers(user1, token)) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.data["interval"][0] == "Interval must be less than or equal to the number of selected days" + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.data["interval"][0] == expected_error @pytest.mark.django_db @@ -1275,6 +1281,7 @@ def test_on_call_shift_preview_permissions( "rolling_users": [[user.public_primary_key]], "priority_level": 2, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } url = reverse("api-internal:oncall_shifts-preview") @@ -1299,6 +1306,7 @@ def test_on_call_shift_preview_missing_data( "rolling_users": [[user.public_primary_key]], "priority_level": 2, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } url = reverse("api-internal:oncall_shifts-preview") @@ -1336,6 +1344,7 @@ def test_on_call_shift_preview( "duration": timezone.timedelta(hours=9), "priority_level": 1, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, "schedule": schedule, } on_call_shift = make_on_call_shift( @@ -1357,6 +1366,7 @@ def test_on_call_shift_preview( "rolling_users": [[other_user.public_primary_key]], "priority_level": 2, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } response = client.post(url, shift_data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK @@ -1450,6 +1460,7 @@ def test_on_call_shift_preview_without_users( "rolling_users": [], "priority_level": 2, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } response = client.post(url, shift_data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK @@ -1528,6 +1539,7 @@ def test_on_call_shift_preview_merge_events( "rolling_users": [[user.public_primary_key, other_user.public_primary_key]], "priority_level": 2, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } response = client.post(url, shift_data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK @@ -1637,6 +1649,7 @@ def test_on_call_shift_preview_update( "rolling_users": [[other_user.public_primary_key]], "priority_level": 1, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } response = client.post(url, shift_data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK @@ -1752,6 +1765,7 @@ def test_on_call_shift_preview_update_not_started_reuse_pk( "rolling_users": [[other_user.public_primary_key]], "priority_level": 1, "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "interval": 1, } response = client.post(url, shift_data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK diff --git a/engine/apps/public_api/serializers/on_call_shifts.py b/engine/apps/public_api/serializers/on_call_shifts.py index 76d922e2..1a02d862 100644 --- a/engine/apps/public_api/serializers/on_call_shifts.py +++ b/engine/apps/public_api/serializers/on_call_shifts.py @@ -120,7 +120,10 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer def create(self, validated_data): self._validate_frequency_and_week_start( - validated_data["type"], validated_data.get("frequency"), validated_data.get("week_start") + validated_data["type"], + validated_data.get("frequency"), + validated_data.get("interval", 1), # if field is missing, the default value will be used + validated_data.get("week_start"), ) validated_data = self._correct_validated_data(validated_data["type"], validated_data) self._validate_start_rotation_from_user_index( @@ -196,12 +199,15 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer result.append(users_dict) return result - def _validate_frequency_and_week_start(self, event_type, frequency, week_start): + def _validate_frequency_and_week_start(self, event_type, frequency, interval, week_start): if event_type not in (CustomOnCallShift.TYPE_SINGLE_EVENT, CustomOnCallShift.TYPE_OVERRIDE): if frequency is None: raise BadRequest(detail="Field 'frequency' is required for this on-call shift type") elif frequency == CustomOnCallShift.FREQUENCY_WEEKLY and week_start is None: raise BadRequest(detail="Field 'week_start' is required for frequency type 'weekly'") + # frequency is not None + if interval is None: + raise BadRequest(detail="Field 'interval' must be a positive integer") def _validate_frequency_daily(self, event_type, frequency, interval, by_day, by_monthday): if event_type == CustomOnCallShift.TYPE_ROLLING_USERS_EVENT: @@ -337,6 +343,9 @@ class CustomOnCallShiftSerializer(EagerLoadingMixin, serializers.ModelSerializer validated_data[field] = None if validated_data.get("start") is not None: validated_data["start"] = validated_data["start"].replace(tzinfo=None) + if validated_data.get("frequency") and validated_data.get("interval") is None: + # if there is frequency but no interval is given, default to 1 + validated_data["interval"] = 1 # Populate "rolling_users" field using "users" field for web overrides # This emulates the behavior of the web UI, which creates overrides populating the rolling_users field @@ -367,10 +376,10 @@ class CustomOnCallShiftUpdateSerializer(CustomOnCallShiftSerializer): "start_rotation_from_user_index", instance.start_rotation_from_user_index ) week_start = validated_data.get("week_start") - if frequency != instance.frequency: - self._validate_frequency_and_week_start(event_type, frequency, week_start) - interval = validated_data.get("interval", instance.interval) + if frequency != instance.frequency: + self._validate_frequency_and_week_start(event_type, frequency, interval, week_start) + by_day = validated_data.get("by_day", instance.by_day) by_monthday = validated_data.get("by_monthday", instance.by_monthday) self._validate_frequency_daily(event_type, frequency, interval, by_day, by_monthday) diff --git a/engine/apps/public_api/tests/test_on_call_shifts.py b/engine/apps/public_api/tests/test_on_call_shifts.py index 925ab493..a25fb68a 100644 --- a/engine/apps/public_api/tests/test_on_call_shifts.py +++ b/engine/apps/public_api/tests/test_on_call_shifts.py @@ -178,6 +178,88 @@ def test_create_on_call_shift(make_organization_and_user_with_token): assert response.data == result +@pytest.mark.django_db +def test_create_on_call_shift_using_default_interval(make_organization_and_user_with_token): + _, user, token = make_organization_and_user_with_token() + client = APIClient() + + url = reverse("api-public:on_call_shifts-list") + + start = datetime.datetime.now() + until = start + datetime.timedelta(days=30) + data = { + "team_id": None, + "name": "test name", + "type": "recurrent_event", + "level": 1, + "start": start.strftime("%Y-%m-%dT%H:%M:%S"), + "rotation_start": start.strftime("%Y-%m-%dT%H:%M:%S"), + "duration": 10800, + "users": [user.public_primary_key], + "week_start": "MO", + "frequency": "weekly", + "until": until.strftime("%Y-%m-%dT%H:%M:%S"), + "by_day": ["MO", "WE", "FR"], + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + on_call_shift = CustomOnCallShift.objects.get(public_primary_key=response.data["id"]) + + expected = { + "id": on_call_shift.public_primary_key, + "team_id": None, + "name": data["name"], + "type": "recurrent_event", + "time_zone": None, + "level": data["level"], + "start": data["start"], + "rotation_start": data["rotation_start"], + "duration": data["duration"], + "frequency": data["frequency"], + "interval": 1, + "until": data["until"], + "week_start": data["week_start"], + "by_day": data["by_day"], + "users": [user.public_primary_key], + "by_month": None, + "by_monthday": None, + } + + assert response.status_code == status.HTTP_201_CREATED + assert response.data == expected + + +@pytest.mark.django_db +def test_create_on_call_shift_using_none_interval_fails(make_organization_and_user_with_token): + _, user, token = make_organization_and_user_with_token() + client = APIClient() + + url = reverse("api-public:on_call_shifts-list") + + start = datetime.datetime.now() + until = start + datetime.timedelta(days=30) + data = { + "team_id": None, + "name": "test name", + "type": "recurrent_event", + "level": 1, + "start": start.strftime("%Y-%m-%dT%H:%M:%S"), + "rotation_start": start.strftime("%Y-%m-%dT%H:%M:%S"), + "duration": 10800, + "users": [user.public_primary_key], + "week_start": "MO", + "frequency": "weekly", + "interval": None, + "until": until.strftime("%Y-%m-%dT%H:%M:%S"), + "by_day": ["MO", "WE", "FR"], + } + + response = client.post(url, data=data, format="json", HTTP_AUTHORIZATION=f"{token}") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == {"detail": "Field 'interval' must be a positive integer"} + + @pytest.mark.django_db def test_create_override_on_call_shift(make_organization_and_user_with_token): _, user, token = make_organization_and_user_with_token()