#200: moved observer of Issue to lib

Gemfile

@@ -95,5 +95,6 @@ group :test do
   gem 'rr',                 '~> 1.0.4'
   gem 'shoulda'
   gem 'mock_redis',         '0.6.2'
+  gem 'test_after_commit',  '0.2.0'
   gem 'rake'
 end

Gemfile.lock

@@ -363,6 +363,7 @@ GEM
     state_machine (1.1.2)
     stringex (1.4.0)
     systemu (2.5.2)
+    test_after_commit (0.2.0)
     therubyracer (0.10.2)
       libv8 (~> 3.3.10)
     thin (1.5.1)
@@ -466,6 +467,7 @@ DEPENDENCIES
   shotgun
   shoulda
   state_machine
+  test_after_commit (= 0.2.0)
   therubyracer (~> 0.10.2)
   therubyrhino (~> 1.73.1)
   trinidad (~> 1.0.2)

app/models/activity_feed_observer.rb

@@ -1,6 +1,6 @@
 # -*- encoding : utf-8 -*-
 class ActivityFeedObserver < ActiveRecord::Observer
-  observe :issue, :comment, :user, :build_list
+  observe :comment, :user, :build_list
 
   BUILD_LIST_STATUSES = [
                           BuildList::BUILD_PUBLISHED,
@@ -10,7 +10,11 @@ class ActivityFeedObserver < ActiveRecord::Observer
                           BuildList::TESTS_FAILED
                         ].freeze
 
-  def after_create(record)
+  def after_commit(record)
+    # See:
+    # - http://rails-bestpractices.com/posts/695-use-after_commit
+    # - https://coderwall.com/p/f5-vlq
+    return unless record.send(:transaction_include_action?, :create) 
     case record.class.to_s
     when 'User'
       ActivityFeed.create(
@@ -19,29 +23,29 @@ class ActivityFeedObserver < ActiveRecord::Observer
         :data => {:user_name => record.user_appeal, :user_email => record.email}
       )
 
-    when 'Issue'
+    # when 'Issue'
-      record.collect_recipients.each do |recipient|
+    #   record.collect_recipients.each do |recipient|
-        next if record.user_id == recipient.id
+    #     next if record.user_id == recipient.id
-        UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue
+    #     UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue
-        ActivityFeed.create(
+    #     ActivityFeed.create(
-          :user => recipient,
+    #       :user => recipient,
-          :kind => 'new_issue_notification',
+    #       :kind => 'new_issue_notification',
-          :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id,:issue_serial_id => record.serial_id,
+    #       :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id,:issue_serial_id => record.serial_id,
-                           :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname}
+    #                        :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname}
-        )
+    #     )
-      end
+    #   end
 
-      if record.assignee_id_changed?
+    #   if record.assignee_id_changed?
-        UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify
+    #     UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify
-        ActivityFeed.create(
+    #     ActivityFeed.create(
-          :user => record.user,
+    #       :user => record.user,
-          :kind => 'issue_assign_notification',
+    #       :kind => 'issue_assign_notification',
-          :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :issue_serial_id => record.serial_id,
+    #       :data => {:user_name => record.user.name, :user_email => record.user.email, :user_id => record.user_id, :issue_serial_id => record.serial_id,
-                           :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname}
+    #                        :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname}
-        )
+    #     )
-      end
+    #   end
-      record.project.hooks.each{ |h| h.receive_issues(record, :create) }
+    #   record.project.hooks.each{ |h| h.receive_issues(record, :create) }
-      Comment.create_link_on_issues_from_item(record)
+    #   Comment.create_link_on_issues_from_item(record)
     when 'Comment'
       return if record.automatic
       if record.issue_comment?
@@ -138,19 +142,19 @@ class ActivityFeedObserver < ActiveRecord::Observer
 
   def after_update(record)
     case record.class.to_s
