diff options
Diffstat (limited to 'app')
39 files changed, 695 insertions, 479 deletions
diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index 3f041172ff3..d1d24b411a4 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -5,7 +5,7 @@ let $commentButtonTemplate; var bind = function(fn, me) { return function() { return fn.apply(me, arguments); }; }; -window.FilesCommentButton = (function() { +this.FilesCommentButton = (function() { var COMMENT_BUTTON_CLASS, EMPTY_CELL_CLASS, LINE_COLUMN_CLASSES, LINE_CONTENT_CLASS, LINE_HOLDER_CLASS, LINE_NUMBER_CLASS, OLD_LINE_CLASS, TEXT_FILE_SELECTOR, UNFOLDABLE_LINE_CLASS; COMMENT_BUTTON_CLASS = '.add-diff-note'; @@ -55,14 +55,19 @@ window.FilesCommentButton = (function() { textFileElement = this.getTextFileElement($currentTarget); buttonParentElement.append(this.buildButton({ + discussionID: lineContentElement.attr('data-discussion-id'), + lineType: lineContentElement.attr('data-line-type'), + noteableType: textFileElement.attr('data-noteable-type'), noteableID: textFileElement.attr('data-noteable-id'), commitID: textFileElement.attr('data-commit-id'), noteType: lineContentElement.attr('data-note-type'), - position: lineContentElement.attr('data-position'), - lineType: lineContentElement.attr('data-line-type'), - discussionID: lineContentElement.attr('data-discussion-id'), - lineCode: lineContentElement.attr('data-line-code') + + // LegacyDiffNote + lineCode: lineContentElement.attr('data-line-code'), + + // DiffNote + position: lineContentElement.attr('data-position') })); }; @@ -76,14 +81,19 @@ window.FilesCommentButton = (function() { FilesCommentButton.prototype.buildButton = function(buttonAttributes) { return $commentButtonTemplate.clone().attr({ + 'data-discussion-id': buttonAttributes.discussionID, + 'data-line-type': buttonAttributes.lineType, + 'data-noteable-type': buttonAttributes.noteableType, 'data-noteable-id': buttonAttributes.noteableID, 'data-commit-id': buttonAttributes.commitID, 'data-note-type': buttonAttributes.noteType, + + // LegacyDiffNote 'data-line-code': buttonAttributes.lineCode, - 'data-position': buttonAttributes.position, - 'data-discussion-id': buttonAttributes.discussionID, - 'data-line-type': buttonAttributes.lineType + + // DiffNote + 'data-position': buttonAttributes.position }); }; diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 1d563c63f39..71c03c89314 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -213,11 +213,7 @@ require('./task_list'); _this.last_fetched_at = data.last_fetched_at; _this.setPollingInterval(data.notes.length); return $.each(notes, function(i, note) { - if (note.discussion_html != null) { - return _this.renderDiscussionNote(note); - } else { - return _this.renderNote(note); - } + _this.renderNote(note); }); }; })(this) @@ -278,6 +274,10 @@ require('./task_list'); Notes.prototype.renderNote = function(note) { var $notesList; + if (note.discussion_html != null) { + return this.renderDiscussionNote(note); + } + if (!note.valid) { if (note.errors.commands_only) { new Flash(note.errors.commands_only, 'notice', this.parentTimeline); @@ -323,9 +323,9 @@ require('./task_list'); return; } this.note_ids.push(note.id); - form = $("#new-discussion-note-form-" + note.discussion_id); - if ((note.original_discussion_id != null) && form.length === 0) { - form = $("#new-discussion-note-form-" + note.original_discussion_id); + form = $(".js-discussion-note-form[data-discussion-id='" + note.discussion_id + "']"); + if (form.length === 0) { + form = $(".js-discussion-note-form[data-original-discussion-id='" + note.original_discussion_id + "']"); } row = form.closest("tr"); lineType = this.isParallelView() ? form.find('#line_type').val() : 'old'; @@ -334,8 +334,8 @@ require('./task_list'); note_html.renderGFM(); // is this the first note of discussion? discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']"); - if ((note.original_discussion_id != null) && discussionContainer.length === 0) { - discussionContainer = $(".notes[data-discussion-id='" + note.original_discussion_id + "']"); + if (discussionContainer.length === 0) { + discussionContainer = $(".notes[data-original-discussion-id='" + note.original_discussion_id + "']"); } if (discussionContainer.length === 0) { if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { @@ -363,7 +363,7 @@ require('./task_list'); // Add note to 'Changes' page discussions discussionContainer.append(note_html); // Init discussion on 'Discussion' page if it is merge request page - if ($('body').attr('data-page').indexOf('projects:merge_request') === 0) { + if ($('body').attr('data-page').indexOf('projects:merge_request') === 0 || !note.diff_discussion_html) { $('ul.main-notes-list').append(note.discussion_html).renderGFM(); } } else { @@ -456,6 +456,7 @@ require('./task_list'); form.find("#note_line_code").remove(); form.find("#note_position").remove(); form.find("#note_type").remove(); + form.find("#note_in_reply_to_discussion_id").remove(); form.find('.js-comment-resolve-button').closest('comment-and-resolve-btn').remove(); return this.parentTimeline = form.parents('.timeline'); }; @@ -470,10 +471,24 @@ require('./task_list'); */ Notes.prototype.setupNoteForm = function(form) { - var textarea; + var textarea, key; new gl.GLForm(form); textarea = form.find(".js-note-text"); - return new Autosave(textarea, ["Note", form.find("#note_noteable_type").val(), form.find("#note_noteable_id").val(), form.find("#note_commit_id").val(), form.find("#note_type").val(), form.find("#note_line_code").val(), form.find("#note_position").val()]); + key = [ + "Note", + form.find("#note_noteable_type").val(), + form.find("#note_noteable_id").val(), + form.find("#note_commit_id").val(), + form.find("#note_type").val(), + form.find("#in_reply_to_discussion_id").val(), + + // LegacyDiffNote + form.find("#note_line_code").val(), + + // DiffNote + form.find("#note_position").val() + ]; + return new Autosave(textarea, key); }; /* @@ -510,7 +525,7 @@ require('./task_list'); } } - this.renderDiscussionNote(note); + this.renderNote(note); // cleanup after successfully creating a diff/discussion note this.removeDiscussionNoteForm($form); }; @@ -727,23 +742,35 @@ require('./task_list'); Sets some hidden fields in the form. - Note: dataHolder must have the "discussionId", "lineCode", "noteableType" - and "noteableId" data attributes set. + Note: dataHolder must have the "discussionId" and "lineCode" data attributes set. */ Notes.prototype.setupDiscussionNoteForm = function(dataHolder, form) { // setup note target - form.attr('id', "new-discussion-note-form-" + (dataHolder.data("discussionId"))); + var discussionID = dataHolder.data("discussionId"); + + form.attr('id', "new-discussion-note-form-" + discussionID); + form.attr("data-discussion-id", discussionID); + form.attr("data-original-discussion-id", dataHolder.data("originalDiscussionId") || discussionID); form.attr("data-line-code", dataHolder.data("lineCode")); - form.find("#note_type").val(dataHolder.data("noteType")); + form.find("#line_type").val(dataHolder.data("lineType")); + form.find("#in_reply_to_discussion_id").val(dataHolder.data("originalDiscussionId")); + + form.find("#note_noteable_type").val(dataHolder.data("noteableType")); + form.find("#note_noteable_id").val(dataHolder.data("noteableId")); form.find("#note_commit_id").val(dataHolder.data("commitId")); + form.find("#note_type").val(dataHolder.data("noteType")); + + // LegacyDiffNote form.find("#note_line_code").val(dataHolder.data("lineCode")); + + // DiffNote form.find("#note_position").val(dataHolder.attr("data-position")); - form.find("#note_noteable_type").val(dataHolder.data("noteableType")); - form.find("#note_noteable_id").val(dataHolder.data("noteableId")); + form.find('.js-note-discard').show().removeClass('js-note-discard').addClass('js-close-discussion-note-form').text(form.find('.js-close-discussion-note-form').data('cancel-text')); form.find('.js-note-target-close').remove(); + form.find('.js-note-new-discussion').remove(); this.setupNoteForm(form); if (typeof gl.diffNotesCompileComponents !== 'undefined') { diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index cc67f688d51..98cd4ce344a 100644 --- a/app/controllers/projects/commit_controller.rb +++ b/app/controllers/projects/commit_controller.rb @@ -2,6 +2,7 @@ # # Not to be confused with CommitsController, plural. class Projects::CommitController < Projects::ApplicationController + include NotesHelper include CreatesCommit include DiffForPath include DiffHelper @@ -111,22 +112,19 @@ class Projects::CommitController < Projects::ApplicationController end def define_note_vars - @grouped_diff_discussions = commit.notes.grouped_diff_discussions - @notes = commit.notes.non_diff_notes.fresh - - Banzai::NoteRenderer.render( - @grouped_diff_discussions.values.flat_map(&:notes) + @notes, - @project, - current_user, - ) - + @noteable = @commit @note = @project.build_commit_note(commit) - @noteable = @commit - @comments_target = { + @new_diff_note_attrs = { noteable_type: 'Commit', commit_id: @commit.id } + + @grouped_diff_discussions = commit.grouped_diff_discussions + @discussions = commit.discussions + + @notes = (@grouped_diff_discussions.values + @discussions).flat_map(&:notes) + @notes = prepare_notes_for_rendering(@notes) end def assign_change_commit_vars diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb index 1349b015a63..f4a18a5e8f7 100644 --- a/app/controllers/projects/discussions_controller.rb +++ b/app/controllers/projects/discussions_controller.rb @@ -28,7 +28,7 @@ class Projects::DiscussionsController < Projects::ApplicationController end def discussion - @discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404 + @discussion ||= @merge_request.find_discussion(params[:id]) || render_404 end def authorize_resolve_discussion! diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index d984e6d3918..0c6267f065b 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -84,15 +84,11 @@ class Projects::IssuesController < Projects::ApplicationController end def show - raw_notes = @issue.notes.inc_relations_for_view.fresh - - @notes = Banzai::NoteRenderer. - render(raw_notes, @project, current_user, @path, @project_wiki, @ref) - - @note = @project.notes.new(noteable: @issue) @noteable = @issue + @note = @project.notes.new(noteable: @issue) - preload_max_access_for_authors(@notes, @project) + @discussions = @issue.discussions + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) respond_to do |format| format.html diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 37e3ac05916..7863fe18f32 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -570,20 +570,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @note = @project.notes.new(noteable: @merge_request) @discussions = @merge_request.discussions - - preload_noteable_for_regular_notes(@discussions.flat_map(&:notes)) - - # This is not executed lazily - @notes = Banzai::NoteRenderer.render( - @discussions.flat_map(&:notes), - @project, - current_user, - @path, - @project_wiki, - @ref - ) - - preload_max_access_for_authors(@notes, @project) + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) end def define_widget_vars @@ -596,22 +583,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController end def define_diff_comment_vars - @comments_target = { + @new_diff_note_attrs = { noteable_type: 'MergeRequest', noteable_id: @merge_request.id } @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs? - @grouped_diff_discussions = @merge_request.notes.inc_relations_for_view.grouped_diff_discussions - - Banzai::NoteRenderer.render( - @grouped_diff_discussions.values.flat_map(&:notes), - @project, - current_user, - @path, - @project_wiki, - @ref - ) + + @grouped_diff_discussions = @merge_request.grouped_diff_discussions + @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flat_map(&:notes)) end def define_pipelines_vars diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index d00177e7612..f80313bbee0 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -6,13 +6,14 @@ class Projects::NotesController < Projects::ApplicationController before_action :authorize_create_note!, only: [:create] before_action :authorize_admin_note!, only: [:update, :destroy] before_action :authorize_resolve_note!, only: [:resolve, :unresolve] - before_action :find_current_user_notes, only: [:index] def index current_fetched_at = Time.now.to_i notes_json = { notes: [], last_fetched_at: current_fetched_at } + @notes = notes_finder.execute.inc_author + @notes.each do |note| next if note.cross_reference_not_visible_for?(current_user) @@ -23,7 +24,11 @@ class Projects::NotesController < Projects::ApplicationController end def create - create_params = note_params.merge(merge_request_diff_head_sha: params[:merge_request_diff_head_sha]) + create_params = note_params.merge( + merge_request_diff_head_sha: params[:merge_request_diff_head_sha], + in_reply_to_discussion_id: params[:in_reply_to_discussion_id], + new_discussion: params[:new_discussion], + ) @note = Notes::CreateService.new(project, current_user, create_params).execute if @note.is_a?(Note) @@ -111,6 +116,17 @@ class Projects::NotesController < Projects::ApplicationController ) end + def discussion_html(discussion) + return if discussion.render_as_individual_notes? + + render_to_string( + "discussions/_discussion", + layout: false, + formats: [:html], + locals: { discussion: discussion } + ) + end + def diff_discussion_html(discussion) return unless discussion.diff_discussion? @@ -135,17 +151,6 @@ class Projects::NotesController < Projects::ApplicationController ) end - def discussion_html(discussion) - return unless discussion.diff_discussion? - - render_to_string( - "discussions/_discussion", - layout: false, - formats: [:html], - locals: { discussion: discussion } - ) - end - def note_json(note) attrs = { id: note.id @@ -156,33 +161,22 @@ class Projects::NotesController < Projects::ApplicationController attrs.merge!( valid: true, - discussion_id: note.discussion_id, + discussion_id: note.discussion_id(noteable), html: note_html(note), note: note.note ) - if note.diff_note? - discussion = note.to_discussion - + discussion = note.to_discussion(noteable) + unless discussion.render_as_individual_notes? attrs.merge!( diff_discussion_html: diff_discussion_html(discussion), - discussion_html: discussion_html(discussion) - ) + discussion_html: discussion_html(discussion), - # The discussion_id is used to add the comment to the correct discussion - # element on the merge request page. Among other things, the discussion_id - # contains the sha of head commit of the merge request. - # When new commits are pushed into the merge request after the initial - # load of the merge request page, the discussion elements will still have - # the old discussion_ids, with the old head commit sha. The new comment, - # however, will have the new discussion_id with the new commit sha. - # To ensure that these new comments will still end up in the correct - # discussion element, we also send the original discussion_id, with the - # old commit sha, along, and fall back on this value when no discussion - # element with the new discussion_id could be found. - if note.new_diff_note? && note.position != note.original_position - attrs[:original_discussion_id] = note.original_discussion_id - end + # Since the `discussion_id` can change, for example when new commits are pushed into an MR, + # the never-changing `original_discussion_id` is used as a fallback to the find the relevant + # discussion container to add this note to. + original_discussion_id: note.original_discussion_id + ) end else attrs.merge!( @@ -205,14 +199,30 @@ class Projects::NotesController < Projects::ApplicationController def note_params params.require(:note).permit( - :note, :noteable, :noteable_id, :noteable_type, :project_id, - :attachment, :line_code, :commit_id, :type, :position + :project_id, + :noteable_type, + :noteable_id, + :commit_id, + :noteable, + :type, + + :note, + :attachment, + + # LegacyDiffNote + :line_code, + + # DiffNote + :position ) end - def find_current_user_notes - @notes = NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at)) - .execute.inc_author + def notes_finder + @notes_finder ||= NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at)) + end + + def noteable + @noteable ||= notes_finder.target end def last_fetched_at diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index ea1a97b7cf0..2600a9d7256 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -1,4 +1,5 @@ class Projects::SnippetsController < Projects::ApplicationController + include NotesHelper include ToggleAwardEmoji include SpammableActions include SnippetsActions @@ -55,8 +56,10 @@ class Projects::SnippetsController < Projects::ApplicationController def show @note = @project.notes.new(noteable: @snippet) - @notes = Banzai::NoteRenderer.render(@snippet.notes.fresh, @project, current_user) @noteable = @snippet + + @discussions = @snippet.discussions + @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes)) end def destroy diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 6630c6384f2..855940762f3 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -17,29 +17,46 @@ class NotesFinder @project = project @current_user = current_user @params = params - init_collection end def execute - @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at] + @notes = init_collection + @notes = since_fetch_at(@params[:last_fetched_at], @notes) if @params[:last_fetched_at] @notes end - private + def target + return @target if defined?(@target) - def init_collection - @notes = - if @params[:target_id] - on_target(@params[:target_type], @params[:target_id]) + target_type = @params[:target_type] + target_id = @params[:target_id] + + return @target = nil unless target_type && target_id + + @target = + if target_type == "commit" + if Ability.allowed?(@current_user, :download_code, @project) + @project.commit(target_id) + end else - notes_of_any_type + noteables_for_type(target_type).find(target_id) end end + private + + def init_collection + if target + notes_on_target + else + notes_of_any_type + end + end + 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(@params[:search], notes) } if @params[:search] UnionFinder.new.find_union(note_relations, Note) end @@ -69,17 +86,11 @@ class NotesFinder end end - def on_target(target_type, target_id) - if target_type == "commit" - notes_for_type('commit').for_commit_id(target_id) + def notes_on_target + if target.respond_to?(:related_notes) + target.related_notes else - target = noteables_for_type(target_type).find(target_id) - - if target.respond_to?(:related_notes) - target.related_notes - else - target.notes - end + target.notes end end @@ -94,10 +105,9 @@ class NotesFinder # Notes changed since last fetch # Uses overlapping intervals to avoid worrying about race conditions - def since_fetch_at(fetch_time) + def since_fetch_at(fetch_time, notes_relation = @notes) # Default to 0 to remain compatible with old clients last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i) - - @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh + notes_relation.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh end end diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index b0331f36a2f..01ecac983cf 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -24,9 +24,9 @@ module NotesHelper end def diff_view_data - return {} unless @comments_target + return {} unless @new_diff_note_attrs - @comments_target.slice(:noteable_id, :noteable_type, :commit_id) + @new_diff_note_attrs.slice(:noteable_id, :noteable_type, :commit_id) end def diff_view_line_data(line_code, position, line_type) @@ -53,37 +53,26 @@ module NotesHelper } if use_legacy_diff_note - discussion_id = LegacyDiffNote.discussion_id( - @comments_target[:noteable_type], - @comments_target[:noteable_id] || @comments_target[:commit_id], - line_code - ) - - data.merge!( - note_type: LegacyDiffNote.name, - discussion_id: discussion_id - ) + new_note = LegacyDiffNote.new(@new_diff_note_attrs.merge(line_code: line_code)) + discussion_id = new_note.discussion_id else - discussion_id = DiffNote.discussion_id( - @comments_target[:noteable_type], - @comments_target[:noteable_id] || @comments_target[:commit_id], - position - ) - - data.merge!( - position: position.to_json, - note_type: DiffNote.name, - discussion_id: discussion_id - ) + new_note = DiffNote.new(@new_diff_note_attrs.merge(position: position)) + discussion_id = new_note.discussion_id + + data[:position] = position.to_json end - data + data.merge( + note_type: new_note.type, + discussion_id: discussion_id + ) end def link_to_reply_discussion(discussion, line_type = nil) return unless current_user - data = discussion.reply_attributes.merge(line_type: line_type) + data = { discussion_id: discussion.id, original_discussion_id: discussion.original_id, line_type: line_type } + data[:line_code] = discussion.line_code if discussion.respond_to?(:line_code) button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', data: data, title: 'Add a reply' @@ -95,7 +84,15 @@ module NotesHelper end def preload_noteable_for_regular_notes(notes) - ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable) + 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) diff --git a/app/models/commit.rb b/app/models/commit.rb index ce92cc369ad..5c452f78546 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -2,6 +2,7 @@ class Commit extend ActiveModel::Naming include ActiveModel::Conversion + include Noteable include Participable include Mentionable include Referable @@ -203,6 +204,10 @@ class Commit project.notes.for_commit_id(self.id) end + def discussion_notes + notes.non_diff_notes + end + def notes_with_associations notes.includes(:author) end diff --git a/app/models/commit_discussion.rb b/app/models/commit_discussion.rb new file mode 100644 index 00000000000..bcca3155335 --- /dev/null +++ b/app/models/commit_discussion.rb @@ -0,0 +1,9 @@ +class CommitDiscussion < Discussion + def self.override_discussion_id(note) + discussion_id(note) + end + + def potentially_resolvable? + false + end +end diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index b8dd27a7afe..9ce40f329d8 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -24,12 +24,4 @@ module NoteOnDiff def diff_attributes raise NotImplementedError end - - def can_be_award_emoji? - false - end - - def to_discussion - Discussion.new([self]) - end end diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb new file mode 100644 index 00000000000..7900af6aaac --- /dev/null +++ b/app/models/concerns/noteable.rb @@ -0,0 +1,17 @@ +module Noteable + def discussion_notes + notes + end + + delegate :find_discussion, :find_original_discussion, to: :discussion_notes + + def discussions + @discussions ||= discussion_notes + .inc_relations_for_view + .discussions(self) + end + + def grouped_diff_discussions + notes.inc_relations_for_view.grouped_diff_discussions + end +end diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb new file mode 100644 index 00000000000..eecb77ebf80 --- /dev/null +++ b/app/models/concerns/resolvable_note.rb @@ -0,0 +1,61 @@ +module ResolvableNote + extend ActiveSupport::Concern + + included do + belongs_to :resolved_by, class_name: "User" + + validates :resolved_by, presence: true, if: :resolved? + + # Keep this scope in sync with the logic in `#resolvable?` in `Note` subclasses that are resolvable + scope :resolvable, -> { where(type: %w(DiffNote DiscussionNote)).user.where(noteable_type: 'MergeRequest') } + scope :resolved, -> { resolvable.where.not(resolved_at: nil) } + scope :unresolved, -> { resolvable.where(resolved_at: nil) } + end + + module ClassMethods + # This method must be kept in sync with `#resolve!` + def resolve!(current_user) + unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id) + end + + # This method must be kept in sync with `#unresolve!` + def unresolve! + resolved.update_all(resolved_at: nil, resolved_by_id: nil) + end + end + + # If you update this method remember to also update the scope `resolvable` + def resolvable? + to_discussion.potentially_resolvable? && !system? + end + + def resolved? + return false unless resolvable? + + self.resolved_at.present? + end + + def to_be_resolved? + resolvable? && !resolved? + end + + # If you update this method remember to also update `.resolve!` + def resolve!(current_user) + return unless resolvable? + return if resolved? + + self.resolved_at = Time.now + self.resolved_by = current_user + save! + end + + # If you update this method remember to also update `.unresolve!` + def unresolve! + return unless resolvable? + return unless resolved? + + self.resolved_at = nil + self.resolved_by = nil + save! + end +end diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb new file mode 100644 index 00000000000..9a934c7520c --- /dev/null +++ b/app/models/diff_discussion.rb @@ -0,0 +1,69 @@ +class DiffDiscussion < Discussion + NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + + delegate :line_code, + :original_line_code, + :diff_file, + :for_line?, + :active?, + + to: :first_note + + delegate :blob, + :highlighted_diff_lines, + :diff_lines, + + to: :diff_file, + allow_nil: true + + def self.build_discussion_id(note) + [*super(note), *unique_position_identifier(note)] + end + + def self.build_original_discussion_id(note) + [*Discussion.build_discussion_id(note), *note.original_position.key] + end + + def self.unique_position_identifier(note) + note.position.key + end + + def diff_discussion? + true + end + + def legacy_diff_discussion? + false + end + + def active? + return @active if @active.present? + + @active = first_note.active? + end + MEMOIZED_VALUES << :active + + def reply_attributes + super.merge(first_note.diff_attributes) + end + + # Returns an array of at most 16 highlighted lines above a diff note + def truncated_diff_lines(highlight: true) + lines = highlight ? highlighted_diff_lines : diff_lines + prev_lines = [] + + lines.each do |line| + if line.meta? + prev_lines.clear + else + prev_lines << line + + break if for_line?(line) + + prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + prev_lines + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 895a91139c9..e5f437f8647 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -9,58 +9,44 @@ class DiffNote < Note validates :diff_line, presence: true validates :line_code, presence: true, line_code: true validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) } - validates :resolved_by, presence: true, if: :resolved? validate :positions_complete validate :verify_supported - # Keep this scope in sync with the logic in `#resolvable?` - scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') } - scope :resolved, -> { resolvable.where.not(resolved_at: nil) } - scope :unresolved, -> { resolvable.where(resolved_at: nil) } - - after_initialize :ensure_original_discussion_id before_validation :set_original_position, :update_position, on: :create - before_validation :set_line_code, :set_original_discussion_id + before_validation :set_line_code # We need to do this again, because it's already in `Note`, but is affected by # `update_position` and needs to run after that. - before_validation :set_discussion_id + before_validation :set_discussion_id, if: :position_changed? after_save :keep_around_commits - class << self - def build_discussion_id(noteable_type, noteable_id, position) - [super(noteable_type, noteable_id), *position.key].join("-") - end - - # This method must be kept in sync with `#resolve!` - def resolve!(current_user) - unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id) - end - - # This method must be kept in sync with `#unresolve!` - def unresolve! - resolved.update_all(resolved_at: nil, resolved_by_id: nil) - end - end - def new_diff_note? true end + def discussion_class(*) + DiffDiscussion + end + def diff_attributes - { position: position.to_json } + { + original_position: original_position.to_json, + position: position.to_json, + } end - def position=(new_position) - if new_position.is_a?(String) - new_position = JSON.parse(new_position) rescue nil - end + %i(original_position= position=).each do |meth| + define_method meth do |new_position| + if new_position.is_a?(String) + new_position = JSON.parse(new_position) rescue nil + end - if new_position.is_a?(Hash) - new_position = new_position.with_indifferent_access - new_position = Gitlab::Diff::Position.new(new_position) - end + if new_position.is_a?(Hash) + new_position = new_position.with_indifferent_access + new_position = Gitlab::Diff::Position.new(new_position) + end - super(new_position) + super(new_position) + end end def diff_file @@ -88,43 +74,6 @@ class DiffNote < Note self.position.diff_refs == diff_refs end - # If you update this method remember to also update the scope `resolvable` - def resolvable? - !system? && for_merge_request? - end - - def resolved? - return false unless resolvable? - - self.resolved_at.present? - end - - # If you update this method remember to also update `.resolve!` - def resolve!(current_user) - return unless resolvable? - return if resolved? - - self.resolved_at = Time.now - self.resolved_by = current_user - save! - end - - # If you update this method remember to also update `.unresolve!` - def unresolve! - return unless resolvable? - return unless resolved? - - self.resolved_at = nil - self.resolved_by = nil - save! - end - - def discussion - return unless resolvable? - - self.noteable.find_diff_discussion(self.discussion_id) - end - private def supported? @@ -140,33 +89,13 @@ class DiffNote < Note end def set_original_position - self.original_position = self.position.dup + self.original_position = self.position.dup unless self.original_position&.complete? end def set_line_code self.line_code = self.position.line_code(self.project.repository) end - def ensure_original_discussion_id - return unless self.persisted? - return if self.original_discussion_id - - set_original_discussion_id - update_column(:original_discussion_id, self.original_discussion_id) - end - - def set_original_discussion_id - self.original_discussion_id = Digest::SHA1.hexdigest(build_original_discussion_id) - end - - def build_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position) - end - - def build_original_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position) - end - def update_position return unless supported? return if for_commit? diff --git a/app/models/discussion.rb b/app/models/discussion.rb index bbe813db823..314aea2c63a 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,5 +1,5 @@ class Discussion - NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + MEMOIZED_VALUES = [] # rubocop:disable Style/MutableConstant attr_reader :notes @@ -11,12 +11,6 @@ class Discussion :for_commit?, :for_merge_request?, - :line_code, - :original_line_code, - :diff_file, - :for_line?, - :active?, - to: :first_note delegate :resolved_at, @@ -25,29 +19,46 @@ class Discussion to: :last_resolved_note, allow_nil: true - delegate :blob, - :highlighted_diff_lines, - :diff_lines, + def self.build(notes, noteable = nil) + notes.first.discussion_class(noteable).new(notes, noteable) + end - to: :diff_file, - allow_nil: true + def self.build_collection(notes, noteable = nil) + notes.group_by { |n| n.discussion_id(noteable) }.values.map { |notes| build(notes, noteable) } + end - def self.for_notes(notes) - notes.group_by(&:discussion_id).values.map { |notes| new(notes) } + def self.discussion_id(note) + Digest::SHA1.hexdigest(build_discussion_id(note).join("-")) end - def self.for_diff_notes(notes) - notes.group_by(&:line_code).values.map { |notes| new(notes) } + # Optionally override the discussion ID at runtime depending on circumstances + def self.override_discussion_id(note) + nil end - def initialize(notes) - @notes = notes + def self.build_discussion_id(note) + noteable_id = note.noteable_id || note.commit_id + [:discussion, note.noteable_type.try(:underscore), noteable_id] end - def last_resolved_note - return unless resolved? + def self.original_discussion_id(note) + original_discussion_id = build_original_discussion_id(note) + if original_discussion_id + Digest::SHA1.hexdigest(original_discussion_id.join("-")) + else + note.discussion_id + end + end - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + # Optionally build a separate original discussion ID that will never change, + # if the main discussion ID _can_ change, like in the case of DiffDiscussion. + def self.build_original_discussion_id(note) + nil + end + + def initialize(notes, noteable = nil) + @notes = notes + @noteable = noteable end def last_updated_at @@ -59,42 +70,64 @@ class Discussion end def id - first_note.discussion_id + first_note.discussion_id(noteable) end alias_method :to_param, :id + def original_id + first_note.original_discussion_id + end + def diff_discussion? - first_note.diff_note? + false + end + + def render_as_individual_notes? + false end - def legacy_diff_discussion? - notes.any?(&:legacy_diff_note?) + def potentially_resolvable? + first_note.for_merge_request? end def resolvable? return @resolvable if @resolvable.present? - @resolvable = diff_discussion? && notes.any?(&:resolvable?) + @resolvable = potentially_resolvable? && notes.any?(&:resolvable?) end + MEMOIZED_VALUES << :resolvable def resolved? return @resolved if @resolved.present? @resolved = resolvable? && notes.none?(&:to_be_resolved?) end + MEMOIZED_VALUES << :resolved def first_note - @first_note ||= @notes.first + @first_note ||= notes.first end + MEMOIZED_VALUES << :first_note def first_note_to_resolve - @first_note_to_resolve ||= notes.detect(&:to_be_resolved?) + return unless resolvable? + + @first_note_to_resolve ||= notes.find(&:to_be_resolved?) end + MEMOIZED_VALUES << :first_note_to_resolve + + def last_resolved_note + return unless resolved? + + @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + end + MEMOIZED_VALUES << :last_resolved_note def last_note - @last_note ||= @notes.last + @last_note ||= notes.last end + MEMOIZED_VALUES << :last_note def resolved_notes notes.select(&:resolved?) @@ -124,25 +157,12 @@ class Discussion update { |notes| notes.unresolve! } end - def for_target?(target) - self.noteable == target && !diff_discussion? - end - - def active? - return @active if @active.present? - - @active = first_note.active? - end - def collapsed? - return false unless diff_discussion? - if resolvable? # New diff discussions only disappear once they are marked resolved resolved? else - # Old diff discussions disappear once they become outdated - !active? + false end end @@ -151,52 +171,22 @@ class Discussion end def reply_attributes - data = { - noteable_type: first_note.noteable_type, - noteable_id: first_note.noteable_id, - commit_id: first_note.commit_id, - discussion_id: self.id, - } - - if diff_discussion? - data[:note_type] = first_note.type - - data.merge!(first_note.diff_attributes) - end - - data - end - - # Returns an array of at most 16 highlighted lines above a diff note - def truncated_diff_lines(highlight: true) - lines = highlight ? highlighted_diff_lines : diff_lines - prev_lines = [] - - lines.each do |line| - if line.meta? - prev_lines.clear - else - prev_lines << line - - break if for_line?(line) - - prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES - end - end - - prev_lines + first_note.slice(:type, :noteable_type, :noteable_id, :commit_id) end private def update - notes_relation = DiffNote.where(id: notes.map(&:id)).fresh + # Do not select `Note.resolvable`, so that system notes remain in the collection + notes_relation = Note.where(id: notes.map(&:id)) + yield(notes_relation) # Set the notes array to the updated notes - @notes = notes_relation.to_a + @notes = notes_relation.fresh.to_a - # Reset the memoized values - @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil + MEMOIZED_VALUES.each do |var| + instance_variable_set(:"@#{var}", nil) + end end end diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb new file mode 100644 index 00000000000..fc168ae74a9 --- /dev/null +++ b/app/models/discussion_note.rb @@ -0,0 +1,9 @@ +class DiscussionNote < Note + NOTEABLE_TYPES = %w(MergeRequest).freeze + + validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } + + def discussion_class(*) + SimpleDiscussion + end +end diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb new file mode 100644 index 00000000000..8c82d217126 --- /dev/null +++ b/app/models/individual_note_discussion.rb @@ -0,0 +1,13 @@ +class IndividualNoteDiscussion < Discussion + def self.build_discussion_id(note) + [*super(note), SecureRandom.hex] + end + + def potentially_resolvable? + false + end + + def render_as_individual_notes? + true + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index 472796df9df..5010b6e1edf 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord' class Issue < ActiveRecord::Base include InternalId include Issuable + include Noteable include Referable include Sortable include Spammable diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb new file mode 100644 index 00000000000..e13f17abdbe --- /dev/null +++ b/app/models/legacy_diff_discussion.rb @@ -0,0 +1,21 @@ +class LegacyDiffDiscussion < DiffDiscussion + def self.unique_position_identifier(note) + note.line_code + end + + def self.build_original_discussion_id(note) + Discussion.build_original_discussion_id(note) + end + + def legacy_diff_discussion? + true + end + + def potentially_resolvable? + false + end + + def collapsed? + !active? + end +end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 40277a9b139..4952a1ce2ca 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -7,10 +7,8 @@ class LegacyDiffNote < Note before_create :set_diff - class << self - def build_discussion_id(noteable_type, noteable_id, line_code) - [super(noteable_type, noteable_id), line_code].join("-") - end + def discussion_class(*) + LegacyDiffDiscussion end def legacy_diff_note? @@ -119,8 +117,4 @@ class LegacyDiffNote < Note diffs = noteable.raw_diffs(Commit.max_diff_options) diffs.find { |d| d.new_path == self.diff.new_path } end - - def build_discussion_id - self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code) - end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 8d740adb771..546fd2b8e35 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,6 +1,7 @@ class MergeRequest < ActiveRecord::Base include InternalId include Issuable + include Noteable include Referable include Sortable @@ -475,44 +476,32 @@ class MergeRequest < ActiveRecord::Base ) end - def discussions - @discussions ||= self.related_notes. - inc_relations_for_view. - fresh. - discussions - end - - def diff_discussions - @diff_discussions ||= self.notes.diff_notes.discussions - end + alias_method :discussion_notes, :related_notes def resolvable_discussions - @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?) - end - - def discussions_can_be_resolved_by?(user) - resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) } - end - - def find_diff_discussion(discussion_id) - notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a - return if notes.empty? - - Discussion.new(notes) + @resolvable_discussions ||= notes.resolvable.discussions end def discussions_resolvable? - diff_discussions.any?(&:resolvable?) + resolvable_discussions.any?(&:resolvable?) end def discussions_resolved? - discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?) + discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?) end def discussions_to_be_resolved? discussions_resolvable? && !discussions_resolved? end + def discussions_to_be_resolved + @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?) + end + + def discussions_can_be_resolved_by?(user) + discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) } + end + def mergeable_discussions_state? return true unless project.only_allow_merge_if_all_discussions_are_resolved? diff --git a/app/models/note.rb b/app/models/note.rb index 16d66cb1427..6385747b571 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -8,6 +8,7 @@ class Note < ActiveRecord::Base include FasterCacheKeys include CacheMarkdownField include AfterCommitQueue + include ResolvableNote cache_markdown_field :note, pipeline: :note @@ -32,9 +33,6 @@ class Note < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :updated_by, class_name: "User" - # Only used by DiffNote, but defined here so that it can be used in `Note.includes` - belongs_to :resolved_by, class_name: "User" - has_many :todos, dependent: :destroy has_many :events, as: :target, dependent: :destroy has_one :system_note_metadata @@ -54,6 +52,7 @@ class Note < ActiveRecord::Base validates :noteable_id, presence: true, unless: [:for_commit?, :importing?] validates :commit_id, presence: true, if: :for_commit? validates :author, presence: true + validates :discussion_id, :original_discussion_id, presence: true, format: { with: /\A\h{40}\z/ } validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note| unless note.noteable.try(:project) == note.project @@ -76,7 +75,7 @@ class Note < ActiveRecord::Base end scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) } - scope :non_diff_notes, ->{ where(type: ['Note', nil]) } + scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) } scope :with_associations, -> do # FYI noteable cannot be loaded for LegacyDiffNote for commits @@ -84,9 +83,9 @@ class Note < ActiveRecord::Base project: [:project_members, { group: [:group_members] }]) end - after_initialize :ensure_discussion_id + after_initialize :ensure_discussion_id, :ensure_original_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code - before_validation :set_discussion_id + before_validation :set_discussion_id, :set_original_discussion_id, on: :create after_save :keep_around_commit, unless: :for_personal_snippet? after_save :expire_etag_cache @@ -95,22 +94,31 @@ class Note < ActiveRecord::Base ActiveModel::Name.new(self, nil, 'note') end - def build_discussion_id(noteable_type, noteable_id) - [:discussion, noteable_type.try(:underscore), noteable_id].join("-") + def discussions(noteable = nil) + Discussion.build_collection(fresh, noteable) end - def discussion_id(*args) - Digest::SHA1.hexdigest(build_discussion_id(*args)) + def find_original_discussion(discussion_id) + note = find_by(original_discussion_id: discussion_id) + return unless note + + note.to_discussion end - def discussions - Discussion.for_notes(fresh) + def find_discussion(discussion_id) + notes = where(discussion_id: discussion_id).fresh.to_a + return if notes.empty? + + Discussion.build(notes) end def grouped_diff_discussions - active_notes = diff_notes.fresh.select(&:active?) - Discussion.for_diff_notes(active_notes). - map { |d| [d.line_code, d] }.to_h + diff_notes. + fresh. + select(&:active?). + group_by(&:line_code). + map { |line_code, notes| [line_code, DiffDiscussion.build(notes)] }. + to_h end def count_for_collection(ids, type) @@ -121,7 +129,7 @@ class Note < ActiveRecord::Base end def cross_reference? - system && SystemNoteService.cross_reference?(note) + system? && SystemNoteService.cross_reference?(note) end def diff_note? @@ -140,18 +148,6 @@ class Note < ActiveRecord::Base true end - def resolvable? - false - end - - def resolved? - false - end - - def to_be_resolved? - resolvable? && !resolved? - end - def max_attachment_size current_application_settings.max_attachment_size.megabytes.to_i end @@ -228,7 +224,7 @@ class Note < ActiveRecord::Base end def can_be_award_emoji? - noteable.is_a?(Awardable) + noteable.is_a?(Awardable) && !part_of_discussion? end def contains_emoji_only? @@ -239,6 +235,42 @@ class Note < ActiveRecord::Base for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore end + def can_be_discussion_note? + DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) + 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 + if noteable && noteable != self.noteable && for_commit? + CommitDiscussion + else + IndividualNoteDiscussion + end + end + + def discussion_id(noteable = nil) + discussion_class(noteable).override_discussion_id(self) || super() + end + + # Returns a discussion containing just this note + def to_discussion(noteable = nil) + Discussion.build([self], noteable) + end + + # Returns the entire discussion this note is part of + def discussion + if part_of_discussion? + self.noteable.notes.find_discussion(self.discussion_id) + else + to_discussion + end + end + + def part_of_discussion? + !to_discussion.render_as_individual_notes? + end + private def keep_around_commit @@ -264,17 +296,21 @@ class Note < ActiveRecord::Base end def set_discussion_id - self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id) + self.discussion_id ||= discussion_class.discussion_id(self) end - def build_discussion_id - if for_merge_request? - # Notes on merge requests are always in a discussion of their own, - # so we generate a unique discussion ID. - [:discussion, :note, SecureRandom.hex].join("-") - else - self.class.build_discussion_id(noteable_type, noteable_id || commit_id) - end + def ensure_original_discussion_id + return unless self.persisted? + # Needed in case the SELECT statement doesn't ask for `original_discussion_id` + return unless self.has_attribute?(:original_discussion_id) + return if self.original_discussion_id + + set_original_discussion_id + update_column(:original_discussion_id, self.original_discussion_id) + end + + def set_original_discussion_id + self.original_discussion_id = discussion_class.original_discussion_id(self) end def expire_etag_cache diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index f4bcb49b34d..6770af6fffd 100644 --- a/app/models/sent_notification.rb +++ b/app/models/sent_notification.rb @@ -5,10 +5,11 @@ class SentNotification < ActiveRecord::Base belongs_to :noteable, polymorphic: true belongs_to :recipient, class_name: "User" - validates :project, :recipient, :reply_key, presence: true - validates :reply_key, uniqueness: true + validates :project, :recipient, presence: true + validates :reply_key, presence: true, uniqueness: true validates :noteable_id, presence: true, unless: :for_commit? validates :commit_id, presence: true, if: :for_commit? + validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true } validate :note_valid after_save :keep_around_commit @@ -34,23 +35,20 @@ class SentNotification < ActiveRecord::Base end attrs.reverse_merge!( - project: noteable.project, - noteable_type: noteable.class.name, - noteable_id: noteable_id, - commit_id: commit_id, - recipient_id: recipient_id, - reply_key: reply_key + project: noteable.project, + recipient_id: recipient_id, + reply_key: reply_key, + + noteable_type: noteable.class.name, + noteable_id: noteable_id, + commit_id: commit_id, ) create(attrs) end def record_note(note, recipient_id, reply_key, attrs = {}) - if note.diff_note? - attrs[:note_type] = note.type - - attrs.merge!(note.diff_attributes) - end + attrs[:in_reply_to_discussion_id] = note.original_discussion_id record(note.noteable, recipient_id, reply_key, attrs) end @@ -89,31 +87,34 @@ class SentNotification < ActiveRecord::Base self.reply_key end - def note_attributes - { - project: self.project, - author: self.recipient, - type: self.note_type, - noteable_type: self.noteable_type, - noteable_id: self.noteable_id, - commit_id: self.commit_id, - line_code: self.line_code, - position: self.position.to_json + def note_params + attrs = { + noteable_type: self.noteable_type, + noteable_id: self.noteable_id, + commit_id: self.commit_id } - end - def create_note(note) - Notes::CreateService.new( - self.project, - self.recipient, - self.note_attributes.merge(note: note) - ).execute + if self.in_reply_to_discussion_id.present? + attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id + else + attrs.merge!( + type: self.note_type, + + # LegacyDiffNote + line_code: self.line_code, + + # DiffNote + position: self.position.to_json + ) + end + + attrs end private def note_valid - Note.new(note_attributes.merge(note: "Test")).valid? + Notes::BuildService.new(self.project, self.recipient, note_params.merge(note: 'Test')).execute.valid? end def keep_around_commit diff --git a/app/models/simple_discussion.rb b/app/models/simple_discussion.rb new file mode 100644 index 00000000000..a5ef5c565e7 --- /dev/null +++ b/app/models/simple_discussion.rb @@ -0,0 +1,9 @@ +class SimpleDiscussion < Discussion + def self.build_discussion_id(note) + [*super(note), SecureRandom.hex] + end + + def reply_attributes + super.merge(discussion_id: self.id) + end +end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 30aca62499c..380835707e8 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -2,6 +2,7 @@ class Snippet < ActiveRecord::Base include Gitlab::VisibilityLevel include Linguist::BlobHelper include CacheMarkdownField + include Noteable include Participable include Referable include Sortable diff --git a/app/services/concerns/issues/resolve_discussions.rb b/app/services/concerns/issues/resolve_discussions.rb index 297c7d696c3..378967f0231 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -25,7 +25,7 @@ module Issues Array(discussion_or_nil) else merge_request_to_resolve_discussions_of - .resolvable_discussions + .discussions_to_be_resolved end end end diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb new file mode 100644 index 00000000000..47ed3e3fc3d --- /dev/null +++ b/app/services/notes/build_service.rb @@ -0,0 +1,29 @@ +module Notes + class BuildService < BaseService + def execute + # TODO: Remove when we use a selectbox instead of a submit button + params[:type] = DiscussionNote.name if params.delete(:new_discussion) + + in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id) + if project && in_reply_to_discussion_id.present? + discussion = + project.notes.find_original_discussion(in_reply_to_discussion_id) || + project.notes.find_discussion(in_reply_to_discussion_id) + + unless discussion + note = Note.new + note.errors.add(:base, 'Discussion to reply to cannot be found') + return note + end + + params.merge!(discussion.reply_attributes) + end + + note = Note.new(params) + note.project = project + note.author = current_user + + note + end + end +end diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb index 61d66a26932..c08cddcbee5 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -3,10 +3,8 @@ module Notes def execute merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha) - note = Note.new(params) - note.project = project - note.author = current_user - note.system = false + note = Notes::BuildService.new(project, current_user, params).execute + return note unless note.valid? # We execute commands (extracted from `params[:note]`) on the noteable # **before** we save the note because if the note consists of commands diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 35cfcc3682e..c9e25c7aaa2 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -228,12 +228,10 @@ module SystemNoteService def discussion_continued_in_issue(discussion, project, author, issue) body = "created #{issue.to_reference} to continue this discussion" + note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body) - note_params = discussion.reply_attributes.merge(project: project, author: author, note: body) - note_params[:type] = note_params.delete(:note_type) - - note = Note.create(note_params.merge(system: true)) - note.system_note_metadata = SystemNoteMetadata.new({ action: 'discussion' }) + note = Note.create(note_attributes.merge(system: true)) + note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion') note end diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 2d78c55211e..38112f67881 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -18,19 +18,21 @@ .inline.discussion-headline-light = discussion.author.to_reference - started a discussion on + started a discussion - - if discussion.for_commit? + - if discussion.for_commit? && @noteable != discussion.noteable + on - commit = discussion.noteable - if commit commit - = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code), class: 'monospace' + - anchor = discussion.line_code if discussion.diff_discussion? + = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor), class: 'monospace' - else a deleted commit - - else + - elsif discussion.diff_discussion? + on - if discussion.active? - = link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) do - the diff + = link_to 'the diff', discussion_diff_path(discussion) - else an outdated diff diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index 2789391819c..c8fcc37d1ab 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,18 +1,19 @@ -%ul.notes{ data: { discussion_id: discussion.id } } +%ul.notes{ data: { discussion_id: discussion.id, original_discussion_id: discussion.original_id } } = render partial: "projects/notes/note", collection: discussion.notes, as: :note - if current_user .discussion-reply-holder - - if discussion.diff_discussion? + - if discussion.potentially_resolvable? - line_type = local_assigns.fetch(:line_type, nil) .btn-group-justified.discussion-with-resolve-btn{ role: "group" } .btn-group{ role: "group" } = link_to_reply_discussion(discussion, line_type) + = render "discussions/resolve_all", discussion: discussion - - if discussion.for_merge_request? - .btn-group.discussion-actions - = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable - = render "discussions/jump_to_next", discussion: discussion + + .btn-group.discussion-actions + = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable + = render "discussions/jump_to_next", discussion: discussion - else = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_resolve_all.html.haml b/app/views/discussions/_resolve_all.html.haml index e30ee1b0e05..689a22acd27 100644 --- a/app/views/discussions/_resolve_all.html.haml +++ b/app/views/discussions/_resolve_all.html.haml @@ -1,9 +1,8 @@ -- if discussion.for_merge_request? - %resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'", - ":merge-request-id" => discussion.noteable.iid, - ":can-resolve" => discussion.can_resolve?(current_user), - "inline-template" => true } - .btn-group{ role: "group", "v-if" => "showButton" } - %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" } - = icon("spinner spin", "v-show" => "loading") - {{ buttonText }} +%resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'", + ":merge-request-id" => discussion.noteable.iid, + ":can-resolve" => discussion.can_resolve?(current_user), + "inline-template" => true } + .btn-group{ role: "group", "v-if" => "showButton" } + %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading", "v-cloak" => "true" } + = icon("spinner spin", "v-show" => "loading") + {{ buttonText }} diff --git a/app/views/projects/commit/show.html.haml b/app/views/projects/commit/show.html.haml index d5fc283aa8d..0d11da2451a 100644 --- a/app/views/projects/commit/show.html.haml +++ b/app/views/projects/commit/show.html.haml @@ -10,6 +10,7 @@ - else .block-connector = render "projects/diffs/diffs", diffs: @diffs, environment: @environment + = render "projects/notes/notes_with_form" - if can_collaborate_with_project? - %w(revert cherry-pick).each do |type| diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index b561052e721..80db16ea578 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -4,12 +4,18 @@ = hidden_field_tag :view, diff_view = hidden_field_tag :line_type = hidden_field_tag :merge_request_diff_head_sha, @note.noteable.try(:diff_head_sha) + = hidden_field_tag :in_reply_to_discussion_id + = note_target_fields(@note) - = f.hidden_field :commit_id - = f.hidden_field :line_code - = f.hidden_field :noteable_id = f.hidden_field :noteable_type + = f.hidden_field :noteable_id + = f.hidden_field :commit_id = f.hidden_field :type + + -# LegacyDiffNote + = f.hidden_field :line_code + + -# DiffNote = f.hidden_field :position = render layout: 'projects/md_preview', locals: { preview_class: "md-preview", referenced_users: true } do @@ -23,6 +29,11 @@ .note-form-actions.clearfix = f.submit 'Comment', class: "btn btn-nr btn-create append-right-10 comment-btn js-comment-button" + + - if @note.can_be_discussion_note? + = submit_tag 'Start discussion', name: 'new_discussion', class: "btn btn-nr append-right-10 btn-inverted js-note-new-discussion" + = yield(:note_actions) + %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } Discard draft diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 6c0e6d48d6c..097dd17c094 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -31,7 +31,7 @@ - if note.resolvable? - can_resolve = can?(current_user, :resolve_note, note) %resolve-btn{ "project-path" => project_path(note.project), - "discussion-id" => note.discussion_id, + "discussion-id" => note.discussion_id(@noteable), ":note-id" => note.id, ":resolved" => note.resolved?, ":can-resolve" => can_resolve, diff --git a/app/views/projects/notes/_notes.html.haml b/app/views/projects/notes/_notes.html.haml index 022578bd6db..50809068453 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -1,7 +1,7 @@ - if @discussions.present? - @discussions.each do |discussion| - - if discussion.for_target?(@noteable) - = render partial: "projects/notes/note", object: discussion.first_note, as: :note + - if discussion.render_as_individual_notes? + = render partial: "projects/notes/note", collection: discussion.notes, as: :note - else = render 'discussions/discussion', discussion: discussion - else |