Merge remote-tracking branch 'upstream/master' into 36089-handle-ref-failure-better

* upstream/master: (47 commits)
  Update CHANGELOG.md for 9.4.5
  Update charlock_holmes
  add a changelog entry
  switch to multi-line before block
  restructure the #new_key notification spec
  don't send devise notifications to the ghost user
  reset_delivered_emails before testing #new_key
  skip the :read_project check for new_project_member
  move the member spec to be with the other ones
  add a spec for new_group_member
  add a spec for never emailing the ghost user
  rubocop fix
  a membership with no user is always notifiable
  check notifiability for more emails
  add Member#notifiable?(type, opts)
  make NotificationRecipient a little more customizable
  Add notes about database performance for MySQL
  fix confidential border issue as well as confidential styles leaking on new MR
  Migrate force push check to Gitaly
  Add option to disable project export on instance
  ...

4 files changed, 354 insertions, 15 deletions

diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb
index 42adb044190..02d7ddeb86b 100644
--- a/spec/services/event_create_service_spec.rb
+++ b/spec/services/event_create_service_spec.rb
@@ -117,12 +117,52 @@ describe EventCreateService do
     let(:project) { create(:project) }
     let(:user) { create(:user) }
 
+    let(:push_data) do
+      {
+        commits: [
+          {
+            id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
+            message: 'This is a commit'
+          }
+        ],
+        before: '0000000000000000000000000000000000000000',
+        after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
+        total_commits_count: 1,
+        ref: 'refs/heads/my-branch'
+      }
+    end
+
     it 'creates a new event' do
-      expect { service.push(project, user, {}) }.to change { Event.count }
+      expect { service.push(project, user, push_data) }.to change { Event.count }
+    end
+
+    it 'creates the push event payload' do
+      expect(PushEventPayloadService).to receive(:new)
+        .with(an_instance_of(PushEvent), push_data)
+        .and_call_original
+
+      service.push(project, user, push_data)
     end
 
     it 'updates user last activity' do
-      expect { service.push(project, user, {}) }.to change { user_activity(user) }
+      expect { service.push(project, user, push_data) }
+        .to change { user_activity(user) }
+    end
+
+    it 'does not create any event data when an error is raised' do
+      payload_service = double(:service)
+
+      allow(payload_service).to receive(:execute)
+        .and_raise(RuntimeError)
+
+      allow(PushEventPayloadService).to receive(:new)
+        .and_return(payload_service)
+
+      expect { service.push(project, user, push_data) }
+        .to raise_error(RuntimeError)
+
+      expect(Event.count).to eq(0)
+      expect(PushEventPayload.count).to eq(0)
     end
   end
 
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index a6449a3c9f5..8485605b398 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -141,10 +141,13 @@ describe GitPushService, services: true do
     let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) }
     let(:event) { Event.find_by_action(Event::PUSHED) }
 
-    it { expect(event).not_to be_nil }
+    it { expect(event).to be_an_instance_of(PushEvent) }
     it { expect(event.project).to eq(project) }
     it { expect(event.action).to eq(Event::PUSHED) }
-    it { expect(event.data).to eq(push_data) }
+    it { expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) }
+    it { expect(event.push_event_payload.commit_from).to eq(oldrev) }
+    it { expect(event.push_event_payload.commit_to).to eq(newrev) }
+    it { expect(event.push_event_payload.ref).to eq('master') }
 
     context "Updates merge requests" do
       it "when pushing a new branch for the first time" do
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 64981c199e4..44b2d28d1d4 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -80,12 +80,16 @@ describe NotificationService, :mailer do
 
   describe 'Keys' do
     describe '#new_key' do
-      let!(:key) { create(:personal_key) }
+      let(:key_options) { {} }
+      let!(:key) { create(:personal_key, key_options) }
 
       it { expect(notification.new_key(key)).to be_truthy }
+      it { should_email(key.user) }
 
-      it 'sends email to key owner' do
-        expect { notification.new_key(key) }.to change { ActionMailer::Base.deliveries.size }.by(1)
+      describe 'never emails the ghost user' do
+        let(:key_options) { { user: User.ghost } }
+
+        it { should_not_email_anyone }
       end
     end
   end
@@ -1173,19 +1177,39 @@ describe NotificationService, :mailer do
       end
     end
 
-    describe '#project_exported' do
-      it do
-        notification.project_exported(project, @u_disabled)
+    context 'user with notifications disabled' do
+      describe '#project_exported' do
+        it do
+          notification.project_exported(project, @u_disabled)
+
+          should_not_email_anyone
+        end
+      end
+
+      describe '#project_not_exported' do
+        it do
+          notification.project_not_exported(project, @u_disabled, ['error'])
 
-        should_only_email(@u_disabled)
+          should_not_email_anyone
+        end
       end
     end
 
-    describe '#project_not_exported' do
-      it do
-        notification.project_not_exported(project, @u_disabled, ['error'])
+    context 'user with notifications enabled' do
+      describe '#project_exported' do
+        it do
+          notification.project_exported(project, @u_participating)
 
-        should_only_email(@u_disabled)
+          should_only_email(@u_participating)
+        end
+      end
+
+      describe '#project_not_exported' do
+        it do
+          notification.project_not_exported(project, @u_participating, ['error'])
+
+          should_only_email(@u_participating)
+        end
       end
     end
   end
@@ -1209,6 +1233,35 @@ describe NotificationService, :mailer do
         end.to change { ActionMailer::Base.deliveries.size }.by(1)
       end
     end
+
+    describe '#new_group_member' do
+      let(:group) { create(:group) }
+      let(:added_user) { create(:user) }
+
+      def create_member!
+        GroupMember.create(
+          group: group,
+          user: added_user,
+          access_level: Gitlab::Access::GUEST
+        )
+      end
+
+      it 'sends a notification' do
+        create_member!
+        should_only_email(added_user)
+      end
+
+      describe 'when notifications are disabled' do
+        before do
+          create_global_setting_for(added_user, :disabled)
+        end
+
+        it 'does not send a notification' do
+          create_member!
+          should_not_email_anyone
+        end
+      end
+    end
   end
 
   describe 'ProjectMember' do
@@ -1228,6 +1281,31 @@ describe NotificationService, :mailer do
         end.to change { ActionMailer::Base.deliveries.size }.by(1)
       end
     end
+
+    describe '#new_project_member' do
+      let(:project) { create(:project) }
+      let(:added_user) { create(:user) }
+
+      def create_member!
+        create(:project_member, user: added_user, project: project)
+      end
+
+      it do
+        create_member!
+        should_only_email(added_user)
+      end
+
+      describe 'when notifications are disabled' do
+        before do
+          create_global_setting_for(added_user, :disabled)
+        end
+
+        it do
+          create_member!
+          should_not_email_anyone
+        end
+      end
+    end
   end
 
   context 'guest user in private project' do
