From 853c69c48a1641ad742cf9f70326fc93c44ea58d Mon Sep 17 00:00:00 2001 From: Riyad Preukschas Date: Sat, 13 Oct 2012 20:55:50 +0200 Subject: [PATCH 1/2] Reorder and group methods in Notify mailer --- app/mailers/notify.rb | 119 ++++++++++++++++++++++++++---------------- 1 file changed, 75 insertions(+), 44 deletions(-) diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 9e64b64a..7f3862ad 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -9,11 +9,11 @@ class Notify < ActionMailer::Base default from: Gitlab.config.email_from - def new_user_email(user_id, password) - @user = User.find(user_id) - @password = password - mail(to: @user.email, subject: subject("Account was created for you")) - end + + + # + # Issue + # def new_issue_email(issue_id) @issue = Issue.find(issue_id) @@ -21,40 +21,26 @@ class Notify < ActionMailer::Base mail(to: @issue.assignee_email, subject: subject("new issue ##{@issue.id}", @issue.title)) end - def note_wall_email(recipient_id, note_id) - @note = Note.find(note_id) - @project = @note.project - mail(to: recipient(recipient_id), subject: subject) + def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id) + @issue = Issue.find(issue_id) + @previous_assignee ||= User.find(previous_assignee_id) + @project = @issue.project + mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title)) end - def note_commit_email(recipient_id, note_id) - @note = Note.find(note_id) - @commit = @note.noteable - @commit = CommitDecorator.decorate(@commit) - @project = @note.project - mail(to: recipient(recipient_id), subject: subject("note for commit #{@commit.short_id}", @commit.title)) + def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) + @issue = Issue.find issue_id + @issue_status = status + @updated_by = User.find updated_by_user_id + mail(to: recipient(recipient_id), + subject: subject("changed issue ##{@issue.id}", @issue.title)) end - def note_merge_request_email(recipient_id, note_id) - @note = Note.find(note_id) - @merge_request = @note.noteable - @project = @note.project - mail(to: recipient(recipient_id), subject: subject("note for merge request !#{@merge_request.id}")) - end - def note_issue_email(recipient_id, note_id) - @note = Note.find(note_id) - @issue = @note.noteable - @project = @note.project - mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}")) - end - def note_wiki_email(recipient_id, note_id) - @note = Note.find(note_id) - @wiki = @note.noteable - @project = @note.project - mail(to: recipient(recipient_id), subject: subject("note for wiki")) - end + # + # Merge Request + # def new_merge_request_email(merge_request_id) @merge_request = MergeRequest.find(merge_request_id) @@ -69,13 +55,53 @@ class Notify < ActionMailer::Base mail(to: recipient(recipient_id), subject: subject("changed merge request !#{@merge_request.id}", @merge_request.title)) end - def reassigned_issue_email(recipient_id, issue_id, previous_assignee_id) - @issue = Issue.find(issue_id) - @previous_assignee ||= User.find(previous_assignee_id) - @project = @issue.project - mail(to: recipient(recipient_id), subject: subject("changed issue ##{@issue.id}", @issue.title)) + + + # + # Note + # + + def note_commit_email(recipient_id, note_id) + @note = Note.find(note_id) + @commit = @note.noteable + @commit = CommitDecorator.decorate(@commit) + @project = @note.project + mail(to: recipient(recipient_id), subject: subject("note for commit #{@commit.short_id}", @commit.title)) end + def note_issue_email(recipient_id, note_id) + @note = Note.find(note_id) + @issue = @note.noteable + @project = @note.project + mail(to: recipient(recipient_id), subject: subject("note for issue ##{@issue.id}")) + end + + def note_merge_request_email(recipient_id, note_id) + @note = Note.find(note_id) + @merge_request = @note.noteable + @project = @note.project + mail(to: recipient(recipient_id), subject: subject("note for merge request !#{@merge_request.id}")) + end + + def note_wall_email(recipient_id, note_id) + @note = Note.find(note_id) + @project = @note.project + mail(to: recipient(recipient_id), subject: subject) + end + + def note_wiki_email(recipient_id, note_id) + @note = Note.find(note_id) + @wiki = @note.noteable + @project = @note.project + mail(to: recipient(recipient_id), subject: subject("note for wiki")) + end + + + + # + # Project + # + def project_access_granted_email(user_project_id) @users_project = UsersProject.find user_project_id @project = @users_project.project @@ -83,14 +109,19 @@ class Notify < ActionMailer::Base subject: subject("access to project was granted")) end - def issue_status_changed_email(recipient_id, issue_id, status, updated_by_user_id) - @issue = Issue.find issue_id - @issue_status = status - @updated_by = User.find updated_by_user_id - mail(to: recipient(recipient_id), - subject: subject("changed issue ##{@issue.id}", @issue.title)) + + + # + # User + # + + def new_user_email(user_id, password) + @user = User.find(user_id) + @password = password + mail(to: @user.email, subject: subject("Account was created for you")) end + private # Look up a User by their ID and return their email address From 413778b6458c71706685150f191becee5d398391 Mon Sep 17 00:00:00 2001 From: Riyad Preukschas Date: Sat, 13 Oct 2012 22:46:27 +0200 Subject: [PATCH 2/2] Rename NoteObserver methods and clarify things --- app/observers/note_observer.rb | 21 ++++++---- spec/observers/note_observer_spec.rb | 61 ++++++++++++++++++---------- 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/app/observers/note_observer.rb b/app/observers/note_observer.rb index d328338a..083aa705 100644 --- a/app/observers/note_observer.rb +++ b/app/observers/note_observer.rb @@ -1,9 +1,14 @@ class NoteObserver < ActiveRecord::Observer def after_create(note) + send_notify_mails(note) + end + + protected + + def send_notify_mails(note) if note.notify - # Notify whole team except author of note - notify_team_of_new_note(note) + notify_team(note) elsif note.notify_author # Notify only author of resource Notify.note_commit_email(note.commit_author.id, note.id).deliver @@ -13,15 +18,15 @@ class NoteObserver < ActiveRecord::Observer end end - protected - - def notify_team_of_new_note(note) - note_is_on = note.noteable_type || 'Wall' - notify_method = 'note_' + note_is_on.underscore + '_email' + # Notifies the whole team except the author of note + def notify_team(note) + # Note: wall posts are not "attached" to anything, so fall back to "Wall" + noteable_type = note.noteable_type || "Wall" + notify_method = "note_#{noteable_type.underscore}_email".to_sym if Notify.respond_to? notify_method team_without_note_author(note).map do |u| - Notify.send(notify_method.to_sym, u.id, note.id).deliver + Notify.send(notify_method, u.id, note.id).deliver end end end diff --git a/spec/observers/note_observer_spec.rb b/spec/observers/note_observer_spec.rb index fb5a3544..203a58a4 100644 --- a/spec/observers/note_observer_spec.rb +++ b/spec/observers/note_observer_spec.rb @@ -3,8 +3,11 @@ require 'spec_helper' describe NoteObserver do subject { NoteObserver.instance } + let(:team_without_author) { (1..2).map { |n| double :user, id: n } } + let(:delivery_success) { double deliver: true } + describe '#after_create' do - let(:note) { double :note, notify: false, notify_author: false } + let(:note) { double :note } it 'is called after a note is created' do subject.should_receive :after_create @@ -14,40 +17,51 @@ describe NoteObserver do end end + it 'sends out notifications' do + subject.should_receive(:send_notify_mails).with(note) + + subject.after_create(note) + end + end + + describe "#send_notify_mails" do + let(:note) { double :note, notify: false, notify_author: false } + it 'notifies team of new note when flagged to notify' do note.stub(:notify).and_return(true) - subject.should_receive(:notify_team_of_new_note).with(note) + subject.should_receive(:notify_team).with(note) subject.after_create(note) end + it 'does not notify team of new note when not flagged to notify' do - subject.should_not_receive(:notify_team_of_new_note).with(note) + subject.should_not_receive(:notify_team).with(note) subject.after_create(note) end + it 'notifies the author of a commit when flagged to notify the author' do note.stub(:notify_author).and_return(true) note.stub(:id).and_return(42) author = double :user, id: 1 note.stub(:commit_author).and_return(author) - Notify.should_receive(:note_commit_email).and_return(double(deliver: true)) + Notify.should_receive(:note_commit_email).and_return(delivery_success) subject.after_create(note) end + it 'does not notify the author of a commit when not flagged to notify the author' do Notify.should_not_receive(:note_commit_email) subject.after_create(note) end + it 'does nothing if no notify flags are set' do subject.after_create(note).should be_nil end end - - let(:team_without_author) { (1..2).map { |n| double :user, id: n } } - - describe '#notify_team_of_new_note' do + describe '#notify_team' do let(:note) { double :note, id: 1 } before :each do @@ -57,40 +71,45 @@ describe NoteObserver do context 'notifies team of a new note on' do it 'a commit' do note.stub(:noteable_type).and_return('Commit') - Notify.should_receive(:note_commit_email).twice.and_return(double(deliver: true)) + Notify.should_receive(:note_commit_email).twice.and_return(delivery_success) - subject.send(:notify_team_of_new_note, note) + subject.send(:notify_team, note) end + it 'an issue' do note.stub(:noteable_type).and_return('Issue') - Notify.should_receive(:note_issue_email).twice.and_return(double(deliver: true)) + Notify.should_receive(:note_issue_email).twice.and_return(delivery_success) - subject.send(:notify_team_of_new_note, note) + subject.send(:notify_team, note) end + it 'a wiki page' do note.stub(:noteable_type).and_return('Wiki') - Notify.should_receive(:note_wiki_email).twice.and_return(double(deliver: true)) + Notify.should_receive(:note_wiki_email).twice.and_return(delivery_success) - subject.send(:notify_team_of_new_note, note) + subject.send(:notify_team, note) end + it 'a merge request' do note.stub(:noteable_type).and_return('MergeRequest') - Notify.should_receive(:note_merge_request_email).twice.and_return(double(deliver: true)) + Notify.should_receive(:note_merge_request_email).twice.and_return(delivery_success) - subject.send(:notify_team_of_new_note, note) + subject.send(:notify_team, note) end - it 'a wall' do - note.stub(:noteable_type).and_return(nil) - Notify.should_receive(:note_wall_email).twice.and_return(double(deliver: true)) - subject.send(:notify_team_of_new_note, note) + it 'a wall' do + # Note: wall posts have #noteable_type of nil + note.stub(:noteable_type).and_return(nil) + Notify.should_receive(:note_wall_email).twice.and_return(delivery_success) + + subject.send(:notify_team, note) end end it 'does nothing for a new note on a snippet' do note.stub(:noteable_type).and_return('Snippet') - subject.send(:notify_team_of_new_note, note).should be_nil + subject.send(:notify_team, note).should be_nil end end