Commit 3faa14e7a748097528ed36cf02ccacd62cd8f044

Authored by Dmitriy Zaporozhets
2 parents a23d0ef9 49f977d6

Merge branch 'reduce-observers' into 'master'

Move issue create/update code to services

Reduce observers role in GitLab code
app/controllers/projects/issues_controller.rb
@@ -59,9 +59,7 @@ class Projects::IssuesController < Projects::ApplicationController @@ -59,9 +59,7 @@ class Projects::IssuesController < Projects::ApplicationController
59 end 59 end
60 60
61 def create 61 def create
62 - @issue = @project.issues.new(params[:issue])  
63 - @issue.author = current_user  
64 - @issue.save 62 + @issue = Issues::CreateService.new(project, current_user, params[:issue]).execute
65 63
66 respond_to do |format| 64 respond_to do |format|
67 format.html do 65 format.html do
@@ -76,8 +74,7 @@ class Projects::IssuesController < Projects::ApplicationController @@ -76,8 +74,7 @@ class Projects::IssuesController < Projects::ApplicationController
76 end 74 end
77 75
78 def update 76 def update
79 - @issue.update_attributes(params[:issue])  
80 - @issue.reset_events_cache 77 + @issue = Issues::UpdateService.new(project, current_user, params[:issue]).execute(issue)
81 78
82 respond_to do |format| 79 respond_to do |format|
83 format.js 80 format.js
app/observers/issue_observer.rb
@@ -1,46 +0,0 @@ @@ -1,46 +0,0 @@
1 -class IssueObserver < BaseObserver  
2 - def after_create(issue)  
3 - notification.new_issue(issue, current_user)  
4 - event_service.open_issue(issue, current_user)  
5 - issue.create_cross_references!(issue.project, current_user)  
6 - execute_hooks(issue)  
7 - end  
8 -  
9 - def after_close(issue, transition)  
10 - notification.close_issue(issue, current_user)  
11 - event_service.close_issue(issue, current_user)  
12 - create_note(issue)  
13 - execute_hooks(issue)  
14 - end  
15 -  
16 - def after_reopen(issue, transition)  
17 - event_service.reopen_issue(issue, current_user)  
18 - create_note(issue)  
19 - execute_hooks(issue)  
20 - end  
21 -  
22 - def after_update(issue)  
23 - if issue.is_being_reassigned?  
24 - notification.reassigned_issue(issue, current_user)  
25 - create_assignee_note(issue)  
26 - end  
27 -  
28 - issue.notice_added_references(issue.project, current_user)  
29 - execute_hooks(issue)  
30 - end  
31 -  
32 - protected  
33 -  
34 - # Create issue note with service comment like 'Status changed to closed'  
35 - def create_note(issue)  
36 - Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)  
37 - end  
38 -  
39 - def create_assignee_note(issue)  
40 - Note.create_assignee_change_note(issue, issue.project, current_user, issue.assignee)  
41 - end  
42 -  
43 - def execute_hooks(issue)  
44 - issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)  
45 - end  
46 -end  
app/services/base_service.rb
@@ -16,4 +16,16 @@ class BaseService @@ -16,4 +16,16 @@ class BaseService
16 def can?(object, action, subject) 16 def can?(object, action, subject)
17 abilities.allowed?(object, action, subject) 17 abilities.allowed?(object, action, subject)
18 end 18 end
  19 +
  20 + def notification_service
  21 + NotificationService.new
  22 + end
  23 +
  24 + def event_service
  25 + EventCreateService.new
  26 + end
  27 +
  28 + def log_info message
  29 + Gitlab::AppLogger.info message
  30 + end
