Commit bb215549 authored by Patrick Bajao's avatar Patrick Bajao

Create single or bulk push events

Use the `push_event_activities_limit` setting to determine this.

If it exceeds the limit, individual push events won't be created
and bulk push events will be created instead.
parent ba92f9f7
......@@ -26,6 +26,8 @@ class PushEvent < Event
delegate :commit_count, to: :push_event_payload
alias_method :commits_count, :commit_count
delegate :ref_count, to: :push_event_payload
# Returns events of pushes that either pushed to an existing ref or created a
# new one.
def self.created_or_pushed
......
# frozen_string_literal: true
class BulkPushEventPayloadService
def initialize(event, push_data)
@event = event
@push_data = push_data
end
def execute
@event.build_push_event_payload(
action: @push_data[:action],
commit_count: 0,
ref_count: @push_data[:ref_count],
ref_type: @push_data[:ref_type]
)
@event.push_event_payload.tap(&:save!)
end
end
......@@ -73,15 +73,27 @@ class EventCreateService
end
def push(project, current_user, push_data)
create_push_event(PushEventPayloadService, project, current_user, push_data)
end
def bulk_push(project, current_user, push_data)
create_push_event(BulkPushEventPayloadService, project, current_user, push_data)
end
private
def create_record_event(record, current_user, status)
create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
end
def create_push_event(service_class, project, current_user, push_data)
# We're using an explicit transaction here so that any errors that may occur
# when creating push payload data will result in the event creation being
# rolled back as well.
event = Event.transaction do
new_event = create_event(project, current_user, Event::PUSHED)
PushEventPayloadService
.new(new_event, push_data)
.execute
service_class.new(new_event, push_data).execute
new_event
end
......@@ -92,12 +104,6 @@ class EventCreateService
Users::ActivityService.new(current_user, 'push').execute
end
private
def create_record_event(record, current_user, status)
create_event(record.resource_parent, current_user, status, target_id: record.id, target_type: record.class.name)
end
def create_event(resource_parent, current_user, status, attributes = {})
attributes.reverse_merge!(
action: status,
......
......@@ -48,6 +48,8 @@ module Git
# Push events in the activity feed only show information for the
# last commit.
def create_events
return unless params.fetch(:create_push_event, true)
EventCreateService.new.push(project, current_user, event_push_data)
end
......
......@@ -16,8 +16,8 @@ module Git
def process_changes_by_action(ref_type, changes)
changes_by_action = group_changes_by_action(changes)
changes_by_action.each do |_, changes|
process_changes(ref_type, changes, execute_project_hooks: execute_project_hooks?(changes)) if changes.any?
changes_by_action.each do |action, changes|
process_changes(ref_type, action, changes, execute_project_hooks: execute_project_hooks?(changes)) if changes.any?
end
end
......@@ -38,9 +38,11 @@ module Git
(changes.size <= Gitlab::CurrentSettings.push_event_hooks_limit) || Feature.enabled?(:git_push_execute_all_project_hooks, project)
end
def process_changes(ref_type, changes, execute_project_hooks:)
def process_changes(ref_type, action, changes, execute_project_hooks:)
push_service_class = push_service_class_for(ref_type)
create_bulk_push_event = changes.size > Gitlab::CurrentSettings.push_event_activities_limit
changes.each do |change|
push_service_class.new(
project,
......@@ -48,9 +50,20 @@ module Git
change: change,
push_options: params[:push_options],
create_pipelines: change[:index] < PIPELINE_PROCESS_LIMIT || Feature.enabled?(:git_push_create_all_pipelines, project),
execute_project_hooks: execute_project_hooks
execute_project_hooks: execute_project_hooks,
create_push_event: !create_bulk_push_event
).execute
end
create_bulk_push_event(ref_type, action, changes) if create_bulk_push_event
end
def create_bulk_push_event(ref_type, action, changes)
EventCreateService.new.bulk_push(
project,
current_user,
Gitlab::DataBuilder::Push.build_bulk(action: action, ref_type: ref_type, changes: changes)
)
end
def push_service_class_for(ref_type)
......
......@@ -6,11 +6,13 @@
.event-title.d-flex.flex-wrap
= inline_event_icon(event)
%span.event-type.d-inline-block.append-right-4.pushed #{event.action_name} #{event.ref_type}
%span.append-right-4
- commits_link = project_commits_path(project, event.ref_name)
- should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name)
= link_to_if should_link, event.ref_name, commits_link, class: 'ref-name'
- many_refs = event.ref_count.to_i > 1
%span.event-type.d-inline-block.append-right-4.pushed= many_refs ? "#{event.action_name} #{event.ref_count} #{event.ref_type.pluralize}" : "#{event.action_name} #{event.ref_type}"
- unless many_refs
%span.append-right-4
- commits_link = project_commits_path(project, event.ref_name)
- should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name)
= link_to_if should_link, event.ref_name, commits_link, class: 'ref-name'
= render "events/event_scope", event: event
......
# frozen_string_literal: true
class AddRefCountToPushEventPayloads < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :push_event_payloads, :ref_count, :integer
end
end
......@@ -3159,6 +3159,7 @@ ActiveRecord::Schema.define(version: 2019_10_16_072826) do
t.binary "commit_to"
t.text "ref"
t.string "commit_title", limit: 70
t.integer "ref_count"
t.index ["event_id"], name: "index_push_event_payloads_on_event_id", unique: true
end
......
......@@ -933,8 +933,8 @@ module API
end
class PushEventPayload < Grape::Entity
expose :commit_count, :action, :ref_type, :commit_from, :commit_to
expose :ref, :commit_title
expose :commit_count, :action, :ref_type, :commit_from, :commit_to, :ref,
:commit_title, :ref_count
end
class Event < Grape::Entity
......
......@@ -107,6 +107,14 @@ module Gitlab
}
end
def build_bulk(action:, ref_type:, changes:)
{
action: action,
ref_count: changes.count,
ref_type: ref_type
}
end
# This method provides a sample data generated with
# existing project and commits to test webhooks
def build_sample(project, user)
......
......@@ -90,4 +90,12 @@ describe Gitlab::DataBuilder::Push do
.not_to raise_error
end
end
describe '.build_bulk' do
subject do
described_class.build_bulk(action: :created, ref_type: :branch, changes: [double, double])
end
it { is_expected.to eq(action: :created, ref_count: 2, ref_type: :branch) }
end
end
......@@ -47,6 +47,7 @@ PushEventPayload:
- commit_to
- ref
- commit_title
- ref_count
Note:
- id
- note
......
......@@ -122,6 +122,7 @@ describe API::Events do
expect(payload_hash['action']).to eq(payload.action)
expect(payload_hash['ref_type']).to eq(payload.ref_type)
expect(payload_hash['commit_to']).to eq(payload.commit_to)
expect(payload_hash['ref_count']).to eq(payload.ref_count)
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe BulkPushEventPayloadService do
let(:event) { create(:push_event) }
let(:push_data) do
{
action: :created,
ref_count: 4,
ref_type: :branch
}
end
subject { described_class.new(event, push_data) }
it 'creates a PushEventPayload' do
push_event_payload = subject.execute
expect(push_event_payload).to be_persisted
expect(push_event_payload.action).to eq(push_data[:action].to_s)
expect(push_event_payload.commit_count).to eq(0)
expect(push_event_payload.ref_count).to eq(push_data[:ref_count])
expect(push_event_payload.ref_type).to eq(push_data[:ref_type].to_s)
end
end
......@@ -113,40 +113,21 @@ describe EventCreateService do
end
end
describe '#push', :clean_gitlab_redis_shared_state 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
shared_examples_for 'service for creating a push event' do |service_class|
it 'creates a new event' do
expect { service.push(project, user, push_data) }.to change { Event.count }
expect { subject }.to change { Event.count }
end
it 'creates the push event payload' do
expect(PushEventPayloadService).to receive(:new)
expect(service_class).to receive(:new)
.with(an_instance_of(PushEvent), push_data)
.and_call_original
service.push(project, user, push_data)
subject
end
it 'updates user last activity' do
expect { service.push(project, user, push_data) }
.to change { user.last_activity_on }.to(Date.today)
expect { subject }.to change { user.last_activity_on }.to(Date.today)
end
it 'caches the last push event for the user' do
......@@ -154,7 +135,7 @@ describe EventCreateService do
.to receive(:cache_last_push_event)
.with(an_instance_of(PushEvent))
service.push(project, user, push_data)
subject
end
it 'does not create any event data when an error is raised' do
......@@ -163,17 +144,56 @@ describe EventCreateService do
allow(payload_service).to receive(:execute)
.and_raise(RuntimeError)
allow(PushEventPayloadService).to receive(:new)
allow(service_class).to receive(:new)
.and_return(payload_service)
expect { service.push(project, user, push_data) }
.to raise_error(RuntimeError)
expect { subject }.to raise_error(RuntimeError)
expect(Event.count).to eq(0)
expect(PushEventPayload.count).to eq(0)
end
end
describe '#push', :clean_gitlab_redis_shared_state 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
subject { service.push(project, user, push_data) }
it_behaves_like 'service for creating a push event', PushEventPayloadService
end
describe '#bulk_push', :clean_gitlab_redis_shared_state do
let(:project) { create(:project) }
let(:user) { create(:user) }
let(:push_data) do
{
action: :created,
ref_count: 4,
ref_type: :branch
}
end
subject { service.bulk_push(project, user, push_data) }
it_behaves_like 'service for creating a push event', BulkPushEventPayloadService
end
describe 'Project' do
let(:user) { create :user }
let(:project) { create(:project) }
......
......@@ -12,8 +12,8 @@ describe Git::BaseHooksService do
let(:newrev) { "8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b" } # gitlab-test: git rev-parse refs/tags/v1.1.0
let(:ref) { 'refs/tags/v1.1.0' }
describe '#execute_project_hooks' do
class TestService < described_class
let(:test_service) do
Class.new(described_class) do
def hook_name
:push_hooks
end
......@@ -22,22 +22,44 @@ describe Git::BaseHooksService do
[]
end
end
end
let(:project) { create(:project, :repository) }
subject { test_service.new(project, user, params) }
let(:params) do
{
change: {
oldrev: oldrev,
newrev: newrev,
ref: ref
}
let(:params) do
{
change: {
oldrev: oldrev,
newrev: newrev,
ref: ref
}
}
end
describe 'push event' do
it 'creates push event' do
expect_next_instance_of(EventCreateService) do |service|
expect(service).to receive(:push)
end
subject.execute
end
subject { TestService.new(project, user, params) }
context 'create_push_event is set to false' do
before do
params[:create_push_event] = false
end
it 'does not create push event' do
expect(EventCreateService).not_to receive(:new)
subject.execute
end
end
end
context '#execute_hooks' do
describe 'project hooks and services' do
context 'hooks' do
before do
expect(project).to receive(:has_active_hooks?).and_return(active)
end
......@@ -65,7 +87,7 @@ describe Git::BaseHooksService do
end
end
context '#execute_services' do
context 'services' do
before do
expect(project).to receive(:has_active_services?).and_return(active)
end
......
......@@ -13,6 +13,12 @@ describe Git::ProcessRefChangesService do
let(:service) { double(execute: true) }
let(:git_changes) { double(branch_changes: [], tag_changes: []) }
def multiple_changes(change, count)
Array.new(count).map.with_index do |n, index|
{ index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" }
end
end
let(:changes) do
[
{ index: 0, oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" },
......@@ -28,7 +34,7 @@ describe Git::ProcessRefChangesService do
it "calls #{push_service_class}" do
expect(push_service_class)
.to receive(:new)
.with(project, project.owner, hash_including(execute_project_hooks: true))
.with(project, project.owner, hash_including(execute_project_hooks: true, create_push_event: true))
.exactly(changes.count).times
.and_return(service)
......@@ -36,12 +42,6 @@ describe Git::ProcessRefChangesService do
end
context 'changes exceed push_event_hooks_limit' do
def multiple_changes(change, count)
Array.new(count).map.with_index do |n, index|
{ index: index, oldrev: change[:oldrev], newrev: change[:newrev], ref: "#{change[:ref]}#{n}" }
end
end
let(:push_event_hooks_limit) { 3 }
let(:changes) do
......@@ -88,6 +88,40 @@ describe Git::ProcessRefChangesService do
end
end
context 'changes exceed push_event_activities_limit per action' do
let(:push_event_activities_limit) { 3 }
let(:changes) do
[
{ oldrev: Gitlab::Git::BLANK_SHA, newrev: '789012', ref: "#{ref_prefix}/create" },
{ oldrev: '123456', newrev: '789012', ref: "#{ref_prefix}/update" },
{ oldrev: '123456', newrev: Gitlab::Git::BLANK_SHA, ref: "#{ref_prefix}/delete" }
].map do |change|
multiple_changes(change, push_event_activities_limit + 1)
end.flatten
end
before do
stub_application_setting(push_event_activities_limit: push_event_activities_limit)
end
it "calls #{push_service_class} with create_push_event set to false" do
expect(push_service_class)
.to receive(:new)
.with(project, project.owner, hash_including(create_push_event: false))
.exactly(changes.count).times
.and_return(service)
subject.execute
end
it 'creates events per action' do
allow(push_service_class).to receive(:new).and_return(service)
expect { subject.execute }.to change { Event.count }.by(3)
end
end
context 'pipeline creation' do
context 'with valid .gitlab-ci.yml' do
before do
......
......@@ -28,6 +28,23 @@ describe 'events/event/_push.html.haml' do
expect(rendered).not_to have_link(event.ref_name)
end
end
context 'ref_count is more than 1' do
let(:payload) do
build_stubbed(
:push_event_payload,
event: event,
ref_count: 4,
ref_type: :branch
)
end
it 'includes the count in the text' do
render partial: 'events/event/push', locals: { event: event }
expect(rendered).to include('4 branches')
end
end
end
context 'with a tag' do
......@@ -53,5 +70,22 @@ describe 'events/event/_push.html.haml' do
expect(rendered).not_to have_link(event.ref_name)
end
end
context 'ref_count is more than 1' do
let(:payload) do
build_stubbed(
:push_event_payload,
event: event,
ref_count: 4,
ref_type: :tag
)
end
it 'includes the count in the text' do
render partial: 'events/event/push', locals: { event: event }
expect(rendered).to include('4 tags')
end
end
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