summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <robert@gitlab.com>2018-03-08 00:23:39 +0000
committerRobert Speicher <robert@gitlab.com>2018-03-08 00:23:39 +0000
commitf8e06b50eea2aecaf1f37fb7228292e8516e2613 (patch)
tree8ae0bf52afcad2f79eee8db15899cab7d08ec7c6
parentbeb1f43816b465b9cff05a91634403a637b4e097 (diff)
parent74a24a4fab3d8020950ee3da371a432f7361c569 (diff)
downloadgitlab-ce-f8e06b50eea2aecaf1f37fb7228292e8516e2613.tar.gz
Merge branch 'unassign-when-leaving' into 'master'
Don't delete todos or unassign issues and MRs when a user leaves a project Closes #43899 See merge request gitlab-org/gitlab-ce!17615
-rw-r--r--app/models/member.rb5
-rw-r--r--app/models/members/project_member.rb6
-rw-r--r--app/models/user.rb36
-rw-r--r--app/services/members/destroy_service.rb40
-rw-r--r--changelogs/unreleased/unassign-when-leaving.yml5
-rw-r--r--spec/models/members/project_member_spec.rb23
-rw-r--r--spec/services/members/destroy_service_spec.rb96
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