diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index eef849d8209a5a61398383d9b456ed5344bf2855..4e7a716bfe411deafd8bd3e7cc7ece5781b2b772 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 @@ -76,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 deleted file mode 100644 index 30da1f83da796c7a79dfe16ee0447775ab52ec80..0000000000000000000000000000000000000000 --- a/app/observers/issue_observer.rb +++ /dev/null @@ -1,46 +0,0 @@ -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) - 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 - - 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' - 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/app/services/base_service.rb b/app/services/base_service.rb index 610f04748722a7c729fba28e590531bc204f97f2..9ad8092315266ea094631f45134f486c3fbc42ec 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/git_push_service.rb b/app/services/git_push_service.rb index fcc03c3e4b8e0db076152bdb894658a590cdfe95..351b446457dfdb2e0e0ec1cce1de31028e5b9bb3 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 new file mode 100644 index 0000000000000000000000000000000000000000..2e1e1f7e0f0ada3eb8e4d3937af767e84c010b49 --- /dev/null +++ b/app/services/issues/base_service.rb @@ -0,0 +1,14 @@ +module Issues + class BaseService < ::BaseService + + private + + 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/close_service.rb b/app/services/issues/close_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..85c0226ccab252b2504088f6ca11f6eb44304434 --- /dev/null +++ b/app/services/issues/close_service.rb @@ -0,0 +1,20 @@ +module Issues + class CloseService < Issues::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/create_service.rb b/app/services/issues/create_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..d6137833bb9d2bab4a2b366c7ef049b7c33eaf67 --- /dev/null +++ b/app/services/issues/create_service.rb @@ -0,0 +1,17 @@ +module Issues + class CreateService < Issues::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 + end +end diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..a931398aff694a8a88edb1d50110a0d18b776890 --- /dev/null +++ b/app/services/issues/reopen_service.rb @@ -0,0 +1,19 @@ +module Issues + class ReopenService < Issues::BaseService + def execute(issue) + if issue.reopen + event_service.reopen_issue(issue, current_user) + create_note(issue) + 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 new file mode 100644 index 0000000000000000000000000000000000000000..b562c401fd4043acc8ea6394c76134f9b01377bb --- /dev/null +++ b/app/services/issues/update_service.rb @@ -0,0 +1,28 @@ +module Issues + class UpdateService < Issues::BaseService + def execute(issue) + 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.previous_changes.include?('assignee_id') + notification_service.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/config/application.rb b/config/application.rb index 4970c9921cff39d2b3ae00df3c1cff1d690031be..3933c046af08b4c09b6128788eed08708b22c039 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, diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 3d15c35b8cc4db079fc3d193a6db37ae001a5be1..f50be3a815d9038b8345c998d22ed1a09f385411 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 @@ -76,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? + issue = ::Issues::UpdateService.new(user_project, current_user, attrs).execute(issue) - if @issue.update_attributes attrs - present @issue, with: Entities::Issue - else - not_found! - end + if issue.valid? + present issue, with: Entities::Issue + else + not_found! end end diff --git a/spec/observers/issue_observer_spec.rb b/spec/observers/issue_observer_spec.rb deleted file mode 100644 index 9a0a2c4329c84e47e891e2451afec8ecbade2676..0000000000000000000000000000000000000000 --- 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 diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 90738c681fa0a9d4328d9ad5078e6cb0712eb4ba..6b89f213bece5aff595feb4feecde6a217a24999 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 diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..d4f2cc1339bef8a917e1dacfc8cd325edd3e4f6c --- /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/create_service_spec.rb b/spec/services/issues/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..90720be5ded3618eca088277014a943151a84ee4 --- /dev/null +++ b/spec/services/issues/create_service_spec.rb @@ -0,0 +1,23 @@ +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 } + 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 0000000000000000000000000000000000000000..347560414e78bbfb937aa3ada177153c05a84039 --- /dev/null +++ b/spec/services/issues/update_service_spec.rb @@ -0,0 +1,44 @@ +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 + opts = { + title: 'New title', + description: 'Also please fix', + assignee_id: user2.id, + state_event: 'close' + } + + @issue = Issues::UpdateService.new(project, user, opts).execute(issue) + end + + 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