diff options
| -rw-r--r-- | app/models/member.rb | 5 | ||||
| -rw-r--r-- | app/models/members/project_member.rb | 6 | ||||
| -rw-r--r-- | app/models/user.rb | 36 | ||||
| -rw-r--r-- | app/services/members/destroy_service.rb | 40 | ||||
| -rw-r--r-- | changelogs/unreleased/unassign-when-leaving.yml | 5 | ||||
| -rw-r--r-- | spec/models/members/project_member_spec.rb | 23 | ||||
| -rw-r--r-- | spec/services/members/destroy_service_spec.rb | 96 |
7 files changed, 95 insertions, 116 deletions
diff --git a/app/models/member.rb b/app/models/member.rb index 408e8b2d704..36090676051 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -85,6 +85,7 @@ class Member < ActiveRecord::Base after_create :create_notification_setting, unless: [:pending?, :importing?] after_create :post_create_hook, unless: [:pending?, :importing?] after_update :post_update_hook, unless: [:pending?, :importing?] + after_destroy :destroy_notification_setting after_destroy :post_destroy_hook, unless: :pending? after_commit :refresh_member_authorized_projects @@ -315,6 +316,10 @@ class Member < ActiveRecord::Base user.notification_settings.find_or_create_for(source) end + def destroy_notification_setting + notification_setting&.destroy + end + def notification_setting @notification_setting ||= user&.notification_settings_for(source) end diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index b6f1dd272cd..1c7ed4a96df 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -13,8 +13,6 @@ class ProjectMember < Member scope :in_project, ->(project) { where(source_id: project.id) } - before_destroy :delete_member_todos - class << self # Add users to projects with passed access option # @@ -93,10 +91,6 @@ class ProjectMember < Member private - def delete_member_todos - user.todos.where(project_id: source_id).destroy_all if user - end - def send_invite notification_service.invite_project_member(self, @raw_invite_token) diff --git a/app/models/user.rb b/app/models/user.rb index 9c60adf0c90..951b161203c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1035,14 +1035,33 @@ class User < ActiveRecord::Base end end + def todos_done_count(force: false) + Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: 20.minutes) do + TodosFinder.new(self, state: :done).execute.count + end + end + + def todos_pending_count(force: false) + Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: 20.minutes) do + TodosFinder.new(self, state: :pending).execute.count + end + end + def update_cache_counts assigned_open_merge_requests_count(force: true) assigned_open_issues_count(force: true) end + def update_todos_count_cache + todos_done_count(force: true) + todos_pending_count(force: true) + end + def invalidate_cache_counts invalidate_issue_cache_counts invalidate_merge_request_cache_counts + invalidate_todos_done_count + invalidate_todos_pending_count end def invalidate_issue_cache_counts @@ -1053,21 +1072,12 @@ class User < ActiveRecord::Base Rails.cache.delete(['users', id, 'assigned_open_merge_requests_count']) end - def todos_done_count(force: false) - Rails.cache.fetch(['users', id, 'todos_done_count'], force: force, expires_in: 20.minutes) do - TodosFinder.new(self, state: :done).execute.count - end + def invalidate_todos_done_count + Rails.cache.delete(['users', id, 'todos_done_count']) end - def todos_pending_count(force: false) - Rails.cache.fetch(['users', id, 'todos_pending_count'], force: force, expires_in: 20.minutes) do - TodosFinder.new(self, state: :pending).execute.count - end - end - - def update_todos_count_cache - todos_done_count(force: true) - todos_pending_count(force: true) + def invalidate_todos_pending_count + Rails.cache.delete(['users', id, 'todos_pending_count']) end # This is copied from Devise::Models::Lockable#valid_for_authentication?, as our auth diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index b141bfd5fbc..5b51e1982f1 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -5,12 +5,9 @@ module Members return member if member.is_a?(GroupMember) && member.source.last_owner?(member.user) - Member.transaction do - unassign_issues_and_merge_requests(member) unless member.invite? - member.notification_setting&.destroy + member.destroy - member.destroy - end + member.user&.invalidate_cache_counts if member.request? && member.user != current_user notification_service.decline_access_request(member) @@ -37,38 +34,5 @@ module Members raise "Unknown member type: #{member}!" end end - - def unassign_issues_and_merge_requests(member) - if member.is_a?(GroupMember) - issues = Issue.unscoped.select(1) - .joins(:project) - .where('issues.id = issue_assignees.issue_id AND projects.namespace_id = ?', member.source_id) - - # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...) - IssueAssignee.unscoped - .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues) - .delete_all - - MergeRequestsFinder.new(current_user, group_id: member.source_id, assignee_id: member.user_id) - .execute - .update_all(assignee_id: nil) - else - project = member.source - - # SELECT 1 FROM issues WHERE issues.id = issue_assignees.issue_id AND issues.project_id = X - issues = Issue.unscoped.select(1) - .where('issues.id = issue_assignees.issue_id') - .where(project_id: project.id) - - # DELETE FROM issue_assignees WHERE user_id = X AND EXISTS (...) - IssueAssignee.unscoped - .where('user_id = :user_id AND EXISTS (:sub)', user_id: member.user_id, sub: issues) - .delete_all - - project.merge_requests.opened.assigned_to(member.user).update_all(assignee_id: nil) - end - - member.user.invalidate_cache_counts - end end end diff --git a/changelogs/unreleased/unassign-when-leaving.yml b/changelogs/unreleased/unassign-when-leaving.yml new file mode 100644 index 00000000000..c00a87b1749 --- /dev/null +++ b/changelogs/unreleased/unassign-when-leaving.yml @@ -0,0 +1,5 @@ +--- +title: Don't delete todos or unassign issues and MRs when a user leaves a project +merge_request: +author: +type: fixed diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 3e46fa36375..b8b0e63f92e 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -45,14 +45,6 @@ describe ProjectMember do let(:project) { owner.project } let(:master) { create(:project_member, project: project) } - let(:owner_todos) { (0...2).map { create(:todo, user: owner.user, project: project) } } - let(:master_todos) { (0...3).map { create(:todo, user: master.user, project: project) } } - - before do - owner_todos - master_todos - end - it "creates an expired event when left due to expiry" do expired = create(:project_member, project: project, expires_at: Time.now - 6.days) expired.destroy @@ -63,21 +55,6 @@ describe ProjectMember do master.destroy expect(Event.recent.first.action).to eq(Event::LEFT) end - - it "destroys itself and delete associated todos" do - expect(owner.user.todos.size).to eq(2) - expect(master.user.todos.size).to eq(3) - expect(Todo.count).to eq(5) - - master_todo_ids = master_todos.map(&:id) - master.destroy - - expect(owner.user.todos.size).to eq(2) - expect(Todo.count).to eq(2) - master_todo_ids.each do |id| - expect(Todo.exists?(id)).to eq(false) - end - end end describe '.import_team' do diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 10c264a90c5..36b6e5a701e 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -19,32 +19,11 @@ describe Members::DestroyService do end end - def number_of_assigned_issuables(user) - Issue.assigned_to(user).count + MergeRequest.assigned_to(user).count - end - shared_examples 'a service destroying a member' do it 'destroys the member' do expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1) end - it 'unassigns issues and merge requests' do - if member.invite? - expect { described_class.new(current_user).execute(member, opts) } - .not_to change { number_of_assigned_issuables(member_user) } - else - create :issue, assignees: [member_user] - issue = create :issue, project: group_project, assignees: [member_user] - merge_request = create :merge_request, target_project: group_project, source_project: group_project, assignee: member_user - - expect { described_class.new(current_user).execute(member, opts) } - .to change { number_of_assigned_issuables(member_user) }.from(3).to(1) - - expect(issue.reload.assignee_ids).to be_empty - expect(merge_request.reload.assignee_id).to be_nil - end - end - it 'destroys member notification_settings' do if member_user.notification_settings.any? expect { described_class.new(current_user).execute(member, opts) } @@ -56,6 +35,29 @@ describe Members::DestroyService do end end + shared_examples 'a service destroying a member with access' do + it_behaves_like 'a service destroying a member' + + it 'invalidates cached counts for todos and assigned issues and merge requests', :aggregate_failures do + create(:issue, project: group_project, assignees: [member_user]) + create(:merge_request, source_project: group_project, assignee: member_user) + create(:todo, :pending, project: group_project, user: member_user) + create(:todo, :done, project: group_project, user: member_user) + + expect(member_user.assigned_open_merge_requests_count).to be(1) + expect(member_user.assigned_open_issues_count).to be(1) + expect(member_user.todos_pending_count).to be(1) + expect(member_user.todos_done_count).to be(1) + + described_class.new(current_user).execute(member, opts) + + expect(member_user.assigned_open_merge_requests_count).to be(0) + expect(member_user.assigned_open_issues_count).to be(0) + expect(member_user.todos_pending_count).to be(0) + expect(member_user.todos_done_count).to be(0) + end + end + shared_examples 'a service destroying an access requester' do it_behaves_like 'a service destroying a member' @@ -74,29 +76,39 @@ describe Members::DestroyService do end end - context 'with a member' do + context 'with a member with access' do before do - group_project.add_developer(member_user) - group.add_developer(member_user) + group_project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE) + group.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE) end context 'when current user cannot destroy the given member' do - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + context 'with a project member' do let(:member) { group_project.members.find_by(user_id: member_user.id) } - end - it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } - let(:member) { group_project.members.find_by(user_id: member_user.id) } - end + before do + group_project.add_developer(member_user) + end - it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do - let(:member) { group.members.find_by(user_id: member_user.id) } + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' + + it_behaves_like 'a service destroying a member with access' do + let(:opts) { { skip_authorization: true } } + end end - it_behaves_like 'a service destroying a member' do - let(:opts) { { skip_authorization: true } } + context 'with a group member' do let(:member) { group.members.find_by(user_id: member_user.id) } + + before do + group.add_developer(member_user) + end + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' + + it_behaves_like 'a service destroying a member with access' do + let(:opts) { { skip_authorization: true } } + end end end @@ -106,12 +118,24 @@ describe Members::DestroyService do group.add_owner(current_user) end - it_behaves_like 'a service destroying a member' do + context 'with a project member' do let(:member) { group_project.members.find_by(user_id: member_user.id) } + + before do + group_project.add_developer(member_user) + end + + it_behaves_like 'a service destroying a member with access' end - it_behaves_like 'a service destroying a member' do + context 'with a group member' do let(:member) { group.members.find_by(user_id: member_user.id) } + + before do + group.add_developer(member_user) + end + + it_behaves_like 'a service destroying a member with access' end end end |