Download as
Browse Code »

Commit 16ceae895e32c2474de04c42307d914bf9a4c304

Authored by Robb Kidd
1 parent 2b7fd29b
Exists in master and in 4 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb, spb-stable

Separate observing of Note and MergeRequests 

* Move is_assigned? and is_being_xx? methods to IssueCommonality

  This is behavior merge requests have in common with issues. Moved
  methods to IssueCommonality role. Put specs directly into
  merge_request_spec because setup differs for issues and MRs
  specifically in the "closed" factory to use.

* Add MergeRequestObserver. Parallels IssueObserver in almost every way.

  Ripe for refactoring.

* Rename MailerObserver to NoteObserver

  With merge request observing moved out of MailerObserver, all that
  was left was Note logic. Renamed to NoteObserver, added tests and
  updated application config for new observer names. Refactored
  NoteObserver to use the note's author and not rely on current_user.

* Set current_user for MergeRequestObserver

  IssueObserver and MergeRequestObserver are the only observers that
  need a reference to the current_user that they cannot look up on
  the objects they are observing.
Showing 11 changed files with 445 additions and 102 deletions   Show diff stats
app/controllers/application_controller.rb
  Diff comments   View file @16ceae8
1 1 class ApplicationController < ActionController::Base
2 2 before_filter :authenticate_user!
3 3 before_filter :reject_blocked!
4   - before_filter :set_current_user_for_mailer
5 4 before_filter :set_current_user_for_observers
6 5 before_filter :dev_tools if Rails.env == 'development'
7 6  
... ... @@ -41,11 +40,8 @@ class ApplicationController &lt; ActionController::Base
41 40 end
42 41 end
43 42  
44   - def set_current_user_for_mailer
45   - MailerObserver.current_user = current_user
46   - end
47   -
48 43 def set_current_user_for_observers
  44 + MergeRequestObserver.current_user = current_user
49 45 IssueObserver.current_user = current_user
50 46 end
51 47  
... ...
app/models/issue.rb
  Diff comments   View file @16ceae8
... ... @@ -14,22 +14,6 @@ class Issue &lt; ActiveRecord::Base
14 14 def self.open_for(user)
15 15 opened.assigned(user)
16 16 end
17   -
18   - def is_assigned?
19   - !!assignee_id
20   - end
21   -
22   - def is_being_reassigned?
23   - assignee_id_changed?
24   - end
25   -
26   - def is_being_closed?
27   - closed_changed? && closed
28   - end
29   -
30   - def is_being_reopened?
31   - closed_changed? && !closed
32   - end
33 17 end
34 18  
35 19 # == Schema Information
... ...
app/observers/mailer_observer.rb
View file @2b7fd29
... ... @@ -1,80 +0,0 @@
1   -class MailerObserver < ActiveRecord::Observer
2   - observe :note, :merge_request
3   - cattr_accessor :current_user
4   -
5   - def after_create(model)
6   - new_note(model) if model.kind_of?(Note)
7   - new_merge_request(model) if model.kind_of?(MergeRequest)
8   - end
9   -
10   - def after_update(model)
11   - changed_merge_request(model) if model.kind_of?(MergeRequest)
12   - end
13   -
14   - protected
15   -
16   - def new_note(note)
17   - if note.notify
18   - # Notify whole team except author of note
19   - notify_note(note)
20   - elsif note.notify_author
21   - # Notify only author of resource
22   - Notify.note_commit_email(note.commit_author.id, note.id).deliver
23   - else
24   - # Otherwise ignore it
25   - nil
26   - end
27   - end
28   -
29   - def notify_note note
30   - # reject author of note from mail list
31   - users = note.project.users.reject { |u| u.id == current_user.id }
32   -
33   - users.each do |u|
34   - case note.noteable_type
35   - when "Commit"; Notify.note_commit_email(u.id, note.id).deliver
36   - when "Issue"; Notify.note_issue_email(u.id, note.id).deliver
37   - when "Wiki"; Notify.note_wiki_email(u.id, note.id).deliver
38   - when "MergeRequest"; Notify.note_merge_request_email(u.id, note.id).deliver
39   - when "Snippet"; true
40   - else
41   - Notify.note_wall_email(u.id, note.id).deliver
42   - end
43   - end
44   - end
45   -
46   - def new_merge_request(merge_request)
47   - if merge_request.assignee && merge_request.assignee != current_user
48   - Notify.new_merge_request_email(merge_request.id).deliver
49   - end
50   - end
51   -
52   - def changed_merge_request(merge_request)
53   - status_notify_and_comment merge_request, :reassigned_merge_request_email
54   - end
55   -
56   - # This method used for Issues & Merge Requests
57   - #
58   - # It create a comment for Issue or MR if someone close/reopen.
59   - # It also notify via email if assignee was changed
60   - #
61   - def status_notify_and_comment target, mail_method
62   - # If assigne changed - notify to recipients
63   - if target.assignee_id_changed?
64   - recipients_ids = target.assignee_id_was, target.assignee_id
65   - recipients_ids.delete current_user.id
66   -
67   - recipients_ids.each do |recipient_id|
68   - Notify.send(mail_method, recipient_id, target.id, target.assignee_id_was).deliver
69   - end
70   - end
71   -
72   - # Create comment about status changed
73   - if target.closed_changed?
74   - note = Note.new(noteable: target, project: target.project)
75   - note.author = current_user
76   - note.note = "_Status changed to #{target.closed ? 'closed' : 'reopened'}_"
77   - note.save()
78   - end
79   - end
80   -end
app/observers/merge_request_observer.rb 0 → 100644
  Diff comments   View file @16ceae8
... ... @@ -0,0 +1,31 @@
  1 +class MergeRequestObserver < ActiveRecord::Observer
  2 + cattr_accessor :current_user
  3 +
  4 + def after_create(merge_request)
  5 + if merge_request.assignee && merge_request.assignee != current_user
  6 + Notify.new_merge_request_email(merge_request.id).deliver
  7 + end
  8 + end
  9 +
  10 + def after_update(merge_request)
  11 + send_reassigned_email(merge_request) if merge_request.is_being_reassigned?
  12 +
  13 + status = nil
  14 + status = 'closed' if merge_request.is_being_closed?
  15 + status = 'reopened' if merge_request.is_being_reopened?
  16 + if status
  17 + Note.create_status_change_note(merge_request, current_user, status)
  18 + end
  19 + end
  20 +
  21 + protected
  22 +
  23 + def send_reassigned_email(merge_request)
  24 + recipients_ids = merge_request.assignee_id_was, merge_request.assignee_id
  25 + recipients_ids.delete current_user.id
  26 +
  27 + recipients_ids.each do |recipient_id|
  28 + Notify.reassigned_merge_request_email(recipient_id, merge_request.id, merge_request.assignee_id_was).deliver
  29 + end
  30 + end
  31 +end
... ...
app/observers/note_observer.rb 0 → 100644
  Diff comments   View file @16ceae8
... ... @@ -0,0 +1,36 @@
  1 +class NoteObserver < ActiveRecord::Observer
  2 +
  3 + def after_create(note)
  4 + if note.notify
  5 + # Notify whole team except author of note
  6 + notify_team_of_new_note(note)
  7 + elsif note.notify_author
  8 + # Notify only author of resource
  9 + Notify.note_commit_email(note.commit_author.id, note.id).deliver
  10 + else
  11 + # Otherwise ignore it
  12 + nil
  13 + end
  14 + end
  15 +
  16 + protected
  17 +
  18 + def notify_team_of_new_note(note)
  19 + team_without_note_author(note).map do |u|
  20 + case note.noteable_type
  21 + when "Commit"; Notify.note_commit_email(u.id, note.id).deliver
  22 + when "Issue"; Notify.note_issue_email(u.id, note.id).deliver
  23 + when "Wiki"; Notify.note_wiki_email(u.id, note.id).deliver
  24 + when "MergeRequest"; Notify.note_merge_request_email(u.id, note.id).deliver
  25 + when "Wall"; Notify.note_wall_email(u.id, note.id).deliver
  26 + when "Snippet"; true # no notifications for snippets?
  27 + else
  28 + true
  29 + end
  30 + end
  31 + end
  32 +
  33 + def team_without_note_author(note)
  34 + note.project.users.reject { |u| u.id == note.author.id }
  35 + end
  36 +end
