Merge branch '344854-fix-cross-references-naming' into 'master'

Fix incorrect naming in code related to cross-references

See merge request gitlab-org/gitlab!74903

app/models/integrations/base_issue_tracker.rb

@@ -128,7 +128,7 @@ module Integrations
       false
     end
 
-    def create_cross_reference_note(mentioned, noteable, author)
+    def create_cross_reference_note(external_issue, mentioned_in, author)
       # implement inside child
     end
 

app/models/integrations/jira.rb

@@ -234,19 +234,19 @@ module Integrations
     end
 
     override :create_cross_reference_note
-    def create_cross_reference_note(mentioned, noteable, author)
-      unless can_cross_reference?(noteable)
-        return s_("JiraService|Events for %{noteable_model_name} are disabled.") % { noteable_model_name: noteable.model_name.plural.humanize(capitalize: false) }
+    def create_cross_reference_note(external_issue, mentioned_in, author)
+      unless can_cross_reference?(mentioned_in)
+        return s_("JiraService|Events for %{noteable_model_name} are disabled.") % { noteable_model_name: mentioned_in.model_name.plural.humanize(capitalize: false) }
       end
 
-      jira_issue = find_issue(mentioned.id)
+      jira_issue = find_issue(external_issue.id)
 
       return unless jira_issue.present?
 
-      noteable_id   = noteable.respond_to?(:iid) ? noteable.iid : noteable.id
-      noteable_type = noteable_name(noteable)
-      entity_url    = build_entity_url(noteable_type, noteable_id)
-      entity_meta   = build_entity_meta(noteable)
+      mentioned_in_id = mentioned_in.respond_to?(:iid) ? mentioned_in.iid : mentioned_in.id
+      mentioned_in_type = mentionable_name(mentioned_in)
+      entity_url = build_entity_url(mentioned_in_type, mentioned_in_id)
+      entity_meta = build_entity_meta(mentioned_in)
 
       data = {
         user: {
@@ -259,9 +259,9 @@ module Integrations
         },
         entity: {
           id: entity_meta[:id],
-          name: noteable_type.humanize.downcase,
+          name: mentioned_in_type.humanize.downcase,
           url: entity_url,
-          title: noteable.title,
+          title: mentioned_in.title,
           description: entity_meta[:description],
           branch: entity_meta[:branch]
         }
@@ -302,11 +302,11 @@ module Integrations
 
     private
 
-    def branch_name(noteable)
+    def branch_name(commit)
       if Feature.enabled?(:jira_use_first_ref_by_oid, project, default_enabled: :yaml)
-        noteable.first_ref_by_oid(project.repository)
+        commit.first_ref_by_oid(project.repository)
       else
-        noteable.ref_names(project.repository).first
+        commit.ref_names(project.repository).first
       end
     end
 
@@ -316,8 +316,8 @@ module Integrations
       end
     end
 
-    def can_cross_reference?(noteable)
-      case noteable
+    def can_cross_reference?(mentioned_in)
+      case mentioned_in
       when Commit then commit_events
       when MergeRequest then merge_requests_events
       else true
@@ -487,36 +487,36 @@ module Integrations
       "#{Settings.gitlab.base_url.chomp("/")}#{resource}"
     end
 
-    def build_entity_url(noteable_type, entity_id)
+    def build_entity_url(entity_type, entity_id)
       polymorphic_url(
         [
           self.project,
-          noteable_type.to_sym
+          entity_type.to_sym
         ],
         id:   entity_id,
         host: Settings.gitlab.base_url
       )
     end
 
-    def build_entity_meta(noteable)
-      if noteable.is_a?(Commit)
+    def build_entity_meta(entity)
+      if entity.is_a?(Commit)
         {
-          id: noteable.short_id,
-          description: noteable.safe_message,
-          branch: branch_name(noteable)
+          id: entity.short_id,
+          description: entity.safe_message,
+          branch: branch_name(entity)
         }
-      elsif noteable.is_a?(MergeRequest)
+      elsif entity.is_a?(MergeRequest)
         {
-          id: noteable.to_reference,
-          branch: noteable.source_branch
+          id: entity.to_reference,
+          branch: entity.source_branch
         }
       else
         {}
       end
     end
 
-    def noteable_name(noteable)
-      name = noteable.model_name.singular
+    def mentionable_name(mentionable)
+      name = mentionable.model_name.singular
 
       # ProjectSnippet inherits from Snippet class so it causes
       # routing error building the URL.

app/services/system_note_service.rb

@@ -213,12 +213,12 @@ module SystemNoteService
     ::SystemNotes::MergeRequestsService.new(noteable: issue, project: project, author: author).new_merge_request(merge_request)
   end
 
-  def cross_reference(noteable, mentioner, author)
-    ::SystemNotes::IssuablesService.new(noteable: noteable, author: author).cross_reference(mentioner)
+  def cross_reference(mentioned, mentioned_in, author)
+    ::SystemNotes::IssuablesService.new(noteable: mentioned, author: author).cross_reference(mentioned_in)
   end
 
-  def cross_reference_exists?(noteable, mentioner)
-    ::SystemNotes::IssuablesService.new(noteable: noteable).cross_reference_exists?(mentioner)
+  def cross_reference_exists?(mentioned, mentioned_in)
+    ::SystemNotes::IssuablesService.new(noteable: mentioned).cross_reference_exists?(mentioned_in)
   end
 
   def change_task_status(noteable, project, author, new_task)
@@ -249,8 +249,8 @@ module SystemNoteService
     ::SystemNotes::IssuablesService.new(noteable: issuable, project: issuable.project, author: author).discussion_lock
   end
 
-  def cross_reference_disallowed?(noteable, mentioner)
-    ::SystemNotes::IssuablesService.new(noteable: noteable).cross_reference_disallowed?(mentioner)
+  def cross_reference_disallowed?(mentioned, mentioned_in)
+    ::SystemNotes::IssuablesService.new(noteable: mentioned).cross_reference_disallowed?(mentioned_in)
   end
 
   def zoom_link_added(issue, project, author)

app/services/system_notes/issuables_service.rb

@@ -154,9 +154,8 @@ module SystemNotes
       create_note(NoteSummary.new(noteable, project, author, body, action: 'description'))
     end
 
-    # Called when a Mentionable references a Noteable
-    #
-    # mentioner - Mentionable object
+    # Called when a Mentionable (the `mentioned_in`) references another Mentionable (the `mentioned`,
+    # passed to this service as `noteable`).
     #
     # Example Note text:
     #
@@ -168,19 +167,20 @@ module SystemNotes
     #
     # See cross_reference_note_content.
     #
-    # Returns the created Note object
-    def cross_reference(mentioner)
-      return if cross_reference_disallowed?(mentioner)
+    # @param mentioned_in [Mentionable]
+    # @return [Note]
+    def cross_reference(mentioned_in)
+      return if cross_reference_disallowed?(mentioned_in)
 
-      gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group)
+      gfm_reference = mentioned_in.gfm_reference(noteable.project || noteable.group)
       body = cross_reference_note_content(gfm_reference)
 
       if noteable.is_a?(ExternalIssue)
         Integrations::CreateExternalCrossReferenceWorker.perform_async(
           noteable.project_id,
           noteable.id,
-          mentioner.class.name,
-          mentioner.id,
+          mentioned_in.class.name,
+          mentioned_in.id,
           author.id
         )
       else
@@ -195,15 +195,14 @@ module SystemNotes
     # in a merge request. Additionally, it prevents the creation of references to
     # external issues (which would fail).
     #
-    # mentioner - Mentionable object
-    #
-    # Returns Boolean
-    def cross_reference_disallowed?(mentioner)
+    # @param mentioned_in [Mentionable]
+    # @return [Boolean]
+    def cross_reference_disallowed?(mentioned_in)
       return true if noteable.is_a?(ExternalIssue) && !noteable.project&.external_references_supported?
-      return false unless mentioner.is_a?(MergeRequest)
+      return false unless mentioned_in.is_a?(MergeRequest)
       return false unless noteable.is_a?(Commit)
 
-      mentioner.commits.include?(noteable)
+      mentioned_in.commits.include?(noteable)
     end
 
     # Called when the status of a Task has changed
@@ -309,19 +308,19 @@ module SystemNotes
       create_resource_state_event(status: status, mentionable_source: source)
     end
 
-    # Check if a cross reference to a noteable from a mentioner already exists
+    # Check if a cross reference to a Mentionable from the `mentioned_in` Mentionable
+    # already exists.
     #
     # This method is used to prevent multiple notes being created for a mention
-    # when a issue is updated, for example. The method also calls notes_for_mentioner
-    # to check if the mentioner is a commit, and return matches only on commit hash
+    # when a issue is updated, for example. The method also calls `existing_mentions_for`
+    # to check if the mention is in a commit, and return matches only on commit hash
     # instead of project + commit, to avoid repeated mentions from forks.
     #
