Commit 0be1975020db7c5e55edf589342b30b928ae3943
Committed by
Antonio Terceiro
1 parent
1cc1999d
Exists in
master
and in
28 other branches
ActionItem628: send mail when add friend, add member and approve article
Showing
12 changed files
with
102 additions
and
50 deletions
Show diff stats
app/controllers/my_profile/cms_controller.rb
| @@ -159,7 +159,6 @@ class CmsController < MyProfileController | @@ -159,7 +159,6 @@ class CmsController < MyProfileController | ||
| 159 | end | 159 | end |
| 160 | 160 | ||
| 161 | def publish | 161 | def publish |
| 162 | - | ||
| 163 | @article = profile.articles.find(params[:id]) | 162 | @article = profile.articles.find(params[:id]) |
| 164 | record_coming_from_public_view | 163 | record_coming_from_public_view |
| 165 | @groups = profile.memberships - [profile] | 164 | @groups = profile.memberships - [profile] |
app/models/add_friend.rb
| @@ -17,12 +17,6 @@ class AddFriend < Task | @@ -17,12 +17,6 @@ class AddFriend < Task | ||
| 17 | target.add_friend(requestor, group_for_friend) | 17 | target.add_friend(requestor, group_for_friend) |
| 18 | end | 18 | end |
| 19 | 19 | ||
| 20 | - # Returns <tt>false</tt>. Adding friends by itself does not trigger e-mail | ||
| 21 | - # sending. | ||
| 22 | - def sends_email? | ||
| 23 | - false | ||
| 24 | - end | ||
| 25 | - | ||
| 26 | def description | 20 | def description |
| 27 | _('%s wants to be your friend') % requestor.name | 21 | _('%s wants to be your friend') % requestor.name |
| 28 | end | 22 | end |
| @@ -31,4 +25,9 @@ class AddFriend < Task | @@ -31,4 +25,9 @@ class AddFriend < Task | ||
| 31 | :manage_friends | 25 | :manage_friends |
| 32 | end | 26 | end |
| 33 | 27 | ||
| 28 | + def target_notification_message | ||
| 29 | + description + "\n\n" + | ||
| 30 | + _('You need login to accept this.') | ||
| 31 | + end | ||
| 32 | + | ||
| 34 | end | 33 | end |
app/models/add_member.rb
| @@ -19,11 +19,6 @@ class AddMember < Task | @@ -19,11 +19,6 @@ class AddMember < Task | ||
| 19 | target.affiliate(requestor, self.roles.map{|i| Role.find(i)}) | 19 | target.affiliate(requestor, self.roles.map{|i| Role.find(i)}) |
| 20 | end | 20 | end |
| 21 | 21 | ||
| 22 | - # FIXME should send email to community admin? | ||
| 23 | - def sends_email? | ||
| 24 | - false | ||
| 25 | - end | ||
| 26 | - | ||
| 27 | def description | 22 | def description |
| 28 | _('%s wants to be a member') % requestor.name | 23 | _('%s wants to be a member') % requestor.name |
| 29 | end | 24 | end |
| @@ -32,4 +27,9 @@ class AddMember < Task | @@ -32,4 +27,9 @@ class AddMember < Task | ||
| 32 | :manage_memberships | 27 | :manage_memberships |
| 33 | end | 28 | end |
| 34 | 29 | ||
| 30 | + def target_notification_message | ||
| 31 | + description + "\n\n" + | ||
| 32 | + _('You need login to accept this.') | ||
| 33 | + end | ||
| 34 | + | ||
| 35 | end | 35 | end |
app/models/approve_article.rb
| 1 | class ApproveArticle < Task | 1 | class ApproveArticle < Task |
| 2 | serialize :data, Hash | 2 | serialize :data, Hash |
| 3 | 3 | ||
| 4 | + validates_presence_of :requestor_id, :target_id | ||
| 5 | + | ||
| 4 | def description | 6 | def description |
| 5 | _('%s wants to publish %s') % [requestor.name, article.name] | 7 | _('%s wants to publish %s') % [requestor.name, article.name] |
| 6 | end | 8 | end |
| @@ -37,7 +39,9 @@ class ApproveArticle < Task | @@ -37,7 +39,9 @@ class ApproveArticle < Task | ||
| 37 | PublishedArticle.create(:name => name, :profile => target, :reference_article => article) | 39 | PublishedArticle.create(:name => name, :profile => target, :reference_article => article) |
| 38 | end | 40 | end |
| 39 | 41 | ||
| 40 | - def sends_email? | ||
| 41 | - true | 42 | + def target_notification_message |
| 43 | + description + "\n\n" + | ||
| 44 | + _('You need login to accept this.') | ||
| 42 | end | 45 | end |
| 46 | + | ||
| 43 | end | 47 | end |
app/models/task.rb
| @@ -51,11 +51,17 @@ class Task < ActiveRecord::Base | @@ -51,11 +51,17 @@ class Task < ActiveRecord::Base | ||
| 51 | end | 51 | end |
| 52 | 52 | ||
| 53 | after_create do |task| | 53 | after_create do |task| |
| 54 | - task.send(:send_notification, :created) | 54 | + begin |
| 55 | + task.send(:send_notification, :created) | ||
| 56 | + rescue NotImplementedError => ex | ||
| 57 | + RAILS_DEFAULT_LOGGER.info ex.to_s | ||
| 58 | + end | ||
| 55 | 59 | ||
| 56 | - target_msg = task.target_notification_message | ||
| 57 | - unless target_msg.nil? | 60 | + begin |
| 61 | + target_msg = task.target_notification_message | ||
| 58 | TaskMailer.deliver_target_notification(task, target_msg) | 62 | TaskMailer.deliver_target_notification(task, target_msg) |
| 63 | + rescue NotImplementedError => ex | ||
| 64 | + RAILS_DEFAULT_LOGGER.info ex.to_s | ||
| 59 | end | 65 | end |
| 60 | end | 66 | end |
| 61 | 67 | ||
| @@ -68,7 +74,11 @@ class Task < ActiveRecord::Base | @@ -68,7 +74,11 @@ class Task < ActiveRecord::Base | ||
| 68 | self.end_date = Time.now | 74 | self.end_date = Time.now |
| 69 | self.save! | 75 | self.save! |
| 70 | self.perform | 76 | self.perform |
| 71 | - send_notification(:finished) | 77 | + begin |
| 78 | + send_notification(:finished) | ||
| 79 | + rescue NotImplementedError => ex | ||
| 80 | + RAILS_DEFAULT_LOGGER.info ex.to_s | ||
| 81 | + end | ||
| 72 | end | 82 | end |
| 73 | end | 83 | end |
| 74 | 84 | ||
| @@ -79,7 +89,11 @@ class Task < ActiveRecord::Base | @@ -79,7 +89,11 @@ class Task < ActiveRecord::Base | ||
| 79 | self.status = Task::Status::CANCELLED | 89 | self.status = Task::Status::CANCELLED |
| 80 | self.end_date = Time.now | 90 | self.end_date = Time.now |
| 81 | self.save! | 91 | self.save! |
| 82 | - send_notification(:cancelled) | 92 | + begin |
| 93 | + send_notification(:cancelled) | ||
| 94 | + rescue NotImplementedError => ex | ||
| 95 | + RAILS_DEFAULT_LOGGER.info ex.to_s | ||
| 96 | + end | ||
| 83 | end | 97 | end |
| 84 | end | 98 | end |
| 85 | 99 | ||
| @@ -95,20 +109,19 @@ class Task < ActiveRecord::Base | @@ -95,20 +109,19 @@ class Task < ActiveRecord::Base | ||
| 95 | # The message that will be sent to the requestor of the task when the task is | 109 | # The message that will be sent to the requestor of the task when the task is |
| 96 | # created. | 110 | # created. |
| 97 | def task_created_message | 111 | def task_created_message |
| 98 | - # FIXME: use a date properly recorded. | ||
| 99 | - _("The task was created at %s") % Time.now | 112 | + raise NotImplementedError, "#{self} does not implement #task_created_message" |
| 100 | end | 113 | end |
| 101 | 114 | ||
| 102 | # The message that will be sent to the requestor of the task when its | 115 | # The message that will be sent to the requestor of the task when its |
| 103 | # finished. | 116 | # finished. |
| 104 | def task_finished_message | 117 | def task_finished_message |
| 105 | - _("The task was finished at %s") % (self.end_date.to_s) | 118 | + raise NotImplementedError, "#{self} does not implement #task_finished_message" |
| 106 | end | 119 | end |
| 107 | 120 | ||
| 108 | # The message that will be sent to the requestor of the task when its | 121 | # The message that will be sent to the requestor of the task when its |
| 109 | # cancelled. | 122 | # cancelled. |
| 110 | def task_cancelled_message | 123 | def task_cancelled_message |
| 111 | - _("The task was cancelled at %s") % (self.end_date.to_s) | 124 | + raise NotImplementedError, "#{self} does not implement #task_cancelled_message" |
| 112 | end | 125 | end |
| 113 | 126 | ||
| 114 | # The message that will be sent to the *target* of the task when it is | 127 | # The message that will be sent to the *target* of the task when it is |
| @@ -119,7 +132,7 @@ class Task < ActiveRecord::Base | @@ -119,7 +132,7 @@ class Task < ActiveRecord::Base | ||
| 119 | # not to be sent. If you want to send a notification to the target upon task | 132 | # not to be sent. If you want to send a notification to the target upon task |
| 120 | # creation, override this method and return a String. | 133 | # creation, override this method and return a String. |
| 121 | def target_notification_message | 134 | def target_notification_message |
| 122 | - nil | 135 | + raise NotImplementedError, "#{self} does not implement #target_notification_message" |
| 123 | end | 136 | end |
| 124 | 137 | ||
| 125 | # What permission is required to perform task? | 138 | # What permission is required to perform task? |
app/models/task_mailer.rb
| @@ -15,7 +15,7 @@ class TaskMailer < ActionMailer::Base | @@ -15,7 +15,7 @@ class TaskMailer < ActionMailer::Base | ||
| 15 | recipients task.target.contact_email | 15 | recipients task.target.contact_email |
| 16 | 16 | ||
| 17 | from self.class.generate_from(task) | 17 | from self.class.generate_from(task) |
| 18 | - subject task.description | 18 | + subject _('%s - %s') % [task.requestor.environment.name, task.description] |
| 19 | body :requestor => task.requestor.name, | 19 | body :requestor => task.requestor.name, |
| 20 | :target => task.target.name, | 20 | :target => task.target.name, |
| 21 | :message => msg, | 21 | :message => msg, |
app/views/task_mailer/target_notification.rhtml
test/unit/add_friend_test.rb
| @@ -57,26 +57,19 @@ class AddFriendTest < ActiveSupport::TestCase | @@ -57,26 +57,19 @@ class AddFriendTest < ActiveSupport::TestCase | ||
| 57 | ok('must validate when target is given') { task.errors.invalid?(:target_id)} | 57 | ok('must validate when target is given') { task.errors.invalid?(:target_id)} |
| 58 | end | 58 | end |
| 59 | 59 | ||
| 60 | - should 'not send e-mails' do | ||
| 61 | - | 60 | + should 'send e-mails' do |
| 62 | p1 = create_user('testuser1').person | 61 | p1 = create_user('testuser1').person |
| 63 | p2 = create_user('testuser2').person | 62 | p2 = create_user('testuser2').person |
| 64 | 63 | ||
| 65 | - TaskMailer.expects(:deliver_task_finished).never | ||
| 66 | - TaskMailer.expects(:deliver_task_created).never | 64 | + TaskMailer.expects(:deliver_target_notification).at_least_once |
| 67 | 65 | ||
| 68 | task = AddFriend.create!(:person => p1, :friend => p2) | 66 | task = AddFriend.create!(:person => p1, :friend => p2) |
| 69 | - task.finish | ||
| 70 | - | ||
| 71 | end | 67 | end |
| 72 | 68 | ||
| 73 | should 'provide proper description' do | 69 | should 'provide proper description' do |
| 74 | p1 = create_user('testuser1').person | 70 | p1 = create_user('testuser1').person |
| 75 | p2 = create_user('testuser2').person | 71 | p2 = create_user('testuser2').person |
| 76 | 72 | ||
| 77 | - TaskMailer.expects(:deliver_task_finished).never | ||
| 78 | - TaskMailer.expects(:deliver_task_created).never | ||
| 79 | - | ||
| 80 | task = AddFriend.create!(:person => p1, :friend => p2) | 73 | task = AddFriend.create!(:person => p1, :friend => p2) |
| 81 | 74 | ||
| 82 | assert_equal 'testuser1 wants to be your friend', task.description | 75 | assert_equal 'testuser1 wants to be your friend', task.description |
| @@ -96,4 +89,13 @@ class AddFriendTest < ActiveSupport::TestCase | @@ -96,4 +89,13 @@ class AddFriendTest < ActiveSupport::TestCase | ||
| 96 | end | 89 | end |
| 97 | end | 90 | end |
| 98 | 91 | ||
| 92 | + should 'override target notification message method from Task' do | ||
| 93 | + p1 = create_user('testuser1').person | ||
| 94 | + p2 = create_user('testuser2').person | ||
| 95 | + task = AddFriend.new(:person => p1, :friend => p2) | ||
| 96 | + assert_nothing_raised NotImplementedError do | ||
| 97 | + task.target_notification_message | ||
| 98 | + end | ||
| 99 | + end | ||
| 100 | + | ||
| 99 | end | 101 | end |
test/unit/add_member_test.rb
| @@ -9,6 +9,7 @@ class AddMemberTest < ActiveSupport::TestCase | @@ -9,6 +9,7 @@ class AddMemberTest < ActiveSupport::TestCase | ||
| 9 | should 'actually add memberships when confirmed' do | 9 | should 'actually add memberships when confirmed' do |
| 10 | p = create_user('testuser1').person | 10 | p = create_user('testuser1').person |
| 11 | c = Community.create!(:name => 'closed community', :closed => true) | 11 | c = Community.create!(:name => 'closed community', :closed => true) |
| 12 | + TaskMailer.stubs(:deliver_target_notification) | ||
| 12 | task = AddMember.create!(:person => p, :community => c) | 13 | task = AddMember.create!(:person => p, :community => c) |
| 13 | assert_difference c, :members, [p] do | 14 | assert_difference c, :members, [p] do |
| 14 | task.finish | 15 | task.finish |
| @@ -38,23 +39,20 @@ class AddMemberTest < ActiveSupport::TestCase | @@ -38,23 +39,20 @@ class AddMemberTest < ActiveSupport::TestCase | ||
| 38 | ok('must validate when target is given') { task.errors.invalid?(:target_id)} | 39 | ok('must validate when target is given') { task.errors.invalid?(:target_id)} |
| 39 | end | 40 | end |
| 40 | 41 | ||
| 41 | - should 'not send e-mails' do | 42 | + should 'send e-mails' do |
| 42 | p = create_user('testuser1').person | 43 | p = create_user('testuser1').person |
| 43 | c = Community.create!(:name => 'closed community', :closed => true) | 44 | c = Community.create!(:name => 'closed community', :closed => true) |
| 44 | 45 | ||
| 45 | - TaskMailer.expects(:deliver_task_finished).never | ||
| 46 | - TaskMailer.expects(:deliver_task_created).never | 46 | + TaskMailer.expects(:deliver_target_notification).at_least_once |
| 47 | 47 | ||
| 48 | task = AddMember.create!(:person => p, :community => c) | 48 | task = AddMember.create!(:person => p, :community => c) |
| 49 | - task.finish | ||
| 50 | end | 49 | end |
| 51 | 50 | ||
| 52 | should 'provide proper description' do | 51 | should 'provide proper description' do |
| 53 | p = create_user('testuser1').person | 52 | p = create_user('testuser1').person |
| 54 | c = Community.create!(:name => 'closed community', :closed => true) | 53 | c = Community.create!(:name => 'closed community', :closed => true) |
| 55 | 54 | ||
| 56 | - TaskMailer.expects(:deliver_task_finished).never | ||
| 57 | - TaskMailer.expects(:deliver_task_created).never | 55 | + TaskMailer.stubs(:deliver_target_notification) |
| 58 | 56 | ||
| 59 | task = AddMember.create!(:person => p, :community => c) | 57 | task = AddMember.create!(:person => p, :community => c) |
| 60 | 58 | ||
| @@ -74,6 +72,7 @@ class AddMemberTest < ActiveSupport::TestCase | @@ -74,6 +72,7 @@ class AddMemberTest < ActiveSupport::TestCase | ||
| 74 | should 'have roles' do | 72 | should 'have roles' do |
| 75 | p = create_user('testuser1').person | 73 | p = create_user('testuser1').person |
| 76 | c = Community.create!(:name => 'community_test') | 74 | c = Community.create!(:name => 'community_test') |
| 75 | + TaskMailer.stubs(:deliver_target_notification) | ||
| 77 | task = AddMember.create!(:roles => [1,2,3], :person => p, :community => c) | 76 | task = AddMember.create!(:roles => [1,2,3], :person => p, :community => c) |
| 78 | assert_equal [1,2,3], task.roles | 77 | assert_equal [1,2,3], task.roles |
| 79 | end | 78 | end |
| @@ -83,6 +82,7 @@ class AddMemberTest < ActiveSupport::TestCase | @@ -83,6 +82,7 @@ class AddMemberTest < ActiveSupport::TestCase | ||
| 83 | c = Community.create!(:name => 'community_test') | 82 | c = Community.create!(:name => 'community_test') |
| 84 | 83 | ||
| 85 | roles = [Profile::Roles.member, Profile::Roles.admin] | 84 | roles = [Profile::Roles.member, Profile::Roles.admin] |
| 85 | + TaskMailer.stubs(:deliver_target_notification) | ||
| 86 | task = AddMember.create!(:roles => roles.map(&:id), :person => p, :community => c) | 86 | task = AddMember.create!(:roles => roles.map(&:id), :person => p, :community => c) |
| 87 | task.finish | 87 | task.finish |
| 88 | 88 | ||
| @@ -91,4 +91,13 @@ class AddMemberTest < ActiveSupport::TestCase | @@ -91,4 +91,13 @@ class AddMemberTest < ActiveSupport::TestCase | ||
| 91 | assert_includes current_roles, roles[1] | 91 | assert_includes current_roles, roles[1] |
| 92 | end | 92 | end |
| 93 | 93 | ||
| 94 | + should 'override target notification message method from Task' do | ||
| 95 | + p1 = create_user('testuser1').person | ||
| 96 | + p2 = create_user('testuser2').person | ||
| 97 | + task = AddFriend.new(:person => p1, :friend => p2) | ||
| 98 | + assert_nothing_raised NotImplementedError do | ||
| 99 | + task.target_notification_message | ||
| 100 | + end | ||
| 101 | + end | ||
| 102 | + | ||
| 94 | end | 103 | end |
test/unit/approve_article_test.rb
| @@ -6,7 +6,7 @@ class ApproveArticleTest < ActiveSupport::TestCase | @@ -6,7 +6,7 @@ class ApproveArticleTest < ActiveSupport::TestCase | ||
| 6 | profile = create_user('test_user').person | 6 | profile = create_user('test_user').person |
| 7 | article = profile.articles.create!(:name => 'test article') | 7 | article = profile.articles.create!(:name => 'test article') |
| 8 | 8 | ||
| 9 | - a = ApproveArticle.create!(:name => 'test name', :article => article, :target => profile) | 9 | + a = ApproveArticle.create!(:name => 'test name', :article => article, :target => profile, :requestor => profile) |
| 10 | 10 | ||
| 11 | assert_equal 'test name', a.name | 11 | assert_equal 'test name', a.name |
| 12 | assert_equal article, a.article | 12 | assert_equal article, a.article |
| @@ -16,11 +16,20 @@ class ApproveArticleTest < ActiveSupport::TestCase | @@ -16,11 +16,20 @@ class ApproveArticleTest < ActiveSupport::TestCase | ||
| 16 | should 'create published article when finished' do | 16 | should 'create published article when finished' do |
| 17 | profile = create_user('test_user').person | 17 | profile = create_user('test_user').person |
| 18 | article = profile.articles.create!(:name => 'test article') | 18 | article = profile.articles.create!(:name => 'test article') |
| 19 | - a = ApproveArticle.create!(:name => 'test name', :article => article, :target => profile) | 19 | + a = ApproveArticle.create!(:name => 'test name', :article => article, :target => profile, :requestor => profile) |
| 20 | 20 | ||
| 21 | assert_difference PublishedArticle, :count do | 21 | assert_difference PublishedArticle, :count do |
| 22 | a.finish | 22 | a.finish |
| 23 | end | 23 | end |
| 24 | + end | ||
| 24 | 25 | ||
| 26 | + should 'override target notification message method from Task' do | ||
| 27 | + p1 = create_user('testuser1').person | ||
| 28 | + p2 = create_user('testuser2').person | ||
| 29 | + task = AddFriend.new(:person => p1, :friend => p2) | ||
| 30 | + assert_nothing_raised NotImplementedError do | ||
| 31 | + task.target_notification_message | ||
| 32 | + end | ||
| 25 | end | 33 | end |
| 34 | + | ||
| 26 | end | 35 | end |
test/unit/create_enterprise_test.rb
| @@ -161,10 +161,11 @@ class CreateEnterpriseTest < Test::Unit::TestCase | @@ -161,10 +161,11 @@ class CreateEnterpriseTest < Test::Unit::TestCase | ||
| 161 | end | 161 | end |
| 162 | 162 | ||
| 163 | should 'override message methods from Task' do | 163 | should 'override message methods from Task' do |
| 164 | - generic = Task.new | ||
| 165 | specific = CreateEnterprise.new | 164 | specific = CreateEnterprise.new |
| 166 | %w[ task_created_message task_finished_message task_cancelled_message ].each do |method| | 165 | %w[ task_created_message task_finished_message task_cancelled_message ].each do |method| |
| 167 | - assert_not_equal generic.send(method), specific.send(method) | 166 | + assert_nothing_raised NotImplementedError do |
| 167 | + specific.send(method) | ||
| 168 | + end | ||
| 168 | end | 169 | end |
| 169 | end | 170 | end |
| 170 | 171 |
test/unit/task_test.rb
| @@ -127,7 +127,7 @@ class TaskTest < Test::Unit::TestCase | @@ -127,7 +127,7 @@ class TaskTest < Test::Unit::TestCase | ||
| 127 | assert_nil Task.find_by_code(task.code) | 127 | assert_nil Task.find_by_code(task.code) |
| 128 | end | 128 | end |
| 129 | 129 | ||
| 130 | - should 'be able to find active tasks ' do | 130 | + should 'be able to find active tasks' do |
| 131 | task = Task.new | 131 | task = Task.new |
| 132 | task.requestor = sample_user | 132 | task.requestor = sample_user |
| 133 | task.save! | 133 | task.save! |
| @@ -144,11 +144,11 @@ class TaskTest < Test::Unit::TestCase | @@ -144,11 +144,11 @@ class TaskTest < Test::Unit::TestCase | ||
| 144 | assert_equal 7, Task.create(:code_length => 7).code.size | 144 | assert_equal 7, Task.create(:code_length => 7).code.size |
| 145 | end | 145 | end |
| 146 | 146 | ||
| 147 | - should 'not send notification to target when target_notification_message is nil (as in Task base class)' do | 147 | + should 'throws exception when try to send target_notification_message in Task base class' do |
| 148 | task = Task.new | 148 | task = Task.new |
| 149 | - TaskMailer.expects(:deliver_target_notification).never | ||
| 150 | - task.save! | ||
| 151 | - assert_nil task.target_notification_message | 149 | + assert_raise NotImplementedError do |
| 150 | + task.target_notification_message | ||
| 151 | + end | ||
| 152 | end | 152 | end |
| 153 | 153 | ||
| 154 | should 'send notification to target just after task creation' do | 154 | should 'send notification to target just after task creation' do |
| @@ -191,6 +191,22 @@ class TaskTest < Test::Unit::TestCase | @@ -191,6 +191,22 @@ class TaskTest < Test::Unit::TestCase | ||
| 191 | end | 191 | end |
| 192 | end | 192 | end |
| 193 | 193 | ||
| 194 | + should 'not deliver notification message to target' do | ||
| 195 | + task = Task.new | ||
| 196 | + assert_raise NotImplementedError do | ||
| 197 | + task.target_notification_message | ||
| 198 | + end | ||
| 199 | + end | ||
| 200 | + | ||
| 201 | + should 'not send message when created, finished or cancelled' do | ||
| 202 | + task = Task.new | ||
| 203 | + %w[ created finished cancelled ].each do |action| | ||
| 204 | + assert_raise NotImplementedError do | ||
| 205 | + task.send("task_#{action}_message") | ||
| 206 | + end | ||
| 207 | + end | ||
| 208 | + end | ||
| 209 | + | ||
| 194 | protected | 210 | protected |
| 195 | 211 | ||
| 196 | def sample_user | 212 | def sample_user |