Merge branch 'duplicate_notifications_fix' into 'master'

Fixed duplicated issue note email notifications.

Fixes #2560 



See issue for the details.

Without `uniq` modified tests were failing with:
```
     Failure/Error: notification.new_note(note)
       (Notify (class)).note_issue_email(21, 1)
           expected: 1 time with arguments: (21, 1)
           received: 2 times with arguments: (21, 1)
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/extensions/action_mailer.rb:17:in `public_send'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/extensions/action_mailer.rb:17:in `perform'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/testing.rb:74:in `block in raw_push'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/testing.rb:69:in `each'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/testing.rb:69:in `raw_push'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/client.rb:68:in `push'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/worker.rb:85:in `client_push'
     # /home/bak1an/.rvm/gems/ruby-2.1.6@gitlab/gems/sidekiq-3.3.0/lib/sidekiq/extensions/generic_proxy.rb:19:in `method_missing'
     # ./app/services/notification_service.rb:144:in `block in new_note'
     # ./app/services/notification_service.rb:143:in `each'
     # ./app/services/notification_service.rb:143:in `new_note'
     # ./spec/services/notification_service_spec.rb:63:in `block (5 levels) in <top (required)>'
```

I have also added `once` to all `should_email` checks within `notification_service_spec.rb` since it's probably the correct behavior to notify users only once on the same event. Nothing else failed out of the box but we can keep these assertions for future.

See merge request !1925
This commit is contained in:
Stan Hu
2015-12-06 15:13:53 +00:00
3 changed files with 8 additions and 3 deletions
+1
View File
@@ -23,6 +23,7 @@ v 8.2.2
- Prevent "413 Request entity too large" errors when pushing large files with LFS
- Fix invalid links within projects dashboard header
- Make current user the first user in assignee dropdown in issues detail page (Stan Hu)
- Fix: duplicate email notifications on issue comments
v 8.2.1
- Forcefully update builds that didn't want to update with state machine
+1
View File
@@ -145,6 +145,7 @@ class NotificationService
recipients = reject_unsubscribed_users(recipients, note.noteable)
recipients.delete(note.author)
recipients = recipients.uniq
# build notify method like 'note_commit_email'
notify_method = "note_#{note.noteable_type.underscore}_email".to_sym
+6 -3
View File
@@ -45,6 +45,7 @@ describe NotificationService do
project.team << [issue.author, :master]
project.team << [issue.assignee, :master]
project.team << [note.author, :master]
create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy')
end
describe :new_note do
@@ -60,6 +61,7 @@ describe NotificationService do
should_email(note.noteable.assignee)
should_email(@u_mentioned)
should_email(@subscriber)
should_email(@subscribed_participant)
should_not_email(note.author)
should_not_email(@u_participating)
should_not_email(@u_disabled)
@@ -381,18 +383,19 @@ describe NotificationService do
def add_users_with_subscription(project, issuable)
@subscriber = create :user
@unsubscriber = create :user
@subscribed_participant = create(:user, username: 'subscribed_participant', notification_level: Notification::N_PARTICIPATING)
project.team << [@subscribed_participant, :master]
project.team << [@subscriber, :master]
project.team << [@unsubscriber, :master]
issuable.subscriptions.create(user: @subscriber, subscribed: true)
issuable.subscriptions.create(user: @subscribed_participant, subscribed: true)
issuable.subscriptions.create(user: @unsubscriber, subscribed: false)
end
def sent_to_user?(user)
ActionMailer::Base.deliveries.any? do |message|
message.to.include?(user.email)
end
ActionMailer::Base.deliveries.map(&:to).flatten.count(user.email) == 1
end
def should_email(user)