Merge branch 'master' into sh-support-bitbucket-server-import

app/models/user.rb

@@ -14,7 +14,6 @@ class User < ActiveRecord::Base
   include IgnorableColumn
   include FeatureGate
   include CreatedAtFilterable
-  include IgnorableColumn
   include BulkMemberAccessLoad
   include BlocksJsonSerialization
   include WithUploads

app/services/prometheus/adapter_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Prometheus
   class AdapterService
     def initialize(project, deployment_platform = nil)

app/services/protected_branches/access_level_params.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedBranches
   class AccessLevelParams
     attr_reader :type, :params

app/services/protected_branches/api_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedBranches
   class ApiService < BaseService
     def create

app/services/protected_branches/create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedBranches
   class CreateService < BaseService
     def execute(skip_authorization: false)

app/services/protected_branches/destroy_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedBranches
   class DestroyService < BaseService
     def execute(protected_branch)

app/services/protected_branches/legacy_api_create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # The branches#protect API still uses the `developers_can_push` and `developers_can_merge`
 # flags for backward compatibility, and so performs translation between that format and the
 # internal data model (separate access levels). The translation code is non-trivial, and so

app/services/protected_branches/legacy_api_update_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # The branches#protect API still uses the `developers_can_push` and `developers_can_merge`
 # flags for backward compatibility, and so performs translation between that format and the
 # internal data model (separate access levels). The translation code is non-trivial, and so

app/services/protected_branches/update_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedBranches
   class UpdateService < BaseService
     def execute(protected_branch)

app/services/protected_tags/create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedTags
   class CreateService < BaseService
     attr_reader :protected_tag

app/services/protected_tags/destroy_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedTags
   class DestroyService < BaseService
     def execute(protected_tag)

app/services/protected_tags/update_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ProtectedTags
   class UpdateService < BaseService
     def execute(protected_tag)

app/services/quick_actions/interpret_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module QuickActions
   class InterpretService < BaseService
     include Gitlab::QuickActions::Dsl

app/services/search/global_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Search
   class GlobalService
     attr_accessor :current_user, :params

app/services/search/group_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Search
   class GroupService < Search::GlobalService
     attr_accessor :group

app/services/search/project_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Search
   class ProjectService
     attr_accessor :project, :current_user, :params

app/services/search/snippet_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Search
   class SnippetService
     attr_accessor :current_user, :params

app/services/tags/create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Tags
   class CreateService < BaseService
     def execute(tag_name, target, message, release_description = nil)

app/services/tags/destroy_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Tags
   class DestroyService < BaseService
     def execute(tag_name)

app/services/test_hooks/base_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module TestHooks
   class BaseService
     attr_accessor :hook, :current_user, :trigger

app/services/test_hooks/project_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module TestHooks
   class ProjectService < TestHooks::BaseService
     attr_writer :project

app/services/test_hooks/system_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module TestHooks
   class SystemService < TestHooks::BaseService
     private

app/services/users/activity_service.rb

@@ -1,12 +1,21 @@
+# frozen_string_literal: true
+
 module Users
   class ActivityService
+    LEASE_TIMEOUT = 1.minute.to_i
+
     def initialize(author, activity)
-      @author = author.respond_to?(:user) ? author.user : author
+      @user = if author.respond_to?(:username)
+                author
+              elsif author.respond_to?(:user)
+                author.user
+              end
+
       @activity = activity
     end
 
     def execute
-      return unless @author && @author.is_a?(User)
+      return unless @user
 
       record_activity
     end
@@ -14,9 +23,14 @@ module Users
     private
 
     def record_activity
-      Gitlab::UserActivities.record(@author.id) if Gitlab::Database.read_write?
+      return if Gitlab::Database.read_only?
 
