From d97a030872abfdbfc544a0d5fb5315699a52f38a Mon Sep 17 00:00:00 2001 From: Robert Resch Date: Mon, 30 Oct 2023 21:43:24 +0100 Subject: [PATCH] Refactor todo services and their schema (#103079) --- homeassistant/components/todo/__init__.py | 86 +++++------ homeassistant/components/todo/services.yaml | 27 ++-- homeassistant/components/todo/strings.json | 46 +++--- tests/components/todo/test_init.py | 152 +++++++++++--------- 4 files changed, 149 insertions(+), 162 deletions(-) diff --git a/homeassistant/components/todo/__init__.py b/homeassistant/components/todo/__init__.py index 12eac858f75..968256ce3d9 100644 --- a/homeassistant/components/todo/__init__.py +++ b/homeassistant/components/todo/__init__.py @@ -43,14 +43,11 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: websocket_api.async_register_command(hass, websocket_handle_todo_item_move) component.async_register_entity_service( - "create_item", + "add_item", { - vol.Required("summary"): vol.All(cv.string, vol.Length(min=1)), - vol.Optional("status", default=TodoItemStatus.NEEDS_ACTION): vol.In( - {TodoItemStatus.NEEDS_ACTION, TodoItemStatus.COMPLETED} - ), + vol.Required("item"): vol.All(cv.string, vol.Length(min=1)), }, - _async_create_todo_item, + _async_add_todo_item, required_features=[TodoListEntityFeature.CREATE_TODO_ITEM], ) component.async_register_entity_service( @@ -58,30 +55,26 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: vol.All( cv.make_entity_service_schema( { - vol.Optional("uid"): cv.string, - vol.Optional("summary"): vol.All(cv.string, vol.Length(min=1)), + vol.Required("item"): vol.All(cv.string, vol.Length(min=1)), + vol.Optional("rename"): vol.All(cv.string, vol.Length(min=1)), vol.Optional("status"): vol.In( {TodoItemStatus.NEEDS_ACTION, TodoItemStatus.COMPLETED} ), } ), - cv.has_at_least_one_key("uid", "summary"), + cv.has_at_least_one_key("rename", "status"), ), _async_update_todo_item, required_features=[TodoListEntityFeature.UPDATE_TODO_ITEM], ) component.async_register_entity_service( - "delete_item", - vol.All( - cv.make_entity_service_schema( - { - vol.Optional("uid"): vol.All(cv.ensure_list, [cv.string]), - vol.Optional("summary"): vol.All(cv.ensure_list, [cv.string]), - } - ), - cv.has_at_least_one_key("uid", "summary"), + "remove_item", + cv.make_entity_service_schema( + { + vol.Required("item"): vol.All(cv.ensure_list, [cv.string]), + } ), - _async_delete_todo_items, + _async_remove_todo_items, required_features=[TodoListEntityFeature.DELETE_TODO_ITEM], ) @@ -114,13 +107,6 @@ class TodoItem: status: TodoItemStatus | None = None """A status or confirmation of the To-do item.""" - @classmethod - def from_dict(cls, obj: dict[str, Any]) -> "TodoItem": - """Create a To-do Item from a dictionary parsed by schema validators.""" - return cls( - summary=obj.get("summary"), status=obj.get("status"), uid=obj.get("uid") - ) - class TodoListEntity(Entity): """An entity that represents a To-do list.""" @@ -232,39 +218,43 @@ async def websocket_handle_todo_item_move( connection.send_result(msg["id"]) -def _find_by_summary(summary: str, items: list[TodoItem] | None) -> TodoItem | None: - """Find a To-do List item by summary name.""" +def _find_by_uid_or_summary( + value: str, items: list[TodoItem] | None +) -> TodoItem | None: + """Find a To-do List item by uid or summary name.""" for item in items or (): - if item.summary == summary: + if value in (item.uid, item.summary): return item return None -async def _async_create_todo_item(entity: TodoListEntity, call: ServiceCall) -> None: +async def _async_add_todo_item(entity: TodoListEntity, call: ServiceCall) -> None: """Add an item to the To-do list.""" - await entity.async_create_todo_item(item=TodoItem.from_dict(call.data)) + await entity.async_create_todo_item( + item=TodoItem(summary=call.data["item"], status=TodoItemStatus.NEEDS_ACTION) + ) async def _async_update_todo_item(entity: TodoListEntity, call: ServiceCall) -> None: """Update an item in the To-do list.""" - item = TodoItem.from_dict(call.data) - if not item.uid: - found = _find_by_summary(call.data["summary"], entity.todo_items) - if not found: - raise ValueError(f"Unable to find To-do item with summary '{item.summary}'") - item.uid = found.uid + item = call.data["item"] + found = _find_by_uid_or_summary(item, entity.todo_items) + if not found: + raise ValueError(f"Unable to find To-do item '{item}'") - await entity.async_update_todo_item(item=item) + update_item = TodoItem( + uid=found.uid, summary=call.data.get("rename"), status=call.data.get("status") + ) + + await entity.async_update_todo_item(item=update_item) -async def _async_delete_todo_items(entity: TodoListEntity, call: ServiceCall) -> None: - """Delete an item in the To-do list.""" - uids = call.data.get("uid", []) - if not uids: - summaries = call.data.get("summary", []) - for summary in summaries: - item = _find_by_summary(summary, entity.todo_items) - if not item: - raise ValueError(f"Unable to find To-do item with summary '{summary}") - uids.append(item.uid) +async def _async_remove_todo_items(entity: TodoListEntity, call: ServiceCall) -> None: + """Remove an item in the To-do list.""" + uids = [] + for item in call.data.get("item", []): + found = _find_by_uid_or_summary(item, entity.todo_items) + if not found or not found.uid: + raise ValueError(f"Unable to find To-do item '{item}") + uids.append(found.uid) await entity.async_delete_todo_items(uids=uids) diff --git a/homeassistant/components/todo/services.yaml b/homeassistant/components/todo/services.yaml index c31a7e88808..4d6237760ca 100644 --- a/homeassistant/components/todo/services.yaml +++ b/homeassistant/components/todo/services.yaml @@ -1,23 +1,15 @@ -create_item: +add_item: target: entity: domain: todo supported_features: - todo.TodoListEntityFeature.CREATE_TODO_ITEM fields: - summary: + item: required: true example: "Submit income tax return" selector: text: - status: - example: "needs_action" - selector: - select: - translation_key: status - options: - - needs_action - - completed update_item: target: entity: @@ -25,11 +17,13 @@ update_item: supported_features: - todo.TodoListEntityFeature.UPDATE_TODO_ITEM fields: - uid: + item: + required: true + example: "Submit income tax return" selector: text: - summary: - example: "Submit income tax return" + rename: + example: "Something else" selector: text: status: @@ -40,16 +34,13 @@ update_item: options: - needs_action - completed -delete_item: +remove_item: target: entity: domain: todo supported_features: - todo.TodoListEntityFeature.DELETE_TODO_ITEM fields: - uid: - selector: - object: - summary: + item: selector: object: diff --git a/homeassistant/components/todo/strings.json b/homeassistant/components/todo/strings.json index 623c46375f0..6ba8aaba1a5 100644 --- a/homeassistant/components/todo/strings.json +++ b/homeassistant/components/todo/strings.json @@ -6,49 +6,41 @@ } }, "services": { - "create_item": { - "name": "Create to-do list item", + "add_item": { + "name": "Add to-do list item", "description": "Add a new to-do list item.", "fields": { - "summary": { - "name": "Summary", - "description": "The short summary that represents the to-do item." - }, - "status": { - "name": "Status", - "description": "A status or confirmation of the to-do item." + "item": { + "name": "Item name", + "description": "The name that represents the to-do item." } } }, "update_item": { "name": "Update to-do list item", - "description": "Update an existing to-do list item based on either its unique ID or summary.", + "description": "Update an existing to-do list item based on its name.", "fields": { - "uid": { - "name": "To-do item unique ID", - "description": "Unique identifier for the to-do list item." + "item": { + "name": "Item name", + "description": "The name for the to-do list item." }, - "summary": { - "name": "Summary", - "description": "The short summary that represents the to-do item." + "rename": { + "name": "Rename item", + "description": "The new name of the to-do item" }, "status": { - "name": "Status", + "name": "Set status", "description": "A status or confirmation of the to-do item." } } }, - "delete_item": { - "name": "Delete a to-do list item", - "description": "Delete an existing to-do list item either by its unique ID or summary.", + "remove_item": { + "name": "Remove a to-do list item", + "description": "Remove an existing to-do list item by its name.", "fields": { - "uid": { - "name": "To-do item unique IDs", - "description": "Unique identifiers for the to-do list items." - }, - "summary": { - "name": "Summary", - "description": "The short summary that represents the to-do item." + "item": { + "name": "Item name", + "description": "The name for the to-do list items." } } } diff --git a/tests/components/todo/test_init.py b/tests/components/todo/test_init.py index f4d671ad352..3e84049efa8 100644 --- a/tests/components/todo/test_init.py +++ b/tests/components/todo/test_init.py @@ -197,28 +197,18 @@ async def test_unsupported_websocket( assert resp.get("error", {}).get("code") == "not_found" -@pytest.mark.parametrize( - ("item_data", "expected_status"), - [ - ({}, TodoItemStatus.NEEDS_ACTION), - ({"status": "needs_action"}, TodoItemStatus.NEEDS_ACTION), - ({"status": "completed"}, TodoItemStatus.COMPLETED), - ], -) -async def test_create_item_service( +async def test_add_item_service( hass: HomeAssistant, - item_data: dict[str, Any], - expected_status: TodoItemStatus, test_entity: TodoListEntity, ) -> None: - """Test creating an item in a To-do list.""" + """Test adding an item in a To-do list.""" await create_mock_platform(hass, [test_entity]) await hass.services.async_call( DOMAIN, - "create_item", - {"summary": "New item", **item_data}, + "add_item", + {"item": "New item"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -229,14 +219,14 @@ async def test_create_item_service( assert item assert item.uid is None assert item.summary == "New item" - assert item.status == expected_status + assert item.status == TodoItemStatus.NEEDS_ACTION -async def test_create_item_service_raises( +async def test_add_item_service_raises( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test creating an item in a To-do list that raises an error.""" + """Test adding an item in a To-do list that raises an error.""" await create_mock_platform(hass, [test_entity]) @@ -244,8 +234,8 @@ async def test_create_item_service_raises( with pytest.raises(HomeAssistantError, match="Ooops"): await hass.services.async_call( DOMAIN, - "create_item", - {"summary": "New item", "status": "needs_action"}, + "add_item", + {"item": "New item"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -255,27 +245,23 @@ async def test_create_item_service_raises( ("item_data", "expected_error"), [ ({}, "required key not provided"), - ({"status": "needs_action"}, "required key not provided"), - ( - {"summary": "", "status": "needs_action"}, - "length of value must be at least 1", - ), + ({"item": ""}, "length of value must be at least 1"), ], ) -async def test_create_item_service_invalid_input( +async def test_add_item_service_invalid_input( hass: HomeAssistant, test_entity: TodoListEntity, item_data: dict[str, Any], expected_error: str, ) -> None: - """Test invalid input to the create item service.""" + """Test invalid input to the add item service.""" await create_mock_platform(hass, [test_entity]) with pytest.raises(vol.Invalid, match=expected_error): await hass.services.async_call( DOMAIN, - "create_item", + "add_item", item_data, target={"entity_id": "todo.entity1"}, blocking=True, @@ -293,7 +279,7 @@ async def test_update_todo_item_service_by_id( await hass.services.async_call( DOMAIN, "update_item", - {"uid": "item-1", "summary": "Updated item", "status": "completed"}, + {"item": "1", "rename": "Updated item", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -302,7 +288,7 @@ async def test_update_todo_item_service_by_id( assert args item = args.kwargs.get("item") assert item - assert item.uid == "item-1" + assert item.uid == "1" assert item.summary == "Updated item" assert item.status == TodoItemStatus.COMPLETED @@ -318,7 +304,7 @@ async def test_update_todo_item_service_by_id_status_only( await hass.services.async_call( DOMAIN, "update_item", - {"uid": "item-1", "status": "completed"}, + {"item": "1", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -327,12 +313,12 @@ async def test_update_todo_item_service_by_id_status_only( assert args item = args.kwargs.get("item") assert item - assert item.uid == "item-1" + assert item.uid == "1" assert item.summary is None assert item.status == TodoItemStatus.COMPLETED -async def test_update_todo_item_service_by_id_summary_only( +async def test_update_todo_item_service_by_id_rename( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: @@ -343,7 +329,7 @@ async def test_update_todo_item_service_by_id_summary_only( await hass.services.async_call( DOMAIN, "update_item", - {"uid": "item-1", "summary": "Updated item"}, + {"item": "1", "rename": "Updated item"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -352,7 +338,7 @@ async def test_update_todo_item_service_by_id_summary_only( assert args item = args.kwargs.get("item") assert item - assert item.uid == "item-1" + assert item.uid == "1" assert item.summary == "Updated item" assert item.status is None @@ -368,7 +354,7 @@ async def test_update_todo_item_service_raises( await hass.services.async_call( DOMAIN, "update_item", - {"uid": "item-1", "summary": "Updated item", "status": "completed"}, + {"item": "1", "rename": "Updated item", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -378,7 +364,7 @@ async def test_update_todo_item_service_raises( await hass.services.async_call( DOMAIN, "update_item", - {"uid": "item-1", "summary": "Updated item", "status": "completed"}, + {"item": "1", "rename": "Updated item", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -395,7 +381,7 @@ async def test_update_todo_item_service_by_summary( await hass.services.async_call( DOMAIN, "update_item", - {"summary": "Item #1", "status": "completed"}, + {"item": "Item #1", "rename": "Something else", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -405,10 +391,35 @@ async def test_update_todo_item_service_by_summary( item = args.kwargs.get("item") assert item assert item.uid == "1" - assert item.summary == "Item #1" + assert item.summary == "Something else" assert item.status == TodoItemStatus.COMPLETED +async def test_update_todo_item_service_by_summary_only_status( + hass: HomeAssistant, + test_entity: TodoListEntity, +) -> None: + """Test updating an item in a To-do list by summary.""" + + await create_mock_platform(hass, [test_entity]) + + await hass.services.async_call( + DOMAIN, + "update_item", + {"item": "Item #1", "rename": "Something else"}, + target={"entity_id": "todo.entity1"}, + blocking=True, + ) + + args = test_entity.async_update_todo_item.call_args + assert args + item = args.kwargs.get("item") + assert item + assert item.uid == "1" + assert item.summary == "Something else" + assert item.status is None + + async def test_update_todo_item_service_by_summary_not_found( hass: HomeAssistant, test_entity: TodoListEntity, @@ -421,7 +432,7 @@ async def test_update_todo_item_service_by_summary_not_found( await hass.services.async_call( DOMAIN, "update_item", - {"summary": "Item #7", "status": "completed"}, + {"item": "Item #7", "status": "completed"}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -430,10 +441,11 @@ async def test_update_todo_item_service_by_summary_not_found( @pytest.mark.parametrize( ("item_data", "expected_error"), [ - ({}, "must contain at least one of"), - ({"status": "needs_action"}, "must contain at least one of"), + ({}, r"required key not provided @ data\['item'\]"), + ({"status": "needs_action"}, r"required key not provided @ data\['item'\]"), + ({"item": "Item #1"}, "must contain at least one of"), ( - {"summary": "", "status": "needs_action"}, + {"item": "", "status": "needs_action"}, "length of value must be at least 1", ), ], @@ -458,32 +470,32 @@ async def test_update_item_service_invalid_input( ) -async def test_delete_todo_item_service_by_id( +async def test_remove_todo_item_service_by_id( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test deleting an item in a To-do list.""" + """Test removing an item in a To-do list.""" await create_mock_platform(hass, [test_entity]) await hass.services.async_call( DOMAIN, - "delete_item", - {"uid": ["item-1", "item-2"]}, + "remove_item", + {"item": ["1", "2"]}, target={"entity_id": "todo.entity1"}, blocking=True, ) args = test_entity.async_delete_todo_items.call_args assert args - assert args.kwargs.get("uids") == ["item-1", "item-2"] + assert args.kwargs.get("uids") == ["1", "2"] -async def test_delete_todo_item_service_raises( +async def test_remove_todo_item_service_raises( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test deleting an item in a To-do list that raises an error.""" + """Test removing an item in a To-do list that raises an error.""" await create_mock_platform(hass, [test_entity]) @@ -491,43 +503,45 @@ async def test_delete_todo_item_service_raises( with pytest.raises(HomeAssistantError, match="Ooops"): await hass.services.async_call( DOMAIN, - "delete_item", - {"uid": ["item-1", "item-2"]}, + "remove_item", + {"item": ["1", "2"]}, target={"entity_id": "todo.entity1"}, blocking=True, ) -async def test_delete_todo_item_service_invalid_input( +async def test_remove_todo_item_service_invalid_input( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test invalid input to the delete item service.""" + """Test invalid input to the remove item service.""" await create_mock_platform(hass, [test_entity]) - with pytest.raises(vol.Invalid, match="must contain at least one of"): + with pytest.raises( + vol.Invalid, match=r"required key not provided @ data\['item'\]" + ): await hass.services.async_call( DOMAIN, - "delete_item", + "remove_item", {}, target={"entity_id": "todo.entity1"}, blocking=True, ) -async def test_delete_todo_item_service_by_summary( +async def test_remove_todo_item_service_by_summary( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test deleting an item in a To-do list by summary.""" + """Test removing an item in a To-do list by summary.""" await create_mock_platform(hass, [test_entity]) await hass.services.async_call( DOMAIN, - "delete_item", - {"summary": ["Item #1"]}, + "remove_item", + {"item": ["Item #1"]}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -537,19 +551,19 @@ async def test_delete_todo_item_service_by_summary( assert args.kwargs.get("uids") == ["1"] -async def test_delete_todo_item_service_by_summary_not_found( +async def test_remove_todo_item_service_by_summary_not_found( hass: HomeAssistant, test_entity: TodoListEntity, ) -> None: - """Test deleting an item in a To-do list by summary which is not found.""" + """Test removing an item in a To-do list by summary which is not found.""" await create_mock_platform(hass, [test_entity]) with pytest.raises(ValueError, match="Unable to find"): await hass.services.async_call( DOMAIN, - "delete_item", - {"summary": ["Item #7"]}, + "remove_item", + {"item": ["Item #7"]}, target={"entity_id": "todo.entity1"}, blocking=True, ) @@ -656,22 +670,22 @@ async def test_move_todo_item_service_invalid_input( ("service_name", "payload"), [ ( - "create_item", + "add_item", { - "summary": "New item", + "item": "New item", }, ), ( - "delete_item", + "remove_item", { - "uid": ["1"], + "item": ["1"], }, ), ( "update_item", { - "uid": "1", - "summary": "Updated item", + "item": "1", + "rename": "Updated item", }, ), ],