summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-09-11 09:40:59 +0000
committerJarka Kadlecova <jarka@gitlab.com>2017-09-12 13:08:13 +0200
commitaf326e2331e981576c5bd745a627ea92472225c8 (patch)
tree5cd3b93ade879a50374d3a0476d3b3386daf95ce
parent5442739f938b8a89bf7f663d18a63c19710f7d97 (diff)
downloadgitlab-ce-af326e2331e981576c5bd745a627ea92472225c8.tar.gz
Merge branch 'user-recent-push' into 'master'
Rework how recent push events are retrieved Closes #35990 See merge request !13995
-rw-r--r--app/helpers/projects_helper.rb10
-rw-r--r--app/models/event.rb2
-rw-r--r--app/models/push_event.rb38
-rw-r--r--app/models/user.rb19
-rw-r--r--app/services/event_create_service.rb13
-rw-r--r--app/services/users/last_push_event_service.rb83
-rw-r--r--changelogs/unreleased/user-recent-push.yml5
-rw-r--r--spec/features/dashboard/projects_spec.rb4
-rw-r--r--spec/helpers/projects_helper_spec.rb15
-rw-r--r--spec/models/push_event_spec.rb88
-rw-r--r--spec/models/user_spec.rb58
-rw-r--r--spec/services/event_create_service_spec.rb8
-rw-r--r--spec/services/users/last_push_event_service_spec.rb112
13 files changed, 369 insertions, 86 deletions
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index c0114dd0256..0c8cb9ba235 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -137,15 +137,7 @@ module ProjectsHelper
end
def last_push_event
- return unless current_user
- return current_user.recent_push unless @project
-
- project_ids = [@project.id]
- if fork = current_user.fork_of(@project)
- project_ids << fork.id
- end
-
- current_user.recent_push(project_ids)
+ current_user&.recent_push(@project)
end
def project_feature_access_select(field)
diff --git a/app/models/event.rb b/app/models/event.rb
index c313bbb66f8..8e9490b66f4 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -49,7 +49,7 @@ class Event < ActiveRecord::Base
belongs_to :author, class_name: "User"
belongs_to :project
belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations
- has_one :push_event_payload, foreign_key: :event_id
+ has_one :push_event_payload
# Callbacks
after_create :reset_project_activity
diff --git a/app/models/push_event.rb b/app/models/push_event.rb
index 23ffb0d4ea8..708513c7861 100644
--- a/app/models/push_event.rb
+++ b/app/models/push_event.rb
@@ -30,6 +30,44 @@ class PushEvent < Event
delegate :commit_count, to: :push_event_payload
alias_method :commits_count, :commit_count
+ # Returns events of pushes that either pushed to an existing ref or created a
+ # new one.
+ def self.created_or_pushed
+ actions = [
+ PushEventPayload.actions[:pushed],
+ PushEventPayload.actions[:created]
+ ]
+
+ joins(:push_event_payload)
+ .where(push_event_payloads: { action: actions })
+ end
+
+ # Returns events of pushes to a branch.
+ def self.branch_events
+ ref_type = PushEventPayload.ref_types[:branch]
+
+ joins(:push_event_payload)
+ .where(push_event_payloads: { ref_type: ref_type })
+ end
+
+ # Returns PushEvent instances for which no merge requests have been created.
+ def self.without_existing_merge_requests
+ existing_mrs = MergeRequest.except(:order)
+ .select(1)
+ .where('merge_requests.source_project_id = events.project_id')
+ .where('merge_requests.source_branch = push_event_payloads.ref')
+
+ # For reasons unknown the use of #eager_load will result in the
+ # "push_event_payload" association not being set. Because of this we're
+ # using "joins" here, which does mean an additional query needs to be
+ # executed in order to retrieve the "push_event_association" when the
+ # returned PushEvent is used.
+ joins(:push_event_payload)
+ .where('NOT EXISTS (?)', existing_mrs)
+ .created_or_pushed
+ .branch_events
+ end
+
def self.sti_name
PUSHED
end
diff --git a/app/models/user.rb b/app/models/user.rb
index d7549409b15..09c9b3250eb 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -651,20 +651,13 @@ class User < ActiveRecord::Base
@personal_projects_count ||= personal_projects.count
end
- def recent_push(project_ids = nil)
- # Get push events not earlier than 2 hours ago
- events = recent_events.code_push.where("created_at > ?", Time.now - 2.hours)
- events = events.where(project_id: project_ids) if project_ids
+ def recent_push(project = nil)
+ service = Users::LastPushEventService.new(self)
- # Use the latest event that has not been pushed or merged recently
- events.includes(:project).recent.find do |event|
- next unless event.project.repository.branch_exists?(event.branch_name)
-
- merge_requests = MergeRequest.where("created_at >= ?", event.created_at)
- .where(source_project_id: event.project.id,
- source_branch: event.branch_name)
-
- merge_requests.empty?
+ if project
+ service.last_event_for_project(project)
+ else
+ service.last_event_for_user
end
end
diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb
index 0b7e4f187f7..6328d567a07 100644
--- a/app/services/event_create_service.rb
+++ b/app/services/event_create_service.rb
@@ -74,12 +74,19 @@ class EventCreateService
# 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.transaction do
- event = create_event(project, current_user, Event::PUSHED)
+ event = Event.transaction do
+ new_event = create_event(project, current_user, Event::PUSHED)
- PushEventPayloadService.new(event, push_data).execute
+ PushEventPayloadService
+ .new(new_event, push_data)
+ .execute
+
+ new_event
end
+ Users::LastPushEventService.new(current_user)
+ .cache_last_push_event(event)
+
Users::ActivityService.new(current_user, 'push').execute
end
diff --git a/app/services/users/last_push_event_service.rb b/app/services/users/last_push_event_service.rb
new file mode 100644
index 00000000000..f2bfb60604f
--- /dev/null
+++ b/app/services/users/last_push_event_service.rb
@@ -0,0 +1,83 @@
+module Users
+ # Service class for caching and retrieving the last push event of a user.
+ class LastPushEventService
+ EXPIRATION = 2.hours
+
+ def initialize(user)
+ @user = user
+ end
+
+ # Caches the given push event for the current user in the Rails cache.
+ #
+ # event - An instance of PushEvent to cache.
+ def cache_last_push_event(event)
+ keys = [
+ project_cache_key(event.project),
+ user_cache_key
+ ]
+
+ if event.project.forked?
+ keys << project_cache_key(event.project.forked_from_project)
+ end
+
+ keys.each { |key| set_key(key, event.id) }
+ end
+
+ # Returns the last PushEvent for the current user.
+ #
+ # This method will return nil if no event was found.
+ def last_event_for_user
+ find_cached_event(user_cache_key)
+ end
+
+ # Returns the last PushEvent for the current user and the given project.
+ #
+ # project - An instance of Project for which to retrieve the PushEvent.
+ #
+ # This method will return nil if no event was found.
+ def last_event_for_project(project)
+ find_cached_event(project_cache_key(project))
+ end
+
+ def find_cached_event(cache_key)
+ event_id = get_key(cache_key)
+
+ return unless event_id
+
+ unless (event = find_event_in_database(event_id))
+ # We don't want to keep querying the same data over and over when a
+ # merge request has been created, thus we remove the key if no event
+ # (meaning an MR was created) is returned.
+ Rails.cache.delete(cache_key)
+ end
+
+ event
+ end
+
+ private
+
+ def find_event_in_database(id)
+ PushEvent
+ .without_existing_merge_requests
+ .find_by(id: id)
+ end
+
+ def user_cache_key
+ "last-push-event/#{@user.id}"
+ end
+
+ def project_cache_key(project)
+ "last-push-event/#{@user.id}/#{project.id}"
+ end
+
+ def get_key(key)
+ Rails.cache.read(key, raw: true)
+ end
+
+ def set_key(key, value)
+ # We're using raw values here since this takes up less space and we don't
+ # store complex objects.
+ Rails.cache.write(key, value, raw: true, expires_in: EXPIRATION)
+ end
+ end
+end
diff --git a/changelogs/unreleased/user-recent-push.yml b/changelogs/unreleased/user-recent-push.yml
new file mode 100644
index 00000000000..defd5cdfd8e
--- /dev/null
+++ b/changelogs/unreleased/user-recent-push.yml
@@ -0,0 +1,5 @@
+---
+title: Rework how recent push events are retrieved
+merge_request:
+author:
+type: other
diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb
index 0613c158c54..9a7b8e3ba6b 100644
--- a/spec/features/dashboard/projects_spec.rb
+++ b/spec/features/dashboard/projects_spec.rb
@@ -83,12 +83,14 @@ feature 'Dashboard Projects' do
end
end
- context 'last push widget' do
+ context 'last push widget', :use_clean_rails_memory_store_caching do
before do
event = create(:push_event, project: project, author: user)
create(:push_event_payload, event: event, ref: 'feature', action: :created)
+ Users::LastPushEventService.new(user).cache_last_push_event(event)
+
visit dashboard_projects_path
end
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index 49cb7c954b4..1437479831e 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -313,23 +313,10 @@ describe ProjectsHelper do
it 'returns recent push on the current project' do
event = double(:event)
- expect(user).to receive(:recent_push).with([project.id]).and_return(event)
+ expect(user).to receive(:recent_push).with(project).and_return(event)
expect(helper.last_push_event).to eq(event)
end
-
- context 'when current user has a fork of the current project' do
- let(:fork) { double(:fork, id: 2) }
-
- it 'returns recent push considering fork events' do
- expect(user).to receive(:fork_of).with(project).and_return(fork)
-
- event_on_fork = double(:event)
- expect(user).to receive(:recent_push).with([project.id, fork.id]).and_return(event_on_fork)
-
- expect(helper.last_push_event).to eq(event_on_fork)
- end
- end
end
describe "#project_feature_access_select" do
diff --git a/spec/models/push_event_spec.rb b/spec/models/push_event_spec.rb
index 532fb024261..ad3c3a406d9 100644
--- a/spec/models/push_event_spec.rb
+++ b/spec/models/push_event_spec.rb
@@ -11,6 +11,94 @@ describe PushEvent do
event
end
+ describe '.created_or_pushed' do
+ let(:event1) { create(:push_event) }
+ let(:event2) { create(:push_event) }
+ let(:event3) { create(:push_event) }
+
+ before do
+ create(:push_event_payload, event: event1, action: :pushed)
+ create(:push_event_payload, event: event2, action: :created)
+ create(:push_event_payload, event: event3, action: :removed)
+ end
+
+ let(:relation) { described_class.created_or_pushed }
+
+ it 'includes events for pushing to existing refs' do
+ expect(relation).to include(event1)
+ end
+
+ it 'includes events for creating new refs' do
+ expect(relation).to include(event2)
+ end
+
+ it 'does not include events for removing refs' do
+ expect(relation).not_to include(event3)
+ end
+ end
+
+ describe '.branch_events' do
+ let(:event1) { create(:push_event) }
+ let(:event2) { create(:push_event) }
+
+ before do
+ create(:push_event_payload, event: event1, ref_type: :branch)
+ create(:push_event_payload, event: event2, ref_type: :tag)
+ end
+
+ let(:relation) { described_class.branch_events }
+
+ it 'includes events for branches' do
+ expect(relation).to include(event1)
+ end
+
+ it 'does not include events for tags' do
+ expect(relation).not_to include(event2)
+ end
+ end
+
+ describe '.without_existing_merge_requests' do
+ let(:project) { create(:project, :repository) }
+ let(:event1) { create(:push_event, project: project) }
+ let(:event2) { create(:push_event, project: project) }
+ let(:event3) { create(:push_event, project: project) }
+ let(:event4) { create(:push_event, project: project) }
+
+ before do
+ create(:push_event_payload, event: event1, ref: 'foo', action: :created)
+ create(:push_event_payload, event: event2, ref: 'bar', action: :created)
+ create(:push_event_payload, event: event3, ref: 'baz', action: :removed)
+ create(:push_event_payload, event: event4, ref: 'baz', ref_type: :tag)
+
+ project.repository.create_branch('bar', 'master')
+
+ create(
+ :merge_request,
+ source_project: project,
+ target_project: project,
+ source_branch: 'bar'
+ )
+ end
+
+ let(:relation) { described_class.without_existing_merge_requests }
+
+ it 'includes events that do not have a corresponding merge request' do
+ expect(relation).to include(event1)
+ end
+
+ it 'does not include events that have a corresponding merge request' do
+ expect(relation).not_to include(event2)
+ end
+
+ it 'does not include events for removed refs' do
+ expect(relation).not_to include(event3)
+ end
+
+ it 'does not include events for pushing to tags' do
+ expect(relation).not_to include(event4)
+ end
+ end
+
describe '.sti_name' do
it 'returns Event::PUSHED' do
expect(described_class.sti_name).to eq(Event::PUSHED)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 73a1e47149c..c1affa812aa 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1349,56 +1349,24 @@ describe User do
end
describe "#recent_push" do
- subject { create(:user) }
- let!(:project1) { create(:project, :repository) }
- let!(:project2) { create(:project, :repository, forked_from_project: project1) }
-
- let!(:push_event) do
- event = create(:push_event, project: project2, author: subject)
-
- create(:push_event_payload,
- event: event,
- commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
- commit_count: 0,
- ref: 'master')
-
- event
- end
-
- before do
- project1.team << [subject, :master]
- project2.team << [subject, :master]
- end
-
- it "includes push event" do
- expect(subject.recent_push).to eq(push_event)
- end
-
- it "excludes push event if branch has been deleted" do
- allow_any_instance_of(Repository).to receive(:branch_exists?).with('master').and_return(false)
-
- expect(subject.recent_push).to eq(nil)
- end
+ let(:user) { build(:user) }
+ let(:project) { build(:project) }
+ let(:event) { build(:push_event) }
- it "excludes push event if MR is opened for it" do
- create(:merge_request, source_project: project2, target_project: project1, source_branch: project2.default_branch, target_branch: 'fix', author: subject)
+ it 'returns the last push event for the user' do
+ expect_any_instance_of(Users::LastPushEventService)
+ .to receive(:last_event_for_user)
+ .and_return(event)
- expect(subject.recent_push).to eq(nil)
+ expect(user.recent_push).to eq(event)
end
- it "includes push events on any of the provided projects" do
- expect(subject.recent_push(project1)).to eq(nil)
- expect(subject.recent_push(project2)).to eq(push_event)
-
- push_event1 = create(:push_event, project: project1, author: subject)
-
- create(:push_event_payload,
- event: push_event1,
- commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2',
- commit_count: 0,
- ref: 'master')
+ it 'returns the last push event for a project when one is given' do
+ expect_any_instance_of(Users::LastPushEventService)
+ .to receive(:last_event_for_project)
+ .and_return(event)
- expect(subject.recent_push([project1, project2])).to eq(push_event1) # Newest
+ expect(user.recent_push(project)).to eq(event)
end
end
diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb
index 02d7ddeb86b..13395a7cac3 100644
--- a/spec/services/event_create_service_spec.rb
+++ b/spec/services/event_create_service_spec.rb
@@ -149,6 +149,14 @@ describe EventCreateService do
.to change { user_activity(user) }
end
+ it 'caches the last push event for the user' do
+ expect_any_instance_of(Users::LastPushEventService)
+ .to receive(:cache_last_push_event)
+ .with(an_instance_of(PushEvent))
+
+ service.push(project, user, push_data)
+ end
+
it 'does not create any event data when an error is raised' do
payload_service = double(:service)
diff --git a/spec/services/users/last_push_event_service_spec.rb b/spec/services/users/last_push_event_service_spec.rb
new file mode 100644
index 00000000000..956358738fe
--- /dev/null
+++ b/spec/services/users/last_push_event_service_spec.rb
@@ -0,0 +1,112 @@
+require 'spec_helper'
+
+describe Users::LastPushEventService do
+ let(:user) { build(:user, id: 1) }
+ let(:project) { build(:project, id: 2) }
+ let(:event) { build(:push_event, id: 3, author: user, project: project) }
+ let(:service) { described_class.new(user) }
+
+ describe '#cache_last_push_event' do
+ it "caches the event for the event's project and current user" do
+ expect(service).to receive(:set_key)
+ .ordered
+ .with('last-push-event/1/2', 3)
+
+ expect(service).to receive(:set_key)
+ .ordered
+ .with('last-push-event/1', 3)
+
+ service.cache_last_push_event(event)
+ end
+
+ it 'caches the event for the origin project when pushing to a fork' do
+ source = build(:project, id: 5)
+
+ allow(project).to receive(:forked?).and_return(true)
+ allow(project).to receive(:forked_from_project).and_return(source)
+
+ expect(service).to receive(:set_key)
+ .ordered
+ .with('last-push-event/1/2', 3)
+
+ expect(service).to receive(:set_key)
+ .ordered
+ .with('last-push-event/1', 3)
+
+ expect(service).to receive(:set_key)
+ .ordered
+ .with('last-push-event/1/5', 3)
+
+ service.cache_last_push_event(event)
+ end
+ end
+
+ describe '#last_event_for_user' do
+ it 'returns the last push event for the current user' do
+ expect(service).to receive(:find_cached_event)
+ .with('last-push-event/1')
+ .and_return(event)
+
+ expect(service.last_event_for_user).to eq(event)
+ end
+
+ it 'returns nil when no push event could be found' do
+ expect(service).to receive(:find_cached_event)
+ .with('last-push-event/1')
+ .and_return(nil)
+
+ expect(service.last_event_for_user).to be_nil
+ end
+ end
+
+ describe '#last_event_for_project' do
+ it 'returns the last push event for the given project' do
+ expect(service).to receive(:find_cached_event)
+ .with('last-push-event/1/2')
+ .and_return(event)
+
+ expect(service.last_event_for_project(project)).to eq(event)
+ end
+
+ it 'returns nil when no push event could be found' do
+ expect(service).to receive(:find_cached_event)
+ .with('last-push-event/1/2')
+ .and_return(nil)
+
+ expect(service.last_event_for_project(project)).to be_nil
+ end
+ end
+
+ describe '#find_cached_event', :use_clean_rails_memory_store_caching do
+ context 'with a non-existing cache key' do
+ it 'returns nil' do
+ expect(service.find_cached_event('bla')).to be_nil
+ end
+ end
+
+ context 'with an existing cache key' do
+ before do
+ service.cache_last_push_event(event)
+ end
+
+ it 'returns a PushEvent when no merge requests exist for the event' do
+ allow(service).to receive(:find_event_in_database)
+ .with(event.id)
+ .and_return(event)
+
+ expect(service.find_cached_event('last-push-event/1')).to eq(event)
+ end
+
+ it 'removes the cache key when no event could be found and returns nil' do
+ allow(PushEvent).to receive(:without_existing_merge_requests)
+ .and_return(PushEvent.none)
+
+ expect(Rails.cache).to receive(:delete)
+ .with('last-push-event/1')
+ .and_call_original
+
+ expect(service.find_cached_event('last-push-event/1')).to be_nil
+ end
+ end
+ end
+end