Merge branch 'remove-thread-var' into 'master'

Remove thread variables Lets get rid of thread variables. It produces additional complexity and weird stack trace. Also part of !1133 - - - Also it fixes issue/merge_request close and reopen bug via API when no dashboard event and comment was created.

Merge branch 'remove-thread-var' into 'master'
Remove thread variables Lets get rid of thread variables. It produces additional complexity and weird stack trace. Also part of !1133 - - - Also it fixes issue/merge_request close and reopen bug via API when no dashboard event and comment was created.
Dmitriy Zaporozhets
2 parents f451a697 f6ee55aa
Showing 18 changed files with 109 additions and 107 deletions Show diff stats
app/controllers/application_controller.rb
app/controllers/projects/milestones_controller.rb
app/observers/base_observer.rb
app/observers/milestone_observer.rb
app/observers/note_observer.rb
app/services/issues/update_service.rb
app/services/merge_requests/update_service.rb
app/services/milestones/base_service.rb
app/services/milestones/close_service.rb
app/services/milestones/create_service.rb
app/services/milestones/reopen_service.rb
app/services/milestones/update_service.rb
config/application.rb
lib/api/helpers.rb
lib/api/merge_requests.rb
lib/api/milestones.rb
lib/api/notes.rb
lib/gitlab/seeder.rb
@@ -4,7 +4,6 @@ class ApplicationController &lt; 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 &lt; 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
@@ -37,7 +37,7 @@ class Projects::MilestonesController &lt; 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 &lt; 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
@@ -10,12 +10,4 @@ class BaseObserver &lt; 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
@@ -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
@@ -5,7 +5,7 @@ class NoteObserver &lt; 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 &lt; BaseObserver
   end
   def after_update(note)
-    note.notice_added_references(note.project, current_user)
+    note.notice_added_references(note.project, note.author)
   end
 end
 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'
@@ -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'
@@ -0,0 +1,4 @@
+module Milestones
+  class BaseService < ::BaseService
+  end
+end
@@ -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
@@ -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
@@ -0,0 +1,11 @@
+module Milestones
+  class ReopenService < Milestones::BaseService
+    def execute(milestone)
+      if milestone.activate
+        event_service.reopen_milestone(milestone, current_user)
+      end
+
+      milestone
+    end
+  end
+end
@@ -0,0 +1,20 @@
+module Milestones
+  class UpdateService < Milestones::BaseService
+    def execute(milestone)
+      state = params.delete('state_event') || params.delete(:state_event)
+
+      case state
+      when 'activate'
+        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
@@ -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,
@@ -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!
@@ -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
@@ -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
@@ -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
@@ -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
	@@ -37,7 +37,7 @@ class Projects::MilestonesController < Projects::ApplicationController		@@ -37,7 +37,7 @@ class Projects::MilestonesController < Projects::ApplicationController
37	end	37	end
38		38
39	def create	39	def create
40	- @milestone = @project.milestones.new(params[:milestone])	40	+ @milestone = Milestones::CreateService.new(project, current_user, params[:milestone]).execute
41		41
42	if @milestone.save	42	if @milestone.save
43	redirect_to project_milestone_path(@project, @milestone)	43	redirect_to project_milestone_path(@project, @milestone)
	@@ -47,7 +47,7 @@ class Projects::MilestonesController < Projects::ApplicationController		@@ -47,7 +47,7 @@ class Projects::MilestonesController < Projects::ApplicationController
47	end	47	end
48		48
49	def update	49	def update
50	- @milestone.update_attributes(params[:milestone])	50	+ @milestone = Milestones::UpdateService.new(project, current_user, params[:milestone]).execute(milestone)
51		51
52	respond_to do \|format\|	52	respond_to do \|format\|
53	format.js	53	format.js
	@@ -10,12 +10,4 @@ class BaseObserver < ActiveRecord::Observer		@@ -10,12 +10,4 @@ class BaseObserver < ActiveRecord::Observer
10	def log_info message	10	def log_info message
11	Gitlab::AppLogger.info message	11	Gitlab::AppLogger.info message
12	end	12	end
13	-
14	- def current_user
15	- Thread.current[:current_user]
16	- end
17	-
18	- def current_commit
19	- Thread.current[:current_commit]
20	- end
21	end	13	end
	@@ -1,13 +0,0 @@	@@ -1,13 +0,0 @@
