From 9dbd7e5aec921e43f3ea89c8e3357ca0174b0937 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Sat, 7 Feb 2015 00:23:58 +0100 Subject: [PATCH] Allow notification email to be set separately from primary email. Closes #1932. --- CHANGELOG | 1 + app/controllers/admin/users_controller.rb | 3 + app/controllers/profiles/emails_controller.rb | 3 + .../profiles/notifications_controller.rb | 22 +++++- app/mailers/emails/profile.rb | 6 +- app/mailers/emails/projects.rb | 2 +- app/mailers/notify.rb | 2 +- app/models/user.rb | 18 +++++ app/views/profiles/emails/index.html.haml | 6 +- .../profiles/notifications/show.html.haml | 73 ++++++++++++------- ...06222854_add_notification_email_to_user.rb | 11 +++ db/schema.rb | 3 +- spec/mailers/notify_spec.rb | 9 ++- 13 files changed, 120 insertions(+), 39 deletions(-) create mode 100644 db/migrate/20150206222854_add_notification_email_to_user.rb diff --git a/CHANGELOG b/CHANGELOG index 7addfa7f356..7f20f97d142 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -60,6 +60,7 @@ v 7.8.0 - API: Access groups with their path (Julien Bianchi) - Added link to milestone and keeping resource context on smaller viewports for issues and merge requests (Jason Blanchard) - + - Allow notification email to be set separately from primary email. - - API: Add support for editing an existing project (Mika Mäenpää and Hannes Rosenögger) - diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index aea8545d38e..b4c78814a19 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -102,6 +102,9 @@ class Admin::UsersController < Admin::ApplicationController email = user.emails.find(params[:email_id]) email.destroy + user.set_notification_email + user.save if user.notification_email_changed? + respond_to do |format| format.html { redirect_to :back, notice: "Successfully removed email." } format.js { render nothing: true } diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb index f3f0e69b83a..4a65c978e5c 100644 --- a/app/controllers/profiles/emails_controller.rb +++ b/app/controllers/profiles/emails_controller.rb @@ -18,6 +18,9 @@ class Profiles::EmailsController < ApplicationController @email = current_user.emails.find(params[:id]) @email.destroy + current_user.set_notification_email + current_user.save if current_user.notification_email_changed? + respond_to do |format| format.html { redirect_to profile_emails_url } format.js { render nothing: true } diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 638d1f9789b..433c19189af 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -2,6 +2,7 @@ class Profiles::NotificationsController < ApplicationController layout 'profile' def show + @user = current_user @notification = current_user.notification @project_members = current_user.project_members @group_members = current_user.group_members @@ -11,8 +12,7 @@ class Profiles::NotificationsController < ApplicationController type = params[:notification_type] @saved = if type == 'global' - current_user.notification_level = params[:notification_level] - current_user.save + current_user.update_attributes(user_params) elsif type == 'group' users_group = current_user.group_members.find(params[:notification_id]) users_group.notification_level = params[:notification_level] @@ -22,5 +22,23 @@ class Profiles::NotificationsController < ApplicationController project_member.notification_level = params[:notification_level] project_member.save end + + respond_to do |format| + format.html do + if @saved + flash[:notice] = "Notification settings saved" + else + flash[:alert] = "Failed to save new settings" + end + + redirect_to :back + end + + format.js + end + end + + def user_params + params.require(:user).permit(:notification_email, :notification_level) end end diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb index 6d7f8eb4b02..ab5b0765352 100644 --- a/app/mailers/emails/profile.rb +++ b/app/mailers/emails/profile.rb @@ -4,20 +4,20 @@ module Emails @user = User.find(user_id) @target_url = user_url(@user) @token = token - mail(to: @user.email, subject: subject("Account was created for you")) + mail(to: @user.notification_email, subject: subject("Account was created for you")) end def new_email_email(email_id) @email = Email.find(email_id) @user = @email.user - mail(to: @user.email, subject: subject("Email was added to your account")) + mail(to: @user.notification_email, subject: subject("Email was added to your account")) end def new_ssh_key_email(key_id) @key = Key.find(key_id) @user = @key.user @target_url = user_url(@user) - mail(to: @user.email, subject: subject("SSH key was added to your account")) + mail(to: @user.notification_email, subject: subject("SSH key was added to your account")) end end end diff --git a/app/mailers/emails/projects.rb b/app/mailers/emails/projects.rb index d6edfd7059f..dc2ebc969c1 100644 --- a/app/mailers/emails/projects.rb +++ b/app/mailers/emails/projects.rb @@ -12,7 +12,7 @@ module Emails @user = User.find user_id @project = Project.find project_id @target_url = project_url(@project) - mail(to: @user.email, + mail(to: @user.notification_email, subject: subject("Project was moved")) end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 5ae07d771fa..45fc53fcdb2 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -60,7 +60,7 @@ class Notify < ActionMailer::Base # Returns a String containing the User's email address. def recipient(recipient_id) if recipient = User.find(recipient_id) - recipient.email + recipient.notification_email end end diff --git a/app/models/user.rb b/app/models/user.rb index 552a37c9533..34dedb057d5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -43,6 +43,7 @@ # website_url :string(255) default(""), not null # last_credential_check_at :datetime # github_access_token :string(255) +# notification_email :string(255) # require 'carrierwave/orm/activerecord' @@ -114,6 +115,7 @@ class User < ActiveRecord::Base # validates :name, presence: true validates :email, presence: true, email: { strict_mode: true }, uniqueness: true + validates :notification_email, presence: true, email: { strict_mode: true } validates :bio, length: { maximum: 255 }, allow_blank: true validates :projects_limit, presence: true, numericality: { greater_than_or_equal_to: 0 } validates :username, @@ -127,10 +129,12 @@ class User < ActiveRecord::Base validate :namespace_uniq, if: ->(user) { user.username_changed? } validate :avatar_type, if: ->(user) { user.avatar_changed? } validate :unique_email, if: ->(user) { user.email_changed? } + validate :owns_notification_email, if: ->(user) { user.notification_email_changed? } validates :avatar, file_size: { maximum: 200.kilobytes.to_i } before_validation :generate_password, on: :create before_validation :sanitize_attrs + before_validation :set_notification_email, if: ->(user) { user.email_changed? } before_save :ensure_authentication_token after_save :ensure_namespace_correct @@ -286,6 +290,10 @@ class User < ActiveRecord::Base self.errors.add(:email, 'has already been taken') if Email.exists?(email: self.email) end + def owns_notification_email + self.errors.add(:notification_email, "is not an email you own") unless self.all_emails.include?(self.notification_email) + end + # Groups user has access to def authorized_groups @authorized_groups ||= begin @@ -431,6 +439,12 @@ class User < ActiveRecord::Base end end + def set_notification_email + if self.notification_email.blank? || !self.all_emails.include?(self.notification_email) + self.notification_email = self.email + end + end + def requires_ldap_check? if !Gitlab.config.ldap.enabled false @@ -504,6 +518,10 @@ class User < ActiveRecord::Base end end + def all_emails + [self.email, *self.emails.map(&:email)] + end + def hook_attrs { name: name, diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index ca980db2f3c..0b30e772336 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/app/views/profiles/emails/index.html.haml @@ -3,7 +3,11 @@ %p.light Your %b Primary Email - will be used for account notifications, avatar detection and web based operations, such as edits and merges. + will be used for avatar detection and web based operations, such as edits and merges. + %br + Your + %b Notification Email + will be used for account notifications. %br All email addresses will be used to identify your commits. diff --git a/app/views/profiles/notifications/show.html.haml b/app/views/profiles/notifications/show.html.haml index bc6f76a2661..28bc5a426ac 100644 --- a/app/views/profiles/notifications/show.html.haml +++ b/app/views/profiles/notifications/show.html.haml @@ -1,40 +1,57 @@ %h3.page-title Notifications settings %p.light - GitLab uses the email specified in your profile for notifications + These are your global notification settings. %hr -= form_tag profile_notifications_path, method: :put, remote: true, class: 'update-notifications form-horizontal global-notifications-form' do + + += form_for @user, url: profile_notifications_path, method: :put, html: { class: 'update-notifications form-horizontal global-notifications-form' } do |f| + -if @user.errors.any? + %div.alert.alert-danger + %ul + - @user.errors.full_messages.each do |msg| + %li= msg + = hidden_field_tag :notification_type, 'global' - = label_tag :notification_level, 'Notification level', class: 'control-label' - .col-sm-10 - .radio - = label_tag nil, class: '' do - = radio_button_tag :notification_level, Notification::N_DISABLED, @notification.disabled?, class: 'trigger-submit' - .level-title - Disabled - %p You will not get any notifications via email + .form-group + = f.label :notification_email, class: "control-label" + .col-sm-10 + = f.select :notification_email, @user.all_emails, { include_blank: false }, class: "form-control" - .radio - = label_tag nil, class: '' do - = radio_button_tag :notification_level, Notification::N_MENTION, @notification.mention?, class: 'trigger-submit' - .level-title - Mention - %p You will receive notifications only for comments in which you were @mentioned + .form-group + = f.label :notification_level, class: 'control-label' + .col-sm-10 + .radio + = f.label :notification_level, value: Notification::N_DISABLED do + = f.radio_button :notification_level, Notification::N_DISABLED + .level-title + Disabled + %p You will not get any notifications via email - .radio - = label_tag nil, class: '' do - = radio_button_tag :notification_level, Notification::N_PARTICIPATING, @notification.participating?, class: 'trigger-submit' - .level-title - Participating - %p You will only receive notifications from related resources (e.g. from your commits or assigned issues) + .radio + = f.label :notification_level, value: Notification::N_MENTION do + = f.radio_button :notification_level, Notification::N_MENTION + .level-title + Mention + %p You will receive notifications only for comments in which you were @mentioned - .radio - = label_tag nil, class: '' do - = radio_button_tag :notification_level, Notification::N_WATCH, @notification.watch?, class: 'trigger-submit' - .level-title - Watch - %p You will receive all notifications from projects in which you participate + .radio + = f.label :notification_level, value: Notification::N_PARTICIPATING do + = f.radio_button :notification_level, Notification::N_PARTICIPATING + .level-title + Participating + %p You will only receive notifications from related resources (e.g. from your commits or assigned issues) + + .radio + = f.label :notification_level, value: Notification::N_WATCH do + = f.radio_button :notification_level, Notification::N_WATCH + .level-title + Watch + %p You will receive all notifications from projects in which you participate + + .form-actions + = f.submit 'Save changes', class: "btn btn-save" .clearfix %hr diff --git a/db/migrate/20150206222854_add_notification_email_to_user.rb b/db/migrate/20150206222854_add_notification_email_to_user.rb new file mode 100644 index 00000000000..ab80f7e582f --- /dev/null +++ b/db/migrate/20150206222854_add_notification_email_to_user.rb @@ -0,0 +1,11 @@ +class AddNotificationEmailToUser < ActiveRecord::Migration + def up + add_column :users, :notification_email, :string + + execute "UPDATE users SET notification_email = email" + end + + def down + remove_column :users, :notification_email + end +end diff --git a/db/schema.rb b/db/schema.rb index 0e4af3df7c2..e679f0106d3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150125163100) do +ActiveRecord::Schema.define(version: 20150206222854) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -436,6 +436,7 @@ ActiveRecord::Schema.define(version: 20150125163100) do t.string "website_url", default: "", null: false t.string "github_access_token" t.string "gitlab_access_token" + t.string "notification_email" end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index a0c37587b23..5af622f946e 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -9,9 +9,14 @@ describe Notify do let(:recipient) { create(:user, email: 'recipient@example.com') } let(:project) { create(:project) } + before(:each) do + email = recipient.emails.create(email: "notifications@example.com") + recipient.update_attribute(:notification_email, email.email) + end + shared_examples 'a multiple recipients email' do it 'is sent to the given recipient' do - should deliver_to recipient.email + should deliver_to recipient.notification_email end end @@ -441,7 +446,7 @@ describe Notify do end it 'is sent to the given recipient' do - should deliver_to recipient.email + should deliver_to recipient.notification_email end it 'contains the message from the note' do