fix PUT /api/v1/escalation_policies/<id> issue related to updating fr…

…om_time and to_time (#3581)

# Which issue(s) this PR fixes

Closes grafana/oncall-private#2373

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] 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)

CHANGELOG.md

@@ -33,6 +33,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Add backend for multi-stack support for mobile-app @Ferril ([#3500](https://github.com/grafana/oncall/pull/3500))
 
+### Fixed
+
+- Fix PUT /api/v1/escalation_policies/id issue when updating `from_time` and `to_time` by @joeyorlando ([#3581](https://github.com/grafana/oncall/pull/3581))
+
 ## v1.3.78 (2023-12-12)
 
 ### Changed

engine/apps/public_api/serializers/escalation_policies.py

@@ -10,7 +10,6 @@
 from apps.user_management.models import User
 from apps.webhooks.models import Webhook
 from common.api_helpers.custom_fields import (
-    CustomTimeField,
     OrganizationFilteredPrimaryKeyRelatedField,
     UsersFilteredByOrganizationField,
 )
@@ -78,8 +77,14 @@ class EscalationPolicySerializer(EagerLoadingMixin, OrderedModelSerializer):
         source="custom_webhook",
     )
     important = serializers.BooleanField(required=False)
-    notify_if_time_from = CustomTimeField(required=False, source="from_time")
-    notify_if_time_to = CustomTimeField(required=False, source="to_time")
+
+    TIME_FORMAT = "%H:%M:%SZ"
+    notify_if_time_from = serializers.TimeField(
+        required=False, source="from_time", format=TIME_FORMAT, input_formats=[TIME_FORMAT]
+    )
+    notify_if_time_to = serializers.TimeField(
+        required=False, source="to_time", format=TIME_FORMAT, input_formats=[TIME_FORMAT]
+    )
 
     class Meta:
         model = EscalationPolicy

engine/apps/public_api/tests/test_escalation_policies.py

@@ -418,3 +418,41 @@ def test_update_escalation_policy_using_button_to_webhook(
     assert response.data == serializer.data
     # step is migrated
     assert escalation_policy.step == EscalationPolicy.STEP_TRIGGER_CUSTOM_WEBHOOK
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "value,expected_status",
+    [
+        (5, status.HTTP_400_BAD_REQUEST),
+        ("5", status.HTTP_400_BAD_REQUEST),
+        ("5:00", status.HTTP_400_BAD_REQUEST),
+        ("05:00:00", status.HTTP_400_BAD_REQUEST),
+        ("05:00:00Z", status.HTTP_200_OK),
+    ],
+)
+def test_update_escalation_policy_from_and_to_time(
+    make_organization_and_user_with_token,
+    make_escalation_chain,
+    make_escalation_policy,
+    value,
+    expected_status,
+):
+    organization, _, token = make_organization_and_user_with_token()
+    escalation_chain = make_escalation_chain(organization)
+    escalation_policy = make_escalation_policy(escalation_chain, EscalationPolicy.STEP_NOTIFY_IF_TIME)
+
+    client = APIClient()
+    url = reverse("api-public:escalation_policies-detail", kwargs={"pk": escalation_policy.public_primary_key})
+
+    for field in ["notify_if_time_from", "notify_if_time_to"]:
+        response = client.put(url, data={field: value}, format="json", HTTP_AUTHORIZATION=token)
+
+        assert response.status_code == expected_status
+
+        if expected_status == status.HTTP_200_OK:
+            escalation_policy = EscalationPolicy.objects.get(public_primary_key=response.data["id"])
+            serializer = EscalationPolicySerializer(escalation_policy)
+            assert response.data == serializer.data
+        else:
+            assert response.json()[field][0] == "Time has wrong format. Use one of these formats instead: hh:mm:ssZ."

engine/common/api_helpers/custom_fields.py

@@ -1,5 +1,3 @@
-import time
-
 from django.core.exceptions import ObjectDoesNotExist
 from rest_framework import fields, serializers
 from rest_framework.exceptions import ValidationError
@@ -102,25 +100,6 @@ def to_internal_value(self, data):
         return queryset.filter(organization=request.user.organization, public_primary_key__in=data).distinct()
 
 
-class CustomTimeField(fields.TimeField):
-    def to_representation(self, value):
-        result = super().to_representation(value)
-        if result[-1] != "Z":
-            result += "Z"
-        return result
-
-    def to_internal_value(self, data):
-        TIME_FORMAT_LEN = len("00:00:00Z")
-        if len(data) == TIME_FORMAT_LEN:
-            try:
-                time.strptime(data, "%H:%M:%SZ")
-            except ValueError:
-                raise BadRequest(detail="Invalid time format, should be '00:00:00Z'")
-        else:
-            raise BadRequest(detail="Invalid time format, should be '00:00:00Z'")
-        return data
-
-
 class RouteIdField(fields.CharField):
     def to_internal_value(self, data):
         try:

grafana-plugin/e2e-tests/escalationChains/escalationPolicy.test.ts

@@ -1,4 +1,4 @@
-import { expect, test } from '../fixtures';
+import { Locator, expect, test } from '../fixtures';
 import { createEscalationChain, EscalationStep, selectEscalationStepValue } from '../utils/escalationChain';
 import { generateRandomValue } from '../utils/forms';
 
@@ -15,3 +15,45 @@ test('escalation policy does not go back to "Default" after adding users to noti
   await page.reload();
   await expect(page.getByText('Important')).toBeVisible();
 });
+
+// TODO: unskip when https://github.com/grafana/oncall/issues/3585 is patched
+test.skip('from_time and to_time for "Continue escalation if current UTC time is in range" escalation step type can be properly updated', async ({
+  adminRolePage,
+}) => {
+  const FROM_TIME = '10:31';
+  const TO_TIME = '10:32';
+
+  const { page } = adminRolePage;
+  const escalationChainName = generateRandomValue();
+
+  // create escalation step w/ Continue escalation if current UTC time is in policy step
+  await createEscalationChain(page, escalationChainName, EscalationStep.ContinueEscalationIfCurrentUTCTimeIsIn);
+
+  const _getFromTimeInput = () => page.locator('[data-testid="time-range-from"] >> input');
+  const _getToTimeInput = () => page.locator('[data-testid="time-range-to"] >> input');
+
+  const clickAndInputValue = async (locator: Locator, value: string) => {
+    // the first click opens up dropdown which contains the time selector scrollable lists
+    await locator.click();
+
+    // the second click focuses on the input where we can actually type the time instead, much easier
+    const actualInput = page.locator('input[class="rc-time-picker-panel-input"]');
+    await actualInput.click();
+    await actualInput.selectText();
+    await actualInput.fill(value);
+
+    // click anywhere to close the dropdown
+    await page.click('body');
+  };
+
+  // update from and to time values
+  await clickAndInputValue(_getFromTimeInput(), FROM_TIME);
+  await clickAndInputValue(_getToTimeInput(), TO_TIME);
+
+  // reload and check that these values have been persisted
+  await page.reload();
+  await page.waitForLoadState('networkidle');
+
+  expect(await _getFromTimeInput().textContent()).toBe(FROM_TIME);
+  expect(await _getToTimeInput().textContent()).toBe(FROM_TIME);
+});

grafana-plugin/e2e-tests/utils/escalationChain.ts

@@ -6,9 +6,10 @@ import { goToOnCallPage } from './navigation';
 export enum EscalationStep {
   NotifyUsers = 'Notify users',
   NotifyUsersFromOnCallSchedule = 'Notify users from on-call schedule',
+  ContinueEscalationIfCurrentUTCTimeIsIn = 'Continue escalation if current UTC time is in range',
 }
 
-const escalationStepValuePlaceholder: Record<EscalationStep, string> = {
+const escalationStepValuePlaceholder: Partial<Record<EscalationStep, string>> = {
   [EscalationStep.NotifyUsers]: 'Select User',
   [EscalationStep.NotifyUsersFromOnCallSchedule]: 'Select Schedule',
 };

grafana-plugin/src/components/TimeRange/TimeRange.tsx

@@ -92,11 +92,15 @@ const TimeRange = (props: TimeRangeProps) => {
   return (
     <div className={cx('root', className)}>
       <HorizontalGroup wrap>
-        {/* @ts-ignore actually TimeOfDayPicker uses Moment objects */}
-        <TimeOfDayPicker disabled={disabled} value={from} minuteStep={5} onChange={handleChangeFrom} />
+        <div data-testid="time-range-from">
+          {/* @ts-ignore actually TimeOfDayPicker uses Moment objects */}
+          <TimeOfDayPicker disabled={disabled} value={from} minuteStep={5} onChange={handleChangeFrom} />
+        </div>
         to
-        {/* @ts-ignore actually TimeOfDayPicker uses Moment objects */}
-        <TimeOfDayPicker disabled={disabled} value={to} minuteStep={5} onChange={handleChangeTo} />
+        <div data-testid="time-range-to">
+          {/* @ts-ignore actually TimeOfDayPicker uses Moment objects */}
+          <TimeOfDayPicker disabled={disabled} value={to} minuteStep={5} onChange={handleChangeTo} />
+        </div>
         {showNextDayTip && 'next day'}
       </HorizontalGroup>
     </div>