Merge branch '349683-add-crm-system-notes' into 'master'

Create system note when issue contacts are changed

See merge request gitlab-org/gitlab!77816

app/helpers/system_note_helper.rb

@@ -42,7 +42,8 @@ module SystemNoteHelper
     'cloned' => 'documents',
     'issue_type' => 'pencil-square',
     'attention_requested' => 'user',
-    'attention_request_removed' => 'user'
+    'attention_request_removed' => 'user',
+    'contact' => 'users'
   }.freeze
 
   def system_note_icon_name(note)

app/models/discussion.rb

@@ -25,7 +25,7 @@ class Discussion
             :to_ability_name,
             :editable?,
             :resolved_by_id,
-            :system_note_with_references_visible_for?,
+            :system_note_visible_for?,
             :resource_parent,
             :save,
             to: :first_note

app/models/note.rb

@@ -27,10 +27,14 @@ class Note < ApplicationRecord
 
   redact_field :note
 
-  TYPES_RESTRICTED_BY_ABILITY = {
+  TYPES_RESTRICTED_BY_PROJECT_ABILITY = {
     branch: :download_code
   }.freeze
 
+  TYPES_RESTRICTED_BY_GROUP_ABILITY = {
+    contact: :read_crm_contact
+  }.freeze
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_by, :updated_by
@@ -119,7 +123,7 @@ class Note < ApplicationRecord
   scope :inc_author, -> { includes(:author) }
   scope :with_api_entity_associations, -> { preload(:note_diff_file, :author) }
   scope :inc_relations_for_view, -> do
-    includes(:project, { author: :status }, :updated_by, :resolved_by, :award_emoji,
+    includes({ project: :group }, { author: :status }, :updated_by, :resolved_by, :award_emoji,
              { system_note_metadata: :description_version }, :note_diff_file, :diff_note_positions, :suggestions)
   end
 
@@ -565,10 +569,10 @@ class Note < ApplicationRecord
     noteable.user_mentions.where(note: self)
   end
 
-  def system_note_with_references_visible_for?(user)
+  def system_note_visible_for?(user)
     return true unless system?
 
-    (!system_note_with_references? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user)
+    system_note_viewable_by?(user) && all_referenced_mentionables_allowed?(user)
   end
 
   def parent_user
@@ -617,10 +621,17 @@ class Note < ApplicationRecord
   def system_note_viewable_by?(user)
     return true unless system_note_metadata
 
-    restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym]
-    return Ability.allowed?(user, restriction, project) if restriction
+    system_note_viewable_by_project_ability?(user) && system_note_viewable_by_group_ability?(user)
+  end
 
-    true
+  def system_note_viewable_by_project_ability?(user)
+    project_restriction = TYPES_RESTRICTED_BY_PROJECT_ABILITY[system_note_metadata.action.to_sym]
+    !project_restriction || Ability.allowed?(user, project_restriction, project)
+  end
+
+  def system_note_viewable_by_group_ability?(user)
+    group_restriction = TYPES_RESTRICTED_BY_GROUP_ABILITY[system_note_metadata.action.to_sym]
+    !group_restriction || Ability.allowed?(user, group_restriction, project&.group)
   end
 
   def keep_around_commit
@@ -646,6 +657,8 @@ class Note < ApplicationRecord
   end
 
   def all_referenced_mentionables_allowed?(user)
+    return true unless system_note_with_references?
+
     if user_visible_reference_count.present? && total_reference_count.present?
       # if they are not equal, then there are private/confidential references as well
       user_visible_reference_count > 0 && user_visible_reference_count == total_reference_count

app/models/system_note_metadata.rb

@@ -24,7 +24,7 @@ class SystemNoteMetadata < ApplicationRecord
     opened closed merged duplicate locked unlocked outdated reviewer
     tag due_date pinned_embed cherry_pick health_status approved unapproved
     status alert_issue_added relate unrelate new_alert_added severity
-    attention_requested attention_request_removed
+    attention_requested attention_request_removed contact
   ].freeze
 
   validates :note, presence: true, unless: :importing?

app/policies/note_policy.rb

@@ -16,7 +16,7 @@ class NotePolicy < BasePolicy
 
   condition(:for_design) { @subject.for_design? }
 
-  condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) }
+  condition(:is_visible) { @subject.system_note_visible_for?(@user) }
 
   condition(:confidential, scope: :subject) { @subject.confidential? }
 

app/services/issues/set_crm_contacts_service.rb

@@ -16,6 +16,9 @@ module Issues
       determine_changes if params[:replace_ids].present?
       return error_too_many if too_many?
 
+      @added_count = 0
+      @removed_count = 0
+
       add if params[:add_ids].present?
       remove if params[:remove_ids].present?
 
@@ -25,6 +28,7 @@ module Issues
       if issue.valid?
         GraphqlTriggers.issue_crm_contacts_updated(issue)
         issue.touch
+        create_system_note
         ServiceResponse.success(payload: issue)
       else
         # The default error isn't very helpful: "Issue customer relations contacts is invalid"
@@ -36,7 +40,7 @@ module Issues
 
     private
 
-    attr_accessor :issue, :errors, :existing_ids
+    attr_accessor :issue, :errors, :existing_ids, :added_count, :removed_count
 
     def determine_changes
       params[:add_ids] = params[:replace_ids] - existing_ids
@@ -63,7 +67,9 @@ module Issues
       contact_ids.uniq.each do |contact_id|
         issue_contact = issue.issue_customer_relations_contacts.create(contact_id: contact_id)
 
-        unless issue_contact.persisted?
+        if issue_contact.persisted?
+          @added_count += 1
+        else
           # The validation ensures that the id exists and the user has permission
           errors << "#{contact_id}: The resource that you are attempting to access does not exist or you don't have permission to perform this action"
         end
@@ -81,7 +87,7 @@ module Issues
 
     def remove_by_id(contact_ids)
       contact_ids &= existing_ids
-      issue.issue_customer_relations_contacts
+      @removed_count += issue.issue_customer_relations_contacts
         .where(contact_id: contact_ids) # rubocop: disable CodeReuse/ActiveRecord
         .delete_all
     end
@@ -129,6 +135,11 @@ module Issues
       params[:add_emails] && params[:add_emails].length > MAX_ADDITIONAL_CONTACTS
     end
 
+    def create_system_note
+      SystemNoteService.change_issuable_contacts(
+        issue, issue.project, current_user, added_count, removed_count)
+    end
+
     def error_no_permissions
       ServiceResponse.error(message: _('You have insufficient permissions to set customer relations contacts for this issue'))
     end

app/services/system_note_service.rb

@@ -45,6 +45,10 @@ module SystemNoteService
     ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_reviewers(old_reviewers)
   end
 
+  def change_issuable_contacts(issuable, project, author, added_count, removed_count)
+    ::SystemNotes::IssuablesService.new(noteable: issuable, project: project, author: author).change_issuable_contacts(added_count, removed_count)
+  end
+
   def relate_issue(noteable, noteable_ref, user)
     ::SystemNotes::IssuablesService.new(noteable: noteable, project: noteable.project, author: user).relate_issue(noteable_ref)
   end

app/services/system_notes/issuables_service.rb

@@ -111,6 +111,35 @@ module SystemNotes
       create_note(NoteSummary.new(noteable, project, author, body, action: 'reviewer'))
     end
 
+    # Called when the contacts of an issuable are changed or removed
+    # We intend to reference the contacts but for security we are just
+    # going to state how many were added/removed for now. See discussion:
+    # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77816#note_806114273
+    #
+    # added_count - number of contacts added, or 0
+    # removed_count - number of contacts  removed, or 0
+    #
+    # Example Note text:
+    #
+    #   "added 2 contacts"
+    #
+    #   "added 3 contacts and removed one contact"
+    #
+    # Returns the created Note object
+    def change_issuable_contacts(added_count, removed_count)
+      text_parts = []
+
+      Gitlab::I18n.with_default_locale do
+        text_parts << "added #{added_count} #{'contact'.pluralize(added_count)}" if added_count > 0
+        text_parts << "removed #{removed_count} #{'contact'.pluralize(removed_count)}" if removed_count > 0
+      end
+
+      return if text_parts.empty?
+
+      body = text_parts.join(' and ')
+      create_note(NoteSummary.new(noteable, project, author, body, action: 'contact'))
+    end
+
     # Called when the title of a Noteable is changed
     #
     # old_title - Previous String title

