Address review comments

app/controllers/concerns/renders_notes.rb

@@ -0,0 +1,20 @@
+module RendersNotes
+  def prepare_notes_for_rendering(notes)
+    preload_noteable_for_regular_notes(notes)
+    preload_max_access_for_authors(notes, @project)
+    Banzai::NoteRenderer.render(notes, @project, current_user)
+
+    notes
+  end
+
+  private
+
+  def preload_max_access_for_authors(notes, project)
+    user_ids = notes.map(&:author_id)
+    project.team.max_member_access_for_user_ids(user_ids)
+  end
+
+  def preload_noteable_for_regular_notes(notes)
+    ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
+  end
+end

app/controllers/projects/commit_controller.rb

@@ -2,7 +2,7 @@
 #
 # Not to be confused with CommitsController, plural.
 class Projects::CommitController < Projects::ApplicationController
-  include NotesHelper
+  include RendersNotes
   include CreatesCommit
   include DiffForPath
   include DiffHelper

app/controllers/projects/issues_controller.rb

@@ -1,5 +1,5 @@
 class Projects::IssuesController < Projects::ApplicationController
-  include NotesHelper
+  include RendersNotes
   include ToggleSubscriptionAction
   include IssuableActions
   include ToggleAwardEmoji

app/controllers/projects/merge_requests_controller.rb

@@ -3,7 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   include DiffForPath
   include DiffHelper
   include IssuableActions
-  include NotesHelper
+  include RendersNotes
   include ToggleAwardEmoji
   include IssuableCollections
 

app/controllers/projects/notes_controller.rb

@@ -1,5 +1,5 @@
 class Projects::NotesController < Projects::ApplicationController
-  include NotesHelper
+  include RendersNotes
   include ToggleAwardEmoji
 
   # Authorize

app/controllers/projects/snippets_controller.rb

@@ -1,5 +1,5 @@
 class Projects::SnippetsController < Projects::ApplicationController
-  include NotesHelper
+  include RendersNotes
   include ToggleAwardEmoji
   include SpammableActions
   include SnippetsActions

app/finders/notes_finder.rb

@@ -20,9 +20,9 @@ class NotesFinder
   end
 
   def execute
-    @notes = init_collection
-    @notes = since_fetch_at(@params[:last_fetched_at], @notes) if @params[:last_fetched_at]
-    @notes
+    notes = init_collection
+    notes = since_fetch_at(notes)
+    notes.fresh
   end
 
   def target
@@ -56,7 +56,7 @@ class NotesFinder
   def notes_of_any_type
     types = %w(commit issue merge_request snippet)
     note_relations = types.map { |t| notes_for_type(t) }
-    note_relations.map! { |notes| search(@params[:search], notes) } if @params[:search]
+    note_relations.map! { |notes| search(notes) }
     UnionFinder.new.find_union(note_relations, Note)
   end
 
@@ -98,16 +98,21 @@ class NotesFinder
   #
   # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
   #
-  def search(query, notes_relation = @notes)
+  def search(query, notes)
+    query = @params[:search]
+    return unless query
+
     pattern = "%#{query}%"
-    notes_relation.where(Note.arel_table[:note].matches(pattern))
+    notes.where(Note.arel_table[:note].matches(pattern))
   end
 
   # Notes changed since last fetch
   # Uses overlapping intervals to avoid worrying about race conditions
-  def since_fetch_at(fetch_time, notes_relation = @notes)
+  def since_fetch_at(notes)
+    return notes unless @params[:last_fetched_at]
+
     # Default to 0 to remain compatible with old clients
     last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
-    notes_relation.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
+    notes.updated_after(last_fetched_at - FETCH_OVERLAP)
   end
 end

app/helpers/notes_helper.rb

@@ -57,23 +57,6 @@ module NotesHelper
                            data: data, title: 'Add a reply'
   end
 
-  def preload_max_access_for_authors(notes, project)
-    user_ids = notes.map(&:author_id)
-    project.team.max_member_access_for_user_ids(user_ids)
-  end
-
-  def preload_noteable_for_regular_notes(notes)
-    ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
-  end
-
-  def prepare_notes_for_rendering(notes)
-    preload_noteable_for_regular_notes(notes)
-    preload_max_access_for_authors(notes, @project)
-    Banzai::NoteRenderer.render(notes, @project, current_user)
-
-    notes
-  end
-
   def note_max_access_for_user(note)
     note.project.team.human_max_access(note.author_id)
   end

