Add user activity service and spec. Also added relevant - NOT offline - migration

It uses a user activity table instead of a column in users.
Tested with mySQL and postgreSQL

CHANGELOG-EE.md

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 ## 8.13.0 (2016-10-22)
+  - Add user activity table and service to query for active users
   - Fix 500 error updating mirror URLs for projects
   - Fix validations related to mirroring settings form. !773
   - Fix Git access panel for Wikis when Kerberos authentication is enabled (Borja Aparicio)

app/controllers/projects/git_http_controller.rb

@@ -7,6 +7,8 @@ class Projects::GitHttpController < Projects::GitHttpClientController
   # GET /foo/bar.git/info/refs?service=git-receive-pack (git push)
   def info_refs
     if upload_pack? && upload_pack_allowed?
+      log_user_activity
+
       render_ok
     elsif receive_pack? && receive_pack_allowed?
       render_ok
@@ -106,4 +108,8 @@ class Projects::GitHttpController < Projects::GitHttpClientController
     access_check.allowed?
   end
+
+  def log_user_activity
+    Users::ActivityService.new(user, 'pull').execute
+  end
 end

app/controllers/sessions_controller.rb

@@ -33,6 +33,7 @@ class SessionsController < Devise::SessionsController
                                    reset_password_sent_at: nil)
       end
       log_audit_event(current_user, with: authentication_method)
+      log_user_activity(current_user)
     end
   end
@@ -137,6 +138,10 @@ class SessionsController < Devise::SessionsController
       for_authentication.security_event
   end
+  def log_user_activity(user)
+    Users::ActivityService.new(user, 'login').execute
+  end
+
   def load_recaptcha
     Gitlab::Recaptcha.load_configurations!
   end

app/models/user.rb

@@ -92,6 +92,7 @@ class User < ActiveRecord::Base
   has_many :notification_settings,    dependent: :destroy
   has_many :award_emoji,              dependent: :destroy
   has_many :path_locks,               dependent: :destroy
+  has_one :user_activity, dependent: :destroy
   # Protected Branch Access
   has_many :protected_branch_merge_access_levels, dependent: :destroy, class_name: ProtectedBranch::MergeAccessLevel
@@ -146,6 +147,7 @@ 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

app/models/user_activity.rb

+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

app/services/event_create_service.rb

@@ -68,6 +68,8 @@ class EventCreateService
   def push(project, current_user, push_data)
     create_event(project, current_user, Event::PUSHED, data: push_data)
+
+    Users::ActivityService.new(current_user, 'push').execute
   end
   private

app/services/users/activity_service.rb

+module Users
+  class ActivityService
+    def initialize(author, activity)
+      @author = author.respond_to?(:user) ? author.user : author
+      @activity = activity
+    end
+
+    def execute
+      return unless @author && @author.is_a?(User)
+
+      record_activity
+    end
+
+    private
+
+    def record_activity
+      user_activity.touch
+
+      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

db/migrate/20161007073613_create_user_activities.rb

+class CreateUserActivities < ActiveRecord::Migration
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = true
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  DOWNTIME_REASON = 'Adding foreign key'
+
+  # 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
+    create_table :user_activities do |t|
+      t.belongs_to :user, index: { unique: true }, foreign_key: { on_delete: :cascade }
+      t.datetime :last_activity_at, null: false
+    end
+  end
+end

db/schema.rb

@@ -1254,6 +1254,13 @@ ActiveRecord::Schema.define(version: 20161007133303) do
   add_index "u2f_registrations", ["key_handle"], name: "index_u2f_registrations_on_key_handle", using: :btree
   add_index "u2f_registrations", ["user_id"], name: "index_u2f_registrations_on_user_id", 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
@@ -1392,4 +1399,5 @@ ActiveRecord::Schema.define(version: 20161007133303) do
   add_foreign_key "remote_mirrors", "projects"
   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

lib/api/internal.rb

@@ -43,6 +43,14 @@ module API
             :push_code
           ]
         end
+
+        def log_user_activity(actor)
+          commands = Gitlab::GitAccess::DOWNLOAD_COMMANDS +
+                      Gitlab::GitAccess::PUSH_COMMANDS +
+                      Gitlab::GitAccess::GIT_ANNEX_COMMANDS
+
+          ::Users::ActivityService.new(actor, 'Git SSH').execute if commands.include?(params[:action])
+        end
       end
       post "/allowed" do
@@ -69,6 +77,8 @@ module API
         response = { status: access_status.status, message: access_status.message }
         if access_status.status
+          log_user_activity(actor)
+
           # Return the repository full path so that gitlab-shell has it when
           # handling ssh commands
           response[:repository_path] =

spec/controllers/sessions_controller_spec.rb

@@ -30,6 +30,12 @@ describe SessionsController do
           expect { post(:create, user: { login: user.username, password: user.password }) }.to change { SecurityEvent.count }.by(1)
           expect(SecurityEvent.last.details[:with]).to eq("standard")
         end
+
+        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
       end
     end

spec/factories/user_activities.rb

+FactoryGirl.define do
+  factory :user_activity do
+    last_activity_at { Time.now }
+    user
+  end
+end

spec/requests/api/internal_spec.rb

@@ -199,6 +199,11 @@ describe API::API, api: true  do
     context "access granted" do
       before do
         project.team << [user, :developer]
+        Timecop.freeze
+      end
+
+      after do
+        Timecop.return
       end
       context "git push with project.wiki" do
@@ -208,6 +213,7 @@ describe API::API, 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)
         end
       end
@@ -218,6 +224,7 @@ describe API::API, 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)
         end
       end
@@ -228,6 +235,7 @@ describe API::API, 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)
         end
       end
@@ -238,6 +246,7 @@ describe API::API, 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)
         end
       end
     end
@@ -253,6 +262,7 @@ describe API::API, 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
         end
       end
@@ -262,6 +272,7 @@ describe API::API, 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
         end
       end
     end
@@ -279,6 +290,7 @@ describe API::API, 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
         end
       end
@@ -288,6 +300,7 @@ describe API::API, 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
         end
       end
     end

spec/requests/git_http_spec.rb

@@ -178,6 +178,12 @@ describe 'Git HTTP requests', lib: true do
                   expect(response.status).to eq(200)
                 end
               end
+
+              it 'updates the user last activity' do
+                download(path, env) do |response|
+                  expect(user.reload.last_activity_at).not_to be_nil
+                end
+              end
             end
             it "complies with RFC4559" do

spec/services/event_create_service_spec.rb

@@ -110,4 +110,17 @@ describe EventCreateService, services: true do
       end
     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

spec/services/users/activity_service_spec.rb

+require 'spec_helper'
+
+describe Users::ActivityService, services: true do
+  let(:user) { create(:user) }
+  subject(:service) { described_class.new(user, 'type') }
+
+  describe '#execute' do
+    context 'when last activity is nil' do
+      it 'sets the last activity timestamp' do
+        service.execute
+
+        expect(user.last_activity_at).not_to be_nil
+      end
+    end
+
+    context 'when activity_at is not nil' do
+      it 'updates the activity multiple times' do
+        activity = create(:user_activity, user: user)
+
+        Timecop.travel(activity.last_activity_at + 1.minute) do
+          expect { service.execute }.to change { user.reload.last_activity_at }
+        end
+      end
+    end
+  end
+end