ee/app/models/ee/note.rb

@@ -68,8 +68,8 @@ module EE
       for_epic? ? noteable.group : super
     end
 
-    override :system_note_with_references_visible_for?
-    def system_note_with_references_visible_for?(user)
+    override :system_note_visible_for?
+    def system_note_visible_for?(user)
       return false unless super
 
       return true unless system_note_for_epic? && created_before_noteable?

ee/spec/models/note_spec.rb

@@ -42,12 +42,12 @@ RSpec.describe Note do
         end
 
         it 'returns visible but not readable for a non-member user' do
-          expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
+          expect(note.system_note_visible_for?(non_member)).to be_truthy
           expect(note.readable_by?(non_member)).to be_falsy
         end
 
         it 'returns visible but not readable for a nil user' do
-          expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
+          expect(note.system_note_visible_for?(nil)).to be_truthy
           expect(note.readable_by?(nil)).to be_falsy
         end
       end

lib/api/discussions.rb

@@ -252,7 +252,7 @@ module API
           .fresh
 
         # Without RendersActions#prepare_notes_for_rendering,
-        # Note#system_note_with_references_visible_for? will attempt to render
+        # Note#system_note_visible_for? will attempt to render
         # Markdown references mentioned in the note to see whether they
         # should be redacted. For notes that reference a commit, this
         # would also incur a Gitaly call to verify the commit exists.

spec/models/note_spec.rb

@@ -445,7 +445,7 @@ RSpec.describe Note do
     end
   end
 
-  describe "#system_note_with_references_visible_for?" do
+  describe "#system_note_visible_for?" do
     let(:project) { create(:project, :public) }
     let(:user) { create(:user) }
     let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user }
@@ -469,22 +469,25 @@ RSpec.describe Note do
       end
 
       it 'returns visible but not readable for non-member user' do
-        expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
+        expect(note.system_note_visible_for?(non_member)).to be_truthy
         expect(note.readable_by?(non_member)).to be_falsy
       end
 
       it 'returns visible but not readable for a nil user' do
-        expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
+        expect(note.system_note_visible_for?(nil)).to be_truthy
         expect(note.readable_by?(nil)).to be_falsy
       end
     end
   end
 
   describe "#system_note_viewable_by?(user)" do
-    let_it_be(:note) { create(:note) }
+    let_it_be(:group) { create(:group, :private) }
+    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:note) { create(:note, project: project) }
     let_it_be(:user) { create(:user) }
 
-    let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") }
+    let(:action) { "commit" }
+    let!(:metadata) { create(:system_note_metadata, note: note, action: action) }
 
     context "when system_note_metadata is not present" do
       it "returns true" do
@@ -494,16 +497,15 @@ RSpec.describe Note do
       end
     end
 
-    context "system_note_metadata isn't of type 'branch'" do
-      before do
-        metadata.action = "not_a_branch"
-      end
-
+    context "system_note_metadata isn't of type 'branch' or 'contact'" do
       it "returns true" do
         expect(note.send(:system_note_viewable_by?, user)).to be_truthy
       end
     end
 
+    context "system_note_metadata is of type 'branch'" do
+      let(:action) { "branch" }
+
       context "user doesn't have :download_code ability" do
         it "returns false" do
           expect(note.send(:system_note_viewable_by?, user)).to be_falsey
@@ -519,7 +521,26 @@ RSpec.describe Note do
       end
     end
 
