diff options
| -rw-r--r-- | app/services/merge_requests/create_service.rb | 3 | ||||
| -rw-r--r-- | app/services/merge_requests/update_service.rb | 1 | ||||
| -rw-r--r-- | app/services/task_service.rb | 36 | ||||
| -rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 40 | ||||
| -rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 25 | ||||
| -rw-r--r-- | spec/services/task_service_spec.rb | 52 |
6 files changed, 142 insertions, 15 deletions
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 009d5a6867e..b5691cdf44f 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -2,7 +2,7 @@ module MergeRequests class CreateService < MergeRequests::BaseService def execute # @project is used to determine whether the user can set the merge request's - # assignee, milestone and labels. Whether they can depends on their + # assignee, milestone and labels. Whether they can depends on their # permissions on the target project. source_project = @project @project = Project.find(params[:target_project_id]) if params[:target_project_id] @@ -18,6 +18,7 @@ module MergeRequests merge_request.update_attributes(label_ids: label_params) event_service.open_mr(merge_request, current_user) notification_service.new_merge_request(merge_request, current_user) + task_service.new_merge_request(merge_request, current_user) merge_request.create_cross_references!(current_user) execute_hooks(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 55322bf47bb..85b11def231 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -28,6 +28,7 @@ module MergeRequests if merge_request.previous_changes.include?('assignee_id') create_assignee_note(merge_request) notification_service.reassigned_merge_request(merge_request, current_user) + task_service.reassigned_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') || diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 57f68c61d00..329a041061a 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -11,9 +11,7 @@ class TaskService # * creates a pending task for assignee if issue is assigned # def new_issue(issue, current_user) - if issue.is_assigned? && issue.assignee != current_user - create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) - end + new_issuable(issue, current_user) end # When close an issue we should: @@ -29,9 +27,23 @@ class TaskService # * creates a pending task for new assignee if issue is assigned # def reassigned_issue(issue, current_user) - if issue.is_assigned? - create_task(issue.project, issue, current_user, issue.assignee, Task::ASSIGNED) - end + reassigned_issuable(issue, current_user) + end + + # When create a merge request we should: + # + # * creates a pending task for assignee if merge request is assigned + # + def new_merge_request(merge_request, current_user) + new_issuable(merge_request, current_user) + end + + # When we reassign an merge request we should: + # + # * creates a pending task for new assignee if merge request is assigned + # + def reassigned_merge_request(merge_request, current_user) + reassigned_issuable(merge_request, current_user) end # When we mark a task as done we should: @@ -83,4 +95,16 @@ class TaskService def pending_tasks_for(user, project, target) user.tasks.pending.where(project: project, target: target) end + + def new_issuable(issuable, user) + if issuable.is_assigned? && issuable.assignee != user + create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + end + end + + def reassigned_issuable(issuable, user) + if issuable.is_assigned? + create_task(issuable.project, issuable, user, issuable.assignee, Task::ASSIGNED) + end + end end diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index be8f1676eeb..2e3c9f88369 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe MergeRequests::CreateService, services: true do let(:project) { create(:project) } let(:user) { create(:user) } + let(:assignee) { create(:user) } describe :execute do context 'valid params' do @@ -14,10 +15,12 @@ describe MergeRequests::CreateService, services: true do target_branch: 'master' } end + let(:service) { MergeRequests::CreateService.new(project, user, opts) } before do project.team << [user, :master] + project.team << [assignee, :developer] allow(service).to receive(:execute_hooks) @merge_request = service.execute @@ -25,10 +28,47 @@ describe MergeRequests::CreateService, services: true do it { expect(@merge_request).to be_valid } it { expect(@merge_request.title).to eq('Awesome merge_request') } + it { expect(@merge_request.assignee).to be_nil } it 'should execute hooks with default action' do expect(service).to have_received(:execute_hooks).with(@merge_request) end + + it 'does not creates a pending task' do + attributes = { + project: project, + target: @merge_request + } + + expect(Task.where(attributes).count).to be_zero + end + + context 'when merge request is assigned to someone' do + let(:opts) do + { + title: 'Awesome merge_request', + description: 'please fix', + source_branch: 'feature', + target_branch: 'master', + assignee: assignee + } + end + + it { expect(@merge_request.assignee).to eq assignee } + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: assignee, + target: @merge_request, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end end end end diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 2e9e6e0870d..88853483784 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -98,6 +98,31 @@ describe MergeRequests::UpdateService, services: true do end end + context 'task queue' do + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) + end + + context 'when is reassigned' do + before do + update_merge_request({ assignee: user2 }) + end + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target: merge_request, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end + end + context 'when MergeRequest has tasks' do before { update_merge_request({ description: "- [ ] Task 1\n- [ ] Task 2" }) } diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 2fd75d10b6c..0a072dda94b 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -1,20 +1,20 @@ require 'spec_helper' describe TaskService, services: true do + let(:author) { create(:user) } + let(:john_doe) { create(:user) } + let(:project) { create(:project) } let(:service) { described_class.new } + before do + project.team << [author, :developer] + project.team << [john_doe, :developer] + end + describe 'Issues' do - let(:author) { create(:user) } - let(:john_doe) { create(:user) } - let(:project) { create(:empty_project, :public) } let(:assigned_issue) { create(:issue, project: project, assignee: john_doe) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } - before do - project.team << [author, :developer] - project.team << [john_doe, :developer] - end - describe '#new_issue' do it 'creates a pending task if assigned' do service.new_issue(assigned_issue, author) @@ -116,6 +116,42 @@ describe TaskService, services: true do end end + describe 'Merge Requests' do + let(:mr_assigned) { create(:merge_request, source_project: project, assignee: john_doe) } + let(:mr_unassigned) { create(:merge_request, source_project: project, assignee: nil) } + + describe '#new_merge_request' do + it 'creates a pending task if assigned' do + service.new_merge_request(mr_assigned, author) + + is_expected_to_create_pending_task(user: john_doe, target: mr_assigned, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, author) } + end + + it 'does not create a task if assignee is the current user' do + is_expected_to_not_create_task { service.new_merge_request(mr_unassigned, john_doe) } + end + end + + describe '#reassigned_merge_request' do + it 'creates a pending task for new assignee' do + mr_unassigned.update_attribute(:assignee, john_doe) + service.reassigned_merge_request(mr_unassigned, author) + + is_expected_to_create_pending_task(user: john_doe, target: mr_unassigned, action: Task::ASSIGNED) + end + + it 'does not create a task if unassigned' do + mr_assigned.update_attribute(:assignee, nil) + + is_expected_to_not_create_task { service.reassigned_merge_request(mr_assigned, author) } + end + end + end + def is_expected_to_create_pending_task(attributes = {}) attributes.reverse_merge!( project: project, |