Merge branch '23986-choose-commit-email' into 'master'

Resolve "Add functionality to change what email address online actions commit using"

Closes #23986

See merge request gitlab-org/gitlab-ce!21598

app/controllers/profiles_controller.rb

@@ -96,6 +96,7 @@ class ProfilesController < Profiles::ApplicationController
       :location,
       :name,
       :public_email,
+      :commit_email,
       :skype,
       :twitter,
       :username,

app/models/user.rb

@@ -161,6 +161,7 @@ class User < ActiveRecord::Base
   validates :notification_email, presence: true
   validates :notification_email, email: true, if: ->(user) { user.notification_email != user.email }
   validates :public_email, presence: true, uniqueness: true, email: true, allow_blank: true
+  validates :commit_email, email: true, allow_nil: true, if: ->(user) { user.commit_email != user.email }
   validates :bio, length: { maximum: 255 }, allow_blank: true
   validates :projects_limit,
     presence: true,
@@ -173,12 +174,15 @@ class User < ActiveRecord::Base
   validate :unique_email, if: :email_changed?
   validate :owns_notification_email, if: :notification_email_changed?
   validate :owns_public_email, if: :public_email_changed?
+  validate :owns_commit_email, if: :commit_email_changed?
   validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
 
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :new_record?
   before_validation :set_public_email, if: :public_email_changed?
+  before_validation :set_commit_email, if: :commit_email_changed?
   before_save :set_public_email, if: :public_email_changed? # in case validation is skipped
+  before_save :set_commit_email, if: :commit_email_changed? # in case validation is skipped
   before_save :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? }
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
@@ -619,6 +623,32 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  def owns_commit_email
+    return if read_attribute(:commit_email).blank?
+
+    errors.add(:commit_email, "is not an email you own") unless verified_emails.include?(commit_email)
+  end
+
+  # Define commit_email-related attribute methods explicitly instead of relying
+  # on ActiveRecord to provide them. Some of the specs use the current state of
+  # the model code but an older database schema, so we need to guard against the
+  # possibility of the commit_email column not existing.
+
+  def commit_email
+    return unless has_attribute?(:commit_email)
+
+    # The commit email is the same as the primary email if undefined
+    super.presence || self.email
+  end
+
+  def commit_email=(email)
+    super if has_attribute?(:commit_email)
+  end
+
+  def commit_email_changed?
+    has_attribute?(:commit_email) && super
+  end
+
   # see if the new email is already a verified secondary email
   def check_for_verified_email
     skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
@@ -873,10 +903,17 @@ class User < ActiveRecord::Base
     end
   end
 
+  def set_commit_email
+    if commit_email.blank? || verified_emails.exclude?(commit_email)
+      self.commit_email = nil
+    end
+  end
+
   def update_secondary_emails!
     set_notification_email
     set_public_email
-    save if notification_email_changed? || public_email_changed?
+    set_commit_email
+    save if notification_email_changed? || public_email_changed? || commit_email_changed?
   end
 
   def set_projects_limit

app/views/profiles/emails/index.html.haml

@@ -22,7 +22,9 @@
     .account-well.append-bottom-default
       %ul
         %li
-          Your Primary Email will be used for avatar detection and web based operations, such as edits and merges.
+          Your Primary Email will be used for avatar detection.
+        %li
+          Your Commit Email will be used for web based operations, such as edits and merges.
         %li
           Your Notification Email will be used for account notifications.
         %li
@@ -34,6 +36,8 @@
         = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.float-right
           %span.badge.badge-success Primary email
+          - if @primary_email === current_user.commit_email
+            %span.badge.badge-info Commit email
           - if @primary_email === current_user.public_email
             %span.badge.badge-info Public email
           - if @primary_email === current_user.notification_email
@@ -42,6 +46,8 @@
         %li
           = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
           %span.float-right
+            - if email.email === current_user.commit_email
+              %span.badge.badge-info Commit email
             - if email.email === current_user.public_email
               %span.badge.badge-info Public email
             - if email.email === current_user.notification_email

app/views/profiles/show.html.haml

@@ -91,6 +91,9 @@
       = f.select :public_email, options_for_select(@user.all_emails, selected: @user.public_email),
         { help: s_("Profiles|This email will be displayed on your public profile."), include_blank: s_("Profiles|Do not show on profile") },
         control_class: 'select2'
+      = f.select :commit_email, options_for_select(@user.verified_emails, selected: @user.commit_email),
+        { help: 'This email will be used for web based operations, such as edits and merges.' },
+        control_class: 'select2'
       = f.select :preferred_language, Gitlab::I18n::AVAILABLE_LANGUAGES.map { |value, label| [label, value] },
         { help: s_("Profiles|This feature is experimental and translations are not complete yet.") },
         control_class: 'select2'

changelogs/unreleased/23986-choose-commit-email.yml

+---
+title: Allow user to choose the email used for commits made through GitLab's UI.
+merge_request: 21213
+author: Joshua Campbell
+type: added

db/migrate/20180814153625_add_commit_email_to_users.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddCommitEmailToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :users, :commit_email, :string
+  end
+end

db/schema.rb

