webhooks: Create Honeybadger Integration. #8066

ViRu-ThE-ViRuS · 2018-01-12T13:53:37Z

eeshangarg

@ViRu-ThE-ViRuS: This looks good! A couple of comments:

Add an empty doc.md file.
Add a logo file (the missing logo is the reason the build is failing).

Make sure you run ./tools/test-all before you submit a PR or push to it. If you do that, you'll find that you can discover most errors before someone reviews your PR! :) Thanks!

ViRu-ThE-ViRuS · 2018-01-12T17:23:08Z

I usually do that. Its just that the last PR I made for Webhooks (Intercom), someone commented that I should remove the empty doc file and the logo as they are part of another task [Link]. Nevermind... I will do as you suggest here and notify. :-)

eeshangarg · 2018-01-12T18:02:25Z

@ViRu-ThE-ViRuS: Yeah, so the reason Robert said that is because you have to write the contents of the doc.md file as part of another task. But you have to add en empty one to make sure that the build doesn't fail because it can't find a doc.md file. After you do that, you can then claim the documentation task and then write the doc.md file! :)

eeshangarg · 2018-01-12T18:38:52Z

@ViRu-ThE-ViRuS: This looks good to me! So I am approving this on GCI! I would recommend claiming the documentation task next and write the contents of the doc.md file! Thanks! :)

eeshangarg

Oops forgot to approve the changes! :)

ViRu-ThE-ViRuS · 2018-01-13T08:30:48Z

GCI Task for documentation.

eeshangarg

@ViRu-ThE-ViRuS: This looks good! I left one comment I'd like you to address! Thanks! :)

eeshangarg · 2018-01-13T16:15:50Z

zerver/webhooks/honeybadger/doc.md

+4. Test the webhook by clicking `Test This Integration`.
+
+5. Click `Save changes` to create the webhook.
+


@ViRu-ThE-ViRuS: This looks great! But you are supposed to have have screenshots for the various steps to make it more clear as to where the user is supposed to go/click, etc. I would recommend looking at how the other integrations are documented and adding similar screenshots for Honeybadger. Thanks! :)

Added some screenshots (but no so many that they become distracting)!

roberthoenig

Almost ready @ViRu-ThE-ViRuS! Left two comments. Additionally, I think there are too many screenshots. Instead of having one for every instructions, it's better to go with 1-2 screenshots at most, e.g. one for the beginning and one for the end.

roberthoenig · 2018-01-14T10:52:05Z

zerver/webhooks/honeybadger/doc.md

+the triggers for the webhook.
+![](/static/images/integrations/honeybadger/004.png)
+
+4. Test the webhook by clicking `Test This Integration`.


Screenshots shouldn't display any private tokens - even if you are fine with sharing them publicly. You could blur the key in this image, but I recommend removing it completely, as it is not crucial for the process and there already are several other screenshots.

roberthoenig · 2018-01-14T10:55:01Z

zerver/webhooks/honeybadger/doc.md

+
+{!create-bot-construct-url.md!}
+
+Honeybadger will use the `Honeybadger` stream by default if no stream is given


We recommend lower case stream names, and the introductory section of this integration doc recommends the stream name "honeybadger". Can you change "Honeybadger" to "honeybadger" here and for the stream name in your first commit?

sampritipanda · 2018-01-14T20:23:16Z

This is a minor nitpick but should we use just the bolt (https://www.honeybadger.io/assets/) as the logo without the "Honeybadger.io" text? That seems to go inline with the logos from the other integrations (https://chat.zulip.org/integrations/)

sampritipanda · 2018-01-14T20:51:24Z

zerver/webhooks/honeybadger/doc.md

+
+**Follow these steps:**
+
+1. Go to `Alerts & Integrations` page of your `Honeybadger` Console.


This step was a bit hard to follow for me. Something like "Go to the Alerts & Integrations page in the project settings" or however you would like to put it, would be better I think.

sampritipanda · 2018-01-14T21:03:51Z

zerver/webhooks/honeybadger/doc.md

+the triggers for the webhook.
+![](/static/images/integrations/honeybadger/003.png)
+
+4. Test the webhook by clicking `Test This Integration`.


How about "Test the webhook by clicking on the Test This Integration button at the bottom of the page".

Makes it a bit easier to find I think

aero31aero

LGTM. Ready for merging if Travis build succeeds.

ViRu-ThE-ViRuS · 2018-01-22T11:18:00Z

Sorry for the late update. Build seems to pass now. Can someone give a final review?

eeshangarg

@ViRu-ThE-ViRuS: Left a few more comments! This is mostly good to go! Also,

Squash the two commits into one please and make the commit message webhooks: Create Honeybadger integration..
After you address my feedback, please generate a new screenshot for the sample message!

Thanks for working on this! :)

eeshangarg · 2018-01-22T20:00:46Z

zerver/webhooks/honeybadger/view.py

+def api_honeybadger_webhook(request: HttpRequest, user_profile: UserProfile,
+                            payload: Dict[str, Any] = REQ(argument_type='body'),
+                            stream: Text = REQ(default='honeybadger')) -> HttpResponse:
+    body = '**{}**: {}'.format(payload['event'], payload['message'])


I don't think the message needs to have the **up**:, since the event type is already in the topic of the message. :)

eeshangarg · 2018-01-22T20:04:54Z

zerver/webhooks/honeybadger/view.py

+        body += '\n**Message**: {}\n**Site**: [{}]({})'.format(payload['fault']['message'],
+                                                               payload['project']['name'],
+                                                               payload['fault']['url'])
+


I would have the topic include the <event_type>: <site>. :)

eeshangarg · 2018-01-22T20:05:58Z

zerver/webhooks/honeybadger/view.py

+def api_honeybadger_webhook(request: HttpRequest, user_profile: UserProfile,
+                            payload: Dict[str, Any] = REQ(argument_type='body'),
+                            stream: Text = REQ(default='honeybadger')) -> HttpResponse:
+    body = '**{}**: {}'.format(payload['event'], payload['message'])


Also, maybe surround payload['message'] by a quoteblock, since it looks like a Traceback.

eeshangarg · 2018-01-22T20:06:30Z

zerver/webhooks/honeybadger/view.py

+        body += '\n**Site**: [{}]({})'.format(payload['site']['name'], payload['site']['url'])
+
+    elif payload['event'] == 'occurred':
+        body += '\n**Message**: {}\n**Site**: [{}]({})'.format(payload['fault']['message'],


I don't think you need **Message**: before the message. :)

ViRu-ThE-ViRuS · 2018-01-26T12:09:25Z

@eeshangarg Changes made.

eeshangarg

@ViRu-ThE-ViRuS: A couple of more final comments, mostly about code style! Thanks for bearing with me through multiple rounds of feedback! Looking forward to seeing this get merged! :)

eeshangarg · 2018-01-26T19:38:22Z

zerver/webhooks/honeybadger/view.py

+                                                    payload['fault']['url'])
+
+        check_send_stream_message(user_profile, request.client, stream,
+                                  '{}: {}'.format(payload['event'], payload['project']['name']), body)


@ViRu-ThE-ViRuS: This section is a bit confusing. I would suggest doing something like:

topic = '{}: {}'.format(payload['event'], payload['project']['name']) check_send_stream_message(user_profile, request.client, stream, topic, body)

Do that for all of the code in thie file! :)

eeshangarg · 2018-01-26T19:40:27Z

zerver/webhooks/honeybadger/view.py

+    body = '**{}**'.format(payload['message'])
+
+    if payload['event'] == 'check_in_missing':
+        reported_at = datetime.strptime(payload['check_in']['reported_at'], "%Y-%m-%dT%H:%M:%S.%fZ") \


Also, could you please wrap this code at 80 characters per line? And I would suggest avoiding breaking up function calls over multiple lines with a \, like you do with .strftime(). :)

timabbott · 2018-03-05T04:12:46Z

@ViRu-ThE-ViRuS are you planning to finish this? You haven't replied to @eeshangarg's comments in a month.

punchagan · 2020-03-30T03:47:11Z

@ViRu-ThE-ViRuS thanks for the initial work on this.
#14281 adds a more complete integration.

zulipbot added the needs review label Jan 12, 2018

eeshangarg reviewed Jan 12, 2018

View reviewed changes

zulipbot added needs more work and removed needs review labels Jan 12, 2018

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from cfb0b25 to 5420e6d Compare January 12, 2018 17:55

zulipbot added needs review and removed needs more work labels Jan 12, 2018

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from 5420e6d to ac7911f Compare January 12, 2018 18:06

eeshangarg approved these changes Jan 12, 2018

View reviewed changes

zulipbot added needs more work and removed needs review labels Jan 12, 2018

eeshangarg reviewed Jan 13, 2018

View reviewed changes

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from 7ae6a64 to b57e549 Compare January 14, 2018 08:09

zulipbot added the size: L label Jan 14, 2018

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from b57e549 to 091a9a5 Compare January 14, 2018 08:11

roberthoenig reviewed Jan 14, 2018

View reviewed changes

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from 091a9a5 to 3204941 Compare January 14, 2018 12:52

sampritipanda reviewed Jan 14, 2018

View reviewed changes

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch 2 times, most recently from e484937 to de52e5e Compare January 15, 2018 10:18

aero31aero approved these changes Jan 15, 2018

View reviewed changes

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from de52e5e to ba5445b Compare January 22, 2018 10:55

eeshangarg reviewed Jan 22, 2018

View reviewed changes

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from ba5445b to 93e0000 Compare January 26, 2018 12:06

zulipbot added size: XL and removed size: L labels Jan 26, 2018

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from 93e0000 to 0513fe7 Compare January 26, 2018 12:14

webhooks: Create Honeybadger Integration.

b05d5a0

ViRu-ThE-ViRuS force-pushed the honeybadger_integration_code branch from 0513fe7 to b05d5a0 Compare January 26, 2018 12:31

eeshangarg reviewed Jan 26, 2018

View reviewed changes

adnrs96 mentioned this pull request May 11, 2018

Migration: Move from the use of typing.Text to use str #9360

Closed

punchagan closed this Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webhooks: Create Honeybadger Integration. #8066

webhooks: Create Honeybadger Integration. #8066

ViRu-ThE-ViRuS commented Jan 12, 2018

eeshangarg left a comment

ViRu-ThE-ViRuS commented Jan 12, 2018

eeshangarg commented Jan 12, 2018

eeshangarg commented Jan 12, 2018

eeshangarg left a comment

ViRu-ThE-ViRuS commented Jan 13, 2018 •

edited

Loading

eeshangarg left a comment

eeshangarg Jan 13, 2018

ViRu-ThE-ViRuS Jan 14, 2018

roberthoenig left a comment

roberthoenig Jan 14, 2018

roberthoenig Jan 14, 2018

ViRu-ThE-ViRuS Jan 14, 2018

sampritipanda commented Jan 14, 2018 •

edited

Loading

sampritipanda Jan 14, 2018 •

edited

Loading

sampritipanda Jan 14, 2018

ViRu-ThE-ViRuS Jan 15, 2018

aero31aero left a comment •

edited

Loading

ViRu-ThE-ViRuS commented Jan 22, 2018 •

edited

Loading

eeshangarg left a comment

eeshangarg Jan 22, 2018

eeshangarg Jan 22, 2018

eeshangarg Jan 22, 2018

eeshangarg Jan 22, 2018

ViRu-ThE-ViRuS commented Jan 26, 2018

eeshangarg left a comment

eeshangarg Jan 26, 2018

eeshangarg Jan 26, 2018

timabbott commented Mar 5, 2018

punchagan commented Mar 30, 2020

		4. Test the webhook by clicking `Test This Integration`.

		5. Click `Save changes` to create the webhook.


		{!create-bot-construct-url.md!}

		Honeybadger will use the `Honeybadger` stream by default if no stream is given


		Follow these steps:

		1. Go to `Alerts & Integrations` page of your `Honeybadger` Console.

webhooks: Create Honeybadger Integration. #8066

webhooks: Create Honeybadger Integration. #8066

Conversation

ViRu-ThE-ViRuS commented Jan 12, 2018

eeshangarg left a comment

Choose a reason for hiding this comment

ViRu-ThE-ViRuS commented Jan 12, 2018

eeshangarg commented Jan 12, 2018

eeshangarg commented Jan 12, 2018

eeshangarg left a comment

Choose a reason for hiding this comment

ViRu-ThE-ViRuS commented Jan 13, 2018 • edited Loading

eeshangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roberthoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampritipanda commented Jan 14, 2018 • edited Loading

sampritipanda Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aero31aero left a comment • edited Loading

Choose a reason for hiding this comment

ViRu-ThE-ViRuS commented Jan 22, 2018 • edited Loading

eeshangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViRu-ThE-ViRuS commented Jan 26, 2018

eeshangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Mar 5, 2018

punchagan commented Mar 30, 2020

ViRu-ThE-ViRuS commented Jan 13, 2018 •

edited

Loading

sampritipanda commented Jan 14, 2018 •

edited

Loading

sampritipanda Jan 14, 2018 •

edited

Loading

aero31aero left a comment •

edited

Loading

ViRu-ThE-ViRuS commented Jan 22, 2018 •

edited

Loading