-      Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username})")
+      lease = Gitlab::ExclusiveLease.new("acitvity_service:#{@user.id}",
+                                         timeout: LEASE_TIMEOUT)
+      return unless lease.try_obtain
+
+      @user.update_attribute(:last_activity_on, Date.today)
+      Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@user.id} (username: #{@user.username})")
     end
   end
 end

app/services/users/build_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   class BuildService < BaseService
     def initialize(current_user, params = {})

app/services/users/create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   class CreateService < BaseService
     include NewUserNotifier

app/services/users/destroy_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   class DestroyService
     attr_accessor :current_user

app/services/users/last_push_event_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   # Service class for caching and retrieving the last push event of a user.
   class LastPushEventService

app/services/users/migrate_to_ghost_user_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 # When a user is destroyed, some of their associated records are
 # moved to a "Ghost User", to prevent these associated records from
 # being destroyed.

app/services/users/refresh_authorized_projects_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   # Service for refreshing the authorized projects of a user.
   #

app/services/users/respond_to_terms_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   class RespondToTermsService
     def initialize(user, term)

app/services/users/update_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module Users
   class UpdateService < BaseService
     include NewUserNotifier

app/services/wiki_pages/base_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module WikiPages
   class BaseService < ::BaseService
     private

app/services/wiki_pages/create_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module WikiPages
   class CreateService < WikiPages::BaseService
     def execute

app/services/wiki_pages/destroy_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module WikiPages
   class DestroyService < WikiPages::BaseService
     def execute(page)

app/services/wiki_pages/update_service.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module WikiPages
   class UpdateService < WikiPages::BaseService
     def execute(page)

app/views/profiles/two_factor_auths/show.html.haml

@@ -13,10 +13,16 @@
       - if current_user.two_factor_otp_enabled?
         %p
           You've already enabled two-factor authentication using mobile authenticator applications. In order to register a different device, you must first disable two-factor authentication.
+        %p
+          If you lose your recovery codes you can generate new ones, invalidating all previous codes.
+        %div
           = link_to 'Disable two-factor authentication', profile_two_factor_auth_path,
                 method: :delete,
                 data: { confirm: "Are you sure? This will invalidate your registered applications and U2F devices." },
-                class: 'btn btn-danger'
+                class: 'btn btn-danger append-right-10'
+          = form_tag codes_profile_two_factor_auth_path, {style: 'display: inline-block', method: :post} do |f|
+            = submit_tag 'Regenerate recovery codes', class: 'btn'
+
       - else
         %p
           Download the Google Authenticator application from App Store or Google Play Store and scan this code.

app/workers/all_queues.yml

@@ -13,7 +13,6 @@
 - cronjob:repository_archive_cache
 - cronjob:repository_check_dispatch
 - cronjob:requests_profiles
-- cronjob:schedule_update_user_activity
 - cronjob:stuck_ci_jobs
 - cronjob:stuck_import_jobs
 - cronjob:stuck_merge_jobs
@@ -114,7 +113,6 @@
 - storage_migrator
 - system_hook_push
 - update_merge_requests
-- update_user_activity
 - upload_checksum
 - web_hook
 - repository_update_remote_mirror

app/workers/schedule_update_user_activity_worker.rb

@@ -1,12 +0,0 @@
-# frozen_string_literal: true
-
-class ScheduleUpdateUserActivityWorker
-  include ApplicationWorker
-  include CronjobQueue
-
-  def perform(batch_size = 500)
-    Gitlab::UserActivities.new.each_slice(batch_size) do |batch|
-      UpdateUserActivityWorker.perform_async(Hash[batch])
-    end
-  end
-end

app/workers/update_user_activity_worker.rb

@@ -1,27 +0,0 @@
-# frozen_string_literal: true
-
-class UpdateUserActivityWorker
-  include ApplicationWorker
-
-  def perform(pairs)
-    pairs = cast_data(pairs)
-    ids = pairs.keys
-    conditions = 'WHEN id = ? THEN ? ' * ids.length
-
-    User.where(id: ids)
-      .update_all([
-        "last_activity_on = CASE #{conditions} ELSE last_activity_on END",
-        *pairs.to_a.flatten
-      ])
-
-    Gitlab::UserActivities.new.delete(*ids)
-  end
-
-  private
-
-  def cast_data(pairs)
-    pairs.each_with_object({}) do |(key, value), new_pairs|
-      new_pairs[key.to_i] = Time.at(value.to_i).to_s(:db)
-    end
-  end
-end

changelogs/unreleased/43312-remove_user_activity_workers.yml

@@ -0,0 +1,5 @@
+---
+title: Delete UserActivities and related workers
+merge_request: 20597
+author:
+type: performance

changelogs/unreleased/add-total-time-flat-printer-for-profiling.yml

@@ -0,0 +1,6 @@
+---
+title: Add a Gitlab::Profiler.print_by_total_time convenience method for profiling
+  from a Rails console
+merge_request:
+author:
+type: other

changelogs/unreleased/frozen-string-enable-apps-services-inner-even-more.yml

@@ -0,0 +1,5 @@
+---
+title: Enable even more frozen string in app/services/**/*.rb
+merge_request: 20702
+author: gfyoung
+type: performance

changelogs/unreleased/gitaly-ff-branch-nil.yml

@@ -0,0 +1,5 @@
+---
+title: Add missing Gitaly branch_update nil checks
+merge_request: 20711
+author:
+type: fixed

changelogs/unreleased/rails5-fix-revert-modal-spec.yml

@@ -0,0 +1,5 @@
+---
+title: Rails5 fix user sees revert modal spec
+merge_request: 20706
+author: Jasper Maes
+type: fixed

changelogs/unreleased/regen-2fa-codes.yml

@@ -0,0 +1,5 @@
+---
+title: Added button to regenerate 2FA codes
+merge_request:
+author: Luke Picciau
+type: added

config/initializers/1_settings.rb

@@ -319,10 +319,6 @@ Settings.cron_jobs['gitlab_usage_ping_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['gitlab_usage_ping_worker']['cron'] ||= Settings.__send__(:cron_for_usage_ping)
 Settings.cron_jobs['gitlab_usage_ping_worker']['job_class'] = 'GitlabUsagePingWorker'
 
-Settings.cron_jobs['schedule_update_user_activity_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['schedule_update_user_activity_worker']['cron'] ||= '30 0 * * *'
-Settings.cron_jobs['schedule_update_user_activity_worker']['job_class'] = 'ScheduleUpdateUserActivityWorker'
-
 Settings.cron_jobs['remove_old_web_hook_logs_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['remove_old_web_hook_logs_worker']['cron'] ||= '40 0 * * *'
 Settings.cron_jobs['remove_old_web_hook_logs_worker']['job_class'] = 'RemoveOldWebHookLogsWorker'

config/sidekiq_queues.yml

@@ -62,7 +62,6 @@
   - [default, 1]
   - [pages, 1]
   - [system_hook_push, 1]
-  - [update_user_activity, 1]
   - [propagate_service_template, 1]
   - [background_migration, 1]
   - [gcp_cluster, 1]
@@ -77,4 +76,3 @@
   - [repository_remove_remote, 1]
   - [create_note_diff_file, 1]
   - [delete_diff_files, 1]
- 

doc/api/projects.md

@@ -55,6 +55,8 @@ GET /projects
 | `with_custom_attributes` | boolean | no | Include [custom attributes](custom_attributes.md) in response (admins only) |
 | `with_issues_enabled` | boolean | no | Limit by enabled issues feature |
 | `with_merge_requests_enabled` | boolean | no | Limit by enabled merge requests feature |
+| `wiki_checksum_failed` | boolean | no | Limit projects where the wiki checksum calculation has failed _([Introduced][ee-6137] in [GitLab Premium][eep] 11.2)_ |
+| `repository_checksum_failed` | boolean | no | Limit projects where the repository checksum calculation has failed _([Introduced][ee-6137] in [GitLab Premium][eep] 11.2)_ |
 
 When `simple=true` or the user is unauthenticated this returns something like:
 
@@ -1509,3 +1511,6 @@ GET /projects/:id/snapshot
 | --------- | ---- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
 | `wiki`    | boolean | no | Whether to download the wiki, rather than project, repository |
+
+[eep]: https://about.gitlab.com/pricing/ "Available only in GitLab Premium"
+[ee-6137]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/6137

doc/development/profiling.md

@@ -42,6 +42,36 @@ Passing a `logger:` keyword argument to `Gitlab::Profiler.profile` will send
 ActiveRecord and ActionController log output to that logger. Further options are
 documented with the method source.
 
+There is also a RubyProf printer available:
+`Gitlab::Profiler::TotalTimeFlatPrinter`. This acts like
+`RubyProf::FlatPrinter`, but its `min_percent` option works on the method's
+total time, not its self time. (This is because we often spend most of our time
+in library code, but this comes from calls in our application.) It also offers a
+`max_percent` option to help filter out outer calls that aren't useful (like
+`ActionDispatch::Integration::Session#process`).
+
+There is a convenience method for using this,
+`Gitlab::Profiler.print_by_total_time`:
+
+```ruby
+result = Gitlab::Profiler.profile('/my-user')
+Gitlab::Profiler.print_by_total_time(result, max_percent: 60, min_percent: 2)
+# Measure Mode: wall_time
+# Thread ID: 70005223698240
+# Fiber ID: 70004894952580
+# Total: 1.768912
+# Sort by: total_time
+#
+#  %self      total      self      wait     child     calls  name
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::Helpers::RenderingHelper#render
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::Renderer#render_partial
+#   0.00      1.017     0.000     0.000     1.017       14  *ActionView::PartialRenderer#render
+#   0.00      1.007     0.000     0.000     1.007       14  *ActionView::PartialRenderer#render_partial
+#   0.00      0.930     0.000     0.000     0.930       14   Hamlit::TemplateHandler#call
+#   0.00      0.928     0.000     0.000     0.928       14   Temple::Engine#call
+#   0.02      0.865     0.000     0.000     0.864      638  *Enumerable#inject
+```
+
 [GitLab-Profiler](https://gitlab.com/gitlab-com/gitlab-profiler) is a project
 that builds on this to add some additional niceties, such as allowing
 configuration with a single Yaml file for multiple URLs, and uploading of the

lib/api/projects.rb

@@ -8,6 +8,21 @@ module API
 
     before { authenticate_non_get! }
 
+    helpers do
+      params :optional_filter_params_ee do
+        # EE::API::Projects would override this helper
+      end
+
+      # EE::API::Projects would override this method
+      def apply_filters(projects)
+        projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
+        projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
+        projects = projects.with_statistics if params[:statistics]
+
+        projects
+      end
+    end
+
     helpers do
       params :statistics_params do
         optional :statistics, type: Boolean, default: false, desc: 'Include project statistics'
@@ -39,6 +54,8 @@ module API
         optional :membership, type: Boolean, default: false, desc: 'Limit by projects that the current user is a member of'
         optional :with_issues_enabled, type: Boolean, default: false, desc: 'Limit by enabled issues feature'
         optional :with_merge_requests_enabled, type: Boolean, default: false, desc: 'Limit by enabled merge requests feature'
+
+        use :optional_filter_params_ee
       end
 
       params :create_params do
@@ -52,9 +69,7 @@ module API
 
       def present_projects(projects, options = {})
         projects = reorder_projects(projects)
-        projects = projects.with_issues_available_for_user(current_user) if params[:with_issues_enabled]
+        projects = apply_filters(projects)
-        projects = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled]
-        projects = projects.with_statistics if params[:statistics]
         projects = paginate(projects)
         projects, options = with_custom_attributes(projects, options)
 

lib/gitlab/git/operation_service.rb

@@ -8,6 +8,8 @@ module Gitlab
         alias_method :branch_created?, :branch_created
 
         def self.from_gitaly(branch_update)
+          return if branch_update.nil?
+
           new(
             branch_update.commit_id,
             branch_update.repo_created,

lib/gitlab/git_ref_validator.rb

@@ -7,11 +7,11 @@ module Gitlab
     #
     # Returns true for a valid reference name, false otherwise
     def validate(ref_name)
-      return false if ref_name.start_with?('refs/heads/')
+      not_allowed_prefixes = %w(refs/heads/ refs/remotes/ -)
-      return false if ref_name.start_with?('refs/remotes/')
+      return false if ref_name.start_with?(*not_allowed_prefixes)
+      return false if ref_name == 'HEAD'
 
-      Gitlab::Utils.system_silent(
+      Rugged::Reference.valid_name? "refs/heads/#{ref_name}"
-        %W(#{Gitlab.config.git.bin_path} check-ref-format --branch #{ref_name}))
     end
   end
 end

lib/gitlab/gitaly_client/operation_service.rb

@@ -144,13 +144,14 @@ module Gitlab
           branch: encode_binary(target_branch)
         )
 
-        branch_update = GitalyClient.call(
+        response = GitalyClient.call(
           @repository.storage,
           :operation_service,
           :user_ff_branch,
           request
-        ).branch_update
+        )
-        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(branch_update)
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       rescue GRPC::FailedPrecondition => e
         raise Gitlab::Git::CommitError, e
       end
@@ -306,9 +307,9 @@ module Gitlab
           raise Gitlab::Git::CommitError, response.commit_error
         elsif response.create_tree_error.presence
           raise Gitlab::Git::Repository::CreateTreeError, response.create_tree_error
-        else
-          Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
         end
+
+        Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)
       end
 
       def user_commit_files_request_header(

lib/gitlab/profiler.rb

@@ -146,5 +146,11 @@ module Gitlab
         logger.info("#{model} total (#{query_count}): #{time.round(2)}ms")
       end
     end
+
+    def self.print_by_total_time(result, options = {})
+      default_options = { sort_method: :total_time }
+
+      Gitlab::Profiler::TotalTimeFlatPrinter.new(result).print(STDOUT, default_options.merge(options))
+    end
   end
 end

lib/gitlab/profiler/total_time_flat_printer.rb

@@ -0,0 +1,39 @@
+module Gitlab
+  module Profiler
+    class TotalTimeFlatPrinter < RubyProf::FlatPrinter
+      def max_percent
+        @options[:max_percent] || 100
+      end
+
+      # Copied from:
+      #   <https://github.com/ruby-prof/ruby-prof/blob/master/lib/ruby-prof/printers/flat_printer.rb>
+      #
+      # The changes are just to filter by total time, not self time, and add a
+      # max_percent option as well.
+      def print_methods(thread)
+        total_time = thread.total_time
+        methods = thread.methods.sort_by(&sort_method).reverse
+
+        sum = 0
+        methods.each do |method|
+          total_percent = (method.total_time / total_time) * 100
+          next if total_percent < min_percent
+          next if total_percent > max_percent
+
+          sum += method.self_time
+
+          @output << "%6.2f  %9.3f %9.3f %9.3f %9.3f %8d  %s%s\n" % [
+            method.self_time / total_time * 100, # %self
+            method.total_time,                   # total
+            method.self_time,                    # self
+            method.wait_time,                    # wait
+            method.children_time,                # children
+            method.called,                       # calls
+            method.recursive? ? "*" : " ",       # cycle
+            method_name(method)                  # name
+          ]
+        end
+      end
+    end
+  end
+end

lib/gitlab/user_activities.rb

@@ -1,34 +0,0 @@
-module Gitlab
-  class UserActivities
-    include Enumerable
-
-    KEY = 'users:activities'.freeze
-    BATCH_SIZE = 500
-
-    def self.record(key, time = Time.now)
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.hset(KEY, key, time.to_i)
-      end
-    end
-
-    def delete(*keys)
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.hdel(KEY, keys)
-      end
-    end
-
-    def each
-      cursor = 0
-      loop do
-        cursor, pairs =
-          Gitlab::Redis::SharedState.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

scripts/lint-rugged

@@ -14,7 +14,10 @@ ALLOWED = [
   'lib/tasks/gitlab/cleanup.rake',
 
   # The only place where Rugged code is still allowed in production
-  'lib/gitlab/git/'
+  'lib/gitlab/git/',
+
+  # Needed to avoid using the git binary to validate a branch name
+  'lib/gitlab/git_ref_validator.rb'
 ].freeze
 
 rugged_lines = IO.popen(%w[git grep -i -n rugged -- app config lib], &:read).lines

spec/controllers/sessions_controller_spec.rb

@@ -50,8 +50,6 @@ describe SessionsController do
       end
 
       context 'when using valid password', :clean_gitlab_redis_shared_state do
-        include UserActivitiesHelpers
-
         let(:user) { create(:user) }
         let(:user_params) { { login: user.username, password: user.password } }
 
@@ -77,7 +75,7 @@ describe SessionsController do
         it 'updates the user activity' do
           expect do
             post(:create, user: user_params)
-          end.to change { user_activity(user) }
+          end.to change { user.reload.last_activity_on }.to(Date.today)
         end
       end
 

spec/features/user_sees_revert_modal_spec.rb

@@ -9,6 +9,9 @@ describe 'Merge request > User sees revert modal', :js do
     sign_in(user)
     visit(project_merge_request_path(project, merge_request))
     click_button('Merge')
+
+    wait_for_requests
+
     visit(merge_request_path(merge_request))
     click_link('Revert')
   end

spec/javascripts/helpers/vuex_action_helper.js

@@ -84,14 +84,12 @@ export default (
     done();
   };
 
-  return new Promise((resolve, reject) => {
+  const result = action({ commit, state, dispatch, rootState: state }, payload);
-    try {
+
-      const result = action({ commit, state, dispatch, rootState: state }, payload);
+  return new Promise(resolve => {
-      resolve(result);
+    setImmediate(resolve);
-    } catch (e) {
-      reject(e);
-    }
   })
+    .then(() => result)
     .catch(error => {
       validateResults();
       throw error;

spec/javascripts/helpers/vuex_action_helper_spec.js

@@ -138,4 +138,29 @@ describe('VueX test helper (testAction)', () => {
         });
     });
   });
+
+  it('should work with async actions not returning promises', done => {
+    const data = { FOO: 'BAR' };
+
+    const promiseAction = ({ commit, dispatch }) => {
+      dispatch('ACTION');
+
+      axios
+        .get(TEST_HOST)
+        .then(() => {
+          commit('SUCCESS');
+          return data;
+        })
+        .catch(error => {
+          commit('ERROR');
+          throw error;
+        });
+    };
+
+    mock.onGet(TEST_HOST).replyOnce(200, 42);
+
+    assertion = { mutations: [{ type: 'SUCCESS' }], actions: [{ type: 'ACTION' }] };
+
+    testAction(promiseAction, null, {}, assertion.mutations, assertion.actions, done);
+  });
 });

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

@@ -1,10 +1,10 @@
 require 'spec_helper'
 
 describe Gitlab::GitalyClient::OperationService do
-  let(:project) { create(:project) }
+  set(:project) { create(:project, :repository) }
   let(:repository) { project.repository.raw }
   let(:client) { described_class.new(repository) }
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
   let(:gitaly_user) { Gitlab::Git::User.from_gitlab(user).to_gitaly }
 
   describe '#user_create_branch' do
@@ -151,18 +151,104 @@ describe Gitlab::GitalyClient::OperationService do
     end
     let(:response) { Gitaly::UserFFBranchResponse.new(branch_update: branch_update) }
 
-    subject { client.user_ff_branch(user, source_sha, target_branch) }
+    before do
-
-    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
       expect_any_instance_of(Gitaly::OperationService::Stub)
         .to receive(:user_ff_branch).with(request, kind_of(Hash))
         .and_return(response)
+    end
 
+    subject { client.user_ff_branch(user, source_sha, target_branch) }
+
+    it 'sends a user_ff_branch message and returns a BranchUpdate object' do
       expect(subject).to be_a(Gitlab::Git::OperationService::BranchUpdate)
       expect(subject.newrev).to eq(source_sha)
       expect(subject.repo_created).to be(false)
       expect(subject.branch_created).to be(false)
     end
+
+    context 'when the response has no branch_update' do
+      let(:response) { Gitaly::UserFFBranchResponse.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  shared_examples 'cherry pick and revert errors' do
+    context 'when a pre_receive_error is present' do
+      let(:response) { response_class.new(pre_receive_error: "something failed") }
+
+      it 'raises a PreReceiveError' do
+        expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+      end
+    end
+
+    context 'when a commit_error is present' do
+      let(:response) { response_class.new(commit_error: "something failed") }
+
+      it 'raises a CommitError' do
+        expect { subject }.to raise_error(Gitlab::Git::CommitError, "something failed")
+      end
+    end
+
+    context 'when a create_tree_error is present' do
+      let(:response) { response_class.new(create_tree_error: "something failed") }
+
+      it 'raises a CreateTreeError' do
+        expect { subject }.to raise_error(Gitlab::Git::Repository::CreateTreeError, "something failed")
+      end
+    end
+
+    context 'when branch_update is nil' do
+      let(:response) { response_class.new }
+
+      it { expect(subject).to be_nil }
+    end
+  end
+
+  describe '#user_cherry_pick' do
+    let(:response_class) { Gitaly::UserCherryPickResponse }
+
+    subject do
+      client.user_cherry_pick(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Cherry-pick message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_cherry_pick).with(kind_of(Gitaly::UserCherryPickRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
+  end
+
+  describe '#user_revert' do
+    let(:response_class) { Gitaly::UserRevertResponse }
+
+    subject do
+      client.user_revert(
+        user: user,
+        commit: repository.commit,
+        branch_name: 'master',
+        message: 'Revert message',
+        start_branch_name: 'master',
+        start_repository: repository
+      )
+    end
+
+    before do
+      expect_any_instance_of(Gitaly::OperationService::Stub)
+        .to receive(:user_revert).with(kind_of(Gitaly::UserRevertRequest), kind_of(Hash))
+        .and_return(response)
+    end
+
+    it_behaves_like 'cherry pick and revert errors'
   end
 
   describe '#user_squash' do
@@ -203,7 +289,7 @@ describe Gitlab::GitalyClient::OperationService do
         Gitaly::UserSquashResponse.new(git_error: "something failed")
       end
 
-      it "throws a PreReceive exception" do
+      it "raises a GitError exception" do
         expect_any_instance_of(Gitaly::OperationService::Stub)
           .to receive(:user_squash).with(request, kind_of(Hash))
           .and_return(response)
@@ -212,5 +298,41 @@ describe Gitlab::GitalyClient::OperationService do
           Gitlab::Git::Repository::GitError, "something failed")
       end
     end
+
+    describe '#user_commit_files' do
+      subject do
+        client.user_commit_files(
+          gitaly_user, 'my-branch', 'Commit files message', [], 'janedoe@example.com', 'Jane Doe',
+          'master', repository)
+      end
+
+      before do
+        expect_any_instance_of(Gitaly::OperationService::Stub)
+          .to receive(:user_commit_files).with(kind_of(Enumerator), kind_of(Hash))
+          .and_return(response)
+      end
+
+      context 'when a pre_receive_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(pre_receive_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::PreReceiveError, "something failed")
+        end
+      end
+
+      context 'when an index_error is present' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new(index_error: "something failed") }
+
+        it 'raises a PreReceiveError' do
+          expect { subject }.to raise_error(Gitlab::Git::Index::IndexError, "something failed")
+        end
+      end
+
+      context 'when branch_update is nil' do
+        let(:response) { Gitaly::UserCommitFilesResponse.new }
+
+        it { expect(subject).to be_nil }
+      end
+    end
   end
 end

spec/lib/gitlab/user_activities_spec.rb

@@ -1,127 +0,0 @@
-require 'spec_helper'
-
-describe Gitlab::UserActivities, :clean_gitlab_redis_shared_state do
-  let(:now) { Time.now }
-
-  describe '.record' do
-    context 'with no time given' do
-      it 'uses Time.now and records an activity in SharedState' do
-        Timecop.freeze do
-          now # eager-load now
-          described_class.record(42)
-        end
-
-        Gitlab::Redis::SharedState.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 SharedState' do
-        described_class.record(42, now)
-
-        Gitlab::Redis::SharedState.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 SharedState' do
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.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::SharedState.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 SharedState' do
-          Gitlab::Redis::SharedState.with do |redis|
-            expect(redis.hscan(described_class::KEY, 0)).to eq(['0', []])
-          end
-
-          subject.delete(42)
-
-          Gitlab::Redis::SharedState.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 SharedState' do
-          described_class.record(41, now)
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.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::SharedState.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 SharedState' do
-          described_class.record(42, now)
-
-          Gitlab::Redis::SharedState.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::SharedState.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

spec/requests/api/internal_spec.rb

@@ -279,7 +279,7 @@ describe API::Internal do
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq('/')
           expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -291,7 +291,7 @@ describe API::Internal do
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq('/')
           expect(json_response["gl_repository"]).to eq("wiki-#{project.id}")
-          expect(user).to have_an_activity_record
+          expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
 
@@ -309,7 +309,7 @@ describe API::Internal do
           expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
           expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
           expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(user).to have_an_activity_record
+          expect(user.reload.last_activity_on).to eql(Date.today)
         end
       end
 
@@ -328,7 +328,7 @@ describe API::Internal do
             expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
             expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
             expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-            expect(user).not_to have_an_activity_record
+            expect(user.reload.last_activity_on).to be_nil
           end
         end
       end
@@ -345,7 +345,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -355,7 +355,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
     end
@@ -373,7 +373,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
 
@@ -383,7 +383,7 @@ describe API::Internal do
 
           expect(response).to have_gitlab_http_status(200)
           expect(json_response["status"]).to be_falsey
-          expect(user).not_to have_an_activity_record
+          expect(user.reload.last_activity_on).to be_nil
         end
       end
     end

spec/requests/git_http_spec.rb

@@ -5,7 +5,6 @@ describe 'Git HTTP requests' do
   include TermsHelper
   include GitHttpHelpers
   include WorkhorseHelpers
-  include UserActivitiesHelpers
 
   shared_examples 'pulls require Basic HTTP Authentication' do
     context "when no credentials are provided" do
@@ -440,10 +439,10 @@ describe 'Git HTTP requests' do
                 end
 
                 it 'updates the user last activity', :clean_gitlab_redis_shared_state do
-                  expect(user_activity(user)).to be_nil
+                  expect(user.last_activity_on).to be_nil
 
                   download(path, env) do |response|
-                    expect(user_activity(user)).to be_present
+                    expect(user.reload.last_activity_on).to eql(Date.today)
                   end
                 end
               end

spec/services/event_create_service_spec.rb

@@ -1,8 +1,6 @@
 require 'spec_helper'
 
 describe EventCreateService do
-  include UserActivitiesHelpers
-
   let(:service) { described_class.new }
 
   describe 'Issues' do
@@ -146,7 +144,7 @@ describe EventCreateService do
 
     it 'updates user last activity' do
       expect { service.push(project, user, push_data) }
-        .to change { user_activity(user) }
+        .to change {  user.last_activity_on }.to(Date.today)
     end
 
     it 'caches the last push event for the user' do

spec/services/users/activity_service_spec.rb

@@ -1,60 +1,61 @@
 require 'spec_helper'
 
 describe Users::ActivityService do
-  include UserActivitiesHelpers
+  include ExclusiveLeaseHelpers
 
-  let(:user) { create(:user) }
+  let(:user) { create(:user, last_activity_on: last_activity_on) }
 
-  subject(:service) { described_class.new(user, 'type') }
+  subject { described_class.new(user, 'type') }
 
   describe '#execute', :clean_gitlab_redis_shared_state do
     context 'when last activity is nil' do
-      before do
+      let(:last_activity_on) { nil }
-        service.execute
+
+      it 'updates last_activity_on for the user' do
+        expect { subject.execute }
+          .to change(user, :last_activity_on).from(last_activity_on).to(Date.today)
       end
+    end
 
-      it 'sets the last activity timestamp for the user' do
+    context 'when last activity is in the past' do
-        expect(last_hour_user_ids).to eq([user.id])
+      let(:last_activity_on) { Date.today - 1.week }
+
+      it 'updates last_activity_on for the user' do
+        expect { subject.execute }
+          .to change(user, :last_activity_on)
+                .from(last_activity_on)
+                .to(Date.today)
       end
+    end
 
-      it 'updates the same user' do
+    context 'when last activity is today' do
-        service.execute
+      let(:last_activity_on) { Date.today }
 
-        expect(last_hour_user_ids).to eq([user.id])
+      it 'does not update last_activity_on' do
-      end
+        expect { subject.execute }.not_to change(user, :last_activity_on)
-
-      it 'updates the timestamp of an existing user' do
-        Timecop.freeze(Date.tomorrow) do
-          expect { service.execute }.to change { user_activity(user) }.to(Time.now.to_i.to_s)
-        end
-      end
-
-      describe 'other user' do
-        it 'updates other user' do
-          other_user = create(:user)
-          described_class.new(other_user, 'type').execute
-
-          expect(last_hour_user_ids).to match_array([user.id, other_user.id])
-        end
       end
     end
 
     context 'when in GitLab read-only instance' do
+      let(:last_activity_on) { nil }
+
       before do
         allow(Gitlab::Database).to receive(:read_only?).and_return(true)
       end
 
-      it 'does not update last_activity_at' do
+      it 'does not update last_activity_on' do
-        service.execute
+        expect { subject.execute }.not_to change(user, :last_activity_on)
+      end
+    end
 
-        expect(last_hour_user_ids).to eq([])
+    context 'when a lease could not be obtained' do
+      let(:last_activity_on) { nil }
+
+      it 'does not update last_activity_on' do
+        stub_exclusive_lease_taken("acitvity_service:#{user.id}", timeout: 1.minute.to_i)
+
+        expect { subject.execute }.not_to change(user, :last_activity_on)
       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

spec/support/helpers/user_activities_helpers.rb

@@ -1,7 +0,0 @@
-module UserActivitiesHelpers
-  def user_activity(user)
-    Gitlab::UserActivities.new
-      .find { |k, _| k == user.id.to_s }&.
-      second
-  end
-end

spec/support/matchers/user_activity_matchers.rb

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

spec/workers/schedule_update_user_activity_worker_spec.rb

@@ -1,25 +0,0 @@
-require 'spec_helper'
-
-describe ScheduleUpdateUserActivityWorker, :clean_gitlab_redis_shared_state do
-  let(:now) { Time.now }
-
-  before do
-    Gitlab::UserActivities.record('1', now)
-    Gitlab::UserActivities.record('2', now)
-  end
-
-  it 'schedules UpdateUserActivityWorker once' do
-    expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s, '2' => now.to_i.to_s })
-
-    subject.perform
-  end
-
-  context 'when specifying a batch size' do
-    it 'schedules UpdateUserActivityWorker twice' do
-      expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '1' => now.to_i.to_s })
-      expect(UpdateUserActivityWorker).to receive(:perform_async).with({ '2' => now.to_i.to_s })
-
-      subject.perform(1)
-    end
-  end
-end

