Merge branch '2256-cascade-crm-objects' into 'master'

Allow issue contacts from parent groups

See merge request gitlab-org/gitlab!77352

app/models/customer_relations/contact.rb

@@ -26,10 +26,10 @@ class CustomerRelations::Contact < ApplicationRecord
   validate :validate_email_format
   validate :unique_email_for_group_hierarchy
 
-  def self.find_ids_by_emails(group_id, emails)
+  def self.find_ids_by_emails(group, emails)
     raise ArgumentError, "Cannot lookup more than #{MAX_PLUCK} emails" if emails.length > MAX_PLUCK
 
-    where(group_id: group_id, email: emails)
+    where(group_id: group.self_and_ancestor_ids, email: emails)
       .pluck(:id)
   end
 

app/models/customer_relations/issue_contact.rb

@@ -6,7 +6,7 @@ class CustomerRelations::IssueContact < ApplicationRecord
   belongs_to :issue, optional: false, inverse_of: :customer_relations_contacts
   belongs_to :contact, optional: false, inverse_of: :issue_contacts
 
-  validate :contact_belongs_to_issue_group
+  validate :contact_belongs_to_issue_group_or_ancestor
 
   def self.find_contact_ids_by_emails(issue_id, emails)
     raise ArgumentError, "Cannot lookup more than #{MAX_PLUCK} emails" if emails.length > MAX_PLUCK
@@ -18,11 +18,11 @@ class CustomerRelations::IssueContact < ApplicationRecord
 
   private
 
-  def contact_belongs_to_issue_group
+  def contact_belongs_to_issue_group_or_ancestor
     return unless contact&.group_id
     return unless issue&.project&.namespace_id
-    return if contact.group_id == issue.project.namespace_id
+    return if issue.project.group&.self_and_ancestor_ids&.include?(contact.group_id)
 
-    errors.add(:base, _('The contact does not belong to the same group as the issue'))
+    errors.add(:base, _('The contact does not belong to the issue group or an ancestor'))
   end
 end

app/services/issues/set_crm_contacts_service.rb

@@ -48,7 +48,7 @@ module Issues
     end
 
     def add_by_email
-      contact_ids = ::CustomerRelations::Contact.find_ids_by_emails(project_group.id, params[:add_emails])
+      contact_ids = ::CustomerRelations::Contact.find_ids_by_emails(project_group, params[:add_emails])
       add_by_id(contact_ids)
     end
 

locale/gitlab.pot

@@ -35367,7 +35367,7 @@ msgstr ""
 msgid "The connection will time out after %{timeout}. For repositories that take longer, use a clone/push combination."
 msgstr ""
 
-msgid "The contact does not belong to the same group as the issue"
+msgid "The contact does not belong to the issue group or an ancestor"
 msgstr ""
 
 msgid "The content editor may change the markdown formatting style of the document, which may not match your original markdown style."

spec/models/customer_relations/contact_spec.rb

@@ -75,20 +75,27 @@ RSpec.describe CustomerRelations::Contact, type: :model do
     let_it_be(:other_contacts) { create_list(:contact, 2) }
 
     it 'returns ids of contacts from group' do
-      contact_ids = described_class.find_ids_by_emails(group.id, group_contacts.pluck(:email))
+      contact_ids = described_class.find_ids_by_emails(group, group_contacts.pluck(:email))
+
+      expect(contact_ids).to match_array(group_contacts.pluck(:id))
+    end
+
+    it 'returns ids of contacts from parent group' do
+      subgroup = create(:group, parent: group)
+      contact_ids = described_class.find_ids_by_emails(subgroup, group_contacts.pluck(:email))
 
       expect(contact_ids).to match_array(group_contacts.pluck(:id))
     end
 
     it 'does not return ids of contacts from other groups' do
-      contact_ids = described_class.find_ids_by_emails(group.id, other_contacts.pluck(:email))
+      contact_ids = described_class.find_ids_by_emails(group, other_contacts.pluck(:email))
 
       expect(contact_ids).to be_empty
     end
 
     it 'raises ArgumentError when called with too many emails' do
       too_many_emails = described_class::MAX_PLUCK + 1
-      expect { described_class.find_ids_by_emails(group.id, Array(0..too_many_emails)) }.to raise_error(ArgumentError)
+      expect { described_class.find_ids_by_emails(group, Array(0..too_many_emails)) }.to raise_error(ArgumentError)
     end
   end
 end

spec/models/customer_relations/issue_contact_spec.rb

@@ -5,7 +5,8 @@ require 'spec_helper'
 RSpec.describe CustomerRelations::IssueContact do
   let_it_be(:issue_contact, reload: true) { create(:issue_customer_relations_contact) }
   let_it_be(:group) { create(:group) }
-  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:subgroup) { create(:group, parent: group) }
+  let_it_be(:project) { create(:project, group: subgroup) }
   let_it_be(:issue) { create(:issue, project: project) }
 
   subject { issue_contact }
@@ -33,17 +34,29 @@ RSpec.describe CustomerRelations::IssueContact do
     end
 
     it 'builds using the same group', :aggregate_failures do
-      expect(for_issue.contact.group).to eq(group)
+      expect(for_issue.contact.group).to eq(subgroup)
       expect(for_contact.issue.project.group).to eq(group)
     end
   end
 
   describe 'validation' do
-    let(:built) { build(:issue_customer_relations_contact, issue: create(:issue), contact: create(:contact)) }
+    it 'fails when the contact group does not belong to the issue group or ancestors' do
+      built = build(:issue_customer_relations_contact, issue: create(:issue), contact: create(:contact))
 
-    it 'fails when the contact group does not match the issue group' do
       expect(built).not_to be_valid
     end
+
+    it 'succeeds when the contact group is the same as the issue group' do
+      built = build(:issue_customer_relations_contact, issue: create(:issue, project: project), contact: create(:contact, group: subgroup))
+
+      expect(built).to be_valid
+    end
+
+    it 'succeeds when the contact group is an ancestor of the issue group' do
+      built = build(:issue_customer_relations_contact, issue: create(:issue, project: project), contact: create(:contact, group: group))
+
+      expect(built).to be_valid
+    end
   end
 
   describe '#self.find_contact_ids_by_emails' do

spec/requests/api/graphql/mutations/issues/set_crm_contacts_spec.rb

@@ -7,12 +7,17 @@ RSpec.describe 'Setting issues crm contacts' do
 
   let_it_be(:user) { create(:user) }
   let_it_be(:group) { create(:group, :crm_enabled) }
-  let_it_be(:project) { create(:project, group: group) }
-  let_it_be(:contacts) { create_list(:contact, 4, group: group) }
+  let_it_be(:subgroup) { create(:group, :crm_enabled, parent: group) }
+  let_it_be(:project) { create(:project, group: subgroup) }
+  let_it_be(:group_contacts) { create_list(:contact, 4, group: group) }
+  let_it_be(:subgroup_contacts) { create_list(:contact, 4, group: subgroup) }
 
   let(:issue) { create(:issue, project: project) }
   let(:operation_mode) { Types::MutationOperationModeEnum.default_mode }
-  let(:contact_ids) { [global_id_of(contacts[1]), global_id_of(contacts[2])] }
+  let(:contacts) { subgroup_contacts }
+  let(:initial_contacts) { contacts[0..1] }
+  let(:mutation_contacts) { contacts[1..2] }
+  let(:contact_ids) { contact_global_ids(mutation_contacts) }
   let(:does_not_exist_or_no_permission) { "The resource that you are attempting to access does not exist or you don't have permission to perform this action" }
 
   let(:mutation) do
@@ -42,10 +47,47 @@ RSpec.describe 'Setting issues crm contacts' do
     graphql_mutation_response(:issue_set_crm_contacts)
   end
 
-  context 'when the feature is enabled' do
+  def contact_global_ids(contacts)
+    contacts.map { |contact| global_id_of(contact) }
+  end
+
   before do
-      create(:issue_customer_relations_contact, issue: issue, contact: contacts[0])
-      create(:issue_customer_relations_contact, issue: issue, contact: contacts[1])
+    initial_contacts.each { |contact| create(:issue_customer_relations_contact, issue: issue, contact: contact) }
+  end
+
+  shared_examples 'successful mutation' do
+    context 'replace' do
+      it 'updates the issue with correct contacts' do
+        post_graphql_mutation(mutation, current_user: user)
+
+        expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
+          .to match_array(contact_global_ids(mutation_contacts))
+      end
+    end
+
+    context 'append' do
+      let(:mutation_contacts) { [contacts[3]] }
+      let(:operation_mode) { Types::MutationOperationModeEnum.enum[:append] }
+
+      it 'updates the issue with correct contacts' do
+        post_graphql_mutation(mutation, current_user: user)
+
+        expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
+          .to match_array(contact_global_ids(initial_contacts + mutation_contacts))
+      end
+    end
+
+    context 'remove' do
+      let(:mutation_contacts) { [contacts[0]] }
+      let(:operation_mode) { Types::MutationOperationModeEnum.enum[:remove] }
+
+      it 'updates the issue with correct contacts' do
+        post_graphql_mutation(mutation, current_user: user)
+
+        expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
+          .to match_array(contact_global_ids(initial_contacts - mutation_contacts))
+      end
+    end
   end
 
   context 'when the user has no permission' do
@@ -74,37 +116,14 @@ RSpec.describe 'Setting issues crm contacts' do
       end
     end
 
-      context 'replace' do
-        it 'updates the issue with correct contacts' do
-          post_graphql_mutation(mutation, current_user: user)
+    context 'with issue group contacts' do
+      let(:contacts) { subgroup_contacts }
 
-          expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
-            .to match_array([global_id_of(contacts[1]), global_id_of(contacts[2])])
+      it_behaves_like 'successful mutation'
     end
-      end
-
-      context 'append' do
-        let(:contact_ids) { [global_id_of(contacts[3])] }
-        let(:operation_mode) { Types::MutationOperationModeEnum.enum[:append] }
-
-        it 'updates the issue with correct contacts' do
-          post_graphql_mutation(mutation, current_user: user)
 
-          expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
-            .to match_array([global_id_of(contacts[0]), global_id_of(contacts[1]), global_id_of(contacts[3])])
-        end
-      end
-
-      context 'remove' do
-        let(:contact_ids) { [global_id_of(contacts[0])] }
-        let(:operation_mode) { Types::MutationOperationModeEnum.enum[:remove] }
-
-        it 'updates the issue with correct contacts' do
-          post_graphql_mutation(mutation, current_user: user)
-
-          expect(graphql_data_at(:issue_set_crm_contacts, :issue, :customer_relations_contacts, :nodes, :id))
-            .to match_array([global_id_of(contacts[1])])
-        end
+    context 'with issue ancestor group contacts' do
+      it_behaves_like 'successful mutation'
     end
 
     context 'when the contact does not exist' do
@@ -159,10 +178,10 @@ RSpec.describe 'Setting issues crm contacts' do
       end
     end
   end
-  end
 
   context 'when crm_enabled is false' do
     let(:issue) { create(:issue) }
+    let(:initial_contacts) { [] }
 
     it 'raises expected error' do
       issue.project.add_reporter(user)