From cfd9fd30d60c5a880785acda27e9f3d55b17e4ef Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 13:38:35 +0300
Subject: [PATCH 1/9] Move code for issue creation to service.

The goal of suych refactoring is to get rid of observers.
Its much easier to test and code when object creation and all other
related actions done in one class instead of splited across observers,
callbacks etc.

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/controllers/projects/issues_controller.rb |  4 +---
 app/observers/issue_observer.rb               |  7 ------
 app/services/base_service.rb                  | 12 ++++++++++
 app/services/issues/create_service.rb         | 23 +++++++++++++++++++
 lib/api/issues.rb                             | 20 ++++++++--------
 spec/services/issues/create_service_spec.rb   | 22 ++++++++++++++++++
 6 files changed, 67 insertions(+), 21 deletions(-)
 create mode 100644 app/services/issues/create_service.rb
 create mode 100644 spec/services/issues/create_service_spec.rb

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index eef849d820..ca85ba6b25 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -59,9 +59,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def create
-    @issue = @project.issues.new(params[:issue])
-    @issue.author = current_user
-    @issue.save
+    @issue = Issues::CreateService.new(project, current_user, params[:issue]).execute
 
     respond_to do |format|
       format.html do
diff --git a/app/observers/issue_observer.rb b/app/observers/issue_observer.rb
index 30da1f83da..c2132ddca5 100644
--- a/app/observers/issue_observer.rb
+++ b/app/observers/issue_observer.rb
@@ -1,11 +1,4 @@
 class IssueObserver < BaseObserver
-  def after_create(issue)
-    notification.new_issue(issue, current_user)
-    event_service.open_issue(issue, current_user)
-    issue.create_cross_references!(issue.project, current_user)
-    execute_hooks(issue)
-  end
-
   def after_close(issue, transition)
     notification.close_issue(issue, current_user)
     event_service.close_issue(issue, current_user)
diff --git a/app/services/base_service.rb b/app/services/base_service.rb
index 610f047487..9ad8092315 100644
--- a/app/services/base_service.rb
+++ b/app/services/base_service.rb
@@ -16,4 +16,16 @@ class BaseService
   def can?(object, action, subject)
     abilities.allowed?(object, action, subject)
   end
+
+  def notification_service
+    NotificationService.new
+  end
+
+  def event_service
+    EventCreateService.new
+  end
+
+  def log_info message
+    Gitlab::AppLogger.info message
+  end
 end
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
new file mode 100644
index 0000000000..37f440fc40
--- /dev/null
+++ b/app/services/issues/create_service.rb
@@ -0,0 +1,23 @@
+module Issues
+  class CreateService < BaseService
+    def execute
+      issue = project.issues.new(params)
+      issue.author = current_user
+
+      if issue.save
+        notification_service.new_issue(issue, current_user)
+        event_service.open_issue(issue, current_user)
+        issue.create_cross_references!(issue.project, current_user)
+        execute_hooks(issue)
+      end
+
+      issue
+    end
+
+    private
+
+    def execute_hooks(issue)
+      issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)
+    end
+  end
+end
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 3d15c35b8c..169c58b007 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -48,17 +48,15 @@ module API
       # Example Request:
       #   POST /projects/:id/issues
       post ":id/issues" do
-        set_current_user_for_thread do
-          required_attributes! [:title]
-          attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
-          attrs[:label_list] = params[:labels] if params[:labels].present?
-          @issue = user_project.issues.new attrs
-          @issue.author = current_user
-          if @issue.save
-            present @issue, with: Entities::Issue
-          else
-            not_found!
-          end
+        required_attributes! [:title]
+        attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
+        attrs[:label_list] = params[:labels] if params[:labels].present?
+        issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute
+
+        if issue.valid?
+          present issue, with: Entities::Issue
+        else
+          not_found!
         end
       end
 
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
new file mode 100644
index 0000000000..7e2d5ad2e8
--- /dev/null
+++ b/spec/services/issues/create_service_spec.rb
@@ -0,0 +1,22 @@
+require 'spec_helper'
+
+describe Issues::CreateService do
+  let(:project) { create(:empty_project) }
+  let(:user) { create(:user) }
+
+  describe :execute do
+    context "valid params" do
+      before do
+        project.team << [user, :master]
+        opts = {
+          title: 'Awesome issue',
+          description: 'please fix'
+        }
+
+        @issue = Issues::CreateService.new(project, user, opts).execute
+      end
+
+      it { @issue.should be_valid }
+    end
+  end
+end

From c4e81ed9de6a5bbfe089e9b61ca0400167e489f3 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 13:54:41 +0300
Subject: [PATCH 2/9] Move update issue code to separate service

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/controllers/projects/issues_controller.rb |  3 +--
 app/observers/issue_observer.rb               | 10 --------
 app/services/issues/base_service.rb           | 19 +++++++++++++++
 app/services/issues/create_service.rb         |  6 -----
 app/services/issues/update_service.rb         | 19 +++++++++++++++
 lib/api/issues.rb                             | 20 ++++++++--------
 spec/services/issues/create_service_spec.rb   |  1 +
 spec/services/issues/update_service_spec.rb   | 24 +++++++++++++++++++
 8 files changed, 74 insertions(+), 28 deletions(-)
 create mode 100644 app/services/issues/base_service.rb
 create mode 100644 app/services/issues/update_service.rb
 create mode 100644 spec/services/issues/update_service_spec.rb

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index ca85ba6b25..4e7a716bfe 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -74,8 +74,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def update
-    @issue.update_attributes(params[:issue])
-    @issue.reset_events_cache
+    @issue = Issues::UpdateService.new(project, current_user, params[:issue]).execute(issue)
 
     respond_to do |format|
       format.js
diff --git a/app/observers/issue_observer.rb b/app/observers/issue_observer.rb
index c2132ddca5..b4880b12fd 100644
--- a/app/observers/issue_observer.rb
+++ b/app/observers/issue_observer.rb
@@ -12,16 +12,6 @@ class IssueObserver < BaseObserver
     execute_hooks(issue)
   end
 
-  def after_update(issue)
-    if issue.is_being_reassigned?
-      notification.reassigned_issue(issue, current_user)
-      create_assignee_note(issue)
-    end
-
-    issue.notice_added_references(issue.project, current_user)
-    execute_hooks(issue)
-  end
-
   protected
 
   # Create issue note with service comment like 'Status changed to closed'
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
new file mode 100644
index 0000000000..e04c1c6fb7
--- /dev/null
+++ b/app/services/issues/base_service.rb
@@ -0,0 +1,19 @@
+module Issues
+  class BaseService < ::BaseService
+
+    private
+
+    # Create issue note with service comment like 'Status changed to closed'
+    def create_note(issue)
+      Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
+    end
+
+    def create_assignee_note(issue)
+      Note.create_assignee_change_note(issue, issue.project, current_user, issue.assignee)
+    end
+
+    def execute_hooks(issue)
+      issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)
+    end
+  end
+end
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 37f440fc40..8008086dae 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -13,11 +13,5 @@ module Issues
 
       issue
     end
-
-    private
-
-    def execute_hooks(issue)
-      issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)
-    end
   end
 end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
new file mode 100644
index 0000000000..4db9dee11d
--- /dev/null
+++ b/app/services/issues/update_service.rb
@@ -0,0 +1,19 @@
+module Issues
+  class UpdateService < BaseService
+    def execute(issue)
+      if issue.update_attributes(params)
+        issue.reset_events_cache
+
+        if issue.is_being_reassigned?
+          notification.reassigned_issue(issue, current_user)
+          create_assignee_note(issue)
+        end
+
+        issue.notice_added_references(issue.project, current_user)
+        execute_hooks(issue)
+      end
+
+      issue
+    end
+  end
+end
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 169c58b007..f50be3a815 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -74,18 +74,18 @@ module API
       # Example Request:
       #   PUT /projects/:id/issues/:issue_id
       put ":id/issues/:issue_id" do
-        set_current_user_for_thread do
-          @issue = user_project.issues.find(params[:issue_id])
-          authorize! :modify_issue, @issue
+        issue = user_project.issues.find(params[:issue_id])
+        authorize! :modify_issue, issue
 