... ...
app/roles/issue_commonality.rb
  Diff comments   View file @16ceae8
... ... @@ -46,4 +46,21 @@ module IssueCommonality
46 46 def new?
47 47 today? && created_at == updated_at
48 48 end
  49 +
  50 + def is_assigned?
  51 + !!assignee_id
  52 + end
  53 +
  54 + def is_being_reassigned?
  55 + assignee_id_changed?
  56 + end
  57 +
  58 + def is_being_closed?
  59 + closed_changed? && closed
  60 + end
  61 +
  62 + def is_being_reopened?
  63 + closed_changed? && !closed
  64 + end
  65 +
49 66 end
... ...
config/application.rb
  Diff comments   View file @16ceae8
... ... @@ -23,7 +23,15 @@ module Gitlab
23 23 # config.plugins = [ :exception_notification, :ssl_requirement, :all ]
24 24  
25 25 # Activate observers that should always be running.
26   - config.active_record.observers = :mailer_observer, :activity_observer, :project_observer, :key_observer, :issue_observer, :user_observer, :system_hook_observer, :users_project_observer
  26 + config.active_record.observers = :activity_observer,
  27 + :issue_observer,
  28 + :key_observer,
  29 + :merge_request_observer,
  30 + :note_observer,
  31 + :project_observer,
  32 + :system_hook_observer,
  33 + :user_observer,
  34 + :users_project_observer
27 35  
28 36 # Set Time.zone default to the specified zone and make Active Record auto-convert to this zone.
29 37 # Run "rake -D time" for a list of tasks for finding time zone names. Default is UTC.
... ...
spec/factories.rb
  Diff comments   View file @16ceae8
... ... @@ -76,6 +76,12 @@ FactoryGirl.define do
76 76 project
77 77 source_branch "master"
78 78 target_branch "stable"
  79 +
  80 + trait :closed do
  81 + closed true
  82 + end
  83 +
  84 + factory :closed_merge_request, traits: [:closed]
79 85 end
80 86  
81 87 factory :note do
... ...
spec/models/merge_request_spec.rb
  Diff comments   View file @16ceae8
... ... @@ -50,4 +50,47 @@ describe MergeRequest do
50 50 merge_request.mr_and_commit_notes.count.should == 2
51 51 end
52 52 end
  53 +
  54 + subject { Factory.create(:merge_request) }
  55 +
  56 + describe '#is_being_reassigned?' do
  57 + it 'returns true if the merge_request assignee has changed' do
  58 + subject.assignee = Factory(:user)
  59 + subject.is_being_reassigned?.should be_true
  60 + end
  61 + it 'returns false if the merge request assignee has not changed' do
  62 + subject.is_being_reassigned?.should be_false
  63 + end
  64 + end
  65 +
  66 + describe '#is_being_closed?' do
  67 + it 'returns true if the closed attribute has changed and is now true' do
  68 + subject.closed = true
  69 + subject.is_being_closed?.should be_true
  70 + end
  71 + it 'returns false if the closed attribute has changed and is now false' do
  72 + merge_request = Factory.create(:closed_merge_request)
  73 + merge_request.closed = false
  74 + merge_request.is_being_closed?.should be_false
  75 + end
  76 + it 'returns false if the closed attribute has not changed' do
  77 + subject.is_being_closed?.should be_false
  78 + end
  79 + end
  80 +
  81 +
  82 + describe '#is_being_reopened?' do
  83 + it 'returns true if the closed attribute has changed and is now false' do
  84 + merge_request = Factory.create(:closed_merge_request)
  85 + merge_request.closed = false
  86 + merge_request.is_being_reopened?.should be_true
  87 + end
  88 + it 'returns false if the closed attribute has changed and is now true' do
  89 + subject.closed = true
  90 + subject.is_being_reopened?.should be_false
  91 + end
  92 + it 'returns false if the closed attribute has not changed' do
  93 + subject.is_being_reopened?.should be_false
  94 + end
  95 + end
53 96 end
... ...
spec/observers/merge_request_observer_spec.rb 0 → 100644
  Diff comments   View file @16ceae8
... ... @@ -0,0 +1,193 @@
  1 +require 'spec_helper'
  2 +
  3 +describe MergeRequestObserver do
  4 + let(:some_user) { double(:user, id: 1) }
  5 + let(:assignee) { double(:user, id: 2) }
  6 + let(:author) { double(:user, id: 3) }
  7 + let(:mr) { double(:merge_request, id: 42, assignee: assignee, author: author) }
  8 +
  9 + before(:each) { subject.stub(:current_user).and_return(some_user) }
  10 +
  11 + subject { MergeRequestObserver.instance }
  12 +
  13 + describe '#after_create' do
  14 +
  15 + it 'is called when a merge request is created' do
  16 + subject.should_receive(:after_create)
  17 +
  18 + MergeRequest.observers.enable :merge_request_observer do
  19 + Factory.create(:merge_request, project: Factory.create(:project))
  20 + end
  21 + end
  22 +
  23 + it 'sends an email to the assignee' do
  24 + Notify.should_receive(:new_merge_request_email).with(mr.id).
  25 + and_return(double(deliver: true))
  26 +
  27 + subject.after_create(mr)
  28 + end
  29 +
  30 + it 'does not send an email to the assignee if assignee created the merge request' do
  31 + subject.stub(:current_user).and_return(assignee)
  32 + Notify.should_not_receive(:new_merge_request_email)
  33 +
  34 + subject.after_create(mr)
  35 + end
  36 + end
  37 +
  38 + context '#after_update' do
  39 + before(:each) do
  40 + mr.stub(:is_being_reassigned?).and_return(false)
  41 + mr.stub(:is_being_closed?).and_return(false)
  42 + mr.stub(:is_being_reopened?).and_return(false)
  43 + end
  44 +
  45 + it 'is called when a merge request is changed' do
  46 + changed = Factory.create(:merge_request, project: Factory.create(:project))
  47 + subject.should_receive(:after_update)
  48 +
  49 + MergeRequest.observers.enable :merge_request_observer do
  50 + changed.title = 'I changed'
  51 + changed.save
  52 + end
  53 + end
  54 +
  55 + context 'a reassigned email' do
  56 + it 'is sent if the merge request is being reassigned' do
  57 + mr.should_receive(:is_being_reassigned?).and_return(true)
  58 + subject.should_receive(:send_reassigned_email).with(mr)
  59 +
  60 + subject.after_update(mr)
  61 + end
  62 +
  63 + it 'is not sent if the merge request is not being reassigned' do
  64 + mr.should_receive(:is_being_reassigned?).and_return(false)
  65 + subject.should_not_receive(:send_reassigned_email)
  66 +
  67 + subject.after_update(mr)
  68 + end
  69 + end
  70 +
  71 + context 'a status "closed"' do
  72 + it 'note is created if the merge request is being closed' do
  73 + mr.should_receive(:is_being_closed?).and_return(true)
  74 + Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed')
  75 +
  76 + subject.after_update(mr)
  77 + end
  78 +
  79 + it 'note is not created if the merge request is not being closed' do
  80 + mr.should_receive(:is_being_closed?).and_return(false)
  81 + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed')
  82 +
  83 + subject.after_update(mr)
  84 + end
  85 +
  86 + it 'notification is delivered if the merge request being closed' do
  87 + mr.stub(:is_being_closed?).and_return(true)
  88 + Note.should_receive(:create_status_change_note).with(mr, some_user, 'closed')
  89 +
  90 + subject.after_update(mr)
  91 + end
  92 +
  93 + it 'notification is not delivered if the merge request not being closed' do
  94 + mr.stub(:is_being_closed?).and_return(false)
  95 + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'closed')
  96 +
  97 + subject.after_update(mr)
  98 + end
  99 +
  100 + it 'notification is delivered only to author if the merge request is being closed' do
  101 + mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil)
  102 + mr_without_assignee.stub(:is_being_reassigned?).and_return(false)
  103 + mr_without_assignee.stub(:is_being_closed?).and_return(true)
  104 + mr_without_assignee.stub(:is_being_reopened?).and_return(false)
  105 + Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'closed')
  106 +
  107 + subject.after_update(mr_without_assignee)
  108 + end
  109 + end
  110 +
  111 + context 'a status "reopened"' do
  112 + it 'note is created if the merge request is being reopened' do
  113 + mr.should_receive(:is_being_reopened?).and_return(true)
  114 + Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened')
  115 +
  116 + subject.after_update(mr)
  117 + end
  118 +
  119 + it 'note is not created if the merge request is not being reopened' do
  120 + mr.should_receive(:is_being_reopened?).and_return(false)
  121 + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened')
  122 +
  123 + subject.after_update(mr)
  124 + end
  125 +
  126 + it 'notification is delivered if the merge request being reopened' do
  127 + mr.stub(:is_being_reopened?).and_return(true)
  128 + Note.should_receive(:create_status_change_note).with(mr, some_user, 'reopened')
  129 +
  130 + subject.after_update(mr)
  131 + end
  132 +
  133 + it 'notification is not delivered if the merge request is not being reopened' do
  134 + mr.stub(:is_being_reopened?).and_return(false)
  135 + Note.should_not_receive(:create_status_change_note).with(mr, some_user, 'reopened')
  136 +
  137 + subject.after_update(mr)
  138 + end
  139 +
  140 + it 'notification is delivered only to author if the merge request is being reopened' do
  141 + mr_without_assignee = double(:merge_request, id: 42, author: author, assignee: nil)
  142 + mr_without_assignee.stub(:is_being_reassigned?).and_return(false)
  143 + mr_without_assignee.stub(:is_being_closed?).and_return(false)
  144 + mr_without_assignee.stub(:is_being_reopened?).and_return(true)
  145 + Note.should_receive(:create_status_change_note).with(mr_without_assignee, some_user, 'reopened')
  146 +
  147 + subject.after_update(mr_without_assignee)
  148 + end
  149 + end
  150 + end
  151 +
  152 + describe '#send_reassigned_email' do
  153 + let(:previous_assignee) { double(:user, id: 3) }
  154 +
  155 + before(:each) do
  156 + mr.stub(:assignee_id).and_return(assignee.id)
  157 + mr.stub(:assignee_id_was).and_return(previous_assignee.id)
  158 + end
  159 +
  160 + def it_sends_a_reassigned_email_to(recipient)
  161 + Notify.should_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id).
  162 + and_return(double(deliver: true))
  163 + end
  164 +
  165 + def it_does_not_send_a_reassigned_email_to(recipient)
  166 + Notify.should_not_receive(:reassigned_merge_request_email).with(recipient, mr.id, previous_assignee.id)
  167 + end
  168 +
  169 + it 'sends a reassigned email to the previous and current assignees' do
  170 + it_sends_a_reassigned_email_to assignee.id
  171 + it_sends_a_reassigned_email_to previous_assignee.id
  172 +
  173 + subject.send(:send_reassigned_email, mr)
  174 + end
  175 +
  176 + context 'does not send an email to the user who made the reassignment' do
  177 + it 'if the user is the assignee' do
  178 + subject.stub(:current_user).and_return(assignee)
  179 + it_sends_a_reassigned_email_to previous_assignee.id
  180 + it_does_not_send_a_reassigned_email_to assignee.id
  181 +
  182 + subject.send(:send_reassigned_email, mr)
  183 + end
  184 + it 'if the user is the previous assignee' do
  185 + subject.stub(:current_user).and_return(previous_assignee)
  186 + it_sends_a_reassigned_email_to assignee.id
  187 + it_does_not_send_a_reassigned_email_to previous_assignee.id
  188 +
  189 + subject.send(:send_reassigned_email, mr)
  190 + end
  191 + end
  192 + end
  193 +end
