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.

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

Already on GitHub? Sign in to your account

Add Discourse integration #6201

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@davidtaylorhq
Contributor

davidtaylorhq commented Aug 21, 2017

This PR adds an integration for Discourse. It links to the discourse-chat-integration plugin.

Details of its use can be found here: https://meta.discourse.org/t/set-up-zulip-notifications-using-the-discourse-chat-integration-plugin/68501

I have followed the "Webhook Walkthrough" from the documentation to create the integration - I hope it is all acceptable. Please do let me know if anything needs to be adjusted 馃檪 .

@roberthoenig

@davidtaylorhq thank you so much for your work here, great job! For the example chat, maybe we should use a bot that displays the discourse logo. @timabbott apart from a couple little suggestions, this looks ready to merge to me!

@@ -240,6 +240,7 @@ def __init__(self, name, *args, **kwargs):
display_name='Desk.com',
stream_name='desk'
),
WebhookIntegration('discourse', ['communication'], display_name="Discourse"),

This comment has been minimized.

@roberthoenig

roberthoenig Aug 22, 2017

Contributor

nit: single quotes for "Discourse" :)

{!congrats.md!}
![](/static/images/integrations/discourse/001.png)

This comment has been minimized.

@roberthoenig

roberthoenig Aug 22, 2017

Contributor

We probably want to use the {!create-stream.md!} macro in the beginning here. The rest is a partial duplicate of create-bot-construct-url.md, but I see why we cannot use that macro here - it contains unfitting instructions.

For the future, maybe we should consider splitting that macro?

This comment has been minimized.

@timabbott

timabbott Aug 22, 2017

Member

@derAnfaenger it's worth thinking about refactoring that macro for this use case, yeah.

This comment has been minimized.

@davidtaylorhq

davidtaylorhq Aug 22, 2017

Contributor

I didn't include the create-stream macro, as there there isn't a requirement to do so. The integration is designed so it can post into any existing stream.

Re. the macros - I started with the create-stream macro but found some stuff wasn't particularly relevent. Is there anything I need to do for the purpose of this PR, or is it ok to leave as-is?

@timabbott

This comment has been minimized.

Member

timabbott commented Aug 22, 2017

@davidtaylorhq this looks pretty good! I think if you fix the little style/text duplication nits, we can merge this.

That said, given that the actual formatting is being done on the Discourse side, I wonder if we should instead be defining a "generic" Zulip incoming webhook endpoint that can be used by Discourse/IFTTT/etc. (aka the services that have code for constructing the message formatting on the third party service's side). I see no reason to block merging this on that idea, but @derAnfaenger @eeshangarg @showell what do you think about that idea?

Even if we go that route, I'd still want to have documentation on the Zulip side for the individual services we integrate with, but it would allow many integrations of this form to be added without any new code on our end.

@davidtaylorhq

This comment has been minimized.

Contributor

davidtaylorhq commented Aug 22, 2017

Thanks for the comments - will resolve them tomorrow.

I 100% agree with the idea of having a "generic" webhook destination for future use. I tried using the /api/v1/messages endpoint, but it would seem that it doesn't work with an "incoming webhook" bot api_key.

@timabbott

This comment has been minimized.

Member

timabbott commented Aug 22, 2017

@davidtaylorhq ahh, yes, I think we should actually just changing that endpoint to allow access from incoming webhook bots, which I've just done in . This was on my list of things to check before our first release containing the limited incoming webhook bots in the first place, so thanks for bringing this up :).

Do you think it'd be useful to have something more specialized for "incoming webhooks", or does that solve the problem?

@davidtaylorhq

This comment has been minimized.

Contributor

davidtaylorhq commented Aug 22, 2017

I don't think anything more specific is required for my use case - as long as it can take "stream", "subject" and "message" parameters, and authenticate with the api_key then that works great.

If this change is live in master, shall I just use that instead? Ideally then we'd have the concept of a "documentation only" integration so that discourse can be listed alongside all the others.

@timabbott

This comment has been minimized.

Member

timabbott commented Aug 22, 2017

Yeah, it's live in master; why don't you try it out and see how the API feels on your end. I think we already support "documentation-only" integrations in the form of the stuff under templates/zerver/integrations/ (those mostly all live the python-zulip-api repository).

@davidtaylorhq davidtaylorhq force-pushed the davidtaylorhq:discourse-integration branch from 96e76ae to 9d75bcc Aug 23, 2017

@davidtaylorhq

This comment has been minimized.

Contributor

davidtaylorhq commented Aug 23, 2017

@timabbott I have now changed it to use the /api/v1/messages API - works great for what I need. The only difference I noticed is that this API requires the bot's email address in addition to the api key, whereas the other webhook integrations only require the key. Not an issue at all, just an observation 馃槃

I have now updated this PR to remove the webhook integration, and add a 'documentation only' integration. I've updated the screenshot to include the discourse logo as the bot's avatar as-per earlier comments.

Head over to the
[Discourse Chat Integration Setup Instructions](https://meta.discourse.org/t/68501)
and complete them.

This comment has been minimized.

@timabbott

timabbott Aug 23, 2017

Member

Is there a particular version of Discourse required?

This comment has been minimized.

@davidtaylorhq

davidtaylorhq Aug 23, 2017

Contributor

No specific version of Discourse - you just need to install the discourse-chat-integration plugin. The details of that are included in the page I've linked to.

@timabbott

This comment has been minimized.

Member

timabbott commented Aug 23, 2017

Looks great; just posted one comment.

@timabbott

This comment has been minimized.

Member

timabbott commented Aug 23, 2017

Merged, thanks for the contribution @davidtaylorhq!

@davidtaylorhq

This comment has been minimized.

Contributor

davidtaylorhq commented Aug 23, 2017

Great, thanks for the help working everything out 馃槂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment