Merge branch 'fj-40053-error-500-members-list' into 'master'

Fix user membership destroy relation

Closes #40053

See merge request gitlab-org/gitlab-ce!15970

9 files changed, 164 insertions, 4 deletions

diff --git a/app/models/user.rb b/app/models/user.rb
index b52f17cd6a8..9d99a3f0c67 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -94,8 +94,8 @@ class User < ActiveRecord::Base
   has_one :user_synced_attributes_metadata, autosave: true
 
   # Groups
-  has_many :members, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-  has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, source: 'GroupMember' # rubocop:disable Cop/ActiveRecordDependent
+  has_many :members
+  has_many :group_members, -> { where(requested_at: nil) }, source: 'GroupMember'
   has_many :groups, through: :group_members
   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
@@ -103,7 +103,7 @@ class User < ActiveRecord::Base
   # Projects
   has_many :groups_projects,          through: :groups, 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 :created_projects,         foreign_key: :creator_id, class_name: 'Project'
   has_many :users_star_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index 8e20de8dfa5..00db8a2c434 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -31,6 +31,11 @@ module Users
         return user
       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|
         Groups::DestroyService.new(group, current_user).execute
       end
diff --git a/changelogs/unreleased/fj-40053-error-500-members-list.yml b/changelogs/unreleased/fj-40053-error-500-members-list.yml
new file mode 100644
index 00000000000..8c82950bd41
--- /dev/null
+++ b/changelogs/unreleased/fj-40053-error-500-members-list.yml
@@ -0,0 +1,5 @@
+---
+title: Fixing error 500 when member exist but not the user
+merge_request: 15970
+author:
+type: fixed
diff --git a/db/migrate/20171216111734_clean_up_for_members.rb b/db/migrate/20171216111734_clean_up_for_members.rb
new file mode 100644
index 00000000000..22e0997dce6
--- /dev/null
+++ b/db/migrate/20171216111734_clean_up_for_members.rb
@@ -0,0 +1,31 @@
+# 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
diff --git a/db/migrate/20171216112339_add_foreign_key_for_members.rb b/db/migrate/20171216112339_add_foreign_key_for_members.rb
new file mode 100644
index 00000000000..be17769be6a
--- /dev/null
+++ b/db/migrate/20171216112339_add_foreign_key_for_members.rb
@@ -0,0 +1,21 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 88885f706b7..42715d5e1e8 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1989,6 +1989,7 @@ ActiveRecord::Schema.define(version: 20171220191323) do
   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", "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_files", "merge_request_diffs", on_delete: :cascade
   add_foreign_key "merge_request_diffs", "merge_requests", name: "fk_8483f3258f", on_delete: :cascade
diff --git a/spec/migrations/clean_up_for_members_spec.rb b/spec/migrations/clean_up_for_members_spec.rb
new file mode 100644
index 00000000000..0258860d169
--- /dev/null
+++ b/spec/migrations/clean_up_for_members_spec.rb
@@ -0,0 +1,78 @@
+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
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 2557ce71f2b..047a46886c7 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -22,7 +22,9 @@ describe User do
   describe 'associations' do
     it { is_expected.to have_one(:namespace) }
     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(:keys).dependent(:destroy) }
     it { is_expected.to have_many(:deploy_keys).dependent(:destroy) }
diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb
index 58a5bede3de..aeba9cd60bc 100644
--- a/spec/services/users/destroy_service_spec.rb
+++ b/spec/services/users/destroy_service_spec.rb
@@ -188,5 +188,22 @@ describe Users::DestroyService do
         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