diff --git a/spec/services/push_event_payload_service_spec.rb b/spec/services/push_event_payload_service_spec.rb
new file mode 100644
index 00000000000..81956200bff
--- /dev/null
+++ b/spec/services/push_event_payload_service_spec.rb
@@ -0,0 +1,218 @@
+require 'spec_helper'
+
+describe PushEventPayloadService do
+  let(:event) { create(:push_event) }
+
+  describe '#execute' do
+    let(:push_data) do
+      {
+        commits: [
+          {
+            id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
+            message: 'This is a commit'
+          }
+        ],
+        before: '0000000000000000000000000000000000000000',
+        after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
+        total_commits_count: 1,
+        ref: 'refs/heads/my-branch'
+      }
+    end
+
+    it 'creates a new PushEventPayload row' do
+      payload = described_class.new(event, push_data).execute
+
+      expect(payload.commit_count).to eq(1)
+      expect(payload.action).to eq('created')
+      expect(payload.ref_type).to eq('branch')
+      expect(payload.commit_from).to be_nil
+      expect(payload.commit_to).to eq(push_data[:after])
+      expect(payload.ref).to eq('my-branch')
+      expect(payload.commit_title).to eq('This is a commit')
+      expect(payload.event_id).to eq(event.id)
+    end
+
+    it 'sets the push_event_payload association of the used event' do
+      payload = described_class.new(event, push_data).execute
+
+      expect(event.push_event_payload).to eq(payload)
+    end
+  end
+
+  describe '#commit_title' do
+    it 'returns nil if no commits were pushed' do
+      service = described_class.new(event, commits: [])
+
+      expect(service.commit_title).to be_nil
+    end
+
+    it 'returns a String limited to 70 characters' do
+      service = described_class.new(event, commits: [{ message: 'a' * 100 }])
+
+      expect(service.commit_title).to eq(('a' * 67) + '...')
+    end
+
+    it 'does not truncate the commit message if it is shorter than 70 characters' do
+      service = described_class.new(event, commits: [{ message: 'Hello' }])
+
+      expect(service.commit_title).to eq('Hello')
+    end
+
+    it 'includes the first line of a commit message if the message spans multiple lines' do
+      service = described_class
+        .new(event, commits: [{ message: "Hello\n\nworld" }])
+
+      expect(service.commit_title).to eq('Hello')
+    end
+  end
+
+  describe '#commit_from_id' do
+    it 'returns nil when creating a new ref' do
+      service = described_class.new(event, before: Gitlab::Git::BLANK_SHA)
+
+      expect(service.commit_from_id).to be_nil
+    end
+
+    it 'returns the ID of the first commit when pushing to an existing ref' do
+      service = described_class.new(event, before: '123')
+
+      expect(service.commit_from_id).to eq('123')
+    end
+  end
+
+  describe '#commit_to_id' do
+    it 'returns nil when removing an existing ref' do
+      service = described_class.new(event, after: Gitlab::Git::BLANK_SHA)
+
+      expect(service.commit_to_id).to be_nil
+    end
+  end
+
+  describe '#commit_count' do
+    it 'returns the number of commits' do
+      service = described_class.new(event, total_commits_count: 1)
+
+      expect(service.commit_count).to eq(1)
+    end
+
+    it 'raises when the push data does not contain the commits count' do
+      service = described_class.new(event, {})
+
+      expect { service.commit_count }.to raise_error(KeyError)
+    end
+  end
+
+  describe '#ref' do
+    it 'returns the name of the ref' do
+      service = described_class.new(event, ref: 'refs/heads/foo')
+
+      expect(service.ref).to eq('refs/heads/foo')
+    end
+
+    it 'raises when the push data does not contain the ref name' do
+      service = described_class.new(event, {})
+
+      expect { service.ref }.to raise_error(KeyError)
+    end
+  end
+
+  describe '#revision_before' do
+    it 'returns the revision from before the push' do
+      service = described_class.new(event, before: 'foo')
+
+      expect(service.revision_before).to eq('foo')
+    end
+
+    it 'raises when the push data does not contain the before revision' do
+      service = described_class.new(event, {})
+
+      expect { service.revision_before }.to raise_error(KeyError)
+    end
+  end
+
+  describe '#revision_after' do
+    it 'returns the revision from after the push' do
+      service = described_class.new(event, after: 'foo')
+
+      expect(service.revision_after).to eq('foo')
+    end
+
+    it 'raises when the push data does not contain the after revision' do
+      service = described_class.new(event, {})
+
+      expect { service.revision_after }.to raise_error(KeyError)
+    end
+  end
+
+  describe '#trimmed_ref' do
+    it 'returns the ref name without its prefix' do
+      service = described_class.new(event, ref: 'refs/heads/foo')
+
+      expect(service.trimmed_ref).to eq('foo')
+    end
+  end
+
+  describe '#create?' do
+    it 'returns true when creating a new ref' do
+      service = described_class.new(event, before: Gitlab::Git::BLANK_SHA)
+
+      expect(service.create?).to eq(true)
+    end
+
+    it 'returns false when pushing to an existing ref' do
+      service = described_class.new(event, before: 'foo')
+
+      expect(service.create?).to eq(false)
+    end
+  end
+
+  describe '#remove?' do
+    it 'returns true when removing an existing ref' do
+      service = described_class.new(event, after: Gitlab::Git::BLANK_SHA)
+
+      expect(service.remove?).to eq(true)
+    end
+
+    it 'returns false pushing to an existing ref' do
+      service = described_class.new(event, after: 'foo')
+
+      expect(service.remove?).to eq(false)
+    end
+  end
+
+  describe '#action' do
+    it 'returns :created when creating a ref' do
+      service = described_class.new(event, before: Gitlab::Git::BLANK_SHA)
+
+      expect(service.action).to eq(:created)
+    end
+
+    it 'returns :removed when removing an existing ref' do
+      service = described_class.new(event,
+                                    before: '123',
+                                    after: Gitlab::Git::BLANK_SHA)
+
+      expect(service.action).to eq(:removed)
+    end
+
+    it 'returns :pushed when pushing to an existing ref' do
+      service = described_class.new(event, before: '123', after: '456')
+
+      expect(service.action).to eq(:pushed)
+    end
+  end
+
+  describe '#ref_type' do
+    it 'returns :tag for a tag' do
+      service = described_class.new(event, ref: 'refs/tags/1.2')
+
+      expect(service.ref_type).to eq(:tag)
+    end
+
+    it 'returns :branch for a branch' do
+      service = described_class.new(event, ref: 'refs/heads/master')
+
+      expect(service.ref_type).to eq(:branch)
+    end
+  end
+end