Handle CRM objects when moving groups

Changelog: fixed

app/models/customer_relations/contact.rb

@@ -23,7 +23,7 @@ class CustomerRelations::Contact < ApplicationRecord
   validates :last_name, presence: true, length: { maximum: 255 }
   validates :email, length: { maximum: 255 }
   validates :description, length: { maximum: 1024 }
-  validates :email, uniqueness: { scope: :group_id }
+  validates :email, uniqueness: { case_sensitive: false, scope: :group_id }
   validate :validate_email_format
   validate :validate_root_group
 
@@ -42,7 +42,7 @@ class CustomerRelations::Contact < ApplicationRecord
   def self.find_ids_by_emails(group, emails)
     raise ArgumentError, "Cannot lookup more than #{MAX_PLUCK} emails" if emails.length > MAX_PLUCK
 
-    where(group: group, email: emails).pluck(:id)
+    where(group: group).where('lower(email) in (?)', emails.map(&:downcase)).pluck(:id)
   end
 
   def self.exists_for_group?(group)
@@ -51,6 +51,34 @@ class CustomerRelations::Contact < ApplicationRecord
     exists?(group: group)
   end
 
+  def self.move_to_root_group(group)
+    update_query = <<~SQL
+      UPDATE #{CustomerRelations::IssueContact.table_name}
+      SET contact_id = new_contacts.id
+      FROM #{table_name} AS existing_contacts
+      JOIN #{table_name} AS new_contacts ON new_contacts.group_id = :old_group_id AND LOWER(new_contacts.email) = LOWER(existing_contacts.email)
+      WHERE existing_contacts.group_id = :new_group_id AND contact_id = existing_contacts.id
+    SQL
+    connection.execute(sanitize_sql([
+      update_query,
+      old_group_id: group.root_ancestor.id,
+      new_group_id: group.id
+      ]))
+
+    dupes_query = <<~SQL
+      DELETE FROM #{table_name} AS existing_contacts
+      USING #{table_name} AS new_contacts
+      WHERE existing_contacts.group_id = :new_group_id AND new_contacts.group_id = :old_group_id AND LOWER(new_contacts.email) = LOWER(existing_contacts.email)
+    SQL
+    connection.execute(sanitize_sql([
+      dupes_query,
+      old_group_id: group.root_ancestor.id,
+      new_group_id: group.id
+      ]))
+
+    where(group: group).update_all(group_id: group.root_ancestor.id)
+  end
+
   private
 
   def validate_email_format

app/models/customer_relations/issue_contact.rb

@@ -8,6 +8,8 @@ class CustomerRelations::IssueContact < ApplicationRecord
 
   validate :contact_belongs_to_root_group
 
+  BATCH_DELETE_SIZE = 1_000
+
   def self.find_contact_ids_by_emails(issue_id, emails)
     raise ArgumentError, "Cannot lookup more than #{MAX_PLUCK} emails" if emails.length > MAX_PLUCK
 
@@ -17,9 +19,17 @@ class CustomerRelations::IssueContact < ApplicationRecord
   end
 
   def self.delete_for_project(project_id)
-    joins(:issue)
-      .where(issues: { project_id: project_id })
-      .delete_all
+    loop do
+      deleted_records = joins(:issue).where(issues: { project_id: project_id }).limit(BATCH_DELETE_SIZE).delete_all
+      break if deleted_records == 0
+    end
+  end
+
+  def self.delete_for_group(group)
+    loop do
+      deleted_records = joins(issue: :project).where(projects: { namespace: group.self_and_descendants }).limit(BATCH_DELETE_SIZE).delete_all
+      break if deleted_records == 0
+    end
   end
 
   private

app/models/customer_relations/organization.rb

@@ -26,6 +26,34 @@ class CustomerRelations::Organization < ApplicationRecord
     .where('LOWER(name) = LOWER(?)', name)
   end
 