-    # mentioner - Mentionable object
-    #
-    # Returns Boolean
-    def cross_reference_exists?(mentioner)
+    # @param mentioned_in [Mentionable]
+    # @return [Boolean]
+    def cross_reference_exists?(mentioned_in)
       notes = noteable.notes.system
-      notes_for_mentioner(mentioner, noteable, notes).exists?
+      existing_mentions_for(mentioned_in, noteable, notes).exists?
     end
 
     # Called when a Noteable has been marked as a duplicate of another Issue
@@ -398,12 +397,12 @@ module SystemNotes
       "#{self.class.cross_reference_note_prefix}#{gfm_reference}"
     end
 
-    def notes_for_mentioner(mentioner, noteable, notes)
-      if mentioner.is_a?(Commit)
-        text = "#{self.class.cross_reference_note_prefix}%#{mentioner.to_reference(nil)}"
+    def existing_mentions_for(mentioned_in, noteable, notes)
+      if mentioned_in.is_a?(Commit)
+        text = "#{self.class.cross_reference_note_prefix}%#{mentioned_in.to_reference(nil)}"
         notes.like_note_or_capitalized_note(text)
       else
-        gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group)
+        gfm_reference = mentioned_in.gfm_reference(noteable.project || noteable.group)
         text = cross_reference_note_content(gfm_reference)
         notes.for_note_or_capitalized_note(text)
       end

ee/spec/services/ee/system_notes/issuables_service_spec.rb

@@ -62,9 +62,9 @@ RSpec.describe ::SystemNotes::IssuablesService do
   end
 
   describe '#cross_reference' do
-    let(:mentioner) { create(:issue, project: project) }
+    let(:mentioned_in) { create(:issue, project: project) }
 
-    subject { service.cross_reference(mentioner) }
+    subject { service.cross_reference(mentioned_in) }
 
     context 'when noteable is an epic' do
       let(:noteable) { epic }

spec/models/integrations/jira_spec.rb

@@ -863,7 +863,7 @@ RSpec.describe Integrations::Jira do
     subject { jira_integration.create_cross_reference_note(jira_issue, resource, user) }
 
     shared_examples 'handles cross-references' do
-      let(:resource_name) { jira_integration.send(:noteable_name, resource) }
+      let(:resource_name) { jira_integration.send(:mentionable_name, resource) }
       let(:resource_url) { jira_integration.send(:build_entity_url, resource_name, resource.to_param) }
       let(:issue_url) { "#{url}/rest/api/2/issue/JIRA-123" }
       let(:comment_url) { "#{issue_url}/comment" }

spec/services/system_note_service_spec.rb

@@ -287,38 +287,38 @@ RSpec.describe SystemNoteService do
   end
 
   describe '.cross_reference' do
-    let(:mentioner) { double }
+    let(:mentioned_in) { double }
 
     it 'calls IssuableService' do
       expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
-        expect(service).to receive(:cross_reference).with(mentioner)
+        expect(service).to receive(:cross_reference).with(mentioned_in)
       end
 
-      described_class.cross_reference(double, mentioner, double)
+      described_class.cross_reference(double, mentioned_in, double)
     end
   end
 
   describe '.cross_reference_disallowed?' do
-    let(:mentioner) { double }
+    let(:mentioned_in) { double }
 
     it 'calls IssuableService' do
       expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
-        expect(service).to receive(:cross_reference_disallowed?).with(mentioner)
+        expect(service).to receive(:cross_reference_disallowed?).with(mentioned_in)
       end
 
-      described_class.cross_reference_disallowed?(double, mentioner)
+      described_class.cross_reference_disallowed?(double, mentioned_in)
     end
   end
 
   describe '.cross_reference_exists?' do
-    let(:mentioner) { double }
+    let(:mentioned_in) { double }
 
     it 'calls IssuableService' do
       expect_next_instance_of(::SystemNotes::IssuablesService) do |service|
-        expect(service).to receive(:cross_reference_exists?).with(mentioner)
+        expect(service).to receive(:cross_reference_exists?).with(mentioned_in)
       end
 
-      described_class.cross_reference_exists?(double, mentioner)
+      described_class.cross_reference_exists?(double, mentioned_in)
     end
   end
 

