From e60999ec5c789f1e4e91dd5c71f7c28348164b03 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Wed, 1 Jun 2016 18:24:38 -0300 Subject: Improve notification settings event keys and add some specs --- app/models/notification_setting.rb | 34 ++++++++-------- app/services/notification_service.rb | 62 ++++++++++++++++-------------- spec/services/notification_service_spec.rb | 56 ++++++++++++++++++++++++--- 3 files changed, 102 insertions(+), 50 deletions(-) diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index be4fcbd8467..3f8c5b2caa9 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -15,23 +15,21 @@ class NotificationSetting < ActiveRecord::Base scope :for_groups, -> { where(source_type: 'Namespace') } scope :for_projects, -> { where(source_type: 'Project') } - serialize :events - EMAIL_EVENTS = [ - :new_issue_email, - :new_note_email, - :closed_issue_email, - :reassigned_issue_email, - :relabeled_issue_email, - :new_merge_request_email, - :reassigned_merge_request_email, - :relabeled_merge_request_email, - :closed_merge_request_email, - :issue_status_changed_email, - :merged_merge_request_email, - :merge_request_status_email + :new_note, + :new_issue, + :reopen_issue, + :closed_issue, + :reassign_issue, + :new_merge_request, + :reopen_merge_request, + :close_merge_request, + :reassign_merge_request, + :merge_merge_request ] + store :events, accessors: EMAIL_EVENTS, coder: JSON + before_save :set_events def self.find_or_create_for(source) @@ -44,7 +42,13 @@ class NotificationSetting < ActiveRecord::Base setting end + # Set all event attributes as true when level is not custom def set_events - self.events = EMAIL_EVENTS if level == "watch" + # Level is a ENUM cannot compare to symbol + return if level == "custom" + + EMAIL_EVENTS.each do |event| + self.send("#{event}=", true) + end end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 768f513c195..38cc00f1a05 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -153,6 +153,9 @@ class NotificationService # Merge project watchers recipients = add_project_watchers(recipients, note.project) + # Merge project with custom notification + recipients = add_project_custom_notifications(recipients, note.project, :new_note) + # Reject users with Mention notification level, except those mentioned in _this_ note. recipients = reject_mention_users(recipients - mentioned_users, note.project) recipients = recipients + mentioned_users @@ -248,21 +251,22 @@ class NotificationService protected + # Get project/group users with CUSTOM notification level def add_project_custom_notifications(recipients, project, action) user_ids = [] - user_ids += project_member_notification(project, :custom, action) - user_ids += group_member_notification(project, :custom, action) + user_ids += notification_settings_for(project, :custom, action) + user_ids += notification_settings_for(project.group, :custom, action) recipients.concat(User.find(user_ids)) end # Get project users with WATCH notification level def project_watchers(project) - project_members = project_member_notification(project) + project_members = notification_settings_for(project) - users_with_project_level_global = project_member_notification(project, :global) - users_with_group_level_global = group_member_notification(project, :global) + users_with_project_level_global = notification_settings_for(project, :global) + users_with_group_level_global = notification_settings_for(project, :global) users = users_with_global_level_watch([users_with_project_level_global, users_with_group_level_global].flatten.uniq) users_with_project_setting = select_project_member_setting(project, users_with_project_level_global, users) @@ -271,23 +275,15 @@ class NotificationService User.where(id: users_with_project_setting.concat(users_with_group_setting).uniq).to_a end - def project_member_notification(project, notification_level=nil, action=nil) - if notification_level - settings = project.notification_settings.where(level: NotificationSetting.levels[notification_level]) - settings = settings.where(events: action.to_yaml) if action.present? - settings.pluck(:user_id) - else - project.notification_settings.pluck(:user_id) - end - end + def notification_settings_for(resource, notification_level = nil, action = nil) + return [] unless resource - def group_member_notification(project, notification_level, action=nil) - if project.group - settings = project.group.notification_settings.where(level: NotificationSetting.levels[notification_level]) - settings = settings.where(events: action.to_yaml) if action.present? - settings.pluck(:user_id) + if notification_level + settings = resource.notification_settings.where(level: NotificationSetting.levels[notification_level]) + settings = settings.select { |setting| setting.events[action] } if action.present? + settings.map(&:user_id) else - [] + resource.notification_settings.pluck(:user_id) end end @@ -301,7 +297,7 @@ class NotificationService # Build a list of users based on project notifcation settings def select_project_member_setting(project, global_setting, users_global_level_watch) - users = project_member_notification(project, :watch) + users = notification_settings_for(project, :watch) # If project setting is global, add to watch list if global setting is watch global_setting.each do |user_id| @@ -315,7 +311,7 @@ class NotificationService # Build a list of users based on group notification settings def select_group_member_setting(project, project_members, global_setting, users_global_level_watch) - uids = group_member_notification(project, :watch) + uids = notification_settings_for(project, :watch) # Group setting is watch, add to users list if user is not project member users = [] @@ -421,7 +417,7 @@ class NotificationService end def new_resource_email(target, project, method) - recipients = build_recipients(target, project, target.author, action: method) + recipients = build_recipients(target, project, target.author, action: "new") recipients.each do |recipient| mailer.send(method, recipient.id, target.id).deliver_later @@ -429,7 +425,8 @@ class NotificationService end def close_resource_email(target, project, current_user, method) - recipients = build_recipients(target, project, current_user, action: method) + action = method == :merged_merge_request_email ? "merge" : "close" + recipients = build_recipients(target, project, current_user, action: action) recipients.each do |recipient| mailer.send(method, recipient.id, target.id, current_user.id).deliver_later @@ -440,7 +437,7 @@ class NotificationService previous_assignee_id = previous_record(target, 'assignee_id') previous_assignee = User.find_by(id: previous_assignee_id) if previous_assignee_id - recipients = build_recipients(target, project, current_user, action: method, previous_assignee: previous_assignee) + recipients = build_recipients(target, project, current_user, action: "reassign", previous_assignee: previous_assignee) recipients.each do |recipient| mailer.send( @@ -463,7 +460,7 @@ class NotificationService end def reopen_resource_email(target, project, current_user, method, status) - recipients = build_recipients(target, project, current_user, action: method) + recipients = build_recipients(target, project, current_user, action: "reopen") recipients.each do |recipient| mailer.send(method, recipient.id, target.id, status, current_user.id).deliver_later @@ -471,9 +468,11 @@ class NotificationService end def build_recipients(target, project, current_user, action: nil, previous_assignee: nil) + custom_action = build_custom_key(action, target) + recipients = target.participants(current_user) recipients = add_project_watchers(recipients, project) - recipients = add_project_custom_notifications(recipients, project, action) + recipients = add_project_custom_notifications(recipients, project, custom_action) recipients = reject_mention_users(recipients, project) recipients = recipients.uniq @@ -481,7 +480,7 @@ class NotificationService # Re-assign is considered as a mention of the new assignee so we add the # new assignee to the list of recipients after we rejected users with # the "on mention" notification level - if [:reassigned_merge_request_email, :reassigned_issue_email].include?(action) + if [:reassign_merge_request, :reassign_issue].include?(custom_action) recipients << previous_assignee if previous_assignee recipients << target.assignee end @@ -489,7 +488,7 @@ class NotificationService recipients = reject_muted_users(recipients, project) recipients = add_subscribed_users(recipients, target) - if [:new_issue_email, :new_merge_request_email].include?(action) + if [:new_issue, :new_merge_request].include?(custom_action) recipients = add_labels_subscribers(recipients, target) end @@ -519,4 +518,9 @@ class NotificationService end end end + + # Builds key to be used if user has custom notification setting + def build_custom_key(action, object) + "#{action}_#{object.class.name.underscore}".to_sym + end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index b99e02ba678..49eaeabb8f4 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -46,6 +46,7 @@ describe NotificationService, services: true do 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') + update_custom_notification(:new_note) end describe :new_note do @@ -66,6 +67,7 @@ describe NotificationService, services: true do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@subscribed_participant) + should_not_email(@u_guest_custom) should_not_email(@u_guest_watcher) should_not_email(note.author) should_not_email(@u_participating) @@ -116,6 +118,7 @@ describe NotificationService, services: true do should_email(note.noteable.author) should_email(note.noteable.assignee) should_email(@u_mentioned) + should_not_email(@u_guest_custom) should_not_email(@u_guest_watcher) should_not_email(@u_watcher) should_not_email(note.author) @@ -238,13 +241,14 @@ describe NotificationService, services: true do build_team(note.project) ActionMailer::Base.deliveries.clear allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) + update_custom_notification(:new_note) end describe '#new_note, #perform_enqueued_jobs' do it do notification.new_note(note) - should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_committer) should_email(@u_watcher) should_not_email(@u_mentioned) @@ -285,6 +289,7 @@ describe NotificationService, services: true do build_team(issue.project) add_users_with_subscription(issue.project, issue) ActionMailer::Base.deliveries.clear + update_custom_notification(:new_issue) end describe '#new_issue' do @@ -294,6 +299,7 @@ describe NotificationService, services: true do should_email(issue.assignee) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_not_email(@u_mentioned) should_not_email(@u_participating) @@ -349,12 +355,15 @@ describe NotificationService, services: true do end describe '#reassigned_issue' do + before { update_custom_notification(:reassign_issue) } + it 'emails new assignee' do notification.reassigned_issue(issue, @u_disabled) should_email(issue.assignee) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(@unsubscriber) @@ -371,6 +380,7 @@ describe NotificationService, services: true do should_email(@u_mentioned) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(@unsubscriber) @@ -387,6 +397,7 @@ describe NotificationService, services: true do should_email(issue.assignee) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(@unsubscriber) @@ -403,6 +414,7 @@ describe NotificationService, services: true do should_email(issue.assignee) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(@unsubscriber) @@ -418,6 +430,7 @@ describe NotificationService, services: true do expect(issue.assignee).to be @u_mentioned should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_not_email(issue.assignee) @@ -518,6 +531,8 @@ describe NotificationService, services: true do end describe '#close_issue' do + before { update_custom_notification(:close_issue) } + it 'should sent email to issue assignee and issue author' do notification.close_issue(issue, @u_disabled) @@ -525,6 +540,7 @@ describe NotificationService, services: true do should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -564,6 +580,8 @@ describe NotificationService, services: true do end describe '#reopen_issue' do + before { update_custom_notification(:reopen_issue) } + it 'should send email to issue assignee and issue author' do notification.reopen_issue(issue, @u_disabled) @@ -571,6 +589,7 @@ describe NotificationService, services: true do should_email(issue.author) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -620,6 +639,8 @@ describe NotificationService, services: true do end describe '#new_merge_request' do + before { update_custom_notification(:new_merge_request) } + it do notification.new_merge_request(merge_request, @u_disabled) @@ -628,6 +649,7 @@ describe NotificationService, services: true do should_email(@watcher_and_subscriber) should_email(@u_participant_mentioned) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_not_email(@u_participating) should_not_email(@u_disabled) should_not_email(@u_lazy_participant) @@ -674,6 +696,8 @@ describe NotificationService, services: true do end describe '#reassigned_merge_request' do + before { update_custom_notification(:reassign_merge_request) } + it do notification.reassigned_merge_request(merge_request, merge_request.author) @@ -683,6 +707,7 @@ describe NotificationService, services: true do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -750,12 +775,15 @@ describe NotificationService, services: true do end describe '#closed_merge_request' do + before { update_custom_notification(:close_merge_request) } + it do notification.close_mr(merge_request, @u_disabled) should_email(merge_request.assignee) should_email(@u_watcher) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) @@ -796,6 +824,8 @@ describe NotificationService, services: true do end describe '#merged_merge_request' do + before { update_custom_notification(:merge_merge_request) } + it do notification.merge_mr(merge_request, @u_disabled) @@ -805,6 +835,7 @@ describe NotificationService, services: true do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -842,6 +873,8 @@ describe NotificationService, services: true do end describe '#reopen_merge_request' do + before { update_custom_notification(:reopen_merge_request) } + it do notification.reopen_mr(merge_request, @u_disabled) @@ -851,6 +884,7 @@ describe NotificationService, services: true do should_email(@subscriber) should_email(@watcher_and_subscriber) should_email(@u_guest_watcher) + should_email(@u_guest_custom) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) @@ -904,6 +938,7 @@ describe NotificationService, services: true do should_email(@u_participating) should_email(@u_lazy_participant) should_not_email(@u_guest_watcher) + should_not_email(@u_guest_custom) should_not_email(@u_disabled) end end @@ -924,7 +959,8 @@ describe NotificationService, services: true do # It should be treated with a :participating notification_level @u_lazy_participant = create(:user, username: 'lazy-participant') - create_guest_watcher + @u_guest_watcher = create_user_with_notification(:watch, 'guest_watching') + @u_guest_custom = create_user_with_notification(:custom, 'guest_custom') project.team << [@u_watcher, :master] project.team << [@u_participating, :master] @@ -944,10 +980,18 @@ describe NotificationService, services: true do user end - def create_guest_watcher - @u_guest_watcher = create(:user, username: 'guest_watching') - setting = @u_guest_watcher.notification_settings_for(project) - setting.level = :watch + def create_user_with_notification(level, username) + user = create(:user, username: username) + setting = user.notification_settings_for(project) + setting.level = level + setting.save + + user + end + + def update_custom_notification(event) + setting = @u_guest_custom.notification_settings_for(project) + setting.events[event] = true setting.save end -- cgit v1.2.1