+  def self.move_to_root_group(group)
+    update_query = <<~SQL
+      UPDATE #{CustomerRelations::Contact.table_name}
+      SET organization_id = new_organizations.id
+      FROM #{table_name} AS existing_organizations
+      JOIN #{table_name} AS new_organizations ON new_organizations.group_id = :old_group_id AND LOWER(new_organizations.name) = LOWER(existing_organizations.name)
+      WHERE existing_organizations.group_id = :new_group_id AND organization_id = existing_organizations.id
+    SQL
+    connection.execute(sanitize_sql([
+      update_query,
+      old_group_id: group.root_ancestor.id,
+      new_group_id: group.id
+      ]))
+
+    dupes_query = <<~SQL
+      DELETE FROM #{table_name} AS existing_organizations
+      USING #{table_name} AS new_organizations
+      WHERE existing_organizations.group_id = :new_group_id AND new_organizations.group_id = :old_group_id AND LOWER(new_organizations.name) = LOWER(existing_organizations.name)
+    SQL
+    connection.execute(sanitize_sql([
+      dupes_query,
+      old_group_id: group.root_ancestor.id,
+      new_group_id: group.id
+      ]))
+
+    where(group: group).update_all(group_id: group.root_ancestor.id)
+  end
+
   private
 
   def validate_root_group

app/models/group.rb

@@ -61,8 +61,9 @@ class Group < Namespace
   has_many :boards
   has_many :badges, class_name: 'GroupBadge'
 
-  has_many :organizations, class_name: 'CustomerRelations::Organization', inverse_of: :group
-  has_many :contacts, class_name: 'CustomerRelations::Contact', inverse_of: :group
+  # AR defaults to nullify when trying to delete via has_many associations unless we set dependent: :delete_all
+  has_many :organizations, class_name: 'CustomerRelations::Organization', inverse_of: :group, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
+  has_many :contacts, class_name: 'CustomerRelations::Contact', inverse_of: :group, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :cluster_groups, class_name: 'Clusters::Group'
   has_many :clusters, through: :cluster_groups, class_name: 'Clusters::Cluster'

app/services/groups/transfer_service.rb

@@ -25,10 +25,15 @@ module Groups
     private
 
     def proceed_to_transfer
+      old_root_ancestor_id = @group.root_ancestor.id
+      was_root_group = @group.root?
+
       Group.transaction do
         update_group_attributes
         ensure_ownership
         update_integrations
+        remove_issue_contacts(old_root_ancestor_id, was_root_group)
+        update_crm_objects(was_root_group)
       end
 
       post_update_hooks(@updated_project_ids)
@@ -53,6 +58,17 @@ module Groups
       raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images?
       raise_transfer_error(:cannot_transfer_to_subgroup) if transfer_to_subgroup?
       raise_transfer_error(:group_contains_npm_packages) if group_with_npm_packages?
+      raise_transfer_error(:no_permissions_to_migrate_crm) if no_permissions_to_migrate_crm?
+    end
+
+    def no_permissions_to_migrate_crm?
+      return false unless group && @new_parent_group
+      return false if group.root_ancestor == @new_parent_group.root_ancestor
+
+      return true if group.contacts.exists? && !current_user.can?(:admin_crm_contact, @new_parent_group.root_ancestor)
+      return true if group.organizations.exists? && !current_user.can?(:admin_crm_organization, @new_parent_group.root_ancestor)
+
+      false
     end
 
     def group_with_npm_packages?
@@ -202,7 +218,8 @@ module Groups
         invalid_policies: s_("TransferGroup|You don't have enough permissions."),
         group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.'),
         cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.'),
-        group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
+        group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.'),
+        no_permissions_to_migrate_crm: s_("TransferGroup|Group contains contacts/organizations and you don't have enough permissions to move them to the new root group.")
       }.freeze
     end
 
@@ -238,6 +255,20 @@ module Groups
         namespace_id: group.id
       }
     end
+
+    def update_crm_objects(was_root_group)
+      return unless was_root_group
+
+      CustomerRelations::Contact.move_to_root_group(group)
+      CustomerRelations::Organization.move_to_root_group(group)
+    end
+
+    def remove_issue_contacts(old_root_ancestor_id, was_root_group)
+      return if was_root_group
+      return if old_root_ancestor_id == @group.root_ancestor.id
+
+      CustomerRelations::IssueContact.delete_for_group(@group)
+    end
   end
 end
 

db/fixtures/development/32_crm.rb

@@ -29,7 +29,7 @@ class Gitlab::Seeder::Crm
         group_id: group.id,
         first_name: first_name,
         last_name: last_name,
-        email: "#{first_name}.#{last_name}@example.org",
+        email: "#{first_name}.#{last_name}-#{index}@example.org",
         organization_id: organization_id
       )
 

db/post_migrate/20220316112118_update_organizations_name_index_add_id.rb