spec/services/system_notes/issuables_service_spec.rb

@@ -274,9 +274,9 @@ RSpec.describe ::SystemNotes::IssuablesService do
   describe '#cross_reference' do
     let(:service) { described_class.new(noteable: noteable, author: author) }
 
-    let(:mentioner) { create(:issue, project: project) }
+    let(:mentioned_in) { create(:issue, project: project) }
 
-    subject { service.cross_reference(mentioner) }
+    subject { service.cross_reference(mentioned_in) }
 
     it_behaves_like 'a system note' do
       let(:action) { 'cross_reference' }
@@ -314,35 +314,35 @@ RSpec.describe ::SystemNotes::IssuablesService do
       describe 'note_body' do
         context 'cross-project' do
           let(:project2) { create(:project, :repository) }
-          let(:mentioner) { create(:issue, project: project2) }
+          let(:mentioned_in) { create(:issue, project: project2) }
 
           context 'from Commit' do
-            let(:mentioner) { project2.repository.commit }
+            let(:mentioned_in) { project2.repository.commit }
 
             it 'references the mentioning commit' do
-              expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference(project)}"
+              expect(subject.note).to eq "mentioned in commit #{mentioned_in.to_reference(project)}"
             end
           end
 
           context 'from non-Commit' do
             it 'references the mentioning object' do
-              expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference(project)}"
+              expect(subject.note).to eq "mentioned in issue #{mentioned_in.to_reference(project)}"
             end
           end
         end
 
         context 'within the same project' do
           context 'from Commit' do
-            let(:mentioner) { project.repository.commit }
+            let(:mentioned_in) { project.repository.commit }
 
             it 'references the mentioning commit' do
-              expect(subject.note).to eq "mentioned in commit #{mentioner.to_reference}"
+              expect(subject.note).to eq "mentioned in commit #{mentioned_in.to_reference}"
             end
           end
 
           context 'from non-Commit' do
             it 'references the mentioning object' do
-              expect(subject.note).to eq "mentioned in issue #{mentioner.to_reference}"
+              expect(subject.note).to eq "mentioned in issue #{mentioned_in.to_reference}"
             end
           end
         end
@@ -350,14 +350,14 @@ RSpec.describe ::SystemNotes::IssuablesService do
 
       context 'with external issue' do
         let(:noteable) { ExternalIssue.new('JIRA-123', project) }
-        let(:mentioner) { project.commit }
+        let(:mentioned_in) { project.commit }
 
         it 'queues a background worker' do
           expect(Integrations::CreateExternalCrossReferenceWorker).to receive(:perform_async).with(
             project.id,
             'JIRA-123',
             'Commit',
-            mentioner.id,
+            mentioned_in.id,
             author.id
           )
 
@@ -716,28 +716,28 @@ RSpec.describe ::SystemNotes::IssuablesService do
   end
 
   describe '#cross_reference_disallowed?' do
-    context 'when mentioner is not a MergeRequest' do
+    context 'when mentioned_in is not a MergeRequest' do
       it 'is falsey' do
-        mentioner = noteable.dup
+        mentioned_in = noteable.dup
 
-        expect(service.cross_reference_disallowed?(mentioner)).to be_falsey
+        expect(service.cross_reference_disallowed?(mentioned_in)).to be_falsey
       end
     end
 
-    context 'when mentioner is a MergeRequest' do
-      let(:mentioner) { create(:merge_request, :simple, source_project: project) }
-      let(:noteable)  { project.commit }
+    context 'when mentioned_in is a MergeRequest' do
+      let(:mentioned_in) { create(:merge_request, :simple, source_project: project) }
+      let(:noteable) { project.commit }
 
       it 'is truthy when noteable is in commits' do
-        expect(mentioner).to receive(:commits).and_return([noteable])
+        expect(mentioned_in).to receive(:commits).and_return([noteable])
 
-        expect(service.cross_reference_disallowed?(mentioner)).to be_truthy
+        expect(service.cross_reference_disallowed?(mentioned_in)).to be_truthy
       end
 
       it 'is falsey when noteable is not in commits' do
-        expect(mentioner).to receive(:commits).and_return([])
+        expect(mentioned_in).to receive(:commits).and_return([])
 
-        expect(service.cross_reference_disallowed?(mentioner)).to be_falsey
+        expect(service.cross_reference_disallowed?(mentioned_in)).to be_falsey
       end
     end