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

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