+# frozen_string_literal: true
+
+class UpdateOrganizationsNameIndexAddId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  OLD_INDEX = 'index_customer_relations_organizations_on_unique_name_per_group'
+  NEW_INDEX = 'index_organizations_on_unique_name_per_group'
+
+  def up
+    add_concurrent_index :customer_relations_organizations, 'group_id, lower(name), id', name: NEW_INDEX, unique: true
+
+    remove_concurrent_index_by_name :customer_relations_organizations, OLD_INDEX
+  end
+
+  def down
+    add_concurrent_index :customer_relations_organizations, 'group_id, lower(name)', name: OLD_INDEX, unique: true
+
+    remove_concurrent_index_by_name :customer_relations_organizations, NEW_INDEX
+  end
+end

db/post_migrate/20220316112206_add_contacts_index_on_group_email_and_id.rb

+# frozen_string_literal: true
+
+class AddContactsIndexOnGroupEmailAndId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_customer_relations_contacts_on_unique_email_per_group'
+
+  def up
+    add_concurrent_index :customer_relations_contacts, 'group_id, lower(email), id', name: INDEX_NAME, unique: true
+  end
+
+  def down
+    remove_concurrent_index_by_name :customer_relations_contacts, INDEX_NAME
+  end
+end

db/schema_migrations/20220316112118

+659accb8efe0223028a74346ecf3aa5b649cda825ac7941bc932bc1d7e6f8d9a
\ No newline at end of file

db/schema_migrations/20220316112206

+d24c5a5414e44385a132e8f342cb67cc5a7c5fe4bfcc4c15c473397076350bc2
\ No newline at end of file

db/structure.sql

@@ -27273,7 +27273,7 @@ CREATE INDEX index_customer_relations_contacts_on_group_id ON customer_relations
 
 CREATE INDEX index_customer_relations_contacts_on_organization_id ON customer_relations_contacts USING btree (organization_id);
 
-CREATE UNIQUE INDEX index_customer_relations_organizations_on_unique_name_per_group ON customer_relations_organizations USING btree (group_id, lower(name));
+CREATE UNIQUE INDEX index_customer_relations_contacts_on_unique_email_per_group ON customer_relations_contacts USING btree (group_id, lower(email), id);
 
 CREATE UNIQUE INDEX index_cycle_analytics_stage_event_hashes_on_hash_sha_256 ON analytics_cycle_analytics_stage_event_hashes USING btree (hash_sha256);
 
@@ -28393,6 +28393,8 @@ CREATE UNIQUE INDEX index_ops_feature_flags_issues_on_feature_flag_id_and_issue_
 
 CREATE UNIQUE INDEX index_ops_strategies_user_lists_on_strategy_id_and_user_list_id ON operations_strategies_user_lists USING btree (strategy_id, user_list_id);
 
+CREATE UNIQUE INDEX index_organizations_on_unique_name_per_group ON customer_relations_organizations USING btree (group_id, lower(name), id);
+
 CREATE INDEX index_packages_build_infos_on_pipeline_id ON packages_build_infos USING btree (pipeline_id);
 
 CREATE INDEX index_packages_build_infos_package_id_pipeline_id ON packages_build_infos USING btree (package_id, pipeline_id);

locale/gitlab.pot

@@ -39243,6 +39243,9 @@ msgstr ""
 msgid "TransferGroup|Database is not supported."
 msgstr ""
 
+msgid "TransferGroup|Group contains contacts/organizations and you don't have enough permissions to move them to the new root group."
+msgstr ""
+
 msgid "TransferGroup|Group contains projects with NPM packages."
 msgstr ""
 

spec/models/customer_relations/contact_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe CustomerRelations::Contact, type: :model do
     it { is_expected.to validate_length_of(:email).is_at_most(255) }
     it { is_expected.to validate_length_of(:description).is_at_most(1024) }
 
-    it { is_expected.to validate_uniqueness_of(:email).scoped_to(:group_id) }
+    it { is_expected.to validate_uniqueness_of(:email).case_insensitive.scoped_to(:group_id) }
 
     it_behaves_like 'an object with RFC3696 compliant email-formatted attributes', :email
   end
@@ -87,6 +87,15 @@ RSpec.describe CustomerRelations::Contact, type: :model do
       too_many_emails = described_class::MAX_PLUCK + 1
       expect { described_class.find_ids_by_emails(group, Array(0..too_many_emails)) }.to raise_error(ArgumentError)
     end