-  describe "system_note_with_references_visible_for?" do
+    context "system_note_metadata is of type 'contact'" do
+      let(:action) { "contact" }
+
+      context "user doesn't have :read_crm_contact ability" do
+        it "returns false" do
+          expect(note.send(:system_note_viewable_by?, user)).to be_falsey
+        end
+      end
+
+      context "user has the :read_crm_contact ability" do
+        it "returns true" do
+          expect(Ability).to receive(:allowed?).with(user, :read_crm_contact, note.project.group).and_return(true)
+
+          expect(note.send(:system_note_viewable_by?, user)).to be_truthy
+        end
+      end
+    end
+  end
+
+  describe "system_note_visible_for?" do
     let_it_be(:private_user)    { create(:user) }
     let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
     let_it_be(:private_issue)   { create(:issue, project: private_project) }
@@ -529,11 +550,11 @@ RSpec.describe Note do
 
     shared_examples "checks references" do
       it "returns false" do
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
+        expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy
       end
 
       it "returns true" do
-        expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy
+        expect(note.system_note_visible_for?(private_user)).to be_truthy
       end
 
       it "returns true if user visible reference count set" do
@@ -541,7 +562,7 @@ RSpec.describe Note do
         note.total_reference_count = 1
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy
       end
 
       it "returns false if user visible reference count set but does not match total reference count" do
@@ -549,14 +570,14 @@ RSpec.describe Note do
         note.total_reference_count = 2
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
+        expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy
       end
 
       it "returns false if ref count is 0" do
         note.user_visible_reference_count = 0
 
         expect(note).not_to receive(:reference_mentionables)
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
+        expect(note.system_note_visible_for?(ext_issue.author)).to be_falsy
       end
     end
 
@@ -622,11 +643,11 @@ RSpec.describe Note do
       end
 
       it "returns true for other users" do
-        expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
+        expect(note.system_note_visible_for?(ext_issue.author)).to be_truthy
       end
 
       it "returns true for anonymous users" do
-        expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
+        expect(note.system_note_visible_for?(nil)).to be_truthy
       end
     end
   end

spec/services/issues/set_crm_contacts_service_spec.rb

@@ -32,6 +32,16 @@ RSpec.describe Issues::SetCrmContactsService do
       end
     end
 
+    shared_examples 'adds system note' do |added_count, removed_count|
+      it 'calls SystemNoteService.change_issuable_contacts with correct counts' do
+        expect(SystemNoteService)
+          .to receive(:change_issuable_contacts)
+          .with(issue, project, user, added_count, removed_count)
+
+        set_crm_contacts
+      end
+    end
+
     context 'when the user has no permission' do
       let(:params) { { replace_ids: [contacts[1].id, contacts[2].id] } }
 
@@ -81,6 +91,7 @@ RSpec.describe Issues::SetCrmContactsService do
         let(:expected_contacts) { [contacts[1], contacts[2]] }
 
         it_behaves_like 'setting contacts'
+        it_behaves_like 'adds system note', 1, 1
       end
 
       context 'add' do
@@ -89,6 +100,7 @@ RSpec.describe Issues::SetCrmContactsService do
         let(:expected_contacts) { [issue_contact_1, issue_contact_2, added_contact] }
 
         it_behaves_like 'setting contacts'
+        it_behaves_like 'adds system note', 1, 0
       end
 
       context 'add by email' do
@@ -99,12 +111,14 @@ RSpec.describe Issues::SetCrmContactsService do
           let(:params) { { add_emails: [contacts[3].email] } }
 
           it_behaves_like 'setting contacts'
+          it_behaves_like 'adds system note', 1, 0
         end
 
         context 'with autocomplete prefix emails in params' do
           let(:params) { { add_emails: ["[\"contact:\"#{contacts[3].email}\"]"] } }
 
           it_behaves_like 'setting contacts'
+          it_behaves_like 'adds system note', 1, 0
         end
       end
 
@@ -113,6 +127,7 @@ RSpec.describe Issues::SetCrmContactsService do
         let(:expected_contacts) { [contacts[1]] }
 
         it_behaves_like 'setting contacts'
