From 6617eaaf9b9ff5a76cb2c4150623a685357966d4 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Mon, 21 May 2012 13:30:53 -0400 Subject: [PATCH] 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 fd9db264326..2b4b311da78 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 461b3eb0dfe..7215dc56332 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 dbbd7d2f86a..80ce8b365e2 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 7bd5703b31a..4531b5ead6c 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 fec09488d89..42cf81c5fff 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 9668f0b23f2..fd3af62d74d 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 #