summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNick Thomas <nick@gitlab.com>2018-09-14 09:52:09 +0000
committerDouwe Maan <douwe@gitlab.com>2018-09-14 09:52:09 +0000
commitfc0194b589238df6171ba3f4aeec79f8034b7128 (patch)
treecf61e7a7f0fa45b80de2a47f59d34bb60c1e4aab
parent9de6efe6d18b0c6881aa5074d6e805ac6202d5e5 (diff)
downloadgitlab-ce-fc0194b589238df6171ba3f4aeec79f8034b7128.tar.gz
Resolve "Add functionality to change what email address online actions commit using"
-rw-r--r--app/controllers/profiles_controller.rb1
-rw-r--r--app/models/user.rb39
-rw-r--r--app/views/profiles/emails/index.html.haml8
-rw-r--r--app/views/profiles/show.html.haml3
-rw-r--r--changelogs/unreleased/23986-choose-commit-email.yml5
-rw-r--r--db/migrate/20180814153625_add_commit_email_to_users.rb33
-rw-r--r--db/schema.rb1
-rw-r--r--doc/user/profile/index.md1
-rw-r--r--doc/user/project/repository/web_editor.md4
-rw-r--r--lib/gitlab/git/user.rb2
-rw-r--r--spec/factories/emails.rb1
-rw-r--r--spec/lib/gitlab/git/user_spec.rb15
-rw-r--r--spec/models/user_spec.rb50
13 files changed, 156 insertions, 7 deletions
diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 94e6efca487..a43d47797f1 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -96,6 +96,7 @@ class ProfilesController < Profiles::ApplicationController
:location,
:name,
:public_email,
+ :commit_email,
:skype,
:twitter,
:username,
diff --git a/app/models/user.rb b/app/models/user.rb
index dac8779488d..d68108a8e8e 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 04a19ab14dd..c8faf2b3af3 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/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
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index 0a1ee648d97..51f5ecf2166 100644
--- a/app/views/profiles/show.html.haml
+++ b/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'
diff --git a/changelogs/unreleased/23986-choose-commit-email.yml b/changelogs/unreleased/23986-choose-commit-email.yml
new file mode 100644
index 00000000000..1ebd62cd5b1
--- /dev/null
+++ b/changelogs/unreleased/23986-choose-commit-email.yml
@@ -0,0 +1,5 @@
+---
+title: Allow user to choose the email used for commits made through GitLab's UI.
+merge_request: 21213
+author: Joshua Campbell
+type: added
diff --git a/db/migrate/20180814153625_add_commit_email_to_users.rb b/db/migrate/20180814153625_add_commit_email_to_users.rb
new file mode 100644
index 00000000000..5c87d73688e
--- /dev/null
+++ b/db/migrate/20180814153625_add_commit_email_to_users.rb
@@ -0,0 +1,33 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index d888891c8ea..2528a0c1b72 100644
--- a/db/schema.rb
+++ b/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
diff --git a/doc/user/profile/index.md b/doc/user/profile/index.md
index 6b225147232..8604ea27f99 100644
--- a/doc/user/profile/index.md
+++ b/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
diff --git a/doc/user/project/repository/web_editor.md b/doc/user/project/repository/web_editor.md
index 33c9a1a4d6b..035028c9266 100644
--- a/doc/user/project/repository/web_editor.md
+++ b/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
diff --git a/lib/gitlab/git/user.rb b/lib/gitlab/git/user.rb
index e573cd0e143..338e1a30c45 100644
--- a/lib/gitlab/git/user.rb
+++ b/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)
diff --git a/spec/factories/emails.rb b/spec/factories/emails.rb
index 4dc7961060a..d23ddf9d79b 100644
--- a/spec/factories/emails.rb
+++ b/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
diff --git a/spec/lib/gitlab/git/user_spec.rb b/spec/lib/gitlab/git/user_spec.rb
index 99d850e1df9..d9d338206f8 100644
--- a/spec/lib/gitlab/git/user_spec.rb
+++ b/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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bee4a3d24a7..27aec49e348 100644
--- a/spec/models/user_spec.rb
+++ b/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