Commit fc0194b5 authored by Nick Thomas's avatar Nick Thomas Committed by Douwe Maan

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

parent 9de6efe6
...@@ -96,6 +96,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -96,6 +96,7 @@ class ProfilesController < Profiles::ApplicationController
:location, :location,
:name, :name,
:public_email, :public_email,
:commit_email,
:skype, :skype,
:twitter, :twitter,
:username, :username,
......
...@@ -161,6 +161,7 @@ class User < ActiveRecord::Base ...@@ -161,6 +161,7 @@ class User < ActiveRecord::Base
validates :notification_email, presence: true validates :notification_email, presence: true
validates :notification_email, email: true, if: ->(user) { user.notification_email != user.email } 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 :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 :bio, length: { maximum: 255 }, allow_blank: true
validates :projects_limit, validates :projects_limit,
presence: true, presence: true,
...@@ -173,12 +174,15 @@ class User < ActiveRecord::Base ...@@ -173,12 +174,15 @@ class User < ActiveRecord::Base
validate :unique_email, if: :email_changed? validate :unique_email, if: :email_changed?
validate :owns_notification_email, if: :notification_email_changed? validate :owns_notification_email, if: :notification_email_changed?
validate :owns_public_email, if: :public_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 } validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
before_validation :sanitize_attrs before_validation :sanitize_attrs
before_validation :set_notification_email, if: :new_record? before_validation :set_notification_email, if: :new_record?
before_validation :set_public_email, if: :public_email_changed? 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_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_incoming_email_token
before_save :ensure_user_rights_and_limits, if: ->(user) { user.new_record? || user.external_changed? } 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) } before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
...@@ -619,6 +623,32 @@ class User < ActiveRecord::Base ...@@ -619,6 +623,32 @@ class User < ActiveRecord::Base
errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email) errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
end 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 # see if the new email is already a verified secondary email
def check_for_verified_email def check_for_verified_email
skip_reconfirmation! if emails.confirmed.where(email: self.email).any? skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
...@@ -873,10 +903,17 @@ class User < ActiveRecord::Base ...@@ -873,10 +903,17 @@ class User < ActiveRecord::Base
end end
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! def update_secondary_emails!
set_notification_email set_notification_email
set_public_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 end
def set_projects_limit def set_projects_limit
......
...@@ -22,7 +22,9 @@ ...@@ -22,7 +22,9 @@
.account-well.append-bottom-default .account-well.append-bottom-default
%ul %ul
%li %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 %li
Your Notification Email will be used for account notifications. Your Notification Email will be used for account notifications.
%li %li
...@@ -34,6 +36,8 @@ ...@@ -34,6 +36,8 @@
= render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? } = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
%span.float-right %span.float-right
%span.badge.badge-success Primary email %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 - if @primary_email === current_user.public_email
%span.badge.badge-info Public email %span.badge.badge-info Public email
- if @primary_email === current_user.notification_email - if @primary_email === current_user.notification_email
...@@ -42,6 +46,8 @@ ...@@ -42,6 +46,8 @@
%li %li
= render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? } = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
%span.float-right %span.float-right
- if email.email === current_user.commit_email
%span.badge.badge-info Commit email
- if email.email === current_user.public_email - if email.email === current_user.public_email
%span.badge.badge-info Public email %span.badge.badge-info Public email
- if email.email === current_user.notification_email - if email.email === current_user.notification_email
......
...@@ -91,6 +91,9 @@ ...@@ -91,6 +91,9 @@
= f.select :public_email, options_for_select(@user.all_emails, selected: @user.public_email), = 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") }, { help: s_("Profiles|This email will be displayed on your public profile."), include_blank: s_("Profiles|Do not show on profile") },
control_class: 'select2' 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] }, = 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.") }, { help: s_("Profiles|This feature is experimental and translations are not complete yet.") },
control_class: 'select2' control_class: 'select2'
......
---
title: Allow user to choose the email used for commits made through GitLab's UI.
merge_request: 21213
author: Joshua Campbell
type: added
# 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
...@@ -2207,6 +2207,7 @@ ActiveRecord::Schema.define(version: 20180906101639) do ...@@ -2207,6 +2207,7 @@ ActiveRecord::Schema.define(version: 20180906101639) do
t.string "feed_token" t.string "feed_token"
t.boolean "private_profile" t.boolean "private_profile"
t.boolean "include_private_contributions" t.boolean "include_private_contributions"
t.string "commit_email"
end end
add_index "users", ["admin"], name: "index_users_on_admin", using: :btree add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
......
...@@ -37,6 +37,7 @@ From there, you can: ...@@ -37,6 +37,7 @@ From there, you can:
[use GitLab as an OAuth provider](../../integration/oauth_provider.md#introduction-to-oauth) [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 - 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 - 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 [SSH keys](../../ssh/README.md#ssh) to access your account via SSH
- Manage your [preferences](preferences.md#syntax-highlighting-theme) - Manage your [preferences](preferences.md#syntax-highlighting-theme)
to customize your own GitLab experience to customize your own GitLab experience
......
...@@ -177,5 +177,9 @@ you commit the changes you will be taken to a new merge request form. ...@@ -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) ![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 [ce-2808]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2808
[issue closing pattern]: ../issues/automatic_issue_closing.md [issue closing pattern]: ../issues/automatic_issue_closing.md
...@@ -4,7 +4,7 @@ module Gitlab ...@@ -4,7 +4,7 @@ module Gitlab
attr_reader :username, :name, :email, :gl_id attr_reader :username, :name, :email, :gl_id
def self.from_gitlab(gitlab_user) 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 end
def self.from_gitaly(gitaly_user) def self.from_gitaly(gitaly_user)
......
...@@ -4,5 +4,6 @@ FactoryBot.define do ...@@ -4,5 +4,6 @@ FactoryBot.define do
email { generate(:email_alias) } email { generate(:email_alias) }
trait(:confirmed) { confirmed_at Time.now } trait(:confirmed) { confirmed_at Time.now }
trait(:skip_validate) { to_create {|instance| instance.save(validate: false) } }
end end
end end
...@@ -22,12 +22,21 @@ describe Gitlab::Git::User do ...@@ -22,12 +22,21 @@ describe Gitlab::Git::User do
end end
describe '.from_gitlab' do describe '.from_gitlab' do
let(:user) { build(: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) } 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 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 describe '#==' do
def eq_other(username, name, email, gl_id) def eq_other(username, name, email, gl_id)
eq(described_class.new(username, name, email, gl_id)) eq(described_class.new(username, name, email, gl_id))
......
...@@ -167,6 +167,55 @@ describe User do ...@@ -167,6 +167,55 @@ describe User do
subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } } subject { build(:user).tap { |user| user.emails << build(:email, email: email_value) } }
end 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 describe 'email' do
context 'when no signup domains whitelisted' do context 'when no signup domains whitelisted' do
before do before do
...@@ -1390,7 +1439,6 @@ describe User do ...@@ -1390,7 +1439,6 @@ describe User do
it 'returns only confirmed emails' do it 'returns only confirmed emails' do
email_confirmed = create :email, user: user, confirmed_at: Time.now email_confirmed = create :email, user: user, confirmed_at: Time.now
create :email, user: user create :email, user: user
user.reload
expect(user.verified_emails).to match_array([user.email, email_confirmed.email]) expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment