Download as
Browse Code »

Commit 61e5fb6f370bc336799211d311131132a2159906

Authored by Dmitriy Zaporozhets
2 parents ca42e91e 413778b6
Exists in master and in 4 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb, spb-stable

Merge pull request #1695 from riyad/cleanup-note-observer-and-notifications 

Code cleanup on NoteObserver and Notify
Showing 3 changed files with 117 additions and 62 deletions   Show diff stats
app/mailers/notify.rb
  Diff comments   View file @61e5fb6
... ... @@ -9,11 +9,11 @@ class Notify < ActionMailer::Base
9 9  
10 10 default from: Gitlab.config.email_from
11 11  
12   - def new_user_email(user_id, password)
13   - @user = User.find(user_id)
14   - @password = password
15   - mail(to: @user.email, subject: subject("Account was created for you"))
16   - end
  12 +
  13 +
  14 + #
  15 + # Issue
  16 + #
17 17  
18 18 def new_issue_email(issue_id)
19 19 @issue = Issue.find(issue_id)
... ... @@ -21,12 +21,46 @@ class Notify < ActionMailer::Base
21 21 mail(to: @issue.assignee_email, subject: subject("new issue ##{@issue.id}", @issue.title))
22 22 end
23 23  
24   - def note_wall_email(recipient_id, note_id)
25   - @note = Note.find(note_id)
26   - @project = @note.project
27   - mail(to: recipient(recipient_id), subject: subject)
  24 + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id)
  25 + @issue = Issue.find(issue_id)
  26 + @previous_assignee ||= User.find(previous_assignee_id)
  27 + @project = @issue.project
  28 + mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title))
  29 + end
  30 +
  31 + def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
  32 + @issue = Issue.find issue_id
  33 + @issue_status = status
  34 + @updated_by = User.find updated_by_user_id
  35 + mail(to: recipient(recipient_id),
  36 + subject: subject("changed issue ##{@issue.id}", @issue.title))
  37 + end
  38 +
  39 +
  40 +
  41 + #
  42 + # Merge Request
  43 + #
  44 +
  45 + def new_merge_request_email(merge_request_id)
  46 + @merge_request = MergeRequest.find(merge_request_id)
  47 + @project = @merge_request.project
  48 + mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
  49 + end
  50 +
  51 + def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
  52 + @merge_request = MergeRequest.find(merge_request_id)
  53 + @previous_assignee ||= User.find(previous_assignee_id)
  54 + @project = @merge_request.project
  55 + mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
28 56 end
29 57  
  58 +
  59 +
  60 + #
  61 + # Note
  62 + #
  63 +
30 64 def note_commit_email(recipient_id, note_id)
31 65 @note = Note.find(note_id)
32 66 @commit = @note.noteable
... ... @@ -35,6 +69,13 @@ class Notify < ActionMailer::Base
35 69 mail(to: recipient(recipient_id), subject: subject("note for commit #{@commit.short_id}", @commit.title))
36 70 end
37 71  
  72 + def note_issue_email(recipient_id, note_id)
  73 + @note = Note.find(note_id)
  74 + @issue = @note.noteable
  75 + @project = @note.project
  76 + mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}"))
  77 + end
  78 +
38 79 def note_merge_request_email(recipient_id, note_id)
39 80 @note = Note.find(note_id)
40 81 @merge_request = @note.noteable
... ... @@ -42,11 +83,10 @@ class Notify < ActionMailer::Base
42 83 mail(to: recipient(recipient_id), subject: subject("note for merge request !#{@merge_request.id}"))
43 84 end
44 85  
45   - def note_issue_email(recipient_id, note_id)
  86 + def note_wall_email(recipient_id, note_id)
46 87 @note = Note.find(note_id)
47   - @issue = @note.noteable
48 88 @project = @note.project
49   - mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}"))
  89 + mail(to: recipient(recipient_id), subject: subject)
