From 65dc68b35c0ad455336abf33def5d920166f7c83 Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Thu, 12 Jul 2012 15:36:33 +0300 Subject: Refactoring of hook functionality & bootsrap system hooks --- Gemfile | 2 +- app/controllers/hooks_controller.rb | 12 ++++++------ app/models/project.rb | 2 +- app/models/project_hook.rb | 3 +++ app/models/system_hook.rb | 3 +++ app/models/web_hook.rb | 2 -- app/roles/git_push.rb | 2 +- db/migrate/20120712080407_add_type_to_web_hook.rb | 5 +++++ db/schema.rb | 7 ++++--- 9 files changed, 24 insertions(+), 14 deletions(-) create mode 100644 app/models/project_hook.rb create mode 100644 app/models/system_hook.rb create mode 100644 db/migrate/20120712080407_add_type_to_web_hook.rb diff --git a/Gemfile b/Gemfile index c89d244333f..76dc1856424 100644 --- a/Gemfile +++ b/Gemfile @@ -71,7 +71,6 @@ group :development, :test do gem "awesome_print" gem "database_cleaner" gem "launchy" - gem "webmock" end group :test do @@ -82,4 +81,5 @@ group :test do gem "shoulda-matchers" gem 'email_spec' gem 'resque_spec' + gem "webmock" end diff --git a/app/controllers/hooks_controller.rb b/app/controllers/hooks_controller.rb index 9627aba9771..ad2fb3ae781 100644 --- a/app/controllers/hooks_controller.rb +++ b/app/controllers/hooks_controller.rb @@ -11,24 +11,24 @@ class HooksController < ApplicationController respond_to :html def index - @hooks = @project.web_hooks.all - @hook = WebHook.new + @hooks = @project.hooks.all + @hook = ProjectHook.new end def create - @hook = @project.web_hooks.new(params[:hook]) + @hook = @project.hooks.new(params[:hook]) @hook.save if @hook.valid? redirect_to project_hooks_path(@project) else - @hooks = @project.web_hooks.all + @hooks = @project.hooks.all render :index end end def test - @hook = @project.web_hooks.find(params[:id]) + @hook = @project.hooks.find(params[:id]) commits = @project.commits(@project.default_branch, nil, 3) data = @project.post_receive_data(commits.last.id, commits.first.id, "refs/heads/#{@project.default_branch}", current_user) @hook.execute(data) @@ -37,7 +37,7 @@ class HooksController < ApplicationController end def destroy - @hook = @project.web_hooks.find(params[:id]) + @hook = @project.hooks.find(params[:id]) @hook.destroy redirect_to project_hooks_path(@project) diff --git a/app/models/project.rb b/app/models/project.rb index ec4893e2b17..4773cf373f3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -19,7 +19,7 @@ class Project < ActiveRecord::Base has_many :notes, :dependent => :destroy has_many :snippets, :dependent => :destroy has_many :deploy_keys, :dependent => :destroy, :foreign_key => "project_id", :class_name => "Key" - has_many :web_hooks, :dependent => :destroy + has_many :hooks, :dependent => :destroy, :class_name => "ProjectHook" has_many :wikis, :dependent => :destroy has_many :protected_branches, :dependent => :destroy diff --git a/app/models/project_hook.rb b/app/models/project_hook.rb new file mode 100644 index 00000000000..06388aaeb4c --- /dev/null +++ b/app/models/project_hook.rb @@ -0,0 +1,3 @@ +class ProjectHook < WebHook + belongs_to :project +end diff --git a/app/models/system_hook.rb b/app/models/system_hook.rb new file mode 100644 index 00000000000..178b7585d5b --- /dev/null +++ b/app/models/system_hook.rb @@ -0,0 +1,3 @@ +class SystemHook < WebHook + +end diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index 26288476a6c..43b4f16b846 100644 --- a/app/models/web_hook.rb +++ b/app/models/web_hook.rb @@ -4,8 +4,6 @@ class WebHook < ActiveRecord::Base # HTTParty timeout default_timeout 10 - belongs_to :project - validates :url, presence: true, format: { diff --git a/app/roles/git_push.rb b/app/roles/git_push.rb index b4c59472a5a..d0267b59b6d 100644 --- a/app/roles/git_push.rb +++ b/app/roles/git_push.rb @@ -35,7 +35,7 @@ module GitPush data = post_receive_data(oldrev, newrev, ref, user) - web_hooks.each { |web_hook| web_hook.execute(data) } + hooks.each { |web_hook| web_hook.execute(data) } end def post_receive_data(oldrev, newrev, ref, user) diff --git a/db/migrate/20120712080407_add_type_to_web_hook.rb b/db/migrate/20120712080407_add_type_to_web_hook.rb new file mode 100644 index 00000000000..18ab024c817 --- /dev/null +++ b/db/migrate/20120712080407_add_type_to_web_hook.rb @@ -0,0 +1,5 @@ +class AddTypeToWebHook < ActiveRecord::Migration + def change + add_column :web_hooks, :type, :string, :default => "ProjectHook" + end +end diff --git a/db/schema.rb b/db/schema.rb index f40ee260dc3..c4c54f562a3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20120706065612) do +ActiveRecord::Schema.define(:version => 20120712080407) do create_table "events", :force => true do |t| t.string "target_type" @@ -187,8 +187,9 @@ ActiveRecord::Schema.define(:version => 20120706065612) do create_table "web_hooks", :force => true do |t| t.string "url" t.integer "project_id" - t.datetime "created_at", :null => false - t.datetime "updated_at", :null => false + t.datetime "created_at", :null => false + t.datetime "updated_at", :null => false + t.string "type", :default => "ProjectHook" end create_table "wikis", :force => true do |t| -- cgit v1.2.1 From c38578428b19b6375f24266ea5f4a98ca290eb58 Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Thu, 12 Jul 2012 22:10:34 +0300 Subject: System Hooks: CRUD has done --- app/controllers/admin/hooks_controller.rb | 39 ++++++++++++++++++++++++++ app/controllers/admin/mailer_controller.rb | 45 ------------------------------ app/models/web_hook.rb | 2 -- app/views/admin/hooks/_data_ex.html.erb | 45 ++++++++++++++++++++++++++++++ app/views/admin/hooks/index.html.haml | 39 ++++++++++++++++++++++++++ app/views/admin/mailer/preview.html.haml | 28 ------------------- app/views/help/system_hooks.html.haml | 13 +++++++++ app/views/layouts/admin.html.haml | 2 +- config/routes.rb | 5 +++- 9 files changed, 141 insertions(+), 77 deletions(-) create mode 100644 app/controllers/admin/hooks_controller.rb delete mode 100644 app/controllers/admin/mailer_controller.rb create mode 100644 app/views/admin/hooks/_data_ex.html.erb create mode 100644 app/views/admin/hooks/index.html.haml delete mode 100644 app/views/admin/mailer/preview.html.haml create mode 100644 app/views/help/system_hooks.html.haml diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb new file mode 100644 index 00000000000..2e47b6c2c4e --- /dev/null +++ b/app/controllers/admin/hooks_controller.rb @@ -0,0 +1,39 @@ +class Admin::HooksController < ApplicationController + layout "admin" + before_filter :authenticate_user! + before_filter :authenticate_admin! + + def index + @hooks = SystemHook.all + @hook = SystemHook.new + end + + def create + @hook = SystemHook.new(params[:hook]) + + respond_to do |format| + if @hook.save + format.html { redirect_to admin_hooks_path, notice: 'Hook was successfully created.' } + else + format.html { render :index } + end + end + end + + def destroy + @hook = SystemHook.find(params[:id]) + @hook.destroy + + redirect_to admin_hooks_path + end + + + def test + @hook = @project.hooks.find(params[:id]) + commits = @project.commits(@project.default_branch, nil, 3) + data = @project.post_receive_data(commits.last.id, commits.first.id, "refs/heads/#{@project.default_branch}", current_user) + @hook.execute(data) + + redirect_to :back + end +end diff --git a/app/controllers/admin/mailer_controller.rb b/app/controllers/admin/mailer_controller.rb deleted file mode 100644 index 2352e189204..00000000000 --- a/app/controllers/admin/mailer_controller.rb +++ /dev/null @@ -1,45 +0,0 @@ -class Admin::MailerController < ApplicationController - layout "admin" - before_filter :authenticate_user! - before_filter :authenticate_admin! - - def preview - - end - - def preview_note - @note = Note.first - @user = @note.author - @project = @note.project - case params[:type] - when "Commit" then - @commit = @project.commit - render :file => 'notify/note_commit_email', :layout => 'notify' - when "Issue" then - @issue = Issue.first - render :file => 'notify/note_issue_email', :layout => 'notify' - else - render :file => 'notify/note_wall_email', :layout => 'notify' - end - rescue - render :text => "Preview not available" - end - - def preview_user_new - @user = User.first - @password = "DHasJKDHAS!" - - render :file => 'notify/new_user_email', :layout => 'notify' - rescue - render :text => "Preview not available" - end - - def preview_issue_new - @issue = Issue.first - @user = @issue.assignee - @project = @issue.project - render :file => 'notify/new_issue_email', :layout => 'notify' - rescue - render :text => "Preview not available" - end -end diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index 43b4f16b846..7a0423481db 100644 --- a/app/models/web_hook.rb +++ b/app/models/web_hook.rb @@ -12,8 +12,6 @@ class WebHook < ActiveRecord::Base def execute(data) WebHook.post(url, body: data.to_json, headers: { "Content-Type" => "application/json" }) - rescue - # There was a problem calling this web hook, let's forget about it. end end # == Schema Information diff --git a/app/views/admin/hooks/_data_ex.html.erb b/app/views/admin/hooks/_data_ex.html.erb new file mode 100644 index 00000000000..8d3de3f0bf2 --- /dev/null +++ b/app/views/admin/hooks/_data_ex.html.erb @@ -0,0 +1,45 @@ +<% data_ex_str = < "95790bf891e76fee5e1747ab589903a6a1f80f22", + :after => "da1560886d4f094c3e6c9ef40349f7d38b5d27d7", + :ref => "refs/heads/master", + :user_id => 4, + :user_name => "John Smith", + :repository => { + :name => "Diaspora", + :url => "localhost/diaspora", + :description => "", + :homepage => "localhost/diaspora", + :private => true + }, + :commits => [ + [0] { + :id => "450d0de7532f8b663b9c5cce183b...", + :message => "Update Catalan translation to e38cb41.", + :timestamp => "2011-12-12T14:27:31+02:00", + :url => "http://localhost/diaspora/commits/450d0de7532f...", + :author => { + :name => "Jordi Mallach", + :email => "jordi@softcatala.org" + } + }, + + .... + + [3] { + :id => "da1560886d4f094c3e6c9ef40349...", + :message => "fixed readme", + :timestamp => "2012-01-03T23:36:29+02:00", + :url => "http://localhost/diaspora/commits/da1560886d...", + :author => { + :name => "gitlab dev user", + :email => "gitlabdev@dv6700.(none)" + } + } + ], + total_commits_count => 3 +} +eos +%> +<% js_lexer = Pygments::Lexer[:js] %> +<%= raw js_lexer.highlight(data_ex_str) %> diff --git a/app/views/admin/hooks/index.html.haml b/app/views/admin/hooks/index.html.haml new file mode 100644 index 00000000000..030e6136b1f --- /dev/null +++ b/app/views/admin/hooks/index.html.haml @@ -0,0 +1,39 @@ +.alert.alert-info + %span + Post receive hooks for binding events. + %br + Read more about system hooks + %strong #{link_to "here", help_system_hooks_path, :class => "vlink"} + += form_for @hook, :as => :hook, :url => admin_hooks_path do |f| + -if @hook.errors.any? + .alert-message.block-message.error + - @hook.errors.full_messages.each do |msg| + %p= msg + .clearfix + = f.label :url, "URL:" + .input + = f.text_field :url, :class => "text_field xxlarge" +   + = f.submit "Add System Hook", :class => "btn primary" +%hr + +-if @hooks.any? + %h3 + Hooks + %small (#{@hooks.count}) + %br + %table.admin-table + %tr + %th URL + %th Method + %th + - @hooks.each do |hook| + %tr + %td + = link_to admin_hook_path(hook) do + %strong= hook.url + = link_to 'Test Hook', admin_hook_test_path(hook), :class => "btn small right" + %td POST + %td + = link_to 'Remove', admin_hook_path(hook), :confirm => 'Are you sure?', :method => :delete, :class => "danger btn small right" diff --git a/app/views/admin/mailer/preview.html.haml b/app/views/admin/mailer/preview.html.haml deleted file mode 100644 index 23ea7381cf5..00000000000 --- a/app/views/admin/mailer/preview.html.haml +++ /dev/null @@ -1,28 +0,0 @@ -%p This is page with preview for all system emails that are sent to user -%p Email previews built based on existing Project/Commit/Issue base - so some preview maybe unavailable unless object appear in system - -#accordion - %h3 - %a New user - %div - %iframe{ :src=> admin_mailer_preview_user_new_path, :width=>"100%", :height=>"350"} - %h3 - %a New issue - %div - %iframe{ :src=> admin_mailer_preview_issue_new_path, :width=>"100%", :height=>"350"} - %h3 - %a Commit note - %div - %iframe{ :src=> admin_mailer_preview_note_path(:type => "Commit"), :width=>"100%", :height=>"350"} - %h3 - %a Issue note - %div - %iframe{ :src=> admin_mailer_preview_note_path(:type => "Issue"), :width=>"100%", :height=>"350"} - %h3 - %a Wall note - %div - %iframe{ :src=> admin_mailer_preview_note_path(:type => "Wall"), :width=>"100%", :height=>"350"} - -:javascript - $(function() { - $("#accordion").accordion(); }); diff --git a/app/views/help/system_hooks.html.haml b/app/views/help/system_hooks.html.haml new file mode 100644 index 00000000000..2088208ad47 --- /dev/null +++ b/app/views/help/system_hooks.html.haml @@ -0,0 +1,13 @@ +%h3 System hooks +.back_link + = link_to :back do + ← back +%hr + +%p.slead + Your Gitlab instance can perform HTTP POST request on next event: create_project, delete_project, create_user, delete_user, change_team_member. + %br + System Hooks can be used for logging or change information in LDAP server. + %br +%h5 Hooks request example: += render "admin/hooks/data_ex" diff --git a/app/views/layouts/admin.html.haml b/app/views/layouts/admin.html.haml index 8de25821ee7..69304edef3a 100644 --- a/app/views/layouts/admin.html.haml +++ b/app/views/layouts/admin.html.haml @@ -15,7 +15,7 @@ %li{:class => tab_class(:admin_logs)} = link_to "Logs", admin_logs_path %li{:class => tab_class(:admin_emails)} - = link_to "Emails", admin_emails_path + = link_to "Hooks", admin_hooks_path %li{:class => tab_class(:admin_resque)} = link_to "Resque", admin_resque_path diff --git a/config/routes.rb b/config/routes.rb index 73b9f643ad5..dea4df46a30 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -28,6 +28,7 @@ Gitlab::Application.routes.draw do get 'help/workflow' => 'help#workflow' get 'help/api' => 'help#api' get 'help/web_hooks' => 'help#web_hooks' + get 'help/system_hooks' => 'help#system_hooks' # # Admin Area @@ -47,11 +48,13 @@ Gitlab::Application.routes.draw do end end resources :team_members, :only => [:edit, :update, :destroy] - get 'emails', :to => 'mailer#preview' get 'mailer/preview_note' get 'mailer/preview_user_new' get 'mailer/preview_issue_new' + resources :hooks, :only => [:index, :create, :destroy] do + get :test + end resource :logs resource :resque, :controller => 'resque' root :to => "dashboard#index" -- cgit v1.2.1 From f5908cef19a3cd2c44be02b453fecb568b2c6c8e Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Sun, 15 Jul 2012 15:29:06 +0300 Subject: System Hook: implemented --- app/controllers/admin/hooks_controller.rb | 11 +++++------ app/models/project.rb | 28 +++++++++++++++++++++++++++- app/models/system_hook.rb | 10 ++++++++++ app/models/user.rb | 19 +++++++++++++++++++ app/models/users_project.rb | 30 +++++++++++++++++++++++++++++- app/models/web_hook.rb | 1 + app/workers/system_hook_worker.rb | 7 +++++++ resque.sh | 2 +- resque_dev.sh | 2 +- 9 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 app/workers/system_hook_worker.rb diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb index 2e47b6c2c4e..446d4ea90a6 100644 --- a/app/controllers/admin/hooks_controller.rb +++ b/app/controllers/admin/hooks_controller.rb @@ -11,12 +11,11 @@ class Admin::HooksController < ApplicationController def create @hook = SystemHook.new(params[:hook]) - respond_to do |format| - if @hook.save - format.html { redirect_to admin_hooks_path, notice: 'Hook was successfully created.' } - else - format.html { render :index } - end + if @hook.save + redirect_to admin_hooks_path, notice: 'Hook was successfully created.' + else + @hooks = SystemHook.all + render :index end end diff --git a/app/models/project.rb b/app/models/project.rb index 4773cf373f3..d3dac34ae89 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -107,6 +107,32 @@ class Project < ActiveRecord::Base validate :check_limit validate :repo_name + after_create :create_hooks + after_destroy :destroy_hooks + + def create_hooks + SystemHook.all_hooks_fire({ + event_name: "project_create", + name: self.name, + path: self.path, + project_id: self.id, + owner_name: self.owner.name, + owner_email: self.owner.email, + created_at: self.created_at + }) + end + + def destroy_hooks + SystemHook.all_hooks_fire({ + event_name: "project_destroy", + name: self.name, + path: self.path, + project_id: self.id, + owner_name: self.owner.name, + owner_email: self.owner.email, + }) + end + def check_limit unless owner.can_create_project? errors[:base] << ("Your own projects limit is #{owner.projects_limit}! Please contact administrator to increase it") @@ -120,7 +146,7 @@ class Project < ActiveRecord::Base errors.add(:path, " like 'gitolite-admin' is not allowed") end end - + def self.access_options UsersProject.access_roles end diff --git a/app/models/system_hook.rb b/app/models/system_hook.rb index 178b7585d5b..8517d43a9de 100644 --- a/app/models/system_hook.rb +++ b/app/models/system_hook.rb @@ -1,3 +1,13 @@ class SystemHook < WebHook + + def async_execute(data) + Resque.enqueue(SystemHookWorker, id, data) + end + def self.all_hooks_fire(data) + SystemHook.all.each do |sh| + sh.async_execute data + end + end + end diff --git a/app/models/user.rb b/app/models/user.rb index a3e08fa7d0b..8eb114c4727 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,6 +57,25 @@ class User < ActiveRecord::Base scope :active, where(:blocked => false) before_validation :generate_password, :on => :create + after_create :create_hooks + after_destroy :destroy_hooks + + def create_hooks + SystemHook.all_hooks_fire({ + event_name: "user_create", + name: self.name, + email: self.email, + created_at: self.created_at + }) + end + + def destroy_hooks + SystemHook.all_hooks_fire({ + event_name: "user_destroy", + name: self.name, + email: self.email + }) + end def generate_password if self.force_random_password diff --git a/app/models/users_project.rb b/app/models/users_project.rb index 6ba72370931..128e61b7682 100644 --- a/app/models/users_project.rb +++ b/app/models/users_project.rb @@ -11,6 +11,9 @@ class UsersProject < ActiveRecord::Base after_save :update_repository after_destroy :update_repository + after_create :add_to_team_hooks + after_destroy :remove_from_team_hooks + validates_uniqueness_of :user_id, :scope => [:project_id] validates_presence_of :user_id @@ -18,6 +21,31 @@ class UsersProject < ActiveRecord::Base delegate :name, :email, :to => :user, :prefix => true + def add_to_team_hooks + SystemHook.all_hooks_fire({ + event_name: "user_add_to_team", + project_name: self.project.name, + project_path: self.project.path, + project_id: self.project_id, + user_name: self.user.name, + user_email: self.user.email, + project_access: self.repo_access_human, + created_at: self.created_at + }) + end + + def remove_from_team_hooks + SystemHook.all_hooks_fire({ + event_name: "user_remove_from_team", + project_name: self.project.name, + project_path: self.project.path, + project_id: self.project_id, + user_name: self.user.name, + user_email: self.user.email, + project_access: self.repo_access_human + }) + end + def self.bulk_import(project, user_ids, project_access) UsersProject.transaction do user_ids.each do |user_id| @@ -68,7 +96,7 @@ class UsersProject < ActiveRecord::Base end def repo_access_human - "" + self.class.access_roles.invert[self.project_access] end end # == Schema Information diff --git a/app/models/web_hook.rb b/app/models/web_hook.rb index 7a0423481db..85d87898682 100644 --- a/app/models/web_hook.rb +++ b/app/models/web_hook.rb @@ -13,6 +13,7 @@ class WebHook < ActiveRecord::Base def execute(data) WebHook.post(url, body: data.to_json, headers: { "Content-Type" => "application/json" }) end + end # == Schema Information # diff --git a/app/workers/system_hook_worker.rb b/app/workers/system_hook_worker.rb new file mode 100644 index 00000000000..ca154136b97 --- /dev/null +++ b/app/workers/system_hook_worker.rb @@ -0,0 +1,7 @@ +class SystemHookWorker + @queue = :system_hook + + def self.perform(hook_id, data) + SystemHook.find(hook_id).execute data + end +end diff --git a/resque.sh b/resque.sh index ce7c944b735..ab67c650805 100755 --- a/resque.sh +++ b/resque.sh @@ -1,2 +1,2 @@ mkdir -p tmp/pids -bundle exec rake environment resque:work QUEUE=post_receive,mailer RAILS_ENV=production PIDFILE=tmp/pids/resque_worker.pid BACKGROUND=yes +bundle exec rake environment resque:work QUEUE=post_receive,mailer,system_hook RAILS_ENV=production PIDFILE=tmp/pids/resque_worker.pid BACKGROUND=yes diff --git a/resque_dev.sh b/resque_dev.sh index 9df4dc1d087..b09cfd9e383 100755 --- a/resque_dev.sh +++ b/resque_dev.sh @@ -1 +1 @@ -bundle exec rake environment resque:work QUEUE=* VVERBOSE=1 +bundle exec rake environment resque:work QUEUE=post_receive,mailer,system_hook VVERBOSE=1 -- cgit v1.2.1 From 655418bed2f81651c54988963713769f85d8b5da Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Sun, 15 Jul 2012 17:36:06 +0300 Subject: System hooks: fix broken tests --- app/roles/git_push.rb | 6 +++--- spec/factories.rb | 2 +- spec/models/project_hooks_spec.rb | 28 ++++++++++++++-------------- spec/models/project_spec.rb | 2 +- spec/models/web_hook_spec.rb | 20 ++++++++++---------- spec/requests/admin/security_spec.rb | 8 ++++---- spec/requests/hooks_spec.rb | 7 ++++--- spec/workers/post_receive_spec.rb | 4 ++-- 8 files changed, 39 insertions(+), 38 deletions(-) diff --git a/app/roles/git_push.rb b/app/roles/git_push.rb index d0267b59b6d..4ee7e62a69e 100644 --- a/app/roles/git_push.rb +++ b/app/roles/git_push.rb @@ -27,7 +27,7 @@ module GitPush true end - def execute_web_hooks(oldrev, newrev, ref, user) + def execute_hooks(oldrev, newrev, ref, user) ref_parts = ref.split('/') # Return if this is not a push to a branch (e.g. new commits) @@ -35,7 +35,7 @@ module GitPush data = post_receive_data(oldrev, newrev, ref, user) - hooks.each { |web_hook| web_hook.execute(data) } + hooks.each { |hook| hook.execute(data) } end def post_receive_data(oldrev, newrev, ref, user) @@ -97,7 +97,7 @@ module GitPush self.update_merge_requests(oldrev, newrev, ref, user) # Execute web hooks - self.execute_web_hooks(oldrev, newrev, ref, user) + self.execute_hooks(oldrev, newrev, ref, user) # Create satellite self.satellite.create unless self.satellite.exists? diff --git a/spec/factories.rb b/spec/factories.rb index ea8c7aef0e2..d6570551803 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -60,7 +60,7 @@ Factory.add(:key, Key) do |obj| obj.key = File.read(File.join(Rails.root, "db", "pkey.example")) end -Factory.add(:web_hook, WebHook) do |obj| +Factory.add(:project_hook, ProjectHook) do |obj| obj.url = Faker::Internet.uri("http") end diff --git a/spec/models/project_hooks_spec.rb b/spec/models/project_hooks_spec.rb index fcc969ceba5..5544c5a8683 100644 --- a/spec/models/project_hooks_spec.rb +++ b/spec/models/project_hooks_spec.rb @@ -21,44 +21,44 @@ describe Project, "Hooks" do end end - describe "Web hooks" do + describe "Project hooks" do context "with no web hooks" do it "raises no errors" do lambda { - project.execute_web_hooks('oldrev', 'newrev', 'ref', @user) + project.execute_hooks('oldrev', 'newrev', 'ref', @user) }.should_not raise_error end end context "with web hooks" do before do - @webhook = Factory(:web_hook) - @webhook_2 = Factory(:web_hook) - project.web_hooks << [@webhook, @webhook_2] + @project_hook = Factory(:project_hook) + @project_hook_2 = Factory(:project_hook) + project.hooks << [@project_hook, @project_hook_2] end it "executes multiple web hook" do - @webhook.should_receive(:execute).once - @webhook_2.should_receive(:execute).once + @project_hook.should_receive(:execute).once + @project_hook_2.should_receive(:execute).once - project.execute_web_hooks('oldrev', 'newrev', 'refs/heads/master', @user) + project.execute_hooks('oldrev', 'newrev', 'refs/heads/master', @user) end end context "does not execute web hooks" do before do - @webhook = Factory(:web_hook) - project.web_hooks << [@webhook] + @project_hook = Factory(:project_hook) + project.hooks << [@project_hook] end it "when pushing a branch for the first time" do - @webhook.should_not_receive(:execute) - project.execute_web_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/master', @user) + @project_hook.should_not_receive(:execute) + project.execute_hooks('00000000000000000000000000000000', 'newrev', 'refs/heads/master', @user) end it "when pushing tags" do - @webhook.should_not_receive(:execute) - project.execute_web_hooks('oldrev', 'newrev', 'refs/tags/v1.0.0', @user) + @project_hook.should_not_receive(:execute) + project.execute_hooks('oldrev', 'newrev', 'refs/tags/v1.0.0', @user) end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 381fe7592c9..351f5748b4d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -11,7 +11,7 @@ describe Project do it { should have_many(:issues).dependent(:destroy) } it { should have_many(:notes).dependent(:destroy) } it { should have_many(:snippets).dependent(:destroy) } - it { should have_many(:web_hooks).dependent(:destroy) } + it { should have_many(:hooks).dependent(:destroy) } it { should have_many(:deploy_keys).dependent(:destroy) } end diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb index 9971bd5819d..885947614d7 100644 --- a/spec/models/web_hook_spec.rb +++ b/spec/models/web_hook_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe WebHook do +describe ProjectHook do describe "Associations" do it { should belong_to :project } end @@ -23,32 +23,32 @@ describe WebHook do describe "execute" do before(:each) do - @webhook = Factory :web_hook + @project_hook = Factory :project_hook @project = Factory :project - @project.web_hooks << [@webhook] + @project.hooks << [@project_hook] @data = { before: 'oldrev', after: 'newrev', ref: 'ref'} - WebMock.stub_request(:post, @webhook.url) + WebMock.stub_request(:post, @project_hook.url) end it "POSTs to the web hook URL" do - @webhook.execute(@data) - WebMock.should have_requested(:post, @webhook.url).once + @project_hook.execute(@data) + WebMock.should have_requested(:post, @project_hook.url).once end it "POSTs the data as JSON" do json = @data.to_json - @webhook.execute(@data) - WebMock.should have_requested(:post, @webhook.url).with(body: json).once + @project_hook.execute(@data) + WebMock.should have_requested(:post, @project_hook.url).with(body: json).once end it "catches exceptions" do WebHook.should_receive(:post).and_raise("Some HTTP Post error") lambda { - @webhook.execute(@data) - }.should_not raise_error + @project_hook.execute(@data) + }.should raise_error end end end diff --git a/spec/requests/admin/security_spec.rb b/spec/requests/admin/security_spec.rb index 0b0edb85a37..0c369740cff 100644 --- a/spec/requests/admin/security_spec.rb +++ b/spec/requests/admin/security_spec.rb @@ -13,9 +13,9 @@ describe "Admin::Projects" do it { admin_users_path.should be_denied_for :visitor } end - describe "GET /admin/emails" do - it { admin_emails_path.should be_allowed_for :admin } - it { admin_emails_path.should be_denied_for :user } - it { admin_emails_path.should be_denied_for :visitor } + describe "GET /admin/hooks" do + it { admin_hooks_path.should be_allowed_for :admin } + it { admin_hooks_path.should be_denied_for :user } + it { admin_hooks_path.should be_denied_for :visitor } end end diff --git a/spec/requests/hooks_spec.rb b/spec/requests/hooks_spec.rb index a508e5ea517..05432f13f3c 100644 --- a/spec/requests/hooks_spec.rb +++ b/spec/requests/hooks_spec.rb @@ -9,7 +9,7 @@ describe "Hooks" do describe "GET index" do it "should be available" do - @hook = Factory :web_hook, :project => @project + @hook = Factory :project_hook, :project => @project visit project_hooks_path(@project) page.should have_content "Hooks" page.should have_content @hook.url @@ -21,7 +21,7 @@ describe "Hooks" do @url = Faker::Internet.uri("http") visit project_hooks_path(@project) fill_in "hook_url", :with => @url - expect { click_button "Add Web Hook" }.to change(WebHook, :count).by(1) + expect { click_button "Add Web Hook" }.to change(ProjectHook, :count).by(1) end it "should open new team member popup" do @@ -32,7 +32,8 @@ describe "Hooks" do describe "Test" do before do - @hook = Factory :web_hook, :project => @project + @hook = Factory :project_hook, :project => @project + stub_request(:post, @hook.url) visit project_hooks_path(@project) click_link "Test Hook" end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index c70b2f6cebb..5f3b6d3daec 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -22,14 +22,14 @@ describe PostReceive do Key.stub(find_by_identifier: nil) project.should_not_receive(:observe_push) - project.should_not_receive(:execute_web_hooks) + project.should_not_receive(:execute_hooks) PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master', key_id).should be_false end it "asks the project to execute web hooks" do Project.stub(find_by_path: project) - project.should_receive(:execute_web_hooks).with('sha-old', 'sha-new', 'refs/heads/master', project.owner) + project.should_receive(:execute_hooks).with('sha-old', 'sha-new', 'refs/heads/master', project.owner) PostReceive.perform(project.path, 'sha-old', 'sha-new', 'refs/heads/master', key_id) end -- cgit v1.2.1 From 86bd11cbd8796adc31849aa02317604abb1576e2 Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Thu, 19 Jul 2012 00:24:37 +0300 Subject: System Hooks: rspec --- app/controllers/admin/hooks_controller.rb | 12 ++++-- spec/factories.rb | 10 +++++ spec/models/system_hook_spec.rb | 61 +++++++++++++++++++++++++++++++ spec/requests/admin/admin_hooks_spec.rb | 53 +++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 spec/models/system_hook_spec.rb create mode 100644 spec/requests/admin/admin_hooks_spec.rb diff --git a/app/controllers/admin/hooks_controller.rb b/app/controllers/admin/hooks_controller.rb index 446d4ea90a6..7f832fd5697 100644 --- a/app/controllers/admin/hooks_controller.rb +++ b/app/controllers/admin/hooks_controller.rb @@ -28,9 +28,15 @@ class Admin::HooksController < ApplicationController def test - @hook = @project.hooks.find(params[:id]) - commits = @project.commits(@project.default_branch, nil, 3) - data = @project.post_receive_data(commits.last.id, commits.first.id, "refs/heads/#{@project.default_branch}", current_user) + @hook = SystemHook.find(params[:hook_id]) + data = { + event_name: "project_create", + name: "Ruby", + path: "ruby", + project_id: 1, + owner_name: "Someone", + owner_email: "example@gitlabhq.com" + } @hook.execute(data) redirect_to :back diff --git a/spec/factories.rb b/spec/factories.rb index d6570551803..ab2ca4687da 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -7,6 +7,12 @@ Factory.add(:project, Project) do |obj| obj.code = 'LGT' end +Factory.add(:project_without_owner, Project) do |obj| + obj.name = Faker::Internet.user_name + obj.path = 'gitlabhq' + obj.code = 'LGT' +end + Factory.add(:public_project, Project) do |obj| obj.name = Faker::Internet.user_name obj.path = 'gitlabhq' @@ -64,6 +70,10 @@ Factory.add(:project_hook, ProjectHook) do |obj| obj.url = Faker::Internet.uri("http") end +Factory.add(:system_hook, SystemHook) do |obj| + obj.url = Faker::Internet.uri("http") +end + Factory.add(:wiki, Wiki) do |obj| obj.title = Faker::Lorem.sentence obj.content = Faker::Lorem.sentence diff --git a/spec/models/system_hook_spec.rb b/spec/models/system_hook_spec.rb new file mode 100644 index 00000000000..661ba6b85cb --- /dev/null +++ b/spec/models/system_hook_spec.rb @@ -0,0 +1,61 @@ +require "spec_helper" + +describe SystemHook do + describe "execute" do + before(:each) do + @system_hook = Factory :system_hook + WebMock.stub_request(:post, @system_hook.url) + end + + it "project_create hook" do + user = Factory :user + with_resque do + project = Factory :project_without_owner, :owner => user + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /project_create/).once + end + + it "project_destroy hook" do + project = Factory :project + with_resque do + project.destroy + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /project_destroy/).once + end + + it "user_create hook" do + with_resque do + Factory :user + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /user_create/).once + end + + it "user_destroy hook" do + user = Factory :user + with_resque do + user.destroy + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /user_destroy/).once + end + + it "project_create hook" do + user = Factory :user + project = Factory :project + with_resque do + project.users << user + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /user_add_to_team/).once + end + + it "project_destroy hook" do + user = Factory :user + project = Factory :project + project.users << user + with_resque do + project.users_projects.clear + end + WebMock.should have_requested(:post, @system_hook.url).with(body: /user_remove_from_team/).once + end + end + +end diff --git a/spec/requests/admin/admin_hooks_spec.rb b/spec/requests/admin/admin_hooks_spec.rb new file mode 100644 index 00000000000..e8a345b689f --- /dev/null +++ b/spec/requests/admin/admin_hooks_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe "Admin::Hooks" do + before do + @project = Factory :project, + :name => "LeGiT", + :code => "LGT" + login_as :admin + + @system_hook = Factory :system_hook + + end + + describe "GET /admin/hooks" do + it "should be ok" do + visit admin_root_path + within ".main_menu" do + click_on "Hooks" + end + current_path.should == admin_hooks_path + end + + it "should have hooks list" do + visit admin_hooks_path + page.should have_content(@system_hook.url) + end + end + + describe "New Hook" do + before do + @url = Faker::Internet.uri("http") + visit admin_hooks_path + fill_in "hook_url", :with => @url + expect { click_button "Add System Hook" }.to change(SystemHook, :count).by(1) + end + + it "should open new hook popup" do + page.current_path.should == admin_hooks_path + page.should have_content(@url) + end + end + + describe "Test" do + before do + WebMock.stub_request(:post, @system_hook.url) + visit admin_hooks_path + click_link "Test Hook" + end + + it { page.current_path.should == admin_hooks_path } + end + +end -- cgit v1.2.1 From d9cd6269e991e187c2acf272240fcb332270a5c4 Mon Sep 17 00:00:00 2001 From: Valeriy Sizov Date: Fri, 20 Jul 2012 01:01:29 +0300 Subject: System Hooks: move callback to observer --- app/models/project.rb | 26 -------------- app/models/user.rb | 20 +---------- app/models/users_project.rb | 28 --------------- app/observers/system_hook_observer.rb | 67 +++++++++++++++++++++++++++++++++++ config/application.rb | 2 +- 5 files changed, 69 insertions(+), 74 deletions(-) create mode 100644 app/observers/system_hook_observer.rb diff --git a/app/models/project.rb b/app/models/project.rb index d3dac34ae89..a49b3f519b3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -107,32 +107,6 @@ class Project < ActiveRecord::Base validate :check_limit validate :repo_name - after_create :create_hooks - after_destroy :destroy_hooks - - def create_hooks - SystemHook.all_hooks_fire({ - event_name: "project_create", - name: self.name, - path: self.path, - project_id: self.id, - owner_name: self.owner.name, - owner_email: self.owner.email, - created_at: self.created_at - }) - end - - def destroy_hooks - SystemHook.all_hooks_fire({ - event_name: "project_destroy", - name: self.name, - path: self.path, - project_id: self.id, - owner_name: self.owner.name, - owner_email: self.owner.email, - }) - end - def check_limit unless owner.can_create_project? errors[:base] << ("Your own projects limit is #{owner.projects_limit}! Please contact administrator to increase it") diff --git a/app/models/user.rb b/app/models/user.rb index 8eb114c4727..99d50ffcede 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,4 +1,5 @@ class User < ActiveRecord::Base + include Account devise :database_authenticatable, :token_authenticatable, :lockable, @@ -57,25 +58,6 @@ class User < ActiveRecord::Base scope :active, where(:blocked => false) before_validation :generate_password, :on => :create - after_create :create_hooks - after_destroy :destroy_hooks - - def create_hooks - SystemHook.all_hooks_fire({ - event_name: "user_create", - name: self.name, - email: self.email, - created_at: self.created_at - }) - end - - def destroy_hooks - SystemHook.all_hooks_fire({ - event_name: "user_destroy", - name: self.name, - email: self.email - }) - end def generate_password if self.force_random_password diff --git a/app/models/users_project.rb b/app/models/users_project.rb index 128e61b7682..4ff86290a92 100644 --- a/app/models/users_project.rb +++ b/app/models/users_project.rb @@ -11,9 +11,6 @@ class UsersProject < ActiveRecord::Base after_save :update_repository after_destroy :update_repository - after_create :add_to_team_hooks - after_destroy :remove_from_team_hooks - validates_uniqueness_of :user_id, :scope => [:project_id] validates_presence_of :user_id @@ -21,31 +18,6 @@ class UsersProject < ActiveRecord::Base delegate :name, :email, :to => :user, :prefix => true - def add_to_team_hooks - SystemHook.all_hooks_fire({ - event_name: "user_add_to_team", - project_name: self.project.name, - project_path: self.project.path, - project_id: self.project_id, - user_name: self.user.name, - user_email: self.user.email, - project_access: self.repo_access_human, - created_at: self.created_at - }) - end - - def remove_from_team_hooks - SystemHook.all_hooks_fire({ - event_name: "user_remove_from_team", - project_name: self.project.name, - project_path: self.project.path, - project_id: self.project_id, - user_name: self.user.name, - user_email: self.user.email, - project_access: self.repo_access_human - }) - end - def self.bulk_import(project, user_ids, project_access) UsersProject.transaction do user_ids.each do |user_id| diff --git a/app/observers/system_hook_observer.rb b/app/observers/system_hook_observer.rb new file mode 100644 index 00000000000..312cd2b3622 --- /dev/null +++ b/app/observers/system_hook_observer.rb @@ -0,0 +1,67 @@ +class SystemHookObserver < ActiveRecord::Observer + observe :user, :project, :users_project + + def after_create(model) + if model.kind_of? Project + SystemHook.all_hooks_fire({ + event_name: "project_create", + name: model.name, + path: model.path, + project_id: model.id, + owner_name: model.owner.name, + owner_email: model.owner.email, + created_at: model.created_at + }) + elsif model.kind_of? User + SystemHook.all_hooks_fire({ + event_name: "user_create", + name: model.name, + email: model.email, + created_at: model.created_at + }) + + elsif model.kind_of? UsersProject + SystemHook.all_hooks_fire({ + event_name: "user_add_to_team", + project_name: model.project.name, + project_path: model.project.path, + project_id: model.project_id, + user_name: model.user.name, + user_email: model.user.email, + project_access: model.repo_access_human, + created_at: model.created_at + }) + + end + end + + def after_destroy(model) + if model.kind_of? Project + SystemHook.all_hooks_fire({ + event_name: "project_destroy", + name: model.name, + path: model.path, + project_id: model.id, + owner_name: model.owner.name, + owner_email: model.owner.email, + }) + elsif model.kind_of? User + SystemHook.all_hooks_fire({ + event_name: "user_destroy", + name: model.name, + email: model.email + }) + + elsif model.kind_of? UsersProject + SystemHook.all_hooks_fire({ + event_name: "user_remove_from_team", + project_name: model.project.name, + project_path: model.project.path, + project_id: model.project_id, + user_name: model.user.name, + user_email: model.user.email, + project_access: model.repo_access_human + }) + end + end +end diff --git a/config/application.rb b/config/application.rb index 3585c4b0a87..937262237e9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,7 +23,7 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer + config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer, :system_hook_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. -- cgit v1.2.1 From 60ee383eb96285d1bc21e7f439306d5eb55048bd Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Fri, 20 Jul 2012 12:08:59 +0300 Subject: Enable observe for system hooks --- spec/models/system_hook_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/models/system_hook_spec.rb b/spec/models/system_hook_spec.rb index 661ba6b85cb..4ad4d1681fc 100644 --- a/spec/models/system_hook_spec.rb +++ b/spec/models/system_hook_spec.rb @@ -2,6 +2,8 @@ require "spec_helper" describe SystemHook do describe "execute" do + before(:each) { ActiveRecord::Base.observers.enable(:all) } + before(:each) do @system_hook = Factory :system_hook WebMock.stub_request(:post, @system_hook.url) -- cgit v1.2.1