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

Better group support in notes-related code

See merge request gitlab-org/gitlab-ce!18150

app/models/note.rb

@@ -317,6 +317,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)
@@ -582,7 +582,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")

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