Merge branch 'change-primary-email' into 'master'

Allow primary email to be set to an email that you've already added.

Fixes gitlab-com/support-forum#106.

When the user sets their primary email to an email that they've already added to their account, this patch makes sure that secondary email record is destroyed, and a new email record is created for the old primary email. This is based on the assumption that in this case no email was meant to be deleted, but the user simply wanted to change which of their emails is primary.

See merge request !591

6 files changed, 46 insertions, 18 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 104d3869f93..52783cbdd92 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,7 @@ v 7.11.0 (unreleased)
   - Add application setting to restrict user signups to e-mail domains (Stan Hu)
   - Don't allow a merge request to be merged when its title starts with "WIP".
   - Add a page title to every page.
+  - Allow primary email to be set to an email that you've already added.
   - Get Gitorious importer to work again.
   - Fix clone URL field and X11 Primary selection (Dmitry Medvinsky)
   - Ignore invalid lines in .gitmodules
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index adb83996f8b..d36e359934c 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -102,8 +102,7 @@ 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?
+    user.update_secondary_emails!
 
     respond_to do |format|
       format.html { redirect_to :back, notice: "Successfully removed email." }
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 3e904700de5..0ede9b8e21b 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -1,14 +1,17 @@
 class Profiles::EmailsController < Profiles::ApplicationController
   def index
     @primary = current_user.email
-    @public_email = current_user.public_email
     @emails = current_user.emails
   end
 
   def create
     @email = current_user.emails.new(email_params)
 
-    flash[:alert] = @email.errors.full_messages.first unless @email.save
+    if @email.save
+      NotificationService.new.new_email(@email)
+    else
+      flash[:alert] = @email.errors.full_messages.first
+    end
 
     redirect_to profile_emails_url
   end
@@ -17,9 +20,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
     @email = current_user.emails.find(params[:id])
     @email.destroy
 
-    current_user.set_notification_email
-    current_user.set_public_email
-    current_user.save if current_user.notification_email_changed? or current_user.public_email_changed?
+    current_user.update_secondary_emails!
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url }
diff --git a/app/models/email.rb b/app/models/email.rb
index 556b0e9586e..935705e2ed4 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -18,7 +18,6 @@ class Email < ActiveRecord::Base
   validates :email, presence: true, email: { strict_mode: true }, uniqueness: true
   validate :unique_email, if: ->(email) { email.email_changed? }
 
-  after_create :notify
   before_validation :cleanup_email
 
   def cleanup_email
@@ -28,8 +27,4 @@ class Email < ActiveRecord::Base
   def unique_email
     self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
   end
-
-  def notify
-    NotificationService.new.new_email(self)
-  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index f22fdc28435..9f198368129 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -139,6 +139,7 @@ class User < ActiveRecord::Base
   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? }
+  validate :owns_public_email, if: ->(user) { user.public_email_changed? }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
   before_validation :generate_password, on: :create
@@ -147,6 +148,7 @@ class User < ActiveRecord::Base
   before_validation :set_notification_email, if: ->(user) { user.email_changed? }
   before_validation :set_public_email, if: ->(user) { user.public_email_changed? }
 
+  after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? }
   before_save :ensure_authentication_token
   after_save :ensure_namespace_correct
   after_initialize :set_projects_limit
@@ -277,13 +279,29 @@ class User < ActiveRecord::Base
   end
 
   def unique_email
-    self.errors.add(:email, 'has already been taken') if Email.exists?(email: self.email)
+    if !self.emails.exists?(email: self.email) && Email.exists?(email: self.email)
+      self.errors.add(:email, 'has already been taken')
+    end
   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
 
+  def owns_public_email
+    self.errors.add(:public_email, "is not an email you own") unless self.all_emails.include?(self.public_email)
+  end
+
+  def update_emails_with_primary_email
+    primary_email_record = self.emails.find_by(email: self.email)
+    if primary_email_record
+      primary_email_record.destroy
+      self.emails.create(email: self.email_was)
+      
+      self.update_secondary_emails!
+    end
+  end
+
   # Groups user has access to
   def authorized_groups
     @authorized_groups ||= begin
@@ -449,10 +467,16 @@ class User < ActiveRecord::Base
 
   def set_public_email
     if self.public_email.blank? || !self.all_emails.include?(self.public_email)
-      self.public_email = ''
+      self.public_email = nil
     end
   end
 
+  def update_secondary_emails!
+    self.set_notification_email
+    self.set_public_email
+    self.save if self.notification_email_changed? || self.public_email_changed?
+  end
+
   def set_projects_limit
     connection_default_value_defined = new_record? && !projects_limit_changed?
     return unless self.projects_limit.nil? || connection_default_value_defined
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index c17e01425d8..2c0d0e10a4c 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -5,11 +5,15 @@
   Your
   %b Primary Email
   will be used for avatar detection and web based operations, such as edits and merges.
-  %br
+%p.light
   Your
   %b Notification Email
   will be used for account notifications.
-  %br
+%p.light
+  Your
+  %b Public Email
+  will be displayed on your public profile.
+%p.light
   All email addresses will be used to identify your commits.
 
 %hr
@@ -21,13 +25,17 @@
     %li
       %strong= @primary
       %span.label.label-success Primary Email
-      - if @primary === @public_email
+      - if @primary === current_user.public_email
         %span.label.label-info Public Email
+      - if @primary === current_user.notification_email
+        %span.label.label-info Notification Email
     - @emails.each do |email|
       %li
         %strong= email.email
-        - if email.email === @public_email
+        - if email.email === current_user.public_email
           %span.label.label-info Public Email
+        - if email.email === current_user.notification_email
+          %span.label.label-info Notification Email
         %span.cgray
           added #{time_ago_with_tooltip(email.created_at)}
         = link_to 'Remove', profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-remove pull-right'