Merge branch 'separate_user_and_issue_observer_from_mail_observer' of...

Merge branch 'separate_user_and_issue_observer_from_mail_observer' of https://github.com/robbkidd/gitlabhq into robbkidd-separate_user_and_issue_observer_from_mail_observer

Gemfile

@@ -77,4 +77,5 @@ group :test do
   gem "simplecov", :require => false
   gem "shoulda-matchers"
   gem 'email_spec'
+  gem 'resque_spec'
 end

Gemfile.lock

@@ -257,6 +257,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)
@@ -385,6 +388,7 @@ DEPENDENCIES
   redcarpet (~> 2.1.1)
   resque (~> 1.20.0)
   resque_mailer
+  resque_spec
   rspec-rails
   sass-rails (= 3.2.5)
   seed-fu

app/models/issue.rb

@@ -55,6 +55,18 @@ class Issue < ActiveRecord::Base
   def new?
     today? && created_at == updated_at
   end
+
+  def is_being_reassigned?
+    assignee_id_changed?
+  end
+
+  def is_being_closed?
+    closed_changed? && closed
+  end
+
+  def is_being_reopened?
+    closed_changed? && !closed
+  end
 end
 # == Schema Information
 #

app/models/issue_observer.rb

+class IssueObserver < ActiveRecord::Observer
+  cattr_accessor :current_user
+
+  def after_create(issue)
+    Notify.new_issue_email(issue.id).deliver if issue.assignee != current_user
+  end
+
+  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
+
+  protected
+
+  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).deliver
+    end
+  end
+end

app/models/mailer_observer.rb

 class MailerObserver < ActiveRecord::Observer
-  observe :issue, :user, :note, :merge_request
+  observe :note, :merge_request
   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)
   end
 
   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
-
   def new_note(note)
     if note.notify
       # Notify whole team except author of note
@@ -65,10 +52,6 @@ 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.

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

app/models/user_observer.rb

+class UserObserver < ActiveRecord::Observer
+  def after_create(user)
+    Notify.new_user_email(user.id, user.password).deliver
+  end
+end

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, :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.

config/initializers/resque_mailer.rb

+Resque::Mailer.excluded_environments = []

spec/models/activity_observer_spec.rb

@@ -10,9 +10,11 @@ describe ActivityObserver do
 
   describe "Merge Request created" do 
     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
     it { @event.action.should == Event::Created }
@@ -21,9 +23,11 @@ describe ActivityObserver do
 
   describe "Issue created" do 
     before do
+      Issue.observers.enable :activity_observer do
         @issue = Factory :issue, :project => project
         @event = Event.last
       end
+    end
 
     it_should_be_valid_event
     it { @event.action.should == Event::Created }

spec/models/issue_observer_spec.rb

+require 'spec_helper'
+
+describe IssueObserver do
+  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) }
+
+  subject { IssueObserver.instance }
+
+  describe '#after_create' do
+
+    it 'is called when an issue is created' do
+      subject.should_receive(:after_create)
+
+      Issue.observers.enable :issue_observer do
+        Factory.create(:issue, :project => Factory.create(:project))
+      end
+    end
+
+    it 'sends an email to the assignee' do
+      Notify.should_receive(:new_issue_email).with(issue.id).
+        and_return(double(:deliver => true))
+
+      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 '#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)
+
+      Issue.observers.enable :issue_observer do
+        changed.description = 'I changed'
+        changed.save
+      end
+    end
+
+    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_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_update(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_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_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
+
+  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
+
+    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
+      it_sends_a_reassigned_email_to assignee.id
+      it_sends_a_reassigned_email_to previous_assignee.id
+
+      subject.send(: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)
+        it_sends_a_reassigned_email_to previous_assignee.id
+        it_does_not_send_a_reassigned_email_to assignee.id
+
+        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(:send_reassigned_email, issue)
+      end
+    end
+  end
+end

spec/models/issue_spec.rb

@@ -20,10 +20,60 @@ describe Issue do
     it { Issue.should respond_to :opened }
   end
 
-  it { Factory.create(:issue,
+  subject { Factory.create(:issue,
                            :author => Factory(:user),
                            :assignee => Factory(:user),
-                      :project => Factory.create(:project)).should be_valid }
+                           :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 '#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 '#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) }
@@ -56,6 +106,7 @@ describe Issue do
       subject.upvotes.should == 2
     end
   end
+
 end
 # == Schema Information
 #

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

spec/models/project_spec.rb

@@ -73,15 +73,28 @@ describe Project do
 
   describe "last_activity" do
     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 }
 

spec/models/user_observer_spec.rb

+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

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

spec/requests/admin/admin_users_spec.rb

@@ -41,11 +41,17 @@ describe "Admin::Users" do
 
     it "should call send mail" do
       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 :user_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")
@@ -53,6 +59,7 @@ describe "Admin::Users" do
         email.body.should have_content(@password)
       end
     end
+  end
 
   describe "GET /admin/users/:id" do
     before do

spec/requests/issues_spec.rb

@@ -128,17 +128,23 @@ describe "Issues" do
         end
 
         it "should call send mail" do
+          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
+          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
+        end
 
       end
     end

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