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

Database Migration and Model Update for Organization Webhook #668

Merged
merged 19 commits into from
Jul 25, 2016
Merged

Database Migration and Model Update for Organization Webhook #668

merged 19 commits into from
Jul 25, 2016

Conversation

cyhsutw
Copy link
Contributor

@cyhsutw cyhsutw commented Jul 11, 2016

Hello guys,

This pull request is a part of #667.

Summary of changes:

  • Create two webhook related fields for Organization
  • Add two methods to create and delete webhook on GitHub
  • Add related test cases

Would love to hear your feedback on this 😄

cyhsutw
cyhsutw reviewed Jul 11, 2016
View reviewed changes
app/models/organization.rb
@@ -22,6 +22,10 @@ class Organization < ActiveRecord::Base

validates :slug, uniqueness: true

validates :webhook_id, uniqueness: true, allow_nil: true
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 made this optional since this feature will be placed under Flipper.

@cyhsutw cyhsutw closed this Jul 11, 2016
@cyhsutw cyhsutw reopened this Jul 11, 2016
@cyhsutw
Copy link
Contributor Author

cyhsutw commented Jul 12, 2016 •
edited

Hi @tarebyte @johndbritton

I think this pull request is ready for review. Would you mind take a look at it when you're available?

Thanks 😄

tarebyte
tarebyte reviewed Jul 12, 2016
View reviewed changes
app/models/github_organization.rb
@@ -98,6 +98,24 @@ def team_invitations_url
"https://github.com/orgs/#{login}/invitations/new"
end

# Public
#
Copy link
Member

Choose a reason for hiding this comment

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

Let's ✂️ the comments out for now, they more of place holders for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✂️ in d5902e2

@tarebyte
Copy link
Member

@cyhsutw I'll give this a proper 👀 tomorrow.

tarebyte
tarebyte reviewed Jul 17, 2016
View reviewed changes
app/models/github_organization.rb
def create_organization_webhook(config: {}, options: {})
GitHub::Errors.with_error_handling do
hook_config = github_org_hook_default_config.merge(config)
.tap { |hash| hash[:secret] = ENV['WEBHOOK_SECRET'] }
Copy link
Member

Choose a reason for hiding this comment

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

Let's add this to the Rails secrets.yml file so that we don't pull from the ENV

@tarebyte
Copy link
Member

@cyhsutw this is looking ✨.

@cyhsutw
Copy link
Contributor Author

cyhsutw commented Jul 17, 2016

@tarebyte Thanks for the feedback and suggestions.

Changes have been made according to your comments.

I would really appreciate it if you could have a 👀 at them when you're available.

tarebyte
tarebyte reviewed Jul 25, 2016
View reviewed changes
app/models/organization.rb
def create_organization_webhook(webhook_url)
webhook = github_organization.create_organization_webhook(config: { url: webhook_url })
update_attributes(webhook_id: webhook.id)
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is method is called. Is will this be used in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will be used in the next PR.

@tarebyte
Copy link
Member

@cyhsutw I posted one last question, and then I think this'll be good to ship!

@tarebyte
Copy link
Member 

  ActiveRecord::SchemaMigration Load (13.4ms)  SELECT "schema_migrations".* FROM "schema_migrations"
Migrating to AddWebhookToOrganizations (20160620141513)
   (1.7ms)  BEGIN
== 20160620141513 AddWebhookToOrganizations: migrating ========================
-- add_column(:organizations, :webhook_id, :string)
   (8.3ms)  ALTER TABLE "organizations" ADD "webhook_id" character varying
   -> 0.0091s
-- add_column(:organizations, :is_webhook_active, :boolean, {:default=>false})
   (49.0ms)  ALTER TABLE "organizations" ADD "is_webhook_active" boolean DEFAULT 'f'
   -> 0.0561s
== 20160620141513 AddWebhookToOrganizations: migrated (0.0655s) ===============

  SQL (5.1ms)  INSERT INTO "schema_migrations" ("version") VALUES ($1)  [["version", "20160620141513"]]
   (16.5ms)  COMMIT

Migration was successful.

@tarebyte tarebyte merged commit 5444e46 into master Jul 25, 2016
@tarebyte tarebyte deleted the webhook-migrations branch July 25, 2016 13:35
@cyhsutw
Copy link
Contributor Author

cyhsutw commented Jul 25, 2016

Thanks a lot @tarebyte!

@cyhsutw cyhsutw mentioned this pull request Aug 15, 2016
Closed
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cyhsutw @tarebyte @BenEmdon