Merge branch 'ee-jprovazn-comment-refs' into 'master'

[EE] fix label and issuable referencing in epics

Closes #5390

See merge request gitlab-org/gitlab-ee!5205

app/models/concerns/mentionable/reference_regexes.rb

@@ -10,7 +10,7 @@ module Mentionable
 
     DEFAULT_PATTERN = begin
       issue_pattern = Issue.reference_pattern
-      link_patterns = Regexp.union([Issue, Commit, MergeRequest].map(&:link_reference_pattern))
+      link_patterns = Regexp.union([Issue, Commit, MergeRequest, Epic].map(&:link_reference_pattern))
       reference_pattern(link_patterns, issue_pattern)
     end
 

app/models/note.rb

@@ -325,6 +325,10 @@ class Note < ActiveRecord::Base
     !system? && !for_snippet?
   end
 
+  def can_create_notification?
+    true
+  end
+
   def discussion_class(noteable = nil)
     # When commit notes are rendered on an MR's Discussion page, they are
     # displayed in one discussion instead of individually.

app/services/notes/post_process_service.rb

@@ -11,7 +11,7 @@ module Notes
       unless @note.system?
         EventCreateService.new.leave_note(@note, @note.author)
 
-        return unless @note.for_project_noteable?
+        return if @note.for_personal_snippet?
 
         @note.create_cross_references!
         execute_note_hooks
@@ -23,6 +23,8 @@ module Notes
     end
 
     def execute_note_hooks
+      return unless @note.project
+
       note_data = hook_data
       hooks_scope = @note.confidential? ? :confidential_note_hooks : :note_hooks
 

app/services/system_note_service.rb

@@ -429,7 +429,7 @@ module SystemNoteService
   def cross_reference(noteable, mentioner, author)
     return if cross_reference_disallowed?(noteable, mentioner)
 
-    gfm_reference = mentioner.gfm_reference(noteable.project)
+    gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group)
     body = cross_reference_note_content(gfm_reference)
 
     if noteable.is_a?(ExternalIssue)
@@ -687,7 +687,7 @@ module SystemNoteService
       text = "#{cross_reference_note_prefix}%#{mentioner.to_reference(nil)}"
       notes.where('(note LIKE ? OR note LIKE ?)', text, text.capitalize)
     else
-      gfm_reference = mentioner.gfm_reference(noteable.project)
+      gfm_reference = mentioner.gfm_reference(noteable.project || noteable.group)
       text = cross_reference_note_content(gfm_reference)
       notes.where(note: [text, text.capitalize])
     end

app/workers/new_note_worker.rb

@@ -5,7 +5,7 @@ class NewNoteWorker
   # old `NewNoteWorker` jobs (can remove later)
   def perform(note_id, _params = {})
     if note = Note.find_by(id: note_id)
-      NotificationService.new.new_note(note)
+      NotificationService.new.new_note(note) if note.can_create_notification?
       Notes::PostProcessService.new(note).execute
     else
       Rails.logger.error("NewNoteWorker: couldn't find note with ID=#{note_id}, skipping job")

ee/app/models/ee/epic.rb

@@ -114,11 +114,15 @@ module EE
     end
 
     def mentionable_params
-      { group: group }
+      { group: group, label_url_method: :group_epics_url }
     end
 
     def discussions_rendered_on_frontend?
       true
     end
+
+    def banzai_render_context(field)
+      super.merge(label_url_method: :group_epics_url)
+    end
   end
 end

ee/app/models/ee/note.rb

@@ -21,6 +21,11 @@ module EE
       !for_epic? && super
     end
 
+    override :can_create_notification?
+    def can_create_notification?
+      !for_epic? && super
+    end
+
     override :etag_key
     def etag_key
       if for_epic?
@@ -34,7 +39,20 @@ module EE
     def banzai_render_context(field)
       return super unless for_epic?
 
-      super.merge(group: noteable.group)
+      super.merge(banzai_context_params)
+    end
+
+    override :mentionable_params
+    def mentionable_params
+      return super unless for_epic?
+
+      super.merge(banzai_context_params)
+    end
+
+    private
+
+    def banzai_context_params
+      { group: noteable.group, label_url_method: :group_epics_url }
     end
   end
 end

ee/changelogs/unreleased/ee-jprovazn-comment-refs.yml

+---
+title: Fix label and issuable referencing in epics and epic notes
+merge_request:
+author:
+type: fixed

ee/lib/ee/banzai/filter/epic_reference_filter.rb

@@ -42,12 +42,6 @@ module EE
 
         private
 
-        def full_group_path(group_ref)
-          return current_parent_path unless group_ref
-
-          group_ref
-        end
-
         def parent_type
           :group
         end

ee/spec/features/epics/epic_labels_spec.rb