-          attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]
-          attrs[:label_list] = params[:labels] if params[:labels].present?
+        attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]
+        attrs[:label_list] = params[:labels] if params[:labels].present?
 
-          if @issue.update_attributes attrs
-            present @issue, with: Entities::Issue
-          else
-            not_found!
-          end
+        issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
+
+        if issue.valid?
+          present issue, with: Entities::Issue
+        else
+          not_found!
         end
       end
 
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 7e2d5ad2e8..90720be5de 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -17,6 +17,7 @@ describe Issues::CreateService do
       end
 
       it { @issue.should be_valid }
+      it { @issue.title.should == 'Awesome issue' }
     end
   end
 end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
new file mode 100644
index 0000000000..9bfc0f674d
--- /dev/null
+++ b/spec/services/issues/update_service_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe Issues::UpdateService do
+  let(:project) { create(:empty_project) }
+  let(:user) { create(:user) }
+  let(:issue) { create(:issue) }
+
+  describe :execute do
+    context "valid params" do
+      before do
+        project.team << [user, :master]
+        opts = {
+          title: 'New title',
+          description: 'Also please fix'
+        }
+
+        @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
+      end
+
+      it { @issue.should be_valid }
+      it { @issue.title.should == 'New title' }
+    end
+  end
+end

From 0d41f6f0a3ab23cee63e349eda5fb79240734dd4 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 15:37:57 +0300
Subject: [PATCH 3/9] Remove issue observer

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/observers/issue_observer.rb | 29 -----------------------------
 config/application.rb           |  1 -
 2 files changed, 30 deletions(-)
 delete mode 100644 app/observers/issue_observer.rb

diff --git a/app/observers/issue_observer.rb b/app/observers/issue_observer.rb
deleted file mode 100644
index b4880b12fd..0000000000
--- a/app/observers/issue_observer.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-class IssueObserver < BaseObserver
-  def after_close(issue, transition)
-    notification.close_issue(issue, current_user)
-    event_service.close_issue(issue, current_user)
-    create_note(issue)
-    execute_hooks(issue)
-  end
-
-  def after_reopen(issue, transition)
-    event_service.reopen_issue(issue, current_user)
-    create_note(issue)
-    execute_hooks(issue)
-  end
-
-  protected
-
-  # Create issue note with service comment like 'Status changed to closed'
-  def create_note(issue)
-    Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
-  end
-
-  def create_assignee_note(issue)
-    Note.create_assignee_change_note(issue, issue.project, current_user, issue.assignee)
-  end
-
-  def execute_hooks(issue)
-    issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)
-  end
-end
diff --git a/config/application.rb b/config/application.rb
index a782dd1d01..f7791b47b0 100644
--- a/config/application.rb
+++ b/config/application.rb
@@ -21,7 +21,6 @@ module Gitlab
     # Activate observers that should always be running.
     config.active_record.observers = :milestone_observer,
                                      :project_activity_cache_observer,
-                                     :issue_observer,
                                      :key_observer,
                                      :merge_request_observer,
                                      :note_observer,

From cc773654883d34c70462eeeeb280453473c35e21 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 15:38:24 +0300
Subject: [PATCH 4/9] Create services for issue close and reopen

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/services/git_push_service.rb      |  7 +++----
 app/services/issues/base_service.rb   |  5 -----
 app/services/issues/close_service.rb  | 20 ++++++++++++++++++++
 app/services/issues/reopen_service.rb | 19 +++++++++++++++++++
 app/services/issues/update_service.rb |  2 +-
 5 files changed, 43 insertions(+), 10 deletions(-)
 create mode 100644 app/services/issues/close_service.rb
 create mode 100644 app/services/issues/reopen_service.rb

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index fcc03c3e4b..351b446457 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -86,10 +86,9 @@ class GitPushService
       author = commit_user(commit)
 
       if !issues_to_close.empty? && is_default_branch
-        Thread.current[:current_user] = author
-        Thread.current[:current_commit] = commit
-
-        issues_to_close.each { |i| i.close && i.save }
+        issues_to_close.each do |issue|
+          Issues::CloseService.new(project, author, {}).execute(issue, commit)
+        end
       end
 
       # Create cross-reference notes for any other references. Omit any issues that were referenced in an
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index e04c1c6fb7..2e1e1f7e0f 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -3,11 +3,6 @@ module Issues
 
     private
 
-    # Create issue note with service comment like 'Status changed to closed'
-    def create_note(issue)
-      Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
-    end
-
     def create_assignee_note(issue)
       Note.create_assignee_change_note(issue, issue.project, current_user, issue.assignee)
     end
diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
new file mode 100644
index 0000000000..780bc8e413
--- /dev/null
+++ b/app/services/issues/close_service.rb
@@ -0,0 +1,20 @@
+module Issues
+  class CloseService < BaseService
+    def execute(issue, commit = nil)
+      if issue.close
+        notification_service.close_issue(issue, current_user)
+        event_service.close_issue(issue, current_user)
+        create_note(issue, commit)
+        execute_hooks(issue)
+      end
+
+      issue
+    end
+
+    private
+
+    def create_note(issue, current_commit)
+      Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
+    end
+  end
+end
diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
new file mode 100644
index 0000000000..743a5d6c4a
--- /dev/null
+++ b/app/services/issues/reopen_service.rb
@@ -0,0 +1,19 @@
+module Issues
+  class ReopenService < BaseService
+    def execute(issue)
+      if issue.reopen
+        event_service.reopen_issue(issue, current_user)
+        create_note(issue, commit)
+        execute_hooks(issue)
+      end
+
+      issue
+    end
+
+    private
+
+    def create_note(issue)
+      Note.create_status_change_note(issue, issue.project, current_user, issue.state, nil)
+    end
+  end
+end
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 4db9dee11d..4d8d71b4c9 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -5,7 +5,7 @@ module Issues
         issue.reset_events_cache
 
         if issue.is_being_reassigned?
-          notification.reassigned_issue(issue, current_user)
+          notification_service.reassigned_issue(issue, current_user)
           create_assignee_note(issue)
         end
 

From ed67ba966336c71583229063a12553f432643d64 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 15:50:50 +0300
Subject: [PATCH 5/9] Add support for close/reopen actions in update service

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/services/issues/reopen_service.rb |  2 +-
 app/services/issues/update_service.rb | 11 ++++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
index 743a5d6c4a..e1e5a97229 100644
--- a/app/services/issues/reopen_service.rb
+++ b/app/services/issues/reopen_service.rb
@@ -3,7 +3,7 @@ module Issues
     def execute(issue)
       if issue.reopen
         event_service.reopen_issue(issue, current_user)
-        create_note(issue, commit)
+        create_note(issue)
         execute_hooks(issue)
       end
 
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 4d8d71b4c9..9e73953bdc 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -1,7 +1,16 @@
 module Issues
   class UpdateService < BaseService
     def execute(issue)
-      if issue.update_attributes(params)
+      state = params.delete('state_event')
+
+      case state
+      when 'reopen'
+        Issues::ReopenService.new(project, current_user, {}).execute(issue)
+      when 'close'
+        Issues::CloseService.new(project, current_user, {}).execute(issue)
+      end
+
+      if params.present? && issue.update_attributes(params)
         issue.reset_events_cache
 
         if issue.is_being_reassigned?

From 7d8d9bd1a16e8cf1d87bc7e5115d6c1e7eb14c75 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 15:51:37 +0300
Subject: [PATCH 6/9] Remove issue observer tests

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 spec/observers/issue_observer_spec.rb | 99 ---------------------------
 1 file changed, 99 deletions(-)
 delete mode 100644 spec/observers/issue_observer_spec.rb