19 end 31 end
app/services/git_push_service.rb
@@ -86,10 +86,9 @@ class GitPushService @@ -86,10 +86,9 @@ class GitPushService
86 author = commit_user(commit) 86 author = commit_user(commit)
87 87
88 if !issues_to_close.empty? && is_default_branch 88 if !issues_to_close.empty? && is_default_branch
89 - Thread.current[:current_user] = author  
90 - Thread.current[:current_commit] = commit  
91 -  
92 - issues_to_close.each { |i| i.close && i.save } 89 + issues_to_close.each do |issue|
  90 + Issues::CloseService.new(project, author, {}).execute(issue, commit)
  91 + end
93 end 92 end
94 93
95 # Create cross-reference notes for any other references. Omit any issues that were referenced in an 94 # Create cross-reference notes for any other references. Omit any issues that were referenced in an
app/services/issues/base_service.rb 0 → 100644
@@ -0,0 +1,14 @@ @@ -0,0 +1,14 @@
  1 +module Issues
  2 + class BaseService < ::BaseService
  3 +
  4 + private
  5 +
  6 + def create_assignee_note(issue)
  7 + Note.create_assignee_change_note(issue, issue.project, current_user, issue.assignee)
  8 + end
  9 +
  10 + def execute_hooks(issue)
  11 + issue.project.execute_hooks(issue.to_hook_data, :issue_hooks)
  12 + end
  13 + end
  14 +end
app/services/issues/close_service.rb 0 → 100644
@@ -0,0 +1,20 @@ @@ -0,0 +1,20 @@
  1 +module Issues
  2 + class CloseService < Issues::BaseService
  3 + def execute(issue, commit = nil)
  4 + if issue.close
  5 + notification_service.close_issue(issue, current_user)
  6 + event_service.close_issue(issue, current_user)
  7 + create_note(issue, commit)
  8 + execute_hooks(issue)
  9 + end
  10 +
  11 + issue
  12 + end
  13 +
  14 + private
  15 +
  16 + def create_note(issue, current_commit)
  17 + Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit)
  18 + end
  19 + end
  20 +end
app/services/issues/create_service.rb 0 → 100644
@@ -0,0 +1,17 @@ @@ -0,0 +1,17 @@
  1 +module Issues
  2 + class CreateService < Issues::BaseService
  3 + def execute
  4 + issue = project.issues.new(params)
  5 + issue.author = current_user
  6 +
  7 + if issue.save
  8 + notification_service.new_issue(issue, current_user)
  9 + event_service.open_issue(issue, current_user)
  10 + issue.create_cross_references!(issue.project, current_user)
  11 + execute_hooks(issue)
  12 + end
  13 +
  14 + issue
  15 + end
  16 + end
  17 +end
app/services/issues/reopen_service.rb 0 → 100644
@@ -0,0 +1,19 @@ @@ -0,0 +1,19 @@
  1 +module Issues
  2 + class ReopenService < Issues::BaseService
  3 + def execute(issue)
  4 + if issue.reopen
  5 + event_service.reopen_issue(issue, current_user)
  6 + create_note(issue)
  7 + execute_hooks(issue)
  8 + end
  9 +
  10 + issue
  11 + end
  12 +
  13 + private
  14 +
  15 + def create_note(issue)
  16 + Note.create_status_change_note(issue, issue.project, current_user, issue.state, nil)
  17 + end
  18 + end
  19 +end
app/services/issues/update_service.rb 0 → 100644
@@ -0,0 +1,28 @@ @@ -0,0 +1,28 @@
  1 +module Issues
  2 + class UpdateService < Issues::BaseService
  3 + def execute(issue)
  4 + state = params.delete('state_event')
  5 +
  6 + case state
  7 + when 'reopen'
  8 + Issues::ReopenService.new(project, current_user, {}).execute(issue)
  9 + when 'close'
  10 + Issues::CloseService.new(project, current_user, {}).execute(issue)
  11 + end
  12 +
  13 + if params.present? && issue.update_attributes(params)
  14 + issue.reset_events_cache
  15 +
  16 + if issue.previous_changes.include?('assignee_id')
  17 + notification_service.reassigned_issue(issue, current_user)
  18 + create_assignee_note(issue)
  19 + end
  20 +
  21 + issue.notice_added_references(issue.project, current_user)
  22 + execute_hooks(issue)
  23 + end
  24 +
  25 + issue
  26 + end
  27 + end
  28 +end