-    when 'Issue'
+    # when 'Issue'
-      if record.assignee_id && record.assignee_id_changed?
+    #   if record.assignee_id && record.assignee_id_changed?
-        UserMailer.issue_assign_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify
+    #     UserMailer.issue_assign_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify
-        ActivityFeed.create(
+    #     ActivityFeed.create(
-          :user => record.assignee,
+    #       :user => record.assignee,
-          :kind => 'issue_assign_notification',
+    #       :kind => 'issue_assign_notification',
-          :data => {:user_name => record.assignee.name, :user_email => record.assignee.email, :issue_serial_id => record.serial_id, :issue_title => record.title,
+    #       :data => {:user_name => record.assignee.name, :user_email => record.assignee.email, :issue_serial_id => record.serial_id, :issue_title => record.title,
-                           :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname}
+    #                        :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname}
-        )
+    #     )
-      end
+    #   end
-      record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed?
+    #   record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed?
-      # dont remove outdated issues link
+    #   # dont remove outdated issues link
-      Comment.create_link_on_issues_from_item(record)
+    #   Comment.create_link_on_issues_from_item(record)
     when 'BuildList'
       if record.mass_build.blank? && ( # Do not show mass build activity in activity feeds
           record.status_changed? && BUILD_LIST_STATUSES.include?(record.status) ||

app/models/issue.rb

@@ -1,5 +1,6 @@
 # -*- encoding : utf-8 -*-
 class Issue < ActiveRecord::Base
+  include Modules::Observers::ActivityFeed::Issue
   STATUSES = ['open', 'closed']
 
   belongs_to :project

lib/modules/observers/activity_feed/issue.rb

@@ -0,0 +1,59 @@
+# -*- encoding : utf-8 -*-
+module Modules::Observers::ActivityFeed::Issue
+  extend ActiveSupport::Concern
+
+  included do
+    after_commit :new_issue_notifications, :on => :create
+    after_update -> { send_assign_notifications(:update) }
+  end
+
+  private
+
+  def new_issue_notifications
+    collect_recipients.each do |recipient|
+      next if user_id == recipient.id
+      UserMailer.new_issue_notification(self, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue
+      ActivityFeed.create(
+        :user => recipient,
+        :kind => 'new_issue_notification',
+        :data => {
+          :user_name        => user.name,
+          :user_email       => user.email,
+          :user_id          => user_id,
+          :issue_serial_id  => serial_id,
+          :issue_title      => title,
+          :project_id       => project.id,
+          :project_name     => project.name,
+          :project_owner    => project.owner.uname
+        }
+      )
+    end
+    send_assign_notifications
+  end
+
+  def send_assign_notifications(action = :create)
+    if assignee_id && assignee_id_changed?
+      if assignee.notifier.issue_assign && assignee.notifier.can_notify
+        user_mailer_action = action == :create ? :new_issue_notification : :issue_assign_notification
+        UserMailer.send(user_mailer_action, self, assignee).deliver
+      end
+      ActivityFeed.create(
+        :user => assignee,
+        :kind => 'issue_assign_notification',
+        :data => {
+          :user_name        => assignee.name,
+          :user_email       => assignee.email,
+          :issue_serial_id  => serial_id,
+          :issue_title      => title,
+          :project_id       => project.id,
+          :project_name     => project.name,
+          :project_owner    => project.owner.uname
+        }
+      )
+    end
+    project.hooks.each{ |h| h.receive_issues(self, action) } if action == :create || status_changed?
+    # dont remove outdated issues link
+    Comment.create_link_on_issues_from_item(self)
+  end
+
+end

spec/lib/observers/activity_feed/issue_spec.rb

@@ -0,0 +1,30 @@
+# -*- encoding : utf-8 -*-
+require 'spec_helper'
+
+describe Modules::Observers::ActivityFeed::Issue do
+  before { stub_symlink_methods }
+
+  it 'sends a notification email after create' do
+    issue = FactoryGirl.build(:issue, :assignee => nil)
+    mailer = mock!.deliver
+    mock(UserMailer).new_issue_notification(issue, issue.project.owner) { mailer }
+    issue.save
+  end
+
+  it 'does not send a notification email after update' do
+    issue = FactoryGirl.create(:issue, :assignee => nil)
+    issue.title = 'new title'
+    dont_allow(UserMailer).new_issue_notification
+    issue.save
+  end
+
+  it 'sends a notification email after a assignee of issue has been changed' do
+    user = FactoryGirl.create(:user)
+    issue = FactoryGirl.build(:issue, :assignee => nil)
+    issue.assignee = user
+    mailer = mock!.deliver
+    mock(UserMailer).issue_assign_notification(issue, user) { mailer }
+    issue.save
+  end
+
+end

spec/mailers/user_mailer_spec.rb

@@ -2,7 +2,6 @@
 require "spec_helper"
 
 describe UserMailer do
-  pending "add some examples to (or delete) #{__FILE__}"
 
   context 'On Issue create' do
     before(:each) do

spec/models/activity_feed_observer_spec.rb

@@ -1,6 +0,0 @@
-# -*- encoding : utf-8 -*-
-require 'spec_helper'
-
-describe ActivityFeedObserver do
-  pending "add some examples to (or delete) #{__FILE__}"
-end