outgoing webhooks: Warn user that PMs are not supported in Slack-format webhook. #9814
Conversation
|
Hello @zulip/server-bots members, this pull request was labeled with the "area: bots" label, so you may want to check it out! |
rheaparekh
force-pushed
the
slack_webhook
branch
2 times, most recently
from
8bab95d
to
88d651e
Compare
|
@timabbott This is ready for a review. |
There was a problem hiding this comment.
This looks broadly fine to me.
I would question the text to use in the response to the user; on one hand I see it useful to be simple at simply stating the feature is not supported, but it also may be nice to have a little detail as to the context (maybe in parentheses after?) to show the source of the limitation?
zerver/worker/queue_processors.py
Outdated
| @@ -479,7 +479,8 @@ def consume(self, event: Mapping[str, Any]) -> None: | |||
| dup_event['service_name'] = str(service.name) | |||
| service_handler = get_outgoing_webhook_service_handler(service) | |||
| rest_operation, request_data = service_handler.process_event(dup_event) | |||
| do_rest_call(rest_operation, request_data, dup_event, service_handler) | |||
| if request_data: | |||
There was a problem hiding this comment.
Could you explain the addition of this? do_rest_call previously is specified as accepting an Optional request_data parameter, so is this warranted?
There was a problem hiding this comment.
I am returning a None here, as I don't want the function do_rest_call to proceed ahead for private messages.
There was a problem hiding this comment.
I understand the correlation to private slack messages, but this code applies generally to outgoing webhooks, not just slack?
| @@ -54,7 +54,7 @@ def test_process_success(self) -> None: | |||
| class TestSlackOutgoingWebhookService(ZulipTestCase): | |||
|
|
|||
| def setUp(self) -> None: | |||
| self.event = { | |||
| self.stream_message_event = { | |||
There was a problem hiding this comment.
This commit just covers a rename for the sake of clarity, correct? If so, consider simply saying so in the commit title, eg. "Rename [blah] for clarity."
|
This is ready for a review. |
rheaparekh
force-pushed
the
slack_webhook
branch
2 times, most recently
from
9d6802b
to
a280ca3
Compare
…at webhook. Private messages are not supported in Slack-format webhook. Instead of raising a NotImplementedError, we warn the user that PM service is not supported by sending a message to the user. Added tests for the same. Fixes zulip#9239
|
Rebased and merged, after fixing the super confusing error message "Private messaging service not supported". Thanks @rheaparekh! |
Private messages are not supported in Slack-format webhook. Instead of raising a
NotImplementedError, we warn the user that PM service is not supported by sending a message to the user.Added tests for the same.
Fixes #9239
Screenshot:
