diff options
12 files changed, 188 insertions, 6 deletions
diff --git a/app/assets/javascripts/profile/profile.js b/app/assets/javascripts/profile/profile.js index c38bc762675..4ccea0624ee 100644 --- a/app/assets/javascripts/profile/profile.js +++ b/app/assets/javascripts/profile/profile.js @@ -25,6 +25,7 @@ bindEvents() { $('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm); $('#user_notification_email').on('change', this.submitForm); + $('#user_notified_of_own_activity').on('change', this.submitForm); $('.update-username').on('ajax:before', this.beforeUpdateUsername); $('.update-username').on('ajax:complete', this.afterUpdateUsername); $('.update-notifications').on('ajax:success', this.onUpdateNotifs); diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index b8b71d295f6..a271e2dfc4b 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -17,6 +17,6 @@ class Profiles::NotificationsController < Profiles::ApplicationController end def user_params - params.require(:user).permit(:notification_email) + params.require(:user).permit(:notification_email, :notified_of_own_activity) end end diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 44ae23fad18..940e850600f 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -38,7 +38,7 @@ class NotificationRecipientService recipients = reject_unsubscribed_users(recipients, target) recipients = reject_users_without_access(recipients, target) - recipients.delete(current_user) if skip_current_user + recipients.delete(current_user) if skip_current_user && !current_user.notified_of_own_activity? recipients.uniq end @@ -47,7 +47,7 @@ class NotificationRecipientService recipients = add_labels_subscribers([], target, labels: labels) recipients = reject_unsubscribed_users(recipients, target) recipients = reject_users_without_access(recipients, target) - recipients.delete(current_user) + recipients.delete(current_user) unless current_user.notified_of_own_activity? recipients.uniq end @@ -88,7 +88,7 @@ class NotificationRecipientService recipients = reject_unsubscribed_users(recipients, note.noteable) recipients = reject_users_without_access(recipients, note.noteable) - recipients.delete(note.author) + recipients.delete(note.author) unless note.author.notified_of_own_activity? recipients.uniq end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index f9aa2346759..2c6f849259e 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -280,8 +280,9 @@ class NotificationService recipients ||= NotificationRecipientService.new(pipeline.project).build_recipients( pipeline, - nil, # The acting user, who won't be added to recipients - action: pipeline.status).map(&:notification_email) + pipeline.user, + action: pipeline.status, + skip_current_user: false).map(&:notification_email) if recipients.any? mailer.public_send(email_template, pipeline, recipients).deliver_later diff --git a/app/views/profiles/notifications/show.html.haml b/app/views/profiles/notifications/show.html.haml index 5c5e5940365..51c4e8e5a73 100644 --- a/app/views/profiles/notifications/show.html.haml +++ b/app/views/profiles/notifications/show.html.haml @@ -34,6 +34,11 @@ .clearfix + = form_for @user, url: profile_notifications_path, method: :put do |f| + %label{ for: 'user_notified_of_own_activity' } + = f.check_box :notified_of_own_activity + %span Receive notifications about your own activity + %hr %h5 Groups (#{@group_notifications.count}) diff --git a/changelogs/unreleased/option-to-be-notified-of-own-activity.yml b/changelogs/unreleased/option-to-be-notified-of-own-activity.yml new file mode 100644 index 00000000000..c2e0410cc33 --- /dev/null +++ b/changelogs/unreleased/option-to-be-notified-of-own-activity.yml @@ -0,0 +1,4 @@ +--- +title: Add option to receive email notifications about your own activity +merge_request: 8836 +author: Richard Macklin diff --git a/db/migrate/20170316061730_readd_notified_of_own_activity_to_users.rb b/db/migrate/20170316061730_readd_notified_of_own_activity_to_users.rb new file mode 100644 index 00000000000..52c0b682fc8 --- /dev/null +++ b/db/migrate/20170316061730_readd_notified_of_own_activity_to_users.rb @@ -0,0 +1,14 @@ +class ReaddNotifiedOfOwnActivityToUsers < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + add_column :users, :notified_of_own_activity, :boolean + end + + def down + remove_column :users, :notified_of_own_activity + end +end diff --git a/db/post_migrate/20170317131326_add_notified_of_own_activity_default.rb b/db/post_migrate/20170317131326_add_notified_of_own_activity_default.rb new file mode 100644 index 00000000000..f01ef6b4119 --- /dev/null +++ b/db/post_migrate/20170317131326_add_notified_of_own_activity_default.rb @@ -0,0 +1,20 @@ +class AddNotifiedOfOwnActivityDefault < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + DOWNTIME = false + + def up + begin + update_column_in_batches(:users, :notified_of_own_activity, false) do |table, query| + query.where(table[:notified_of_own_activity].eq(nil)) + end + + change_column :users, :notified_of_own_activity, :boolean, default: false, null: false + end + end + + def down + change_column_default :users, :notified_of_own_activity, nil + end +end diff --git a/db/schema.rb b/db/schema.rb index 904fef4a381..47833b7e518 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1236,6 +1236,7 @@ ActiveRecord::Schema.define(version: 20170317203554) do t.string "organization" t.boolean "authorized_projects_populated" t.boolean "ghost" + t.boolean "notified_of_own_activity", default: false, null: false end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/spec/controllers/profiles/notifications_controller_spec.rb b/spec/controllers/profiles/notifications_controller_spec.rb new file mode 100644 index 00000000000..58caf7999cf --- /dev/null +++ b/spec/controllers/profiles/notifications_controller_spec.rb @@ -0,0 +1,45 @@ +require 'spec_helper' + +describe Profiles::NotificationsController do + let(:user) do + create(:user) do |user| + user.emails.create(email: 'original@example.com') + user.emails.create(email: 'new@example.com') + user.update(notification_email: 'original@example.com') + user.save! + end + end + + describe 'GET show' do + it 'renders' do + sign_in(user) + + get :show + + expect(response).to render_template :show + end + end + + describe 'POST update' do + it 'updates only permitted attributes' do + sign_in(user) + + put :update, user: { notification_email: 'new@example.com', notified_of_own_activity: true, admin: true } + + user.reload + expect(user.notification_email).to eq('new@example.com') + expect(user.notified_of_own_activity).to eq(true) + expect(user.admin).to eq(false) + expect(controller).to set_flash[:notice].to('Notification settings saved') + end + + it 'shows an error message if the params are invalid' do + sign_in(user) + + put :update, user: { notification_email: '' } + + expect(user.reload.notification_email).to eq('original@example.com') + expect(controller).to set_flash[:alert].to('Failed to save new settings') + end + end +end diff --git a/spec/features/profiles/user_changes_notified_of_own_activity_spec.rb b/spec/features/profiles/user_changes_notified_of_own_activity_spec.rb new file mode 100644 index 00000000000..e05fbb3715c --- /dev/null +++ b/spec/features/profiles/user_changes_notified_of_own_activity_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +feature 'Profile > Notifications > User changes notified_of_own_activity setting', feature: true, js: true do + let(:user) { create(:user) } + + before do + login_as(user) + end + + scenario 'User opts into receiving notifications about their own activity' do + visit profile_notifications_path + + expect(page).not_to have_checked_field('user[notified_of_own_activity]') + + check 'user[notified_of_own_activity]' + + expect(page).to have_content('Notification settings saved') + expect(page).to have_checked_field('user[notified_of_own_activity]') + end + + scenario 'User opts out of receiving notifications about their own activity' do + user.update!(notified_of_own_activity: true) + visit profile_notifications_path + + expect(page).to have_checked_field('user[notified_of_own_activity]') + + uncheck 'user[notified_of_own_activity]' + + expect(page).to have_content('Notification settings saved') + expect(page).not_to have_checked_field('user[notified_of_own_activity]') + end +end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f7240969588..229291f19e9 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -146,6 +146,16 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end + it "emails the note author if they've opted into notifications about their activity" do + add_users_with_subscription(note.project, issue) + note.author.notified_of_own_activity = true + reset_delivered_emails! + + notification.new_note(note) + + should_email(note.author) + end + it 'filters out "mentioned in" notes' do mentioned_note = SystemNoteService.cross_reference(mentioned_issue, issue, issue.author) @@ -476,6 +486,20 @@ describe NotificationService, services: true do should_not_email(issue.assignee) end + it "emails the author if they've opted into notifications about their activity" do + issue.author.notified_of_own_activity = true + + notification.new_issue(issue, issue.author) + + should_email(issue.author) + end + + it "doesn't email the author if they haven't opted into notifications about their activity" do + notification.new_issue(issue, issue.author) + + should_not_email(issue.author) + end + it "emails subscribers of the issue's labels" do user_1 = create(:user) user_2 = create(:user) @@ -665,6 +689,19 @@ describe NotificationService, services: true do should_email(subscriber_to_label_2) end + it "emails the current user if they've opted into notifications about their activity" do + subscriber_to_label_2.notified_of_own_activity = true + notification.relabeled_issue(issue, [group_label_2, label_2], subscriber_to_label_2) + + should_email(subscriber_to_label_2) + end + + it "doesn't email the current user if they haven't opted into notifications about their activity" do + notification.relabeled_issue(issue, [group_label_2, label_2], subscriber_to_label_2) + + should_not_email(subscriber_to_label_2) + end + it "doesn't send email to anyone but subscribers of the given labels" do notification.relabeled_issue(issue, [group_label_2, label_2], @u_disabled) @@ -845,6 +882,20 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end + it "emails the author if they've opted into notifications about their activity" do + merge_request.author.notified_of_own_activity = true + + notification.new_merge_request(merge_request, merge_request.author) + + should_email(merge_request.author) + end + + it "doesn't email the author if they haven't opted into notifications about their activity" do + notification.new_merge_request(merge_request, merge_request.author) + + should_not_email(merge_request.author) + end + it "emails subscribers of the merge request's labels" do user_1 = create(:user) user_2 = create(:user) @@ -1040,6 +1091,14 @@ describe NotificationService, services: true do should_not_email(@u_watcher) end + it "notifies the merger when the pipeline succeeds is false but they've opted into notifications about their activity" do + merge_request.merge_when_pipeline_succeeds = false + @u_watcher.notified_of_own_activity = true + notification.merge_mr(merge_request, @u_watcher) + + should_email(@u_watcher) + end + it_behaves_like 'participating notifications' do let(:participant) { create(:user, username: 'user-participant') } let(:issuable) { merge_request } |