Add a new Gitlab::UserActivities class to track user activities
This new class uses a Redis Hash instead of a Sorted Set. Signed-off-by: Rémy Coutable <remy@rymai.me>
This commit is contained in:
parent
91ac0e038a
commit
cfe19b795e
13 changed files with 191 additions and 205 deletions
|
@ -14,7 +14,7 @@ module Users
|
|||
private
|
||||
|
||||
def record_activity
|
||||
@author.record_activity
|
||||
Gitlab::UserActivities.record(@author.id)
|
||||
|
||||
Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username}")
|
||||
end
|
||||
|
|
34
lib/gitlab/user_activities.rb
Normal file
34
lib/gitlab/user_activities.rb
Normal file
|
@ -0,0 +1,34 @@
|
|||
module Gitlab
|
||||
class UserActivities
|
||||
include Enumerable
|
||||
|
||||
KEY = 'users:activities'.freeze
|
||||
BATCH_SIZE = 500
|
||||
|
||||
def self.record(key, time = Time.now)
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.hset(KEY, key, time.to_i)
|
||||
end
|
||||
end
|
||||
|
||||
def delete(*keys)
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.hdel(KEY, keys)
|
||||
end
|
||||
end
|
||||
|
||||
def each
|
||||
cursor = 0
|
||||
loop do
|
||||
cursor, pairs =
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.hscan(KEY, cursor, count: BATCH_SIZE)
|
||||
end
|
||||
|
||||
Hash[pairs].each { |pair| yield pair }
|
||||
|
||||
break if cursor == '0'
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1,16 +0,0 @@
|
|||
module Gitlab
|
||||
module UserActivities
|
||||
class Activity
|
||||
attr_reader :username
|
||||
|
||||
def initialize(username, time)
|
||||
@username = username
|
||||
@time = time
|
||||
end
|
||||
|
||||
def last_activity_at
|
||||
@last_activity_at ||= Time.at(@time).to_s(:db)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1,67 +0,0 @@
|
|||
module Gitlab
|
||||
module UserActivities
|
||||
class ActivitySet
|
||||
delegate :total_count,
|
||||
:total_pages,
|
||||
:current_page,
|
||||
:limit_value,
|
||||
:first_page?,
|
||||
:prev_page,
|
||||
:last_page?,
|
||||
:next_page, to: :pagination_delegate
|
||||
|
||||
KEY = 'user/activities'
|
||||
|
||||
def self.record(user)
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.zadd(KEY, Time.now.to_i, user.username)
|
||||
end
|
||||
end
|
||||
|
||||
def initialize(from: nil, page: nil, per_page: nil)
|
||||
@from = sanitize_date(from)
|
||||
@to = Time.now.to_i
|
||||
@page = page
|
||||
@per_page = per_page
|
||||
end
|
||||
|
||||
def activities
|
||||
@activities ||= raw_activities.map { |activity| Activity.new(*activity) }
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def sanitize_date(date)
|
||||
Time.strptime(date, "%Y-%m-%d").to_i
|
||||
rescue TypeError, ArgumentError
|
||||
default_from
|
||||
end
|
||||
|
||||
def pagination_delegate
|
||||
@pagination_delegate ||= Gitlab::PaginationDelegate.new(page: @page,
|
||||
per_page: @per_page,
|
||||
count: count)
|
||||
end
|
||||
|
||||
def raw_activities
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.zrangebyscore(KEY, @from, @to, with_scores: true, limit: limit)
|
||||
end
|
||||
end
|
||||
|
||||
def count
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.zcount(KEY, @from, @to)
|
||||
end
|
||||
end
|
||||
|
||||
def limit
|
||||
[pagination_delegate.offset, pagination_delegate.limit_value]
|
||||
end
|
||||
|
||||
def default_from
|
||||
6.months.ago.to_i
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -43,7 +43,7 @@ describe SessionsController do
|
|||
it 'updates the user activity' do
|
||||
expect do
|
||||
post(:create, user: { login: user.username, password: user.password })
|
||||
end.to change { user_score }.from(0)
|
||||
end.to change { user_activity(user) }
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -1,77 +0,0 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Gitlab::UserActivities::ActivitySet, :redis, lib: true do
|
||||
let(:user) { create(:user) }
|
||||
|
||||
it 'shows the last user activity' do
|
||||
Timecop.freeze do
|
||||
user.record_activity
|
||||
|
||||
expect(described_class.new.activities.first).to be_an_instance_of(Gitlab::UserActivities::Activity)
|
||||
end
|
||||
end
|
||||
|
||||
context 'pagination delegation' do
|
||||
let(:pagination_delegate) do
|
||||
Gitlab::PaginationDelegate.new(page: 1,
|
||||
per_page: 10,
|
||||
count: 20)
|
||||
end
|
||||
|
||||
let(:delegated_methods) { %i[total_count total_pages current_page limit_value first_page? prev_page last_page? next_page] }
|
||||
|
||||
before do
|
||||
allow(described_class.new).to receive(:pagination_delegate).and_return(pagination_delegate)
|
||||
end
|
||||
|
||||
it 'includes the delegated methods' do
|
||||
expect(described_class.new.public_methods).to include(*delegated_methods)
|
||||
end
|
||||
end
|
||||
|
||||
context 'paginated activities' do
|
||||
before do
|
||||
Timecop.scale(3600)
|
||||
|
||||
7.times do
|
||||
create(:user).record_activity
|
||||
end
|
||||
end
|
||||
|
||||
after do
|
||||
Timecop.return
|
||||
end
|
||||
|
||||
it 'shows the 5 oldest user activities paginated' do
|
||||
expect(described_class.new(per_page: 5).activities.count).to eq(5)
|
||||
end
|
||||
|
||||
it 'shows the 2 reamining user activities paginated' do
|
||||
expect(described_class.new(per_page: 5, page: 2).activities.count).to eq(2)
|
||||
end
|
||||
|
||||
it 'shows the oldest first' do
|
||||
activities = described_class.new.activities
|
||||
|
||||
expect(activities.first.last_activity_at).to be < activities.last.last_activity_at
|
||||
end
|
||||
end
|
||||
|
||||
context 'filter by date' do
|
||||
before do
|
||||
create(:user).record_activity
|
||||
end
|
||||
|
||||
it 'shows activities from today' do
|
||||
today = Date.today.to_s("%Y-%m-%d")
|
||||
|
||||
expect(described_class.new(from: today).activities.count).to eq(1)
|
||||
end
|
||||
|
||||
it 'filter activities from tomorrow' do
|
||||
tomorrow = Date.tomorrow.to_s("%Y-%m-%d")
|
||||
|
||||
expect(described_class.new(from: tomorrow).activities.count).to eq(0)
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1,14 +0,0 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Gitlab::UserActivities::Activity, :redis, lib: true do
|
||||
let(:username) { 'user' }
|
||||
let(:activity) { described_class.new('user', Time.new(2016, 12, 12).to_i) }
|
||||
|
||||
it 'has the username' do
|
||||
expect(activity.username).to eq(username)
|
||||
end
|
||||
|
||||
it 'has the last activity at' do
|
||||
expect(activity.last_activity_at).to eq('2016-12-12 00:00:00')
|
||||
end
|
||||
end
|
127
spec/lib/gitlab/user_activities_spec.rb
Normal file
127
spec/lib/gitlab/user_activities_spec.rb
Normal file
|
@ -0,0 +1,127 @@
|
|||
require 'spec_helper'
|
||||
|
||||
describe Gitlab::UserActivities, :redis, lib: true do
|
||||
let(:now) { Time.now }
|
||||
|
||||
describe '.record' do
|
||||
context 'with no time given' do
|
||||
it 'uses Time.now and records an activity in Redis' do
|
||||
Timecop.freeze do
|
||||
now # eager-load now
|
||||
described_class.record(42)
|
||||
end
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'with a time given' do
|
||||
it 'uses the given time and records an activity in Redis' do
|
||||
described_class.record(42, now)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe '.delete' do
|
||||
context 'with a single key' do
|
||||
context 'and key exists' do
|
||||
it 'removes the pair from Redis' do
|
||||
described_class.record(42, now)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
|
||||
end
|
||||
|
||||
subject.delete(42)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'and key does not exist' do
|
||||
it 'removes the pair from Redis' do
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
|
||||
end
|
||||
|
||||
subject.delete(42)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'with multiple keys' do
|
||||
context 'and all keys exist' do
|
||||
it 'removes the pair from Redis' do
|
||||
described_class.record(41, now)
|
||||
described_class.record(42, now)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['41', now.to_i.to_s], ['42', now.to_i.to_s]]])
|
||||
end
|
||||
|
||||
subject.delete(41, 42)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context 'and some keys does not exist' do
|
||||
it 'removes the existing pair from Redis' do
|
||||
described_class.record(42, now)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', [['42', now.to_i.to_s]]])
|
||||
end
|
||||
|
||||
subject.delete(41, 42)
|
||||
|
||||
Gitlab::Redis.with do |redis|
|
||||
expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe 'Enumerable' do
|
||||
before do
|
||||
described_class.record(40, now)
|
||||
described_class.record(41, now)
|
||||
described_class.record(42, now)
|
||||
end
|
||||
|
||||
it 'allows to read the activities sequentially' do
|
||||
expected = { '40' => now.to_i.to_s, '41' => now.to_i.to_s, '42' => now.to_i.to_s }
|
||||
|
||||
actual = described_class.new.each_with_object({}) do |(key, time), actual|
|
||||
actual[key] = time
|
||||
end
|
||||
|
||||
expect(actual).to eq(expected)
|
||||
end
|
||||
|
||||
context 'with many records' do
|
||||
before do
|
||||
1_000.times { |i| described_class.record(i, now) }
|
||||
end
|
||||
|
||||
it 'is possible to loop through all the records' do
|
||||
expect(described_class.new.count).to eq(1_000)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
|
@ -148,8 +148,6 @@ describe API::Internal, api: true do
|
|||
end
|
||||
|
||||
describe "POST /internal/allowed", :redis do
|
||||
include UserActivitiesHelpers
|
||||
|
||||
context "access granted" do
|
||||
before do
|
||||
project.team << [user, :developer]
|
||||
|
@ -183,7 +181,7 @@ describe API::Internal, api: true do
|
|||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_truthy
|
||||
expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -194,7 +192,7 @@ describe API::Internal, api: true do
|
|||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_truthy
|
||||
expect(json_response["repository_path"]).to eq(project.wiki.repository.path_to_repo)
|
||||
expect(user_score).not_to be_zero
|
||||
expect(user).to have_an_activity_record
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -205,7 +203,7 @@ describe API::Internal, api: true do
|
|||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_truthy
|
||||
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
|
||||
expect(user_score).not_to be_zero
|
||||
expect(user).to have_an_activity_record
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -216,7 +214,7 @@ describe API::Internal, api: true do
|
|||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_truthy
|
||||
expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
|
||||
context 'project as /namespace/project' do
|
||||
|
@ -252,7 +250,7 @@ describe API::Internal, api: true do
|
|||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_falsey
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -262,7 +260,7 @@ describe API::Internal, api: true do
|
|||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_falsey
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -280,7 +278,7 @@ describe API::Internal, api: true do
|
|||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_falsey
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -290,7 +288,7 @@ describe API::Internal, api: true do
|
|||
|
||||
expect(response).to have_http_status(200)
|
||||
expect(json_response["status"]).to be_falsey
|
||||
expect(user_score).to be_zero
|
||||
expect(user).not_to have_an_activity_record
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -122,7 +122,7 @@ describe EventCreateService, services: true do
|
|||
end
|
||||
|
||||
it 'updates user last activity' do
|
||||
expect { service.push(project, user, {}) }.to change { user_score }
|
||||
expect { service.push(project, user, {}) }.to change { user_activity(user) }
|
||||
end
|
||||
end
|
||||
|
||||
|
|
|
@ -14,18 +14,18 @@ describe Users::ActivityService, services: true do
|
|||
end
|
||||
|
||||
it 'sets the last activity timestamp for the user' do
|
||||
expect(last_hour_members).to eq([user.username])
|
||||
expect(last_hour_user_ids).to eq([user.id])
|
||||
end
|
||||
|
||||
it 'updates the same user' do
|
||||
service.execute
|
||||
|
||||
expect(last_hour_members).to eq([user.username])
|
||||
expect(last_hour_user_ids).to eq([user.id])
|
||||
end
|
||||
|
||||
it 'updates the timestamp of an existing user' do
|
||||
Timecop.freeze(Date.tomorrow) do
|
||||
expect { service.execute }.to change { user_score }.to(Time.now.to_i)
|
||||
expect { service.execute }.to change { user_activity(user) }.to(Time.now.to_i.to_s)
|
||||
end
|
||||
end
|
||||
|
||||
|
@ -34,9 +34,15 @@ describe Users::ActivityService, services: true do
|
|||
other_user = create(:user)
|
||||
described_class.new(other_user, 'type').execute
|
||||
|
||||
expect(last_hour_members).to match_array([user.username, other_user.username])
|
||||
expect(last_hour_user_ids).to match_array([user.id, other_user.id])
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def last_hour_user_ids
|
||||
Gitlab::UserActivities.new.
|
||||
select { |k, v| v >= 1.hour.ago.to_i.to_s }.
|
||||
map { |k, _| k.to_i }
|
||||
end
|
||||
end
|
||||
|
|
5
spec/support/matchers/user_activity_matchers.rb
Normal file
5
spec/support/matchers/user_activity_matchers.rb
Normal file
|
@ -0,0 +1,5 @@
|
|||
RSpec::Matchers.define :have_an_activity_record do |expected|
|
||||
match do |user|
|
||||
expect(Gitlab::UserActivities.new.find { |k, _| k == user.id.to_s }).to be_present
|
||||
end
|
||||
end
|
|
@ -1,17 +1,7 @@
|
|||
module UserActivitiesHelpers
|
||||
def last_hour_members
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.zrangebyscore(user_activities_key, 1.hour.ago.to_i, Time.now.to_i)
|
||||
end
|
||||
end
|
||||
|
||||
def user_score
|
||||
Gitlab::Redis.with do |redis|
|
||||
redis.zscore(user_activities_key, user.username).to_i
|
||||
end
|
||||
end
|
||||
|
||||
def user_activities_key
|
||||
'user/activities'
|
||||
def user_activity(user)
|
||||
Gitlab::UserActivities.new.
|
||||
find { |k, _| k == user.id.to_s }&.
|
||||
second
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue