zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
350 stars 352 forks source link

bot_server: Support trigger value `private_message` renamed to `direct_message`. #791

Closed prakhar1144 closed 9 months ago

prakhar1144 commented 9 months ago

CZO Discussion

The JSON payload that Zulip server POST for outgoing webhooks has trigger as one of the fields.

In https://github.com/zulip/zulip/commit/c4e4737, we renamed the private_message value to direct_message.

This commit adds support to the botserver for handling direct_message as a trigger value. It still supports private_message for self-hosted server compatibility.

prakhar1144 commented 9 months ago

I didn't rename private_message to direct_message in these places:

zulip/integrations/jabber/jabber_mirror_backend.py:                    self.private_message(message)
zulip/integrations/jabber/jabber_mirror_backend.py:    def private_message(self, msg: Dict[str, Any]) -> None:
zulip_bots/zulip_bots/bots/game_handler_bot/test_game_handler_bot.py:    def test_private_message_error(self) -> None:
zulip_bots/zulip_bots/lib.py:def is_private_message_but_not_group_pm(
zulip_bots/zulip_bots/lib.py:        is_private_message = is_private_message_but_not_group_pm(message, restricted_client)
zulip_bots/zulip_bots/lib.py:        if is_private_message or is_mentioned:
zulip_bots/zulip_bots/tests/test_lib.py:    is_private_message_but_not_group_pm,
zulip_bots/zulip_bots/tests/test_lib.py:    def test_is_private_message_but_not_group_pm(self):
zulip_bots/zulip_bots/tests/test_lib.py:        self.assertFalse(is_private_message_but_not_group_pm(message, handler))
zulip_bots/zulip_bots/tests/test_lib.py:        self.assertFalse(is_private_message_but_not_group_pm(message, handler))
zulip_bots/zulip_bots/tests/test_lib.py:        self.assertTrue(is_private_message_but_not_group_pm(message, handler))
zulip_bots/zulip_bots/tests/test_lib.py:        self.assertFalse(is_private_message_but_not_group_pm(message, handler))

as they are related to message["type"] == "private". We should rename it while changing the type from private to direct.

This PR is restricted to handling the outgoing webhook issue we encountered.

mateuszmandera commented 9 months ago

Looks good to me

timabbott commented 9 months ago

The compatibility comment was a great idea, but I changed it to state clearly the condition when we remove it -- namely when we no longer support pre-8.0 Zulip servers, since 8.0 will be the first Zulip server release with this change.

I think we can rename the is_private_message_but_not_group_pm to is_non_group_direct_message (and make its implementation check for both direct and private as types -- doing that now will be helpful down the line. API design discussion: https://chat.zulip.org/#narrow/stream/378-api-design/topic/.22private.22.20.3D.3E.20.22direct.22.20in.20message.20type/near/1644682.