Simplify what developers need to know to write new webhook integrations

[timabbott]

http://zulip.readthedocs.org/en/latest/integration-guide.html is great progress, but now that we're getting a lot of integration contributions, there's probably value in doing work on making more of a framework around the webhook integration writing process.

Some ideas include:

• Making the API for sending a message cleaner than the current check_send_message call
• Moving the decorator auth check out of the individual views files so auth is controlled via urls.py (similar to how rest_dispatch works)
• Factoring out a library of useful helper functions for things like parsing
• Potentially automatically generating the client names from the webhook function name (certainly api_travis_webhook -> ZulipTravisWebhook is basically an algorithmic transformation)
• Creating a webhook testing class that all the hooks can inherit from to cut down on code duplication between the webhook test classes

If folks want to work on this, probably each idea we implement should be its own pull request, so we can make rapid incremental progress rather than trying to batch everything up into a giant overhaul (which usually get stuck and suffer from a lot of merge conflicts).

If anyone's interested in working on this, ping this thread -- I'm happy to help figure out exactly what we should do.

(There are some related notes on ideas from @VallHer in #70)

[RafaelloLollipop]

To start discussion, I think we should create maybe template for writting integrations.

I realize that all requests are different and the solution should be very flexible, however if Zulip aims into bigger number of integrations - it should be standarized to make it more easy to maintain.

All integrations are doing at least:

• receiving request (random data)
• parsing request (standardized data)
• matching data with content templates (usage of data)
• sending message to topic or private

I suggest to start with adding some classes which will help in parsing and matching data.
All handlers should implement:

Integration_NameParser - > drag out data from request
Integration_NameHandler - > match request with process function, use parser to fastly receive data and fill the message information.

This solution is used there:
https://github.com/Vallher/zulip/blob/librato-integration/zerver/views/webhooks/librato.py
and I think it is clear and not complicated way.

However I actually just wrote only one small integration, so let's use this idea maybe like a starting point:)
I saw that @anteq wrote bigger thing (#663) and use some standarized structures, so maybe he will have more ideas.

[anteq]

Well, in my opinion the integrations need some refactoring, but I'm not sure if creating strict boundaries and dividing the functionalities to parsers/creators is the best choice.

My biggest concern is how to properly support versions of webhooks payloads. For example, now we have, if I remember correctly, two versions of Github integrations and I'm not sure what's differentiating between them. Another example - I was working on Taiga integration and I'm not sure if the new version of Taiga will not redefine the webhook payload so it will not be compatible with the previous one... In this case, dividing the code into parsers and creators might make sense - parsers would differ between versions and creators would create a consistent output.

Also, another idea about how to improve Zulip's integration is to simplify the user experience of activating a new integration. End user shouldn't have to create a "bot", remember the API key, create the URL on his own etc etc... Simplified UX in this case would involve Zulip generating the bot and correct webhook address automatically - user then would just have to copy and paste the generated webhook address. There could also be a place for customizing what types of messages should be posted and what should not - if every integration would explicitly define such message types, it should be possible to support user preferences in choosing which messages should be posted to Zulip and which shouldn't per integration.

Of course, the possibility of creating "raw" bots and "raw" webhooks should be preserved, for custom applications.

[timabbott]

@anteq, I agree with your thoughts on simplifying user experience for activating an integration; I created a separate issue for it (and as you can see I already had that idea in my draft Zulip roadmap)

For versioning, I think often vendors only change their APIs infrequently, and when they do it's a major revision. For major revisions, we could handle that the same way we do today with the GitHub integration -- just implement a separate integration endpoint for each version (can still share helper functions).

I'm not that excited by the idea of having a stricter parser/creator divide; in my worldview, ideally writing a new webhook would often be just a very small new function.

One approach on the parsing side is we use the check_dict/check_string validators that we use in routes elsewhere in the Zulip codebase to do some of the parsing for webhooks that are currently just doing a ujson.loads() on request.body.

[TomaszKolek]

@timabbott
Just leave it as a note. Really needed by re-writing GitHub webhook integration and testing.
http://www.ultrahook.com/ - kind of tunnel between some public endpoint and our local Zulip instance

[timabbott]

Cool, yeah, that does seem useful for developing integrations. If you want to add a section to the integration writing guide docs on it (and maybe also add a webhook endpoint one can use with Ultrahook to capture payloads to a file for fixtures without using a service like requestb.in), that'd definitely be valuable.

[timabbott]

@TomaszKolek another idea I was thinking we could do on the code side would be to change the webhook decorators to take an argument that's the name of the product being integrated and use that to generate the client name field. E.g. we could do:

@api_key_only_webhook_view("CircleCI")
def api_circleci_webhook(request, user_profile, client, payload=REQ(argument_type='body'), stream=REQ(default='circleci')):        
   ...

    check_send_message(user_profile, client, 'stream', [stream], subject, body)

and we'd just have modified api_key_only_webhook_view to generate the client object using get_client() and the argument it's passed.

This also enables us to have a registry of what Webhook integrations exist generated entirely from the code -- api_key_only_webhook_view can automatically add the integration into a data structure storing the available integrations using that user-facing name.

What do you think about that idea?

[TomaszKolek]

@timabbott. Sounds really good. I will try to implement i in separate PR

[TomaszKolek]

The latter is already implemented #785

[timabbott]

Nice, definitely looks a lot nicer than before! I posted a couple quick comments.

[timabbott]

OK here's another idea @TomaszKolek -- we're starting to have a few helper functions that we've been using in more than one webhook, and I think it's time we put those together in a library. One idea would be to create it as zerver/lib/webhook.py; I could also see doing zerver/views/webhooks/common.py or even put them in zerver/views/webhooks/init.py.

The things I have in mind for moving there are:

• guess_zulip_user_from_teamcity/jira
• build_message_from_gitlog (currently imported from the github webhook by other integrations)
• build_commit_list_content (currently imported from the github webhook by other integrations)

And then as we develop pieces of code that seem useful to multiple integrations, we can add them there as well. What do you think @TomaszKolek ?

[timabbott]

Another idea that I think would be good here is designing an abstraction that can cut the duplicated code we have in all of our webhook tests.

E.g. the 3 lines here for each tests are very self-similar: https://github.com/zulip/zulip/blob/master/zerver/tests/test_hooks.py#L1267

And then the functions at the top of each test class are also often the same:
https://github.com/zulip/zulip/blob/master/zerver/tests/test_hooks.py#L1071

So I think it make make sense to design a WebhookTestCase class that can provide nice abstractions for writing webhook tests to cut down the duplication of code both across and within the webhook tests. What do you think @TomaszKolek ?

[TomaszKolek]

@timabbott
I thought about it as well.
I think in this class we should create abstract properties like a stream, an email etc.
Make common setUp and one simple test method.

Sounds ok?

[timabbott]

Yeah something like that sounds good!

[timabbott]

While we haven't implemented all the initial ideas, I think we've successfully achieved the goals we had in mind here (the code required to build a new webhook integration is way less than how it began), so I'm closing this.