... ...
spec/observers/note_observer_spec.rb 0 → 100644
  Diff comments   View file @16ceae8
... ... @@ -0,0 +1,109 @@
  1 +require 'spec_helper'
  2 +
  3 +describe NoteObserver do
  4 + subject { NoteObserver.instance }
  5 +
  6 + describe '#after_create' do
  7 + let(:note) { double :note, notify: false, notify_author: false }
  8 +
  9 + it 'is called after a note is created' do
  10 + subject.should_receive :after_create
  11 +
  12 + Note.observers.enable :note_observer do
  13 + Factory.create(:note)
  14 + end
  15 + end
  16 +
  17 + it 'notifies team of new note when flagged to notify' do
  18 + note.stub(:notify).and_return(true)
  19 + subject.should_receive(:notify_team_of_new_note).with(note)
  20 +
  21 + subject.after_create(note)
  22 + end
  23 + 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)
  25 +
  26 + subject.after_create(note)
  27 + end
  28 + it 'notifies the author of a commit when flagged to notify the author' do
  29 + note.stub(:notify_author).and_return(true)
  30 + note.stub(:id).and_return(42)
  31 + author = double :user, id: 1
  32 + note.stub(:commit_author).and_return(author)
  33 + Notify.should_receive(:note_commit_email).and_return(double(deliver: true))
  34 +
  35 + subject.after_create(note)
  36 + end
  37 + it 'does not notify the author of a commit when not flagged to notify the author' do
  38 + Notify.should_not_receive(:note_commit_email)
  39 +
  40 + subject.after_create(note)
  41 + end
  42 + it 'does nothing if no notify flags are set' do
  43 + subject.after_create(note).should be_nil
  44 + end
  45 + end
  46 +
  47 +
  48 + let(:team_without_author) { (1..2).map { |n| double :user, id: n } }
  49 +
  50 + describe '#notify_team_of_new_note' do
  51 + let(:note) { double :note, id: 1 }
  52 +
  53 + before :each do
  54 + subject.stub(:team_without_note_author).with(note).and_return(team_without_author)
  55 + end
  56 +
  57 + context 'notifies team of a new note on' do
  58 + it 'a commit' do
  59 + note.stub(:noteable_type).and_return('Commit')
  60 + Notify.should_receive(:note_commit_email).twice.and_return(double(deliver: true))
  61 +
  62 + subject.send(:notify_team_of_new_note, note)
  63 + end
  64 + it 'an issue' do
  65 + note.stub(:noteable_type).and_return('Issue')
  66 + Notify.should_receive(:note_issue_email).twice.and_return(double(deliver: true))
  67 +
  68 + subject.send(:notify_team_of_new_note, note)
  69 + end
  70 + it 'a wiki page' do
  71 + note.stub(:noteable_type).and_return('Wiki')
  72 + Notify.should_receive(:note_wiki_email).twice.and_return(double(deliver: true))
  73 +
  74 + subject.send(:notify_team_of_new_note, note)
  75 + end
  76 + it 'a merge request' do
  77 + note.stub(:noteable_type).and_return('MergeRequest')
  78 + Notify.should_receive(:note_merge_request_email).twice.and_return(double(deliver: true))
  79 +
  80 + subject.send(:notify_team_of_new_note, note)
  81 + end
  82 + it 'a wall' do
  83 + note.stub(:noteable_type).and_return('Wall')
  84 + Notify.should_receive(:note_wall_email).twice.and_return(double(deliver: true))
  85 +
  86 + subject.send(:notify_team_of_new_note, note)
  87 + end
  88 + end
  89 +
  90 + it 'does nothing for a new note on a snippet' do
  91 + note.stub(:noteable_type).and_return('Snippet')
  92 +
  93 + subject.send(:notify_team_of_new_note, note).should == [true, true]
  94 + end
  95 + end
  96 +
  97 +
  98 + describe '#team_without_note_author' do
  99 + let(:author) { double :user, id: 4 }
  100 +
  101 + let(:users) { team_without_author + [author] }
  102 + let(:project) { double :project, users: users }
  103 + let(:note) { double :note, project: project, author: author }
  104 +
  105 + it 'returns the projects user without the note author included' do
  106 + subject.send(:team_without_note_author, note).should == team_without_author
  107 + end
  108 + end
  109 +end
... ...