Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] webhooks/codebase: Implementing Codebase Webhook using Event Hook API. #12260

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sameerchoubey
Copy link
Collaborator

@sameerchoubey sameerchoubey commented May 3, 2019

This PR currently add support for two events.

ticket_creation and ticket_update

@eeshangarg Can you please look at this?

Fixes #9395

@sameerchoubey sameerchoubey force-pushed the event_hook branch 2 times, most recently from d17ae4e to 6eea43d Compare May 3, 2019 21:03
Copy link
Member

@eeshangarg eeshangarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameerchoubey: Thanks for working on this! I just left a few comments. Also, I would recommend fixing the build errors and amending the commit message to be webhooks/codebase: Add Event Hook API implementation. :)

Cheers! :)


1. Go to your Codebase Dashboard, and click on the settings icon in
the top-right corner. Click on **Event Hooks**.
Now, click **New Event Webhook**.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would join the last two sentences together, Click on **Event Hooks**, and click **New Event Webhook**. It reads better, imo!

name = payload['user']['name']
url = payload['ticket']['project']['url']

body = "Ticket with ID **[{}]({})**, category **{}** has been updated by **{}**".format(id, url, type, name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things:

  • I would move both body template strings to the top of the file with all uppercase names, such as TICKET_UPDATE_TEMPLATE. I would also use named format arguments, such as "has been updated by {author}".format(author=author)
  • Also, I would recommend ending all messages with proper punctuation, i.e., a period. I am currently doing an audit to make sure we use proper punctuation across all of our webhooks.

payload = payload['payload']
payload = json.loads(payload)

type = payload['type']['name']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a reserved Python keyword, I would replace this with something like ticket_type.

payload = json.loads(payload)

id = payload['ticket']['id']
type = payload['ticket']['category']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both id and type are reserved keywords in Python, it is a good habit to use descriptive alternatives such as ticket_type or ticket_id instead!

payload: Dict[str, Iterable[Dict[str, Any]]]=REQ(argument_type='body')
) -> HttpResponse:

if payload is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. is the payload every actually None? I would be surprised if it is.

@sameerchoubey
Copy link
Collaborator Author

@eeshangarg Yeah. I'll make these changes and also add other events.

Thanks for reviewing.

@zulipbot
Copy link
Member

zulipbot commented Sep 9, 2021

Heads up @sameerchoubey, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completion candidate PRs with reviews that may unblock merging has conflicts size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Codebase webhook using the Event Hook API
4 participants