50 90 end
51 91  
52 92 def note_wiki_email(recipient_id, note_id)
... ... @@ -56,25 +96,11 @@ class Notify < ActionMailer::Base
56 96 mail(to: recipient(recipient_id), subject: subject("note for wiki"))
57 97 end
58 98  
59   - def new_merge_request_email(merge_request_id)
60   - @merge_request = MergeRequest.find(merge_request_id)
61   - @project = @merge_request.project
62   - mail(to: @merge_request.assignee_email, subject: subject("new merge request !#{@merge_request.id}", @merge_request.title))
63   - end
64 99  
65   - def reassigned_merge_request_email(recipient_id, merge_request_id, previous_assignee_id)
66   - @merge_request = MergeRequest.find(merge_request_id)
67   - @previous_assignee ||= User.find(previous_assignee_id)
68   - @project = @merge_request.project
69   - mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title))
70   - end
71 100  
72   - def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id)
73   - @issue = Issue.find(issue_id)
74   - @previous_assignee ||= User.find(previous_assignee_id)
75   - @project = @issue.project
76   - mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title))
77   - end
  101 + #
  102 + # Project
  103 + #
78 104  
79 105 def project_access_granted_email(user_project_id)
80 106 @users_project = UsersProject.find user_project_id
... ... @@ -83,14 +109,19 @@ class Notify < ActionMailer::Base
83 109 subject: subject("access to project was granted"))
84 110 end
85 111  
86   - def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id)
87   - @issue = Issue.find issue_id
88   - @issue_status = status
89   - @updated_by = User.find updated_by_user_id
90   - mail(to: recipient(recipient_id),
91   - subject: subject("changed issue ##{@issue.id}", @issue.title))
  112 +
  113 +
  114 + #
  115 + # User
  116 + #
  117 +
  118 + def new_user_email(user_id, password)
  119 + @user = User.find(user_id)
  120 + @password = password
  121 + mail(to: @user.email, subject: subject("Account was created for you"))
92 122 end
93 123  
  124 +
94 125 private
95 126  
96 127 # Look up a User by their ID and return their email address
... ...
app/observers/note_observer.rb
  Diff comments   View file @61e5fb6
1 1 class NoteObserver < ActiveRecord::Observer
2 2  
3 3 def after_create(note)
  4 + send_notify_mails(note)
  5 + end
  6 +
  7 + protected
  8 +
  9 + def send_notify_mails(note)
4 10 if note.notify
5   - # Notify whole team except author of note
6   - notify_team_of_new_note(note)
  11 + notify_team(note)
7 12 elsif note.notify_author
8 13 # Notify only author of resource
9 14 Notify.note_commit_email(note.commit_author.id, note.id).deliver
... ... @@ -13,15 +18,15 @@ class NoteObserver &lt; ActiveRecord::Observer
13 18 end
14 19 end
15 20  
16   - protected
17   -
18   - def notify_team_of_new_note(note)
19   - note_is_on = note.noteable_type || 'Wall'
20   - notify_method = 'note_' + note_is_on.underscore + '_email'
  21 + # Notifies the whole team except the author of note
  22 + def notify_team(note)
  23 + # Note: wall posts are not "attached" to anything, so fall back to "Wall"
  24 + noteable_type = note.noteable_type || "Wall"
  25 + notify_method = "note_#{noteable_type.underscore}_email".to_sym
21 26  
22 27 if Notify.respond_to? notify_method
23 28 team_without_note_author(note).map do |u|
24   - Notify.send(notify_method.to_sym, u.id, note.id).deliver
  29 + Notify.send(notify_method, u.id, note.id).deliver
25 30 end
26 31 end
27 32 end
... ...
spec/observers/note_observer_spec.rb
  Diff comments   View file @61e5fb6
... ... @@ -3,8 +3,11 @@ require &#39;spec_helper&#39;
3 3 describe NoteObserver do
4 4 subject { NoteObserver.instance }
5 5  
  6 + let(:team_without_author) { (1..2).map { |n| double :user, id: n } }
  7 + let(:delivery_success) { double deliver: true }
  8 +
6 9 describe '#after_create' do
7   - let(:note) { double :note, notify: false, notify_author: false }
  10 + let(:note) { double :note }
8 11  
9 12 it 'is called after a note is created' do
10 13 subject.should_receive :after_create
... ... @@ -14,40 +17,51 @@ describe NoteObserver do
14 17 end
15 18 end
16 19  
  20 + it 'sends out notifications' do
  21 + subject.should_receive(:send_notify_mails).with(note)
  22 +
  23 + subject.after_create(note)
  24 + end
  25 + end
  26 +
  27 + describe "#send_notify_mails" do
  28 + let(:note) { double :note, notify: false, notify_author: false }
  29 +
