Commit 82a39e3e authored by Douwe Maan's avatar Douwe Maan

Merge branch...

Merge branch '21010-emailsonpushworker-incorrectly-claims-deleted-a-commit-after-a-push' into 'master'

Resolve "EmailsOnPushWorker incorrectly claims deleted a commit after a push"

## What does this MR do?

Fix the comparison order in the emails on push worker, so regular pushes don't show a message about force pushing.

## Are there points in the code the reviewer needs to double check?

Don't think so, it was just a typo: `after_sha` should map to the `source_branch` argument, because it's the head of the comparison.

## Why was this MR needed?

To fix a regression!

## What are the relevant issue numbers?

Closes #21010.

## Does this MR meet the acceptance criteria?

- [x] ~~[CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added~~
- [x] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- [x] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5894
parents 5e497e66 d0fcd13e
...@@ -33,13 +33,13 @@ class EmailsOnPushWorker ...@@ -33,13 +33,13 @@ class EmailsOnPushWorker
reverse_compare = false reverse_compare = false
if action == :push if action == :push
compare = CompareService.new.execute(project, before_sha, project, after_sha) compare = CompareService.new.execute(project, after_sha, project, before_sha)
diff_refs = compare.diff_refs diff_refs = compare.diff_refs
return false if compare.same return false if compare.same
if compare.commits.empty? if compare.commits.empty?
compare = CompareService.new.execute(project, after_sha, project, before_sha) compare = CompareService.new.execute(project, before_sha, project, after_sha)
diff_refs = compare.diff_refs diff_refs = compare.diff_refs
reverse_compare = true reverse_compare = true
......
...@@ -2,19 +2,19 @@ require 'spec_helper' ...@@ -2,19 +2,19 @@ require 'spec_helper'
describe EmailsOnPushWorker do describe EmailsOnPushWorker do
include RepoHelpers include RepoHelpers
include EmailSpec::Matchers
let(:project) { create(:project) } let(:project) { create(:project) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:data) { Gitlab::DataBuilder::Push.build_sample(project, user) } let(:data) { Gitlab::DataBuilder::Push.build_sample(project, user) }
let(:recipients) { user.email } let(:recipients) { user.email }
let(:perform) { subject.perform(project.id, recipients, data.stringify_keys) } let(:perform) { subject.perform(project.id, recipients, data.stringify_keys) }
let(:email) { ActionMailer::Base.deliveries.last }
subject { EmailsOnPushWorker.new } subject { EmailsOnPushWorker.new }
describe "#perform" do describe "#perform" do
context "when push is a new branch" do context "when push is a new branch" do
let(:email) { ActionMailer::Base.deliveries.last }
before do before do
data_new_branch = data.stringify_keys.merge("before" => Gitlab::Git::BLANK_SHA) data_new_branch = data.stringify_keys.merge("before" => Gitlab::Git::BLANK_SHA)
...@@ -31,8 +31,6 @@ describe EmailsOnPushWorker do ...@@ -31,8 +31,6 @@ describe EmailsOnPushWorker do
end end
context "when push is a deleted branch" do context "when push is a deleted branch" do
let(:email) { ActionMailer::Base.deliveries.last }
before do before do
data_deleted_branch = data.stringify_keys.merge("after" => Gitlab::Git::BLANK_SHA) data_deleted_branch = data.stringify_keys.merge("after" => Gitlab::Git::BLANK_SHA)
...@@ -48,15 +46,40 @@ describe EmailsOnPushWorker do ...@@ -48,15 +46,40 @@ describe EmailsOnPushWorker do
end end
end end
context "when there are no errors in sending" do context "when push is a force push to delete commits" do
let(:email) { ActionMailer::Base.deliveries.last } before do
data_force_push = data.stringify_keys.merge(
"after" => data[:before],
"before" => data[:after]
)
subject.perform(project.id, recipients, data_force_push)
end
it "sends a mail with the correct subject" do
expect(email.subject).to include('Change some files')
end
it "mentions force pushing in the body" do
expect(email).to have_body_text("force push")
end
it "sends the mail to the correct recipient" do
expect(email.to).to eq([user.email])
end
end
context "when there are no errors in sending" do
before { perform } before { perform }
it "sends a mail with the correct subject" do it "sends a mail with the correct subject" do
expect(email.subject).to include('Change some files') expect(email.subject).to include('Change some files')
end end
it "does not mention force pushing in the body" do
expect(email).not_to have_body_text("force push")
end
it "sends the mail to the correct recipient" do it "sends the mail to the correct recipient" do
expect(email.to).to eq([user.email]) expect(email.to).to eq([user.email])
end end
...@@ -66,6 +89,7 @@ describe EmailsOnPushWorker do ...@@ -66,6 +89,7 @@ describe EmailsOnPushWorker do
before do before do
ActionMailer::Base.deliveries.clear ActionMailer::Base.deliveries.clear
allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError) allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError)
allow(subject).to receive_message_chain(:logger, :info)
perform perform
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