diff options
| -rw-r--r-- | app/models/group.rb | 2 | ||||
| -rw-r--r-- | app/models/member.rb | 8 | ||||
| -rw-r--r-- | app/models/user.rb | 13 | ||||
| -rw-r--r-- | app/services/user_project_access_changed_service.rb | 34 | ||||
| -rw-r--r-- | app/workers/authorized_projects_worker.rb | 14 | ||||
| -rw-r--r-- | changelogs/unreleased/refresh-authorizations-without-sidekiq.yml | 4 | ||||
| -rw-r--r-- | config/sidekiq_queues.yml | 1 | ||||
| -rw-r--r-- | spec/models/members/group_member_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/members/project_member_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 4 | ||||
| -rw-r--r-- | spec/services/user_project_access_changed_service_spec.rb | 17 | ||||
| -rw-r--r-- | spec/workers/authorized_projects_worker_spec.rb | 23 |
12 files changed, 74 insertions, 50 deletions
diff --git a/app/models/group.rb b/app/models/group.rb index 99675ddb366..7df188036e1 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -197,7 +197,7 @@ class Group < Namespace end def refresh_members_authorized_projects - UserProjectAccessChangedService.new(users_with_parents.pluck(:id)).execute + UserProjectAccessChangedService.new(users_with_parents).execute end def members_with_parents diff --git a/app/models/member.rb b/app/models/member.rb index c585e0b450e..c25a326ad9c 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -147,7 +147,7 @@ class Member < ActiveRecord::Base member.save end - UserProjectAccessChangedService.new(user.id).execute if user.is_a?(User) + UserProjectAccessChangedService.new(user).execute if user.is_a?(User) member end @@ -275,12 +275,12 @@ class Member < ActiveRecord::Base end def post_create_hook - UserProjectAccessChangedService.new(user.id).execute + UserProjectAccessChangedService.new(user).execute system_hook_service.execute_hooks_for(self, :create) end def post_update_hook - UserProjectAccessChangedService.new(user.id).execute if access_level_changed? + UserProjectAccessChangedService.new(user).execute if access_level_changed? end def post_destroy_hook @@ -294,7 +294,7 @@ class Member < ActiveRecord::Base # member is destroyed through association return if destroyed_by_association.present? - UserProjectAccessChangedService.new(user_id).execute + UserProjectAccessChangedService.new(user).execute end def after_accept_invite diff --git a/app/models/user.rb b/app/models/user.rb index 06dd98a3188..216d8bf5226 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -453,12 +453,21 @@ class User < ActiveRecord::Base end end + # Returns the authorized projects of a user, optionally restricted to a + # certain access level. + # + # Permissions are refreshed automatically whenever necessary. def authorized_projects(min_access_level = nil) refresh_authorized_projects unless authorized_projects_populated - # We're overriding an association, so explicitly call super with no arguments or it would be passed as `force_reload` to the association + # We're overriding an association, so explicitly call super with no + # arguments or it would be passed as `force_reload` to the association projects = super() - projects = projects.where('project_authorizations.access_level >= ?', min_access_level) if min_access_level + + if min_access_level + projects = projects. + where('project_authorizations.access_level >= ?', min_access_level) + end projects end diff --git a/app/services/user_project_access_changed_service.rb b/app/services/user_project_access_changed_service.rb index 2469b4f0d7c..03d0fe143ce 100644 --- a/app/services/user_project_access_changed_service.rb +++ b/app/services/user_project_access_changed_service.rb @@ -1,9 +1,37 @@ +# Service that updates users so their authorizations are refreshed the next time +# they are requested. class UserProjectAccessChangedService - def initialize(user_ids) - @user_ids = Array.wrap(user_ids) + def initialize(users) + @users = Array.wrap(users) end def execute - AuthorizedProjectsWorker.bulk_perform_async(@user_ids.map { |id| [id] }) + @users.length == 1 ? refresh_single : refresh_multiple + end + + # In most cases when a single user instance is passed it's probably going to + # be re-used afterwards (especially in the case of tests). Because of this we + # refresh the object right away in these cases, instead of doing so the next + # time authorizations are requested. + def refresh_single + @users[0].refresh_authorized_projects + end + + # When refreshing multiple users we don't want to spend time doing this in the + # same request as doing so could take quite a lot of time. Instead we defer + # refreshing until the next time the authorizations are requested. This is + # based on the observation that multiple user objects are rarely (if ever) + # used directly after using this service class. + def refresh_multiple + user_ids = @users.map do |user| + # Ensure that existing instances refresh their data. + user.authorized_projects_populated = nil + user.id + end + + # Ensure that any new instances are refreshed as well. This is necessary + # because some of the objects passed to this service might not be used + # afterwards. + User.where(id: user_ids).update_all(authorized_projects_populated: nil) end end diff --git a/app/workers/authorized_projects_worker.rb b/app/workers/authorized_projects_worker.rb deleted file mode 100644 index 2badd0680fb..00000000000 --- a/app/workers/authorized_projects_worker.rb +++ /dev/null @@ -1,14 +0,0 @@ -class AuthorizedProjectsWorker - include Sidekiq::Worker - include DedicatedSidekiqQueue - - def self.bulk_perform_async(args_list) - Sidekiq::Client.push_bulk('class' => self, 'args' => args_list) - end - - def perform(user_id) - user = User.find_by(id: user_id) - - user.refresh_authorized_projects if user - end -end diff --git a/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml b/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml new file mode 100644 index 00000000000..c69a576a9a6 --- /dev/null +++ b/changelogs/unreleased/refresh-authorizations-without-sidekiq.yml @@ -0,0 +1,4 @@ +--- +title: Stop using Sidekiq for authorized projects +merge_request: +author: diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 022b0e80917..33c5dfac489 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -36,7 +36,6 @@ - [clear_database_cache, 1] - [delete_user, 1] - [delete_merged_branches, 1] - - [authorized_projects, 1] - [expire_build_instance_artifacts, 1] - [group_destroy, 1] - [irker, 1] diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 370aeb9e0a9..bf6c7591ac2 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -61,7 +61,7 @@ describe GroupMember, models: true do describe '#after_accept_request' do it 'calls NotificationService.accept_group_access_request' do - member = create(:group_member, user: build_stubbed(:user), requested_at: Time.now) + member = create(:group_member, user: create(:user), requested_at: Time.now) expect_any_instance_of(NotificationService).to receive(:new_group_member) diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 68f72f5c86e..d24d713edab 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -150,7 +150,7 @@ describe ProjectMember, models: true do describe 'notifications' do describe '#after_accept_request' do it 'calls NotificationService.new_project_member' do - member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now) + member = create(:project_member, user: create(:user), requested_at: Time.now) expect_any_instance_of(NotificationService).to receive(:new_project_member) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 8b20ee81614..d8a1c5c81d2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1144,9 +1144,13 @@ describe User, models: true do user = create(:user) member = group.add_developer(user) + user.reload + expect(user.authorized_projects).to include(project) member.destroy + user.reload + expect(user.authorized_projects).not_to include(project) end diff --git a/spec/services/user_project_access_changed_service_spec.rb b/spec/services/user_project_access_changed_service_spec.rb new file mode 100644 index 00000000000..6bc6b4ccfe3 --- /dev/null +++ b/spec/services/user_project_access_changed_service_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe UserProjectAccessChangedService do + describe '#execute' do + it 'schedules refreshing of user permissions' do + user1 = create(:user, authorized_projects_populated: true) + user2 = create(:user, authorized_projects_populated: true) + + described_class.new([user1, user2]).execute + + [user1, user2].each do |user| + user.reload + expect(user.authorized_projects_populated).to eq(nil) + end + end + end +end diff --git a/spec/workers/authorized_projects_worker_spec.rb b/spec/workers/authorized_projects_worker_spec.rb deleted file mode 100644 index b6591f272f6..00000000000 --- a/spec/workers/authorized_projects_worker_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -describe AuthorizedProjectsWorker do - let(:worker) { described_class.new } - - describe '#perform' do - it "refreshes user's authorized projects" do - user = create(:user) - - expect_any_instance_of(User).to receive(:refresh_authorized_projects) - - worker.perform(user.id) - end - - context "when the user is not found" do - it "does nothing" do - expect_any_instance_of(User).not_to receive(:refresh_authorized_projects) - - described_class.new.perform(-1) - end - end - end -end |