diff options
Diffstat (limited to 'app')
88 files changed, 1377 insertions, 852 deletions
diff --git a/app/assets/javascripts/comment_type_toggle.js b/app/assets/javascripts/comment_type_toggle.js new file mode 100644 index 00000000000..df0ba86198c --- /dev/null +++ b/app/assets/javascripts/comment_type_toggle.js @@ -0,0 +1,60 @@ +import DropLab from './droplab/drop_lab'; +import InputSetter from './droplab/plugins/input_setter'; + +class CommentTypeToggle { + constructor(opts = {}) { + this.dropdownTrigger = opts.dropdownTrigger; + this.dropdownList = opts.dropdownList; + this.noteTypeInput = opts.noteTypeInput; + this.submitButton = opts.submitButton; + this.closeButton = opts.closeButton; + this.reopenButton = opts.reopenButton; + } + + initDroplab() { + this.droplab = new DropLab(); + + const config = this.setConfig(); + + this.droplab.init(this.dropdownTrigger, this.dropdownList, [InputSetter], config); + } + + setConfig() { + const config = { + InputSetter: [{ + input: this.noteTypeInput, + valueAttribute: 'data-value', + }, + { + input: this.submitButton, + valueAttribute: 'data-submit-text', + }], + }; + + if (this.closeButton) { + config.InputSetter.push({ + input: this.closeButton, + valueAttribute: 'data-close-text', + }, { + input: this.closeButton, + valueAttribute: 'data-close-text', + inputAttribute: 'data-alternative-text', + }); + } + + if (this.reopenButton) { + config.InputSetter.push({ + input: this.reopenButton, + valueAttribute: 'data-reopen-text', + }, { + input: this.reopenButton, + valueAttribute: 'data-reopen-text', + inputAttribute: 'data-alternative-text', + }); + } + + return config; + } +} + +export default CommentTypeToggle; diff --git a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js index fc2f20e3bcb..eb76b7d15fd 100644 --- a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js +++ b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js @@ -42,10 +42,14 @@ import Vue from 'vue'; } }, created() { - this.discussion = CommentsStore.state[this.discussionId]; + if (this.discussionId) { + this.discussion = CommentsStore.state[this.discussionId]; + } }, mounted: function () { - const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`); + if (!this.discussionId) return; + + const $textarea = $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`); this.textareaIsEmpty = $textarea.val() === ''; $textarea.on('input.comment-and-resolve-btn', () => { @@ -53,7 +57,9 @@ import Vue from 'vue'; }); }, destroyed: function () { - $(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn'); + if (!this.discussionId) return; + + $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`).off('input.comment-and-resolve-btn'); } }); diff --git a/app/assets/javascripts/droplab/drop_down.js b/app/assets/javascripts/droplab/drop_down.js index f686ad33f6f..9588921ebcd 100644 --- a/app/assets/javascripts/droplab/drop_down.js +++ b/app/assets/javascripts/droplab/drop_down.js @@ -35,6 +35,8 @@ Object.assign(DropDown.prototype, { }, clickEvent: function(e) { + if (e.target.tagName === 'UL') return; + var selected = utils.closest(e.target, 'LI'); if (!selected) return; diff --git a/app/assets/javascripts/droplab/plugins/input_setter.js b/app/assets/javascripts/droplab/plugins/input_setter.js index c292cfa7b8f..d01fbc5830d 100644 --- a/app/assets/javascripts/droplab/plugins/input_setter.js +++ b/app/assets/javascripts/droplab/plugins/input_setter.js @@ -35,8 +35,6 @@ const InputSetter = { const newValue = selectedItem.getAttribute(config.valueAttribute); const inputAttribute = config.inputAttribute; - if (!newValue) return; - if (input.hasAttribute(inputAttribute)) return input.setAttribute(inputAttribute, newValue); if (input.tagName === 'INPUT') return input.value = newValue; return input.textContent = newValue; diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index 3f041172ff3..59d6508fc02 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -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 }); }; @@ -121,7 +131,7 @@ window.FilesCommentButton = (function() { }; FilesCommentButton.prototype.validateLineContent = function(lineContentElement) { - return lineContentElement.attr('data-discussion-id') && lineContentElement.attr('data-discussion-id') !== ''; + return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== ''; }; return FilesCommentButton; diff --git a/app/assets/javascripts/gl_form.js b/app/assets/javascripts/gl_form.js index e7c98e16581..ff10f19a4fe 100644 --- a/app/assets/javascripts/gl_form.js +++ b/app/assets/javascripts/gl_form.js @@ -29,7 +29,8 @@ GLForm.prototype.setupForm = function() { this.form.find('.div-dropzone').remove(); this.form.addClass('gfm-form'); // remove notify commit author checkbox for non-commit notes - gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button')); + gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button, .js-note-new-discussion')); + gl.GfmAutoComplete.setup(this.form.find('.js-gfm-input')); new DropzoneInput(this.form); autosize(this.textarea); diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 177cf66b37d..3f92d4ee6cf 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -279,7 +279,7 @@ $(function () { // Disable form buttons while a form is submitting $body.on('ajax:complete, ajax:beforeSend, submit', 'form', function (e) { var buttons; - buttons = $('[type="submit"]', this); + buttons = $('[type="submit"], .js-disable-on-submit', this); switch (e.type) { case 'ajax:beforeSend': case 'submit': diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 1d563c63f39..15f7a813626 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -5,6 +5,7 @@ /* global mrRefreshWidgetUrl */ import Cookies from 'js-cookie'; +import CommentTypeToggle from './comment_type_toggle'; require('./autosave'); window.autosize = require('vendor/autosize'); @@ -110,7 +111,6 @@ require('./task_list'); $(document).on("visibilitychange", this.visibilityChange); // when issue status changes, we need to refresh data $(document).on("issuable:change", this.refresh); - // when a key is clicked on the notes return $(document).on("keydown", ".js-note-text", this.keydownNoteText); }; @@ -137,6 +137,26 @@ require('./task_list'); $(document).off("click", '.system-note-commit-list-toggler'); }; + Notes.initCommentTypeToggle = function (form) { + const dropdownTrigger = form.querySelector('.js-comment-type-dropdown .dropdown-toggle'); + const dropdownList = form.querySelector('.js-comment-type-dropdown .dropdown-menu'); + const noteTypeInput = form.querySelector('#note_type'); + const submitButton = form.querySelector('.js-comment-type-dropdown .js-comment-submit-button'); + const closeButton = form.querySelector('.js-note-target-close'); + const reopenButton = form.querySelector('.js-note-target-reopen'); + + const commentTypeToggle = new CommentTypeToggle({ + dropdownTrigger, + dropdownList, + noteTypeInput, + submitButton, + closeButton, + reopenButton, + }); + + commentTypeToggle.initDroplab(); + }; + Notes.prototype.keydownNoteText = function(e) { var $textarea, discussionNoteForm, editNote, myLastNote, myLastNoteEditBtn, newText, originalText; if (gl.utils.isMetaKey(e)) { @@ -192,7 +212,7 @@ require('./task_list'); }; Notes.prototype.refresh = function() { - if (!document.hidden && document.URL.indexOf(this.noteable_url) === 0) { + if (!document.hidden) { return this.getContent(); } }; @@ -213,11 +233,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) @@ -276,8 +292,12 @@ require('./task_list'); Note: for rendering inline notes use renderDiscussionNote */ - Notes.prototype.renderNote = function(note) { + Notes.prototype.renderNote = function(note, $form) { var $notesList; + if (note.discussion_html != null) { + return this.renderDiscussionNote(note, $form); + } + if (!note.valid) { if (note.errors.commands_only) { new Flash(note.errors.commands_only, 'notice', this.parentTimeline); @@ -317,61 +337,50 @@ require('./task_list'); Note: for rendering inline notes use renderDiscussionNote */ - Notes.prototype.renderDiscussionNote = function(note) { - var discussionContainer, form, note_html, row, lineType, diffAvatarContainer; + Notes.prototype.renderDiscussionNote = function(note, $form) { + var discussionContainer, form, row, lineType, diffAvatarContainer; if (!this.isNewNote(note)) { 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 = $form || $(".js-discussion-note-form[data-discussion-id='" + note.discussion_id + "']"); row = form.closest("tr"); lineType = this.isParallelView() ? form.find('#line_type').val() : 'old'; diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line'); - note_html = $(note.html); - 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) { + discussionContainer = form.closest('.discussion').find('.notes'); } if (discussionContainer.length === 0) { - if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { - // insert the note and the reply button after the temp row - row.after(note.diff_discussion_html); + if (note.diff_discussion_html) { + var $discussion = $(note.diff_discussion_html).renderGFM(); - // remove the note (will be added again below) - row.next().find(".note").remove(); - } else { - // Merge new discussion HTML in - var $discussion = $(note.diff_discussion_html); - var $notes = $discussion.find('.notes[data-discussion-id="' + note.discussion_id + '"]'); - var contentContainerClass = '.' + $notes.closest('.notes_content') - .attr('class') - .split(' ') - .join('.'); - - // remove the note (will be added again below) - $notes.find('.note').remove(); - - row.find(contentContainerClass + ' .content').append($notes.closest('.content').children()); + if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { + // insert the note and the reply button after the temp row + row.after($discussion); + } else { + // Merge new discussion HTML in + var $notes = $discussion.find('.notes[data-discussion-id="' + note.discussion_id + '"]'); + var contentContainerClass = '.' + $notes.closest('.notes_content') + .attr('class') + .split(' ') + .join('.'); + + row.find(contentContainerClass + ' .content').append($notes.closest('.content').children()); + } } - // Before that, the container didn't exist - discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']"); - // 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) { - $('ul.main-notes-list').append(note.discussion_html).renderGFM(); + if ($('body').attr('data-page').indexOf('projects:merge_request') === 0 || !note.diff_discussion_html) { + $('ul.main-notes-list').append($(note.discussion_html).renderGFM()); } } else { // append new note to all matching discussions - discussionContainer.append(note_html); + discussionContainer.append($(note.html).renderGFM()); } - if (typeof gl.diffNotesCompileComponents !== 'undefined' && note.discussion_id) { + if (typeof gl.diffNotesCompileComponents !== 'undefined' && note.discussion_resolvable) { gl.diffNotesCompileComponents(); this.renderDiscussionAvatar(diffAvatarContainer, note); } @@ -455,9 +464,14 @@ require('./task_list'); form.addClass("js-main-target-form"); form.find("#note_line_code").remove(); form.find("#note_position").remove(); - form.find("#note_type").remove(); + form.find("#note_type").val(''); + form.find("#in_reply_to_discussion_id").remove(); form.find('.js-comment-resolve-button').closest('comment-and-resolve-btn').remove(); - return this.parentTimeline = form.parents('.timeline'); + this.parentTimeline = form.parents('.timeline'); + + if (form.length) { + Notes.initCommentTypeToggle(form.get(0)); + } }; /* @@ -470,10 +484,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 +538,7 @@ require('./task_list'); } } - this.renderDiscussionNote(note); + this.renderNote(note, $form); // cleanup after successfully creating a diff/discussion note this.removeDiscussionNoteForm($form); }; @@ -656,7 +684,7 @@ require('./task_list'); return function(i, el) { var note, notes; note = $(el); - notes = note.closest(".notes"); + notes = note.closest(".discussion-notes"); if (typeof gl.diffNotesCompileComponents !== 'undefined') { if (gl.diffNoteApps[noteElId]) { @@ -673,14 +701,13 @@ require('./task_list'); // "Discussions" tab notes.closest(".timeline-entry").remove(); - if (!_this.isParallelView() || notesTr.find('.note').length === 0) { - // "Changes" tab / commit view - notesTr.remove(); + // The notes tr can contain multiple lists of notes, like on the parallel diff + if (notesTr.find('.discussion-notes').length > 1) { + notes.remove(); } else { - notes.closest('.content').empty(); + notesTr.remove(); } } - return note.remove(); }; })(this)); // Decrement the "Discussions" counter only once @@ -711,7 +738,7 @@ require('./task_list'); Notes.prototype.replyToDiscussionNote = function(e) { var form, replyLink; - form = this.formClone.clone(); + form = this.cleanForm(this.formClone.clone()); replyLink = $(e.target).closest(".js-discussion-reply-button"); // insert the form after the button replyLink @@ -727,29 +754,44 @@ 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"); + + if (discussionID) { + form.attr("data-discussion-id", discussionID); + form.find("#in_reply_to_discussion_id").val(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("#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); + form + .removeClass('js-main-target-form') + .addClass("discussion-form js-discussion-note-form"); + if (typeof gl.diffNotesCompileComponents !== 'undefined') { var $commentBtn = form.find('comment-and-resolve-btn'); - $commentBtn - .attr(':discussion-id', "'" + dataHolder.data('discussionId') + "'"); + $commentBtn.attr(':discussion-id', `'${discussionID}'`); gl.diffNotesCompileComponents(); } @@ -757,10 +799,7 @@ require('./task_list'); form.find(".js-note-text").focus(); form .find('.js-comment-resolve-button') - .attr('data-discussion-id', dataHolder.data('discussionId')); - form - .removeClass('js-main-target-form') - .addClass("discussion-form js-discussion-note-form"); + .attr('data-discussion-id', discussionID); }; /* @@ -823,7 +862,7 @@ require('./task_list'); } if (addForm) { - newForm = this.formClone.clone(); + newForm = this.cleanForm(this.formClone.clone()); newForm.appendTo(notesContent); // show the form return this.setupDiscussionNoteForm($link, newForm); @@ -900,9 +939,10 @@ require('./task_list'); reopenbtn = form.find('.js-note-target-reopen'); closebtn = form.find('.js-note-target-close'); discardbtn = form.find('.js-note-discard'); + if (textarea.val().trim().length > 0) { - reopentext = reopenbtn.data('alternative-text'); - closetext = closebtn.data('alternative-text'); + reopentext = reopenbtn.attr('data-alternative-text'); + closetext = closebtn.attr('data-alternative-text'); if (reopenbtn.text() !== reopentext) { reopenbtn.text(reopentext); } @@ -1009,6 +1049,20 @@ require('./task_list'); }); }; + Notes.prototype.cleanForm = function($form) { + // Remove JS classes that are not needed here + $form + .find('.js-comment-type-dropdown') + .removeClass('btn-group'); + + // Remove dropdown + $form + .find('.dropdown-menu') + .remove(); + + return $form; + }; + return Notes; })(); }).call(window); diff --git a/app/assets/javascripts/render_gfm.js b/app/assets/javascripts/render_gfm.js index ea91aaa10a6..2c3a9cacd38 100644 --- a/app/assets/javascripts/render_gfm.js +++ b/app/assets/javascripts/render_gfm.js @@ -8,6 +8,7 @@ $.fn.renderGFM = function() { this.find('.js-syntax-highlight').syntaxHighlight(); this.find('.js-render-math').renderMath(); + return this; }; $(document).on('ready load', function() { diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 927bf9805ce..b637994adf8 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -310,3 +310,94 @@ margin-bottom: 10px; } } + +.comment-type-dropdown { + .comment-btn { + width: auto; + } + + .dropdown-toggle { + float: right; + + .toggle-icon { + color: $white-light; + padding-right: 2px; + margin-top: 2px; + pointer-events: none; + } + } + + .dropdown-menu { + top: initial; + bottom: 40px; + width: 298px; + } + + .description { + display: inline-block; + white-space: normal; + margin-left: 8px; + padding-right: 33px; + } + + li { + padding-top: 6px; + + & > a { + margin: 0; + padding: 0; + color: inherit; + border-radius: 0; + text-overflow: inherit; + + &:hover, + &:focus { + background-color: inherit; + color: inherit; + } + } + + &:hover, + &:focus { + background-color: $dropdown-hover-color; + color: $white-light; + } + + &.droplab-item-selected i { + visibility: visible; + } + + i { + visibility: hidden; + } + } + + i { + display: inline-block; + vertical-align: top; + padding-top: 2px; + } + + .divider { + margin: 0 8px; + padding: 0; + border-top: $gray-darkest; + } + + @media (max-width: $screen-xs-max) { + display: flex; + width: 100%; + + .comment-btn { + flex-grow: 1; + flex-shrink: 0; + width: auto; + } + + .dropdown-toggle { + flex-grow: 0; + flex-shrink: 1; + width: auto; + } + } +} diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index f007e65940e..94ea4c5c8c6 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -294,6 +294,18 @@ ul.notes { border-width: 1px; } + .discussion-notes { + &:not(:first-child) { + border-top: 1px solid $white-normal; + margin-top: 20px; + } + + &:not(:last-child) { + border-bottom: 1px solid $white-normal; + margin-bottom: 20px; + } + } + .notes { background-color: $white-light; } diff --git a/app/controllers/concerns/renders_notes.rb b/app/controllers/concerns/renders_notes.rb new file mode 100644 index 00000000000..dd21066ac13 --- /dev/null +++ b/app/controllers/concerns/renders_notes.rb @@ -0,0 +1,20 @@ +module RendersNotes + def prepare_notes_for_rendering(notes) + preload_noteable_for_regular_notes(notes) + preload_max_access_for_authors(notes, @project) + Banzai::NoteRenderer.render(notes, @project, current_user) + + notes + end + + private + + def preload_max_access_for_authors(notes, project) + user_ids = notes.map(&:author_id) + project.team.max_member_access_for_user_ids(user_ids) + end + + def preload_noteable_for_regular_notes(notes) + ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable) + end +end diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb index f453822bed6..d25bbddd1bb 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 RendersNotes include CreatesCommit include DiffForPath include DiffHelper @@ -113,22 +114,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.flatten + @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 a50e16fa4ff..cbf67137261 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -1,5 +1,5 @@ class Projects::IssuesController < Projects::ApplicationController - include NotesHelper + include RendersNotes include ToggleSubscriptionAction include IssuableActions include ToggleAwardEmoji @@ -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 c107b3ffa88..5c1f7e69ee8 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -3,7 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController include DiffForPath include DiffHelper include IssuableActions - include NotesHelper + include RendersNotes include ToggleAwardEmoji include IssuableCollections @@ -574,20 +574,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 @@ -600,22 +587,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.flatten.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..405ea3c0a4f 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -1,4 +1,5 @@ class Projects::NotesController < Projects::ApplicationController + include RendersNotes include ToggleAwardEmoji # Authorize @@ -6,13 +7,15 @@ 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_relations_for_view + @notes = prepare_notes_for_rendering(@notes) + @notes.each do |note| next if note.cross_reference_not_visible_for?(current_user) @@ -23,7 +26,10 @@ 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] + ) @note = Notes::CreateService.new(project, current_user, create_params).execute if @note.is_a?(Note) @@ -111,6 +117,17 @@ class Projects::NotesController < Projects::ApplicationController ) end + def discussion_html(discussion) + return if discussion.individual_note? + + render_to_string( + "discussions/_discussion", + layout: false, + formats: [:html], + locals: { discussion: discussion } + ) + end + def diff_discussion_html(discussion) return unless discussion.diff_discussion? @@ -118,13 +135,13 @@ class Projects::NotesController < Projects::ApplicationController template = "discussions/_parallel_diff_discussion" locals = if params[:line_type] == 'old' - { discussion_left: discussion, discussion_right: nil } + { discussions_left: [discussion], discussions_right: nil } else - { discussion_left: nil, discussion_right: discussion } + { discussions_left: nil, discussions_right: [discussion] } end else template = "discussions/_diff_discussion" - locals = { discussion: discussion } + locals = { discussions: [discussion] } end render_to_string( @@ -135,54 +152,28 @@ 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 + commands_changes: note.commands_changes } if note.persisted? - Banzai::NoteRenderer.render([note], @project, current_user) - attrs.merge!( valid: true, - discussion_id: note.discussion_id, + id: note.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.individual_note? attrs.merge!( + discussion_resolvable: discussion.resolvable?, + diff_discussion_html: diff_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 end else attrs.merge!( @@ -191,7 +182,6 @@ class Projects::NotesController < Projects::ApplicationController ) end - attrs[:commands_changes] = note.commands_changes attrs end @@ -205,14 +195,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..5c9e0d4d1a1 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 RendersNotes 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..3c499184b41 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 + notes = init_collection + notes = since_fetch_at(notes) + notes.fresh 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(notes) } 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 @@ -87,17 +98,21 @@ class NotesFinder # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. # - def search(query, notes_relation = @notes) + def search(notes) + query = @params[:search] + return notes unless query + pattern = "%#{query}%" - notes_relation.where(Note.arel_table[:note].matches(pattern)) + notes.where(Note.arel_table[:note].matches(pattern)) end # Notes changed since last fetch # Uses overlapping intervals to avoid worrying about race conditions - def since_fetch_at(fetch_time) + def since_fetch_at(notes) + return notes unless @params[:last_fetched_at] + # Default to 0 to remain compatible with old clients last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i) - - @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh + notes.updated_after(last_fetched_at - FETCH_OVERLAP) end end diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb index aed1d7c839f..5e0886cc599 100644 --- a/app/helpers/diff_helper.rb +++ b/app/helpers/diff_helper.rb @@ -62,19 +62,19 @@ module DiffHelper end def parallel_diff_discussions(left, right, diff_file) - discussion_left = discussion_right = nil + discussions_left = discussions_right = nil if left && (left.unchanged? || left.removed?) line_code = diff_file.line_code(left) - discussion_left = @grouped_diff_discussions[line_code] + discussions_left = @grouped_diff_discussions[line_code] end if right && right.added? line_code = diff_file.line_code(right) - discussion_right = @grouped_diff_discussions[line_code] + discussions_right = @grouped_diff_discussions[line_code] end - [discussion_left, discussion_right] + [discussions_left, discussions_right] end def inline_diff_btn diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index b0331f36a2f..5f3d89cf6cb 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -24,57 +24,24 @@ 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) return if @diff_notes_disabled - use_legacy_diff_note = @use_legacy_diff_notes - # If the controller doesn't force the use of legacy diff notes, we - # determine this on a line-by-line basis by seeing if there already exist - # active legacy diff notes at this line, in which case newly created notes - # will use the legacy technology as well. - # We do this because the discussion_id values of legacy and "new" diff - # notes, which are used to group notes on the merge request discussion tab, - # are incompatible. - # If we didn't, diff notes that would show for the same line on the changes - # tab, would show in different discussions on the discussion tab. - use_legacy_diff_note ||= begin - discussion = @grouped_diff_discussions[line_code] - discussion && discussion.legacy_diff_discussion? - end - data = { line_code: line_code, line_type: line_type, } - 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 - ) + if @use_legacy_diff_notes + data[:note_type] = LegacyDiffNote.name 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 - ) + data[:note_type] = DiffNote.name + data[:position] = position.to_json end data @@ -83,21 +50,12 @@ module NotesHelper 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, line_type: line_type } button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button', data: data, title: 'Add a reply' end - def preload_max_access_for_authors(notes, project) - user_ids = notes.map(&:author_id) - project.team.max_member_access_for_user_ids(user_ids) - end - - def preload_noteable_for_regular_notes(notes) - ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable) - end - def note_max_access_for_user(note) note.project.team.human_max_access(note.author_id) end diff --git a/app/mailers/emails/notes.rb b/app/mailers/emails/notes.rb index 46fa6fd9f6d..00707a0023e 100644 --- a/app/mailers/emails/notes.rb +++ b/app/mailers/emails/notes.rb @@ -4,13 +4,8 @@ module Emails setup_note_mail(note_id, recipient_id) @commit = @note.noteable - @discussion = @note.to_discussion if @note.diff_note? @target_url = namespace_project_commit_url(*note_target_url_options) - - mail_answer_thread(@commit, - from: sender(@note.author_id), - to: recipient(recipient_id), - subject: subject("#{@commit.title} (#{@commit.short_id})")) + mail_answer_thread(@commit, note_thread_options(recipient_id)) end def note_issue_email(recipient_id, note_id) @@ -25,7 +20,6 @@ module Emails setup_note_mail(note_id, recipient_id) @merge_request = @note.noteable - @discussion = @note.to_discussion if @note.diff_note? @target_url = namespace_project_merge_request_url(*note_target_url_options) mail_answer_thread(@merge_request, note_thread_options(recipient_id)) end @@ -56,15 +50,18 @@ module Emails { from: sender(@note.author_id), to: recipient(recipient_id), - subject: subject("#{@note.noteable.title} (#{@note.noteable.to_reference})") + subject: subject("#{@note.noteable.title} (#{@note.noteable.reference_link_text})") } end def setup_note_mail(note_id, recipient_id) - @note = Note.find(note_id) + # `note_id` is a `Note` when originating in `NotifyPreview` + @note = note_id.is_a?(Note) ? note_id : Note.find(note_id) @project = @note.project - @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key) + if @project && @note.persisted? + @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key) + end end end end diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 14df6f8f0a3..f315e38bcaa 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -111,7 +111,7 @@ class Notify < BaseMailer headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key - if Gitlab::IncomingEmail.enabled? + if Gitlab::IncomingEmail.enabled? && @sent_notification address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace @@ -176,6 +176,6 @@ class Notify < BaseMailer end headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',') - @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) + @unsubscribe_url = unsubscribe_sent_notification_url(@sent_notification) end end 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/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb new file mode 100644 index 00000000000..87db0c810c3 --- /dev/null +++ b/app/models/concerns/discussion_on_diff.rb @@ -0,0 +1,57 @@ +# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`. +module DiscussionOnDiff + extend ActiveSupport::Concern + + included do + NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + + memoized_values << :active + + delegate :line_code, + :original_line_code, + :diff_file, + :diff_line, + :for_line?, + :active?, + + to: :first_note + + delegate :file_path, + :blob, + :highlighted_diff_lines, + :diff_lines, + + to: :diff_file, + allow_nil: true + end + + def diff_discussion? + true + end + + def active? + return @active if @active.present? + + @active = first_note.active? + 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/concerns/ignorable_column.rb b/app/models/concerns/ignorable_column.rb new file mode 100644 index 00000000000..eb9f3423e48 --- /dev/null +++ b/app/models/concerns/ignorable_column.rb @@ -0,0 +1,28 @@ +# Module that can be included into a model to make it easier to ignore database +# columns. +# +# Example: +# +# class User < ActiveRecord::Base +# include IgnorableColumn +# +# ignore_column :updated_at +# end +# +module IgnorableColumn + extend ActiveSupport::Concern + + module ClassMethods + def columns + super.reject { |column| ignored_columns.include?(column.name) } + end + + def ignored_columns + @ignored_columns ||= Set.new + end + + def ignore_column(name) + ignored_columns << name.to_s + end + end +end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index b4dded7e27e..3d2258d5e3e 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -292,17 +292,6 @@ module Issuable self.class.to_ability_name end - # Convert this Issuable class name to a format usable by notifications. - # - # Examples: - # - # issuable.class # => MergeRequest - # issuable.human_class_name # => "merge request" - - def human_class_name - @human_class_name ||= self.class.name.titleize.downcase - end - # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { diff --git a/app/models/concerns/note_on_diff.rb b/app/models/concerns/note_on_diff.rb index b8dd27a7afe..1a5a7007a2b 100644 --- a/app/models/concerns/note_on_diff.rb +++ b/app/models/concerns/note_on_diff.rb @@ -1,3 +1,4 @@ +# Contains functionality shared between `DiffNote` and `LegacyDiffNote`. module NoteOnDiff extend ActiveSupport::Concern @@ -24,12 +25,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..772ff6a6d2f --- /dev/null +++ b/app/models/concerns/noteable.rb @@ -0,0 +1,68 @@ +module Noteable + # Names of all implementers of `Noteable` that support resolvable notes. + RESOLVABLE_TYPES = %w(MergeRequest).freeze + + def base_class_name + self.class.base_class.name + end + + # Convert this Noteable class name to a format usable by notifications. + # + # Examples: + # + # noteable.class # => MergeRequest + # noteable.human_class_name # => "merge request" + def human_class_name + @human_class_name ||= base_class_name.titleize.downcase + end + + def supports_resolvable_notes? + RESOLVABLE_TYPES.include?(base_class_name) + end + + def supports_discussions? + DiscussionNote::NOTEABLE_TYPES.include?(base_class_name) + end + + def discussion_notes + notes + end + + delegate :find_discussion, to: :discussion_notes + + def discussions + @discussions ||= discussion_notes + .inc_relations_for_view + .discussions(self) + end + + def grouped_diff_discussions + # Doesn't use `discussion_notes`, because this may include commit diff notes + # besides MR diff notes, that we do no want to display on the MR Changes tab. + notes.inc_relations_for_view.grouped_diff_discussions + end + + def resolvable_discussions + @resolvable_discussions ||= discussion_notes.resolvable.discussions(self) + end + + def discussions_resolvable? + resolvable_discussions.any?(&:resolvable?) + end + + def discussions_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 +end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb new file mode 100644 index 00000000000..dd979e7bb17 --- /dev/null +++ b/app/models/concerns/resolvable_discussion.rb @@ -0,0 +1,103 @@ +module ResolvableDiscussion + extend ActiveSupport::Concern + + included do + # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized. + # When this discussion is resolved or unresolved, the values of these properties potentially change. + # To make sure all memoized values are reset when this happens, `update` resets all instance variables with names in + # `memoized_variables`. If you add a memoized method in `ResolvableDiscussion` or any `Discussion` subclass, + # please make sure the instance variable name is added to `memoized_values`, like below. + cattr_accessor :memoized_values, instance_accessor: false do + [] + end + + memoized_values.push( + :resolvable, + :resolved, + :first_note, + :first_note_to_resolve, + :last_resolved_note, + :last_note + ) + + delegate :potentially_resolvable?, to: :first_note + + delegate :resolved_at, + :resolved_by, + + to: :last_resolved_note, + allow_nil: true + end + + def resolvable? + return @resolvable if @resolvable.present? + + @resolvable = potentially_resolvable? && notes.any?(&:resolvable?) + end + + def resolved? + return @resolved if @resolved.present? + + @resolved = resolvable? && notes.none?(&:to_be_resolved?) + end + + def first_note + @first_note ||= notes.first + end + + def first_note_to_resolve + return unless resolvable? + + @first_note_to_resolve ||= notes.find(&:to_be_resolved?) + end + + def last_resolved_note + return unless resolved? + + @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + end + + def resolved_notes + notes.select(&:resolved?) + end + + def to_be_resolved? + resolvable? && !resolved? + end + + def can_resolve?(current_user) + return false unless current_user + return false unless resolvable? + + current_user == self.noteable.author || + current_user.can?(:resolve_note, self.project) + end + + def resolve!(current_user) + return unless resolvable? + + update { |notes| notes.resolve!(current_user) } + end + + def unresolve! + return unless resolvable? + + update { |notes| notes.unresolve! } + end + + private + + def update + # 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.fresh.to_a + + self.class.memoized_values.each do |var| + instance_variable_set(:"@#{var}", nil) + end + end +end diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb new file mode 100644 index 00000000000..05eb6f86704 --- /dev/null +++ b/app/models/concerns/resolvable_note.rb @@ -0,0 +1,72 @@ +module ResolvableNote + extend ActiveSupport::Concern + + # Names of all subclasses of `Note` that can be resolvable. + RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze + + included do + belongs_to :resolved_by, class_name: "User" + + validates :resolved_by, presence: true, if: :resolved? + + # Keep this scope in sync with `#potentially_resolvable?` + scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) } + # Keep this scope in sync with `#resolvable?` + scope :resolvable, -> { potentially_resolvable.user } + + 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 + + # Keep this method in sync with the `potentially_resolvable` scope + def potentially_resolvable? + RESOLVABLE_TYPES.include?(self.class.name) && noteable.supports_resolvable_notes? + end + + # Keep this method in sync with the `resolvable` scope + def resolvable? + 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..d9b7e484e0f --- /dev/null +++ b/app/models/diff_discussion.rb @@ -0,0 +1,26 @@ +# A discussion on merge request or commit diffs consisting of `DiffNote` notes. +# +# A discussion of this type can be resolvable. +class DiffDiscussion < Discussion + include DiscussionOnDiff + + def self.note_class + DiffNote + end + + delegate :position, + :original_position, + + to: :first_note + + def legacy_diff_discussion? + false + end + + def reply_attributes + super.merge( + original_position: original_position.to_json, + position: position.to_json, + ) + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 895a91139c9..1523244f8a8 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -1,6 +1,11 @@ +# A note on merge request or commit diffs +# +# A note of this type can be resolvable. class DiffNote < Note include NoteOnDiff + NOTEABLE_TYPES = %w(MergeRequest Commit).freeze + serialize :original_position, Gitlab::Diff::Position serialize :position, Gitlab::Diff::Position @@ -8,59 +13,31 @@ class DiffNote < Note validates :position, presence: true 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? + validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } 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 - # 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_line_code 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 + def discussion_class(*) + DiffDiscussion end - def diff_attributes - { position: position.to_json } - 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 - def position=(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) + super(new_position) end - - super(new_position) end def diff_file @@ -88,43 +65,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 +80,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..0b6b920ed66 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,7 +1,10 @@ +# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes. +# +# A discussion of this type can be resolvable. class Discussion - NUMBER_OF_TRUNCATED_DIFF_LINES = 16 + include ResolvableDiscussion - attr_reader :notes + attr_reader :notes, :context_noteable delegate :created_at, :project, @@ -11,43 +14,62 @@ class Discussion :for_commit?, :for_merge_request?, - :line_code, - :original_line_code, - :diff_file, - :for_line?, - :active?, - to: :first_note - delegate :resolved_at, - :resolved_by, + def self.build(notes, context_noteable = nil) + notes.first.discussion_class(context_noteable).new(notes, context_noteable) + end - to: :last_resolved_note, - allow_nil: true + def self.build_collection(notes, context_noteable = nil) + notes.group_by { |n| n.discussion_id(context_noteable) }.values.map { |notes| build(notes, context_noteable) } + end - delegate :blob, - :highlighted_diff_lines, - :diff_lines, + # Returns an alphanumeric discussion ID based on `build_discussion_id` + def self.discussion_id(note) + Digest::SHA1.hexdigest(build_discussion_id(note).join("-")) + end - to: :diff_file, - allow_nil: true + # Returns an array of discussion ID components + def self.build_discussion_id(note) + [*base_discussion_id(note), SecureRandom.hex] + end - def self.for_notes(notes) - notes.group_by(&:discussion_id).values.map { |notes| new(notes) } + def self.base_discussion_id(note) + noteable_id = note.noteable_id || note.commit_id + [:discussion, note.noteable_type.try(:underscore), noteable_id] end - def self.for_diff_notes(notes) - notes.group_by(&:line_code).values.map { |notes| new(notes) } + # When notes on a commit are displayed in context of a merge request that contains that commit, + # these notes are to be displayed as if they were part of one discussion, even though they were actually + # individual notes on the commit with different discussion IDs, so that it's clear that these are not + # notes on the merge request itself. + # + # To turn a list of notes into a list of discussions, they are grouped by discussion ID, so to + # get these out-of-context notes to end up in the same discussion, we need to get them to return the same + # `discussion_id` when this grouping happens. To enable this, `Note#discussion_id` calls out + # to the `override_discussion_id` method on the appropriate `Discussion` subclass, as determined by + # the `discussion_class` method on `Note` or a subclass of `Note`. + # + # If no override is necessary, return `nil`. + # For the case described above, see `OutOfContextDiscussion.override_discussion_id`. + def self.override_discussion_id(note) + nil end - def initialize(notes) - @notes = notes + def self.note_class + DiscussionNote end - def last_resolved_note - return unless resolved? + def initialize(notes, context_noteable = nil) + @notes = notes + @context_noteable = context_noteable + end - @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last + def ==(other) + other.class == self.class && + other.context_noteable == self.context_noteable && + other.id == self.id && + other.notes == self.notes end def last_updated_at @@ -59,91 +81,29 @@ class Discussion end def id - first_note.discussion_id + first_note.discussion_id(context_noteable) end alias_method :to_param, :id def diff_discussion? - first_note.diff_note? - end - - def legacy_diff_discussion? - notes.any?(&:legacy_diff_note?) + false end - def resolvable? - return @resolvable if @resolvable.present? - - @resolvable = diff_discussion? && notes.any?(&:resolvable?) + def individual_note? + false end - def resolved? - return @resolved if @resolved.present? - - @resolved = resolvable? && notes.none?(&:to_be_resolved?) - end - - def first_note - @first_note ||= @notes.first - end - - def first_note_to_resolve - @first_note_to_resolve ||= notes.detect(&:to_be_resolved?) + def new_discussion? + notes.length == 1 end def last_note - @last_note ||= @notes.last - end - - def resolved_notes - notes.select(&:resolved?) - end - - def to_be_resolved? - resolvable? && !resolved? - end - - def can_resolve?(current_user) - return false unless current_user - return false unless resolvable? - - current_user == self.noteable.author || - current_user.can?(:resolve_note, self.project) - end - - def resolve!(current_user) - return unless resolvable? - - update { |notes| notes.resolve!(current_user) } - end - - def unresolve! - return unless resolvable? - - 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? + @last_note ||= notes.last 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? - end + resolved? end def expanded? @@ -151,52 +111,6 @@ 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 - end - - private - - def update - notes_relation = DiffNote.where(id: notes.map(&:id)).fresh - yield(notes_relation) - - # Set the notes array to the updated notes - @notes = notes_relation.to_a - - # Reset the memoized values - @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil + first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id) end end diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb new file mode 100644 index 00000000000..e660b024083 --- /dev/null +++ b/app/models/discussion_note.rb @@ -0,0 +1,13 @@ +# A note in a non-diff discussion on an issue, merge request, commit, or snippet. +# +# A note of this type can be resolvable. +class DiscussionNote < Note + # Names of all implementers of `Noteable` that support discussions. + NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze + + validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } + + def discussion_class(*) + Discussion + end +end diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb new file mode 100644 index 00000000000..c3f21c55240 --- /dev/null +++ b/app/models/individual_note_discussion.rb @@ -0,0 +1,13 @@ +# A discussion to wrap a single `Note` note on the root of an issue, merge request, +# commit, or snippet, that is not displayed as a discussion. +# +# A discussion of this type is never resolvable. +class IndividualNoteDiscussion < Discussion + def self.note_class + Note + end + + def individual_note? + true + end +end diff --git a/app/models/issue.rb b/app/models/issue.rb index f9704b0d754..d8d9db477d2 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..cb2651a03f8 --- /dev/null +++ b/app/models/legacy_diff_discussion.rb @@ -0,0 +1,25 @@ +# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes. +# +# All new diff discussions are of the type `DiffDiscussion`, but any diff discussions created +# before the introduction of the new implementation still use `LegacyDiffDiscussion`. +# +# A discussion of this type is never resolvable. +class LegacyDiffDiscussion < Discussion + include DiscussionOnDiff + + def legacy_diff_discussion? + true + end + + def self.note_class + LegacyDiffNote + end + + def collapsed? + !active? + end + + def reply_attributes + super.merge(line_code: line_code) + end +end diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 40277a9b139..9a77557ebcd 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -1,3 +1,9 @@ +# A note on merge request or commit diffs, using the legacy implementation. +# +# All new diff notes are of the type `DiffNote`, but any diff notes created +# before the introduction of the new implementation still use `LegacyDiffNote`. +# +# A note of this type is never resolvable. class LegacyDiffNote < Note include NoteOnDiff @@ -7,18 +13,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 - end - - def legacy_diff_note? - true - end - - def diff_attributes - { line_code: line_code } + def discussion_class(*) + LegacyDiffDiscussion end def project_repository @@ -119,8 +115,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 b2725a314ad..b71a9e17a93 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,43 +476,7 @@ 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 - - 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) - end - - def discussions_resolvable? - diff_discussions.any?(&:resolvable?) - end - - def discussions_resolved? - discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?) - end - - def discussions_to_be_resolved? - discussions_resolvable? && !discussions_resolved? - end + alias_method :discussion_notes, :related_notes def mergeable_discussions_state? return true unless project.only_allow_merge_if_all_discussions_are_resolved? @@ -857,8 +822,8 @@ class MergeRequest < ActiveRecord::Base return unless has_complete_diff_refs? return if new_diff_refs == old_diff_refs - active_diff_notes = self.notes.diff_notes.select do |note| - note.new_diff_note? && note.active?(old_diff_refs) + active_diff_notes = self.notes.new_diff_notes.select do |note| + note.active?(old_diff_refs) end return if active_diff_notes.empty? diff --git a/app/models/note.rb b/app/models/note.rb index 16d66cb1427..1ea7b946061 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -1,3 +1,6 @@ +# A note on the root of an issue, merge request, commit, or snippet. +# +# A note of this type is never resolvable. class Note < ActiveRecord::Base extend ActiveModel::Naming include Gitlab::CurrentSettings @@ -8,6 +11,10 @@ class Note < ActiveRecord::Base include FasterCacheKeys include CacheMarkdownField include AfterCommitQueue + include ResolvableNote + include IgnorableColumn + + ignore_column :original_discussion_id cache_markdown_field :note, pipeline: :note @@ -32,9 +39,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,10 +58,11 @@ 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, 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 - errors.add(:invalid_project, 'Note and noteable project mismatch') + errors.add(:project, 'does not match noteable project') end end @@ -69,6 +74,7 @@ class Note < ActiveRecord::Base scope :user, ->{ where(system: false) } scope :common, ->{ where(noteable_type: ["", nil]) } scope :fresh, ->{ order(created_at: :asc, id: :asc) } + scope :updated_after, ->(time){ where('updated_at > ?', time) } scope :inc_author_project, ->{ includes(:project, :author) } scope :inc_author, ->{ includes(:author) } scope :inc_relations_for_view, -> do @@ -76,7 +82,8 @@ class Note < ActiveRecord::Base end scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) } - scope :non_diff_notes, ->{ where(type: ['Note', nil]) } + scope :new_diff_notes, ->{ where(type: 'DiffNote') } + scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) } scope :with_associations, -> do # FYI noteable cannot be loaded for LegacyDiffNote for commits @@ -86,7 +93,7 @@ class Note < ActiveRecord::Base after_initialize :ensure_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code - before_validation :set_discussion_id + before_validation :set_discussion_id, on: :create after_save :keep_around_commit, unless: :for_personal_snippet? after_save :expire_etag_cache @@ -95,22 +102,23 @@ 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(context_noteable = nil) + Discussion.build_collection(fresh, context_noteable) end - def discussion_id(*args) - Digest::SHA1.hexdigest(build_discussion_id(*args)) - end + def find_discussion(discussion_id) + notes = where(discussion_id: discussion_id).fresh.to_a + return if notes.empty? - def discussions - Discussion.for_notes(fresh) + 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. + discussions. + select(&:active?). + group_by(&:line_code) end def count_for_collection(ids, type) @@ -121,37 +129,17 @@ class Note < ActiveRecord::Base end def cross_reference? - system && SystemNoteService.cross_reference?(note) + system? && SystemNoteService.cross_reference?(note) end def diff_note? false end - def legacy_diff_note? - false - end - - def new_diff_note? - false - end - def active? 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 +216,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 +227,63 @@ class Note < ActiveRecord::Base for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore end + def can_be_discussion_note? + self.noteable.supports_discussions? && !part_of_discussion? + 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. + # See also `#discussion_id` and `Discussion.override_discussion_id`. + if noteable && noteable != self.noteable + OutOfContextDiscussion + else + IndividualNoteDiscussion + end + end + + # See `Discussion.override_discussion_id` for details. + def discussion_id(noteable = nil) + discussion_class(noteable).override_discussion_id(self) || super() + end + + # Returns a discussion containing just this note. + # This method exists as an alternative to `#discussion` to use when the methods + # we intend to call on the Discussion object don't require it to have all of its notes, + # and just depend on the first note or the type of discussion. This saves us a DB query. + def to_discussion(noteable = nil) + Discussion.build([self], noteable) + end + + # Returns the entire discussion this note is part of. + # Consider using `#to_discussion` if we do not need to render the discussion + # and all its notes and if we don't care about the discussion's resolvability status. + def discussion + full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion? + full_discussion || to_discussion + end + + def part_of_discussion? + !to_discussion.individual_note? + end + + def in_reply_to?(other) + case other + when Note + if part_of_discussion? + in_reply_to?(other.noteable) && in_reply_to?(other.to_discussion) + else + in_reply_to?(other.noteable) + end + when Discussion + self.discussion_id == other.id + when Noteable + self.noteable == other + else + false + end + end + private def keep_around_commit @@ -264,17 +309,7 @@ class Note < ActiveRecord::Base end def set_discussion_id - self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id) - 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 + self.discussion_id ||= discussion_class.discussion_id(self) end def expire_etag_cache diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb new file mode 100644 index 00000000000..85794630f70 --- /dev/null +++ b/app/models/out_of_context_discussion.rb @@ -0,0 +1,22 @@ +# When notes on a commit are displayed in the context of a merge request that +# contains that commit, they are displayed as if they were a discussion. +# +# This represents one of those discussions, consisting of `Note` notes. +# +# A discussion of this type is never resolvable. +class OutOfContextDiscussion < Discussion + # Returns an array of discussion ID components + def self.build_discussion_id(note) + base_discussion_id(note) + end + + # To make sure all out-of-context notes end up grouped as one discussion, + # we override the discussion ID to be a newly generated but consistent ID. + def self.override_discussion_id(note) + discussion_id(note) + end + + def self.note_class + Note + end +end diff --git a/app/models/sent_notification.rb b/app/models/sent_notification.rb index f4bcb49b34d..bfaf0eb2fae 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 @@ -22,9 +23,7 @@ class SentNotification < ActiveRecord::Base find_by(reply_key: reply_key) end - def record(noteable, recipient_id, reply_key, attrs = {}) - return unless reply_key - + def record(noteable, recipient_id, reply_key = self.reply_key, attrs = {}) noteable_id = nil commit_id = nil if noteable.is_a?(Commit) @@ -34,23 +33,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 + def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {}) + attrs[:in_reply_to_discussion_id] = note.discussion_id record(note.noteable, recipient_id, reply_key, attrs) end @@ -89,31 +85,45 @@ 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 - } - end - - def create_note(note) - Notes::CreateService.new( - self.project, - self.recipient, - self.note_attributes.merge(note: note) - ).execute + def create_reply(message, dryrun: false) + klass = dryrun ? Notes::BuildService : Notes::CreateService + klass.new(self.project, self.recipient, reply_params.merge(note: message)).execute end private + def reply_params + attrs = { + noteable_type: self.noteable_type, + noteable_id: self.noteable_id, + commit_id: self.commit_id + } + + if self.in_reply_to_discussion_id.present? + attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id + else + # Remove in GitLab 10.0, when we will not support replying to SentNotifications + # that don't have `in_reply_to_discussion_id` anymore. + attrs.merge!( + type: self.note_type, + + # LegacyDiffNote + line_code: self.line_code, + + # DiffNote + position: self.position.to_json + ) + end + + attrs + end + def note_valid - Note.new(note_attributes.merge(note: "Test")).valid? + note = create_reply('Test', dryrun: true) + + unless note.valid? + self.errors.add(:base, "Note parameters are invalid: #{note.errors.full_messages.to_sentence}") + end end def keep_around_commit 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..910a2a15e5d 100644 --- a/app/services/concerns/issues/resolve_discussions.rb +++ b/app/services/concerns/issues/resolve_discussions.rb @@ -21,11 +21,11 @@ module Issues @discussions_to_resolve ||= if discussion_to_resolve_id discussion_or_nil = merge_request_to_resolve_discussions_of - .find_diff_discussion(discussion_to_resolve_id) + .find_discussion(discussion_to_resolve_id) 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/issues/build_service.rb b/app/services/issues/build_service.rb index 77bced4bd5c..3a4f7b159f1 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -35,14 +35,19 @@ module Issues end def item_for_discussion(discussion) - first_note = discussion.first_note_to_resolve || discussion.first_note + first_note_to_resolve = discussion.first_note_to_resolve || discussion.first_note + + is_very_first_note = first_note_to_resolve == discussion.first_note + action = is_very_first_note ? "started" : "commented on" + + note_url = Gitlab::UrlBuilder.build(first_note_to_resolve) + other_note_count = discussion.notes.size - 1 - note_url = Gitlab::UrlBuilder.build(first_note) - discussion_info = "- [ ] #{first_note.author.to_reference} commented on a [discussion](#{note_url}): " + discussion_info = "- [ ] #{first_note_to_resolve.author.to_reference} #{action} a [discussion](#{note_url}): " discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0 - note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note.note).call + note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note_to_resolve.note).call spaces = ' ' * 4 quote = note_without_block_quotes.lines.map { |line| "#{spaces}> #{line}" }.join diff --git a/app/services/notes/build_service.rb b/app/services/notes/build_service.rb new file mode 100644 index 00000000000..ea7cacc956c --- /dev/null +++ b/app/services/notes/build_service.rb @@ -0,0 +1,25 @@ +module Notes + class BuildService < ::BaseService + def execute + in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id) + + if project && in_reply_to_discussion_id.present? + discussion = 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..f3954f6f8c4 100644 --- a/app/services/notes/create_service.rb +++ b/app/services/notes/create_service.rb @@ -1,12 +1,10 @@ module Notes - class CreateService < BaseService + class CreateService < ::BaseService 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/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml index ee452add394..e6d307e5568 100644 --- a/app/views/discussions/_diff_discussion.html.haml +++ b/app/views/discussions/_diff_discussion.html.haml @@ -3,4 +3,4 @@ %td.notes_line{ colspan: 2 } %td.notes_content .content{ class: ('hide' unless expanded) } - = render "discussions/notes", discussion: discussion + = render partial: "discussions/notes", collection: discussions, as: :discussion diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 94408b92374..549364761e6 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -7,7 +7,7 @@ .diff-content.code.js-syntax-highlight %table - - discussions = { discussion.original_line_code => discussion } + - discussions = { discussion.original_line_code => [discussion] } = render partial: "projects/diffs/line", collection: discussion.truncated_diff_lines, as: :line, diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 2d78c55211e..e04958817e4 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -5,7 +5,7 @@ = link_to user_path(discussion.author) do = image_tag avatar_icon(discussion.author), class: "avatar s40" .timeline-content - .discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } } + .discussion.js-toggle-container{ data: { discussion_id: discussion.id } } .discussion-header .discussion-actions %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" } @@ -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..34789808f10 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,18 +1,20 @@ -%ul.notes{ data: { discussion_id: discussion.id } } - = render partial: "projects/notes/note", collection: discussion.notes, as: :note +.discussion-notes + %ul.notes{ data: { discussion_id: discussion.id } } + = render partial: "projects/notes/note", collection: discussion.notes, as: :note -- if current_user - .discussion-reply-holder - - if discussion.diff_discussion? - - line_type = local_assigns.fetch(:line_type, nil) + - if current_user + .discussion-reply-holder + - 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 - .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 - - else - = link_to_reply_discussion(discussion) + - else + = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_parallel_diff_discussion.html.haml b/app/views/discussions/_parallel_diff_discussion.html.haml index 3a19e021643..253cd336882 100644 --- a/app/views/discussions/_parallel_diff_discussion.html.haml +++ b/app/views/discussions/_parallel_diff_discussion.html.haml @@ -1,20 +1,20 @@ -- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?) +- expanded = [*discussions_left, *discussions_right].any?(&:expanded?) %tr.notes_holder{ class: ('hide' unless expanded) } - - if discussion_left + - if discussions_left %td.notes_line.old %td.notes_content.parallel.old - .content{ class: ('hide' unless discussion_left.expanded?) } - = render "discussions/notes", discussion: discussion_left, line_type: 'old' + .content{ class: ('hide' unless discussions_left.any?(&:expanded?)) } + = render partial: "discussions/notes", collection: discussions_left, as: :discussion, line_type: 'old' - else %td.notes_line.old= ("") %td.notes_content.parallel.old .content - - if discussion_right + - if discussions_right %td.notes_line.new %td.notes_content.parallel.new - .content{ class: ('hide' unless discussion_right.expanded?) } - = render "discussions/notes", discussion: discussion_right, line_type: 'new' + .content{ class: ('hide' unless discussions_right.any?(&:expanded?)) } + = render partial: "discussions/notes", collection: discussions_right, as: :discussion, line_type: 'new' - else %td.notes_line.new= ("") %td.notes_content.parallel.new 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/layouts/mailer.text.erb b/app/views/layouts/mailer.text.erb new file mode 100644 index 00000000000..198f30a1dc4 --- /dev/null +++ b/app/views/layouts/mailer.text.erb @@ -0,0 +1,4 @@ +<%= yield -%> + +--- +You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>. diff --git a/app/views/layouts/mailer.text.haml b/app/views/layouts/mailer.text.haml deleted file mode 100644 index 6a9c6ced9cc..00000000000 --- a/app/views/layouts/mailer.text.haml +++ /dev/null @@ -1,5 +0,0 @@ -= yield - -You're receiving this email because of your account on #{Gitlab.config.gitlab.host}. -Manage all notifications: #{profile_notifications_url} -Help: #{help_url} diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index 76268c1b705..40bf45cece7 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -25,8 +25,8 @@ - if @labels_url adjust your #{link_to 'label subscriptions', @labels_url}. - else - - if @sent_notification_url - = link_to "unsubscribe", @sent_notification_url + - if @unsubscribe_url + = link_to "unsubscribe", @unsubscribe_url from this thread or adjust your notification settings. diff --git a/app/views/layouts/notify.text.erb b/app/views/layouts/notify.text.erb new file mode 100644 index 00000000000..b4ce02eead8 --- /dev/null +++ b/app/views/layouts/notify.text.erb @@ -0,0 +1,12 @@ +<%= yield -%> + +--- +<% if @target_url -%> +<% if @reply_by_email -%> +<%= "Reply to this email directly or view it on GitLab: #{@target_url}" -%> +<% else -%> +<%= "View it on GitLab: #{@target_url}" -%> +<% end -%> +<% end -%> + +You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>. diff --git a/app/views/notify/_note_email.html.haml b/app/views/notify/_note_email.html.haml new file mode 100644 index 00000000000..a80518f7986 --- /dev/null +++ b/app/views/notify/_note_email.html.haml @@ -0,0 +1,37 @@ +- discussion = @note.discussion if @note.part_of_discussion? +- if discussion + %p.details + = succeed ':' do + = link_to @note.author_name, user_url(@note.author) + + - if discussion.diff_discussion? + - if discussion.new_discussion? + started a new discussion + - else + commented on a discussion + + on #{link_to discussion.file_path, @target_url} + - else + - if discussion.new_discussion? + started a new discussion + - else + commented on a #{link_to 'discussion', @target_url} + +- elsif current_application_settings.email_author_in_body + %p.details + #{link_to @note.author_name, user_url(@note.author)} commented: + +- if discussion&.diff_discussion? + = content_for :head do + = stylesheet_link_tag 'mailers/highlighted_diff_email' + + %table + = render partial: "projects/diffs/line", + collection: discussion.truncated_diff_lines, + as: :line, + locals: { diff_file: discussion.diff_file, + plain: true, + email: true } + +%div + = markdown(@note.note, pipeline: :email, author: @note.author) diff --git a/app/views/notify/_note_email.text.erb b/app/views/notify/_note_email.text.erb new file mode 100644 index 00000000000..cb2e7fab6d5 --- /dev/null +++ b/app/views/notify/_note_email.text.erb @@ -0,0 +1,26 @@ +<% discussion = @note.discussion if @note.part_of_discussion? -%> +<% if discussion && !discussion.individual_note? -%> +<%= @note.author_name -%> +<% if discussion.new_discussion? -%> +<%= " started a new discussion" -%> +<% else -%> +<%= " commented on a discussion" -%> +<% end -%> +<% if discussion.diff_discussion? -%> +<%= " on #{discussion.file_path}" -%> +<% end -%> +<%= ":" -%> + + +<% elsif current_application_settings.email_author_in_body -%> +<%= "#{@note.author_name} commented:" -%> + + +<% end -%> +<% if discussion&.diff_discussion? -%> +<% discussion.truncated_diff_lines(highlight: false).each do |line| -%> +<%= "> #{line.text}\n" -%> +<% end -%> + +<% end -%> +<%= @note.note -%> diff --git a/app/views/notify/_note_message.html.haml b/app/views/notify/_note_message.html.haml deleted file mode 100644 index e9c66170877..00000000000 --- a/app/views/notify/_note_message.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -- if current_application_settings.email_author_in_body - %div - #{link_to @note.author_name, user_url(@note.author)} wrote: -%div - = markdown(@note.note, pipeline: :email, author: @note.author) diff --git a/app/views/notify/_note_message.text.erb b/app/views/notify/_note_message.text.erb deleted file mode 100644 index f82cbc9a3fc..00000000000 --- a/app/views/notify/_note_message.text.erb +++ /dev/null @@ -1,5 +0,0 @@ -<% if current_application_settings.email_author_in_body %> - <%= @note.author_name %> wrote: -<% end -%> - -<%= @note.note %> diff --git a/app/views/notify/_note_mr_or_commit_email.html.haml b/app/views/notify/_note_mr_or_commit_email.html.haml deleted file mode 100644 index edf8dfe7e9e..00000000000 --- a/app/views/notify/_note_mr_or_commit_email.html.haml +++ /dev/null @@ -1,18 +0,0 @@ -= content_for :head do - = stylesheet_link_tag 'mailers/highlighted_diff_email' - -New comment - -- if @discussion && @discussion.diff_file - on - = link_to @note.diff_file.file_path, @target_url, class: 'details' - \: - %table - = render partial: "projects/diffs/line", - collection: @discussion.truncated_diff_lines, - as: :line, - locals: { diff_file: @note.diff_file, - plain: true, - email: true } - -= render 'note_message' diff --git a/app/views/notify/_note_mr_or_commit_email.text.erb b/app/views/notify/_note_mr_or_commit_email.text.erb deleted file mode 100644 index b4fcdf6b1e9..00000000000 --- a/app/views/notify/_note_mr_or_commit_email.text.erb +++ /dev/null @@ -1,8 +0,0 @@ -<% if @discussion && @discussion.diff_file -%> - on <%= @note.diff_file.file_path -%> -<% end -%>: - -<%= url %> - -<%= render 'simple_diff' if @discussion -%> -<%= render 'note_message' %> diff --git a/app/views/notify/_simple_diff.text.erb b/app/views/notify/_simple_diff.text.erb deleted file mode 100644 index c28d1cc34d3..00000000000 --- a/app/views/notify/_simple_diff.text.erb +++ /dev/null @@ -1,3 +0,0 @@ -<% @discussion.truncated_diff_lines(highlight: false).each do |line| %> -> <%= line.text %> -<% end %> diff --git a/app/views/notify/new_issue_email.html.haml b/app/views/notify/new_issue_email.html.haml index d1855568215..c762578971a 100644 --- a/app/views/notify/new_issue_email.html.haml +++ b/app/views/notify/new_issue_email.html.haml @@ -1,9 +1,11 @@ - if current_application_settings.email_author_in_body - %div - #{link_to @issue.author_name, user_url(@issue.author)} wrote: -- if @issue.description - = markdown(@issue.description, pipeline: :email, author: @issue.author) + %p.details + #{link_to @issue.author_name, user_url(@issue.author)} created an issue: - if @issue.assignee_id.present? %p Assignee: #{@issue.assignee_name} + +- if @issue.description + %div + = markdown(@issue.description, pipeline: :email, author: @issue.author) diff --git a/app/views/notify/new_mention_in_issue_email.html.haml b/app/views/notify/new_mention_in_issue_email.html.haml index 02f21baa368..6b45ac265f7 100644 --- a/app/views/notify/new_mention_in_issue_email.html.haml +++ b/app/views/notify/new_mention_in_issue_email.html.haml @@ -1,12 +1,4 @@ %p You have been mentioned in an issue. -- if current_application_settings.email_author_in_body - %div - #{link_to @issue.author_name, user_url(@issue.author)} wrote: -- if @issue.description - = markdown(@issue.description, pipeline: :email, author: @issue.author) - -- if @issue.assignee_id.present? - %p - Assignee: #{@issue.assignee_name} += render template: 'notify/new_issue_email' diff --git a/app/views/notify/new_mention_in_merge_request_email.html.haml b/app/views/notify/new_mention_in_merge_request_email.html.haml index cbd434be02a..b061f9c106e 100644 --- a/app/views/notify/new_mention_in_merge_request_email.html.haml +++ b/app/views/notify/new_mention_in_merge_request_email.html.haml @@ -1,15 +1,4 @@ %p You have been mentioned in Merge Request #{@merge_request.to_reference} -- if current_application_settings.email_author_in_body - %div - #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote: -%p.details - != merge_path_description(@merge_request, '→') - -- if @merge_request.assignee_id.present? - %p - Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} - -- if @merge_request.description - = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) += render template: 'notify/new_merge_request_email' diff --git a/app/views/notify/new_merge_request_email.html.haml b/app/views/notify/new_merge_request_email.html.haml index 8890b300f7d..951c96bdb9c 100644 --- a/app/views/notify/new_merge_request_email.html.haml +++ b/app/views/notify/new_merge_request_email.html.haml @@ -1,12 +1,14 @@ - if current_application_settings.email_author_in_body - %div - #{link_to @merge_request.author_name, user_url(@merge_request.author)} wrote: + %p.details + #{link_to @merge_request.author_name, user_url(@merge_request.author)} created a merge request: + %p.details != merge_path_description(@merge_request, '→') - if @merge_request.assignee_id.present? %p - Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name} + Assignee: #{@merge_request.assignee_name} - if @merge_request.description - = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) + %div + = markdown(@merge_request.description, pipeline: :email, author: @merge_request.author) diff --git a/app/views/notify/note_commit_email.html.haml b/app/views/notify/note_commit_email.html.haml index 0a650e3b2ca..5e69f01a486 100644 --- a/app/views/notify/note_commit_email.html.haml +++ b/app/views/notify/note_commit_email.html.haml @@ -1,2 +1 @@ -%p.details - = render 'note_mr_or_commit_email' += render 'note_email' diff --git a/app/views/notify/note_commit_email.text.erb b/app/views/notify/note_commit_email.text.erb index 6aa085a172e..413d9e6e9ac 100644 --- a/app/views/notify/note_commit_email.text.erb +++ b/app/views/notify/note_commit_email.text.erb @@ -1,2 +1 @@ -New comment for Commit <%= @commit.short_id -%> -<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url } %> +<%= render 'note_email' %> diff --git a/app/views/notify/note_issue_email.html.haml b/app/views/notify/note_issue_email.html.haml index 2fa2f784661..5e69f01a486 100644 --- a/app/views/notify/note_issue_email.html.haml +++ b/app/views/notify/note_issue_email.html.haml @@ -1 +1 @@ -= render 'note_message' += render 'note_email' diff --git a/app/views/notify/note_issue_email.text.erb b/app/views/notify/note_issue_email.text.erb index e33cbcd70f2..413d9e6e9ac 100644 --- a/app/views/notify/note_issue_email.text.erb +++ b/app/views/notify/note_issue_email.text.erb @@ -1,9 +1 @@ -New comment for Issue <%= @issue.iid %> - -<%= url_for(namespace_project_issue_url(@issue.project.namespace, @issue.project, @issue, anchor: "note_#{@note.id}")) %> - - -Author: <%= @note.author_name %> - -<%= @note.note %> - +<%= render 'note_email' %> diff --git a/app/views/notify/note_merge_request_email.html.haml b/app/views/notify/note_merge_request_email.html.haml index 0a650e3b2ca..5e69f01a486 100644 --- a/app/views/notify/note_merge_request_email.html.haml +++ b/app/views/notify/note_merge_request_email.html.haml @@ -1,2 +1 @@ -%p.details - = render 'note_mr_or_commit_email' += render 'note_email' diff --git a/app/views/notify/note_merge_request_email.text.erb b/app/views/notify/note_merge_request_email.text.erb index 2ce64c494cf..413d9e6e9ac 100644 --- a/app/views/notify/note_merge_request_email.text.erb +++ b/app/views/notify/note_merge_request_email.text.erb @@ -1,2 +1 @@ -New comment for Merge Request <%= @merge_request.to_reference -%> -<%= render partial: 'note_mr_or_commit_email', locals: { url: @target_url }%> +<%= render 'note_email' %> diff --git a/app/views/notify/note_personal_snippet_email.html.haml b/app/views/notify/note_personal_snippet_email.html.haml index 2fa2f784661..5e69f01a486 100644 --- a/app/views/notify/note_personal_snippet_email.html.haml +++ b/app/views/notify/note_personal_snippet_email.html.haml @@ -1 +1 @@ -= render 'note_message' += render 'note_email' diff --git a/app/views/notify/note_personal_snippet_email.text.erb b/app/views/notify/note_personal_snippet_email.text.erb index b2a8809a23b..413d9e6e9ac 100644 --- a/app/views/notify/note_personal_snippet_email.text.erb +++ b/app/views/notify/note_personal_snippet_email.text.erb @@ -1,8 +1 @@ -New comment for Snippet <%= @snippet.id %> - -<%= url_for(snippet_url(@snippet, anchor: "note_#{@note.id}")) %> - - -Author: <%= @note.author_name %> - -<%= @note.note %> +<%= render 'note_email' %> diff --git a/app/views/notify/note_snippet_email.html.haml b/app/views/notify/note_snippet_email.html.haml index 2fa2f784661..5e69f01a486 100644 --- a/app/views/notify/note_snippet_email.html.haml +++ b/app/views/notify/note_snippet_email.html.haml @@ -1 +1 @@ -= render 'note_message' += render 'note_email' diff --git a/app/views/notify/note_snippet_email.text.erb b/app/views/notify/note_snippet_email.text.erb index 4d5a406f4b0..413d9e6e9ac 100644 --- a/app/views/notify/note_snippet_email.text.erb +++ b/app/views/notify/note_snippet_email.text.erb @@ -1,8 +1 @@ -New comment for Snippet <%= @snippet.id %> - -<%= url_for(namespace_project_snippet_url(@snippet.project.namespace, @snippet.project, @snippet, anchor: "note_#{@note.id}")) %> - - -Author: <%= @note.author_name %> - -<%= @note.note %> +<%= render 'note_email' %> 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/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index c09c7b87e24..3e426ee9e7d 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -4,7 +4,7 @@ - type = line.type - line_code = diff_file.line_code(line) - if discussions && !line.meta? - - discussion = discussions[line_code] + - line_discussions = discussions[line_code] %tr.line_holder{ class: type, id: (line_code unless plain) } - case type - when 'match' @@ -20,6 +20,7 @@ = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } + - discussion = line_discussions.try(:first) - if discussion && discussion.resolvable? && !plain %diff-note-avatars{ "discussion-id" => discussion.id } %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } } @@ -34,6 +35,6 @@ - else = diff_line_content(line.text) -- if discussion - - discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?) - = render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded +- if line_discussions + - discussion_expanded = local_assigns.fetch(:discussion_expanded, line_discussions.any?(&:expanded?)) + = render "discussions/diff_discussion", discussions: line_discussions, expanded: discussion_expanded diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index b7346f27ddb..f920f359de2 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -6,7 +6,7 @@ - right = line[:right] - last_line = right.new_pos if right - unless @diff_notes_disabled - - discussion_left, discussion_right = parallel_diff_discussions(left, right, diff_file) + - discussions_left, discussions_right = parallel_diff_discussions(left, right, diff_file) %tr.line_holder.parallel - if left - case left.type @@ -20,6 +20,7 @@ - left_position = diff_file.position(left) %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } + - discussion_left = discussions_left.try(:first) - if discussion_left && discussion_left.resolvable? %diff-note-avatars{ "discussion-id" => discussion_left.id } %td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text) @@ -39,6 +40,7 @@ - right_position = diff_file.position(right) %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } + - discussion_right = discussions_right.try(:first) - if discussion_right && discussion_right.resolvable? %diff-note-avatars{ "discussion-id" => discussion_right.id } %td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text) @@ -46,8 +48,8 @@ %td.old_line.diff-line-num.empty-cell %td.line_content.parallel - - if discussion_left || discussion_right - = render "discussions/parallel_diff_discussion", discussion_left: discussion_left, discussion_right: discussion_right + - if discussions_left || discussions_right + = render "discussions/parallel_diff_discussion", discussions_left: discussions_left, discussions_right: discussions_right - if !diff_file.new_file && !diff_file.deleted_file && diff_file.diff_lines.any? - last_line = diff_file.diff_lines.last - if last_line.new_pos < total_lines diff --git a/app/views/projects/merge_requests/_discussion.html.haml b/app/views/projects/merge_requests/_discussion.html.haml index cfb44bd206c..15b5a51c1d0 100644 --- a/app/views/projects/merge_requests/_discussion.html.haml +++ b/app/views/projects/merge_requests/_discussion.html.haml @@ -1,9 +1,9 @@ - content_for :note_actions do - if can?(current_user, :update_merge_request, @merge_request) - if @merge_request.open? - = link_to 'Close merge request', merge_request_path(@merge_request, merge_request: {state_event: :close }), method: :put, class: "btn btn-nr btn-comment btn-close close-mr-link js-note-target-close", title: "Close merge request", data: {original_text: "Close merge request", alternative_text: "Comment & close merge request"} + = link_to 'Close merge request', merge_request_path(@merge_request, merge_request: { state_event: :close }), method: :put, class: "btn btn-nr btn-comment btn-close close-mr-link js-note-target-close", title: "Close merge request", data: { original_text: "Close merge request", alternative_text: "Comment & close merge request"} - if @merge_request.reopenable? - = link_to 'Reopen merge request', merge_request_path(@merge_request, merge_request: {state_event: :reopen }), method: :put, class: "btn btn-nr btn-comment btn-reopen reopen-mr-link js-note-target-reopen", title: "Reopen merge request", data: {original_text: "Reopen merge request", alternative_text: "Comment & reopen merge request"} + = link_to 'Reopen merge request', merge_request_path(@merge_request, merge_request: { state_event: :reopen }), method: :put, class: "btn btn-nr btn-comment btn-reopen reopen-mr-link js-note-target-close js-note-target-reopen", title: "Reopen merge request", data: { original_text: "Reopen merge request", alternative_text: "Comment & reopen merge request"} %comment-and-resolve-btn{ "inline-template" => true, ":discussion-id" => "" } %button.btn.btn-nr.btn-default.append-right-10.js-comment-resolve-button{ "v-if" => "showButton", type: "submit", data: { project_path: "#{project_path(@merge_request.project)}" } } {{ buttonText }} diff --git a/app/views/projects/notes/_comment_button.html.haml b/app/views/projects/notes/_comment_button.html.haml new file mode 100644 index 00000000000..6bb55f04b6e --- /dev/null +++ b/app/views/projects/notes/_comment_button.html.haml @@ -0,0 +1,30 @@ +- noteable_name = @note.noteable.human_class_name + +.pull-left.btn-group.append-right-10.comment-type-dropdown.js-comment-type-dropdown + %input.btn.btn-nr.btn-create.comment-btn.js-comment-button.js-comment-submit-button{ type: 'submit', value: 'Comment' } + + - if @note.can_be_discussion_note? + = button_tag type: 'button', class: 'btn btn-nr dropdown-toggle comment-btn js-note-new-discussion js-disable-on-submit', data: { 'dropdown-trigger' => '#resolvable-comment-menu' }, 'aria-label' => 'Open comment type dropdown' do + = icon('caret-down', class: 'toggle-icon') + + %ul#resolvable-comment-menu.dropdown-menu{ data: { dropdown: true } } + %li#comment.droplab-item-selected{ data: { value: '', 'submit-text' => 'Comment', 'close-text' => "Comment & close #{noteable_name}", 'reopen-text' => "Comment & reopen #{noteable_name}" } } + %a{ href: '#' } + = icon('check') + .description + %strong Comment + %p + Add a general comment to this #{noteable_name}. + + %li.divider + + %li#discussion{ data: { value: 'DiscussionNote', 'submit-text' => 'Start discussion', 'close-text' => "Start discussion & close #{noteable_name}", 'reopen-text' => "Start discussion & reopen #{noteable_name}" } } + %a{ href: '#' } + = icon('check') + .description + %strong Start discussion + %p + = succeed '.' do + Discuss a specific suggestion or question + - if @note.noteable.supports_resolvable_notes? + that needs to be resolved diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index b561052e721..0d835a9e949 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 @@ -22,7 +28,9 @@ .error-alert .note-form-actions.clearfix - = f.submit 'Comment', class: "btn btn-nr btn-create append-right-10 comment-btn js-comment-button" + = render partial: 'projects/notes/comment_button' + = 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 9130dc128fa..c12c05eeb73 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -34,7 +34,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..2b2bab09c74 100644 --- a/app/views/projects/notes/_notes.html.haml +++ b/app/views/projects/notes/_notes.html.haml @@ -1,7 +1,7 @@ -- if @discussions.present? +- if defined?(@discussions) - @discussions.each do |discussion| - - if discussion.for_target?(@noteable) - = render partial: "projects/notes/note", object: discussion.first_note, as: :note + - if discussion.individual_note? + = render partial: "projects/notes/note", collection: discussion.notes, as: :note - else = render 'discussions/discussion', discussion: discussion - else |