Commit 3cb84e06 authored by James Lopez's avatar James Lopez Committed by Rémy Coutable

Remove user activities table and use redis instead of PG for recording activities

Refactored specs and added a post deployment migration to remove the activity users table.
parent 2951a854
......@@ -101,7 +101,6 @@ class User < ActiveRecord::Base
has_many :assigned_issues, dependent: :nullify, foreign_key: :assignee_id, class_name: "Issue"
has_many :assigned_merge_requests, dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest"
has_one :user_activity, dependent: :destroy
# Issues that a user owns are expected to be moved to the "ghost" user before
# the user is destroyed. If the user owns any issues during deletion, this
......@@ -159,7 +158,6 @@ class User < ActiveRecord::Base
alias_attribute :private_token, :authentication_token
delegate :path, to: :namespace, allow_nil: true, prefix: true
delegate :last_activity_at, to: :user_activity, allow_nil: true
state_machine :state, initial: :active do
event :block do
......@@ -951,6 +949,14 @@ class User < ActiveRecord::Base
end
end
def record_activity
Gitlab::Redis.with do |redis|
redis.zadd('user/activities', Time.now.to_i, self.username)
end
end
private
def access_level=(new_level)
new_level = new_level.to_s
return unless %w(admin regular).include?(new_level)
......
class UserActivity < ActiveRecord::Base
belongs_to :user, inverse_of: :user_activity
validates :user, uniqueness: true, presence: true
validates :last_activity_at, presence: true
# Updated version of http://apidock.com/rails/ActiveRecord/Timestamp/touch
# That accepts a new record.
def touch
current_time = current_time_from_proper_timezone
if persisted?
update_column(:last_activity_at, current_time)
else
self.last_activity_at = current_time
save!(validate: false)
end
end
end
......@@ -14,13 +14,9 @@ module Users
private
def record_activity
user_activity.touch
@author.record_activity
Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username}")
end
def user_activity
UserActivity.find_or_initialize_by(user: @author)
end
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class DropUserActivitiesTable < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
# When using the methods "add_concurrent_index" or "add_column_with_default"
# you must disable the use of transactions as these methods can not run in an
# existing transaction. When using "add_concurrent_index" make sure that this
# method is the _only_ method called in the migration, any other changes
# should go in a separate migration. This ensures that upon failure _only_ the
# index creation fails and can be retried or reverted easily.
#
# To disable transactions uncomment the following line and remove these
# comments:
# disable_ddl_transaction!
def change
drop_table :user_activities
end
end
......@@ -1230,13 +1230,6 @@ ActiveRecord::Schema.define(version: 20170408033905) do
add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree
add_index "uploads", ["path"], name: "index_uploads_on_path", using: :btree
create_table "user_activities", force: :cascade do |t|
t.integer "user_id"
t.datetime "last_activity_at", null: false
end
add_index "user_activities", ["user_id"], name: "index_user_activities_on_user_id", unique: true, using: :btree
create_table "user_agent_details", force: :cascade do |t|
t.string "user_agent", null: false
t.string "ip_address", null: false
......@@ -1396,5 +1389,4 @@ ActiveRecord::Schema.define(version: 20170408033905) do
add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade
add_foreign_key "trending_projects", "projects", on_delete: :cascade
add_foreign_key "u2f_registrations", "users"
add_foreign_key "user_activities", "users", on_delete: :cascade
end
......@@ -60,6 +60,13 @@ module API
rescue JSON::ParserError
{}
end
def log_user_activity(actor)
commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS +
Gitlab::GitAccess::GIT_ANNEX_COMMANDS
::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
end
end
end
end
......@@ -16,7 +16,9 @@ describe SessionsController do
end
end
context 'when using valid password' do
context 'when using valid password', :redis do
include UserActivitiesHelpers
let(:user) { create(:user) }
it 'authenticates user correctly' do
......@@ -41,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.reload.last_activity_at }.from(nil)
end.to change { user_score }.from(0)
end
end
end
......
FactoryGirl.define do
factory :user_activity do
last_activity_at { Time.now }
user
end
end
......@@ -147,7 +147,9 @@ describe API::Internal, api: true do
end
end
describe "POST /internal/allowed" do
describe "POST /internal/allowed", :redis do
include UserActivitiesHelpers
context "access granted" do
before do
project.team << [user, :developer]
......@@ -181,7 +183,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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
expect(user_score).to be_zero
end
end
......@@ -192,7 +194,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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
expect(user_score).not_to be_zero
end
end
......@@ -203,7 +205,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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
expect(user_score).not_to be_zero
end
end
......@@ -214,7 +216,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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
expect(user_score).to be_zero
end
context 'project as /namespace/project' do
......@@ -250,7 +252,7 @@ describe API::Internal, api: true do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_falsey
expect(key.user.reload.last_activity_at).to be_nil
expect(user_score).to be_zero
end
end
......@@ -260,7 +262,7 @@ describe API::Internal, api: true do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_falsey
expect(key.user.reload.last_activity_at).to be_nil
expect(user_score).to be_zero
end
end
end
......@@ -278,7 +280,7 @@ describe API::Internal, api: true do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_falsey
expect(key.user.reload.last_activity_at).to be_nil
expect(user_score).to be_zero
end
end
......@@ -288,7 +290,7 @@ describe API::Internal, api: true do
expect(response).to have_http_status(200)
expect(json_response["status"]).to be_falsey
expect(key.user.reload.last_activity_at).to be_nil
expect(user_score).to be_zero
end
end
end
......
require 'spec_helper'
describe EventCreateService, services: true do
include UserActivitiesHelpers
let(:service) { EventCreateService.new }
describe 'Issues' do
......@@ -111,6 +113,19 @@ describe EventCreateService, services: true do
end
end
describe '#push', :redis do
let(:project) { create(:empty_project) }
let(:user) { create(:user) }
it 'creates a new event' do
expect { service.push(project, user, {}) }.to change { Event.count }
end
it 'updates user last activity' do
expect { service.push(project, user, {}) }.to change { user_score }
end
end
describe 'Project' do
let(:user) { create :user }
let(:project) { create(:empty_project) }
......@@ -129,17 +144,4 @@ describe EventCreateService, services: true do
it { expect { subject }.to change { Event.count }.from(0).to(1) }
end
end
describe '#push' do
let(:project) { create(:empty_project) }
let(:user) { create(:user) }
it 'creates a new event' do
expect { service.push(project, user, {}) }.to change { Event.count }
end
it 'updates user last activity' do
expect { service.push(project, user, {}) }.to change { user.last_activity_at }
end
end
end
require 'spec_helper'
describe Users::ActivityService, services: true do
include UserActivitiesHelpers
let(:user) { create(:user) }
subject(:service) { described_class.new(user, 'type') }
describe '#execute' do
describe '#execute', :redis do
context 'when last activity is nil' do
it 'sets the last activity timestamp' do
before do
service.execute
end
expect(user.last_activity_at).not_to be_nil
it 'sets the last activity timestamp for the user' do
expect(last_hour_members).to eq([user.username])
end
it 'updates the same user' do
service.execute
expect(last_hour_members).to eq([user.username])
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)
end
end
end
context 'when activity_at is not nil' do
it 'updates the activity multiple times' do
activity = create(:user_activity, user: user)
describe 'other user' do
it 'updates other user' do
other_user = create(:user)
described_class.new(other_user, 'type').execute
Timecop.travel(activity.last_activity_at + 1.minute) do
expect { service.execute }.to change { user.reload.last_activity_at }
expect(last_hour_members).to match_array([user.username, other_user.username])
end
end
end
......
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'
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