From f6035552e5d83b36f69e1ac7fa4b40ee5f7ab4fb Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Thu, 17 May 2012 17:23:34 -0400 Subject: [PATCH 01/13] New IssueObserver class and spec. Handles emails for new issues and reassigned issues. Need to add creating a Note on Issue close. --- app/models/issue_observer.rb | 17 ++++++++ spec/models/issue_observer_spec.rb | 67 ++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 app/models/issue_observer.rb create mode 100644 spec/models/issue_observer_spec.rb diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb new file mode 100644 index 00000000..e18195aa --- /dev/null +++ b/app/models/issue_observer.rb @@ -0,0 +1,17 @@ +class IssueObserver < ActiveRecord::Observer + cattr_accessor :current_user + + def after_create(issue) + Notify.new_issue_email(issue.id) if issue.assignee != current_user + end + + def after_change(issue) + if issue.assignee_id_changed? + recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } + + recipient_ids.each do |recipient_id| + Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was) + end + end + end +end diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb new file mode 100644 index 00000000..166e03ce --- /dev/null +++ b/spec/models/issue_observer_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' + +describe IssueObserver do + let(:some_user) { Factory.new(:user, :id => 1) } + let(:assignee) { Factory.new(:user, :id => 2) } + let(:issue) { Factory.new(:issue, :id => 42, :assignee => assignee) } + + before(:each) { subject.stub(:current_user).and_return(some_user) } + + subject { IssueObserver.instance } + + context 'when an issue is created' do + + it 'sends an email to the assignee' do + Notify.should_receive(:new_issue_email).with(issue.id) + + subject.after_create(issue) + end + + it 'does not send an email to the assignee if assignee created the issue' do + subject.stub(:current_user).and_return(assignee) + Notify.should_not_receive(:new_issue_email) + + subject.after_create(issue) + end + end + + context 'when an issue is modified' do + it 'but not reassigned, does not send a reassigned email' do + issue.stub(:assignee_id_changed?).and_return(false) + Notify.should_not_receive(:reassigned_issue_email) + + subject.after_change(issue) + end + + context 'and is reassigned' do + let(:previous_assignee) { Factory.new(:user, :id => 3) } + + before(:each) do + issue.stub(:assignee_id_changed?).and_return(true) + issue.stub(:assignee_id_was).and_return(previous_assignee.id) + end + + it 'sends a reassigned email to the previous and current assignees' do + Notify.should_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) + Notify.should_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) + + subject.after_change(issue) + end + + context 'does not send an email to the user who made the reassignment' do + it 'if the user is the assignee' do + subject.stub(:current_user).and_return(assignee) + Notify.should_not_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) + + subject.after_change(issue) + end + it 'if the user is the previous assignee' do + subject.stub(:current_user).and_return(previous_assignee) + Notify.should_not_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) + + subject.after_change(issue) + end + end + end + end +end From 2416e3cb19d6a668fc3274b1ae3382f4119dac1d Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Sun, 20 May 2012 14:15:13 -0400 Subject: [PATCH 02/13] Add new utility method for an issue to know whether it is being reassigned --- app/models/issue.rb | 11 ++++++++++- spec/models/issue_spec.rb | 19 +++++++++++++++---- 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 3ed47ef1..f1cb2e22 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -27,7 +27,7 @@ class Issue < ActiveRecord::Base validates :title, :presence => true, :length => { :within => 0..255 } - + validates :description, :length => { :within => 0..2000 } @@ -55,6 +55,15 @@ class Issue < ActiveRecord::Base def new? today? && created_at == updated_at end + + # Return the number of +1 comments (upvotes) + def upvotes + notes.select(&:upvote?).size + end + + def is_being_reassigned? + assignee_id_changed? + end end # == Schema Information # diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 791e9cde..68c05f2e 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -20,10 +20,21 @@ describe Issue do it { Issue.should respond_to :opened } end - it { Factory.create(:issue, - :author => Factory(:user), - :assignee => Factory(:user), - :project => Factory.create(:project)).should be_valid } + subject { Factory.create(:issue, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) } + it { should be_valid } + + describe '#is_being_reassigned?' do + it 'returns true if the issue assignee has changed' do + subject.assignee = Factory(:user) + subject.is_being_reassigned?.should be_true + end + it 'returns false if the issue assignee has not changed' do + subject.is_being_reassigned?.should be_false + end + end describe "plus 1" do let(:project) { Factory(:project) } From 00ec81eacb881fbe0223183737d9c95b801ab01c Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Sun, 20 May 2012 14:25:34 -0400 Subject: [PATCH 03/13] Update IssueObserver to send reassigned emails when an issue is reassigned. --- app/models/issue_observer.rb | 13 +++--- spec/models/issue_observer_spec.rb | 71 ++++++++++++++++-------------- 2 files changed, 47 insertions(+), 37 deletions(-) diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb index e18195aa..38c4fde9 100644 --- a/app/models/issue_observer.rb +++ b/app/models/issue_observer.rb @@ -6,12 +6,15 @@ class IssueObserver < ActiveRecord::Observer end def after_change(issue) - if issue.assignee_id_changed? - recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } + send_reassigned_email(issue) if issue.is_being_reassigned? + end - recipient_ids.each do |recipient_id| - Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was) - end + def send_reassigned_email(issue) + recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } + + recipient_ids.each do |recipient_id| + Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was) end end + end diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index 166e03ce..7de9a0ae 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' describe IssueObserver do - let(:some_user) { Factory.new(:user, :id => 1) } - let(:assignee) { Factory.new(:user, :id => 2) } - let(:issue) { Factory.new(:issue, :id => 42, :assignee => assignee) } + let(:some_user) { double(:user, :id => 1) } + let(:assignee) { double(:user, :id => 2) } + let(:issue) { double(:issue, :id => 42, :assignee => assignee) } before(:each) { subject.stub(:current_user).and_return(some_user) } @@ -25,42 +25,49 @@ describe IssueObserver do end end - context 'when an issue is modified' do - it 'but not reassigned, does not send a reassigned email' do - issue.stub(:assignee_id_changed?).and_return(false) - Notify.should_not_receive(:reassigned_issue_email) + context 'when an issue is changed' do + it 'sends a reassigned email, if the issue is being reassigned' do + issue.should_receive(:is_being_reassigned?).and_return(true) + subject.should_receive(:send_reassigned_email).with(issue) subject.after_change(issue) end - context 'and is reassigned' do - let(:previous_assignee) { Factory.new(:user, :id => 3) } + it 'does not send a reassigned email, if the issue was not reassigned' do + issue.should_receive(:is_being_reassigned?).and_return(false) + subject.should_not_receive(:send_reassigned_email) - before(:each) do - issue.stub(:assignee_id_changed?).and_return(true) - issue.stub(:assignee_id_was).and_return(previous_assignee.id) + subject.after_change(issue) + end + end + + describe '#send_reassigned_email' do + let(:previous_assignee) { double(:user, :id => 3) } + + before(:each) do + issue.stub(:assignee_id).and_return(assignee.id) + issue.stub(:assignee_id_was).and_return(previous_assignee.id) + end + + it 'sends a reassigned email to the previous and current assignees' do + Notify.should_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) + Notify.should_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) + + subject.send_reassigned_email(issue) + end + + context 'does not send an email to the user who made the reassignment' do + it 'if the user is the assignee' do + subject.stub(:current_user).and_return(assignee) + Notify.should_not_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) + + subject.send_reassigned_email(issue) end + it 'if the user is the previous assignee' do + subject.stub(:current_user).and_return(previous_assignee) + Notify.should_not_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) - it 'sends a reassigned email to the previous and current assignees' do - Notify.should_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) - Notify.should_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) - - subject.after_change(issue) - end - - context 'does not send an email to the user who made the reassignment' do - it 'if the user is the assignee' do - subject.stub(:current_user).and_return(assignee) - Notify.should_not_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) - - subject.after_change(issue) - end - it 'if the user is the previous assignee' do - subject.stub(:current_user).and_return(previous_assignee) - Notify.should_not_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) - - subject.after_change(issue) - end + subject.send_reassigned_email(issue) end end end From 02924de3e1555bcd89097353ffb7eb552113b42e Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Sun, 20 May 2012 14:35:03 -0400 Subject: [PATCH 04/13] Add method to Note to create notes about status changes. --- app/models/note.rb | 8 ++++++++ spec/models/note_spec.rb | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/models/note.rb b/app/models/note.rb index c655eb80..aa034ef4 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -42,6 +42,14 @@ class Note < ActiveRecord::Base mount_uploader :attachment, AttachmentUploader + def self.create_status_change_note(noteable, author, status) + create({ :noteable => noteable, + :project => noteable.project, + :author => author, + :note => "_Status changed to #{status}_" }, + :without_protection => true) + end + def notify @notify ||= false end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index c74f7277..af6ee9b8 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -70,6 +70,25 @@ describe Note do end end + describe '#create_status_change_note' do + let(:project) { Factory.create(:project) } + let(:thing) { Factory.create(:issue, :project => project) } + let(:author) { Factory(:user) } + let(:status) { 'new_status' } + + subject { Note.create_status_change_note(thing, author, status) } + + it 'creates and saves a Note' do + should be_a Note + subject.id.should_not be_nil + end + + its(:noteable) { should == thing } + its(:project) { should == thing.project } + its(:author) { should == author } + its(:note) { should =~ /Status changed to #{status}/ } + end + describe :authorization do before do @p1 = project From 356430c3c0e8aed3f8c9f2e181aaeaeaa4f1d693 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Sun, 20 May 2012 15:06:13 -0400 Subject: [PATCH 05/13] Add method for an issue to know whether it is being closed Update IssueObserver to create a Note on the issue its being closed. --- app/models/issue.rb | 4 +++ app/models/issue_observer.rb | 2 +- spec/models/issue_observer_spec.rb | 41 +++++++++++++++++++++++------- spec/models/issue_spec.rb | 19 ++++++++++++++ 4 files changed, 56 insertions(+), 10 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index f1cb2e22..fd9db264 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -64,6 +64,10 @@ class Issue < ActiveRecord::Base def is_being_reassigned? assignee_id_changed? end + + def is_being_closed? + closed_changed? && closed + end end # == Schema Information # diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb index 38c4fde9..461b3eb0 100644 --- a/app/models/issue_observer.rb +++ b/app/models/issue_observer.rb @@ -7,6 +7,7 @@ class IssueObserver < ActiveRecord::Observer def after_change(issue) send_reassigned_email(issue) if issue.is_being_reassigned? + Note.create_status_change_note(issue, current_user, 'closed') if issue.is_being_closed? end def send_reassigned_email(issue) @@ -16,5 +17,4 @@ class IssueObserver < ActiveRecord::Observer Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was) end end - end diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index 7de9a0ae..fec09488 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -26,18 +26,41 @@ describe IssueObserver do end context 'when an issue is changed' do - it 'sends a reassigned email, if the issue is being reassigned' do - issue.should_receive(:is_being_reassigned?).and_return(true) - subject.should_receive(:send_reassigned_email).with(issue) - - subject.after_change(issue) + before(:each) do + issue.stub(:is_being_reassigned?).and_return(false) + issue.stub(:is_being_closed?).and_return(false) end - it 'does not send a reassigned email, if the issue was not reassigned' do - issue.should_receive(:is_being_reassigned?).and_return(false) - subject.should_not_receive(:send_reassigned_email) + context 'a reassigned email' do + it 'is sent if the issue is being reassigned' do + issue.should_receive(:is_being_reassigned?).and_return(true) + subject.should_receive(:send_reassigned_email).with(issue) - subject.after_change(issue) + subject.after_change(issue) + end + + it 'is not sent if the issue is not being reassigned' do + issue.should_receive(:is_being_reassigned?).and_return(false) + subject.should_not_receive(:send_reassigned_email) + + subject.after_change(issue) + end + end + + context 'a status "closed" note' do + it 'is created if the issue is being closed' do + issue.should_receive(:is_being_closed?).and_return(true) + Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') + + subject.after_change(issue) + end + + it 'is not created if the issue is not being closed' do + issue.should_receive(:is_being_closed?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') + + subject.after_change(issue) + end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 68c05f2e..9668f0b2 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -36,6 +36,25 @@ describe Issue do end end + describe '#is_being_closed?' do + it 'returns true if the closed attribute has changed and is now true' do + subject.closed = true + subject.is_being_closed?.should be_true + end + it 'returns false if the closed attribute has changed and is now false' do + issue = Factory.create(:issue, + :closed => true, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) + issue.closed = false + issue.is_being_closed?.should be_false + end + it 'returns false if the closed attribute has not changed' do + subject.is_being_closed?.should be_false + end + end + describe "plus 1" do let(:project) { Factory(:project) } subject { From 6617eaaf9b9ff5a76cb2c4150623a685357966d4 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Mon, 21 May 2012 13:30:53 -0400 Subject: [PATCH 06/13] Make IssueObserver handle issus, not MailerObserver --- app/models/issue.rb | 4 +++ app/models/issue_observer.rb | 3 ++- app/models/mailer_observer.rb | 14 +--------- config/application.rb | 2 +- spec/models/issue_observer_spec.rb | 41 +++++++++++++++++++++++++----- spec/models/issue_spec.rb | 21 +++++++++++++++ 6 files changed, 64 insertions(+), 21 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index fd9db264..2b4b311d 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -68,6 +68,10 @@ class Issue < ActiveRecord::Base def is_being_closed? closed_changed? && closed end + + def is_being_reopened? + closed_changed? && !closed + end end # == Schema Information # diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb index 461b3eb0..7215dc56 100644 --- a/app/models/issue_observer.rb +++ b/app/models/issue_observer.rb @@ -5,9 +5,10 @@ class IssueObserver < ActiveRecord::Observer Notify.new_issue_email(issue.id) if issue.assignee != current_user end - def after_change(issue) + def after_update(issue) send_reassigned_email(issue) if issue.is_being_reassigned? Note.create_status_change_note(issue, current_user, 'closed') if issue.is_being_closed? + Note.create_status_change_note(issue, current_user, 'reopened') if issue.is_being_reopened? end def send_reassigned_email(issue) diff --git a/app/models/mailer_observer.rb b/app/models/mailer_observer.rb index dbbd7d2f..80ce8b36 100644 --- a/app/models/mailer_observer.rb +++ b/app/models/mailer_observer.rb @@ -3,7 +3,6 @@ class MailerObserver < ActiveRecord::Observer cattr_accessor :current_user def after_create(model) - new_issue(model) if model.kind_of?(Issue) new_user(model) if model.kind_of?(User) new_note(model) if model.kind_of?(Note) new_merge_request(model) if model.kind_of?(MergeRequest) @@ -11,17 +10,10 @@ class MailerObserver < ActiveRecord::Observer def after_update(model) changed_merge_request(model) if model.kind_of?(MergeRequest) - changed_issue(model) if model.kind_of?(Issue) end protected - def new_issue(issue) - if issue.assignee != current_user - Notify.new_issue_email(issue.id).deliver - end - end - def new_user(user) Notify.new_user_email(user.id, user.password).deliver end @@ -65,12 +57,8 @@ class MailerObserver < ActiveRecord::Observer status_notify_and_comment merge_request, :reassigned_merge_request_email end - def changed_issue(issue) - status_notify_and_comment issue, :reassigned_issue_email - end - # This method used for Issues & Merge Requests - # + # # It create a comment for Issue or MR if someone close/reopen. # It also notify via email if assignee was changed # diff --git a/config/application.rb b/config/application.rb index 7bd5703b..4531b5ea 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,7 +23,7 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer + config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index fec09488..42cf81c5 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -9,7 +9,12 @@ describe IssueObserver do subject { IssueObserver.instance } - context 'when an issue is created' do + describe '#after_create' do + + it 'is called when an issue is created' do + subject.should_receive(:after_create) + Factory.create(:issue, :project => Factory.create(:project)) + end it 'sends an email to the assignee' do Notify.should_receive(:new_issue_email).with(issue.id) @@ -25,10 +30,18 @@ describe IssueObserver do end end - context 'when an issue is changed' do + context '#after_update' do before(:each) do issue.stub(:is_being_reassigned?).and_return(false) issue.stub(:is_being_closed?).and_return(false) + issue.stub(:is_being_reopened?).and_return(false) + end + + it 'is called when an issue is changed' do + changed = Factory.create(:issue, :project => Factory.create(:project)) + subject.should_receive(:after_update) + changed.description = 'I changed' + changed.save end context 'a reassigned email' do @@ -36,14 +49,14 @@ describe IssueObserver do issue.should_receive(:is_being_reassigned?).and_return(true) subject.should_receive(:send_reassigned_email).with(issue) - subject.after_change(issue) + subject.after_update(issue) end it 'is not sent if the issue is not being reassigned' do issue.should_receive(:is_being_reassigned?).and_return(false) subject.should_not_receive(:send_reassigned_email) - subject.after_change(issue) + subject.after_update(issue) end end @@ -52,14 +65,30 @@ describe IssueObserver do issue.should_receive(:is_being_closed?).and_return(true) Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') - subject.after_change(issue) + subject.after_update(issue) end it 'is not created if the issue is not being closed' do issue.should_receive(:is_being_closed?).and_return(false) Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') - subject.after_change(issue) + subject.after_update(issue) + end + end + + context 'a status "reopened" note' do + it 'is created if the issue is being reopened' do + issue.should_receive(:is_being_reopened?).and_return(true) + Note.should_receive(:create_status_change_note).with(issue, some_user, 'reopened') + + subject.after_update(issue) + end + + it 'is not created if the issue is not being reopened' do + issue.should_receive(:is_being_reopened?).and_return(false) + Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'reopened') + + subject.after_update(issue) end end end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 9668f0b2..fd3af62d 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -55,6 +55,26 @@ describe Issue do end end + + describe '#is_being_reopened?' do + it 'returns true if the closed attribute has changed and is now false' do + issue = Factory.create(:issue, + :closed => true, + :author => Factory(:user), + :assignee => Factory(:user), + :project => Factory.create(:project)) + issue.closed = false + issue.is_being_reopened?.should be_true + end + it 'returns false if the closed attribute has changed and is now true' do + subject.closed = true + subject.is_being_reopened?.should be_false + end + it 'returns false if the closed attribute has not changed' do + subject.is_being_reopened?.should be_false + end + end + describe "plus 1" do let(:project) { Factory(:project) } subject { @@ -86,6 +106,7 @@ describe Issue do subject.upvotes.should == 2 end end + end # == Schema Information # From 5303cc285a2067b59f1e8b68f707e8dbf90fe59e Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Mon, 21 May 2012 16:05:12 -0400 Subject: [PATCH 07/13] Add resque_spec to test queuing mail. --- Gemfile | 1 + Gemfile.lock | 4 ++++ config/initializers/resque_mailer.rb | 1 + spec/requests/admin/admin_projects_spec.rb | 1 + spec/requests/admin/admin_users_spec.rb | 4 +++- spec/requests/issues_spec.rb | 4 +++- 6 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 config/initializers/resque_mailer.rb diff --git a/Gemfile b/Gemfile index 72081416..98168dbd 100644 --- a/Gemfile +++ b/Gemfile @@ -77,4 +77,5 @@ group :test do gem "simplecov", :require => false gem "shoulda-matchers" gem 'email_spec' + gem 'resque_spec' end diff --git a/Gemfile.lock b/Gemfile.lock index bdf78c46..41cb556b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -263,6 +263,9 @@ GEM resque_mailer (2.0.3) actionmailer (>= 3.0.0) resque (>= 1.2.3) + resque_spec (0.11.0) + resque (>= 1.19.0) + rspec (>= 2.5.0) rspec (2.10.0) rspec-core (~> 2.10.0) rspec-expectations (~> 2.10.0) @@ -391,6 +394,7 @@ DEPENDENCIES redcarpet (~> 2.1.1) resque (~> 1.20.0) resque_mailer + resque_spec rspec-rails sass-rails (= 3.2.5) seed-fu diff --git a/config/initializers/resque_mailer.rb b/config/initializers/resque_mailer.rb new file mode 100644 index 00000000..cec9dec9 --- /dev/null +++ b/config/initializers/resque_mailer.rb @@ -0,0 +1 @@ +Resque::Mailer.excluded_environments = [] diff --git a/spec/requests/admin/admin_projects_spec.rb b/spec/requests/admin/admin_projects_spec.rb index 9a33c693..fb6577de 100644 --- a/spec/requests/admin/admin_projects_spec.rb +++ b/spec/requests/admin/admin_projects_spec.rb @@ -88,6 +88,7 @@ describe "Admin::Projects" do fill_in 'Name', :with => 'NewProject' fill_in 'Code', :with => 'NPR' fill_in 'Path', :with => 'gitlabhq_1' + fill_in 'Description', :with => 'New Project Description' expect { click_button "Save" }.to change { Project.count }.by(1) @project = Project.last end diff --git a/spec/requests/admin/admin_users_spec.rb b/spec/requests/admin/admin_users_spec.rb index 91082a64..c98ed2cf 100644 --- a/spec/requests/admin/admin_users_spec.rb +++ b/spec/requests/admin/admin_users_spec.rb @@ -45,7 +45,9 @@ describe "Admin::Users" do end it "should send valid email to user with email & password" do - click_button "Save" + with_resque do + click_button "Save" + end user = User.last email = ActionMailer::Base.deliveries.last email.subject.should have_content("Account was created") diff --git a/spec/requests/issues_spec.rb b/spec/requests/issues_spec.rb index 5c59675b..aa43b997 100644 --- a/spec/requests/issues_spec.rb +++ b/spec/requests/issues_spec.rb @@ -133,7 +133,9 @@ describe "Issues" do end it "should send valid email to user" do - click_button "Submit new issue" + with_resque do + click_button "Submit new issue" + end issue = Issue.last email = ActionMailer::Base.deliveries.last email.subject.should have_content("New Issue was created") From dfb5da9da339096ad0a8e754f4f42ca2007b5ae6 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Tue, 12 Jun 2012 14:27:03 -0400 Subject: [PATCH 08/13] Disable observers in specs. Enable only when observer is under test. Used the built-in observer enable/disable feature in ActiveModel[1]. ActiveRecord::Base includes ActiveModel::Observing which provides this behavior. Simple wraps to enable the observer under test were added to the specs for: ActivityObserver, IssueObserver, Admin::Users and Issues. The spec for Project.last_activity was refactored to separate the tests for #last_activity and #last_activity_date. Each had doubles added to isolate the spec from the hidden dependency on the ActivityObserver action to create an Event for the project when an Issue is created. This ActivityObserver behavior is already tested by its spec. [1] http://api.rubyonrails.org/classes/ActiveModel/ObserverArray.html --- spec/models/activity_observer_spec.rb | 16 ++++++++++------ spec/models/issue_observer_spec.rb | 12 +++++++++--- spec/models/project_spec.rb | 21 +++++++++++++++++---- spec/requests/admin/admin_users_spec.rb | 22 +++++++++++++--------- spec/requests/issues_spec.rb | 20 ++++++++++++-------- spec/spec_helper.rb | 1 + 6 files changed, 62 insertions(+), 30 deletions(-) diff --git a/spec/models/activity_observer_spec.rb b/spec/models/activity_observer_spec.rb index 91c7d914..aed1b26d 100644 --- a/spec/models/activity_observer_spec.rb +++ b/spec/models/activity_observer_spec.rb @@ -9,9 +9,11 @@ describe ActivityObserver do end describe "Merge Request created" do - before do - @merge_request = Factory :merge_request, :project => project - @event = Event.last + before do + MergeRequest.observers.enable :activity_observer do + @merge_request = Factory :merge_request, :project => project + @event = Event.last + end end it_should_be_valid_event @@ -20,9 +22,11 @@ describe ActivityObserver do end describe "Issue created" do - before do - @issue = Factory :issue, :project => project - @event = Event.last + before do + Issue.observers.enable :activity_observer do + @issue = Factory :issue, :project => project + @event = Event.last + end end it_should_be_valid_event diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index 42cf81c5..b66803e6 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -13,7 +13,10 @@ describe IssueObserver do it 'is called when an issue is created' do subject.should_receive(:after_create) - Factory.create(:issue, :project => Factory.create(:project)) + + Issue.observers.enable :issue_observer do + Factory.create(:issue, :project => Factory.create(:project)) + end end it 'sends an email to the assignee' do @@ -40,8 +43,11 @@ describe IssueObserver do it 'is called when an issue is changed' do changed = Factory.create(:issue, :project => Factory.create(:project)) subject.should_receive(:after_update) - changed.description = 'I changed' - changed.save + + Issue.observers.enable :issue_observer do + changed.description = 'I changed' + changed.save + end end context 'a reassigned email' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6285a852..d28668b2 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -72,16 +72,29 @@ describe Project do end describe "last_activity" do - let(:project) { Factory :project } + let(:project) { Factory :project } + let(:last_event) { double } before do - @issue = Factory :issue, :project => project + project.stub(:events).and_return( [ double, double, last_event ] ) end - it { project.last_activity.should == Event.last } - it { project.last_activity_date.to_s.should == Event.last.created_at.to_s } + it { project.last_activity.should == last_event } end + describe 'last_activity_date' do + let(:project) { Factory :project } + + it 'returns the creation date of the project\'s last event if present' do + last_event = double(:created_at => 'now') + project.stub(:events).and_return( [double, double, last_event] ) + project.last_activity_date.should == last_event.created_at + end + + it 'returns the project\'s last update date if it has no events' do + project.last_activity_date.should == project.updated_at + end + end describe "fresh commits" do let(:project) { Factory :project } diff --git a/spec/requests/admin/admin_users_spec.rb b/spec/requests/admin/admin_users_spec.rb index c98ed2cf..d9c3472d 100644 --- a/spec/requests/admin/admin_users_spec.rb +++ b/spec/requests/admin/admin_users_spec.rb @@ -40,19 +40,23 @@ describe "Admin::Users" do end it "should call send mail" do - Notify.should_receive(:new_user_email).and_return(stub(:deliver => true)) - click_button "Save" + User.observers.enable :mailer_observer do + Notify.should_receive(:new_user_email).and_return(stub(:deliver => true)) + click_button "Save" + end end it "should send valid email to user with email & password" do - with_resque do - click_button "Save" + User.observers.enable :mailer_observer do + with_resque do + click_button "Save" + end + user = User.last + email = ActionMailer::Base.deliveries.last + email.subject.should have_content("Account was created") + email.body.should have_content(user.email) + email.body.should have_content(@password) end - user = User.last - email = ActionMailer::Base.deliveries.last - email.subject.should have_content("Account was created") - email.body.should have_content(user.email) - email.body.should have_content(@password) end end diff --git a/spec/requests/issues_spec.rb b/spec/requests/issues_spec.rb index aa43b997..2c8650a8 100644 --- a/spec/requests/issues_spec.rb +++ b/spec/requests/issues_spec.rb @@ -128,18 +128,22 @@ describe "Issues" do end it "should call send mail" do - Notify.should_receive(:new_issue_email).and_return(stub(:deliver => true)) - click_button "Submit new issue" + Issue.observers.enable :issue_observer do + Notify.should_receive(:new_issue_email).and_return(stub(:deliver => true)) + click_button "Submit new issue" + end end it "should send valid email to user" do - with_resque do - click_button "Submit new issue" + Issue.observers.enable :issue_observer do + with_resque do + click_button "Submit new issue" + end + issue = Issue.last + email = ActionMailer::Base.deliveries.last + email.subject.should have_content("New Issue was created") + email.body.should have_content(issue.title) end - issue = Issue.last - email = ActionMailer::Base.deliveries.last - email.subject.should have_content("New Issue was created") - email.body.should have_content(issue.title) end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 18b7854d..5556798f 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -52,6 +52,7 @@ RSpec.configure do |config| DatabaseCleaner.start WebMock.disable_net_connect!(allow_localhost: true) + ActiveRecord::Base.observers.disable :all end config.after do From 97ca4f5ddadd9f6880e238e85af00a82dcd8807f Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Fri, 15 Jun 2012 16:23:28 -0400 Subject: [PATCH 09/13] Deliver issue mails. It helps to actually deliver messages. --- app/models/issue_observer.rb | 4 ++-- spec/models/issue_observer_spec.rb | 22 +++++++++++++++++----- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb index 7215dc56..a1f9fade 100644 --- a/app/models/issue_observer.rb +++ b/app/models/issue_observer.rb @@ -2,7 +2,7 @@ class IssueObserver < ActiveRecord::Observer cattr_accessor :current_user def after_create(issue) - Notify.new_issue_email(issue.id) if issue.assignee != current_user + Notify.new_issue_email(issue.id).deliver if issue.assignee != current_user end def after_update(issue) @@ -15,7 +15,7 @@ class IssueObserver < ActiveRecord::Observer recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } recipient_ids.each do |recipient_id| - Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was) + Notify.reassigned_issue_email(recipient_id, issue.id, issue.assignee_id_was).deliver end end end diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index b66803e6..8270d3db 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -20,7 +20,8 @@ describe IssueObserver do end it 'sends an email to the assignee' do - Notify.should_receive(:new_issue_email).with(issue.id) + Notify.should_receive(:new_issue_email).with(issue.id). + and_return(double(:deliver => true)) subject.after_create(issue) end @@ -107,9 +108,18 @@ describe IssueObserver do issue.stub(:assignee_id_was).and_return(previous_assignee.id) end + def it_sends_a_reassigned_email_to(recipient) + Notify.should_receive(:reassigned_issue_email).with(recipient, issue.id, previous_assignee.id). + and_return(double(:deliver => true)) + end + + def it_does_not_send_a_reassigned_email_to(recipient) + Notify.should_not_receive(:reassigned_issue_email).with(recipient, issue.id, previous_assignee.id) + end + it 'sends a reassigned email to the previous and current assignees' do - Notify.should_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) - Notify.should_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) + it_sends_a_reassigned_email_to assignee.id + it_sends_a_reassigned_email_to previous_assignee.id subject.send_reassigned_email(issue) end @@ -117,13 +127,15 @@ describe IssueObserver do context 'does not send an email to the user who made the reassignment' do it 'if the user is the assignee' do subject.stub(:current_user).and_return(assignee) - Notify.should_not_receive(:reassigned_issue_email).with(assignee.id, issue.id, previous_assignee.id) + it_sends_a_reassigned_email_to previous_assignee.id + it_does_not_send_a_reassigned_email_to assignee.id subject.send_reassigned_email(issue) end it 'if the user is the previous assignee' do subject.stub(:current_user).and_return(previous_assignee) - Notify.should_not_receive(:reassigned_issue_email).with(previous_assignee.id, issue.id, previous_assignee.id) + it_sends_a_reassigned_email_to assignee.id + it_does_not_send_a_reassigned_email_to previous_assignee.id subject.send_reassigned_email(issue) end From 88964132edaec721e256449579c032ca731781bc Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Wed, 20 Jun 2012 12:29:10 -0400 Subject: [PATCH 10/13] Extract observation of User to a UserObserver --- app/models/mailer_observer.rb | 7 +------ app/models/user_observer.rb | 5 +++++ config/application.rb | 2 +- spec/models/user_observer_spec.rb | 26 +++++++++++++++++++++++++ spec/requests/admin/admin_users_spec.rb | 7 ++++--- 5 files changed, 37 insertions(+), 10 deletions(-) create mode 100644 app/models/user_observer.rb create mode 100644 spec/models/user_observer_spec.rb diff --git a/app/models/mailer_observer.rb b/app/models/mailer_observer.rb index 80ce8b36..b562d05a 100644 --- a/app/models/mailer_observer.rb +++ b/app/models/mailer_observer.rb @@ -1,9 +1,8 @@ class MailerObserver < ActiveRecord::Observer - observe :issue, :user, :note, :merge_request + observe :issue, :note, :merge_request cattr_accessor :current_user def after_create(model) - new_user(model) if model.kind_of?(User) new_note(model) if model.kind_of?(Note) new_merge_request(model) if model.kind_of?(MergeRequest) end @@ -14,10 +13,6 @@ class MailerObserver < ActiveRecord::Observer protected - def new_user(user) - Notify.new_user_email(user.id, user.password).deliver - end - def new_note(note) if note.notify # Notify whole team except author of note diff --git a/app/models/user_observer.rb b/app/models/user_observer.rb new file mode 100644 index 00000000..d12bcc99 --- /dev/null +++ b/app/models/user_observer.rb @@ -0,0 +1,5 @@ +class UserObserver < ActiveRecord::Observer + def after_create(user) + Notify.new_user_email(user.id, user.password).deliver + end +end diff --git a/config/application.rb b/config/application.rb index 4531b5ea..6242b655 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,7 +23,7 @@ module Gitlab # config.plugins = [ :exception_notification, :ssl_requirement, :all ] # Activate observers that should always be running. - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer + config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. diff --git a/spec/models/user_observer_spec.rb b/spec/models/user_observer_spec.rb new file mode 100644 index 00000000..23dac98b --- /dev/null +++ b/spec/models/user_observer_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe UserObserver do + subject { UserObserver.instance } + + it 'calls #after_create when new users are created' do + new_user = Factory.new(:user) + subject.should_receive(:after_create).with(new_user) + + User.observers.enable :user_observer do + new_user.save + end + end + + context 'when a new user is created' do + let(:user) { double(:user, id: 42, password: 'P@ssword!') } + let(:notification) { double :notification } + + it 'sends an email' do + notification.should_receive(:deliver) + Notify.should_receive(:new_user_email).with(user.id, user.password).and_return(notification) + + subject.after_create(user) + end + end +end diff --git a/spec/requests/admin/admin_users_spec.rb b/spec/requests/admin/admin_users_spec.rb index d9c3472d..ba6831e3 100644 --- a/spec/requests/admin/admin_users_spec.rb +++ b/spec/requests/admin/admin_users_spec.rb @@ -40,14 +40,15 @@ describe "Admin::Users" do end it "should call send mail" do - User.observers.enable :mailer_observer do - Notify.should_receive(:new_user_email).and_return(stub(:deliver => true)) + Notify.should_receive(:new_user_email).and_return(stub(:deliver => true)) + + User.observers.enable :user_observer do click_button "Save" end end it "should send valid email to user with email & password" do - User.observers.enable :mailer_observer do + User.observers.enable :user_observer do with_resque do click_button "Save" end From eba61c111d175f298d7c6541f47a6dcdf3e55b7d Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Wed, 20 Jun 2012 13:18:29 -0400 Subject: [PATCH 11/13] Remove upvotes method from Issue. Must have snuck in during one of the multiple rebases while observers were being refactored. --- app/models/issue.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 2b4b311d..844d418b 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -56,11 +56,6 @@ class Issue < ActiveRecord::Base today? && created_at == updated_at end - # Return the number of +1 comments (upvotes) - def upvotes - notes.select(&:upvote?).size - end - def is_being_reassigned? assignee_id_changed? end From 70c6b48ebc124fba754d940ba11bafadbf86e101 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Wed, 20 Jun 2012 13:48:34 -0400 Subject: [PATCH 12/13] Remove :issue from MailerObserver; handled by IssueObserver now. :issue snuck back in during rebasing. --- app/models/mailer_observer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/mailer_observer.rb b/app/models/mailer_observer.rb index b562d05a..880fd502 100644 --- a/app/models/mailer_observer.rb +++ b/app/models/mailer_observer.rb @@ -1,5 +1,5 @@ class MailerObserver < ActiveRecord::Observer - observe :issue, :note, :merge_request + observe :note, :merge_request cattr_accessor :current_user def after_create(model) From 65989141dcc58eaa3cecfe98bc97a10075cf4f7e Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Wed, 20 Jun 2012 21:23:05 -0400 Subject: [PATCH 13/13] Protect IssueObserver#send_reassigned_email method. --- app/models/issue_observer.rb | 2 ++ spec/models/issue_observer_spec.rb | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/issue_observer.rb b/app/models/issue_observer.rb index a1f9fade..fadedd30 100644 --- a/app/models/issue_observer.rb +++ b/app/models/issue_observer.rb @@ -11,6 +11,8 @@ class IssueObserver < ActiveRecord::Observer Note.create_status_change_note(issue, current_user, 'reopened') if issue.is_being_reopened? end + protected + def send_reassigned_email(issue) recipient_ids = [issue.assignee_id, issue.assignee_id_was].keep_if {|id| id != current_user.id } diff --git a/spec/models/issue_observer_spec.rb b/spec/models/issue_observer_spec.rb index 8270d3db..2b9798f7 100644 --- a/spec/models/issue_observer_spec.rb +++ b/spec/models/issue_observer_spec.rb @@ -121,7 +121,7 @@ describe IssueObserver do it_sends_a_reassigned_email_to assignee.id it_sends_a_reassigned_email_to previous_assignee.id - subject.send_reassigned_email(issue) + subject.send(:send_reassigned_email, issue) end context 'does not send an email to the user who made the reassignment' do @@ -130,14 +130,14 @@ describe IssueObserver do it_sends_a_reassigned_email_to previous_assignee.id it_does_not_send_a_reassigned_email_to assignee.id - subject.send_reassigned_email(issue) + subject.send(:send_reassigned_email, issue) end it 'if the user is the previous assignee' do subject.stub(:current_user).and_return(previous_assignee) it_sends_a_reassigned_email_to assignee.id it_does_not_send_a_reassigned_email_to previous_assignee.id - subject.send_reassigned_email(issue) + subject.send(:send_reassigned_email, issue) end end end