Commit 2665aea6 authored by Francisco Javier López's avatar Francisco Javier López Committed by Douwe Maan

Fix user membership destroy relation

parent 3f44c4ce
...@@ -94,8 +94,8 @@ class User < ActiveRecord::Base ...@@ -94,8 +94,8 @@ class User < ActiveRecord::Base
has_one :user_synced_attributes_metadata, autosave: true has_one :user_synced_attributes_metadata, autosave: true
# Groups # Groups
has_many :members, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :members
has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
has_many :groups, through: :group_members has_many :groups, through: :group_members
has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group has_many :owned_groups, -> { where members: { access_level: Gitlab::Access::OWNER } }, through: :group_members, source: :group
has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group has_many :masters_groups, -> { where members: { access_level: Gitlab::Access::MASTER } }, through: :group_members, source: :group
...@@ -103,7 +103,7 @@ class User < ActiveRecord::Base ...@@ -103,7 +103,7 @@ class User < ActiveRecord::Base
# Projects # Projects
has_many :groups_projects, through: :groups, source: :projects has_many :groups_projects, through: :groups, source: :projects
has_many :personal_projects, through: :namespace, source: :projects has_many :personal_projects, through: :namespace, source: :projects
has_many :project_members, -> { where(requested_at: nil) }, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :project_members, -> { where(requested_at: nil) }
has_many :projects, through: :project_members has_many :projects, through: :project_members
has_many :created_projects, foreign_key: :creator_id, class_name: 'Project' has_many :created_projects, foreign_key: :creator_id, class_name: 'Project'
has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
......
...@@ -31,6 +31,11 @@ module Users ...@@ -31,6 +31,11 @@ module Users
return user return user
end end
# Calling all before/after_destroy hooks for the user because
# there is no dependent: destroy in the relationship. And the removal
# is done by a foreign_key. Otherwise they won't be called
user.members.find_each { |member| member.run_callbacks(:destroy) }
user.solo_owned_groups.each do |group| user.solo_owned_groups.each do |group|
Groups::DestroyService.new(group, current_user).execute Groups::DestroyService.new(group, current_user).execute
end end
......
---
title: Fixing error 500 when member exist but not the user
merge_request: 15970
author:
type: fixed
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class CleanUpForMembers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
class Member < ActiveRecord::Base
include EachBatch
self.table_name = 'members'
end
def up
condition = <<~EOF.squish
invite_token IS NULL AND
NOT EXISTS (SELECT 1 FROM users WHERE users.id = members.user_id)
EOF
Member.each_batch(of: 10_000) do |batch|
batch.where(condition).delete_all
end
end
def down
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddForeignKeyForMembers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_foreign_key(:members,
:users,
column: :user_id)
end
def down
remove_foreign_key(:members, column: :user_id)
end
end
...@@ -1989,6 +1989,7 @@ ActiveRecord::Schema.define(version: 20171220191323) do ...@@ -1989,6 +1989,7 @@ ActiveRecord::Schema.define(version: 20171220191323) do
add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade
add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade
add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade
add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade
add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade add_foreign_key "merge_request_diff_commits", "merge_request_diffs", on_delete: :cascade
add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade add_foreign_key "merge_request_diff_files", "merge_request_diffs", on_delete: :cascade
add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
......
require 'spec_helper'
require Rails.root.join('db', 'migrate', '20171216111734_clean_up_for_members.rb')
describe CleanUpForMembers, :migration do
let(:migration) { described_class.new }
let!(:group_member) { create_group_member }
let!(:unbinded_group_member) { create_group_member }
let!(:invited_group_member) { create_group_member(true) }
let!(:not_valid_group_member) { create_group_member }
let!(:project_member) { create_project_member }
let!(:invited_project_member) { create_project_member(true) }
let!(:unbinded_project_member) { create_project_member }
let!(:not_valid_project_member) { create_project_member }
it 'removes members without proper user_id' do
unbinded_group_member.update_column(:user_id, nil)
not_valid_group_member.update_column(:user_id, 9999)
unbinded_project_member.update_column(:user_id, nil)
not_valid_project_member.update_column(:user_id, 9999)
migrate!
expect(Member.all).not_to include(unbinded_group_member, not_valid_group_member, unbinded_project_member, not_valid_project_member)
expect(Member.all).to include(group_member, invited_group_member, project_member, invited_project_member)
end
def create_group_member(invited = false)
fill_member(GroupMember.new(group: create_group), invited)
end
def create_project_member(invited = false)
fill_member(ProjectMember.new(project: create_project), invited)
end
def fill_member(member_object, invited)
member_object.tap do |m|
m.access_level = 40
m.notification_level = 3
if invited
m.user_id = nil
m.invite_token = 'xxx'
m.invite_email = 'email@email.com'
else
m.user_id = create_user.id
end
m.save
end
member_object
end
def create_group
name = FFaker::Lorem.characters(10)
Group.create(name: name, path: name.downcase.gsub(/\s/, '_'))
end
def create_project
name = FFaker::Lorem.characters(10)
creator = create_user
Project.create(name: name,
path: name.downcase.gsub(/\s/, '_'),
namespace: creator.namespace,
creator: creator)
end
def create_user
User.create(email: FFaker::Internet.email,
password: '12345678',
name: FFaker::Name.name,
username: FFaker::Internet.user_name,
confirmed_at: Time.now,
confirmation_token: nil)
end
end
...@@ -22,7 +22,9 @@ describe User do ...@@ -22,7 +22,9 @@ describe User do
describe 'associations' do describe 'associations' do
it { is_expected.to have_one(:namespace) } it { is_expected.to have_one(:namespace) }
it { is_expected.to have_many(:snippets).dependent(:destroy) } it { is_expected.to have_many(:snippets).dependent(:destroy) }
it { is_expected.to have_many(:project_members).dependent(:destroy) } it { is_expected.to have_many(:members) }
it { is_expected.to have_many(:project_members) }
it { is_expected.to have_many(:group_members) }
it { is_expected.to have_many(:groups) } it { is_expected.to have_many(:groups) }
it { is_expected.to have_many(:keys).dependent(:destroy) } it { is_expected.to have_many(:keys).dependent(:destroy) }
it { is_expected.to have_many(:deploy_keys).dependent(:destroy) } it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
......
...@@ -188,5 +188,22 @@ describe Users::DestroyService do ...@@ -188,5 +188,22 @@ describe Users::DestroyService do
end end
end end
end end
describe "calls the before/after callbacks" do
it 'of project_members' do
expect_any_instance_of(ProjectMember).to receive(:run_callbacks).with(:destroy).once
service.execute(user)
end
it 'of group_members' do
group_member = create(:group_member)
group_member.group.group_members.create(user: user, access_level: 40)
expect_any_instance_of(GroupMember).to receive(:run_callbacks).with(:destroy).once
service.execute(user)
end
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment