python-zulip-api icon indicating copy to clipboard operation
python-zulip-api copied to clipboard

`event` objects for normal bots and outgoing webhook bots not identical.

Open roberthoenig opened this issue 6 years ago • 2 comments

Relevant discussion on Zulip

Issue

The Zulip server sends different event objects for "normal" bots and outgoign webhook bots. This could create issues when developers except their bot to work the same withthe Botserver and zulip-run-bot. Particularly important are the differences in event['message'], since this is what gets passed to a bot's code. If a bot depends on any property that is not shared by normal and outgoing webhook bots, it will break for either the Botserver or zulip-run-bot.

Solutions

Two possible solutions could be to either make sure that all properties in the event['message'] object are identical for normal and outgoing webhook bots, or only expose matching subsets of the event['message'] object properties through zulip-run-bot and the botserver.

roberthoenig avatar May 23 '18 12:05 roberthoenig

For reference, here are two sample messages handled by a helloworld bot; one time through an outgoing webhook bot, one time through a normal bot:

  • Outgoing webhook bot (internal Zulip-server event object):
{
    "command": "@**Outgoing Webhook** asdf",
    "message": {
        "client": "website",
        "content": "@**Outgoing Webhook** asdf",
        "display_recipient": "Verona",
        "id": 342,
        "is_me_message": false,
        "raw_display_recipient": "Verona",
        "reactions": [],
        "recipient_id": 58,
        "recipient_type": 2,
        "recipient_type_id": 19,
        "rendered_content": "<p><span class=\"user-mention\" data-user-id=\"40\">@Outgoing Webhook</span> asdf</p>",
        "sender_avatar_source": "G",
        "sender_avatar_version": 1,
        "sender_email": "[email protected]",
        "sender_full_name": "aaron",
        "sender_id": 23,
        "sender_is_mirror_dummy": false,
        "sender_realm_id": 2,
        "sender_realm_str": "zulip",
        "sender_short_name": "aaron",
        "stream_id": 19,
        "subject": "Verona2",
        "subject_links": [],
        "submessages": [],
        "timestamp": 1527097010,
        "type": "stream"
    },
    "service_name": "test",
    "trigger": "mention",
    "user_profile_id": 40
}
  • Normal bot:
{
    "flags": [
        "mentioned"
    ],
    "id": 0,
    "message": {
        "avatar_url": "https://secure.gravatar.com/avatar/818c212b9f8830dfef491b3f7da99a14?d=identicon&version=1",
        "client": "website",
        "content": "@**converter** asdf",
        "content_type": "text/x-markdown",
        "display_recipient": "Verona",
        "id": 346,
        "is_me_message": false,
        "reactions": [],
        "recipient_id": 58,
        "sender_email": "[email protected]",
        "sender_full_name": "aaron",
        "sender_id": 23,
        "sender_realm_str": "zulip",
        "sender_short_name": "aaron",
        "stream_id": 19,
        "subject": "Verona2",
        "subject_links": [],
        "submessages": [],
        "timestamp": 1527097168,
        "type": "stream"
    },
    "stream_push_notify": false,
    "type": "message"
}
  • Diff (with irrelevant details like message IDs suppressed):
<     "command": "@**Outgoing Webhook** asdf",
---
>     "flags": [
>         "mentioned"
>     ],
>     "id": 0,
3a7
>         "avatar_url": "https://secure.gravatar.com/avatar/818c212b9f8830dfef491b3f7da99a14?d=identicon&version=1",
5c9,10
<         "content": "@**Outgoing Webhook** asdf",
---
>         "content": "@**converter** asdf",
>         "content_type": "text/x-markdown",

9d13
<         "raw_display_recipient": "Verona",
12,16d15
<         "recipient_type": 2,
<         "recipient_type_id": 19,
<         "rendered_content": "<p><span class=\"user-mention\" data-user-id=\"40\">@Outgoing Webhook</span> asdf</p>",
<         "sender_avatar_source": "G",
<         "sender_avatar_version": 1,
20,21d18
<         "sender_is_mirror_dummy": false,
<         "sender_realm_id": 2,

31,33c28,29
<     "service_name": "test",
<     "trigger": "mention",
<     "user_profile_id": 40
---
>     "stream_push_notify": false,
>     "type": "message"

roberthoenig avatar May 23 '18 17:05 roberthoenig

I just edited the diff to suppress the message IDs and timestamps, since those aren't interesting.

I agree we should have the data passed into the bot framework to have identical structure. Here's my thoughts on how to do that:

  • We should remove service_name -- the whole Service concept shouldn't be user-facing.
  • The bots library for the zulip-run-bot runner should compute trigger from mentioned and similar flags
  • The bots library should be able to get user_profile_id and sender_realm_id (from its register content), and splice that into the events it passes into the bot handler.
  • We should probably remove sender_is_mirror_dummy, sender_avatar_*, and the recipient_type fields from outgoing webhooks., and replace those with the more useful avatar_url.
  • content isn't a real difference, right, it's just a different bot?

These fields arguably don't need to be there at all, but certainly should be suppressed somewhere inside the bots subsystem (i.e. dropped by the bots tool before passing to the bot), since they're unlikely to be useful to the bots. But we want them in the events API still, so it'd be a change in the python-zulip-api project:

  • recipient_type and recipient_type_id
  • sender_avatar_source and sender_avatar_version

That doesn't cover everything, but it's enough that we can revisit after we're done.

timabbott avatar May 23 '18 18:05 timabbott