diff options
28 files changed, 97 insertions, 128 deletions
diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb new file mode 100644 index 00000000000..dd21066ac13 --- /dev/null +++ b/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 diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index d1558bdfba8..8362bfc39dd 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0c6267f065b..ed2f06692d4 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 48a10fbe9cc..09ccd19abc2 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/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 diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 7ae590d3de1..881e6d8d4b5 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -1,5 +1,5 @@ class Projects::NotesController < Projects::ApplicationController - include NotesHelper + include RendersNotes include ToggleAwardEmoji # Authorize diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 2600a9d7256..5c9e0d4d1a1 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/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 diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 855940762f3..c20bfb20171 100644 --- a/app/finders/notes_finder.rb +++ b/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 diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 8ec72d5f43a..443e0143647 100644 --- a/app/helpers/notes_helper.rb +++ b/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 diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index a42745acd71..3c78e1fcd68 100644 --- a/app/mailers/emails/notes.rb +++ b/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 diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 3141c150ac9..22cd9bb7fd4 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/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, diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb index 2aba979938b..1155ec22369 100644 --- a/app/models/concerns/resolvable_note.rb +++ b/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 } diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 8acb70eb7cb..a89cce4bf5e 100644 --- a/app/models/diff_discussion.rb +++ b/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 diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 6029fc42e9c..9185a5a0ad8 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -1,3 +1,4 @@ +# A note on merge request or commit diffs class DiffNote < Note include NoteOnDiff diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 782db4044ed..a77076f02b6 100644 --- a/app/models/discussion.rb +++ b/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 diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb index 510aefbf609..8957161805a 100644 --- a/app/models/discussion_note.rb +++ b/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 diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index ebcf60beaf3..506b0a98528 100644 --- a/app/models/individual_note_discussion.rb +++ b/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? diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb index 36612a28ba1..39b57f6d743 100644 --- a/app/models/legacy_diff_discussion.rb +++ b/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 diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 0d86610e473..8fdebef042b 100644 --- a/app/models/legacy_diff_note.rb +++ b/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 diff --git a/app/models/note.rb b/app/models/note.rb index 9d4f99ac9c8..3d2decf6930 100644 --- a/app/models/note.rb +++ b/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? diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb index ecbfd97699f..7be9aa19707 100644 --- a/app/models/out_of_context_discussion.rb +++ b/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. diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb index 41c679daf2d..cd2c142211c 100644 --- a/app/models/simple_discussion.rb +++ b/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 diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml index 8b139a150b7..9e5aaed6f49 100644 --- a/app/views/notify/_note_email.html.haml +++ b/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 } diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb index f2db321859f..b25b513f4d8 100644 --- a/app/views/notify/_note_email.text.erb +++ b/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 -%> diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb index 5585e7c3ee8..413d9e6e9ac 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -1 +1 @@ -<%= render partial: 'note_email' %> +<%= render 'note_email' %> diff --git a/app/views/notify/note_issue_email.text.erb b/app/views/notify/note_issue_email.text.erb index 5585e7c3ee8..413d9e6e9ac 100644 --- a/app/views/notify/note_issue_email.text.erb +++ b/app/views/notify/note_issue_email.text.erb @@ -1 +1 @@ -<%= render partial: 'note_email' %> +<%= render 'note_email' %> diff --git a/app/views/notify/note_personal_snippet_email.text.erb b/app/views/notify/note_personal_snippet_email.text.erb index 5585e7c3ee8..413d9e6e9ac 100644 --- a/app/views/notify/note_personal_snippet_email.text.erb +++ b/app/views/notify/note_personal_snippet_email.text.erb @@ -1 +1 @@ -<%= render partial: 'note_email' %> +<%= render 'note_email' %> diff --git a/app/views/notify/note_snippet_email.text.erb b/app/views/notify/note_snippet_email.text.erb index 5585e7c3ee8..413d9e6e9ac 100644 --- a/app/views/notify/note_snippet_email.text.erb +++ b/app/views/notify/note_snippet_email.text.erb @@ -1 +1 @@ -<%= render partial: 'note_email' %> +<%= render 'note_email' %> diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index b276f9321c7..52a76776239 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/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 |