Commit 38ffb8220c8d8ae030b762f7b2d244eabe8cc0bf
1 parent
58a1ed6d
Exists in
master
and in
4 other branches
use NotificationService for handle notify logic when MR created
Showing
4 changed files
with
33 additions
and
18 deletions
Show diff stats
app/observers/merge_request_observer.rb
| @@ -2,9 +2,7 @@ class MergeRequestObserver < ActiveRecord::Observer | @@ -2,9 +2,7 @@ class MergeRequestObserver < ActiveRecord::Observer | ||
| 2 | cattr_accessor :current_user | 2 | cattr_accessor :current_user |
| 3 | 3 | ||
| 4 | def after_create(merge_request) | 4 | def after_create(merge_request) |
| 5 | - if merge_request.assignee && merge_request.assignee != current_user | ||
| 6 | - Notify.delay.new_merge_request_email(merge_request.id) | ||
| 7 | - end | 5 | + notification.new_merge_request(merge_request, current_user) |
| 8 | end | 6 | end |
| 9 | 7 | ||
| 10 | def after_close(merge_request, transition) | 8 | def after_close(merge_request, transition) |
app/services/notification_service.rb
| @@ -47,7 +47,7 @@ class NotificationService | @@ -47,7 +47,7 @@ class NotificationService | ||
| 47 | end | 47 | end |
| 48 | end | 48 | end |
| 49 | 49 | ||
| 50 | - # When we reassign an issue we should send next emails: | 50 | + # When create an issue we should send next emails: |
| 51 | # | 51 | # |
| 52 | # * issue assignee if his notification level is not Disabled | 52 | # * issue assignee if his notification level is not Disabled |
| 53 | # | 53 | # |
| @@ -56,4 +56,14 @@ class NotificationService | @@ -56,4 +56,14 @@ class NotificationService | ||
| 56 | Notify.delay.new_issue_email(issue.id) | 56 | Notify.delay.new_issue_email(issue.id) |
| 57 | end | 57 | end |
| 58 | end | 58 | end |
| 59 | + | ||
| 60 | + # When create a merge request we should send next emails: | ||
| 61 | + # | ||
| 62 | + # * mr assignee if his notification level is not Disabled | ||
| 63 | + # | ||
| 64 | + def new_merge_request(merge_request, current_user) | ||
| 65 | + if merge_request.assignee && merge_request.assignee != current_user | ||
| 66 | + Notify.delay.new_merge_request_email(merge_request.id) | ||
| 67 | + end | ||
| 68 | + end | ||
| 59 | end | 69 | end |
spec/observers/merge_request_observer_spec.rb
| @@ -10,7 +10,8 @@ describe MergeRequestObserver do | @@ -10,7 +10,8 @@ describe MergeRequestObserver do | ||
| 10 | let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author) } | 10 | let(:closed_assigned_mr) { create(:closed_merge_request, assignee: assignee, author: author) } |
| 11 | let(:closed_unassigned_mr) { create(:closed_merge_request, author: author) } | 11 | let(:closed_unassigned_mr) { create(:closed_merge_request, author: author) } |
| 12 | 12 | ||
| 13 | - before(:each) { subject.stub(:current_user).and_return(some_user) } | 13 | + before { subject.stub(:current_user).and_return(some_user) } |
| 14 | + before { subject.stub(notification: mock('NotificationService').as_null_object) } | ||
| 14 | 15 | ||
| 15 | subject { MergeRequestObserver.instance } | 16 | subject { MergeRequestObserver.instance } |
| 16 | 17 | ||
| @@ -18,21 +19,11 @@ describe MergeRequestObserver do | @@ -18,21 +19,11 @@ describe MergeRequestObserver do | ||
| 18 | 19 | ||
| 19 | it 'is called when a merge request is created' do | 20 | it 'is called when a merge request is created' do |
| 20 | subject.should_receive(:after_create) | 21 | subject.should_receive(:after_create) |
| 21 | - | ||
| 22 | - MergeRequest.observers.enable :merge_request_observer do | ||
| 23 | - create(:merge_request, project: create(:project)) | ||
| 24 | - end | 22 | + create(:merge_request, project: create(:project)) |
| 25 | end | 23 | end |
| 26 | 24 | ||
| 27 | - it 'sends an email to the assignee' do | ||
| 28 | - Notify.should_receive(:new_merge_request_email).with(mr_mock.id) | ||
| 29 | - subject.after_create(mr_mock) | ||
| 30 | - end | ||
| 31 | - | ||
| 32 | - it 'does not send an email to the assignee if assignee created the merge request' do | ||
| 33 | - subject.stub(:current_user).and_return(assignee) | ||
| 34 | - Notify.should_not_receive(:new_merge_request_email) | ||
| 35 | - | 25 | + it 'trigger notification service' do |
| 26 | + subject.should_receive(:notification) | ||
| 36 | subject.after_create(mr_mock) | 27 | subject.after_create(mr_mock) |
| 37 | end | 28 | end |
| 38 | end | 29 | end |
spec/services/notification_service_spec.rb
| @@ -44,4 +44,20 @@ describe NotificationService do | @@ -44,4 +44,20 @@ describe NotificationService do | ||
| 44 | end | 44 | end |
| 45 | end | 45 | end |
| 46 | end | 46 | end |
| 47 | + | ||
| 48 | + describe 'Merge Requests' do | ||
| 49 | + let(:merge_request) { create :merge_request, assignee: create(:user) } | ||
| 50 | + | ||
| 51 | + describe :new_merge_request do | ||
| 52 | + it 'should send email to merge_request assignee' do | ||
| 53 | + Notify.should_receive(:new_merge_request_email).with(merge_request.id) | ||
| 54 | + notification.new_merge_request(merge_request, merge_request.author) | ||
| 55 | + end | ||
| 56 | + | ||
| 57 | + it 'should not send email to merge_request assignee if he is current_user' do | ||
| 58 | + Notify.should_not_receive(:new_merge_request_email).with(merge_request.id) | ||
| 59 | + notification.new_merge_request(merge_request, merge_request.assignee) | ||
| 60 | + end | ||
| 61 | + end | ||
| 62 | + end | ||
| 47 | end | 63 | end |