From 3c5c658a27d1dfe4abf6469f35776b78f2169d81 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 17 Sep 2018 14:14:58 +0100 Subject: [PATCH 1/5] Use the correct email address when committing via a file service --- app/services/files/base_service.rb | 6 ++++-- spec/factories/users.rb | 8 ++++++++ spec/services/files/create_service_spec.rb | 15 ++++++++++++++- spec/services/files/delete_service_spec.rb | 8 ++++++++ spec/services/files/update_service_spec.rb | 8 ++++++++ 5 files changed, 42 insertions(+), 3 deletions(-) diff --git a/app/services/files/base_service.rb b/app/services/files/base_service.rb index fc7b236f7da..39e614d6569 100644 --- a/app/services/files/base_service.rb +++ b/app/services/files/base_service.rb @@ -7,8 +7,10 @@ module Files def initialize(*args) super - @author_email = params[:author_email] || current_user&.email - @author_name = params[:author_name] || current_user&.name + git_user = Gitlab::Git::User.from_gitlab(current_user) if current_user.present? + + @author_email = params[:author_email] || git_user&.email + @author_name = params[:author_name] || git_user&.name @commit_message = params[:commit_message] @last_commit_sha = params[:last_commit_sha] diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 59db8cdc34b..a47bd7cafca 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -58,6 +58,14 @@ FactoryBot.define do project_view :readme end + trait :commit_email do + after(:create) do |user, evaluator| + additional = create(:email, :confirmed, user: user, email: "commit-#{user.email}") + + user.update!(commit_email: additional.email) + end + end + factory :omniauth_user do transient do extern_uid '123456' diff --git a/spec/services/files/create_service_spec.rb b/spec/services/files/create_service_spec.rb index 30d94e4318d..c8f899e6dff 100644 --- a/spec/services/files/create_service_spec.rb +++ b/spec/services/files/create_service_spec.rb @@ -3,7 +3,7 @@ require "spec_helper" describe Files::CreateService do let(:project) { create(:project, :repository) } let(:repository) { project.repository } - let(:user) { create(:user) } + let(:user) { create(:user, :commit_email) } let(:file_content) { 'Test file content' } let(:branch_name) { project.default_branch } let(:start_branch) { branch_name } @@ -20,6 +20,8 @@ describe Files::CreateService do } end + let(:commit) { repository.head_commit } + subject { described_class.new(project, user, commit_params) } before do @@ -75,4 +77,15 @@ describe Files::CreateService do end end end + + context 'commit attribute' do + let(:file_path) { 'test-commit-attributes.txt' } + + it 'uses the commit email' do + subject.execute + + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end + end end diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb index 73566afe8c8..ba4a0594666 100644 --- a/spec/services/files/delete_service_spec.rb +++ b/spec/services/files/delete_service_spec.rb @@ -8,6 +8,7 @@ describe Files::DeleteService do let(:file_path) { 'files/ruby/popen.rb' } let(:branch_name) { project.default_branch } let(:last_commit_sha) { nil } + let(:commit) { project.repository.head_commit } let(:commit_params) do { @@ -34,6 +35,13 @@ describe Files::DeleteService do expect(blob).to be_nil end + + it 'uses the commit email' do + subject.execute + + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end end before do diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index e01fe487ffa..3bfa2717960 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -9,6 +9,7 @@ describe Files::UpdateService do let(:new_contents) { 'New Content' } let(:branch_name) { project.default_branch } let(:last_commit_sha) { nil } + let(:commit) { project.repository.commit } let(:commit_params) do { @@ -54,6 +55,13 @@ describe Files::UpdateService do expect(results.data).to eq(new_contents) end + + it 'uses the commit email' do + subject.execute + + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end end context "when the last_commit_sha is not supplied" do From e35fc8b9299e7bd257e00601f206e2670c559651 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 17 Sep 2018 14:36:22 +0100 Subject: [PATCH 2/5] Use commit email for wiki actions --- app/models/project_wiki.rb | 2 +- spec/models/project_wiki_spec.rb | 26 +++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index f4b3421f04b..d3c6312baf2 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -188,7 +188,7 @@ class ProjectWiki Gitlab::Git::Wiki::CommitDetails.new(@user.id, @user.username, @user.name, - @user.email, + @user.commit_email, commit_message) end diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index 528f5b610d7..f607bb7a917 100644 --- a/spec/models/project_wiki_spec.rb +++ b/spec/models/project_wiki_spec.rb @@ -2,12 +2,13 @@ require "spec_helper" describe ProjectWiki do - let(:project) { create(:project, :wiki_repo) } + let(:user) { create(:user, :commit_email) } + let(:project) { create(:project, :wiki_repo, namespace: user.namespace) } let(:repository) { project.repository } - let(:user) { project.owner } let(:gitlab_shell) { Gitlab::Shell.new } let(:project_wiki) { described_class.new(project, user) } let(:raw_repository) { Gitlab::Git::Repository.new(project.repository_storage, subject.disk_path + '.git', 'foo') } + let(:commit) { project_wiki.repository.head_commit } subject { project_wiki } @@ -276,6 +277,13 @@ describe ProjectWiki do expect(subject.pages.first.page.version.message).to eq("commit message") end + it 'sets the correct commit email' do + subject.create_page('test page', 'content') + + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end + it 'updates project activity' do subject.create_page('Test Page', 'This is content') @@ -320,6 +328,11 @@ describe ProjectWiki do expect(@page.version.message).to eq("updated page") end + it 'sets the correct commit email' do + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end + it 'updates project activity' do subject.update_page( @gitlab_git_wiki_page, @@ -347,6 +360,13 @@ describe ProjectWiki do expect(subject.pages.count).to eq(0) end + it 'sets the correct commit email' do + subject.delete_page(@page) + + expect(commit.author_email).to eq(user.commit_email) + expect(commit.committer_email).to eq(user.commit_email) + end + it 'updates project activity' do subject.delete_page(@page) @@ -420,7 +440,7 @@ describe ProjectWiki do end def commit_details - Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.email, "test commit") + Gitlab::Git::Wiki::CommitDetails.new(user.id, user.username, user.name, user.commit_email, "test commit") end def create_page(name, content) From f54cada7b7e574f0dcd4997ee4d9256f6e5d8529 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 17 Sep 2018 14:44:24 +0100 Subject: [PATCH 3/5] Remove Gitlab::Git::Repository#user_to_committer --- lib/gitlab/git/repository.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 1b8d320ff3b..61786f1d896 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -581,10 +581,6 @@ module Gitlab end end - def user_to_committer(user) - Gitlab::Git.committer_hash(email: user.email, name: user.name) - end - # Delete the specified branch from the repository def delete_branch(branch_name) wrapped_gitaly_errors do From 12f9879295ac9abdd0d6aff47cc3828c9ca36dc7 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Mon, 17 Sep 2018 15:38:10 +0100 Subject: [PATCH 4/5] Add changelog --- changelogs/unreleased/51564-fix-commit-email-usage.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/51564-fix-commit-email-usage.yml diff --git a/changelogs/unreleased/51564-fix-commit-email-usage.yml b/changelogs/unreleased/51564-fix-commit-email-usage.yml new file mode 100644 index 00000000000..2f1b042ae8a --- /dev/null +++ b/changelogs/unreleased/51564-fix-commit-email-usage.yml @@ -0,0 +1,5 @@ +--- +title: Respect the user commit email in more places +merge_request: 21773 +author: +type: fixed From 5a883915793f0f89f850d5393dce497ff9c5f893 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 18 Sep 2018 10:04:15 +0100 Subject: [PATCH 5/5] Guard against regressions in commit email specs --- app/models/project_wiki.rb | 7 ++++--- spec/models/project_wiki_spec.rb | 3 +++ spec/services/files/create_service_spec.rb | 1 + spec/services/files/delete_service_spec.rb | 3 ++- spec/services/files/update_service_spec.rb | 3 ++- 5 files changed, 12 insertions(+), 5 deletions(-) diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index d3c6312baf2..761359b3c9f 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -184,11 +184,12 @@ class ProjectWiki def commit_details(action, message = nil, title = nil) commit_message = message || default_message(action, title) + git_user = Gitlab::Git::User.from_gitlab(@user) Gitlab::Git::Wiki::CommitDetails.new(@user.id, - @user.username, - @user.name, - @user.commit_email, + git_user.username, + git_user.name, + git_user.email, commit_message) end diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index f607bb7a917..f38fc191943 100644 --- a/spec/models/project_wiki_spec.rb +++ b/spec/models/project_wiki_spec.rb @@ -280,6 +280,7 @@ describe ProjectWiki do it 'sets the correct commit email' do subject.create_page('test page', 'content') + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end @@ -329,6 +330,7 @@ describe ProjectWiki do end it 'sets the correct commit email' do + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end @@ -363,6 +365,7 @@ describe ProjectWiki do it 'sets the correct commit email' do subject.delete_page(@page) + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end diff --git a/spec/services/files/create_service_spec.rb b/spec/services/files/create_service_spec.rb index c8f899e6dff..751b7160276 100644 --- a/spec/services/files/create_service_spec.rb +++ b/spec/services/files/create_service_spec.rb @@ -84,6 +84,7 @@ describe Files::CreateService do it 'uses the commit email' do subject.execute + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end diff --git a/spec/services/files/delete_service_spec.rb b/spec/services/files/delete_service_spec.rb index ba4a0594666..309802ce733 100644 --- a/spec/services/files/delete_service_spec.rb +++ b/spec/services/files/delete_service_spec.rb @@ -4,7 +4,7 @@ describe Files::DeleteService do subject { described_class.new(project, user, commit_params) } let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + let(:user) { create(:user, :commit_email) } let(:file_path) { 'files/ruby/popen.rb' } let(:branch_name) { project.default_branch } let(:last_commit_sha) { nil } @@ -39,6 +39,7 @@ describe Files::DeleteService do it 'uses the commit email' do subject.execute + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end diff --git a/spec/services/files/update_service_spec.rb b/spec/services/files/update_service_spec.rb index 3bfa2717960..23db35c2418 100644 --- a/spec/services/files/update_service_spec.rb +++ b/spec/services/files/update_service_spec.rb @@ -4,7 +4,7 @@ describe Files::UpdateService do subject { described_class.new(project, user, commit_params) } let(:project) { create(:project, :repository) } - let(:user) { create(:user) } + let(:user) { create(:user, :commit_email) } let(:file_path) { 'files/ruby/popen.rb' } let(:new_contents) { 'New Content' } let(:branch_name) { project.default_branch } @@ -59,6 +59,7 @@ describe Files::UpdateService do it 'uses the commit email' do subject.execute + expect(user.commit_email).not_to eq(user.email) expect(commit.author_email).to eq(user.commit_email) expect(commit.committer_email).to eq(user.commit_email) end