From 940712454a03c446a20b432ba1e7df42aae94c88 Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Fri, 27 Sep 2024 15:47:23 +0000 Subject: [PATCH 01/10] Added support for changing group contact source (for CRM) Changelog: added --- .../javascripts/pages/groups/edit/index.js | 4 +- app/controllers/concerns/groups/params.rb | 1 + app/models/customer_relations/contact.rb | 87 +++++++++++-- app/models/customer_relations/organization.rb | 21 +-- app/models/group.rb | 6 +- app/services/groups/transfer_service.rb | 27 ++-- app/services/groups/update_service.rb | 21 +-- .../groups/settings/_permissions.html.haml | 6 +- doc/user/crm/index.md | 33 ++++- locale/gitlab.pot | 9 ++ spec/features/groups_spec.rb | 11 -- .../models/customer_relations/contact_spec.rb | 28 ++-- .../customer_relations/organization_spec.rb | 31 +---- spec/models/group_spec.rb | 29 +++++ spec/services/groups/update_service_spec.rb | 122 ++++++++++++++---- 15 files changed, 292 insertions(+), 144 deletions(-) diff --git a/app/assets/javascripts/pages/groups/edit/index.js b/app/assets/javascripts/pages/groups/edit/index.js index 853d9fe9191aaa..f095ae0261974e 100644 --- a/app/assets/javascripts/pages/groups/edit/index.js +++ b/app/assets/javascripts/pages/groups/edit/index.js @@ -30,9 +30,7 @@ initFilePickers(); initConfirmDanger(); initSettingsPanels(); initTransferGroupForm(); -dirtySubmitFactory( - document.querySelectorAll('.js-general-settings-form, .js-general-permissions-form'), -); +dirtySubmitFactory(document.querySelectorAll('.js-general-settings-form')); mountBadgeSettings(GROUP_BADGE); // Initialize Subgroups selector diff --git a/app/controllers/concerns/groups/params.rb b/app/controllers/concerns/groups/params.rb index eafc24524bc3f1..ed448270085e68 100644 --- a/app/controllers/concerns/groups/params.rb +++ b/app/controllers/concerns/groups/params.rb @@ -55,6 +55,7 @@ def group_params_attributes :setup_for_company, :jobs_to_be_done, :crm_enabled, + :crm_source_group_id, :enable_namespace_descendants_cache ] + [group_feature_attributes: group_feature_attributes] end diff --git a/app/models/customer_relations/contact.rb b/app/models/customer_relations/contact.rb index f8971ef013c417..6fb58f5d1a18af 100644 --- a/app/models/customer_relations/contact.rb +++ b/app/models/customer_relations/contact.rb @@ -124,24 +124,85 @@ def self.exists_for_group?(group) exists?(group: group) end - def self.move_to_root_group(group) + def self.move_group(old_group_id, new_group_id, was_crm_source) + copy_query = <<~SQL + WITH org_inserts AS ( + -- Insert organizations in the target group if they don't already exist + INSERT INTO #{CustomerRelations::Organization.table_name} ( + group_id, + created_at, + updated_at, + state, + default_rate, + name, + description + ) + SELECT + :new_group_id, + source_organizations.created_at, + source_organizations.updated_at, + source_organizations.state, + source_organizations.default_rate, + source_organizations.name, + source_organizations.description + FROM #{CustomerRelations::Organization.table_name} source_organizations + LEFT JOIN #{CustomerRelations::Organization.table_name} target_organizations + ON target_organizations.group_id = :new_group_id AND LOWER(target_organizations.name) = LOWER(source_organizations.name) + WHERE source_organizations.group_id = :old_group_id AND target_organizations.id IS NULL + RETURNING id, name + ), + org_map AS ( + -- Create a mapping of old organization IDs to new organization IDs + SELECT source_organizations.id AS old_id, target_organizations.id AS new_id + FROM #{CustomerRelations::Organization.table_name} source_organizations + JOIN #{CustomerRelations::Organization.table_name} target_organizations ON target_organizations.group_id = :new_group_id AND LOWER(target_organizations.name) = LOWER(source_organizations.name) + WHERE source_organizations.group_id = :old_group_id + ) + -- Insert contacts linked to the new organization, deduplicating by email + INSERT INTO #{table_name} ( + group_id, + organization_id, + created_at, + updated_at, + state, + phone, + first_name, + last_name, + email, + description + ) + SELECT DISTINCT + :new_group_id, + org_map.new_id, + source_contacts.created_at, + source_contacts.updated_at, + source_contacts.state, + source_contacts.phone, + source_contacts.first_name, + source_contacts.last_name, + source_contacts.email, + source_contacts.description + FROM #{table_name} source_contacts + LEFT JOIN #{table_name} target_contacts + ON target_contacts.group_id = :new_group_id AND LOWER(target_contacts.email) = LOWER(source_contacts.email) + LEFT JOIN org_map ON org_map.old_id = source_contacts.organization_id + WHERE source_contacts.group_id = :old_group_id AND target_contacts.id IS NULL + SQL + connection.execute(sanitize_sql([copy_query, { old_group_id: old_group_id, new_group_id: new_group_id }])) + 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 + SET contact_id = target_contacts.id + FROM #{table_name} AS source_contacts + JOIN #{table_name} AS target_contacts ON target_contacts.group_id = :new_group_id AND LOWER(target_contacts.email) = LOWER(source_contacts.email) + WHERE source_contacts.group_id = :old_group_id AND contact_id = source_contacts.id SQL - connection.execute(sanitize_sql([update_query, { old_group_id: group.root_ancestor.id, new_group_id: group.id }])) + connection.execute(sanitize_sql([update_query, { old_group_id: old_group_id, new_group_id: new_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 }])) + return unless was_crm_source - where(group: group).update_all(group_id: group.root_ancestor.id) + where(group_id: old_group_id).delete_all + CustomerRelations::Organization.where(group_id: old_group_id).delete_all end def self.counts_by_state diff --git a/app/models/customer_relations/organization.rb b/app/models/customer_relations/organization.rb index 8b0e31f7298927..99fba66ec363ea 100644 --- a/app/models/customer_relations/organization.rb +++ b/app/models/customer_relations/organization.rb @@ -8,6 +8,7 @@ class CustomerRelations::Organization < ApplicationRecord self.table_name = "customer_relations_organizations" belongs_to :group, -> { where(type: Group.sti_name) }, foreign_key: 'group_id' + has_many :contacts strip_attributes! :name @@ -58,26 +59,6 @@ def self.find_by_name(group_id, name) .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 - def self.counts_by_state default_state_counts.merge(group(:state).count) end diff --git a/app/models/group.rb b/app/models/group.rb index 2640c26d6f23da..863ea0a72a8725 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -108,7 +108,7 @@ def of_ancestors_and_self has_many :contacts, class_name: 'CustomerRelations::Contact', inverse_of: :group, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_one :crm_settings, class_name: 'Group::CrmSettings', inverse_of: :group # Groups for which this is the source of CRM contacts/organizations - has_many :crm_targets, class_name: 'Group::CrmSettings', inverse_of: :source_group + has_many :crm_targets, class_name: 'Group::CrmSettings', inverse_of: :source_group, foreign_key: 'source_group_id' has_many :cluster_groups, class_name: 'Clusters::Group' has_many :clusters, through: :cluster_groups, class_name: 'Clusters::Cluster' @@ -1071,6 +1071,10 @@ def crm_group? end strong_memoize_attr :crm_group? + def has_issues_with_contacts? + CustomerRelations::IssueContact.joins(:issue).where(issue: { project_id: Project.where(namespace_id: self_and_descendant_ids) }).exists? + end + private def feature_flag_enabled_for_self_or_ancestor?(feature_flag, type: :development) diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb index ff60c15430972e..1d48e35c134ddd 100644 --- a/app/services/groups/transfer_service.rb +++ b/app/services/groups/transfer_service.rb @@ -67,8 +67,7 @@ def proceed_to_transfer update_group_attributes ensure_ownership update_integrations - remove_issue_contacts(old_root_ancestor_id, was_root_group) - update_crm_objects(was_root_group) + update_crm_objects remove_namespace_commit_emails(was_root_group) end end @@ -112,10 +111,11 @@ def ensure_allowed_transfer 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 false if group.crm_settings&.source_group + return false if group.crm_group == @new_parent_group.crm_group - return true if group.contacts.exists? && !current_user.can?(:admin_crm_contact, @new_parent_group.root_ancestor) - return true if group.crm_organizations.exists? && !current_user.can?(:admin_crm_organization, @new_parent_group.root_ancestor) + return true if group.crm_group.contacts.exists? && !current_user.can?(:admin_crm_contact, @new_parent_group.root_ancestor) + return true if group.crm_group.crm_organizations.exists? && !current_user.can?(:admin_crm_organization, @new_parent_group.root_ancestor) false end @@ -308,18 +308,13 @@ def pending_builds_params ::Ci::PendingBuild.namespace_transfer_params(group) end - def update_crm_objects(was_root_group) - return unless was_root_group + def update_crm_objects + return unless group && new_parent_group + return if group.crm_settings&.source_group + return if group.crm_group == new_parent_group.crm_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) + was_crm_source = group.crm_group == group + CustomerRelations::Contact.move_group(group.crm_group.id, new_parent_group.crm_group.id, was_crm_source) end def publish_event(old_root_ancestor_id) diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 6d11660c3093ed..ed321bd8242bb5 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -23,21 +23,17 @@ def execute end return false unless valid_visibility_level_change?(group, group.visibility_attribute_value(params)) - return false unless valid_share_with_group_lock_change? - return false unless valid_path_change? - return false unless update_shared_runners handle_changes - handle_namespace_settings - handle_hierarchy_cache_update - group.assign_attributes(params) + return false if group.errors.present? + begin success = group.save @@ -157,7 +153,7 @@ def remove_unallowed_params def handle_changes handle_settings_update - handle_crm_settings_update unless params[:crm_enabled].nil? + handle_crm_settings_update end def handle_settings_update @@ -169,11 +165,20 @@ def handle_settings_update end def handle_crm_settings_update + return if params[:crm_enabled].nil? && params[:crm_source_group_id].nil? + crm_enabled = params.delete(:crm_enabled) - return if group.crm_enabled? == crm_enabled + crm_enabled = true if crm_enabled.nil? + crm_source_group_id = params.delete(:crm_source_group_id) + return if group.crm_enabled? == crm_enabled && group.crm_settings&.source_group_id == crm_source_group_id + + if group.crm_settings&.source_group_id != crm_source_group_id && group.has_issues_with_contacts? + group.errors.add(:base, s_('GroupSettings|Contact source cannot be changed when issues already have contacts assigned from a different source.')) + end crm_settings = group.crm_settings || group.build_crm_settings crm_settings.enabled = crm_enabled + crm_settings.source_group_id = crm_source_group_id.presence crm_settings.save end diff --git a/app/views/groups/settings/_permissions.html.haml b/app/views/groups/settings/_permissions.html.haml index 25b0b34b32ef68..938fbd451de5c5 100644 --- a/app/views/groups/settings/_permissions.html.haml +++ b/app/views/groups/settings/_permissions.html.haml @@ -52,6 +52,10 @@ s_('GroupSettings|Customer relations is enabled'), checkbox_options: { checked: @group.crm_enabled? }, help_text: s_('GroupSettings|Organizations and contacts can be created and associated with issues.') + .form-group.gl-mb-3 + = s_('GroupSettings|Contact source') + .js-vue-group-select{ data: { input_name: 'group[crm_source_group_id]', selected: @group.crm_settings&.source_group_id, clearable: 'true' } } + %p.-gl-mt-5= s_("GroupSettings|The group from which to source contacts for issues in this group and it's subgroups.") - if Feature.enabled?(:group_hierarchy_optimization, @group, type: :beta) %h5= _('Performance') @@ -61,4 +65,4 @@ checkbox_options: { checked: @group.namespace_descendants.present? }, help_text: s_('GroupSettings|Building the cache is asynchronous, happens in a background job. The cache invalidation is synchronous with strong consistency guarantees.') - = f.submit _('Save changes'), pajamas_button: true, class: 'gl-mt-3 js-dirty-submit', data: { testid: 'save-permissions-changes-button' } + = f.submit _('Save changes'), pajamas_button: true, class: 'gl-mt-3', data: { testid: 'save-permissions-changes-button' } diff --git a/doc/user/crm/index.md b/doc/user/crm/index.md index 41e4cd9276ef7c..228a56fc9460cd 100644 --- a/doc/user/crm/index.md +++ b/doc/user/crm/index.md @@ -18,7 +18,8 @@ DETAILS: With customer relations management (CRM) you can create a record of contacts (individuals) and organizations (companies) and relate them to issues. -Contacts and organizations can only be created for top-level groups. +By default, contacts and organizations can only be created for top-level groups. +To create contacts and organizations in other groups, [assign the group as a contact source](#configure-the-contact-source). You can use contacts and organizations to tie work to customers for billing and reporting purposes. For more information about what is planned for the future, see [issue 2256](https://gitlab.com/gitlab-org/gitlab/-/issues/2256). @@ -48,6 +49,24 @@ To enable customer relations management in a group or subgroup: 1. Select **Customer relations is enabled**. 1. Select **Save changes**. +## Configure the contact source + +> - [Available](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/164945) in GitLab 17.4. + +By default, contacts are sourced from an issue's top-level group. + +The contact source for a group will apply to all subgroups, +unless they have a contact source configured. + +To configure the contact source for a group or subgroup: + +1. On the left sidebar, select **Search or go to** and find your group or subgroup. +1. Select **Settings > General**. +1. Expand the **Permissions and group features** section. +1. Select **Contact source > Search for a group**. +1. Select the group from which you wish to source contacts. +1. Select **Save changes**. + ## Contacts ### View contacts linked to a group @@ -256,15 +275,17 @@ When you use the `/remove_contacts` quick action, follow it with `[contact:` and ## Moving objects with CRM entries -The top-level group is the topmost group in the group hierarchy. - -When you move an issue, project, or group in the same group hierarchy, +When you move an issue or project and the **parent group contact source matches**, issues retain their contacts. -When you move an issue or project and the top-level group changes, +When you move an issue or project and the **parent group contact source changes**, issues lose their contacts. -When you move a group and its top-level group changes: +When you move a group with a [contact source configured](#configure-the-contact-source) +or it's **contact source remains unchanged**, +issues retain their contacts. + +When you move a group and its **contact source changes**: - All unique contacts and organizations are migrated to the new top-level group. - Contacts that already exist (by email address) are deemed duplicates and deleted. diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 63e060e08c76fd..5c8aa89711dbfe 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -26739,6 +26739,12 @@ msgstr "" msgid "GroupSettings|Configure limits on the number of repositories users can download, clone, or fork in a given time." msgstr "" +msgid "GroupSettings|Contact source" +msgstr "" + +msgid "GroupSettings|Contact source cannot be changed when issues already have contacts assigned from a different source." +msgstr "" + msgid "GroupSettings|Custom project templates" msgstr "" @@ -26883,6 +26889,9 @@ msgstr "" msgid "GroupSettings|The Auto DevOps pipeline runs if no alternative CI configuration file is found." msgstr "" +msgid "GroupSettings|The group from which to source contacts for issues in this group and it's subgroups." +msgstr "" + msgid "GroupSettings|There was a problem updating Auto DevOps pipeline: %{error_messages}." msgstr "" diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index 1e964b9ee0cc5f..b8712a61e270f3 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -353,17 +353,6 @@ visit path end - it_behaves_like 'dirty submit form', [ - { form: '.js-general-settings-form', input: 'input[name="group[name]"]', submit: 'button[type="submit"]' }, - { form: '.js-general-settings-form', input: '#group_visibility_level_0', submit: 'button[type="submit"]' }, - { form: '.js-general-permissions-form', input: '#group_request_access_enabled', submit: 'button[type="submit"]' }, - { - form: '.js-general-permissions-form', - input: 'input[name="group[two_factor_grace_period]"]', - submit: 'button[type="submit"]' - } - ] - it 'saves new settings' do within_testid('general-settings') do # Have to reset it to '' so it overwrites rather than appends diff --git a/spec/models/customer_relations/contact_spec.rb b/spec/models/customer_relations/contact_spec.rb index af8ac7b698db9f..110d7eb7cab4af 100644 --- a/spec/models/customer_relations/contact_spec.rb +++ b/spec/models/customer_relations/contact_spec.rb @@ -129,9 +129,10 @@ end end - describe '#self.move_to_root_group' do + describe '#self.move_group' do let!(:old_root_group) { create(:group) } let!(:contacts) { create_list(:contact, 4, group: old_root_group) } + let!(:orgs) { create_list(:crm_organization, 2, 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]) } @@ -139,23 +140,32 @@ 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) } + let!(:dupe_org) { create(:crm_organization, group: new_root_group, name: orgs[0].name) } before do + contacts[0].update!(organization: orgs[0]) + contacts[1].update!(organization: orgs[1]) old_root_group.update!(parent: new_root_group) - described_class.move_to_root_group(old_root_group) + described_class.move_group(old_root_group.id, new_root_group.id, true) 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) + it 'moves all contacts and organizations out of old root group' do + expect(described_class.where(group_id: old_root_group.id)).to be_empty + expect(CustomerRelations::Organization.where(group_id: old_root_group.id)).to be_empty 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) + it 'updates issue_contact.contact_id for dupes' do expect(issue_contact2.reload.contact_id).to eq(dupe_contact1.id) end + + it 'updates issue_contact for new contacts' do + expect(issue_contact1.reload.contact.group_id).to eq(new_root_group.id) + expect(issue_contact1.reload.contact.email).to eq(contacts[0].email) + end + + it 'attaches dupe org to new contact with matching email' do + expect(dupe_org.contacts.pluck(:email)).to contain_exactly(contacts[0].email) + end end describe '.search' do diff --git a/spec/models/customer_relations/organization_spec.rb b/spec/models/customer_relations/organization_spec.rb index 9896099df4c90b..c90f468a6bf118 100644 --- a/spec/models/customer_relations/organization_spec.rb +++ b/spec/models/customer_relations/organization_spec.rb @@ -7,6 +7,7 @@ describe 'associations' do it { is_expected.to belong_to(:group).with_foreign_key('group_id') } + it { is_expected.to have_many(:contacts) } end describe 'validations' do @@ -66,36 +67,6 @@ end end - describe '#self.move_to_root_group' do - let!(:old_root_group) { create(:group) } - let!(:crm_organizations) { create_list(:crm_organization, 4, group: old_root_group) } - let!(:new_root_group) { create(:group) } - let!(:contact1) { create(:contact, group: new_root_group, organization: crm_organizations[0]) } - let!(:contact2) { create(:contact, group: new_root_group, organization: crm_organizations[1]) } - - let!(:dupe_crm_organization1) { create(:crm_organization, group: new_root_group, name: crm_organizations[1].name) } - let!(:dupe_crm_organization2) do - create(:crm_organization, group: new_root_group, name: crm_organizations[3].name.upcase) - end - - before do - old_root_group.update!(parent: new_root_group) - described_class.move_to_root_group(old_root_group) - end - - it 'moves organizations with unique names and deletes the rest' do - expect(crm_organizations[0].reload.group_id).to eq(new_root_group.id) - expect(crm_organizations[2].reload.group_id).to eq(new_root_group.id) - expect { crm_organizations[1].reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { crm_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(crm_organizations[0].id) - expect(contact2.reload.organization_id).to eq(dupe_crm_organization1.id) - end - end - describe '.search' do let_it_be(:crm_organization_a) do create( diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 7ff9af46645c05..d691428a7a3aae 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -4096,4 +4096,33 @@ def define_cache_expectations(cache_key) end end end + + describe '#has_issues_with_contacts?' do + context 'when group has no issues with contacts' do + it 'returns false' do + expect(group.has_issues_with_contacts?).to be_falsey + end + end + + context 'when group has issues with contacts' do + let!(:issue) { create(:issue, project: create(:project, group: group)) } + let!(:contact) { create(:contact, group: group) } + let!(:issue_contact) { create(:issue_customer_relations_contact, issue: issue, contact: contact) } + + it 'returns true' do + expect(group.has_issues_with_contacts?).to be_truthy + end + end + + context 'when a subgroup has issues with contacts' do + let!(:subgroup) { create(:group, parent: group) } + let!(:issue) { create(:issue, project: create(:project, group: subgroup)) } + let!(:contact) { create(:contact, group: group) } + let!(:issue_contact) { create(:issue_customer_relations_contact, issue: issue, contact: contact) } + + it 'returns true' do + expect(group.has_issues_with_contacts?).to be_truthy + end + end + end end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index f10809e4d9d198..2caa6299077327 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -148,18 +148,18 @@ end end - context 'crm_enabled param' do - context 'when no existing crm_settings' do - it 'when param not present, leave crm enabled' do - params = {} + context 'crm params' do + let(:params) { {} } + context 'when no existing crm_settings' do + it 'when params not present, leave crm enabled' do described_class.new(public_group, user, params).execute updated_group = public_group.reload expect(updated_group.crm_enabled?).to be_truthy end - it 'when param set false, disables crm' do + it 'when crm_enabled param set false, disables crm' do params = { crm_enabled: false } described_class.new(public_group, user, params).execute @@ -167,47 +167,117 @@ expect(updated_group.crm_enabled?).to be_falsy end + + it 'when crm_source_group_id present, updates crm_group' do + params = { crm_source_group_id: internal_group.id } + + described_class.new(public_group, user, params).execute + updated_group = public_group.reload + + expect(updated_group.crm_enabled?).to be_truthy + expect(updated_group.crm_group).to eq(internal_group) + end end context 'with existing crm_settings' do - it 'when param set true, enables crm' do - params = { crm_enabled: true } - create(:crm_settings, group: public_group, enabled: false) + let(:init_enabled) { true } + + before do + create(:crm_settings, group: public_group, enabled: init_enabled) + end + + context 'when crm initially disabled' do + let(:init_enabled) { false } + + context 'when crm_enabled param set true' do + let(:params) { { crm_enabled: true } } + + it 'enables crm' do + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_truthy + end + end + + it 'when crm_enabled param not present, crm remains disabled' do + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_falsy + end + end + + context 'when crm_enabled param set false' do + let(:init_enabled) { true } + let(:params) { { crm_enabled: false } } + it 'disables crm' do + described_class.new(public_group, user, params).execute + + updated_group = public_group.reload + expect(updated_group.crm_enabled?).to be_falsy + end + end + + it 'when crm_enabled param not present, crm remains enabled' do described_class.new(public_group, user, params).execute updated_group = public_group.reload expect(updated_group.crm_enabled?).to be_truthy end - it 'when param set false, disables crm' do - params = { crm_enabled: false } - create(:crm_settings, group: public_group, enabled: true) + context 'when crm_source_group_id present' do + let(:params) { { crm_source_group_id: internal_group.id } } - described_class.new(public_group, user, params).execute + it 'updates crm_group' do + described_class.new(public_group, user, params).execute + updated_group = public_group.reload - updated_group = public_group.reload - expect(updated_group.crm_enabled?).to be_falsy + expect(updated_group.crm_enabled?).to be_truthy + expect(updated_group.crm_settings.source_group).to eq(internal_group) + expect(updated_group.crm_group).to eq(internal_group) + end end - it 'when param not present, crm remains disabled' do - params = {} - create(:crm_settings, group: public_group, enabled: false) + context 'when crm_source_group_id blank' do + let(:params) { { crm_source_group_id: '' } } - described_class.new(public_group, user, params).execute + it 'clears source_group and resets crm_group' do + described_class.new(public_group, user, params).execute + updated_group = public_group.reload - updated_group = public_group.reload - expect(updated_group.crm_enabled?).to be_falsy + expect(updated_group.crm_enabled?).to be_truthy + expect(updated_group.crm_settings.source_group).to be_nil + expect(updated_group.crm_group).to eq(public_group) + end end + end - it 'when param not present, crm remains enabled' do - params = {} - create(:crm_settings, group: public_group, enabled: true) + context 'when changing source' do + let(:params) { { crm_source_group_id: internal_group.id } } - described_class.new(public_group, user, params).execute + context 'when issues do not have contacts' do + it 'updates crm_group' do + described_class.new(public_group, user, params).execute + updated_group = public_group.reload - updated_group = public_group.reload - expect(updated_group.crm_enabled?).to be_truthy + expect(updated_group.crm_group).to eq(internal_group) + end + end + + context 'when issues do have contacts' do + let!(:issue) { create(:issue, project: create(:project, group: public_group)) } + let!(:contact) { create(:contact, group: public_group) } + let!(:issue_contact) { create(:issue_customer_relations_contact, issue: issue, contact: contact) } + + it 'returns an error and does not update crm_group' do + described_class.new(public_group, user, params).execute + updated_group = public_group.reload + + expect(public_group.errors).to contain_exactly('Contact source cannot be changed when issues already have contacts assigned from a different source.') + expect(updated_group.crm_group).to eq(public_group) + end end end end -- GitLab From be9feaa68125529a0be65875f6b4644678d481bc Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Fri, 1 Nov 2024 15:04:51 +0000 Subject: [PATCH 02/10] Update file update_service.rb --- app/services/groups/update_service.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index ed321bd8242bb5..11d0b00bcc4127 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -174,6 +174,7 @@ def handle_crm_settings_update if group.crm_settings&.source_group_id != crm_source_group_id && group.has_issues_with_contacts? group.errors.add(:base, s_('GroupSettings|Contact source cannot be changed when issues already have contacts assigned from a different source.')) + return end crm_settings = group.crm_settings || group.build_crm_settings -- GitLab From 61edfdccd2d602c8804462cad0842886b8833520 Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Sat, 2 Nov 2024 20:09:37 +0000 Subject: [PATCH 03/10] Fix bug moving projects --- app/services/projects/transfer_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index a2becd385c3523..0688d207a7cf4d 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -278,7 +278,7 @@ def pending_builds_params end def remove_issue_contacts - return unless @old_group&.root_ancestor != @new_namespace&.root_ancestor + return unless @old_group&.crm_group != @new_namespace&.crm_group CustomerRelations::IssueContact.delete_for_project(project.id) end -- GitLab From 2a45b4547ade3fecb3ecf8ac83320b652937f994 Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Sat, 2 Nov 2024 20:53:33 +0000 Subject: [PATCH 04/10] Update file user_namespace.rb --- app/models/namespaces/user_namespace.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/models/namespaces/user_namespace.rb b/app/models/namespaces/user_namespace.rb index a6239ab9b39477..a8c5005d460afd 100644 --- a/app/models/namespaces/user_namespace.rb +++ b/app/models/namespaces/user_namespace.rb @@ -55,5 +55,9 @@ def max_member_access_for_user(user, only_concrete_membership: false) owner == user ? Gitlab::Access::OWNER : Gitlab::Access::NO_ACCESS end + + def crm_group + nil + end end end -- GitLab From 35bda48f985bc8b9fb8749604124587deb6eda21 Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Sat, 2 Nov 2024 21:54:20 +0000 Subject: [PATCH 05/10] Update file issue_sidebar_basic_entity.rb --- app/serializers/issue_sidebar_basic_entity.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serializers/issue_sidebar_basic_entity.rb b/app/serializers/issue_sidebar_basic_entity.rb index efb08fc08f122e..fbc84fc6f55828 100644 --- a/app/serializers/issue_sidebar_basic_entity.rb +++ b/app/serializers/issue_sidebar_basic_entity.rb @@ -13,7 +13,7 @@ class IssueSidebarBasicEntity < IssuableSidebarBasicEntity expose :show_crm_contacts do |issuable| current_user&.can?(:read_crm_contacts, issuable) && - CustomerRelations::Contact.exists_for_group?(issuable.project.root_ancestor) + CustomerRelations::Contact.exists_for_group?(issuable.project.crm_group) end end -- GitLab From ad7d017b6c3a463f4af223261b5dfeb6dfb0c43c Mon Sep 17 00:00:00 2001 From: Lee Tickett Date: Tue, 5 Nov 2024 15:41:34 +0000 Subject: [PATCH 06/10] Apply reviewer suggestions --- .../vue_shared/components/entity_select/group_select.vue | 6 ++++++ .../components/entity_select/init_group_selects.js | 2 ++ app/views/groups/settings/_permissions.html.haml | 9 +++++++-- .../components/entity_select/group_select_spec.js | 3 +++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/vue_shared/components/entity_select/group_select.vue b/app/assets/javascripts/vue_shared/components/entity_select/group_select.vue index a8f32db0c8baef..a72143ec4a9bf1 100644 --- a/app/assets/javascripts/vue_shared/components/entity_select/group_select.vue +++ b/app/assets/javascripts/vue_shared/components/entity_select/group_select.vue @@ -34,6 +34,11 @@ export default { required: false, default: '', }, + description: { + type: String, + required: false, + default: '', + }, inputName: { type: String, required: true, @@ -126,6 +131,7 @@ export default {