17 30 it 'notifies team of new note when flagged to notify' do
18 31 note.stub(:notify).and_return(true)
19   - subject.should_receive(:notify_team_of_new_note).with(note)
  32 + subject.should_receive(:notify_team).with(note)
20 33  
21 34 subject.after_create(note)
22 35 end
  36 +
23 37 it 'does not notify team of new note when not flagged to notify' do
24   - subject.should_not_receive(:notify_team_of_new_note).with(note)
  38 + subject.should_not_receive(:notify_team).with(note)
25 39  
26 40 subject.after_create(note)
27 41 end
  42 +
28 43 it 'notifies the author of a commit when flagged to notify the author' do
29 44 note.stub(:notify_author).and_return(true)
30 45 note.stub(:id).and_return(42)
31 46 author = double :user, id: 1
32 47 note.stub(:commit_author).and_return(author)
33   - Notify.should_receive(:note_commit_email).and_return(double(deliver: true))
  48 + Notify.should_receive(:note_commit_email).and_return(delivery_success)
34 49  
35 50 subject.after_create(note)
36 51 end
  52 +
37 53 it 'does not notify the author of a commit when not flagged to notify the author' do
38 54 Notify.should_not_receive(:note_commit_email)
39 55  
40 56 subject.after_create(note)
41 57 end
  58 +
42 59 it 'does nothing if no notify flags are set' do
43 60 subject.after_create(note).should be_nil
44 61 end
45 62 end
46 63  
47   -
48   - let(:team_without_author) { (1..2).map { |n| double :user, id: n } }
49   -
50   - describe '#notify_team_of_new_note' do
  64 + describe '#notify_team' do
51 65 let(:note) { double :note, id: 1 }
52 66  
53 67 before :each do
... ... @@ -57,40 +71,45 @@ describe NoteObserver do
57 71 context 'notifies team of a new note on' do
58 72 it 'a commit' do
59 73 note.stub(:noteable_type).and_return('Commit')
60   - Notify.should_receive(:note_commit_email).twice.and_return(double(deliver: true))
  74 + Notify.should_receive(:note_commit_email).twice.and_return(delivery_success)
61 75  
62   - subject.send(:notify_team_of_new_note, note)
  76 + subject.send(:notify_team, note)
63 77 end
  78 +
64 79 it 'an issue' do
65 80 note.stub(:noteable_type).and_return('Issue')
66   - Notify.should_receive(:note_issue_email).twice.and_return(double(deliver: true))
  81 + Notify.should_receive(:note_issue_email).twice.and_return(delivery_success)
67 82  
68   - subject.send(:notify_team_of_new_note, note)
  83 + subject.send(:notify_team, note)
69 84 end
  85 +
70 86 it 'a wiki page' do
71 87 note.stub(:noteable_type).and_return('Wiki')
72   - Notify.should_receive(:note_wiki_email).twice.and_return(double(deliver: true))
  88 + Notify.should_receive(:note_wiki_email).twice.and_return(delivery_success)
73 89  
74   - subject.send(:notify_team_of_new_note, note)
  90 + subject.send(:notify_team, note)
75 91 end
  92 +
76 93 it 'a merge request' do
77 94 note.stub(:noteable_type).and_return('MergeRequest')
78   - Notify.should_receive(:note_merge_request_email).twice.and_return(double(deliver: true))
  95 + Notify.should_receive(:note_merge_request_email).twice.and_return(delivery_success)
79 96  
80   - subject.send(:notify_team_of_new_note, note)
  97 + subject.send(:notify_team, note)
81 98 end
  99 +
82 100 it 'a wall' do
  101 + # Note: wall posts have #noteable_type of nil
83 102 note.stub(:noteable_type).and_return(nil)
84   - Notify.should_receive(:note_wall_email).twice.and_return(double(deliver: true))
  103 + Notify.should_receive(:note_wall_email).twice.and_return(delivery_success)
85 104  
86   - subject.send(:notify_team_of_new_note, note)
  105 + subject.send(:notify_team, note)
87 106 end
88 107 end
89 108  
90 109 it 'does nothing for a new note on a snippet' do
91 110 note.stub(:noteable_type).and_return('Snippet')
92 111  
93   - subject.send(:notify_team_of_new_note, note).should be_nil
  112 + subject.send(:notify_team, note).should be_nil
94 113 end
95 114 end
96 115  
... ...