config/application.rb
@@ -21,7 +21,6 @@ module Gitlab @@ -21,7 +21,6 @@ module Gitlab
21 # Activate observers that should always be running. 21 # Activate observers that should always be running.
22 config.active_record.observers = :milestone_observer, 22 config.active_record.observers = :milestone_observer,
23 :project_activity_cache_observer, 23 :project_activity_cache_observer,
24 - :issue_observer,  
25 :key_observer, 24 :key_observer,
26 :merge_request_observer, 25 :merge_request_observer,
27 :note_observer, 26 :note_observer,
lib/api/issues.rb
@@ -48,17 +48,15 @@ module API @@ -48,17 +48,15 @@ module API
48 # Example Request: 48 # Example Request:
49 # POST /projects/:id/issues 49 # POST /projects/:id/issues
50 post ":id/issues" do 50 post ":id/issues" do
51 - set_current_user_for_thread do  
52 - required_attributes! [:title]  
53 - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]  
54 - attrs[:label_list] = params[:labels] if params[:labels].present?  
55 - @issue = user_project.issues.new attrs  
56 - @issue.author = current_user  
57 - if @issue.save  
58 - present @issue, with: Entities::Issue  
59 - else  
60 - not_found!  
61 - end 51 + required_attributes! [:title]
  52 + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id]
  53 + attrs[:label_list] = params[:labels] if params[:labels].present?
  54 + issue = ::Issues::CreateService.new(user_project, current_user, attrs).execute
  55 +
  56 + if issue.valid?
  57 + present issue, with: Entities::Issue
  58 + else
  59 + not_found!
62 end 60 end
63 end 61 end
64 62
@@ -76,18 +74,18 @@ module API @@ -76,18 +74,18 @@ module API
76 # Example Request: 74 # Example Request:
77 # PUT /projects/:id/issues/:issue_id 75 # PUT /projects/:id/issues/:issue_id
78 put ":id/issues/:issue_id" do 76 put ":id/issues/:issue_id" do
79 - set_current_user_for_thread do  
80 - @issue = user_project.issues.find(params[:issue_id])  
81 - authorize! :modify_issue, @issue 77 + issue = user_project.issues.find(params[:issue_id])
  78 + authorize! :modify_issue, issue
  79 +
  80 + attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]
  81 + attrs[:label_list] = params[:labels] if params[:labels].present?
82 82
83 - attrs = attributes_for_keys [:title, :description, :assignee_id, :milestone_id, :state_event]  
84 - attrs[:label_list] = params[:labels] if params[:labels].present? 83 + issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue)
85 84
86 - if @issue.update_attributes attrs  
87 - present @issue, with: Entities::Issue  
88 - else  
89 - not_found!  
90 - end 85 + if issue.valid?
  86 + present issue, with: Entities::Issue
  87 + else
  88 + not_found!