+
+    it 'finds contacts regardless of email casing' do
+      new_contact = create(:contact, group: group, email: "UPPERCASE@example.com")
+      emails = [group_contacts[0].email.downcase, group_contacts[1].email.upcase, new_contact.email]
+
+      contact_ids = described_class.find_ids_by_emails(group, emails)
+
+      expect(contact_ids).to contain_exactly(group_contacts[0].id, group_contacts[1].id, new_contact.id)
+    end
   end
 
   describe '#self.exists_for_group?' do
@@ -104,4 +113,33 @@ RSpec.describe CustomerRelations::Contact, type: :model do
       end
     end
   end
+
+  describe '#self.move_to_root_group' do
+    let!(:old_root_group) { create(:group) }
+    let!(:contacts) { create_list(:contact, 4, group: old_root_group) }
+    let!(:project) { create(:project, group: old_root_group) }
+    let!(:issue) { create(:issue, project: project) }
+    let!(:issue_contact1) { create(:issue_customer_relations_contact, issue: issue, contact: contacts[0]) }
+    let!(:issue_contact2) { create(:issue_customer_relations_contact, issue: issue, contact: contacts[1]) }
+    let!(:new_root_group) { create(:group) }
+    let!(:dupe_contact1) { create(:contact, group: new_root_group, email: contacts[1].email) }
+    let!(:dupe_contact2) { create(:contact, group: new_root_group, email: contacts[3].email.upcase) }
+
+    before do
+      old_root_group.update!(parent: new_root_group)
+      CustomerRelations::Contact.move_to_root_group(old_root_group)
+    end
+
+    it 'moves contacts with unique emails and deletes the rest' do
+      expect(contacts[0].reload.group_id).to eq(new_root_group.id)
+      expect(contacts[2].reload.group_id).to eq(new_root_group.id)
+      expect { contacts[1].reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { contacts[3].reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'updates issue_contact.contact_id for dupes and leaves the rest untouched' do
+      expect(issue_contact1.reload.contact_id).to eq(contacts[0].id)
+      expect(issue_contact2.reload.contact_id).to eq(dupe_contact1.id)
+    end
+  end
 end

spec/models/customer_relations/issue_contact_spec.rb

@@ -92,4 +92,16 @@ RSpec.describe CustomerRelations::IssueContact do
       expect { described_class.delete_for_project(project.id) }.to change { described_class.count }.by(-3)
     end
   end
+
+  describe '.delete_for_group' do
+    let(:project_for_root_group) { create(:project, group: group) }
+
+    it 'destroys all issue_contacts for projects in group and subgroups' do
+      create_list(:issue_customer_relations_contact, 2, :for_issue, issue: create(:issue, project: project))
+      create_list(:issue_customer_relations_contact, 2, :for_issue, issue: create(:issue, project: project_for_root_group))
+      create(:issue_customer_relations_contact)
+
+      expect { described_class.delete_for_group(group) }.to change { described_class.count }.by(-4)
+    end
+  end
 end

spec/models/customer_relations/organization_spec.rb

@@ -50,4 +50,32 @@ RSpec.describe CustomerRelations::Organization, type: :model do
       expect(described_class.find_by_name(group.id, 'TEST')).to eq([organiztion1])
     end
   end
+
+  describe '#self.move_to_root_group' do
+    let!(:old_root_group) { create(:group) }
+    let!(:organizations) { create_list(:organization, 4, group: old_root_group) }
+    let!(:new_root_group) { create(:group) }
+    let!(:contact1) { create(:contact, group: new_root_group, organization: organizations[0]) }
+    let!(:contact2) { create(:contact, group: new_root_group, organization: organizations[1]) }
+
+    let!(:dupe_organization1) { create(:organization, group: new_root_group, name: organizations[1].name) }
+    let!(:dupe_organization2) { create(:organization, group: new_root_group, name: organizations[3].name.upcase) }
+
+    before do
+      old_root_group.update!(parent: new_root_group)
+      CustomerRelations::Organization.move_to_root_group(old_root_group)
+    end
+
+    it 'moves organizations with unique names and deletes the rest' do
+      expect(organizations[0].reload.group_id).to eq(new_root_group.id)
+      expect(organizations[2].reload.group_id).to eq(new_root_group.id)
+      expect { organizations[1].reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect { organizations[3].reload }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+
+    it 'updates contact.organization_id for dupes and leaves the rest untouched' do
+      expect(contact1.reload.organization_id).to eq(organizations[0].id)
+      expect(contact2.reload.organization_id).to eq(dupe_organization1.id)
+    end
+  end
 end

spec/services/groups/transfer_service_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
   end
 
   let_it_be(:user) { create(:user) }
-  let_it_be(:new_parent_group) { create(:group, :public) }
+  let_it_be(:new_parent_group) { create(:group, :public, :crm_enabled) }
 
   let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
   let(:transfer_service) { described_class.new(group, user) }
@@ -876,5 +876,108 @@ RSpec.describe Groups::TransferService, :sidekiq_inline do
         end
       end
     end
+
+    context 'crm' do
+      let(:root_group) { create(:group, :public) }
+      let(:subgroup) { create(:group, :public, parent: root_group) }
+      let(:another_subgroup) { create(:group, :public, parent: root_group) }
+      let(:subsubgroup) { create(:group, :public, parent: subgroup) }
+
+      let(:root_project) { create(:project, group: root_group) }
+      let(:sub_project) { create(:project, group: subgroup) }
+      let(:another_project) { create(:project, group: another_subgroup) }
+      let(:subsub_project) { create(:project, group: subsubgroup) }
+
+      let!(:contacts) { create_list(:contact, 4, group: root_group) }
+      let!(:organizations) { create_list(:organization, 2, group: root_group) }
+
+      before do
+        create(:issue_customer_relations_contact, contact: contacts[0], issue: create(:issue, project: root_project))
+        create(:issue_customer_relations_contact, contact: contacts[1], issue: create(:issue, project: sub_project))
+        create(:issue_customer_relations_contact, contact: contacts[2], issue: create(:issue, project: another_project))
+        create(:issue_customer_relations_contact, contact: contacts[3], issue: create(:issue, project: subsub_project))
+        root_group.add_owner(user)
+      end
+
+      context 'moving up' do
+        let(:group) { subsubgroup }
+
+        it 'retains issue contacts' do
+          expect { transfer_service.execute(root_group) }
+            .not_to change { CustomerRelations::IssueContact.count }
+        end
+      end
+
+      context 'moving down' do
+        let(:group) { subgroup }
+
+        it 'retains issue contacts' do
+          expect { transfer_service.execute(another_subgroup) }
+          .not_to change { CustomerRelations::IssueContact.count }
+        end
+      end
+
+      context 'moving sideways' do
+        let(:group) { subsubgroup }
+
+        it 'retains issue contacts' do
+          expect { transfer_service.execute(another_subgroup) }
+          .not_to change { CustomerRelations::IssueContact.count }
+        end
+      end
+
+      context 'moving to new root group' do
+        let(:group) { root_group }
+
+        before do
+          new_parent_group.add_owner(user)
+        end
+
+        it 'moves all crm objects' do
+          expect { transfer_service.execute(new_parent_group) }
+            .to change { root_group.contacts.count }.by(-4)
+            .and change { root_group.organizations.count }.by(-2)
+        end
+
+        it 'retains issue contacts' do
+          expect { transfer_service.execute(new_parent_group) }
+            .not_to change { CustomerRelations::IssueContact.count }
+        end
+      end
+
+      context 'moving to a subgroup within a new root group' do
+        let(:group) { root_group }
+        let(:subgroup_in_new_parent_group) { create(:group, parent: new_parent_group) }
+
+        context 'with permission on the root group' do
+          before do
+            new_parent_group.add_owner(user)
+          end
+
+          it 'moves all crm objects' do
+            expect { transfer_service.execute(subgroup_in_new_parent_group) }
+              .to change { root_group.contacts.count }.by(-4)
+              .and change { root_group.organizations.count }.by(-2)
+          end
+
+          it 'retains issue contacts' do
+            expect { transfer_service.execute(subgroup_in_new_parent_group) }
+              .not_to change { CustomerRelations::IssueContact.count }
+          end
+        end
+
+        context 'with permission on the subgroup' do
+          before do
+            subgroup_in_new_parent_group.add_owner(user)
+          end
+
+          it 'raises error' do
+            transfer_service.execute(subgroup_in_new_parent_group)
+
+            expect(transfer_service.error).to eq("Transfer failed: Group contains contacts/organizations and you don't have enough permissions to move them to the new root group.")
+          end
+        end
+      end
+    end
   end
 end