fix: return 404 when deleting non-existent schedule (#952)

* fix: return 404 when deleting non-existent schedule

Validate schedule existence by calling get_schedules before
delete_schedule. Return a 404 error with a descriptive message
if the requested schedule ID is not found on the device.

Closes #723

Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

* test: add test for get_schedules failure in delete_schedule

Cover the case where get_schedules raises an exception during
the pre-deletion validation, ensuring a 500 is returned and
delete_schedule is never called.

Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

* fix: correct errorneous typo to erroneous in test names

Fix misspelling in test_errorneous_delete_schedule_get_schedules_failure,
test_errorneous_delete_schedule_delete_request, and
test_errorneous_create_schedule.

Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Signed-off-by: Tomer Figenblat <tomer@figenblat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

Author: TomerFi

app/tests/test_web_app.py

@@ -500,7 +500,9 @@ async def test_erroneous_get_schedules_get_request(
     ],
 )
 @patch("aioswitcher.api.SwitcherApi.delete_schedule")
+@patch("aioswitcher.api.SwitcherApi.get_schedules")
 async def test_successful_delete_schedule_delete_request(
+    api_get_schedules,
     api_delete_schedule,
     response_serializer,
     response_mock,
@@ -509,12 +511,19 @@ async def test_successful_delete_schedule_delete_request(
     api_client,
     api_uri,
 ):
+    # stub get_schedules to return a schedule with matching id
+    schedule_mock = Mock()
+    schedule_mock.schedule_id = "5"
+    schedules_response = Mock()
+    schedules_response.schedules = [schedule_mock]
+    api_get_schedules.return_value = schedules_response
     # stub api_delete_schedule to return mock response
     api_delete_schedule.return_value = response_mock
     # send delete request for delete_schedule endpoint
     response = await api_client.delete(api_uri, json={webapp.KEY_SCHEDULE: "5"})
     # verify mocks calling
     api_connect.assert_called_once()
+    api_get_schedules.assert_called_once()
     api_delete_schedule.assert_called_once_with("5")
     response_serializer.assert_called_once_with(response_mock)
     api_disconnect.assert_called_once()
@@ -523,6 +532,45 @@ async def test_successful_delete_schedule_delete_request(
     assert_that(await response.json()).contains_entry(fake_serialized_data)
 
 
+@mark.parametrize(
+    "api_uri",
+    [
+        (delete_schedule_uri),
+        (delete_schedule_uri2),
+    ],
+)
+@patch("aioswitcher.api.SwitcherApi.delete_schedule")
+@patch("aioswitcher.api.SwitcherApi.get_schedules")
+async def test_delete_schedule_returns_404_for_nonexistent_schedule(
+    api_get_schedules,
+    api_delete_schedule,
+    response_serializer,
+    api_connect,
+    api_disconnect,
+    api_client,
+    api_uri,
+):
+    # stub get_schedules to return schedules that don't include the requested id
+    schedule_mock = Mock()
+    schedule_mock.schedule_id = "3"
+    schedules_response = Mock()
+    schedules_response.schedules = [schedule_mock]
+    api_get_schedules.return_value = schedules_response
+    # send delete request for delete_schedule endpoint
+    response = await api_client.delete(api_uri, json={webapp.KEY_SCHEDULE: "8"})
+    # verify mocks calling
+    api_connect.assert_called_once()
+    api_get_schedules.assert_called_once()
+    api_delete_schedule.assert_not_called()
+    response_serializer.assert_not_called()
+    api_disconnect.assert_called_once()
+    # assert the expected response
+    assert_that(response.status).is_equal_to(404)
+    assert_that(await response.json()).contains_entry(
+        {"error": "schedule 8 does not exist"}
+    )
+
+
 @patch("aioswitcher.api.SwitcherApi.delete_schedule")
 async def test_delete_schedule_with_faulty_no_schedule_delete_request(
     api_delete_schedule, response_serializer, api_connect, api_disconnect, api_client
@@ -541,16 +589,59 @@ async def test_delete_schedule_with_faulty_no_schedule_delete_request(
     )
 
 
+@patch("aioswitcher.api.SwitcherApi.delete_schedule")
+@patch(
+    "aioswitcher.api.SwitcherApi.get_schedules",
+    side_effect=Exception("get_schedules failure"),
+)
+async def test_erroneous_delete_schedule_get_schedules_failure(
+    api_get_schedules,
+    api_delete_schedule,
+    response_serializer,
+    api_connect,
+    api_disconnect,
+    api_client,
+):
+    # send delete request for delete_schedule endpoint
+    response = await api_client.delete(
+        delete_schedule_uri, json={webapp.KEY_SCHEDULE: "5"}
+    )
+    # verify mocks calling
+    api_connect.assert_called_once()
+    api_get_schedules.assert_called_once()
+    api_delete_schedule.assert_not_called()
+    response_serializer.assert_not_called()
+    api_disconnect.assert_called_once()
+    # assert the expected response
+    assert_that(response.status).is_equal_to(500)
+    assert_that(await response.json()).contains_entry(
+        {"error": "get_schedules failure"}
+    )
+
+
 @patch("aioswitcher.api.SwitcherApi.delete_schedule", side_effect=Exception("blabla"))
-async def test_errorneous_delete_schedule_delete_request(
-    api_delete_schedule, response_serializer, api_connect, api_disconnect, api_client
+@patch("aioswitcher.api.SwitcherApi.get_schedules")
+async def test_erroneous_delete_schedule_delete_request(
+    api_get_schedules,
+    api_delete_schedule,
+    response_serializer,
+    api_connect,
+    api_disconnect,
+    api_client,
 ):
+    # stub get_schedules to return a schedule with matching id
+    schedule_mock = Mock()
+    schedule_mock.schedule_id = "5"
+    schedules_response = Mock()
+    schedules_response.schedules = [schedule_mock]
+    api_get_schedules.return_value = schedules_response
     # send delete request for delete_schedule endpoint
     response = await api_client.delete(
         delete_schedule_uri, json={webapp.KEY_SCHEDULE: "5"}
     )
     # verify mocks calling
     api_connect.assert_called_once()
+    api_get_schedules.assert_called_once()
     api_delete_schedule.assert_called_once_with("5")
     response_serializer.assert_not_called()
     api_disconnect.assert_called_once()
@@ -708,7 +799,7 @@ async def test_create_schedule_with_faulty_missing_key_post_request(
 
 
 @patch("aioswitcher.api.SwitcherApi.create_schedule", side_effect=Exception("blabla"))
-async def test_errorneous_create_schedule(
+async def test_erroneous_create_schedule(
     api_create_schedule, response_serializer, api_connect, api_disconnect, api_client
 ):
     json_body = {webapp.KEY_START: "11:00", webapp.KEY_STOP: "11:15"}

app/webapp.py

@@ -260,6 +260,13 @@ async def delete_schedule(request: web.Request) -> web.Response:
     async with SwitcherApi(
         device_type, request.query[KEY_IP], request.query[KEY_ID], login_key
     ) as swapi:
+        schedules_response = await swapi.get_schedules()
+        existing_ids = [s.schedule_id for s in schedules_response.schedules]
+        if schedule_id not in existing_ids:
+            return web.json_response(
+                {"error": f"schedule {schedule_id} does not exist"},
+                status=404,
+            )
         return web.json_response(
             _serialize_object(await swapi.delete_schedule(schedule_id))
         )