Add notify_target parameter for Telegram bot actions #159745
Conversation
luca-angemi
left a comment
luca-angemi
left a comment
There was a problem hiding this comment.
Tested as a custom component with latest nightly. All actions are working properly.
|
Some comments are not resolved above. |
|
I think I've resolved all comments but if I missed any, do point them out to me. |
This comment was marked as spam.
This comment was marked as spam.
| async def _call_service( | ||
| service: ServiceCall, notify_service: TelegramNotificationService, chat_id: int | ||
| ) -> dict[str, JsonValueType] | None: | ||
| msgtype = service.service |
There was a problem hiding this comment.
| msgtype = service.service | |
| service_name = service.service |
| def _build_targets( | ||
| hass: HomeAssistant, service: ServiceCall | ||
| ) -> list[tuple[TelegramBotConfigEntry, int, str]]: | ||
| """Build list of targets where each target is represented by its corresponding config entry, chat ID and notify entity id.""" |
There was a problem hiding this comment.
Please limit docstring lines to 72 characters. The first line should be a header and a single sentence. After that an optional body can follow separated from the header by a blank line.
| async def _call_service( | ||
| service: ServiceCall, notify_service: TelegramNotificationService, chat_id: int | ||
| ) -> dict[str, JsonValueType] | None: | ||
| msgtype = service.service |
| chat_ids = migrate_chat_ids | ||
| if ATTR_CHAT_ID in service.data: | ||
| chat_ids = chat_ids | ( | ||
| {int(service.data[ATTR_CHAT_ID])} |
There was a problem hiding this comment.
We don't need to copy to integer if the chat id is already an integer. Isn't the chat id parameter in the service call data either an integer or a list of integers?
There was a problem hiding this comment.
Yes, chat_id is either int or list[int].
service data has the dict[str, Any] type and I was trying to erase the Any type to avoid int | Any.
done in d4f2d7c
| service.hass.config_entries.async_entries(DOMAIN) | ||
| ) | ||
| if len(config_entries) == 1: | ||
| config_entry = config_entries[0] |
There was a problem hiding this comment.
Calculating a default config entry here or specifying one in the service call data only makes sense when not specifying entity_id. An entity is connected to a single config entry, so it's not possible to change what config entry (bot) to use for an entity target.
There was a problem hiding this comment.
I think we need to make the config entry id parameter and the entity_id parameter in the service calls exclusive. It's confusing if we allow both at the same time.
There was a problem hiding this comment.
Calculating the default config entry is necessary to avoid breaking changes with existing automations.
For example, the following automation does not have config_entry_id or chat_id specified and the existing behavior is to automatically select a default bot and chat_id.
action: telegram_bot.send_message
data:
message: test
There was a problem hiding this comment.
I only have a small handful of users that I interact with regularly and the feedback from them was that they preferred to have both entity_id and config_entry_id + chat_id.
The way that they understand it is that config_entry_id + chat_id are additional targets on top of what is specified in entity_id.
They aren't very inclined to the idea of mutually exclusiveness of entity_id and config_entry_id + chat_id since they might need to duplicate their automations.
Example automations below for illustration.
# not mutually exclusive - single action to specify both notify entities and chat ids
actions:
- action: telegram_bot.send_message
data:
message: test
entity_id:
- notify.bot_user
config_entry_id: xxx
chat_id:
- 1111111111
# mutually exclusive - need to duplicate actions
actions:
- action: telegram_bot.send_message
data:
message: test
entity_id:
- notify.bot_user
- action: telegram_bot.send_message
data:
message: test
config_entry_id: xxx
chat_id:
- 1111111111
To balance simplicity with flexibility, that's why I’m supporting both entity_id and config_entry_id + chat_id. By moving the latter to an 'Advanced' section, the basic configuration remains accessible (easy) for most users.
4 participants
Proposed change
Currently,
chat_idis used as targets for telegram bot actions.chat_idis not intuitive since it is a numeric identifier.With the introduction of notify entities in #149853, we can instead use these entities as targets.
This change introduces a new
notify_targetparameter which uses the entity selector to allow the user to select telegram bot notify entities for actions.Example actions for use in automations/scripts:
Detailed changes:
targetaction parameter is being deprecated:targetparameter is used for specifying chat IDschat_idparameter. During this migration period, chat IDs found intargetare automatically copied to the newchat_idfield. A repair issue will also created to inform the user to update their automations/scripts.entity_idtoServiceResponse. This new output value can be used in complex automations e.g. outputentity_idofsend_messagecan be used as input foredit_message.config_entry_idandchat_idfields. This helps to avoid confusion for new users by drawing the user's attention to the newentity_idfield for specifying notify entities as targets. Custom targets are meant for advance usage where the user has to specify the numeric chat ids.entity_id,config_entry_idandtargetfields. The service is then invoked once for each of the targets. (See_build_targets()in__init__.py)Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: