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

app/controllers/projects/git_http_controller.rb

@@ -5,6 +5,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
   def access_klass
     @access_klass ||= wiki? ? Gitlab::GitAccessWiki : Gitlab::GitAccess
   end
+
+  def log_user_activity
+    Users::ActivityService.new(user, 'pull').execute
+  end
 end

app/controllers/sessions_controller.rb

@@ -35,6 +35,7 @@ class SessionsController < Devise::SessionsController
       # hide the signed-in notification
       flash[:notice] = nil
       log_audit_event(current_user, with: authentication_method)
+      log_user_activity(current_user)
     end
   end
 
@@ -123,6 +124,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

@@ -101,6 +101,7 @@ 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
@@ -158,6 +159,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

@@ -0,0 +1,19 @@
+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

@@ -72,6 +72,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

@@ -0,0 +1,26 @@
+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

@@ -0,0 +1,27 @@
+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

@@ -1230,6 +1230,13 @@ 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
@@ -1389,4 +1396,5 @@ 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

lib/api/internal.rb

@@ -15,6 +15,16 @@ module API
       #   project - project path with namespace
       #   action - git action (git-upload-pack or git-receive-pack)
       #   changes - changes as "oldrev newrev ref", see Gitlab::ChangesList
+      helpers do
+        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
         status 200
 
@@ -40,6 +50,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

@@ -37,6 +37,12 @@ describe SessionsController do
             subject.sign_out user
           end
         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

@@ -0,0 +1,6 @@
+FactoryGirl.define do
+  factory :user_activity do
+    last_activity_at { Time.now }
+    user
+  end
+end

spec/requests/api/internal_spec.rb

@@ -151,6 +151,11 @@ describe API::Internal, api: true  do
     context "access granted" do
       before do
         project.team << [user, :developer]
+        Timecop.freeze
+      end
+
+      after do
+        Timecop.return
       end
 
       context 'with env passed as a JSON' do
@@ -176,6 +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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
         end
       end
 
@@ -186,6 +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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
         end
       end
 
@@ -196,6 +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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
         end
       end
 
@@ -206,6 +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(key.user.reload.last_activity_at.to_i).to eq(Time.now.to_i)
         end
 
         context 'project as /namespace/project' do
@@ -241,6 +250,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
         end
       end
 
@@ -250,6 +260,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
         end
       end
     end
@@ -267,6 +278,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
         end
       end
 
@@ -276,6 +288,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
         end
       end
     end

spec/requests/git_http_spec.rb

@@ -157,6 +157,12 @@ describe 'Git HTTP requests', lib: true do
                 expect(response).to have_http_status(:ok)
               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
 
           context 'but only project members are allowed' do

spec/services/event_create_service_spec.rb

@@ -129,4 +129,17 @@ 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

spec/services/users/activity_service_spec.rb

@@ -0,0 +1,26 @@
+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