diff options
author | Stan Hu <stanhu@gmail.com> | 2018-07-20 08:24:37 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-07-20 08:24:37 +0300 |
commit | 81b5611efb40821783eb000f312f91ccce3bcadf (patch) | |
tree | 6cc247db2dc0bdf391f6b8a3524fbe903d2df1d3 | |
parent | 2b9ec17e040863eb37255fa89f03fabdcd113cce (diff) | |
parent | b60364c0f377f4afdf5e96e84aba11988f4ae526 (diff) |
Merge branch 'master' into sh-support-bitbucket-server-import
71 files changed, 442 insertions, 361 deletions
diff --git a/app/models/user.rb b/app/models/user.rb index 4987d01aac6..58429f8d607 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/prometheus/adapter_service.rb b/app/services/prometheus/adapter_service.rb index 4504d2ccfe6..cbba79690c5 100644 --- a/app/services/prometheus/adapter_service.rb +++ b/app/services/prometheus/adapter_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Prometheus class AdapterService def initialize(project, deployment_platform = nil) diff --git a/app/services/protected_branches/access_level_params.rb b/app/services/protected_branches/access_level_params.rb index 4658b0e850d..a7ef573ff0b 100644 --- a/app/services/protected_branches/access_level_params.rb +++ b/app/services/protected_branches/access_level_params.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedBranches class AccessLevelParams attr_reader :type, :params diff --git a/app/services/protected_branches/api_service.rb b/app/services/protected_branches/api_service.rb index 4b40200644b..4340d3e8260 100644 --- a/app/services/protected_branches/api_service.rb +++ b/app/services/protected_branches/api_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedBranches class ApiService < BaseService def create diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 9d947f73af1..87aaf4672a4 100644 --- a/app/services/protected_branches/create_service.rb +++ b/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) diff --git a/app/services/protected_branches/destroy_service.rb b/app/services/protected_branches/destroy_service.rb index 8172c896e76..7190bc2001b 100644 --- a/app/services/protected_branches/destroy_service.rb +++ b/app/services/protected_branches/destroy_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedBranches class DestroyService < BaseService def execute(protected_branch) diff --git a/app/services/protected_branches/legacy_api_create_service.rb b/app/services/protected_branches/legacy_api_create_service.rb index bb7656489c5..aef99a860a0 100644 --- a/app/services/protected_branches/legacy_api_create_service.rb +++ b/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 diff --git a/app/services/protected_branches/legacy_api_update_service.rb b/app/services/protected_branches/legacy_api_update_service.rb index 1df38de0e4a..1f6bbe72f85 100644 --- a/app/services/protected_branches/legacy_api_update_service.rb +++ b/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 diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb index 95e46645374..4d7d498b8ca 100644 --- a/app/services/protected_branches/update_service.rb +++ b/app/services/protected_branches/update_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedBranches class UpdateService < BaseService def execute(protected_branch) diff --git a/app/services/protected_tags/create_service.rb b/app/services/protected_tags/create_service.rb index faba7865a17..9aff55986b2 100644 --- a/app/services/protected_tags/create_service.rb +++ b/app/services/protected_tags/create_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedTags class CreateService < BaseService attr_reader :protected_tag diff --git a/app/services/protected_tags/destroy_service.rb b/app/services/protected_tags/destroy_service.rb index c868d7ad8e6..dc4a1b39848 100644 --- a/app/services/protected_tags/destroy_service.rb +++ b/app/services/protected_tags/destroy_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedTags class DestroyService < BaseService def execute(protected_tag) diff --git a/app/services/protected_tags/update_service.rb b/app/services/protected_tags/update_service.rb index aea6a48968d..3eb5f4955ee 100644 --- a/app/services/protected_tags/update_service.rb +++ b/app/services/protected_tags/update_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ProtectedTags class UpdateService < BaseService def execute(protected_tag) diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 9ac8fdb4cff..cdc8514c47c 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module QuickActions class InterpretService < BaseService include Gitlab::QuickActions::Dsl diff --git a/app/services/search/global_service.rb b/app/services/search/global_service.rb index 92a32e703af..cb1bf0a03a5 100644 --- a/app/services/search/global_service.rb +++ b/app/services/search/global_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Search class GlobalService attr_accessor :current_user, :params diff --git a/app/services/search/group_service.rb b/app/services/search/group_service.rb index b4efba68715..34803d005e3 100644 --- a/app/services/search/group_service.rb +++ b/app/services/search/group_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Search class GroupService < Search::GlobalService attr_accessor :group diff --git a/app/services/search/project_service.rb b/app/services/search/project_service.rb index 9a22abae635..f223c8be103 100644 --- a/app/services/search/project_service.rb +++ b/app/services/search/project_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Search class ProjectService attr_accessor :project, :current_user, :params diff --git a/app/services/search/snippet_service.rb b/app/services/search/snippet_service.rb index 85da0be6fff..e899a36f468 100644 --- a/app/services/search/snippet_service.rb +++ b/app/services/search/snippet_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Search class SnippetService attr_accessor :current_user, :params diff --git a/app/services/tags/create_service.rb b/app/services/tags/create_service.rb index 3cc88d77ba1..329722df747 100644 --- a/app/services/tags/create_service.rb +++ b/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) diff --git a/app/services/tags/destroy_service.rb b/app/services/tags/destroy_service.rb index b84b061e460..800268485a4 100644 --- a/app/services/tags/destroy_service.rb +++ b/app/services/tags/destroy_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Tags class DestroyService < BaseService def execute(tag_name) diff --git a/app/services/test_hooks/base_service.rb b/app/services/test_hooks/base_service.rb index aadc1ea644b..8b5439c00bf 100644 --- a/app/services/test_hooks/base_service.rb +++ b/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 diff --git a/app/services/test_hooks/project_service.rb b/app/services/test_hooks/project_service.rb index 65183e84cce..45e0e61e5c4 100644 --- a/app/services/test_hooks/project_service.rb +++ b/app/services/test_hooks/project_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module TestHooks class ProjectService < TestHooks::BaseService attr_writer :project diff --git a/app/services/test_hooks/system_service.rb b/app/services/test_hooks/system_service.rb index 9016c77b7f0..082830c5538 100644 --- a/app/services/test_hooks/system_service.rb +++ b/app/services/test_hooks/system_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module TestHooks class SystemService < TestHooks::BaseService private diff --git a/app/services/users/activity_service.rb b/app/services/users/activity_service.rb index 5803404c3c8..822df6c646a 100644 --- a/app/services/users/activity_service.rb +++ b/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? + + lease = Gitlab::ExclusiveLease.new("acitvity_service:#{@user.id}", + timeout: LEASE_TIMEOUT) + return unless lease.try_obtain - Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username})") + @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 diff --git a/app/services/users/build_service.rb b/app/services/users/build_service.rb index 4fb6d221909..c69b46cab5a 100644 --- a/app/services/users/build_service.rb +++ b/app/services/users/build_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Users class BuildService < BaseService def initialize(current_user, params = {}) diff --git a/app/services/users/create_service.rb b/app/services/users/create_service.rb index c8a3c461d60..2ac6dfd90fa 100644 --- a/app/services/users/create_service.rb +++ b/app/services/users/create_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Users class CreateService < BaseService include NewUserNotifier diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 06b604dad4d..4bc78b5b64e 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Users class DestroyService attr_accessor :current_user diff --git a/app/services/users/last_push_event_service.rb b/app/services/users/last_push_event_service.rb index 57e446d7f30..a9c9497520b 100644 --- a/app/services/users/last_push_event_service.rb +++ b/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 diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb index a2833b1e051..4d47078bf43 100644 --- a/app/services/users/migrate_to_ghost_user_service.rb +++ b/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. diff --git a/app/services/users/refresh_authorized_projects_service.rb b/app/services/users/refresh_authorized_projects_service.rb index f028f5eb0d4..23b63aaabdf 100644 --- a/app/services/users/refresh_authorized_projects_service.rb +++ b/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. # diff --git a/app/services/users/respond_to_terms_service.rb b/app/services/users/respond_to_terms_service.rb index 06d660186cf..9efa3b285a8 100644 --- a/app/services/users/respond_to_terms_service.rb +++ b/app/services/users/respond_to_terms_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Users class RespondToTermsService def initialize(user, term) diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 15ca1a55a5b..6dadb5a4eac 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Users class UpdateService < BaseService include NewUserNotifier diff --git a/app/services/wiki_pages/base_service.rb b/app/services/wiki_pages/base_service.rb index 260c04a8b94..e259f5bd1bc 100644 --- a/app/services/wiki_pages/base_service.rb +++ b/app/services/wiki_pages/base_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module WikiPages class BaseService < ::BaseService private diff --git a/app/services/wiki_pages/create_service.rb b/app/services/wiki_pages/create_service.rb index 24a817c06c9..2e2e0fd9033 100644 --- a/app/services/wiki_pages/create_service.rb +++ b/app/services/wiki_pages/create_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module WikiPages class CreateService < WikiPages::BaseService def execute diff --git a/app/services/wiki_pages/destroy_service.rb b/app/services/wiki_pages/destroy_service.rb index 6b93fb2f6d7..3f9343339cd 100644 --- a/app/services/wiki_pages/destroy_service.rb +++ b/app/services/wiki_pages/destroy_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module WikiPages class DestroyService < WikiPages::BaseService def execute(page) diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb index 93cbd9a509f..2159dd91e9c 100644 --- a/app/services/wiki_pages/update_service.rb +++ b/app/services/wiki_pages/update_service.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module WikiPages class UpdateService < WikiPages::BaseService def execute(page) diff --git a/app/views/profiles/two_factor_auths/show.html.haml b/app/views/profiles/two_factor_auths/show.html.haml index e35ebdea435..6950e2e332d 100644 --- a/app/views/profiles/two_factor_auths/show.html.haml +++ b/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. diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index d4be1ccfcfa..4de35b9bd06 100644 --- a/app/workers/all_queues.yml +++ b/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 diff --git a/app/workers/schedule_update_user_activity_worker.rb b/app/workers/schedule_update_user_activity_worker.rb deleted file mode 100644 index ff42fb8f0e5..00000000000 --- a/app/workers/schedule_update_user_activity_worker.rb +++ /dev/null @@ -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 diff --git a/app/workers/update_user_activity_worker.rb b/app/workers/update_user_activity_worker.rb deleted file mode 100644 index 15f01a70337..00000000000 --- a/app/workers/update_user_activity_worker.rb +++ /dev/null @@ -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 diff --git a/changelogs/unreleased/43312-remove_user_activity_workers.yml b/changelogs/unreleased/43312-remove_user_activity_workers.yml new file mode 100644 index 00000000000..6dfd018e350 --- /dev/null +++ b/changelogs/unreleased/43312-remove_user_activity_workers.yml @@ -0,0 +1,5 @@ +--- +title: Delete UserActivities and related workers +merge_request: 20597 +author: +type: performance diff --git a/changelogs/unreleased/add-total-time-flat-printer-for-profiling.yml b/changelogs/unreleased/add-total-time-flat-printer-for-profiling.yml new file mode 100644 index 00000000000..37a4e31896e --- /dev/null +++ b/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 diff --git a/changelogs/unreleased/frozen-string-enable-apps-services-inner-even-more.yml b/changelogs/unreleased/frozen-string-enable-apps-services-inner-even-more.yml new file mode 100644 index 00000000000..cee790a07ff --- /dev/null +++ b/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 diff --git a/changelogs/unreleased/gitaly-ff-branch-nil.yml b/changelogs/unreleased/gitaly-ff-branch-nil.yml new file mode 100644 index 00000000000..e7e689e6053 --- /dev/null +++ b/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 diff --git a/changelogs/unreleased/rails5-fix-revert-modal-spec.yml b/changelogs/unreleased/rails5-fix-revert-modal-spec.yml new file mode 100644 index 00000000000..0637e503ca9 --- /dev/null +++ b/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 diff --git a/changelogs/unreleased/regen-2fa-codes.yml b/changelogs/unreleased/regen-2fa-codes.yml new file mode 100644 index 00000000000..596f759df0f --- /dev/null +++ b/changelogs/unreleased/regen-2fa-codes.yml @@ -0,0 +1,5 @@ +--- +title: Added button to regenerate 2FA codes +merge_request: +author: Luke Picciau +type: added diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 44bc72a7185..c3122827a6b 100644 --- a/config/initializers/1_settings.rb +++ b/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' diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 3400142db36..70b584ff9e9 100644 --- a/config/sidekiq_queues.yml +++ b/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] - diff --git a/doc/api/projects.md b/doc/api/projects.md index a35c2a56992..f3ccca46420 100644 --- a/doc/api/projects.md +++ b/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 diff --git a/doc/development/profiling.md b/doc/development/profiling.md index 11878b4009b..0ca8bb67a77 100644 --- a/doc/development/profiling.md +++ b/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 diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 0888e3befac..889e3d4f819 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -9,6 +9,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' end @@ -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 = projects.with_merge_requests_enabled if params[:with_merge_requests_enabled] - projects = projects.with_statistics if params[:statistics] + projects = apply_filters(projects) projects = paginate(projects) projects, options = with_custom_attributes(projects, options) diff --git a/lib/gitlab/git/operation_service.rb b/lib/gitlab/git/operation_service.rb index 280def182d5..57d748343be 100644 --- a/lib/gitlab/git/operation_service.rb +++ b/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, diff --git a/lib/gitlab/git_ref_validator.rb b/lib/gitlab/git_ref_validator.rb index 2e3e4fc3f1f..40636fb204e 100644 --- a/lib/gitlab/git_ref_validator.rb +++ b/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/') - return false if ref_name.start_with?('refs/remotes/') + not_allowed_prefixes = %w(refs/heads/ refs/remotes/ -) + return false if ref_name.start_with?(*not_allowed_prefixes) + return false if ref_name == 'HEAD' - Gitlab::Utils.system_silent( - %W(#{Gitlab.config.git.bin_path} check-ref-format --branch #{ref_name})) + Rugged::Reference.valid_name? "refs/heads/#{ref_name}" end end end diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index 555733d1834..54c78fdb680 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/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( diff --git a/lib/gitlab/profiler.rb b/lib/gitlab/profiler.rb index ecff6ab5d5e..c5bb4648572 100644 --- a/lib/gitlab/profiler.rb +++ b/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 diff --git a/lib/gitlab/profiler/total_time_flat_printer.rb b/lib/gitlab/profiler/total_time_flat_printer.rb new file mode 100644 index 00000000000..2fd0ec10ba8 --- /dev/null +++ b/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 diff --git a/lib/gitlab/user_activities.rb b/lib/gitlab/user_activities.rb deleted file mode 100644 index 125488536e1..00000000000 --- a/lib/gitlab/user_activities.rb +++ /dev/null @@ -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 diff --git a/scripts/lint-rugged b/scripts/lint-rugged index cabd083e9f9..d0c2c544c47 100755 --- a/scripts/lint-rugged +++ b/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 diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 7c00652317b..8e25b61e2f1 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/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 diff --git a/spec/features/user_sees_revert_modal_spec.rb b/spec/features/user_sees_revert_modal_spec.rb index 11a9e470f76..3b48ea4786d 100644 --- a/spec/features/user_sees_revert_modal_spec.rb +++ b/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 diff --git a/spec/javascripts/helpers/vuex_action_helper.js b/spec/javascripts/helpers/vuex_action_helper.js index 4ca7015184e..dd9174194a1 100644 --- a/spec/javascripts/helpers/vuex_action_helper.js +++ b/spec/javascripts/helpers/vuex_action_helper.js @@ -84,14 +84,12 @@ export default ( done(); }; - return new Promise((resolve, reject) => { - try { - const result = action({ commit, state, dispatch, rootState: state }, payload); - resolve(result); - } catch (e) { - reject(e); - } + const result = action({ commit, state, dispatch, rootState: state }, payload); + + return new Promise(resolve => { + setImmediate(resolve); }) + .then(() => result) .catch(error => { validateResults(); throw error; diff --git a/spec/javascripts/helpers/vuex_action_helper_spec.js b/spec/javascripts/helpers/vuex_action_helper_spec.js index 8d6ad6750c0..09f0bd395c3 100644 --- a/spec/javascripts/helpers/vuex_action_helper_spec.js +++ b/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); + }); }); diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index 031d1e87dc1..eaf64e3c9b4 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/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) } - - it 'sends a user_ff_branch message and returns a BranchUpdate object' do + before 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 diff --git a/spec/lib/gitlab/user_activities_spec.rb b/spec/lib/gitlab/user_activities_spec.rb deleted file mode 100644 index 6bce2ee13cf..00000000000 --- a/spec/lib/gitlab/user_activities_spec.rb +++ /dev/null @@ -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 diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index a56b913198c..a2cfa706f58 100644 --- a/spec/requests/api/internal_spec.rb +++ b/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 diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index b030d9862c6..0f3e7157e14 100644 --- a/spec/requests/git_http_spec.rb +++ b/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 diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 13395a7cac3..68e310b0506 100644 --- a/spec/services/event_create_service_spec.rb +++ b/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 diff --git a/spec/services/users/activity_service_spec.rb b/spec/services/users/activity_service_spec.rb index 17eabad73be..f20849e6924 100644 --- a/spec/services/users/activity_service_spec.rb +++ b/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 - service.execute - end + let(:last_activity_on) { nil } - it 'sets the last activity timestamp for the user' do - expect(last_hour_user_ids).to eq([user.id]) + 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 - service.execute + context 'when last activity is in the past' do + let(:last_activity_on) { Date.today - 1.week } - expect(last_hour_user_ids).to eq([user.id]) - end - - 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 + 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 - describe 'other user' do - it 'updates other user' do - other_user = create(:user) - described_class.new(other_user, 'type').execute + context 'when last activity is today' do + let(:last_activity_on) { Date.today } - expect(last_hour_user_ids).to match_array([user.id, other_user.id]) - end + it 'does not update last_activity_on' do + expect { subject.execute }.not_to change(user, :last_activity_on) 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 - service.execute - - expect(last_hour_user_ids).to eq([]) + it 'does not update last_activity_on' do + 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 } + 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 end diff --git a/spec/support/helpers/user_activities_helpers.rb b/spec/support/helpers/user_activities_helpers.rb deleted file mode 100644 index 44feb104644..00000000000 --- a/spec/support/helpers/user_activities_helpers.rb +++ /dev/null @@ -1,7 +0,0 @@ -module UserActivitiesHelpers - def user_activity(user) - Gitlab::UserActivities.new - .find { |k, _| k == user.id.to_s }&. - second - end -end diff --git a/spec/support/matchers/user_activity_matchers.rb b/spec/support/matchers/user_activity_matchers.rb deleted file mode 100644 index ce3b683b6d2..00000000000 --- a/spec/support/matchers/user_activity_matchers.rb +++ /dev/null @@ -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 diff --git a/spec/workers/schedule_update_user_activity_worker_spec.rb b/spec/workers/schedule_update_user_activity_worker_spec.rb deleted file mode 100644 index 32c59381b01..00000000000 --- a/spec/workers/schedule_update_user_activity_worker_spec.rb +++ /dev/null @@ -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 diff --git a/spec/workers/update_user_activity_worker_spec.rb b/spec/workers/update_user_activity_worker_spec.rb deleted file mode 100644 index 268ca1d81f2..00000000000 --- a/spec/workers/update_user_activity_worker_spec.rb +++ /dev/null @@ -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 |