Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

Webhook Creation #694

Closed
wants to merge 52 commits into from
Closed

Webhook Creation #694

wants to merge 52 commits into from

Conversation

cyhsutw
Copy link
Contributor

@cyhsutw cyhsutw commented Aug 15, 2016

Part of #667, follow up of #668.

This pull request:

  • creates orgnization webhook when creating a new classroom (organization)
  • handles ping event of the webhook

TODO:

  • Add tests for WebhookEventController
  • Add background job to send ping event if a hook has not receive one yet

@cyhsutw cyhsutw changed the title Webhook Creation [WIP] Webhook Creation Aug 15, 2016
@cyhsutw cyhsutw added the WIP 💭 PR's that are a 'Work In Progress' label Aug 15, 2016
module OrganizationWebhook
extend ActiveSupport::Concern

def create_organization_webhook
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rubocop will complain if we put this method inside OrganizationController.

I'm not sure if it's correct to extract this as a concern.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with it being here for now.

@@ -46,8 +46,9 @@ def slugify
self.slug = "#{github_id} #{title}".parameterize
end

def create_organization_webhook(webhook_url)
webhook = github_organization.create_organization_webhook(config: { url: webhook_url })
def create_organization_webhook(webhook_url, client = nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an optional parameter client here just in case we randomly picked other admins who don't have enough token scopes to manipulate organization webhook.

describe 'ping event' do
context 'valid payload signature' do
before(:each) do
params = JSON.parse(File.open("#{Rails.root}/spec/support/fixtures/webhook_payloads/ping.json").read)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tarebyte I placed the JSON payload at this path, is this okay?

@cyhsutw
Copy link
Contributor Author

cyhsutw commented Aug 19, 2016

@tarebyte Thanks a lot for reviewing!

I've made changes based on your feedback. Would you mind taking a 👀 at them?

Thanks!

@cyhsutw cyhsutw added WIP 💭 PR's that are a 'Work In Progress' and removed ready for review labels Aug 24, 2016
@cyhsutw
Copy link
Contributor Author

cyhsutw commented Aug 24, 2016

I'll mark it as in progress for now since this PR should be a part of due date feature, which will be built on explicit assignment submission feature.

Please see #720 for the explicit assignment submission feature.

@cyhsutw cyhsutw reopened this Sep 7, 2016
@tarebyte tarebyte mentioned this pull request Sep 25, 2016
@cyhsutw
Copy link
Contributor Author

cyhsutw commented Nov 22, 2016

Closing this in favor of #756 (comment)

@cyhsutw cyhsutw closed this Nov 22, 2016
@cyhsutw cyhsutw deleted the webhook_creation branch November 22, 2016 07:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants