From 3cb84e06b7a118fb46b4e1e0d4885026c9d4a4d1 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 25 Nov 2016 17:10:25 +0100 Subject: 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. --- app/models/user.rb | 10 +++++-- app/models/user_activity.rb | 19 ------------ app/services/users/activity_service.rb | 6 +--- .../20161128170531_drop_user_activities_table.rb | 23 +++++++++++++++ db/schema.rb | 8 ----- lib/api/helpers/internal_helpers.rb | 7 +++++ spec/controllers/sessions_controller_spec.rb | 6 ++-- spec/factories/user_activities.rb | 6 ---- spec/requests/api/internal_spec.rb | 20 +++++++------ spec/services/event_create_service_spec.rb | 28 +++++++++--------- spec/services/users/activity_service_spec.rb | 34 ++++++++++++++++------ spec/support/user_activities_helpers.rb | 17 +++++++++++ 12 files changed, 111 insertions(+), 73 deletions(-) delete mode 100644 app/models/user_activity.rb create mode 100644 db/post_migrate/20161128170531_drop_user_activities_table.rb delete mode 100644 spec/factories/user_activities.rb create mode 100644 spec/support/user_activities_helpers.rb diff --git a/app/models/user.rb b/app/models/user.rb index 1dde5c89699..83e17bcb04e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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) diff --git a/app/models/user_activity.rb b/app/models/user_activity.rb deleted file mode 100644 index c5fdbff0feb..00000000000 --- a/app/models/user_activity.rb +++ /dev/null @@ -1,19 +0,0 @@ -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 diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index b81f947cd01..483821b7f01 100644 --- a/app/services/users/activity_service.rb +++ b/app/services/users/activity_service.rb @@ -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 diff --git a/db/post_migrate/20161128170531_drop_user_activities_table.rb b/db/post_migrate/20161128170531_drop_user_activities_table.rb new file mode 100644 index 00000000000..3ece0722821 --- /dev/null +++ b/db/post_migrate/20161128170531_drop_user_activities_table.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 3ed28a02de2..1c592dd5d6d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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 diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index 810e5063996..8f25bcf2f54 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -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 diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 3459f30ef42..d817099394a 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -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 diff --git a/spec/factories/user_activities.rb b/spec/factories/user_activities.rb deleted file mode 100644 index 32ad8c6a3b2..00000000000 --- a/spec/factories/user_activities.rb +++ /dev/null @@ -1,6 +0,0 @@ -FactoryGirl.define do - factory :user_activity do - last_activity_at { Time.now } - user - end -end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 63f566da7a8..eee8bd51bff 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -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 diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 54e5c0b236b..13c0aac2363 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -1,6 +1,8 @@ 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 diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index 68399118579..07715ad4ca0 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/spec/services/users/activity_service_spec.rb @@ -1,24 +1,40 @@ 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 diff --git a/spec/support/user_activities_helpers.rb b/spec/support/user_activities_helpers.rb new file mode 100644 index 00000000000..ef88dab6c91 --- /dev/null +++ b/spec/support/user_activities_helpers.rb @@ -0,0 +1,17 @@ +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 -- cgit v1.2.1