diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-08-19 18:16:55 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-08-19 18:16:55 +0000 |
commit | 82a39e3e698b6332c44f036b735ae98926767057 (patch) | |
tree | d2fc51bf61f8ff82aedf28bee29f5d9b16639a68 | |
parent | 5e497e6692ca23e32348b74580b1e42c01b17e1a (diff) | |
parent | d0fcd13ea34ab1557d8b071c7a1fb117046261cb (diff) | |
download | gitlab-ce-82a39e3e698b6332c44f036b735ae98926767057.tar.gz |
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
-rw-r--r-- | app/workers/emails_on_push_worker.rb | 4 | ||||
-rw-r--r-- | spec/workers/emails_on_push_worker_spec.rb | 36 |
2 files changed, 32 insertions, 8 deletions
diff --git a/app/workers/emails_on_push_worker.rb b/app/workers/emails_on_push_worker.rb index c6a5af2809a..1dc7e0adef7 100644 --- a/app/workers/emails_on_push_worker.rb +++ b/app/workers/emails_on_push_worker.rb @@ -33,13 +33,13 @@ class EmailsOnPushWorker reverse_compare = false 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 return false if compare.same 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 reverse_compare = true diff --git a/spec/workers/emails_on_push_worker_spec.rb b/spec/workers/emails_on_push_worker_spec.rb index eecc32875a5..7ca2c29da1c 100644 --- a/spec/workers/emails_on_push_worker_spec.rb +++ b/spec/workers/emails_on_push_worker_spec.rb @@ -2,19 +2,19 @@ require 'spec_helper' describe EmailsOnPushWorker do include RepoHelpers + include EmailSpec::Matchers let(:project) { create(:project) } let(:user) { create(:user) } let(:data) { Gitlab::DataBuilder::Push.build_sample(project, user) } let(:recipients) { user.email } let(:perform) { subject.perform(project.id, recipients, data.stringify_keys) } + let(:email) { ActionMailer::Base.deliveries.last } subject { EmailsOnPushWorker.new } describe "#perform" do context "when push is a new branch" do - let(:email) { ActionMailer::Base.deliveries.last } - before do data_new_branch = data.stringify_keys.merge("before" => Gitlab::Git::BLANK_SHA) @@ -31,8 +31,6 @@ describe EmailsOnPushWorker do end context "when push is a deleted branch" do - let(:email) { ActionMailer::Base.deliveries.last } - before do data_deleted_branch = data.stringify_keys.merge("after" => Gitlab::Git::BLANK_SHA) @@ -48,15 +46,40 @@ describe EmailsOnPushWorker do end end - context "when there are no errors in sending" do - let(:email) { ActionMailer::Base.deliveries.last } + context "when push is a force push to delete commits" do + 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 } it "sends a mail with the correct subject" do expect(email.subject).to include('Change some files') 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 expect(email.to).to eq([user.email]) end @@ -66,6 +89,7 @@ describe EmailsOnPushWorker do before do ActionMailer::Base.deliveries.clear allow(Notify).to receive(:repository_push_email).and_raise(Net::SMTPFatalError) + allow(subject).to receive_message_chain(:logger, :info) perform end |