+        it_behaves_like 'adds system note', 0, 1
       end
 
       context 'remove by email' do
@@ -122,12 +137,14 @@ RSpec.describe Issues::SetCrmContactsService do
           let(:params) { { remove_emails: [contacts[0].email] } }
 
           it_behaves_like 'setting contacts'
+          it_behaves_like 'adds system note', 0, 1
         end
 
         context 'with autocomplete prefix and suffix email in params' do
           let(:params) { { remove_emails: ["[contact:#{contacts[0].email}]"] } }
 
           it_behaves_like 'setting contacts'
+          it_behaves_like 'adds system note', 0, 1
         end
       end
 

spec/services/system_note_service_spec.rb

@@ -77,6 +77,19 @@ RSpec.describe SystemNoteService do
     end
   end
 
+  describe '.change_issuable_contacts' do
+    let(:added_count) { 5 }
+    let(:removed_count) { 3 }
+
+    it 'calls IssuableService' do
+      expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
+        expect(service).to receive(:change_issuable_contacts).with(added_count, removed_count)
+      end
+
+      described_class.change_issuable_contacts(noteable, project, author, added_count, removed_count)
+    end
+  end
+
   describe '.close_after_error_tracking_resolve' do
     it 'calls IssuableService' do
       expect_next_instance_of(::SystemNotes::IssuablesService) do |service|

spec/services/system_notes/issuables_service_spec.rb

@@ -188,6 +188,54 @@ RSpec.describe ::SystemNotes::IssuablesService do
     end
   end
 
+  describe '#change_issuable_contacts' do
+    subject { service.change_issuable_contacts(1, 1) }
+
+    let_it_be(:noteable) { create(:issue, project: project) }
+
+    it_behaves_like 'a system note' do
+      let(:action) { 'contact' }
+    end
+
+    def build_note(added_count, removed_count)
+      service.change_issuable_contacts(added_count, removed_count).note
+    end
+
+    it 'builds a correct phrase when one contact is added' do
+      expect(build_note(1, 0)).to eq "added 1 contact"
+    end
+
+    it 'builds a correct phrase when one contact is removed' do
+      expect(build_note(0, 1)).to eq "removed 1 contact"
+    end
+
+    it 'builds a correct phrase when one contact is added and one contact is removed' do
+      expect(build_note(1, 1)).to(
+        eq("added 1 contact and removed 1 contact")
+      )
+    end
+
+    it 'builds a correct phrase when three contacts are added and one removed' do
+      expect(build_note(3, 1)).to(
+        eq("added 3 contacts and removed 1 contact")
+      )
+    end
+
+    it 'builds a correct phrase when three contacts are removed and one added' do
+      expect(build_note(1, 3)).to(
+        eq("added 1 contact and removed 3 contacts")
+      )
+    end
+
+    it 'builds a correct phrase when the locale is different' do
+      Gitlab::I18n.with_locale('pt-BR') do
+        expect(build_note(1, 3)).to(
+          eq("added 1 contact and removed 3 contacts")
+        )
+      end
+    end
+  end
+
   describe '#change_status' do
     subject { service.change_status(status, source) }
 

spec/support/shared_examples/models/note_access_check_shared_examples.rb

@@ -3,7 +3,7 @@
 RSpec.shared_examples 'users with note access' do
   it 'returns true' do
     users.each do |user|
-      expect(note.system_note_with_references_visible_for?(user)).to be_truthy
+      expect(note.system_note_visible_for?(user)).to be_truthy
       expect(note.readable_by?(user)).to be_truthy
     end
   end
@@ -12,7 +12,7 @@ end
 RSpec.shared_examples 'users without note access' do
   it 'returns false' do
     users.each do |user|
-      expect(note.system_note_with_references_visible_for?(user)).to be_falsy
+      expect(note.system_note_visible_for?(user)).to be_falsy
       expect(note.readable_by?(user)).to be_falsy
     end
   end