From a7be3dfa30a452b307d1fdc0b4157ecbe908da8d Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 10 Jun 2014 17:51:49 +0300 Subject: [PATCH 1/5] Remove set of thread variables Signed-off-by: Dmitriy Zaporozhets --- app/controllers/application_controller.rb | 10 ---------- app/observers/base_observer.rb | 8 -------- lib/api/helpers.rb | 10 ---------- lib/gitlab/seeder.rb | 7 +------ 4 files changed, 1 insertion(+), 34 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index aa532de7aa..685d41a552 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -4,7 +4,6 @@ class ApplicationController < ActionController::Base before_filter :authenticate_user! before_filter :reject_blocked! before_filter :check_password_expiration - around_filter :set_current_user_for_thread before_filter :add_abilities before_filter :ldap_security_check before_filter :dev_tools if Rails.env == 'development' @@ -53,15 +52,6 @@ class ApplicationController < ActionController::Base end end - def set_current_user_for_thread - Thread.current[:current_user] = current_user - begin - yield - ensure - Thread.current[:current_user] = nil - end - end - def abilities @abilities ||= Six.new end diff --git a/app/observers/base_observer.rb b/app/observers/base_observer.rb index d685bd5d81..260d1f05db 100644 --- a/app/observers/base_observer.rb +++ b/app/observers/base_observer.rb @@ -10,12 +10,4 @@ class BaseObserver < ActiveRecord::Observer def log_info message Gitlab::AppLogger.info message end - - def current_user - Thread.current[:current_user] - end - - def current_commit - Thread.current[:current_commit] - end end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 654c1f62c6..b6a5806d64 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -36,16 +36,6 @@ module API end end - def set_current_user_for_thread - Thread.current[:current_user] = current_user - - begin - yield - ensure - Thread.current[:current_user] = nil - end - end - def user_project @project ||= find_project(params[:id]) @project || not_found! diff --git a/lib/gitlab/seeder.rb b/lib/gitlab/seeder.rb index 39de1223b1..31aa3528c4 100644 --- a/lib/gitlab/seeder.rb +++ b/lib/gitlab/seeder.rb @@ -9,12 +9,7 @@ module Gitlab end def self.by_user(user) - begin - Thread.current[:current_user] = user - yield - ensure - Thread.current[:current_user] = nil - end + yield end def self.mute_mailer From f8ea52c3a0fe29daf76fbd7a0e65399c09c95f5a Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 10 Jun 2014 17:56:35 +0300 Subject: [PATCH 2/5] Remove thread vars usage from API notes and mr's Signed-off-by: Dmitriy Zaporozhets --- app/observers/note_observer.rb | 4 ++-- lib/api/merge_requests.rb | 19 +++++++--------- lib/api/notes.rb | 40 +++++++++++++++------------------- 3 files changed, 28 insertions(+), 35 deletions(-) diff --git a/app/observers/note_observer.rb b/app/observers/note_observer.rb index 337bb1dc5a..9bb1f0f7b9 100644 --- a/app/observers/note_observer.rb +++ b/app/observers/note_observer.rb @@ -5,7 +5,7 @@ class NoteObserver < BaseObserver # Skip system notes, like status changes and cross-references. # Skip wall notes to prevent spamming of dashboard if note.noteable_type.present? && !note.system - event_service.leave_note(note, current_user) + event_service.leave_note(note, note.author) end unless note.system? @@ -18,6 +18,6 @@ class NoteObserver < BaseObserver end def after_update(note) - note.notice_added_references(note.project, current_user) + note.notice_added_references(note.project, note.author) end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 017cb1f562..fc1f1254a9 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -184,21 +184,18 @@ module API # POST /projects/:id/merge_request/:merge_request_id/comments # post ":id/merge_request/:merge_request_id/comments" do - set_current_user_for_thread do - required_attributes! [:note] + required_attributes! [:note] - merge_request = user_project.merge_requests.find(params[:merge_request_id]) - note = merge_request.notes.new(note: params[:note], project_id: user_project.id) - note.author = current_user + merge_request = user_project.merge_requests.find(params[:merge_request_id]) + note = merge_request.notes.new(note: params[:note], project_id: user_project.id) + note.author = current_user - if note.save - present note, with: Entities::MRNote - else - not_found! - end + if note.save + present note, with: Entities::MRNote + else + not_found! end end - end end end diff --git a/lib/api/notes.rb b/lib/api/notes.rb index f21907b1ff..cb2bc76447 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -41,19 +41,17 @@ module API # Example Request: # POST /projects/:id/notes post ":id/notes" do - set_current_user_for_thread do - required_attributes! [:body] + required_attributes! [:body] - @note = user_project.notes.new(note: params[:body]) - @note.author = current_user + @note = user_project.notes.new(note: params[:body]) + @note.author = current_user - if @note.save - present @note, with: Entities::Note - else - # :note is exposed as :body, but :note is set on error - bad_request!(:note) if @note.errors[:note].any? - not_found! - end + if @note.save + present @note, with: Entities::Note + else + # :note is exposed as :body, but :note is set on error + bad_request!(:note) if @note.errors[:note].any? + not_found! end end @@ -99,19 +97,17 @@ module API # POST /projects/:id/issues/:noteable_id/notes # POST /projects/:id/snippets/:noteable_id/notes post ":id/#{noteables_str}/:#{noteable_id_str}/notes" do - set_current_user_for_thread do - required_attributes! [:body] + required_attributes! [:body] - @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) - @note = @noteable.notes.new(note: params[:body]) - @note.author = current_user - @note.project = user_project + @noteable = user_project.send(:"#{noteables_str}").find(params[:"#{noteable_id_str}"]) + @note = @noteable.notes.new(note: params[:body]) + @note.author = current_user + @note.project = user_project - if @note.save - present @note, with: Entities::Note - else - not_found! - end + if @note.save + present @note, with: Entities::Note + else + not_found! end end end From c4b02642d2ca74f463e64dd591796aabe5c54af9 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 10 Jun 2014 18:07:52 +0300 Subject: [PATCH 3/5] Replace milestone observer with services Signed-off-by: Dmitriy Zaporozhets --- .../projects/milestones_controller.rb | 4 +-- app/observers/milestone_observer.rb | 13 ------- app/services/milestones/base_service.rb | 4 +++ app/services/milestones/close_service.rb | 11 ++++++ app/services/milestones/create_service.rb | 13 +++++++ app/services/milestones/reopen_service.rb | 11 ++++++ app/services/milestones/update_service.rb | 20 +++++++++++ config/application.rb | 3 +- lib/api/milestones.rb | 35 +++++++++---------- 9 files changed, 78 insertions(+), 36 deletions(-) delete mode 100644 app/observers/milestone_observer.rb create mode 100644 app/services/milestones/base_service.rb create mode 100644 app/services/milestones/close_service.rb create mode 100644 app/services/milestones/create_service.rb create mode 100644 app/services/milestones/reopen_service.rb create mode 100644 app/services/milestones/update_service.rb diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 05237bbd2d..227cc1dfba 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -37,7 +37,7 @@ class Projects::MilestonesController < Projects::ApplicationController end def create - @milestone = @project.milestones.new(params[:milestone]) + @milestone = Milestones::CreateService.new(project, current_user, params[:milestone]).execute if @milestone.save redirect_to project_milestone_path(@project, @milestone) @@ -47,7 +47,7 @@ class Projects::MilestonesController < Projects::ApplicationController end def update - @milestone.update_attributes(params[:milestone]) + @milestone = Milestones::UpdateService.new(project, current_user, params[:milestone]).execute(milestone) respond_to do |format| format.js diff --git a/app/observers/milestone_observer.rb b/app/observers/milestone_observer.rb deleted file mode 100644 index a1a62a99a8..0000000000 --- a/app/observers/milestone_observer.rb +++ /dev/null @@ -1,13 +0,0 @@ -class MilestoneObserver < BaseObserver - def after_create(milestone) - event_service.open_milestone(milestone, current_user) - end - - def after_close(milestone, transition) - event_service.close_milestone(milestone, current_user) - end - - def after_reopen(milestone, transition) - event_service.reopen_milestone(milestone, current_user) - end -end diff --git a/app/services/milestones/base_service.rb b/app/services/milestones/base_service.rb new file mode 100644 index 0000000000..176ab9f1ab --- /dev/null +++ b/app/services/milestones/base_service.rb @@ -0,0 +1,4 @@ +module Milestones + class BaseService < ::BaseService + end +end diff --git a/app/services/milestones/close_service.rb b/app/services/milestones/close_service.rb new file mode 100644 index 0000000000..608fc49d76 --- /dev/null +++ b/app/services/milestones/close_service.rb @@ -0,0 +1,11 @@ +module Milestones + class CloseService < Milestones::BaseService + def execute(milestone) + if milestone.close + event_service.close_milestone(milestone, current_user) + end + + milestone + end + end +end diff --git a/app/services/milestones/create_service.rb b/app/services/milestones/create_service.rb new file mode 100644 index 0000000000..b8e08c9f1e --- /dev/null +++ b/app/services/milestones/create_service.rb @@ -0,0 +1,13 @@ +module Milestones + class CreateService < Milestones::BaseService + def execute + milestone = project.milestones.new(params) + + if milestone.save + event_service.open_milestone(milestone, current_user) + end + + milestone + end + end +end diff --git a/app/services/milestones/reopen_service.rb b/app/services/milestones/reopen_service.rb new file mode 100644 index 0000000000..ff1ba23bdb --- /dev/null +++ b/app/services/milestones/reopen_service.rb @@ -0,0 +1,11 @@ +module Milestones + class ReopenService < Milestones::BaseService + def execute(milestone) + if milestone.reopen + event_service.reopen_milestone(milestone, current_user) + end + + milestone + end + end +end diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb new file mode 100644 index 0000000000..69254a7967 --- /dev/null +++ b/app/services/milestones/update_service.rb @@ -0,0 +1,20 @@ +module Milestones + class UpdateService < Milestones::BaseService + def execute(milestone) + state = params.delete('state_event') + + case state + when 'reopen' + Milestones::ReopenService.new(project, current_user, {}).execute(milestone) + when 'close' + Milestones::CloseService.new(project, current_user, {}).execute(milestone) + end + + if params.present? + milestone.update_attributes(params) + end + + milestone + end + end +end diff --git a/config/application.rb b/config/application.rb index f087d3507b..540426b667 100644 --- a/config/application.rb +++ b/config/application.rb @@ -19,8 +19,7 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :milestone_observer, - :project_activity_cache_observer, + config.active_record.observers = :project_activity_cache_observer, :note_observer, :project_observer, :system_hook_observer, diff --git a/lib/api/milestones.rb b/lib/api/milestones.rb index f7e63b2309..a4fdb752d6 100644 --- a/lib/api/milestones.rb +++ b/lib/api/milestones.rb @@ -40,17 +40,15 @@ module API # Example Request: # POST /projects/:id/milestones post ":id/milestones" do - set_current_user_for_thread do - authorize! :admin_milestone, user_project - required_attributes! [:title] + authorize! :admin_milestone, user_project + required_attributes! [:title] + attrs = attributes_for_keys [:title, :description, :due_date] + milestone = ::Milestones::CreateService.new(user_project, current_user, attrs).execute - attrs = attributes_for_keys [:title, :description, :due_date] - @milestone = user_project.milestones.new attrs - if @milestone.save - present @milestone, with: Entities::Milestone - else - not_found! - end + if milestone.valid? + present milestone, with: Entities::Milestone + else + not_found! end end @@ -66,16 +64,15 @@ module API # Example Request: # PUT /projects/:id/milestones/:milestone_id put ":id/milestones/:milestone_id" do - set_current_user_for_thread do - authorize! :admin_milestone, user_project + authorize! :admin_milestone, user_project + attrs = attributes_for_keys [:title, :description, :due_date, :state_event] + milestone = user_project.milestones.find(params[:milestone_id]) + milestone = ::Milestones::UpdateService.new(user_project, current_user, attrs).execute(milestone) - @milestone = user_project.milestones.find(params[:milestone_id]) - attrs = attributes_for_keys [:title, :description, :due_date, :state_event] - if @milestone.update_attributes attrs - present @milestone, with: Entities::Milestone - else - not_found! - end + if milestone.valid? + present milestone, with: Entities::Milestone + else + not_found! end end end From d65bf4605e561b58d3bb8ddba0ffb335e3a4c768 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 10 Jun 2014 19:22:39 +0300 Subject: [PATCH 4/5] Fix milestone reopen Signed-off-by: Dmitriy Zaporozhets --- app/services/milestones/reopen_service.rb | 2 +- app/services/milestones/update_service.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/milestones/reopen_service.rb b/app/services/milestones/reopen_service.rb index ff1ba23bdb..573f9ee5c2 100644 --- a/app/services/milestones/reopen_service.rb +++ b/app/services/milestones/reopen_service.rb @@ -1,7 +1,7 @@ module Milestones class ReopenService < Milestones::BaseService def execute(milestone) - if milestone.reopen + if milestone.activate event_service.reopen_milestone(milestone, current_user) end diff --git a/app/services/milestones/update_service.rb b/app/services/milestones/update_service.rb index 69254a7967..307e96a2b3 100644 --- a/app/services/milestones/update_service.rb +++ b/app/services/milestones/update_service.rb @@ -1,10 +1,10 @@ module Milestones class UpdateService < Milestones::BaseService def execute(milestone) - state = params.delete('state_event') + state = params.delete('state_event') || params.delete(:state_event) case state - when 'reopen' + when 'activate' Milestones::ReopenService.new(project, current_user, {}).execute(milestone) when 'close' Milestones::CloseService.new(project, current_user, {}).execute(milestone) From f6ee55aabd31c624d5bca71bca5c6ed152d844d0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 10 Jun 2014 19:23:02 +0300 Subject: [PATCH 5/5] Fix issue/mr close/reopen via API Signed-off-by: Dmitriy Zaporozhets --- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index f69370bedd..b4a742b003 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -1,7 +1,7 @@ module Issues class UpdateService < Issues::BaseService def execute(issue) - state = params.delete('state_event') + state = params.delete('state_event') || params.delete(:state_event) case state when 'reopen' diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 60d470a060..f1aa8b7393 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -10,7 +10,7 @@ module MergeRequests params.delete(:source_project_id) params.delete(:target_project_id) - state = params.delete('state_event') + state = params.delete('state_event') || params.delete(:state_event) case state when 'reopen'