+require 'spec_helper'
+
+describe 'Assign labels to an epic', :js do
+  let(:user) { create(:user) }
+  let(:group) { create(:group, :public) }
+  let(:label) { create(:group_label, group: group, title: 'bug') }
+  let(:epic) { create(:epic, group: group) }
+
+  before do
+    group.add_developer(user)
+    stub_licensed_features(epics: true)
+    sign_in(user)
+
+    visit group_epic_path(group, epic)
+  end
+
+  context 'when label is referenced' do
+    before do
+      fill_in 'note[note]', with: "refer ~#{label.name}"
+      click_button 'Comment'
+
+      wait_for_requests
+    end
+
+    it 'creates new system note with label pointing to epics index page' do
+      page.within('div#notes li.note div.note-text') do
+        expect(page).to have_content("refer #{label.name}")
+        expect(page.find('a')).to have_content(label.name)
+        expect(page).to have_link(label.name, href: group_epics_path(group, label_name: label.name))
+      end
+    end
+  end
+end

ee/spec/features/epics/referencing_epics_spec.rb

@@ -60,4 +60,69 @@ describe 'Referencing Epics', :js do
       end
     end
   end
+
+  context 'note cross-referencing' do
+    let(:issue) { create(:issue, project: project) }
+
+    before do
+      stub_licensed_features(epics: true)
+      group.add_developer(user)
+
+      sign_in(user)
+    end
+
+    context 'when referencing an epic from an issue note' do
+      let(:note_text) { "Check #{epic.to_reference(full: true)}" }
+
+      before do
+        visit project_issue_path(project, issue)
+
+        fill_in 'note[note]', with: note_text
+        click_button 'Comment'
+
+        wait_for_requests
+      end
+
+      it 'creates a note with reference and cross references the epic' do
+        page.within('div#notes li.note div.note-text') do
+          expect(page).to have_content(note_text)
+          expect(page.find('a')).to have_content(epic.to_reference(full: true))
+        end
+
+        find('div#notes li.note div.note-text a').click
+
+        page.within('div#notes li.note .system-note-message') do
+          expect(page).to have_content('mentioned in issue')
+          expect(page.find('a')).to have_content(issue.to_reference(full: true))
+        end
+      end
+
+      context 'when referencing an issue from an epic' do
+        let(:note_text) { "Check #{issue.to_reference(full: true)}" }
+
+        before do
+          visit group_epic_path(group, epic)
+
+          fill_in 'note[note]', with: note_text
+          click_button 'Comment'
+
+          wait_for_requests
+        end
+
+        it 'creates a note with reference and cross references the issue' do
+          page.within('div#notes li.note div.note-text') do
+            expect(page).to have_content(note_text)
+            expect(page.find('a')).to have_content(issue.to_reference(full: true))
+          end
+
+          find('div#notes li.note div.note-text a').click
+
+          page.within('div#notes li.note .system-note-message') do
+            expect(page).to have_content('mentioned in epic')
+            expect(page.find('a')).to have_content(epic.to_reference(full: true))
+          end
+        end
+      end
+    end
+  end
 end

lib/banzai/cross_project_reference.rb

@@ -4,7 +4,7 @@ module Banzai
   module CrossProjectReference
     # Given a cross-project reference string, get the Project record
     #
-    # Defaults to value of `context[:project]` if:
+    # Defaults to value of `context[:project]`, or `context[:group]` if:
     # * No reference is given OR
     # * Reference given doesn't exist
     #
@@ -12,7 +12,7 @@ module Banzai
     #
     # Returns a Project, or nil if the reference can't be found
     def parent_from_ref(ref)
-      return context[:project] unless ref
+      return context[:project] || context[:group] unless ref
 
       Project.find_by_full_path(ref)
     end

lib/banzai/filter/abstract_reference_filter.rb

@@ -196,13 +196,15 @@ module Banzai
         end
       end
 
-      def data_attributes_for(text, project, object, link_content: false, link_reference: false)
+      def data_attributes_for(text, parent, object, link_content: false, link_reference: false)
+        object_parent_type = parent.is_a?(Group) ? :group : :project
+
         data_attribute(
-          original:       text,
-          link:           link_content,
-          link_reference: link_reference,
-          project:        project.id,
-          object_sym =>   object.id
+          original:             text,
+          link:                 link_content,
+          link_reference:       link_reference,
+          object_parent_type => parent.id,
+          object_sym =>         object.id
         )
       end
 
@@ -337,6 +339,12 @@ module Banzai
       def parent
         parent_type == :project ? project : group
       end
+
+      def full_group_path(group_ref)
+        return current_parent_path unless group_ref
+
+        group_ref
+      end
     end
   end
 end

lib/banzai/filter/label_reference_filter.rb

@@ -32,16 +32,25 @@ module Banzai
         end
       end
 
-      def find_label(project_ref, label_id, label_name)
-        project = parent_from_ref(project_ref)
-        return unless project
+      def find_label(parent_ref, label_id, label_name)
+        parent = parent_from_ref(parent_ref)
+        return unless parent
 
         label_params = label_params(label_id, label_name)