1	-class MilestoneObserver < BaseObserver
2	- def after_create(milestone)
3	- event_service.open_milestone(milestone, current_user)
4	- end
5	-
6	- def after_close(milestone, transition)
7	- event_service.close_milestone(milestone, current_user)
8	- end
9	-
10	- def after_reopen(milestone, transition)
11	- event_service.reopen_milestone(milestone, current_user)
12	- end
13	-end
	@@ -5,7 +5,7 @@ class NoteObserver < BaseObserver		@@ -5,7 +5,7 @@ class NoteObserver < BaseObserver
5	# Skip system notes, like status changes and cross-references.	5	# Skip system notes, like status changes and cross-references.
6	# Skip wall notes to prevent spamming of dashboard	6	# Skip wall notes to prevent spamming of dashboard
7	if note.noteable_type.present? && !note.system	7	if note.noteable_type.present? && !note.system
8	- event_service.leave_note(note, current_user)	8	+ event_service.leave_note(note, note.author)
9	end	9	end
10		10
11	unless note.system?	11	unless note.system?
	@@ -18,6 +18,6 @@ class NoteObserver < BaseObserver		@@ -18,6 +18,6 @@ class NoteObserver < BaseObserver
18	end	18	end
19		19
20	def after_update(note)	20	def after_update(note)
21	- note.notice_added_references(note.project, current_user)	21	+ note.notice_added_references(note.project, note.author)
22	end	22	end
23	end	23	end
1	module Issues	1	module Issues
2	class UpdateService < Issues::BaseService	2	class UpdateService < Issues::BaseService
3	def execute(issue)	3	def execute(issue)
4	- state = params.delete('state_event')	4	+ state = params.delete('state_event') \|\| params.delete(:state_event)
5		5
6	case state	6	case state
7	when 'reopen'	7	when 'reopen'
	@@ -10,7 +10,7 @@ module MergeRequests		@@ -10,7 +10,7 @@ module MergeRequests
10	params.delete(:source_project_id)	10	params.delete(:source_project_id)
11	params.delete(:target_project_id)	11	params.delete(:target_project_id)
12		12
13	- state = params.delete('state_event')	13	+ state = params.delete('state_event') \|\| params.delete(:state_event)
14		14
15	case state	15	case state
16	when 'reopen'	16	when 'reopen'
@@ -0,0 +1,4 @@		@@ -0,0 +1,4 @@
	1	+module Milestones
	2	+ class BaseService < ::BaseService
	3	+ end
	4	+end
@@ -0,0 +1,11 @@		@@ -0,0 +1,11 @@
	1	+module Milestones
	2	+ class CloseService < Milestones::BaseService
	3	+ def execute(milestone)
	4	+ if milestone.close
	5	+ event_service.close_milestone(milestone, current_user)
	6	+ end
	7	+
	8	+ milestone
	9	+ end
	10	+ end
	11	+end
@@ -0,0 +1,13 @@		@@ -0,0 +1,13 @@
	1	+module Milestones
	2	+ class CreateService < Milestones::BaseService
	3	+ def execute
	4	+ milestone = project.milestones.new(params)
	5	+
	6	+ if milestone.save
	7	+ event_service.open_milestone(milestone, current_user)
	8	+ end
	9	+
	10	+ milestone
	11	+ end
	12	+ end
	13	+end
@@ -0,0 +1,20 @@		@@ -0,0 +1,20 @@
	1	+module Milestones
	2	+ class UpdateService < Milestones::BaseService
	3	+ def execute(milestone)
	4	+ state = params.delete('state_event') \|\| params.delete(:state_event)
	5	+
	6	+ case state
	7	+ when 'activate'
	8	+ Milestones::ReopenService.new(project, current_user, {}).execute(milestone)
	9	+ when 'close'
	10	+ Milestones::CloseService.new(project, current_user, {}).execute(milestone)
	11	+ end
	12	+
	13	+ if params.present?
	14	+ milestone.update_attributes(params)
	15	+ end
	16	+
	17	+ milestone
	18	+ end
	19	+ end
	20	+end