diff options
author | Nick Thomas <nick@gitlab.com> | 2018-09-14 09:52:09 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2018-09-14 09:52:09 +0000 |
commit | fc0194b589238df6171ba3f4aeec79f8034b7128 (patch) | |
tree | cf61e7a7f0fa45b80de2a47f59d34bb60c1e4aab | |
parent | 9de6efe6d18b0c6881aa5074d6e805ac6202d5e5 (diff) | |
download | gitlab-ce-fc0194b589238df6171ba3f4aeec79f8034b7128.tar.gz |
Resolve "Add functionality to change what email address online actions commit using"
-rw-r--r-- | app/controllers/profiles_controller.rb | 1 | ||||
-rw-r--r-- | app/models/user.rb | 39 | ||||
-rw-r--r-- | app/views/profiles/emails/index.html.haml | 8 | ||||
-rw-r--r-- | app/views/profiles/show.html.haml | 3 | ||||
-rw-r--r-- | changelogs/unreleased/23986-choose-commit-email.yml | 5 | ||||
-rw-r--r-- | db/migrate/20180814153625_add_commit_email_to_users.rb | 33 | ||||
-rw-r--r-- | db/schema.rb | 1 | ||||
-rw-r--r-- | doc/user/profile/index.md | 1 | ||||
-rw-r--r-- | doc/user/project/repository/web_editor.md | 4 | ||||
-rw-r--r-- | lib/gitlab/git/user.rb | 2 | ||||
-rw-r--r-- | spec/factories/emails.rb | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/git/user_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 50 |
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 |