diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index 483821b7f01..facf21a7f5c 100644 --- a/app/services/users/activity_service.rb +++ b/app/services/users/activity_service.rb @@ -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 diff --git a/lib/gitlab/user_activities.rb b/lib/gitlab/user_activities.rb new file mode 100644 index 00000000000..eb36ab9fded --- /dev/null +++ b/lib/gitlab/user_activities.rb @@ -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 diff --git a/lib/gitlab/user_activities/activity.rb b/lib/gitlab/user_activities/activity.rb deleted file mode 100644 index ec052870ee3..00000000000 --- a/lib/gitlab/user_activities/activity.rb +++ /dev/null @@ -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 diff --git a/lib/gitlab/user_activities/activity_set.rb b/lib/gitlab/user_activities/activity_set.rb deleted file mode 100644 index 6b8e540e99b..00000000000 --- a/lib/gitlab/user_activities/activity_set.rb +++ /dev/null @@ -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 diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index d817099394a..038132cffe0 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -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 diff --git a/spec/lib/gitlab/user_activities/activity_set_spec.rb b/spec/lib/gitlab/user_activities/activity_set_spec.rb deleted file mode 100644 index 56745bdf0d1..00000000000 --- a/spec/lib/gitlab/user_activities/activity_set_spec.rb +++ /dev/null @@ -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 diff --git a/spec/lib/gitlab/user_activities/activity_spec.rb b/spec/lib/gitlab/user_activities/activity_spec.rb deleted file mode 100644 index 6a1150f50c1..00000000000 --- a/spec/lib/gitlab/user_activities/activity_spec.rb +++ /dev/null @@ -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 diff --git a/spec/lib/gitlab/user_activities_spec.rb b/spec/lib/gitlab/user_activities_spec.rb new file mode 100644 index 00000000000..187d88c8c58 --- /dev/null +++ b/spec/lib/gitlab/user_activities_spec.rb @@ -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 diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index eee8bd51bff..3d6010ede73 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -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 diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 13c0aac2363..b06cefe071d 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -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 diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index 07715ad4ca0..8d67ebe3231 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -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 diff --git a/spec/support/matchers/user_activity_matchers.rb b/spec/support/matchers/user_activity_matchers.rb new file mode 100644 index 00000000000..ce3b683b6d2 --- /dev/null +++ b/spec/support/matchers/user_activity_matchers.rb @@ -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 diff --git a/spec/support/user_activities_helpers.rb b/spec/support/user_activities_helpers.rb index ef88dab6c91..f7ca9a31edd 100644 --- a/spec/support/user_activities_helpers.rb +++ b/spec/support/user_activities_helpers.rb @@ -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