diff --git a/spec/observers/issue_observer_spec.rb b/spec/observers/issue_observer_spec.rb
deleted file mode 100644
index 9a0a2c4329..0000000000
--- a/spec/observers/issue_observer_spec.rb
+++ /dev/null
@@ -1,99 +0,0 @@
-require 'spec_helper'
-
-describe IssueObserver do
-  let(:some_user)      { create :user }
-  let(:assignee)       { create :user }
-  let(:author)         { create :user }
-  let(:mock_issue)     { create(:issue, assignee: assignee, author: author) }
-
-
-  before { subject.stub(:current_user).and_return(some_user) }
-  before { subject.stub(:current_commit).and_return(nil) }
-  before { subject.stub(notification: double('NotificationService').as_null_object) }
-  before { mock_issue.project.stub_chain(:repository, :commit).and_return(nil) }
-
-  subject { IssueObserver.instance }
-
-  describe '#after_create' do
-    it 'trigger notification to send emails' do
-      subject.should_receive(:notification)
-
-      subject.after_create(mock_issue)
-    end
-
-    it 'should create cross-reference notes' do
-      other_issue = create(:issue)
-      mock_issue.stub(references: [other_issue])
-
-      Note.should_receive(:create_cross_reference_note).with(other_issue, mock_issue,
-        some_user, mock_issue.project)
-      subject.after_create(mock_issue)
-    end
-  end
-
-  context '#after_close' do
-    context 'a status "closed"' do
-      before { mock_issue.stub(state: 'closed') }
-
-      it 'note is created if the issue is being closed' do
-        Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', nil)
-
-        subject.after_close(mock_issue, nil)
-      end
-
-      it 'trigger notification to send emails' do
-        subject.notification.should_receive(:close_issue).with(mock_issue, some_user)
-        subject.after_close(mock_issue, nil)
-      end
-
-      it 'appends a mention to the closing commit if one is present' do
-        commit = double('commit', gfm_reference: 'commit 123456')
-        subject.stub(current_commit: commit)
-
-        Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', commit)
-
-        subject.after_close(mock_issue, nil)
-      end
-    end
-
-    context 'a status "reopened"' do
-      before { mock_issue.stub(state: 'reopened') }
-
-      it 'note is created if the issue is being reopened' do
-        Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'reopened', nil)
-
-        subject.after_reopen(mock_issue, nil)
-      end
-    end
-  end
-
-  context '#after_update' do
-    before(:each) do
-      mock_issue.stub(:is_being_reassigned?).and_return(false)
-    end
-
-    context 'notification' do
-      it 'triggered if the issue is being reassigned' do
-        mock_issue.should_receive(:is_being_reassigned?).and_return(true)
-        subject.should_receive(:notification)
-
-        subject.after_update(mock_issue)
-      end
-
-      it 'is not triggered if the issue is not being reassigned' do
-        mock_issue.should_receive(:is_being_reassigned?).and_return(false)
-        subject.should_not_receive(:notification)
-
-        subject.after_update(mock_issue)
-      end
-    end
-
-    context 'cross-references' do
-      it 'notices added references' do
-        mock_issue.should_receive(:notice_added_references)
-
-        subject.after_update(mock_issue)
-      end
-    end
-  end
-end

From 1b5fb4ac2187c0a9cd775686740615a9e2311646 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 16:08:49 +0300
Subject: [PATCH 7/9] Fix assignee change in Issues::UpdateService

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/services/issues/update_service.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 9e73953bdc..06c38d8074 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -13,7 +13,7 @@ module Issues
       if params.present? && issue.update_attributes(params)
         issue.reset_events_cache
 
-        if issue.is_being_reassigned?
+        if issue.previous_changes.include?('assignee_id')
           notification_service.reassigned_issue(issue, current_user)
           create_assignee_note(issue)
         end

From 928fbeeec057692d923146994c4b8dff57024417 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 16:33:07 +0300
Subject: [PATCH 8/9] More tests for Isses services

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 spec/services/issues/close_service_spec.rb  | 35 +++++++++++++++++++++
 spec/services/issues/update_service_spec.rb | 24 ++++++++++++--
 2 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 spec/services/issues/close_service_spec.rb

diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
new file mode 100644
index 0000000000..d4f2cc1339
--- /dev/null
+++ b/spec/services/issues/close_service_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Issues::CloseService do
+  let(:project) { create(:empty_project) }
+  let(:user) { create(:user) }
+  let(:user2) { create(:user) }
+  let(:issue) { create(:issue, assignee: user2) }
+
+  before do
+    project.team << [user, :master]
+    project.team << [user2, :developer]
+  end
+
+  describe :execute do
+    context "valid params" do
+      before do
+        @issue = Issues::CloseService.new(project, user, {}).execute(issue)
+      end
+
+      it { @issue.should be_valid }
+      it { @issue.should be_closed }
+
+      it 'should send email to user2 about assign of new issue' do
+        email = ActionMailer::Base.deliveries.last
+        email.to.first.should == user2.email
+        email.subject.should include(issue.title)
+      end
+
+      it 'should create system note about issue reassign' do
+        note = @issue.notes.last
+        note.note.should include "Status changed to closed"
+      end
+    end
+  end
+end
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 9bfc0f674d..347560414e 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -3,15 +3,22 @@ require 'spec_helper'
 describe Issues::UpdateService do
   let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
+  let(:user2) { create(:user) }
   let(:issue) { create(:issue) }
 
+  before do
+    project.team << [user, :master]
+    project.team << [user2, :developer]
+  end
+
   describe :execute do
     context "valid params" do
       before do
-        project.team << [user, :master]
         opts = {
           title: 'New title',
-          description: 'Also please fix'
+          description: 'Also please fix',
+          assignee_id: user2.id,
+          state_event: 'close'
         }
 
         @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
@@ -19,6 +26,19 @@ describe Issues::UpdateService do
 
       it { @issue.should be_valid }
       it { @issue.title.should == 'New title' }
+      it { @issue.assignee.should == user2 }
+      it { @issue.should be_closed }
+
+      it 'should send email to user2 about assign of new issue' do
+        email = ActionMailer::Base.deliveries.last
+        email.to.first.should == user2.email
+        email.subject.should include(issue.title)
+      end
+
+      it 'should create system note about issue reassign' do
+        note = @issue.notes.last
+        note.note.should include "Reassigned to \@#{user2.username}"
+      end
     end
   end
 end

From 49f977d675b44b6aae0f491fbbf86dd1ee05eb64 Mon Sep 17 00:00:00 2001
From: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
Date: Wed, 2 Apr 2014 19:55:23 +0300
Subject: [PATCH 9/9] Fix tests

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
---
 app/services/issues/close_service.rb   | 2 +-
 app/services/issues/create_service.rb  | 2 +-
 app/services/issues/reopen_service.rb  | 2 +-
 app/services/issues/update_service.rb  | 2 +-
 spec/services/git_push_service_spec.rb | 8 +-------
 5 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
index 780bc8e413..85c0226cca 100644
--- a/app/services/issues/close_service.rb
+++ b/app/services/issues/close_service.rb
@@ -1,5 +1,5 @@
 module Issues
-  class CloseService < BaseService
+  class CloseService < Issues::BaseService
     def execute(issue, commit = nil)
       if issue.close
         notification_service.close_issue(issue, current_user)
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 8008086dae..d6137833bb 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -1,5 +1,5 @@
 module Issues
-  class CreateService < BaseService
+  class CreateService < Issues::BaseService
     def execute
       issue = project.issues.new(params)
       issue.author = current_user
diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
index e1e5a97229..a931398aff 100644
--- a/app/services/issues/reopen_service.rb
+++ b/app/services/issues/reopen_service.rb
@@ -1,5 +1,5 @@
 module Issues
-  class ReopenService < BaseService
+  class ReopenService < Issues::BaseService
     def execute(issue)
       if issue.reopen
         event_service.reopen_issue(issue, current_user)
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 06c38d8074..b562c401fd 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -1,5 +1,5 @@
 module Issues
-  class UpdateService < BaseService
+  class UpdateService < Issues::BaseService
     def execute(issue)
       state = params.delete('state_event')
 
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 90738c681f..6b89f213be 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -170,16 +170,10 @@ describe GitPushService do
       Issue.find(issue.id).should be_closed
     end
 
-    it "passes the closing commit as a thread-local" do
-      service.execute(project, user, @oldrev, @newrev, @ref)
-
-      Thread.current[:current_commit].should == closing_commit
-    end
-
     it "doesn't create cross-reference notes for a closing reference" do
       expect {
         service.execute(project, user, @oldrev, @newrev, @ref)
-      }.not_to change { Note.where(project_id: project.id, system: true).count }
+      }.not_to change { Note.where(project_id: project.id, system: true, commit_id: closing_commit.id).count }
     end
 
     it "doesn't close issues when pushed to non-default branches" do