diff options
76 files changed, 1323 insertions, 982 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 diff --git a/changelogs/unreleased/new-resolvable-discussion.yml b/changelogs/unreleased/new-resolvable-discussion.yml new file mode 100644 index 00000000000..af1de6a45e7 --- /dev/null +++ b/changelogs/unreleased/new-resolvable-discussion.yml @@ -0,0 +1,4 @@ +--- +title: Add option to start a new resolvable discussion in an MR +merge_request: +author: diff --git a/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb b/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb new file mode 100644 index 00000000000..d56d83ca1d3 --- /dev/null +++ b/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb @@ -0,0 +1,29 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddInReplyToDiscussionIdToSentNotifications < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def change + add_column :sent_notifications, :in_reply_to_discussion_id, :string + end +end diff --git a/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb b/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb new file mode 100644 index 00000000000..2c823ed84a9 --- /dev/null +++ b/db/migrate/20170308220217_add_index_to_note_original_discussion_id.rb @@ -0,0 +1,18 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexToNoteOriginalDiscussionId < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :notes, :original_discussion_id + end + + def down + remove_index :notes, :original_discussion_id if index_exists? :notes, :original_discussion_id + end +end diff --git a/db/schema.rb b/db/schema.rb index ccf18d07179..1b23bab5af2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -736,6 +736,7 @@ ActiveRecord::Schema.define(version: 20170402231018) do add_index "notes", ["note"], name: "index_notes_on_note_trigram", using: :gin, opclasses: {"note"=>"gin_trgm_ops"} add_index "notes", ["noteable_id", "noteable_type"], name: "index_notes_on_noteable_id_and_noteable_type", using: :btree add_index "notes", ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree + add_index "notes", ["original_discussion_id"], name: "index_notes_on_original_discussion_id", using: :btree add_index "notes", ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree add_index "notes", ["updated_at"], name: "index_notes_on_updated_at", using: :btree @@ -999,6 +1000,7 @@ ActiveRecord::Schema.define(version: 20170402231018) do t.string "line_code" t.string "note_type" t.text "position" + t.string "in_reply_to_discussion_id" end add_index "sent_notifications", ["reply_key"], name: "index_sent_notifications_on_reply_key", unique: true, using: :btree diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb index d87ba427f4b..0a9e928d13f 100644 --- a/lib/gitlab/email/handler/create_note_handler.rb +++ b/lib/gitlab/email/handler/create_note_handler.rb @@ -45,13 +45,7 @@ module Gitlab Notes::CreateService.new( project, author, - note: message, - noteable_type: sent_notification.noteable_type, - noteable_id: sent_notification.noteable_id, - commit_id: sent_notification.commit_id, - line_code: sent_notification.line_code, - position: sent_notification.position, - type: sent_notification.note_type + sent_notification.note_params.merge(note: message) ).execute end end diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb index b223a22ae60..9545b980fbf 100644 --- a/spec/controllers/projects/commit_controller_spec.rb +++ b/spec/controllers/projects/commit_controller_spec.rb @@ -266,7 +266,7 @@ describe Projects::CommitController do diff_for_path(id: commit2.id, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey - expect(assigns(:comments_target)).to eq(noteable_type: 'Commit', + expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'Commit', commit_id: commit2.id) end diff --git a/spec/controllers/projects/discussions_controller_spec.rb b/spec/controllers/projects/discussions_controller_spec.rb index 79ab364a6f3..fe62898fa9b 100644 --- a/spec/controllers/projects/discussions_controller_spec.rb +++ b/spec/controllers/projects/discussions_controller_spec.rb @@ -4,7 +4,7 @@ describe Projects::DiscussionsController do let(:user) { create(:user) } let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } - let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) } + let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } let(:discussion) { note.discussion } let(:request_params) do diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 734966d50b2..731e511c270 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -508,7 +508,7 @@ describe Projects::IssuesController do end context 'resolving discussions in MergeRequest' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 72f41f7209a..6f8755645a1 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -574,7 +574,7 @@ describe Projects::MergeRequestsController do diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path) expect(assigns(:diff_notes_disabled)).to be_falsey - expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest', + expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'MergeRequest', noteable_id: merge_request.id) end diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index d80780b1d90..87084806568 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -14,7 +14,15 @@ describe Projects::NotesController do } end + describe 'GET index' do + # It renders the discussion partial for any threaded note + # TODO: Test + end + describe 'POST create' do + # Test :type, :new_discussion, :in_reply_to_discussion_id (in_reply_to_id?) + # TODO: Test + let(:merge_request) { create(:merge_request) } let(:project) { merge_request.source_project } let(:request_params) do @@ -49,7 +57,8 @@ describe Projects::NotesController do note: 'some note', noteable_id: merge_request.id.to_s, noteable_type: 'MergeRequest', - merge_request_diff_head_sha: 'sha' + merge_request_diff_head_sha: 'sha', + in_reply_to_discussion_id: nil } expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true)) diff --git a/spec/factories/discussions.rb b/spec/factories/discussions.rb new file mode 100644 index 00000000000..5e3a9b1e698 --- /dev/null +++ b/spec/factories/discussions.rb @@ -0,0 +1,5 @@ +FactoryGirl.define do + factory :discussion do + # TODO: Implement + end +end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index fe19a404e16..755c6d7e031 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -16,6 +16,15 @@ FactoryGirl.define do factory :note_on_personal_snippet, traits: [:on_personal_snippet] factory :system_note, traits: [:system] + factory :discussion_note_on_merge_request, traits: [:on_merge_request], class: DiscussionNote do + association :project, :repository + + trait :resolved do + resolved_at { Time.now } + resolved_by { create(:user) } + end + end + factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do association :project, :repository end diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb index 572bca3de21..58f897cba3e 100644 --- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb +++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb @@ -4,7 +4,7 @@ feature 'Resolving all open discussions in a merge request from an issue', featu let(:user) { create(:user) } let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project) } - let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first } + let!(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } describe 'as a user with access to the project' do before do diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb index fab2d532e06..783f2e93909 100644 --- a/spec/features/notes_on_merge_requests_spec.rb +++ b/spec/features/notes_on_merge_requests_spec.rb @@ -25,7 +25,7 @@ describe 'Comments', feature: true do describe 'the note form' do it 'is valid' do is_expected.to have_css('.js-main-target-form', visible: true, count: 1) - expect(find('.js-main-target-form input[type=submit]').value). + expect(find('.js-main-target-form .js-comment-button').value). to eq('Comment') page.within('.js-main-target-form') do expect(page).not_to have_link('Cancel') diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 77a04507be1..d2ec42ae70f 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -202,4 +202,8 @@ describe NotesFinder do end end end + + describe '#target' do + # TODO: Test + end end diff --git a/spec/models/commit_discussion_spec.rb b/spec/models/commit_discussion_spec.rb new file mode 100644 index 00000000000..6a5042d8827 --- /dev/null +++ b/spec/models/commit_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe CommitDiscussion, model: true do + # TODO: Test +end diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb new file mode 100644 index 00000000000..51855fb2f0d --- /dev/null +++ b/spec/models/concerns/noteable_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Noteable, model: true do + # TODO: Test +end diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb new file mode 100644 index 00000000000..3306b311e4a --- /dev/null +++ b/spec/models/concerns/resolvable_note_spec.rb @@ -0,0 +1,276 @@ +require 'spec_helper' + +describe Note, ResolvableNote, models: true do + subject { create(:discussion_note_on_merge_request) } + + describe '.resolvable' do + # TODO: Test + end + + describe '.resolved' do + # TODO: Test + end + + describe '.unresolved' do + # TODO: Test + end + + describe ".resolve!" do + let(:current_user) { create(:user) } + let!(:commit_note) { create(:diff_note_on_commit) } + let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) } + let!(:unresolved_note) { create(:discussion_note_on_merge_request) } + + before do + described_class.resolve!(current_user) + + commit_note.reload + resolved_note.reload + unresolved_note.reload + end + + it 'resolves only the resolvable, not yet resolved notes' do + expect(commit_note.resolved_at).to be_nil + expect(resolved_note.resolved_by).not_to eq(current_user) + expect(unresolved_note.resolved_at).not_to be_nil + expect(unresolved_note.resolved_by).to eq(current_user) + end + end + + describe ".unresolve!" do + let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved) } + + before do + described_class.unresolve! + + resolved_note.reload + end + + it 'unresolves the resolved notes' do + expect(resolved_note.resolved_by).to be_nil + expect(resolved_note.resolved_at).to be_nil + end + end + + describe '#resolvable?' do + context "when potentially resolvable" do + before do + allow(subject).to receive(:discussion_resolvable?).and_return(true) + end + + context "when a system note" do + before do + subject.system = true + end + + it "returns false" do + expect(subject.resolvable?).to be false + end + end + + context "when a regular note" do + it "returns true" do + expect(subject.resolvable?).to be true + end + end + end + + context "when not potentially resolvable" do + before do + allow(subject).to receive(:discussion_resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.resolvable?).to be false + end + end + end + + describe "#to_be_resolved?" do + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns false" do + expect(subject.to_be_resolved?).to be false + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when resolved" do + before do + allow(subject).to receive(:resolved?).and_return(true) + end + + it "returns false" do + expect(subject.to_be_resolved?).to be false + end + end + + context "when not resolved" do + before do + allow(subject).to receive(:resolved?).and_return(false) + end + + it "returns true" do + expect(subject.to_be_resolved?).to be true + end + end + end + end + + describe "#resolved?" do + # TODO: Test + end + + describe "#resolve!" do + let(:current_user) { create(:user) } + + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns nil" do + expect(subject.resolve!(current_user)).to be_nil + end + + it "doesn't set resolved_at" do + subject.resolve!(current_user) + + expect(subject.resolved_at).to be_nil + end + + it "doesn't set resolved_by" do + subject.resolve!(current_user) + + expect(subject.resolved_by).to be_nil + end + + it "doesn't mark as resolved" do + subject.resolve!(current_user) + + expect(subject.resolved?).to be false + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when already resolved" do + let(:user) { create(:user) } + + before do + subject.resolve!(user) + end + + it "returns nil" do + expect(subject.resolve!(current_user)).to be_nil + end + + it "doesn't change resolved_at" do + expect(subject.resolved_at).not_to be_nil + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at } + end + + it "doesn't change resolved_by" do + expect(subject.resolved_by).to eq(user) + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by } + end + + it "doesn't change resolved status" do + expect(subject.resolved?).to be true + + expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } + end + end + + context "when not yet resolved" do + it "returns true" do + expect(subject.resolve!(current_user)).to be true + end + + it "sets resolved_at" do + subject.resolve!(current_user) + + expect(subject.resolved_at).not_to be_nil + end + + it "sets resolved_by" do + subject.resolve!(current_user) + + expect(subject.resolved_by).to eq(current_user) + end + + it "marks as resolved" do + subject.resolve!(current_user) + + expect(subject.resolved?).to be true + end + end + end + end + + describe "#unresolve!" do + context "when not resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(false) + end + + it "returns nil" do + expect(subject.unresolve!).to be_nil + end + end + + context "when resolvable" do + before do + allow(subject).to receive(:resolvable?).and_return(true) + end + + context "when resolved" do + let(:user) { create(:user) } + + before do + subject.resolve!(user) + end + + it "returns true" do + expect(subject.unresolve!).to be true + end + + it "unsets resolved_at" do + subject.unresolve! + + expect(subject.resolved_at).to be_nil + end + + it "unsets resolved_by" do + subject.unresolve! + + expect(subject.resolved_by).to be_nil + end + + it "unmarks as resolved" do + subject.unresolve! + + expect(subject.resolved?).to be false + end + end + + context "when not resolved" do + it "returns nil" do + expect(subject.unresolve!).to be_nil + end + end + end + end +end diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb new file mode 100644 index 00000000000..8f3a4a8cc49 --- /dev/null +++ b/spec/models/diff_discussion_spec.rb @@ -0,0 +1,24 @@ +require 'spec_helper' + +describe DiffDiscussion, model: true do + # TODO: Test + + describe "#truncated_diff_lines" do + let(:truncated_lines) { subject.truncated_diff_lines } + + context "when diff is greater than allowed number of truncated diff lines " do + it "returns fewer lines" do + expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + + expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES + end + end + + context "when some diff lines are meta" do + it "returns no meta lines" do + expect(subject.diff_lines).to include(be_meta) + expect(truncated_lines).not_to include(be_meta) + end + end + end +end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 9ea3a4b7020..92059936188 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -31,43 +31,6 @@ describe DiffNote, models: true do subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) } - describe ".resolve!" do - let(:current_user) { create(:user) } - let!(:commit_note) { create(:diff_note_on_commit) } - let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) } - let!(:unresolved_note) { create(:diff_note_on_merge_request) } - - before do - described_class.resolve!(current_user) - - commit_note.reload - resolved_note.reload - unresolved_note.reload - end - - it 'resolves only the resolvable, not yet resolved notes' do - expect(commit_note.resolved_at).to be_nil - expect(resolved_note.resolved_by).not_to eq(current_user) - expect(unresolved_note.resolved_at).not_to be_nil - expect(unresolved_note.resolved_by).to eq(current_user) - end - end - - describe ".unresolve!" do - let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) } - - before do - described_class.unresolve! - - resolved_note.reload - end - - it 'unresolves the resolved notes' do - expect(resolved_note.resolved_by).to be_nil - expect(resolved_note.resolved_at).to be_nil - end - end - describe "#position=" do context "when provided a string" do it "sets the position" do @@ -94,6 +57,10 @@ describe DiffNote, models: true do end end + describe "#original_position=" do + # TODO: Test + end + describe "#diff_file" do it "returns the correct diff file" do diff_file = subject.diff_file @@ -226,252 +193,6 @@ describe DiffNote, models: true do end end - describe "#resolvable?" do - context "when noteable is a commit" do - subject { create(:diff_note_on_commit, project: project, position: position) } - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - - context "when noteable is a merge request" do - context "when a system note" do - before do - subject.system = true - end - - it "returns false" do - expect(subject.resolvable?).to be false - end - end - - context "when a regular note" do - it "returns true" do - expect(subject.resolvable?).to be true - end - end - end - end - - describe "#to_be_resolved?" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns false" do - expect(subject.to_be_resolved?).to be false - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when resolved" do - before do - allow(subject).to receive(:resolved?).and_return(true) - end - - it "returns false" do - expect(subject.to_be_resolved?).to be false - end - end - - context "when not resolved" do - before do - allow(subject).to receive(:resolved?).and_return(false) - end - - it "returns true" do - expect(subject.to_be_resolved?).to be true - end - end - end - end - - describe "#resolve!" do - let(:current_user) { create(:user) } - - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil - end - - it "doesn't set resolved_at" do - subject.resolve!(current_user) - - expect(subject.resolved_at).to be_nil - end - - it "doesn't set resolved_by" do - subject.resolve!(current_user) - - expect(subject.resolved_by).to be_nil - end - - it "doesn't mark as resolved" do - subject.resolve!(current_user) - - expect(subject.resolved?).to be false - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when already resolved" do - let(:user) { create(:user) } - - before do - subject.resolve!(user) - end - - it "returns nil" do - expect(subject.resolve!(current_user)).to be_nil - end - - it "doesn't change resolved_at" do - expect(subject.resolved_at).not_to be_nil - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at } - end - - it "doesn't change resolved_by" do - expect(subject.resolved_by).to eq(user) - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by } - end - - it "doesn't change resolved status" do - expect(subject.resolved?).to be true - - expect { subject.resolve!(current_user) }.not_to change { subject.resolved? } - end - end - - context "when not yet resolved" do - it "returns true" do - expect(subject.resolve!(current_user)).to be true - end - - it "sets resolved_at" do - subject.resolve!(current_user) - - expect(subject.resolved_at).not_to be_nil - end - - it "sets resolved_by" do - subject.resolve!(current_user) - - expect(subject.resolved_by).to eq(current_user) - end - - it "marks as resolved" do - subject.resolve!(current_user) - - expect(subject.resolved?).to be true - end - end - end - end - - describe "#unresolve!" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.unresolve!).to be_nil - end - end - - context "when resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(true) - end - - context "when resolved" do - let(:user) { create(:user) } - - before do - subject.resolve!(user) - end - - it "returns true" do - expect(subject.unresolve!).to be true - end - - it "unsets resolved_at" do - subject.unresolve! - - expect(subject.resolved_at).to be_nil - end - - it "unsets resolved_by" do - subject.unresolve! - - expect(subject.resolved_by).to be_nil - end - - it "unmarks as resolved" do - subject.unresolve! - - expect(subject.resolved?).to be false - end - end - - context "when not resolved" do - it "returns nil" do - expect(subject.unresolve!).to be_nil - end - end - end - end - - describe "#discussion" do - context "when not resolvable" do - before do - allow(subject).to receive(:resolvable?).and_return(false) - end - - it "returns nil" do - expect(subject.discussion).to be_nil - end - end - - context "when resolvable" do - let!(:diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: subject.position) } - let!(:diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) } - - let(:active_position2) do - Gitlab::Diff::Position.new( - old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: 16, - new_line: 22, - diff_refs: merge_request.diff_refs - ) - end - - it "returns the discussion this note is in" do - discussion = subject.discussion - - expect(discussion.id).to eq(subject.discussion_id) - expect(discussion.notes).to eq([subject, diff_note2]) - end - end - end - describe "#discussion_id" do let(:note) { create(:diff_note_on_merge_request) } diff --git a/spec/models/discussion_note_spec.rb b/spec/models/discussion_note_spec.rb new file mode 100644 index 00000000000..d61ea05e318 --- /dev/null +++ b/spec/models/discussion_note_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe DiscussionNote, models: true do + # TODO: Test +end diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb index bc32fadd391..f48450589d0 100644 --- a/spec/models/discussion_spec.rb +++ b/spec/models/discussion_spec.rb @@ -3,14 +3,34 @@ require 'spec_helper' describe Discussion, model: true do subject { described_class.new([first_note, second_note, third_note]) } - let(:first_note) { create(:diff_note_on_merge_request) } - let(:second_note) { create(:diff_note_on_merge_request) } - let(:third_note) { create(:diff_note_on_merge_request) } + let(:first_note) { create(:discussion_note_on_merge_request) } + let(:second_note) { create(:discussion_note_on_merge_request) } + let(:third_note) { create(:discussion_note_on_merge_request) } + + describe '.build' do + # TODO: Test + end + + describe '.build_collection' do + # TODO: Test + end + + describe '#id' do + # TODO: Test + end + + describe '#original_id' do + # TODO: Test + end + + describe '#potentially_resolvable?' do + # TODO: Test + end describe "#resolvable?" do - context "when a diff discussion" do + context "when potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(true) + allow(subject).to receive(:discussion_resolvable?).and_return(true) end context "when all notes are unresolvable" do @@ -50,9 +70,9 @@ describe Discussion, model: true do end end - context "when not a diff discussion" do + context "when not potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(false) + allow(subject).to receive(:discussion_resolvable?).and_return(false) end it "returns false" do @@ -530,10 +550,14 @@ describe Discussion, model: true do end end + describe "#last_resolved_note" do + # TODO: Test + end + describe "#collapsed?" do - context "when a diff discussion" do + context "when potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(true) + allow(subject).to receive(:discussion_resolvable?).and_return(true) end context "when resolvable" do @@ -567,31 +591,43 @@ describe Discussion, model: true do allow(subject).to receive(:resolvable?).and_return(false) end - context "when active" do + context "when a diff discussion" do before do - allow(subject).to receive(:active?).and_return(true) + allow(subject).to receive(:diff_discussion?).and_return(true) end - it "returns false" do - expect(subject.collapsed?).to be false + context "when active" do + before do + allow(subject).to receive(:active?).and_return(true) + end + + it "returns false" do + expect(subject.collapsed?).to be false + end end - end - context "when outdated" do - before do - allow(subject).to receive(:active?).and_return(false) + context "when outdated" do + before do + allow(subject).to receive(:active?).and_return(false) + end + + it "returns true" do + expect(subject.collapsed?).to be true + end end + end - it "returns true" do - expect(subject.collapsed?).to be true + context "when not a diff discussion" do + it "returns false" do + expect(subject.collapsed?).to be false end end end end - context "when not a diff discussion" do + context "when not potentially resolvable" do before do - allow(subject).to receive(:diff_discussion?).and_return(false) + allow(subject).to receive(:discussion_resolvable?).and_return(false) end it "returns false" do @@ -599,23 +635,4 @@ describe Discussion, model: true do end end end - - describe "#truncated_diff_lines" do - let(:truncated_lines) { subject.truncated_diff_lines } - - context "when diff is greater than allowed number of truncated diff lines " do - it "returns fewer lines" do - expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES - - expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES - end - end - - context "when some diff lines are meta" do - it "returns no meta lines" do - expect(subject.diff_lines).to include(be_meta) - expect(truncated_lines).not_to include(be_meta) - end - end - end end diff --git a/spec/models/individual_note_discussion_spec.rb b/spec/models/individual_note_discussion_spec.rb new file mode 100644 index 00000000000..33de682aedc --- /dev/null +++ b/spec/models/individual_note_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe IndividualNoteDiscussion, models: true do + # TODO: Test +end diff --git a/spec/models/legacy_diff_discussion_spec.rb b/spec/models/legacy_diff_discussion_spec.rb new file mode 100644 index 00000000000..72e33877b40 --- /dev/null +++ b/spec/models/legacy_diff_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe LegacyDiffDiscussion, models: true do + # TODO: Test +end diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb deleted file mode 100644 index 81517a18b74..00000000000 --- a/spec/models/legacy_diff_note_spec.rb +++ /dev/null @@ -1,101 +0,0 @@ -require 'spec_helper' - -describe LegacyDiffNote, models: true do - describe "Commit diff line notes" do - let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") } - let!(:commit) { note.noteable } - - it "saves a valid note" do - expect(note.commit_id).to eq(commit.id) - expect(note.noteable.id).to eq(commit.id) - end - - it "is recognized by #legacy_diff_note?" do - expect(note).to be_legacy_diff_note - end - end - - describe '#active?' do - it 'is always true when the note has no associated diff line' do - note = build(:legacy_diff_note_on_merge_request) - - expect(note).to receive(:diff_line).and_return(nil) - - expect(note).to be_active - end - - it 'is never true when the note has no noteable associated' do - note = build(:legacy_diff_note_on_merge_request) - - expect(note).to receive(:diff_line).and_return(double) - expect(note).to receive(:noteable).and_return(nil) - - expect(note).not_to be_active - end - - it 'returns the memoized value if defined' do - note = build(:legacy_diff_note_on_merge_request) - - note.instance_variable_set(:@active, 'foo') - expect(note).not_to receive(:find_noteable_diff) - - expect(note.active?).to eq 'foo' - end - - context 'for a merge request noteable' do - it 'is false when noteable has no matching diff' do - merge = build_stubbed(:merge_request, :simple) - note = build(:legacy_diff_note_on_merge_request, noteable: merge) - - allow(note).to receive(:diff_line).and_return(double) - expect(note).to receive(:find_noteable_diff).and_return(nil) - - expect(note).not_to be_active - end - - it 'is true when noteable has a matching diff' do - merge = create(:merge_request, :simple) - - # Generate a real line_code value so we know it will match. We use a - # random line from a random diff just for funsies. - diff = merge.raw_diffs.to_a.sample - line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample - code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos) - - # We're persisting in order to trigger the set_diff callback - note = create(:legacy_diff_note_on_merge_request, noteable: merge, - line_code: code, - project: merge.source_project) - - # Make sure we don't get a false positive from a guard clause - expect(note).to receive(:find_noteable_diff).and_call_original - expect(note).to be_active - end - end - end - - describe "#discussion_id" do - let(:note) { create(:note) } - - context "when it is newly created" do - it "has a discussion id" do - expect(note.discussion_id).not_to be_nil - expect(note.discussion_id).to match(/\A\h{40}\z/) - end - end - - context "when it didn't store a discussion id before" do - before do - note.update_column(:discussion_id, nil) - end - - it "has a discussion id" do - # The discussion_id is set in `after_initialize`, so `reload` won't work - reloaded_note = Note.find(note.id) - - expect(reloaded_note.discussion_id).not_to be_nil - expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) - end - end - end -end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 24e7c1b17d9..faa89ff0507 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1225,12 +1225,12 @@ describe MergeRequest, models: true do end context "discussion status" do - let(:first_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } - let(:second_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } - let(:third_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) } + let(:first_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } + let(:second_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } + let(:third_discussion) { Discussion.new([create(:discussion_note_on_merge_request)]) } before do - allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion]) + allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion]) end describe '#resolvable_discussions' do @@ -1245,34 +1245,6 @@ describe MergeRequest, models: true do end end - describe '#discussions_can_be_resolved_by? user' do - let(:user) { build(:user) } - - context 'all discussions can be resolved by the user' do - before do - allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true) - end - - it 'allows a user to resolve the discussions' do - expect(subject.discussions_can_be_resolved_by?(user)).to be(true) - end - end - - context 'one discussion cannot be resolved by the user' do - before do - allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) - allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false) - end - - it 'allows a user to resolve the discussions' do - expect(subject.discussions_can_be_resolved_by?(user)).to be(false) - end - end - end - describe "#discussions_resolvable?" do context "when all discussions are unresolvable" do before do @@ -1398,6 +1370,38 @@ describe MergeRequest, models: true do end end end + + describe "#discussions_to_be_resolved" do + # TODO: Test + end + + describe '#discussions_can_be_resolved_by?' do + let(:user) { build(:user) } + + context 'all discussions can be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(true) + end + end + + context 'one discussion cannot be resolved by the user' do + before do + allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true) + allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false) + end + + it 'allows a user to resolve the discussions' do + expect(subject.discussions_can_be_resolved_by?(user)).to be(false) + end + end + end end describe '#conflicts_can_be_resolved_in_ui?' do diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 33536487c41..c5e4a639f06 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -245,6 +245,18 @@ describe Note, models: true do end end + describe '.discussions' do + # TODO: Test + end + + describe '.find_original_discussion' do + # TODO: Test + end + + describe '.find_discussion' do + # TODO: Test + end + describe ".grouped_diff_discussions" do let!(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } @@ -297,31 +309,6 @@ describe Note, models: true do end end - describe "#discussion_id" do - let(:note) { create(:note) } - - context "when it is newly created" do - it "has a discussion id" do - expect(note.discussion_id).not_to be_nil - expect(note.discussion_id).to match(/\A\h{40}\z/) - end - end - - context "when it didn't store a discussion id before" do - before do - note.update_column(:discussion_id, nil) - end - - it "has a discussion id" do - # The discussion_id is set in `after_initialize`, so `reload` won't work - reloaded_note = Note.find(note.id) - - expect(reloaded_note.discussion_id).not_to be_nil - expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) - end - end - end - describe '#for_personal_snippet?' do it 'returns false for a project snippet note' do expect(build(:note_on_project_snippet).for_personal_snippet?).to be_falsy @@ -388,6 +375,86 @@ describe Note, models: true do end end + describe '#can_be_discussion_note?' do + # TODO: Test + end + + describe '#discussion_class' do + # TODO: Test + end + + describe "#discussion_id" do + let(:note) { create(:note) } + + context "when it is newly created" do + it "has a discussion id" do + expect(note.discussion_id).not_to be_nil + expect(note.discussion_id).to match(/\A\h{40}\z/) + end + end + + context "when it didn't store a discussion id before" do + before do + note.update_column(:discussion_id, nil) + end + + it "has a discussion id" do + # The discussion_id is set in `after_initialize`, so `reload` won't work + reloaded_note = Note.find(note.id) + + expect(reloaded_note.discussion_id).not_to be_nil + expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/) + end + end + end + + describe "#original_discussion_id" do + # TODO: Test + end + + describe '#to_discussion' do + subject { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:discussion_note_on_merge_request, project: subject.project, noteable: subject.noteable, in_reply_to_discussion_id: subject.discussion_id) } + + it "returns a discussion with just this note" do + discussion = subject.to_discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([subject]) + end + end + + describe "#discussion" do + let!(:note1) { create(:discussion_note_on_merge_request) } + let!(:note2) { create(:diff_note_on_merge_request, project: note1.project, noteable: note1.noteable) } + + context 'when the note is part of a discussion' do + subject { create(:discussion_note_on_merge_request, project: note1.project, noteable: note1.noteable, in_reply_to_discussion_id: note1.discussion_id) } + + it "returns the discussion this note is in" do + discussion = subject.discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([note1, subject]) + end + end + + context 'when the note is not part of a discussion' do + subject { create(:note) } + + it "returns a discussion with just this note" do + discussion = subject.discussion + + expect(discussion.id).to eq(subject.discussion_id) + expect(discussion.notes).to eq([subject]) + end + end + end + + describe "#part_of_discussion?" do + # TODO: Test + end + describe 'expiring ETag cache' do let(:note) { build(:note_on_issue) } diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb new file mode 100644 index 00000000000..2fc6cce471f --- /dev/null +++ b/spec/models/sent_notification_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe SentNotification, model: true do + # TODO: Test +end diff --git a/spec/models/simple_discussion_spec.rb b/spec/models/simple_discussion_spec.rb new file mode 100644 index 00000000000..1cb149aa270 --- /dev/null +++ b/spec/models/simple_discussion_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe SimpleDiscussion, model: true do + # TODO: Test +end diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index b1b398a897e..91d9057075f 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -824,7 +824,7 @@ describe API::V3::Issues, api: true do end context 'resolving issues in a merge request' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } before do diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb index 12c3cdf28c6..ab8df7b74cd 100644 --- a/spec/services/discussions/resolve_service_spec.rb +++ b/spec/services/discussions/resolve_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Discussions::ResolveService do describe '#execute' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:project) { merge_request.project } let(:merge_request) { discussion.noteable } let(:user) { create(:user) } @@ -41,7 +41,7 @@ describe Discussions::ResolveService do end it 'can resolve multiple discussions at once' do - other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first + other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion service.execute([discussion, other_discussion]) diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb index 17990f41b3b..d166b9783ad 100644 --- a/spec/services/issues/build_service_spec.rb +++ b/spec/services/issues/build_service_spec.rb @@ -96,13 +96,13 @@ describe Issues::BuildService, services: true do end it 'mentions all the authors in the description' do - authors = merge_request.diff_discussions.map(&:author) + authors = merge_request.resolvable_discussions.map(&:author) expect(issue.description).to include(*authors.map(&:to_reference)) end it 'has a link for each unresolved discussion in the description' do - notes = merge_request.diff_discussions.map(&:first_note) + notes = merge_request.resolvable_discussions.map(&:first_note) links = notes.map { |note| Gitlab::UrlBuilder.build(note) } expect(issue.description).to include(*links) diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 776cbc4296b..80bfb731550 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -141,7 +141,7 @@ describe Issues::CreateService, services: true do it_behaves_like 'new issuable record that supports slash commands' context 'resolving discussions' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb new file mode 100644 index 00000000000..065f6eeeacb --- /dev/null +++ b/spec/services/notes/build_service_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe Notes::CreateService, services: true do + # TODO: Test +end diff --git a/spec/services/notes/create_service_spec.rb b/spec/services/notes/create_service_spec.rb index 152c6d20daa..b406afeb34d 100644 --- a/spec/services/notes/create_service_spec.rb +++ b/spec/services/notes/create_service_spec.rb @@ -13,6 +13,9 @@ describe Notes::CreateService, services: true do project.team << [user, :master] end + # TODO: Test in_reply_to_discussion_id + # TODO: Test new_discussion + context "valid params" do it 'returns a valid note' do note = described_class.new(project, user, opts).execute diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e3146a56495..c4e00fcf080 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -439,7 +439,7 @@ describe NotificationService, services: true do notification.new_note(note) - expect(SentNotification.last.position).to eq(note.position) + expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.original_discussion_id) end end end diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 5ec1ed8237b..42d63a9f9ba 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -796,7 +796,7 @@ describe SystemNoteService, services: true do end describe '.discussion_continued_in_issue' do - let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first } + let(:discussion) { create(:diff_note_on_merge_request).to_discussion } let(:merge_request) { discussion.noteable } let(:project) { merge_request.source_project } let(:issue) { create(:issue, project: project) } |