8000 Fix send telegram notification to multiple chats by saippuakauppias · Pull Request #144922 · home-assistant/core · GitHub
[go: up one dir, main page]
Skip to content

Fix send telegram notification to multiple chats #144922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
+435 −6
Draft

Fix send telegram notification to multiple chats #144922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3179dff
Fix send messages in new config flow
saippuakauppias Jun 4, 2025
8038bc8
Fix ruff linter issues
saippuakauppias Jun 5, 2025
0d2b76d
Merge branch 'dev' into fix_138413
saippuakauppias Jun 5, 2025
4b3ecc9
Merge branch 'dev' into fix_138413
saippuakauppias Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions homeassistant/components/telegram_bot/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
ATTR_VERIFY_SSL,
CONF_ALLOWED_CHAT_IDS,
CONF_BOT_COUNT,
CONF_CHAT_ID,
CONF_CONFIG_ENTRY_ID,
CONF_PROXY_PARAMS,
CONF_PROXY_URL,
Expand Down Expand Up @@ -320,15 +321,49 @@ async def async_send_telegram_message(service: ServiceCall) -> ServiceResponse:
service.hass.config_entries.async_entries(DOMAIN)
)

if len(config_entries) == 1:
config_entry = config_entries[0]

if len(config_entries) > 1:
if len(config_entries) == 0:
raise ServiceValidationError(
"Multiple config entries found. Please specify the Telegram bot to use.",
"No Telegram Bot config entries found. Please set up a Telegram Bot first.",
translation_domain=DOMAIN,
translation_key="multiple_config_entry",
translation_key="no_config_entries",
)
if len(config_entries) == 1:
config_entry = config_entries[0]
else:
# Multiple config entries: try to find matching bot
target = kwargs.get(ATTR_TARGET)
if target is not None:
# Convert single target to list for consistent handling
if isinstance(target, int):
target = [target]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always convert to a set instead of a list.


# Find config entries that contain any of the target chat_ids
matching_entries = []
for entry in config_entries:
if hasattr(entry, "runtime_data") and entry.runtime_data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to check that the bot is in a valid state then use if entry.state == ConfigEntryState.LOADED:.

entry_chat_ids = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here as well, we should use a set. Member checking between sets is much faster than between lists.

subentry.data[CONF_CHAT_ID]
for subentry in entry.subentries.values()
]
if any(chat_id in entry_chat_ids for chat_id in target):
matching_entries.append(entry)

if len(matching_entries) >= 1:
# Use the first matching entry
config_entry = matching_entries[0]
else:
raise ServiceValidationError(
"No config entry found for the specified target chat IDs.",
translation_domain=DOMAIN,
translation_key="no_matching_target",
)
Comment on lines +333 to +359
Copy link
Contributor
@hanwg hanwg Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check is needed.
Using subentries from a different config entry feels odd.
Using the 1st entry as default does fix the particular scenario mentioned in #138413 but there are other cases which will still result in the message being sent to the wrong recipient (since this change always use the 1st matching entry).

e.g.
I create a new chat group in Telegram but I didn't create the corresponding config subentry in HA.
I want to send a message to the group.
The message will be sent to the 1st entry instead.

Instead, remove _get_target_chat_ids(...) from TelegramNotificationService; the validation is not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if there are more than one matching config entries found, we should raise ServiceValidationError as before.

Copy link
Contributor
@hanwg hanwg Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple bots should be able to send messages to the same user.
Hence, it's a valid scenario to have multiple matching entries.
We shouldn't need this check either.

Copy link
Contributor
@hanwg hanwg Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking on the possible options:

  1. Remove the validation check from L333-359 and let the action fail with HomeAssistantError Fix AttributeError in telegram_bot when API calls fail #145003.
  2. ServiceValidationError if any of the targets does not exists in the config subentries.
  3. remove _get_target_chat_ids(...) from TelegramNotificationService so that it does not default to the first chat ID

Are we all in agreement item 3 needs to be done?

Any decision on whether to go with item 1 or 2?
I would prefer item 1 since I rather have some users receive messages than everyone not getting anything. (The action is invoked once for each target and each action can fail independently)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple bots should be able to send messages to the same user. Hence, it's a valid scenario to have multiple matching entries. We shouldn't need this check either.

I guess you mean that we should send the message to all matching bots? This is about sending a message from Home Assistant to bots and not about receiving messages from bots, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following what should be done for 1. We want to differentiate between user errors and API errors. User errors, ie invalid service action input, should raise ServiceValidationError.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we do 3. without a breaking change? Target (chat ID) isn't a required parameter.

My mistake, I forgot about the case where the target is not specified.

Nevertheless, the logic in _get_target_chat_ids(...) is problematic since it defaults to the first subentry.
If I want to send a message to User X but that user's chat ID is not in the subentry, then the integration should raise a ServiceValidationError instead of sending the message to a another user just because that user was the first chat ID that was added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple bots should be able to send messages to the same user. Hence, it's a valid scenario to have multiple matching entries. We shouldn't need this check either.

I guess you mean that we should send the message to all matching bots? This is about sending a message from Home Assistant to bots and not about receiving messages from bots, right?

Please disregard my comment.
I think I'm having brain fog today.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't fully understand: which of the proposed fixes is better? I don't have such a deep knowledge of HA

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have is fine.
Do address the recommendations by Martin.

Also, there's mention about multiple bots with same chat IDs.
e.g.

  • Bot A with chat ID sarah
  • Bot B with chat ID sarah

Your PR selects Bot A by default but the expected result should be a ServiceValidationError because we shouldn't make assumption that Bot A is the desired bot for sending the message.

Thus, I think you can refine it like this:

if len(matching_entries) > 1:
    # multiple matching entries, raise an error instead of making assumptions on which bot to use
    raise ServiceValidationError(...)
elif len(matching_entries) == 1:
    # only 1 matching entry so we can safely select the default bot
    config_entry = matching_entries[0]
else:
    raise ServiceValidationError(
        "No config entry found for the specified target chat IDs.",
        translation_domain=DOMAIN,
        translation_key="no_matching_target",
    )

The other issue is related to the default selection of the recipient (chat ID) handled in _get_target_chat_ids. Maybe we can leave this aside first to avoid confusion.

else:
# No specific target: error out
raise ServiceValidationError(
"Multiple config entries found. Please specify the Telegram bot to use.",
translation_domain=DOMAIN,
translation_key="multiple_config_entry",
)

if not config_entry or not hasattr(config_entry, "runtime_data"):
raise ServiceValidationError(
Expand Down
6 changes: 6 additions & 0 deletions homeassistant/components/telegram_bot/strings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,12 @@
"missing_config_entry": {
"message": "No config entries found or setup failed. Please set up the Telegram Bot first."
},
"no_config_entries": {
"message": "No Telegram Bot config entries found. Please set up a Telegram Bot first."
},
"no_matching_target": {
"message": "No config entry found for the specified target chat IDs."
},
"missing_allowed_chat_ids": {
"message": "No allowed chat IDs found. Please add allowed chat IDs for {bot_name}."
}
Expand Down
Loading
0