[WIP] bots: Allow users to configure incoming webhook bots #12474
Conversation
I'll be adding these soon (most likely tomorrow). |
zerver/lib/actions.py
Outdated
| # the bot's configuration data. | ||
| if bot_profile_id in incoming_webhook_bot_configs.keys(): | ||
| bot_config = incoming_webhook_bot_configs[bot_profile_id] | ||
| service_dicts = [{'config_data': bot_config}] |
There was a problem hiding this comment.
Hmm, could we avoid duplicating the embedded_bot_ids code path and instead do elif bot_type in [UserProfile.INCOMING_WEBHOOK_BOT, Profile.EMBEDDED_BO] both here and above? It seems like the only difference here is the service_name block. And we could save an unnecessary database query.
There was a problem hiding this comment.
I modified my code to remove the extra database query (i.e. the extra call to get_bot_configs). I also deduplicated the code.
|
Just to make sure I understand here, is the model that we're allowing custom configuration fields? I think structurally, we actually want something very similar to the embedded bot model, where the user picks an integration (e.g. "GitHub") and then we offer the BotConfigData form fields appropriate for that integration. We want to move the UI model to the user picking an integration for incoming webhooks (e.g. "GitHub") anyway, because that lets us prepopulate the integration logo. But since that's sorta a prerequisite to being able to do something good with bot config data here, maybe we should pause this PR once the first commit is merged, and work on making it possible to do that before picking this up again? I think we could reasonably structure it like this without too much work:
Does that plan make sense? |
Hypro999
changed the title
|
Yup, it makes sense, and it sounds like a good plan. My only concern is that this is a pre-requisite for the Phabricator integration (issue #9221) since we need to store the API key there. So that would be blocked until this can be done.
@timabbott what would you like to see cleaned up here? |
|
It's OK to have another project be blocked while we implement this piece correctly; hopefully it won't take too long. For clean up, I just meant fixing the comments about duplicated code and unnecessary database queries I made above. |
Note: The test added in this commit is just to make sure that we can patch the configuration data for an incoming webhook type bot. This test tests for behaviour that already exists and should continue to exist.
Now, when users go to settings > your bots > and click on the pencil (edit) icon for an incoming webhook type bot, the edit form will also contain a table of the bot's configuration data in a key-value pair format, the user will then be able to add new keys, or remove or edit existing ones.
Just finished this. Are there any other changes you'd like to see in the first commit? |
Going forwards, Zulip will need to let it's users configure their bots' BotConfigData (for example see the discussion under issue #10907). For embedded bots and outgoing webhook bots, the keys that can be configured are predetermined and users can already modify them from settings > your bots > active bots. Now for Incoming webhook type bots there are no predetermined keys as we do not strongly associate a bot with an integration (e.g. multiple integrations might use the same incoming webhook bot - e.g. A GitHub + GitLab notification bot) - so letting users set these keys-value pairs freely seems like a flexible approach.
This PR introduces 2 commits:
Changes the backend to send config data for incoming webhook type bots in the initial state as well as for update events. Patching the config data is already possible for all kinds of bots and this did not require any changes (it was just not used up until now).
[WIP] Modify the UI to allow users to modify their bot config data - add new keys or edit existing ones. Pending work for this commit:
@timabbott and @eeshangarg I'd appreciate a code review as well as any general opinions you guys have about this.
Getting this completed and merged would make it possible to begin work on developing a framework for how to allow our incoming webhook type bots to contact an API (as a callback), as @eeshangarg suggested.