Merge branch...

Merge branch 'snippets-with-comments-cause-a-500-when-they-show-up-in-search-results-14764' into 'master'

Fix Error 500 when searching for a comment in a project snippet

Closes #14764. /cc @stanhu, and thank you for the spec! ;)

See merge request !3468

CHANGELOG

@@ -27,6 +27,9 @@ v 8.6.3
 v 8.6.3 (unreleased)
   - Mentions on confidential issues doesn't create todos for non-members
 
+v 8.6.3 (unreleased)
+  - Fix Error 500 when searching for a comment in a project snippet. !3468
+
 v 8.6.2
   - Fix dropdown alignment. !3298
   - Fix issuable sidebar overlaps on tablet. !3299

app/models/note.rb

@@ -311,7 +311,7 @@ class Note < ActiveRecord::Base
     for_merge_request? && for_diff_line?
   end
 
-  def for_project_snippet?
+  def for_snippet?
     noteable_type == "Snippet"
   end
 

app/services/todo_service.rb

@@ -123,7 +123,7 @@ class TodoService
 
   def handle_note(note, author)
     # Skip system notes, and notes on project snippet
-    return if note.system? || note.for_project_snippet?
+    return if note.system? || note.for_snippet?
 
     project = note.project
     target  = note.noteable

app/views/search/results/_note.html.haml

 - project = note.project
+- note_url = Gitlab::UrlBuilder.new(:note).build(note.id)
+- noteable_identifier = note.noteable.try(:iid) || note.noteable.id
 .search-result-row
   %h5.note-search-caption.str-truncated
     %i.fa.fa-comment
     = link_to_member(project, note.author, avatar: false)
     commented on
+    = link_to project.name_with_namespace, project
+    ·
 
     - if note.for_commit?
-      = link_to project do
-        = project.name_with_namespace
-      ·
-      = link_to namespace_project_commit_path(project.namespace, project, note.commit_id, anchor: dom_id(note)) do
-        Commit #{truncate_sha(note.commit_id)}
+      = link_to "Commit #{truncate_sha(note.commit_id)}", note_url
     - else
-      = link_to project do
-        = project.name_with_namespace
-      ·
-      %span #{note.noteable_type.titleize} ##{note.noteable.iid}
+      %span #{note.noteable_type.titleize} ##{noteable_identifier}
       ·
-      = link_to [project.namespace.becomes(Namespace), project, note.noteable, anchor: dom_id(note)]  do
-        = note.noteable.title
+      = link_to note.noteable.title, note_url
 
   .note-search-result
     .term

lib/gitlab/note_data_builder.rb

@@ -41,7 +41,7 @@ module Gitlab
           data[:issue] = note.noteable.hook_attrs
         elsif note.for_merge_request?
           data[:merge_request] = note.noteable.hook_attrs
-        elsif note.for_project_snippet?
+        elsif note.for_snippet?
           data[:snippet] = note.noteable.hook_attrs
         end
 

lib/gitlab/url_builder.rb

@@ -2,6 +2,7 @@ module Gitlab
   class UrlBuilder
     include Gitlab::Application.routes.url_helpers
     include GitlabRoutingHelper
+    include ActionView::RecordIdentifier
 
     def initialize(type)
       @type = type
@@ -37,19 +38,16 @@ module Gitlab
         namespace_project_commit_url(namespace_id: note.project.namespace,
                                      id: note.commit_id,
                                      project_id: note.project,
-                                     anchor: "note_#{note.id}")
+                                     anchor: dom_id(note))
       elsif note.for_issue?
         issue = Issue.find(note.noteable_id)
-        issue_url(issue,
-                  anchor: "note_#{note.id}")
+        issue_url(issue, anchor: dom_id(note))
       elsif note.for_merge_request?
         merge_request = MergeRequest.find(note.noteable_id)
-        merge_request_url(merge_request,
-                          anchor: "note_#{note.id}")
-      elsif note.for_project_snippet?
+        merge_request_url(merge_request, anchor: dom_id(note))
+      elsif note.for_snippet?
         snippet = Snippet.find(note.noteable_id)
-        project_snippet_url(snippet,
-                            anchor: "note_#{note.id}")
+        project_snippet_url(snippet, anchor: dom_id(note))
       end
     end
   end

spec/features/search_spec.rb

 require 'spec_helper'
 
 describe "Search", feature: true  do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, namespace: user.namespace) }
+
   before do
-    login_as :user
-    @project = create(:project, namespace: @user.namespace)
-    @project.team << [@user, :reporter]
+    login_with(user)
+    project.team << [user, :reporter]
     visit search_path
+  end
 
+  describe 'searching for Projects' do
+    it 'finds a project' do
       page.within '.search-holder' do
-      fill_in "search", with: @project.name[0..3]
+        fill_in "search", with: project.name[0..3]
         click_button "Search"
       end
+
+      expect(page).to have_content project.name
     end
+  end
+
+  context 'search for comments' do
+    it 'finds a snippet' do
+      snippet = create(:project_snippet, :private, project: project, author: user, title: 'Some title')
+      note = create(:note,
+                    noteable: snippet,
+                    author: user,
+                    note: 'Supercalifragilisticexpialidocious',
+                    project: project)
+      # Must visit project dashboard since global search won't search
+      # everything (e.g. comments, snippets, etc.)
+      visit namespace_project_path(project.namespace, project)
 
-  it "should show project in search results" do
-    expect(page).to have_content @project.name
+      page.within '.search' do
+        fill_in 'search', with: note.note
+        click_button 'Go'
+      end
+
+      click_link 'Comments'
+
+      expect(page).to have_link(snippet.title)
+    end
   end
 end