spec/workers/update_user_activity_worker_spec.rb

@@ -1,35 +0,0 @@
-require 'spec_helper'
-
-describe UpdateUserActivityWorker, :clean_gitlab_redis_shared_state do
-  let(:user_active_2_days_ago) { create(:user, current_sign_in_at: 10.months.ago) }
-  let(:user_active_yesterday_1) { create(:user) }
-  let(:user_active_yesterday_2) { create(:user) }
-  let(:user_active_today) { create(:user) }
-  let(:data) do
-    {
-      user_active_2_days_ago.id.to_s => 2.days.ago.at_midday.to_i.to_s,
-      user_active_yesterday_1.id.to_s => 1.day.ago.at_midday.to_i.to_s,
-      user_active_yesterday_2.id.to_s => 1.day.ago.at_midday.to_i.to_s,
-      user_active_today.id.to_s => Time.now.to_i.to_s
-    }
-  end
-
-  it 'updates users.last_activity_on' do
-    subject.perform(data)
-
-    aggregate_failures do
-      expect(user_active_2_days_ago.reload.last_activity_on).to eq(2.days.ago.to_date)
-      expect(user_active_yesterday_1.reload.last_activity_on).to eq(1.day.ago.to_date)
-      expect(user_active_yesterday_2.reload.last_activity_on).to eq(1.day.ago.to_date)
-      expect(user_active_today.reload.reload.last_activity_on).to eq(Date.today)
-    end
-  end
-
-  it 'deletes the pairs from SharedState' do
-    data.each { |id, time| Gitlab::UserActivities.record(id, time) }
-
-    subject.perform(data)
-
-    expect(Gitlab::UserActivities.new.to_a).to be_empty
-  end
-end