@@ -2207,6 +2207,7 @@ ActiveRecord::Schema.define(version: 20180906101639) do
     t.string "feed_token"
     t.boolean "private_profile"
     t.boolean "include_private_contributions"
+    t.string "commit_email"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

doc/user/profile/index.md

@@ -37,6 +37,7 @@ From there, you can:
 [use GitLab as an OAuth provider](../../integration/oauth_provider.md#introduction-to-oauth)
 - Manage [personal access tokens](personal_access_tokens.md) to access your account via API and authorized applications
 - Add and delete emails linked to your account
+- Choose which email to use for notifications, web-based commits, and display on your public profile
 - Manage [SSH keys](../../ssh/README.md#ssh) to access your account via SSH
 - Manage your [preferences](preferences.md#syntax-highlighting-theme)
 to customize your own GitLab experience

doc/user/project/repository/web_editor.md

@@ -177,5 +177,9 @@ you commit the changes you will be taken to a new merge request form.
 
 ![Start a new merge request with these changes](img/web_editor_start_new_merge_request.png)
 
+If you'd prefer _not_ to use your primary email address for commits created
+through the web editor, you can choose to use another of your linked email
+addresses from the **User Settings > Edit Profile** page.
+
 [ce-2808]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2808
 [issue closing pattern]: ../issues/automatic_issue_closing.md

lib/gitlab/git/user.rb

@@ -4,7 +4,7 @@ module Gitlab
       attr_reader :username, :name, :email, :gl_id
 
       def self.from_gitlab(gitlab_user)
-        new(gitlab_user.username, gitlab_user.name, gitlab_user.email, Gitlab::GlId.gl_id(gitlab_user))
+        new(gitlab_user.username, gitlab_user.name, gitlab_user.commit_email, Gitlab::GlId.gl_id(gitlab_user))
       end
 
       def self.from_gitaly(gitaly_user)

spec/factories/emails.rb

@@ -4,5 +4,6 @@ FactoryBot.define do
     email { generate(:email_alias) }
 
     trait(:confirmed) { confirmed_at Time.now }
+    trait(:skip_validate) { to_create {|instance| instance.save(validate: false) } }
   end
 end

spec/lib/gitlab/git/user_spec.rb

@@ -22,10 +22,19 @@ describe Gitlab::Git::User do
   end
 
   describe '.from_gitlab' do
-    let(:user) { build(:user) }
-    subject { described_class.from_gitlab(user) }
+    context 'when no commit_email has been set' do
+      let(:user) { build(:user, email: 'alice@example.com', commit_email: nil) }
+      subject { described_class.from_gitlab(user) }
 
-    it { expect(subject).to eq(described_class.new(user.username, user.name, user.email, 'user-')) }
+      it { expect(subject).to eq(described_class.new(user.username, user.name, user.email, 'user-')) }
+    end
+
+    context 'when commit_email has been set' do
+      let(:user) { build(:user, email: 'alice@example.com', commit_email: 'bob@example.com') }
+      subject { described_class.from_gitlab(user) }
+
+      it { expect(subject).to eq(described_class.new(user.username, user.name, user.commit_email, 'user-')) }
+    end
   end
 
   describe '#==' do

spec/models/user_spec.rb

@@ -167,6 +167,55 @@ describe User do
       subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
     end
 
+    describe '#commit_email' do
+      subject(:user) { create(:user) }
+
+      it 'defaults to the primary email' do
+        expect(user.email).to be_present
+        expect(user.commit_email).to eq(user.email)
+      end
+
+      it 'defaults to the primary email when the column in the database is null' do
+        user.update_column(:commit_email, nil)
+
+        found_user = described_class.find_by(id: user.id)
+
+        expect(found_user.commit_email).to eq(user.email)
+      end
+
+      it 'can be set to a confirmed email' do
+        confirmed = create(:email, :confirmed, user: user)
+        user.commit_email = confirmed.email
+
+        expect(user).to be_valid
+        expect(user.commit_email).to eq(confirmed.email)
+      end
+
+      it 'can not be set to an unconfirmed email' do
+        unconfirmed = create(:email, user: user)
+        user.commit_email = unconfirmed.email
+
+        # This should set the commit_email attribute to the primary email
+        expect(user).to be_valid
+        expect(user.commit_email).to eq(user.email)
+      end
+
+      it 'can not be set to a non-existent email' do
+        user.commit_email = 'non-existent-email@nonexistent.nonexistent'
+
+        # This should set the commit_email attribute to the primary email
+        expect(user).to be_valid
+        expect(user.commit_email).to eq(user.email)
+      end
+
+      it 'can not be set to an invalid email, even if confirmed' do
+        confirmed = create(:email, :confirmed, :skip_validate, user: user, email: 'invalid')
+        user.commit_email = confirmed.email
+
+        expect(user).not_to be_valid
+      end
+    end
+
     describe 'email' do
       context 'when no signup domains whitelisted' do
         before do
@@ -1390,7 +1439,6 @@ describe User do
     it 'returns only confirmed emails' do
       email_confirmed = create :email, user: user, confirmed_at: Time.now
       create :email, user: user
-      user.reload
 
       expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
     end