-        find_labels(project).find_by(label_params)
+        find_labels(parent).find_by(label_params)
       end
 
-      def find_labels(project)
-        LabelsFinder.new(nil, project_id: project.id, include_ancestor_groups: true).execute(skip_authorization: true)
+      def find_labels(parent)
+        params = if parent.is_a?(Group)
+                   { group_id: parent.id,
+                     include_ancestor_groups: true,
+                     only_group_labels: true }
+                 else
+                   { project_id: parent.id,
+                     include_ancestor_groups: true }
+                 end
+
+        LabelsFinder.new(nil, params).execute(skip_authorization: true)
       end
 
       # Parameters to pass to `Label.find_by` based on the given arguments
@@ -59,20 +68,34 @@ module Banzai
         end
       end
 
-      def url_for_object(label, project)
+      def url_for_object(label, parent)
         h = Gitlab::Routing.url_helpers
-        h.project_issues_url(project, label_name: label.name, only_path: context[:only_path])
+
+        if parent.is_a?(Project)
+          h.project_issues_url(parent, label_name: label.name, only_path: context[:only_path])
+        elsif context[:label_url_method]
+          h.public_send(context[:label_url_method], parent, label_name: label.name, only_path: context[:only_path]) # rubocop:disable GitlabSecurity/PublicSend
+        end
       end
 
       def object_link_text(object, matches)
-        project_path     = full_project_path(matches[:namespace], matches[:project])
-        project_from_ref = from_ref_cached(project_path)
-        reference        = project_from_ref.to_human_reference(project)
-        label_suffix     = " <i>in #{reference}</i>" if reference.present?
+        label_suffix = ''
+
+        if project || full_path_ref?(matches)
+          project_path    = full_project_path(matches[:namespace], matches[:project])
+          parent_from_ref = from_ref_cached(project_path)
+          reference       = parent_from_ref.to_human_reference(project || group)
+
+          label_suffix = " <i>in #{reference}</i>" if reference.present?
+        end
 
         LabelsHelper.render_colored_label(object, label_suffix)
       end
 
+      def full_path_ref?(matches)
+        matches[:namespace] && matches[:project]
+      end
+
       def unescape_html_entities(text)
         CGI.unescapeHTML(text.to_s)
       end

spec/lib/banzai/cross_project_reference_spec.rb

@@ -14,6 +14,16 @@ describe Banzai::CrossProjectReference do
       end
     end
 
+    context 'when no project was referenced in group context' do
+      it 'returns the group from context' do
+        group = double
+
+        allow(self).to receive(:context).and_return({ group: group })
+
+        expect(parent_from_ref(nil)).to eq group
+      end
+    end
+
     context 'when referenced project does not exist' do
       it 'returns nil' do
         expect(parent_from_ref('invalid/reference')).to be_nil

spec/lib/banzai/filter/label_reference_filter_spec.rb

@@ -596,6 +596,27 @@ describe Banzai::Filter::LabelReferenceFilter do
   end
 
   describe 'group context' do
+    it 'points to the page defined in label_url_method' do
+      group = create(:group)
+      label = create(:group_label, group: group)
+      reference = "~#{label.name}"
+
+      result = reference_filter("See #{reference}", { project: nil, group: group, label_url_method: :group_url } )
+
+      expect(result.css('a').first.attr('href')).to eq(urls.group_url(group, label_name: label.name))
+    end
+
+    it 'finds labels also in ancestor groups' do
+      group = create(:group)
+      label = create(:group_label, group: group)
+      subgroup = create(:group, parent: group)
+      reference = "~#{label.name}"
+
+      result = reference_filter("See #{reference}", { project: nil, group: subgroup, label_url_method: :group_url } )
+
+      expect(result.css('a').first.attr('href')).to eq(urls.group_url(subgroup, label_name: label.name))
+    end
+
     it 'points to referenced project issues page' do
       project = create(:project)
       label = create(:label, project: project)
@@ -604,6 +625,7 @@ describe Banzai::Filter::LabelReferenceFilter do
       result = reference_filter("See #{reference}", { project: nil, group: create(:group) } )
 
       expect(result.css('a').first.attr('href')).to eq(urls.project_issues_url(project, label_name: label.name))
+      expect(result.css('a').first.text).to eq "#{label.name} in #{project.full_name}"
     end
   end
 end

spec/lib/gitlab/reference_extractor_spec.rb

@@ -170,7 +170,7 @@ describe Gitlab::ReferenceExtractor do
     @e1 = create(:epic, group: group)
     @e2 = create(:epic, group: create(:group, :private))
 
-    text = "#{@e0.to_reference(group)}, &999, #{@e1.to_reference(group)}, #{@e2.to_reference(group)}"
+    text = "#{@e0.to_reference(group, full: true)}, &999, #{@e1.to_reference(group, full: true)}, #{@e2.to_reference(group, full: true)}"
 
     subject.analyze(text, { group: group })