91 end 89 end
92 end 90 end
93 91
spec/observers/issue_observer_spec.rb
@@ -1,99 +0,0 @@ @@ -1,99 +0,0 @@
1 -require 'spec_helper'  
2 -  
3 -describe IssueObserver do  
4 - let(:some_user) { create :user }  
5 - let(:assignee) { create :user }  
6 - let(:author) { create :user }  
7 - let(:mock_issue) { create(:issue, assignee: assignee, author: author) }  
8 -  
9 -  
10 - before { subject.stub(:current_user).and_return(some_user) }  
11 - before { subject.stub(:current_commit).and_return(nil) }  
12 - before { subject.stub(notification: double('NotificationService').as_null_object) }  
13 - before { mock_issue.project.stub_chain(:repository, :commit).and_return(nil) }  
14 -  
15 - subject { IssueObserver.instance }  
16 -  
17 - describe '#after_create' do  
18 - it 'trigger notification to send emails' do  
19 - subject.should_receive(:notification)  
20 -  
21 - subject.after_create(mock_issue)  
22 - end  
23 -  
24 - it 'should create cross-reference notes' do  
25 - other_issue = create(:issue)  
26 - mock_issue.stub(references: [other_issue])  
27 -  
28 - Note.should_receive(:create_cross_reference_note).with(other_issue, mock_issue,  
29 - some_user, mock_issue.project)  
30 - subject.after_create(mock_issue)  
31 - end  
32 - end  
33 -  
34 - context '#after_close' do  
35 - context 'a status "closed"' do  
36 - before { mock_issue.stub(state: 'closed') }  
37 -  
38 - it 'note is created if the issue is being closed' do  
39 - Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', nil)  
40 -  
41 - subject.after_close(mock_issue, nil)  
42 - end  
43 -  
44 - it 'trigger notification to send emails' do  
45 - subject.notification.should_receive(:close_issue).with(mock_issue, some_user)  
46 - subject.after_close(mock_issue, nil)  
47 - end  
48 -  
49 - it 'appends a mention to the closing commit if one is present' do  
50 - commit = double('commit', gfm_reference: 'commit 123456')  
51 - subject.stub(current_commit: commit)  
52 -  
53 - Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'closed', commit)  
54 -  
55 - subject.after_close(mock_issue, nil)  
56 - end  
57 - end  
58 -  
59 - context 'a status "reopened"' do  
60 - before { mock_issue.stub(state: 'reopened') }  
61 -  
62 - it 'note is created if the issue is being reopened' do  
63 - Note.should_receive(:create_status_change_note).with(mock_issue, mock_issue.project, some_user, 'reopened', nil)  
64 -  
65 - subject.after_reopen(mock_issue, nil)  
66 - end  
67 - end  
68 - end  
69 -  
70 - context '#after_update' do  
71 - before(:each) do  
72 - mock_issue.stub(:is_being_reassigned?).and_return(false)  
73 - end  
74 -  
75 - context 'notification' do  
76 - it 'triggered if the issue is being reassigned' do  
77 - mock_issue.should_receive(:is_being_reassigned?).and_return(true)  
78 - subject.should_receive(:notification)  
79 -  
80 - subject.after_update(mock_issue)  
81 - end  
82 -  
83 - it 'is not triggered if the issue is not being reassigned' do  
84 - mock_issue.should_receive(:is_being_reassigned?).and_return(false)  
85 - subject.should_not_receive(:notification)  
86 -  
87 - subject.after_update(mock_issue)  
88 - end  
89 - end  
90 -  
91 - context 'cross-references' do  
92 - it 'notices added references' do  
93 - mock_issue.should_receive(:notice_added_references)  
94 -  
95 - subject.after_update(mock_issue)  
96 - end  
97 - end  
98 - end  
99 -end  
spec/services/git_push_service_spec.rb
@@ -170,16 +170,10 @@ describe GitPushService do @@ -170,16 +170,10 @@ describe GitPushService do
170 Issue.find(issue.id).should be_closed 170 Issue.find(issue.id).should be_closed
171 end 171 end
172 172
173 - it "passes the closing commit as a thread-local" do  
174 - service.execute(project, user, @oldrev, @newrev, @ref)  
175 -  
176 - Thread.current[:current_commit].should == closing_commit  
177 - end  
178 -  
179 it "doesn't create cross-reference notes for a closing reference" do 173 it "doesn't create cross-reference notes for a closing reference" do
180 expect { 174 expect {
181 service.execute(project, user, @oldrev, @newrev, @ref) 175 service.execute(project, user, @oldrev, @newrev, @ref)
182 - }.not_to change { Note.where(project_id: project.id, system: true).count } 176 + }.not_to change { Note.where(project_id: project.id, system: true, commit_id: closing_commit.id).count }
183 end 177 end
184 178
185 it "doesn't close issues when pushed to non-default branches" do 179 it "doesn't close issues when pushed to non-default branches" do
spec/services/issues/close_service_spec.rb 0 → 100644
@@ -0,0 +1,35 @@ @@ -0,0 +1,35 @@
  1 +require 'spec_helper'
  2 +
  3 +describe Issues::CloseService do
  4 + let(:project) { create(:empty_project) }
  5 + let(:user) { create(:user) }
  6 + let(:user2) { create(:user) }
  7 + let(:issue) { create(:issue, assignee: user2) }
  8 +
  9 + before do
  10 + project.team << [user, :master]
  11 + project.team << [user2, :developer]
  12 + end
  13 +
  14 + describe :execute do
  15 + context "valid params" do
  16 + before do
  17 + @issue = Issues::CloseService.new(project, user, {}).execute(issue)
  18 + end
  19 +
  20 + it { @issue.should be_valid }
  21 + it { @issue.should be_closed }
  22 +
  23 + it 'should send email to user2 about assign of new issue' do
  24 + email = ActionMailer::Base.deliveries.last
  25 + email.to.first.should == user2.email
  26 + email.subject.should include(issue.title)
  27 + end
  28 +
  29 + it 'should create system note about issue reassign' do
  30 + note = @issue.notes.last
  31 + note.note.should include "Status changed to closed"
  32 + end
  33 + end
  34 + end
  35 +end
