From eeed2a49b75e11ca3f173e28be13f9da89d8b2ad Mon Sep 17 00:00:00 2001 From: Vokhmin Alexey V Date: Thu, 27 Jun 2013 17:06:42 +0400 Subject: [PATCH] #200: moved observer of Issue to lib --- Gemfile | 1 + Gemfile.lock | 2 + app/models/activity_feed_observer.rb | 78 ++++++++++--------- app/models/issue.rb | 1 + lib/modules/observers/activity_feed/issue.rb | 59 ++++++++++++++ .../lib/observers/activity_feed/issue_spec.rb | 30 +++++++ spec/mailers/user_mailer_spec.rb | 1 - spec/models/activity_feed_observer_spec.rb | 6 -- 8 files changed, 134 insertions(+), 44 deletions(-) create mode 100644 lib/modules/observers/activity_feed/issue.rb create mode 100644 spec/lib/observers/activity_feed/issue_spec.rb delete mode 100644 spec/models/activity_feed_observer_spec.rb diff --git a/Gemfile b/Gemfile index c9423e923..e55eacfff 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index dfe789d3c..b5c710783 100644 --- a/Gemfile.lock +++ b/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) diff --git a/app/models/activity_feed_observer.rb b/app/models/activity_feed_observer.rb index 85ca47126..ae3c1ae05 100644 --- a/app/models/activity_feed_observer.rb +++ b/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' - record.collect_recipients.each do |recipient| - next if record.user_id == recipient.id - UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue - ActivityFeed.create( - :user => recipient, - :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, - :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end + # when 'Issue' + # record.collect_recipients.each do |recipient| + # next if record.user_id == recipient.id + # UserMailer.new_issue_notification(record, recipient).deliver if recipient.notifier.can_notify && recipient.notifier.new_issue + # ActivityFeed.create( + # :user => recipient, + # :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, + # :issue_title => record.title, :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end - if record.assignee_id_changed? - UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify - ActivityFeed.create( - :user => record.user, - :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, - :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - record.project.hooks.each{ |h| h.receive_issues(record, :create) } - Comment.create_link_on_issues_from_item(record) + # if record.assignee_id_changed? + # UserMailer.new_issue_notification(record, record.assignee).deliver if record.assignee.notifier.issue_assign && record.assignee.notifier.can_notify + # ActivityFeed.create( + # :user => record.user, + # :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, + # :project_id => record.project.id, :issue_title => record.title, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end + # record.project.hooks.each{ |h| h.receive_issues(record, :create) } + # 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' - 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 - ActivityFeed.create( - :user => record.assignee, - :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, - :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} - ) - end - record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed? - # dont remove outdated issues link - Comment.create_link_on_issues_from_item(record) + # when 'Issue' + # 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 + # ActivityFeed.create( + # :user => record.assignee, + # :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, + # :project_id => record.project.id, :project_name => record.project.name, :project_owner => record.project.owner.uname} + # ) + # end + # record.project.hooks.each{ |h| h.receive_issues(record, :update) } if record.status_changed? + # # dont remove outdated issues link + # 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) || diff --git a/app/models/issue.rb b/app/models/issue.rb index c9236a489..9e043eb0a 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/lib/modules/observers/activity_feed/issue.rb b/lib/modules/observers/activity_feed/issue.rb new file mode 100644 index 000000000..d2ed1d016 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/spec/lib/observers/activity_feed/issue_spec.rb b/spec/lib/observers/activity_feed/issue_spec.rb new file mode 100644 index 000000000..37e9d2730 --- /dev/null +++ b/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 diff --git a/spec/mailers/user_mailer_spec.rb b/spec/mailers/user_mailer_spec.rb index 72c8674ec..17dacb5a8 100644 --- a/spec/mailers/user_mailer_spec.rb +++ b/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 diff --git a/spec/models/activity_feed_observer_spec.rb b/spec/models/activity_feed_observer_spec.rb deleted file mode 100644 index f008468e9..000000000 --- a/spec/models/activity_feed_observer_spec.rb +++ /dev/null @@ -1,6 +0,0 @@ -# -*- encoding : utf-8 -*- -require 'spec_helper' - -describe ActivityFeedObserver do - pending "add some examples to (or delete) #{__FILE__}" -end