Commit 6617eaaf9b9ff5a76cb2c4150623a685357966d4
1 parent
356430c3
Exists in
master
and in
4 other branches
Make IssueObserver handle issus, not MailerObserver
Showing
6 changed files
with
64 additions
and
21 deletions
Show diff stats
app/models/issue.rb
| @@ -68,6 +68,10 @@ class Issue < ActiveRecord::Base | @@ -68,6 +68,10 @@ class Issue < ActiveRecord::Base | ||
| 68 | def is_being_closed? | 68 | def is_being_closed? |
| 69 | closed_changed? && closed | 69 | closed_changed? && closed |
| 70 | end | 70 | end |
| 71 | + | ||
| 72 | + def is_being_reopened? | ||
| 73 | + closed_changed? && !closed | ||
| 74 | + end | ||
| 71 | end | 75 | end |
| 72 | # == Schema Information | 76 | # == Schema Information |
| 73 | # | 77 | # |
app/models/issue_observer.rb
| @@ -5,9 +5,10 @@ class IssueObserver < ActiveRecord::Observer | @@ -5,9 +5,10 @@ class IssueObserver < ActiveRecord::Observer | ||
| 5 | Notify.new_issue_email(issue.id) if issue.assignee != current_user | 5 | Notify.new_issue_email(issue.id) if issue.assignee != current_user |
| 6 | end | 6 | end |
| 7 | 7 | ||
| 8 | - def after_change(issue) | 8 | + def after_update(issue) |
| 9 | send_reassigned_email(issue) if issue.is_being_reassigned? | 9 | send_reassigned_email(issue) if issue.is_being_reassigned? |
| 10 | Note.create_status_change_note(issue, current_user, 'closed') if issue.is_being_closed? | 10 | Note.create_status_change_note(issue, current_user, 'closed') if issue.is_being_closed? |
| 11 | + Note.create_status_change_note(issue, current_user, 'reopened') if issue.is_being_reopened? | ||
| 11 | end | 12 | end |
| 12 | 13 | ||
| 13 | def send_reassigned_email(issue) | 14 | def send_reassigned_email(issue) |
app/models/mailer_observer.rb
| @@ -3,7 +3,6 @@ class MailerObserver < ActiveRecord::Observer | @@ -3,7 +3,6 @@ class MailerObserver < ActiveRecord::Observer | ||
| 3 | cattr_accessor :current_user | 3 | cattr_accessor :current_user |
| 4 | 4 | ||
| 5 | def after_create(model) | 5 | def after_create(model) |
| 6 | - new_issue(model) if model.kind_of?(Issue) | ||
| 7 | new_user(model) if model.kind_of?(User) | 6 | new_user(model) if model.kind_of?(User) |
| 8 | new_note(model) if model.kind_of?(Note) | 7 | new_note(model) if model.kind_of?(Note) |
| 9 | new_merge_request(model) if model.kind_of?(MergeRequest) | 8 | new_merge_request(model) if model.kind_of?(MergeRequest) |
| @@ -11,17 +10,10 @@ class MailerObserver < ActiveRecord::Observer | @@ -11,17 +10,10 @@ class MailerObserver < ActiveRecord::Observer | ||
| 11 | 10 | ||
| 12 | def after_update(model) | 11 | def after_update(model) |
| 13 | changed_merge_request(model) if model.kind_of?(MergeRequest) | 12 | changed_merge_request(model) if model.kind_of?(MergeRequest) |
| 14 | - changed_issue(model) if model.kind_of?(Issue) | ||
| 15 | end | 13 | end |
| 16 | 14 | ||
| 17 | protected | 15 | protected |
| 18 | 16 | ||
| 19 | - def new_issue(issue) | ||
| 20 | - if issue.assignee != current_user | ||
| 21 | - Notify.new_issue_email(issue.id).deliver | ||
| 22 | - end | ||
| 23 | - end | ||
| 24 | - | ||
| 25 | def new_user(user) | 17 | def new_user(user) |
| 26 | Notify.new_user_email(user.id, user.password).deliver | 18 | Notify.new_user_email(user.id, user.password).deliver |
| 27 | end | 19 | end |
| @@ -65,12 +57,8 @@ class MailerObserver < ActiveRecord::Observer | @@ -65,12 +57,8 @@ class MailerObserver < ActiveRecord::Observer | ||
| 65 | status_notify_and_comment merge_request, :reassigned_merge_request_email | 57 | status_notify_and_comment merge_request, :reassigned_merge_request_email |
| 66 | end | 58 | end |
| 67 | 59 | ||
| 68 | - def changed_issue(issue) | ||
| 69 | - status_notify_and_comment issue, :reassigned_issue_email | ||
| 70 | - end | ||
| 71 | - | ||
| 72 | # This method used for Issues & Merge Requests | 60 | # This method used for Issues & Merge Requests |
| 73 | - # | 61 | + # |
| 74 | # It create a comment for Issue or MR if someone close/reopen. | 62 | # It create a comment for Issue or MR if someone close/reopen. |
| 75 | # It also notify via email if assignee was changed | 63 | # It also notify via email if assignee was changed |
| 76 | # | 64 | # |
config/application.rb
| @@ -23,7 +23,7 @@ module Gitlab | @@ -23,7 +23,7 @@ module Gitlab | ||
| 23 | # config.plugins = [ :exception_notification, :ssl_requirement, :all ] | 23 | # config.plugins = [ :exception_notification, :ssl_requirement, :all ] |
| 24 | 24 | ||
| 25 | # Activate observers that should always be running. | 25 | # Activate observers that should always be running. |
| 26 | - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer | 26 | + config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer |
| 27 | 27 | ||
| 28 | # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. | 28 | # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone. |
| 29 | # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. | 29 | # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC. |
spec/models/issue_observer_spec.rb
| @@ -9,7 +9,12 @@ describe IssueObserver do | @@ -9,7 +9,12 @@ describe IssueObserver do | ||
| 9 | 9 | ||
| 10 | subject { IssueObserver.instance } | 10 | subject { IssueObserver.instance } |
| 11 | 11 | ||
| 12 | - context 'when an issue is created' do | 12 | + describe '#after_create' do |
| 13 | + | ||
| 14 | + it 'is called when an issue is created' do | ||
| 15 | + subject.should_receive(:after_create) | ||
| 16 | + Factory.create(:issue, :project => Factory.create(:project)) | ||
| 17 | + end | ||
| 13 | 18 | ||
| 14 | it 'sends an email to the assignee' do | 19 | it 'sends an email to the assignee' do |
| 15 | Notify.should_receive(:new_issue_email).with(issue.id) | 20 | Notify.should_receive(:new_issue_email).with(issue.id) |
| @@ -25,10 +30,18 @@ describe IssueObserver do | @@ -25,10 +30,18 @@ describe IssueObserver do | ||
| 25 | end | 30 | end |
| 26 | end | 31 | end |
| 27 | 32 | ||
| 28 | - context 'when an issue is changed' do | 33 | + context '#after_update' do |
| 29 | before(:each) do | 34 | before(:each) do |
| 30 | issue.stub(:is_being_reassigned?).and_return(false) | 35 | issue.stub(:is_being_reassigned?).and_return(false) |
| 31 | issue.stub(:is_being_closed?).and_return(false) | 36 | issue.stub(:is_being_closed?).and_return(false) |
| 37 | + issue.stub(:is_being_reopened?).and_return(false) | ||
| 38 | + end | ||
| 39 | + | ||
| 40 | + it 'is called when an issue is changed' do | ||
| 41 | + changed = Factory.create(:issue, :project => Factory.create(:project)) | ||
| 42 | + subject.should_receive(:after_update) | ||
| 43 | + changed.description = 'I changed' | ||
| 44 | + changed.save | ||
| 32 | end | 45 | end |
| 33 | 46 | ||
| 34 | context 'a reassigned email' do | 47 | context 'a reassigned email' do |
| @@ -36,14 +49,14 @@ describe IssueObserver do | @@ -36,14 +49,14 @@ describe IssueObserver do | ||
| 36 | issue.should_receive(:is_being_reassigned?).and_return(true) | 49 | issue.should_receive(:is_being_reassigned?).and_return(true) |
| 37 | subject.should_receive(:send_reassigned_email).with(issue) | 50 | subject.should_receive(:send_reassigned_email).with(issue) |
| 38 | 51 | ||
| 39 | - subject.after_change(issue) | 52 | + subject.after_update(issue) |
| 40 | end | 53 | end |
| 41 | 54 | ||
| 42 | it 'is not sent if the issue is not being reassigned' do | 55 | it 'is not sent if the issue is not being reassigned' do |
| 43 | issue.should_receive(:is_being_reassigned?).and_return(false) | 56 | issue.should_receive(:is_being_reassigned?).and_return(false) |
| 44 | subject.should_not_receive(:send_reassigned_email) | 57 | subject.should_not_receive(:send_reassigned_email) |
| 45 | 58 | ||
| 46 | - subject.after_change(issue) | 59 | + subject.after_update(issue) |
| 47 | end | 60 | end |
| 48 | end | 61 | end |
| 49 | 62 | ||
| @@ -52,14 +65,30 @@ describe IssueObserver do | @@ -52,14 +65,30 @@ describe IssueObserver do | ||
| 52 | issue.should_receive(:is_being_closed?).and_return(true) | 65 | issue.should_receive(:is_being_closed?).and_return(true) |
| 53 | Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') | 66 | Note.should_receive(:create_status_change_note).with(issue, some_user, 'closed') |
| 54 | 67 | ||
| 55 | - subject.after_change(issue) | 68 | + subject.after_update(issue) |
| 56 | end | 69 | end |
| 57 | 70 | ||
| 58 | it 'is not created if the issue is not being closed' do | 71 | it 'is not created if the issue is not being closed' do |
| 59 | issue.should_receive(:is_being_closed?).and_return(false) | 72 | issue.should_receive(:is_being_closed?).and_return(false) |
| 60 | Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') | 73 | Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'closed') |
| 61 | 74 | ||
| 62 | - subject.after_change(issue) | 75 | + subject.after_update(issue) |
| 76 | + end | ||
| 77 | + end | ||
| 78 | + | ||
| 79 | + context 'a status "reopened" note' do | ||
| 80 | + it 'is created if the issue is being reopened' do | ||
| 81 | + issue.should_receive(:is_being_reopened?).and_return(true) | ||
| 82 | + Note.should_receive(:create_status_change_note).with(issue, some_user, 'reopened') | ||
| 83 | + | ||
| 84 | + subject.after_update(issue) | ||
| 85 | + end | ||
| 86 | + | ||
| 87 | + it 'is not created if the issue is not being reopened' do | ||
| 88 | + issue.should_receive(:is_being_reopened?).and_return(false) | ||
| 89 | + Note.should_not_receive(:create_status_change_note).with(issue, some_user, 'reopened') | ||
| 90 | + | ||
| 91 | + subject.after_update(issue) | ||
| 63 | end | 92 | end |
| 64 | end | 93 | end |
| 65 | end | 94 | end |
spec/models/issue_spec.rb
| @@ -55,6 +55,26 @@ describe Issue do | @@ -55,6 +55,26 @@ describe Issue do | ||
| 55 | end | 55 | end |
| 56 | end | 56 | end |
| 57 | 57 | ||
| 58 | + | ||
| 59 | + describe '#is_being_reopened?' do | ||
| 60 | + it 'returns true if the closed attribute has changed and is now false' do | ||
| 61 | + issue = Factory.create(:issue, | ||
| 62 | + :closed => true, | ||
| 63 | + :author => Factory(:user), | ||
| 64 | + :assignee => Factory(:user), | ||
| 65 | + :project => Factory.create(:project)) | ||
| 66 | + issue.closed = false | ||
| 67 | + issue.is_being_reopened?.should be_true | ||
| 68 | + end | ||
| 69 | + it 'returns false if the closed attribute has changed and is now true' do | ||
| 70 | + subject.closed = true | ||
| 71 | + subject.is_being_reopened?.should be_false | ||
| 72 | + end | ||
| 73 | + it 'returns false if the closed attribute has not changed' do | ||
| 74 | + subject.is_being_reopened?.should be_false | ||
| 75 | + end | ||
| 76 | + end | ||
| 77 | + | ||
| 58 | describe "plus 1" do | 78 | describe "plus 1" do |
| 59 | let(:project) { Factory(:project) } | 79 | let(:project) { Factory(:project) } |
| 60 | subject { | 80 | subject { |
| @@ -86,6 +106,7 @@ describe Issue do | @@ -86,6 +106,7 @@ describe Issue do | ||
| 86 | subject.upvotes.should == 2 | 106 | subject.upvotes.should == 2 |
| 87 | end | 107 | end |
| 88 | end | 108 | end |
| 109 | + | ||
| 89 | end | 110 | end |
| 90 | # == Schema Information | 111 | # == Schema Information |
| 91 | # | 112 | # |