spec/services/issues/create_service_spec.rb 0 → 100644
@@ -0,0 +1,23 @@ @@ -0,0 +1,23 @@
  1 +require 'spec_helper'
  2 +
  3 +describe Issues::CreateService do
  4 + let(:project) { create(:empty_project) }
  5 + let(:user) { create(:user) }
  6 +
  7 + describe :execute do
  8 + context "valid params" do
  9 + before do
  10 + project.team << [user, :master]
  11 + opts = {
  12 + title: 'Awesome issue',
  13 + description: 'please fix'
  14 + }
  15 +
  16 + @issue = Issues::CreateService.new(project, user, opts).execute
  17 + end
  18 +
  19 + it { @issue.should be_valid }
  20 + it { @issue.title.should == 'Awesome issue' }
  21 + end
  22 + end
  23 +end
spec/services/issues/update_service_spec.rb 0 → 100644
@@ -0,0 +1,44 @@ @@ -0,0 +1,44 @@
  1 +require 'spec_helper'
  2 +
  3 +describe Issues::UpdateService do
  4 + let(:project) { create(:empty_project) }
  5 + let(:user) { create(:user) }
  6 + let(:user2) { create(:user) }
  7 + let(:issue) { create(:issue) }
  8 +
  9 + before do
  10 + project.team << [user, :master]
  11 + project.team << [user2, :developer]
  12 + end
  13 +
  14 + describe :execute do
  15 + context "valid params" do
  16 + before do
  17 + opts = {
  18 + title: 'New title',
  19 + description: 'Also please fix',
  20 + assignee_id: user2.id,
  21 + state_event: 'close'
  22 + }
  23 +
  24 + @issue = Issues::UpdateService.new(project, user, opts).execute(issue)
  25 + end
  26 +
  27 + it { @issue.should be_valid }
  28 + it { @issue.title.should == 'New title' }
  29 + it { @issue.assignee.should == user2 }
  30 + it { @issue.should be_closed }
  31 +
  32 + it 'should send email to user2 about assign of new issue' do
  33 + email = ActionMailer::Base.deliveries.last
  34 + email.to.first.should == user2.email
  35 + email.subject.should include(issue.title)
  36 + end
  37 +
  38 + it 'should create system note about issue reassign' do
  39 + note = @issue.notes.last
  40 + note.note.should include "Reassigned to \@#{user2.username}"
  41 + end
  42 + end
  43 + end
  44 +end