app/mailers/emails/notes.rb

@@ -4,7 +4,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @commit = @note.noteable
-      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_commit_url(*note_target_url_options)
 
       mail_answer_thread(@commit,
@@ -17,7 +16,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @issue = @note.noteable
-      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_issue_url(*note_target_url_options)
       mail_answer_thread(@issue, note_thread_options(recipient_id))
     end
@@ -26,7 +24,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @merge_request = @note.noteable
-      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_merge_request_url(*note_target_url_options)
       mail_answer_thread(@merge_request, note_thread_options(recipient_id))
     end
@@ -35,7 +32,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @snippet = @note.noteable
-      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = namespace_project_snippet_url(*note_target_url_options)
       mail_answer_thread(@snippet, note_thread_options(recipient_id))
     end
@@ -44,7 +40,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @snippet = @note.noteable
-      @discussion = @note.discussion if @note.part_of_discussion?
       @target_url = snippet_url(@note.noteable)
       mail_answer_thread(@snippet, note_thread_options(recipient_id))
     end

app/models/concerns/resolvable_discussion.rb

@@ -2,12 +2,14 @@ module ResolvableDiscussion
   extend ActiveSupport::Concern
 
   included do
-    memoized_values << :resolvable
-    memoized_values << :resolved
-    memoized_values << :first_note
-    memoized_values << :first_note_to_resolve
-    memoized_values << :last_resolved_note
-    memoized_values << :last_note
+    memoized_values.push(
+      :resolvable,
+      :resolved,
+      :first_note,
+      :first_note_to_resolve,
+      :last_resolved_note,
+      :last_note
+    )
 
     delegate  :resolved_at,
               :resolved_by,

app/models/concerns/resolvable_note.rb

@@ -8,7 +8,9 @@ module ResolvableNote
 
     validates :resolved_by, presence: true, if: :resolved?
 
-    # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable
+    # Keep this scope in sync with the logic in `#potentially_resolvable?` in `Discussion` subclasses that are resolvable.
+    # `RESOLVABLE_TYPES` should include names of all subclasses that are resolvable (where the method can return true), and
+    # the scope should also match the criteria `ResolvableDiscussion#potentially_resolvable?` puts on resolvability.
     scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') }
     # Keep this scope in sync with `#resolvable?`
     scope :resolvable, -> { potentially_resolvable.user }

app/models/diff_discussion.rb

@@ -1,3 +1,4 @@
+# A discussion on merge request or commit diffs consisting of `DiffNote` notes
 class DiffDiscussion < Discussion
   include DiscussionOnDiff
 

app/models/diff_note.rb

@@ -1,3 +1,4 @@
+# A note on merge request or commit diffs
 class DiffNote < Note
   include NoteOnDiff
 

app/models/discussion.rb

@@ -39,6 +39,7 @@ class Discussion
     [:discussion, note.noteable_type.try(:underscore), noteable_id]
   end
 
+  # Returns an array of discussion ID components
   def self.build_discussion_id(note)
     [*build_discussion_id_base(note), SecureRandom.hex]
   end

app/models/discussion_note.rb

@@ -1,3 +1,4 @@
+# A note in a non-diff discussion on an issue, merge request, commit, or snippet
 class DiscussionNote < Note
   NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze
 

app/models/individual_note_discussion.rb

@@ -1,3 +1,5 @@
+# A discussion to wrap a single `Note` note on the root of an issue, merge request,
+# commit, or snippet, that is not displayed as a discussion
 class IndividualNoteDiscussion < Discussion
   # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote`
   def potentially_resolvable?

app/models/legacy_diff_discussion.rb

@@ -1,3 +1,4 @@
+# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes
 class LegacyDiffDiscussion < Discussion
   include DiscussionOnDiff
 

app/models/legacy_diff_note.rb

@@ -1,3 +1,4 @@
+# A note on merge request or commit diffs, using the legacy implementation
 class LegacyDiffNote < Note
   include NoteOnDiff
 

app/models/note.rb

@@ -68,6 +68,7 @@ class Note < ActiveRecord::Base
   scope :user, ->{ where(system: false) }
   scope :common, ->{ where(noteable_type: ["", nil]) }
   scope :fresh, ->{ order(created_at: :asc, id: :asc) }
+  scope :updated_after, ->(time){ where('updated_at > ?', time) }
   scope :inc_author_project, ->{ includes(:project, :author) }
   scope :inc_author, ->{ includes(:author) }
   scope :inc_relations_for_view, -> do
@@ -238,18 +239,20 @@ class Note < ActiveRecord::Base
     discussion_class(noteable).override_discussion_id(self) || super()
   end
 
-  # Returns a discussion containing just this note
+  # Returns a discussion containing just this note.
+  # This method exists as an alternative to `#discussion` to use when the methods
+  # we intend to call on the Discussion object don't require it to have all of its notes,
+  # and just depend on the first note or the type of discussion. This saves us a DB query.
   def to_discussion(noteable = nil)
     Discussion.build([self], noteable)
   end
 
-  # Returns the entire discussion this note is part of
+  # Returns the entire discussion this note is part of.
+  # Consider using `#to_discussion` if we do not need to render the discussion
+  # and all its notes and if we don't care about the discussion's resolvability status.
   def discussion
-    if part_of_discussion?
-      self.noteable.notes.find_discussion(self.discussion_id) || to_discussion
-    else
-      to_discussion
-    end
+    full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
+    full_discussion || to_discussion
   end
 
   def part_of_discussion?

app/models/out_of_context_discussion.rb

@@ -1,3 +1,5 @@
+# A discussion to wrap a number of `Note` notes on the root of a commit when they
+# are displayed in context of a merge request as if they were part of a discussion.
 class OutOfContextDiscussion < Discussion
   # To make sure all out-of-context notes are displayed in one discussion,
   # we override the discussion ID to be a newly generated but consistent ID.

app/models/simple_discussion.rb

@@ -1,2 +1,3 @@
+# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes
 class SimpleDiscussion < Discussion
 end

app/views/notify/_note_email.html.haml

@@ -1,17 +1,18 @@
-- if @discussion
+- discussion = @note.discussion unless @discussion.individual_note?
+- if discussion
   %p.details
     = succeed ':' do
       = link_to @note.author_name, user_url(@note.author)
 
-      - if @discussion.diff_discussion?
-        - if @discussion.new_discussion?
+      - if discussion.diff_discussion?
+        - if discussion.new_discussion?
           started a new discussion
         - else
           commented on a discussion
 
-        on #{link_to @discussion.file_path, @target_url}
+        on #{link_to discussion.file_path, @target_url}
       - else
-        - if @discussion.new_discussion?
+        - if discussion.new_discussion?
           started a new discussion
         - else
           commented on a #{link_to 'discussion', @target_url}
@@ -20,15 +21,15 @@
   %p.details
     #{link_to @note.author_name, user_url(@note.author)} commented:
 
-- if @discussion&.diff_discussion?
+- if discussion&.diff_discussion?
   = content_for :head do
     = stylesheet_link_tag 'mailers/highlighted_diff_email'
 
   %table
     = render partial: "projects/diffs/line",
-      collection: @discussion.truncated_diff_lines,
+      collection: discussion.truncated_diff_lines,
       as: :line,
-      locals: { diff_file: @discussion.diff_file,
+      locals: { diff_file: discussion.diff_file,
         plain: true,
         email: true }
 

app/views/notify/_note_email.text.erb

@@ -1,12 +1,13 @@
-<%  if @discussion -%>
+<%  discussion = @note.discussion unless @discussion.individual_note? -%>
+<%  if discussion && !discussion.individual_note? -%>
 <%=   @note.author_name -%>
-<%    if @discussion.new_discussion? -%>
+<%    if discussion.new_discussion? -%>
 <%=     " started a new discussion" -%>
 <%    else -%>
 <%=     " commented on a discussion" -%>
 <%    end -%>
-<%    if @discussion.diff_discussion? -%>
-<%=     " on #{@discussion.file_path}" -%>
+<%    if discussion.diff_discussion? -%>
+<%=     " on #{discussion.file_path}" -%>
 <%    end -%>
 <%=   ":" -%>
 
@@ -16,8 +17,8 @@
 
 
 <%  end -%>
-<%  if @discussion&.diff_discussion? -%>
-<%    @discussion.truncated_diff_lines(highlight: false).each do |line| -%>
+<%  if discussion&.diff_discussion? -%>
+<%    discussion.truncated_diff_lines(highlight: false).each do |line| -%>
 <%=     "> #{line.text}\n" -%>
 <%    end -%>
 

app/views/notify/note_commit_email.text.erb

@@ -1 +1 @@
-<%= render partial: 'note_email' %>
+<%= render 'note_email' %>

app/views/notify/note_issue_email.text.erb

@@ -1 +1 @@
-<%= render partial: 'note_email' %>
+<%= render 'note_email' %>

app/views/notify/note_personal_snippet_email.text.erb

@@ -1 +1 @@
-<%= render partial: 'note_email' %>
+<%= render 'note_email' %>

app/views/notify/note_snippet_email.text.erb

@@ -1 +1 @@
-<%= render partial: 'note_email' %>
+<%= render 'note_email' %>

spec/controllers/projects/notes_controller_spec.rb

@@ -15,7 +15,6 @@ describe Projects::NotesController do
   end
 
   describe 'GET index' do
-    let(:last_fetched_at) { '1487756246' }
     let(:request_params) do
       {
         namespace_id: project.namespace,
@@ -35,6 +34,8 @@ describe Projects::NotesController do
     end
 
     it 'passes last_fetched_at from headers to NotesFinder' do
+      last_fetched_at = 3.hours.ago.to_i
+
       request.headers['X-Last-Fetched-At'] = last_fetched_at
 
       expect(NotesFinder).to receive(:new)
@@ -47,21 +48,11 @@ describe Projects::NotesController do
     context 'for a discussion note' do
       let!(:note) { create(:discussion_note_on_issue, noteable: issue, project: project) }
 
-      it 'includes the ID' do
+      it 'responds with the expected attributes' do
         get :index, request_params
 
         expect(note_json[:id]).to eq(note.id)
-      end
-
-      it 'includes discussion_html' do
-        get :index, request_params
-
         expect(note_json[:discussion_html]).not_to be_nil
-      end
-
-      it "doesn't include diff_discussion_html" do
-        get :index, request_params
-
         expect(note_json[:diff_discussion_html]).to be_nil
       end
     end
@@ -72,21 +63,11 @@ describe Projects::NotesController do
 
       let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
 
-      it 'includes the ID' do
+      it 'responds with the expected attributes' do
         get :index, params
 
         expect(note_json[:id]).to eq(note.id)
-      end
-
-      it 'includes discussion_html' do
-        get :index, params
-
         expect(note_json[:discussion_html]).not_to be_nil
-      end
-
-      it 'includes diff_discussion_html' do
-        get :index, params
-
         expect(note_json[:diff_discussion_html]).not_to be_nil
       end
     end
@@ -100,21 +81,11 @@ describe Projects::NotesController do
 
         let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) }
 
-        it 'includes the ID' do
+        it 'responds with the expected attributes' do
           get :index, params
 
           expect(note_json[:id]).to eq(note.id)
-        end
-
-        it 'includes discussion_html' do
-          get :index, params
-
           expect(note_json[:discussion_html]).not_to be_nil
-        end
-
-        it "doesn't include diff_discussion_html" do
-          get :index, params
-
           expect(note_json[:diff_discussion_html]).to be_nil
         end
       end
@@ -122,21 +93,11 @@ describe Projects::NotesController do
       context 'when displayed on the commit' do
         let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) }
 
-        it 'includes the ID' do
+        it 'responds with the expected attributes' do
           get :index, params
 
           expect(note_json[:id]).to eq(note.id)
-        end
-
-        it "doesn't include discussion_html" do
-          get :index, params
-
           expect(note_json[:discussion_html]).to be_nil
-        end
-
-        it "doesn't include diff_discussion_html" do
-          get :index, params
-
           expect(note_json[:diff_discussion_html]).to be_nil
         end
       end
@@ -145,27 +106,12 @@ describe Projects::NotesController do
     context 'for a regular note' do
       let!(:note) { create(:note, noteable: issue, project: project) }
 
-      it 'includes the ID' do
+      it 'responds with the expected attributes' do
         get :index, request_params
 
         expect(note_json[:id]).to eq(note.id)
-      end
-
-      it 'includes html' do
-        get :index, request_params
-
         expect(note_json[:html]).not_to be_nil
-      end
-
-      it "doesn't include discussion_html" do
-        get :index, request_params
-
         expect(note_json[:discussion_html]).to be_nil
-      end
-
-      it "doesn't include diff_discussion_html" do
-        get :index, request_params
-
         expect(note_json[:diff_discussion_html]).to be_nil
       end
     end