summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2017-04-08 06:11:31 +0000
committerDouwe Maan <douwe@gitlab.com>2017-04-08 06:11:31 +0000
commit3d1cade13f61115b63bf6dbda5a1f194ba54b24b (patch)
treee9edc13dbd28ba1eb4bbeb995f596ead2ad28b2c
parent4040776430c2bbef689924c206e2c2c46428e4ef (diff)
parentf058b52b50c76de9557b167a1a31a5c9ba446f31 (diff)
downloadgitlab-ce-3d1cade13f61115b63bf6dbda5a1f194ba54b24b.tar.gz
Merge branch 'new-resolvable-discussion' into 'master'
Add option to start a new resolvable discussion in an MR Closes #24378 See merge request !7527
-rw-r--r--app/assets/javascripts/comment_type_toggle.js60
-rw-r--r--app/assets/javascripts/diff_notes/components/comment_resolve_btn.js12
-rw-r--r--app/assets/javascripts/droplab/drop_down.js2
-rw-r--r--app/assets/javascripts/droplab/plugins/input_setter.js2
-rw-r--r--app/assets/javascripts/files_comment_button.js26
-rw-r--r--app/assets/javascripts/gl_form.js3
-rw-r--r--app/assets/javascripts/main.js2
-rw-r--r--app/assets/javascripts/notes.js196
-rw-r--r--app/assets/javascripts/render_gfm.js1
-rw-r--r--app/assets/stylesheets/pages/note_form.scss91
-rw-r--r--app/assets/stylesheets/pages/notes.scss12
-rw-r--r--app/controllers/concerns/renders_notes.rb20
-rw-r--r--app/controllers/projects/commit_controller.rb20
-rw-r--r--app/controllers/projects/discussions_controller.rb2
-rw-r--r--app/controllers/projects/issues_controller.rb12
-rwxr-xr-xapp/controllers/projects/merge_requests_controller.rb32
-rw-r--r--app/controllers/projects/notes_controller.rb94
-rw-r--r--app/controllers/projects/snippets_controller.rb5
-rw-r--r--app/finders/notes_finder.rb65
-rw-r--r--app/helpers/diff_helper.rb8
-rw-r--r--app/helpers/notes_helper.rb56
-rw-r--r--app/mailers/emails/notes.rb17
-rw-r--r--app/mailers/notify.rb4
-rw-r--r--app/models/commit.rb5
-rw-r--r--app/models/concerns/discussion_on_diff.rb57
-rw-r--r--app/models/concerns/ignorable_column.rb28
-rw-r--r--app/models/concerns/issuable.rb11
-rw-r--r--app/models/concerns/note_on_diff.rb9
-rw-r--r--app/models/concerns/noteable.rb68
-rw-r--r--app/models/concerns/resolvable_discussion.rb103
-rw-r--r--app/models/concerns/resolvable_note.rb72
-rw-r--r--app/models/diff_discussion.rb26
-rw-r--r--app/models/diff_note.rb120
-rw-r--r--app/models/discussion.rb200
-rw-r--r--app/models/discussion_note.rb13
-rw-r--r--app/models/individual_note_discussion.rb13
-rw-r--r--app/models/issue.rb1
-rw-r--r--app/models/legacy_diff_discussion.rb25
-rw-r--r--app/models/legacy_diff_note.rb24
-rw-r--r--app/models/merge_request.rb43
-rw-r--r--app/models/note.rb133
-rw-r--r--app/models/out_of_context_discussion.rb22
-rw-r--r--app/models/sent_notification.rb84
-rw-r--r--app/models/snippet.rb1
-rw-r--r--app/services/concerns/issues/resolve_discussions.rb4
-rw-r--r--app/services/issues/build_service.rb13
-rw-r--r--app/services/notes/build_service.rb25
-rw-r--r--app/services/notes/create_service.rb8
-rw-r--r--app/services/system_note_service.rb8
-rw-r--r--app/views/discussions/_diff_discussion.html.haml2
-rw-r--r--app/views/discussions/_diff_with_notes.html.haml2
-rw-r--r--app/views/discussions/_discussion.html.haml16
-rw-r--r--app/views/discussions/_notes.html.haml28
-rw-r--r--app/views/discussions/_parallel_diff_discussion.html.haml14
-rw-r--r--app/views/discussions/_resolve_all.html.haml17
-rw-r--r--app/views/layouts/mailer.text.erb4
-rw-r--r--app/views/layouts/mailer.text.haml5
-rw-r--r--app/views/layouts/notify.html.haml4
-rw-r--r--app/views/layouts/notify.text.erb12
-rw-r--r--app/views/notify/_note_email.html.haml37
-rw-r--r--app/views/notify/_note_email.text.erb26
-rw-r--r--app/views/notify/_note_message.html.haml5
-rw-r--r--app/views/notify/_note_message.text.erb5
-rw-r--r--app/views/notify/_note_mr_or_commit_email.html.haml18
-rw-r--r--app/views/notify/_note_mr_or_commit_email.text.erb8
-rw-r--r--app/views/notify/_simple_diff.text.erb3
-rw-r--r--app/views/notify/new_issue_email.html.haml10
-rw-r--r--app/views/notify/new_mention_in_issue_email.html.haml10
-rw-r--r--app/views/notify/new_mention_in_merge_request_email.html.haml13
-rw-r--r--app/views/notify/new_merge_request_email.html.haml10
-rw-r--r--app/views/notify/note_commit_email.html.haml3
-rw-r--r--app/views/notify/note_commit_email.text.erb3
-rw-r--r--app/views/notify/note_issue_email.html.haml2
-rw-r--r--app/views/notify/note_issue_email.text.erb10
-rw-r--r--app/views/notify/note_merge_request_email.html.haml3
-rw-r--r--app/views/notify/note_merge_request_email.text.erb3
-rw-r--r--app/views/notify/note_personal_snippet_email.html.haml2
-rw-r--r--app/views/notify/note_personal_snippet_email.text.erb9
-rw-r--r--app/views/notify/note_snippet_email.html.haml2
-rw-r--r--app/views/notify/note_snippet_email.text.erb9
-rw-r--r--app/views/projects/commit/show.html.haml1
-rw-r--r--app/views/projects/diffs/_line.html.haml9
-rw-r--r--app/views/projects/diffs/_parallel_view.html.haml8
-rw-r--r--app/views/projects/merge_requests/_discussion.html.haml4
-rw-r--r--app/views/projects/notes/_comment_button.html.haml30
-rw-r--r--app/views/projects/notes/_form.html.haml16
-rw-r--r--app/views/projects/notes/_note.html.haml2
-rw-r--r--app/views/projects/notes/_notes.html.haml6
-rw-r--r--changelogs/unreleased/new-resolvable-discussion.yml4
-rw-r--r--db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb29
-rw-r--r--db/post_migrate/20170404170532_remove_notes_original_discussion_id.rb23
-rw-r--r--db/schema.rb3
-rw-r--r--features/steps/project/merge_requests.rb3
-rw-r--r--lib/gitlab/diff/line.rb4
-rw-r--r--lib/gitlab/email/handler/create_note_handler.rb13
-rw-r--r--spec/controllers/projects/commit_controller_spec.rb4
-rw-r--r--spec/controllers/projects/discussions_controller_spec.rb2
-rw-r--r--spec/controllers/projects/issues_controller_spec.rb2
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb4
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb133
-rw-r--r--spec/factories/notes.rb28
-rw-r--r--spec/factories/sent_notifications.rb4
-rw-r--r--spec/features/discussion_comments_spec.rb295
-rw-r--r--spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb2
-rw-r--r--spec/features/merge_requests/diff_notes_avatars_spec.rb4
-rw-r--r--spec/features/merge_requests/diff_notes_resolve_spec.rb2
-rw-r--r--spec/features/notes_on_merge_requests_spec.rb2
-rw-r--r--spec/finders/notes_finder_spec.rb41
-rw-r--r--spec/helpers/issues_helper_spec.rb2
-rw-r--r--spec/helpers/notes_helper_spec.rb17
-rw-r--r--spec/javascripts/comment_type_toggle_spec.js157
-rw-r--r--spec/javascripts/droplab/drop_down_spec.js17
-rw-r--r--spec/javascripts/droplab/plugins/input_setter_spec.js34
-rw-r--r--spec/lib/gitlab/email/handler/create_note_handler_spec.rb1
-rw-r--r--spec/mailers/notify_spec.rb142
-rw-r--r--spec/mailers/previews/notify_preview.rb96
-rw-r--r--spec/models/concerns/discussion_on_diff_spec.rb24
-rw-r--r--spec/models/concerns/noteable_spec.rb261
-rw-r--r--spec/models/concerns/resolvable_discussion_spec.rb548
-rw-r--r--spec/models/concerns/resolvable_note_spec.rb329
-rw-r--r--spec/models/diff_discussion_spec.rb19
-rw-r--r--spec/models/diff_note_spec.rb334
-rw-r--r--spec/models/discussion_spec.rb623
-rw-r--r--spec/models/legacy_diff_discussion_spec.rb11
-rw-r--r--spec/models/legacy_diff_note_spec.rb101
-rw-r--r--spec/models/merge_request_spec.rb176
-rw-r--r--spec/models/note_spec.rb293
-rw-r--r--spec/models/sent_notification_spec.rb166
-rw-r--r--spec/requests/api/issues_spec.rb2
-rw-r--r--spec/requests/api/v3/issues_spec.rb2
-rw-r--r--spec/services/discussions/resolve_service_spec.rb4
-rw-r--r--spec/services/issues/build_service_spec.rb16
-rw-r--r--spec/services/issues/create_service_spec.rb2
-rw-r--r--spec/services/issues/resolve_discussions_spec.rb2
-rw-r--r--spec/services/notes/build_service_spec.rb40
-rw-r--r--spec/services/notification_service_spec.rb2
-rw-r--r--spec/services/system_note_service_spec.rb2
-rw-r--r--spec/support/slash_commands_helpers.rb2
-rw-r--r--spec/support/time_tracking_shared_examples.rb2
-rw-r--r--spec/views/projects/notes/_form.html.haml_spec.rb4
140 files changed, 4037 insertions, 2225 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, '&rarr;')
-
-- if @merge_request.assignee_id.present?
- %p
- Assignee: #{@merge_request.author_name} &rarr; #{@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, '&rarr;')
- if @merge_request.assignee_id.present?
%p
- Assignee: #{@merge_request.author_name} &rarr; #{@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
diff --git a/changelogs/unreleased/new-resolvable-discussion.yml b/changelogs/unreleased/new-resolvable-discussion.yml
new file mode 100644
index 00000000000..f4dc4ea3ede
--- /dev/null
+++ b/changelogs/unreleased/new-resolvable-discussion.yml
@@ -0,0 +1,4 @@
+---
+title: Add option to start a new resolvable discussion in an MR
+merge_request: 7527
+author:
diff --git a/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb b/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb
new file mode 100644
index 00000000000..d56d83ca1d3
--- /dev/null
+++ b/db/migrate/20161128095517_add_in_reply_to_discussion_id_to_sent_notifications.rb
@@ -0,0 +1,29 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddInReplyToDiscussionIdToSentNotifications < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ # Set this constant to true if this migration requires downtime.
+ DOWNTIME = false
+
+ # When a migration requires downtime you **must** uncomment the following
+ # constant and define a short and easy to understand explanation as to why the
+ # migration requires downtime.
+ # DOWNTIME_REASON = ''
+
+ # When using the methods "add_concurrent_index" or "add_column_with_default"
+ # you must disable the use of transactions as these methods can not run in an
+ # existing transaction. When using "add_concurrent_index" make sure that this
+ # method is the _only_ method called in the migration, any other changes
+ # should go in a separate migration. This ensures that upon failure _only_ the
+ # index creation fails and can be retried or reverted easily.
+ #
+ # To disable transactions uncomment the following line and remove these
+ # comments:
+ # disable_ddl_transaction!
+
+ def change
+ add_column :sent_notifications, :in_reply_to_discussion_id, :string
+ end
+end
diff --git a/db/post_migrate/20170404170532_remove_notes_original_discussion_id.rb b/db/post_migrate/20170404170532_remove_notes_original_discussion_id.rb
new file mode 100644
index 00000000000..0c3b3bd5eb3
--- /dev/null
+++ b/db/post_migrate/20170404170532_remove_notes_original_discussion_id.rb
@@ -0,0 +1,23 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveNotesOriginalDiscussionId < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ # When using the methods "add_concurrent_index" or "add_column_with_default"
+ # you must disable the use of transactions as these methods can not run in an
+ # existing transaction. When using "add_concurrent_index" make sure that this
+ # method is the _only_ method called in the migration, any other changes
+ # should go in a separate migration. This ensures that upon failure _only_ the
+ # index creation fails and can be retried or reverted easily.
+ #
+ # To disable transactions uncomment the following line and remove these
+ # comments:
+ # disable_ddl_transaction!
+
+ def change
+ remove_column :notes, :original_discussion_id, :string
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 5660c58dc5f..d400c823387 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -12,7 +12,6 @@
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20170407140450) do
-
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
enable_extension "pg_trgm"
@@ -757,7 +756,6 @@ ActiveRecord::Schema.define(version: 20170407140450) do
t.datetime "resolved_at"
t.integer "resolved_by_id"
t.string "discussion_id"
- t.string "original_discussion_id"
t.text "note_html"
end
@@ -1055,6 +1053,7 @@ ActiveRecord::Schema.define(version: 20170407140450) do
t.string "line_code"
t.string "note_type"
t.text "position"
+ t.string "in_reply_to_discussion_id"
end
add_index "sent_notifications", ["reply_key"], name: "index_sent_notifications_on_reply_key", unique: true, using: :btree
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index 263ec321a34..3985fe8f2f7 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -347,6 +347,9 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
end
step 'I should see a discussion by user "John Doe" has started on diff' do
+ # Trigger a refresh of notes
+ execute_script("$(document).trigger('visibilitychange');")
+ wait_for_ajax
page.within(".notes .discussion") do
page.should have_content "#{user_exists("John Doe").name} #{user_exists("John Doe").to_reference} started a discussion"
page.should have_content sample_commit.line_code_path
diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb
index 114656958e3..0a15c6d9358 100644
--- a/lib/gitlab/diff/line.rb
+++ b/lib/gitlab/diff/line.rb
@@ -33,6 +33,10 @@ module Gitlab
new_pos unless removed? || meta?
end
+ def line
+ new_line || old_line
+ end
+
def unchanged?
type.nil?
end
diff --git a/lib/gitlab/email/handler/create_note_handler.rb b/lib/gitlab/email/handler/create_note_handler.rb
index d87ba427f4b..0e22f2189ee 100644
--- a/lib/gitlab/email/handler/create_note_handler.rb
+++ b/lib/gitlab/email/handler/create_note_handler.rb
@@ -1,4 +1,3 @@
-
require 'gitlab/email/handler/base_handler'
require 'gitlab/email/handler/reply_processing'
@@ -42,17 +41,7 @@ module Gitlab
end
def create_note
- Notes::CreateService.new(
- project,
- author,
- note: message,
- noteable_type: sent_notification.noteable_type,
- noteable_id: sent_notification.noteable_id,
- commit_id: sent_notification.commit_id,
- line_code: sent_notification.line_code,
- position: sent_notification.position,
- type: sent_notification.note_type
- ).execute
+ sent_notification.create_reply(message)
end
end
end
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index b223a22ae60..69e4706dc71 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -266,8 +266,8 @@ describe Projects::CommitController do
diff_for_path(id: commit2.id, old_path: existing_path, new_path: existing_path)
expect(assigns(:diff_notes_disabled)).to be_falsey
- expect(assigns(:comments_target)).to eq(noteable_type: 'Commit',
- commit_id: commit2.id)
+ expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'Commit',
+ commit_id: commit2.id)
end
it 'only renders the diffs for the path given' do
diff --git a/spec/controllers/projects/discussions_controller_spec.rb b/spec/controllers/projects/discussions_controller_spec.rb
index 79ab364a6f3..fe62898fa9b 100644
--- a/spec/controllers/projects/discussions_controller_spec.rb
+++ b/spec/controllers/projects/discussions_controller_spec.rb
@@ -4,7 +4,7 @@ describe Projects::DiscussionsController do
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
- let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+ let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
let(:discussion) { note.discussion }
let(:request_params) do
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index d5f1d46bf7f..79034b8d24d 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -519,7 +519,7 @@ describe Projects::IssuesController do
end
context 'resolving discussions in MergeRequest' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 99d5583e683..1739d40ab88 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -586,8 +586,8 @@ describe Projects::MergeRequestsController do
diff_for_path(id: merge_request.iid, old_path: existing_path, new_path: existing_path)
expect(assigns(:diff_notes_disabled)).to be_falsey
- expect(assigns(:comments_target)).to eq(noteable_type: 'MergeRequest',
- noteable_id: merge_request.id)
+ expect(assigns(:new_diff_note_attrs)).to eq(noteable_type: 'MergeRequest',
+ noteable_id: merge_request.id)
end
it 'only renders the diffs for the path given' do
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index d80780b1d90..f140eaef5d5 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -14,6 +14,109 @@ describe Projects::NotesController do
}
end
+ describe 'GET index' do
+ let(:request_params) do
+ {
+ namespace_id: project.namespace,
+ project_id: project,
+ target_type: 'issue',
+ target_id: issue.id,
+ format: 'json'
+ }
+ end
+
+ let(:parsed_response) { JSON.parse(response.body).with_indifferent_access }
+ let(:note_json) { parsed_response[:notes].first }
+
+ before do
+ sign_in(user)
+ project.team << [user, :developer]
+ end
+
+ it 'passes last_fetched_at from headers to NotesFinder' do
+ last_fetched_at = 3.hours.ago.to_i
+
+ request.headers['X-Last-Fetched-At'] = last_fetched_at
+
+ expect(NotesFinder).to receive(:new)
+ .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
+ .and_call_original
+
+ get :index, request_params
+ end
+
+ context 'for a discussion note' do
+ let!(:note) { create(:discussion_note_on_issue, noteable: issue, project: project) }
+
+ it 'responds with the expected attributes' do
+ get :index, request_params
+
+ expect(note_json[:id]).to eq(note.id)
+ expect(note_json[:discussion_html]).not_to be_nil
+ expect(note_json[:diff_discussion_html]).to be_nil
+ end
+ end
+
+ context 'for a diff discussion note' do
+ let(:project) { create(:project, :repository) }
+ let!(:note) { create(:diff_note_on_merge_request, project: project) }
+
+ let(:params) { request_params.merge(target_type: 'merge_request', target_id: note.noteable_id) }
+
+ it 'responds with the expected attributes' do
+ get :index, params
+
+ expect(note_json[:id]).to eq(note.id)
+ expect(note_json[:discussion_html]).not_to be_nil
+ expect(note_json[:diff_discussion_html]).not_to be_nil
+ end
+ end
+
+ context 'for a commit note' do
+ let(:project) { create(:project, :repository) }
+ let!(:note) { create(:note_on_commit, project: project) }
+
+ context 'when displayed on a merge request' do
+ let(:merge_request) { create(:merge_request, source_project: project) }
+
+ let(:params) { request_params.merge(target_type: 'merge_request', target_id: merge_request.id) }
+
+ it 'responds with the expected attributes' do
+ get :index, params
+
+ expect(note_json[:id]).to eq(note.id)
+ expect(note_json[:discussion_html]).not_to be_nil
+ expect(note_json[:diff_discussion_html]).to be_nil
+ end
+ end
+
+ context 'when displayed on the commit' do
+ let(:params) { request_params.merge(target_type: 'commit', target_id: note.commit_id) }
+
+ it 'responds with the expected attributes' do
+ get :index, params
+
+ expect(note_json[:id]).to eq(note.id)
+ expect(note_json[:discussion_html]).to be_nil
+ expect(note_json[:diff_discussion_html]).to be_nil
+ end
+ end
+ end
+
+ context 'for a regular note' do
+ let!(:note) { create(:note, noteable: issue, project: project) }
+
+ it 'responds with the expected attributes' do
+ get :index, request_params
+
+ expect(note_json[:id]).to eq(note.id)
+ expect(note_json[:html]).not_to be_nil
+ expect(note_json[:discussion_html]).to be_nil
+ expect(note_json[:diff_discussion_html]).to be_nil
+ end
+ end
+ end
+
describe 'POST create' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.source_project }
@@ -49,7 +152,8 @@ describe Projects::NotesController do
note: 'some note',
noteable_id: merge_request.id.to_s,
noteable_type: 'MergeRequest',
- merge_request_diff_head_sha: 'sha'
+ merge_request_diff_head_sha: 'sha',
+ in_reply_to_discussion_id: nil
}
expect(Notes::CreateService).to receive(:new).with(project, user, service_params).and_return(double(execute: true))
@@ -200,31 +304,4 @@ describe Projects::NotesController do
end
end
end
-
- describe 'GET index' do
- let(:last_fetched_at) { '1487756246' }
- let(:request_params) do
- {
- namespace_id: project.namespace,
- project_id: project,
- target_type: 'issue',
- target_id: issue.id
- }
- end
-
- before do
- sign_in(user)
- project.team << [user, :developer]
- end
-
- it 'passes last_fetched_at from headers to NotesFinder' do
- request.headers['X-Last-Fetched-At'] = last_fetched_at
-
- expect(NotesFinder).to receive(:new)
- .with(anything, anything, hash_including(last_fetched_at: last_fetched_at))
- .and_call_original
-
- get :index, request_params
- end
- end
end
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb
index fe19a404e16..90c35e2c7f8 100644
--- a/spec/factories/notes.rb
+++ b/spec/factories/notes.rb
@@ -16,10 +16,21 @@ FactoryGirl.define do
factory :note_on_personal_snippet, traits: [:on_personal_snippet]
factory :system_note, traits: [:system]
- factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote do
+ factory :discussion_note_on_merge_request, traits: [:on_merge_request], class: DiscussionNote do
association :project, :repository
+
+ trait :resolved do
+ resolved_at { Time.now }
+ resolved_by { create(:user) }
+ end
end
+ factory :discussion_note_on_issue, traits: [:on_issue], class: DiscussionNote
+
+ factory :discussion_note_on_commit, traits: [:on_commit], class: DiscussionNote
+
+ factory :legacy_diff_note_on_commit, traits: [:on_commit, :legacy_diff_note], class: LegacyDiffNote
+
factory :legacy_diff_note_on_merge_request, traits: [:on_merge_request, :legacy_diff_note], class: LegacyDiffNote do
association :project, :repository
end
@@ -37,7 +48,7 @@ FactoryGirl.define do
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: line_number,
- diff_refs: noteable.diff_refs
+ diff_refs: noteable.try(:diff_refs)
)
end
@@ -108,5 +119,18 @@ FactoryGirl.define do
trait :with_svg_attachment do
attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") }
end
+
+ transient do
+ in_reply_to nil
+ end
+
+ before(:create) do |note, evaluator|
+ discussion = evaluator.in_reply_to
+ next unless discussion
+ discussion = discussion.to_discussion if discussion.is_a?(Note)
+ next unless discussion
+
+ note.assign_attributes(discussion.reply_attributes.merge(project: discussion.project))
+ end
end
end
diff --git a/spec/factories/sent_notifications.rb b/spec/factories/sent_notifications.rb
index 6287c40afe9..99253be5a22 100644
--- a/spec/factories/sent_notifications.rb
+++ b/spec/factories/sent_notifications.rb
@@ -2,7 +2,7 @@ FactoryGirl.define do
factory :sent_notification do
project factory: :empty_project
recipient factory: :user
- noteable factory: :issue
- reply_key "0123456789abcdef" * 2
+ noteable { create(:issue, project: project) }
+ reply_key { SentNotification.reply_key }
end
end
diff --git a/spec/features/discussion_comments_spec.rb b/spec/features/discussion_comments_spec.rb
new file mode 100644
index 00000000000..ae778118c5c
--- /dev/null
+++ b/spec/features/discussion_comments_spec.rb
@@ -0,0 +1,295 @@
+require 'spec_helper'
+
+shared_examples 'discussion comments' do |resource_name|
+ let(:form_selector) { '.js-main-target-form' }
+ let(:dropdown_selector) { "#{form_selector} .comment-type-dropdown" }
+ let(:toggle_selector) { "#{dropdown_selector} .dropdown-toggle" }
+ let(:menu_selector) { "#{dropdown_selector} .dropdown-menu" }
+ let(:submit_selector) { "#{form_selector} .js-comment-submit-button" }
+ let(:close_selector) { "#{form_selector} .btn-comment-and-close" }
+ let(:comments_selector) { '.timeline > .note.timeline-entry' }
+
+ it 'should show a comment type toggle' do
+ expect(page).to have_selector toggle_selector
+ end
+
+ it 'clicking "Comment" will post a comment' do
+ find("#{form_selector} .note-textarea").send_keys('a')
+
+ find(submit_selector).click
+
+ find(comments_selector, match: :first)
+ new_comment = all(comments_selector).last
+
+ expect(new_comment).to have_content 'a'
+ expect(new_comment).not_to have_selector '.discussion'
+ end
+
+ if resource_name == 'issue'
+ it "clicking 'Comment & close #{resource_name}' will post a comment and close the #{resource_name}" do
+ find("#{form_selector} .note-textarea").send_keys('a')
+
+ find(close_selector).click
+
+ find(comments_selector, match: :first)
+ find("#{comments_selector}.system-note")
+ entries = all(comments_selector)
+ close_note = entries.last
+ new_comment = entries[-2]
+
+ expect(close_note).to have_content 'closed'
+ expect(new_comment).not_to have_selector '.discussion'
+ end
+ end
+
+ describe 'when the toggle is clicked' do
+ before do
+ find("#{form_selector} .note-textarea").send_keys('a')
+
+ find(toggle_selector).click
+ end
+
+ it 'opens a comment type dropdown with "Comment" and "Start discussion"' do
+ expect(page).to have_selector menu_selector
+ end
+
+ it 'has a "Comment" item' do
+ menu = find(menu_selector)
+
+ expect(menu).to have_content 'Comment'
+ expect(menu).to have_content "Add a general comment to this #{resource_name}."
+ end
+
+ it 'has a "Start discussion" item' do
+ menu = find(menu_selector)
+
+ expect(menu).to have_content 'Start discussion'
+ expect(menu).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}."
+ end
+
+ it 'has the "Comment" item selected by default' do
+ find("#{menu_selector} li", match: :first)
+ items = all("#{menu_selector} li")
+
+ expect(items.first).to have_content 'Comment'
+ expect(items.first).to have_selector '.fa-check'
+ expect(items.first['class']).to match 'droplab-item-selected'
+
+ expect(items.last).to have_content 'Start discussion'
+ expect(items.last).not_to have_selector '.fa-check'
+ expect(items.last['class']).not_to match 'droplab-item-selected'
+ end
+
+ it 'closes the menu when clicking the toggle' do
+ find(toggle_selector).click
+
+ expect(page).not_to have_selector menu_selector
+ end
+
+ it 'closes the menu when clicking the body' do
+ find('body').click
+
+ expect(page).not_to have_selector menu_selector
+ end
+
+ it 'clicking the ul padding should not change the text' do
+ find(menu_selector).trigger 'click'
+
+ expect(find(dropdown_selector)).to have_content 'Comment'
+ end
+
+ describe 'when selecting "Start discussion"' do
+ before do
+ find("#{menu_selector} li", match: :first)
+ all("#{menu_selector} li").last.click
+ end
+
+ it 'updates the note_type input to "DiscussionNote"' do
+ expect(find("#{form_selector} #note_type", visible: false).value).to eq('DiscussionNote')
+ end
+
+ it 'updates the submit button text' do
+ expect(find(dropdown_selector)).to have_content 'Start discussion'
+ end
+
+ if resource_name =~ /(issue|merge request)/
+ it 'updates the close button text' do
+ expect(find(close_selector)).to have_content "Start discussion & close #{resource_name}"
+ end
+
+ it 'typing does not change the close button text' do
+ find("#{form_selector} .note-textarea").send_keys('b')
+
+ expect(find(close_selector)).to have_content "Start discussion & close #{resource_name}"
+ end
+ end
+
+ it 'closes the dropdown' do
+ expect(page).not_to have_selector menu_selector
+ end
+
+ it 'clicking "Start discussion" will post a discussion' do
+ find(submit_selector).click
+
+ find(comments_selector, match: :first)
+ new_comment = all(comments_selector).last
+
+ expect(new_comment).to have_content 'a'
+ expect(new_comment).to have_selector '.discussion'
+ end
+
+ if resource_name == 'issue'
+ it "clicking 'Start discussion & close #{resource_name}' will post a discussion and close the #{resource_name}" do
+ find(close_selector).click
+
+ find(comments_selector, match: :first)
+ find("#{comments_selector}.system-note")
+ entries = all(comments_selector)
+ close_note = entries.last
+ new_discussion = entries[-2]
+
+ expect(close_note).to have_content 'closed'
+ expect(new_discussion).to have_selector '.discussion'
+ end
+ end
+
+ describe 'when opening the menu' do
+ before do
+ find(toggle_selector).click
+ end
+
+ it 'should have "Start discussion" selected' do
+ find("#{menu_selector} li", match: :first)
+ items = all("#{menu_selector} li")
+
+ expect(items.first).to have_content 'Comment'
+ expect(items.first).not_to have_selector '.fa-check'
+ expect(items.first['class']).not_to match 'droplab-item-selected'
+
+ expect(items.last).to have_content 'Start discussion'
+ expect(items.last).to have_selector '.fa-check'
+ expect(items.last['class']).to match 'droplab-item-selected'
+ end
+
+ describe 'when selecting "Comment"' do
+ before do
+ find("#{menu_selector} li", match: :first).click
+ end
+
+ it 'clears the note_type input"' do
+ expect(find("#{form_selector} #note_type", visible: false).value).to eq('')
+ end
+
+ it 'updates the submit button text' do
+ expect(find(dropdown_selector)).to have_content 'Comment'
+ end
+
+ if resource_name =~ /(issue|merge request)/
+ it 'updates the close button text' do
+ expect(find(close_selector)).to have_content "Comment & close #{resource_name}"
+ end
+
+ it 'typing does not change the close button text' do
+ find("#{form_selector} .note-textarea").send_keys('b')
+
+ expect(find(close_selector)).to have_content "Comment & close #{resource_name}"
+ end
+ end
+
+ it 'closes the dropdown' do
+ expect(page).not_to have_selector menu_selector
+ end
+
+ it 'should have "Comment" selected when opening the menu' do
+ find(toggle_selector).click
+
+ find("#{menu_selector} li", match: :first)
+ items = all("#{menu_selector} li")
+
+ expect(items.first).to have_content 'Comment'
+ expect(items.first).to have_selector '.fa-check'
+ expect(items.first['class']).to match 'droplab-item-selected'
+
+ expect(items.last).to have_content 'Start discussion'
+ expect(items.last).not_to have_selector '.fa-check'
+ expect(items.last['class']).not_to match 'droplab-item-selected'
+ end
+ end
+ end
+ end
+ end
+
+ if resource_name =~ /(issue|merge request)/
+ describe "on a closed #{resource_name}" do
+ before do
+ find("#{form_selector} .js-note-target-close").click
+
+ find("#{form_selector} .note-textarea").send_keys('a')
+ end
+
+ it "should show a 'Comment & reopen #{resource_name}' button" do
+ expect(find("#{form_selector} .js-note-target-reopen")).to have_content "Comment & reopen #{resource_name}"
+ end
+
+ it "should show a 'Start discussion & reopen #{resource_name}' button when 'Start discussion' is selected" do
+ find(toggle_selector).click
+
+ find("#{menu_selector} li", match: :first)
+ all("#{menu_selector} li").last.click
+
+ expect(find("#{form_selector} .js-note-target-reopen")).to have_content "Start discussion & reopen #{resource_name}"
+ end
+ end
+ end
+end
+
+describe 'Discussion Comments', :feature, :js do
+ include RepoHelpers
+
+ let(:user) { create(:user) }
+ let(:project) { create(:project) }
+
+ before do
+ project.team << [user, :developer]
+
+ login_as(user)
+ end
+
+ describe 'on a merge request' do
+ let(:merge_request) { create(:merge_request, source_project: project) }
+
+ before do
+ visit namespace_project_merge_request_path(project.namespace, project, merge_request)
+ end
+
+ it_behaves_like 'discussion comments', 'merge request'
+ end
+
+ describe 'on an issue' do
+ let(:issue) { create(:issue, project: project) }
+
+ before do
+ visit namespace_project_issue_path(project.namespace, project, issue)
+ end
+
+ it_behaves_like 'discussion comments', 'issue'
+ end
+
+ describe 'on an snippet' do
+ let(:snippet) { create(:project_snippet, :private, project: project, author: user) }
+
+ before do
+ visit namespace_project_snippet_path(project.namespace, project, snippet)
+ end
+
+ it_behaves_like 'discussion comments', 'snippet'
+ end
+
+ describe 'on a commit' do
+ before do
+ visit namespace_project_commit_path(project.namespace, project, sample_commit.id)
+ end
+
+ it_behaves_like 'discussion comments', 'commit'
+ end
+end
diff --git a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
index 572bca3de21..58f897cba3e 100644
--- a/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
+++ b/spec/features/issues/create_issue_for_discussions_in_merge_request_spec.rb
@@ -4,7 +4,7 @@ feature 'Resolving all open discussions in a merge request from an issue', featu
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project) }
- let!(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: merge_request, project: project)]).first }
+ let!(:discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion }
describe 'as a user with access to the project' do
before do
diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb
index a6c72b0b3ac..218d95a88b8 100644
--- a/spec/features/merge_requests/diff_notes_avatars_spec.rb
+++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb
@@ -164,9 +164,7 @@ feature 'Diff note avatars', feature: true, js: true do
context 'multiple comments' do
before do
- create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
- create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
- create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position)
+ create_list(:diff_note_on_merge_request, 3, project: project, noteable: merge_request, in_reply_to: note)
visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, view: view)
diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb
index 69164aabdb2..88d28b649a4 100644
--- a/spec/features/merge_requests/diff_notes_resolve_spec.rb
+++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb
@@ -191,7 +191,7 @@ feature 'Diff notes resolve', feature: true, js: true do
context 'multiple notes' do
before do
- create(:diff_note_on_merge_request, project: project, noteable: merge_request)
+ create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: note)
visit_merge_request
end
diff --git a/spec/features/notes_on_merge_requests_spec.rb b/spec/features/notes_on_merge_requests_spec.rb
index fab2d532e06..783f2e93909 100644
--- a/spec/features/notes_on_merge_requests_spec.rb
+++ b/spec/features/notes_on_merge_requests_spec.rb
@@ -25,7 +25,7 @@ describe 'Comments', feature: true do
describe 'the note form' do
it 'is valid' do
is_expected.to have_css('.js-main-target-form', visible: true, count: 1)
- expect(find('.js-main-target-form input[type=submit]').value).
+ expect(find('.js-main-target-form .js-comment-button').value).
to eq('Comment')
page.within('.js-main-target-form') do
expect(page).not_to have_link('Cancel')
diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 77a04507be1..765bf44d863 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -202,4 +202,45 @@ describe NotesFinder do
end
end
end
+
+ describe '#target' do
+ subject { described_class.new(project, user, params) }
+
+ context 'for a issue target' do
+ let(:issue) { create(:issue, project: project) }
+ let(:params) { { target_type: 'issue', target_id: issue.id } }
+
+ it 'returns the issue' do
+ expect(subject.target).to eq(issue)
+ end
+ end
+
+ context 'for a merge request target' do
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ let(:params) { { target_type: 'merge_request', target_id: merge_request.id } }
+
+ it 'returns the merge_request' do
+ expect(subject.target).to eq(merge_request)
+ end
+ end
+
+ context 'for a snippet target' do
+ let(:snippet) { create(:project_snippet, project: project) }
+ let(:params) { { target_type: 'snippet', target_id: snippet.id } }
+
+ it 'returns the snippet' do
+ expect(subject.target).to eq(snippet)
+ end
+ end
+
+ context 'for a commit target' do
+ let(:project) { create(:project, :repository) }
+ let(:commit) { project.commit }
+ let(:params) { { target_type: 'commit', target_id: commit.id } }
+
+ it 'returns the commit' do
+ expect(subject.target).to eq(commit)
+ end
+ end
+ end
end
diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb
index f0554cc068d..540cb0ab1e0 100644
--- a/spec/helpers/issues_helper_spec.rb
+++ b/spec/helpers/issues_helper_spec.rb
@@ -150,7 +150,7 @@ describe IssuesHelper do
describe "when passing a discussion" do
let(:diff_note) { create(:diff_note_on_merge_request) }
let(:merge_request) { diff_note.noteable }
- let(:discussion) { Discussion.new([diff_note]) }
+ let(:discussion) { diff_note.to_discussion }
it "links to the merge request with first note if a single discussion was passed" do
expected_path = Gitlab::UrlBuilder.build(diff_note)
diff --git a/spec/helpers/notes_helper_spec.rb b/spec/helpers/notes_helper_spec.rb
index 9c577501f00..a427de32c4c 100644
--- a/spec/helpers/notes_helper_spec.rb
+++ b/spec/helpers/notes_helper_spec.rb
@@ -36,21 +36,4 @@ describe NotesHelper do
expect(helper.note_max_access_for_user(other_note)).to eq('Reporter')
end
end
-
- describe '#preload_max_access_for_authors' do
- before do
- # This method reads cache from RequestStore, so make sure it's clean.
- RequestStore.clear!
- end
-
- it 'loads multiple users' do
- expected_access = {
- owner.id => Gitlab::Access::OWNER,
- master.id => Gitlab::Access::MASTER,
- reporter.id => Gitlab::Access::REPORTER
- }
-
- expect(helper.preload_max_access_for_authors(notes, project)).to eq(expected_access)
- end
- end
end
diff --git a/spec/javascripts/comment_type_toggle_spec.js b/spec/javascripts/comment_type_toggle_spec.js
new file mode 100644
index 00000000000..dfd0810d52e
--- /dev/null
+++ b/spec/javascripts/comment_type_toggle_spec.js
@@ -0,0 +1,157 @@
+import CommentTypeToggle from '~/comment_type_toggle';
+import * as dropLabSrc from '~/droplab/drop_lab';
+import InputSetter from '~/droplab/plugins/input_setter';
+
+describe('CommentTypeToggle', function () {
+ describe('class constructor', function () {
+ beforeEach(function () {
+ this.dropdownTrigger = {};
+ this.dropdownList = {};
+ this.noteTypeInput = {};
+ this.submitButton = {};
+ this.closeButton = {};
+
+ this.commentTypeToggle = new CommentTypeToggle({
+ dropdownTrigger: this.dropdownTrigger,
+ dropdownList: this.dropdownList,
+ noteTypeInput: this.noteTypeInput,
+ submitButton: this.submitButton,
+ closeButton: this.closeButton,
+ });
+ });
+
+ it('should set .dropdownTrigger', function () {
+ expect(this.commentTypeToggle.dropdownTrigger).toBe(this.dropdownTrigger);
+ });
+
+ it('should set .dropdownList', function () {
+ expect(this.commentTypeToggle.dropdownList).toBe(this.dropdownList);
+ });
+
+ it('should set .noteTypeInput', function () {
+ expect(this.commentTypeToggle.noteTypeInput).toBe(this.noteTypeInput);
+ });
+
+ it('should set .submitButton', function () {
+ expect(this.commentTypeToggle.submitButton).toBe(this.submitButton);
+ });
+
+ it('should set .closeButton', function () {
+ expect(this.commentTypeToggle.closeButton).toBe(this.closeButton);
+ });
+
+ it('should set .reopenButton', function () {
+ expect(this.commentTypeToggle.reopenButton).toBe(this.reopenButton);
+ });
+ });
+
+ describe('initDroplab', function () {
+ beforeEach(function () {
+ this.commentTypeToggle = {
+ dropdownTrigger: {},
+ dropdownList: {},
+ noteTypeInput: {},
+ submitButton: {},
+ closeButton: {},
+ setConfig: () => {},
+ };
+ this.config = {};
+
+ this.droplab = jasmine.createSpyObj('droplab', ['init']);
+
+ spyOn(dropLabSrc, 'default').and.returnValue(this.droplab);
+ spyOn(this.commentTypeToggle, 'setConfig').and.returnValue(this.config);
+
+ CommentTypeToggle.prototype.initDroplab.call(this.commentTypeToggle);
+ });
+
+ it('should instantiate a DropLab instance', function () {
+ expect(dropLabSrc.default).toHaveBeenCalled();
+ });
+
+ it('should set .droplab', function () {
+ expect(this.commentTypeToggle.droplab).toBe(this.droplab);
+ });
+
+ it('should call .setConfig', function () {
+ expect(this.commentTypeToggle.setConfig).toHaveBeenCalled();
+ });
+
+ it('should call DropLab.prototype.init', function () {
+ expect(this.droplab.init).toHaveBeenCalledWith(
+ this.commentTypeToggle.dropdownTrigger,
+ this.commentTypeToggle.dropdownList,
+ [InputSetter],
+ this.config,
+ );
+ });
+ });
+
+ describe('setConfig', function () {
+ describe('if no .closeButton is provided', function () {
+ beforeEach(function () {
+ this.commentTypeToggle = {
+ dropdownTrigger: {},
+ dropdownList: {},
+ noteTypeInput: {},
+ submitButton: {},
+ reopenButton: {},
+ };
+
+ this.setConfig = CommentTypeToggle.prototype.setConfig.call(this.commentTypeToggle);
+ });
+
+ it('should not add .closeButton related InputSetter config', function () {
+ expect(this.setConfig).toEqual({
+ InputSetter: [{
+ input: this.commentTypeToggle.noteTypeInput,
+ valueAttribute: 'data-value',
+ }, {
+ input: this.commentTypeToggle.submitButton,
+ valueAttribute: 'data-submit-text',
+ }, {
+ input: this.commentTypeToggle.reopenButton,
+ valueAttribute: 'data-reopen-text',
+ }, {
+ input: this.commentTypeToggle.reopenButton,
+ valueAttribute: 'data-reopen-text',
+ inputAttribute: 'data-alternative-text',
+ }],
+ });
+ });
+ });
+
+ describe('if no .reopenButton is provided', function () {
+ beforeEach(function () {
+ this.commentTypeToggle = {
+ dropdownTrigger: {},
+ dropdownList: {},
+ noteTypeInput: {},
+ submitButton: {},
+ closeButton: {},
+ };
+
+ this.setConfig = CommentTypeToggle.prototype.setConfig.call(this.commentTypeToggle);
+ });
+
+ it('should not add .reopenButton related InputSetter config', function () {
+ expect(this.setConfig).toEqual({
+ InputSetter: [{
+ input: this.commentTypeToggle.noteTypeInput,
+ valueAttribute: 'data-value',
+ }, {
+ input: this.commentTypeToggle.submitButton,
+ valueAttribute: 'data-submit-text',
+ }, {
+ input: this.commentTypeToggle.closeButton,
+ valueAttribute: 'data-close-text',
+ }, {
+ input: this.commentTypeToggle.closeButton,
+ valueAttribute: 'data-close-text',
+ inputAttribute: 'data-alternative-text',
+ }],
+ });
+ });
+ });
+ });
+});
diff --git a/spec/javascripts/droplab/drop_down_spec.js b/spec/javascripts/droplab/drop_down_spec.js
index bbf953658c8..802e2435672 100644
--- a/spec/javascripts/droplab/drop_down_spec.js
+++ b/spec/javascripts/droplab/drop_down_spec.js
@@ -130,7 +130,7 @@ describe('DropDown', function () {
beforeEach(function () {
this.list = { dispatchEvent: () => {} };
this.dropdown = { hide: () => {}, list: this.list, addSelectedClass: () => {} };
- this.event = { preventDefault: () => {}, target: 'target' };
+ this.event = { preventDefault: () => {}, target: {} };
this.customEvent = {};
this.closestElement = {};
@@ -168,6 +168,21 @@ describe('DropDown', function () {
expect(this.list.dispatchEvent).toHaveBeenCalledWith(this.customEvent);
});
+ describe('if the target is a UL element', function () {
+ beforeEach(function () {
+ this.event = { preventDefault: () => {}, target: { tagName: 'UL' } };
+
+ spyOn(this.event, 'preventDefault');
+ utils.closest.calls.reset();
+
+ DropDown.prototype.clickEvent.call(this.dropdown, this.event);
+ });
+
+ it('should return immediately', function () {
+ expect(utils.closest).not.toHaveBeenCalled();
+ });
+ });
+
describe('if no selected element exists', function () {
beforeEach(function () {
this.event.preventDefault.calls.reset();
diff --git a/spec/javascripts/droplab/plugins/input_setter_spec.js b/spec/javascripts/droplab/plugins/input_setter_spec.js
index c9b7b2b23dc..bd625f4ae80 100644
--- a/spec/javascripts/droplab/plugins/input_setter_spec.js
+++ b/spec/javascripts/droplab/plugins/input_setter_spec.js
@@ -140,22 +140,6 @@ describe('InputSetter', function () {
expect(this.input.value).toBe(this.newValue);
});
- describe('if there is no newValue', function () {
- beforeEach(function () {
- this.newValue = '';
- this.inputSetter = { hook: { trigger: {} } };
- this.config = { valueAttribute: {}, input: this.input };
- this.input = { value: 'oldValue', tagName: 'INPUT' };
- this.selectedItem = { getAttribute: () => {} };
-
- InputSetter.setInput.call(this.inputSetter, this.config, this.selectedItem);
- });
-
- it('should not set the value of the input', function () {
- expect(this.input.value).toBe('oldValue');
- })
- });
-
describe('if no config.input is provided', function () {
beforeEach(function () {
this.config = { valueAttribute: {} };
@@ -181,24 +165,6 @@ describe('InputSetter', function () {
it('should set the textContent of the input', function () {
expect(this.input.textContent).toBe(this.newValue);
});
-
- describe('if there is no new value', function () {
- beforeEach(function () {
- this.selectedItem = { getAttribute: () => {} };
- this.input = { textContent: 'oldValue', tagName: 'INPUT', hasAttribute: () => {} };
- this.config = { valueAttribute: {}, input: this.input };
- this.inputSetter = { hook: { trigger: {} } };
- this.newValue = 'newValue';
-
- spyOn(this.selectedItem, 'getAttribute').and.returnValue(this.newValue);
-
- InputSetter.setInput.call(this.inputSetter, this.config, this.selectedItem);
- });
-
- it('should not set the value of the input', function () {
- expect(this.input.textContent).toBe('oldValue');
- });
- });
});
describe('if there is an inputAttribute', function () {
diff --git a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
index b300feaabe1..3f79eaf7afb 100644
--- a/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
+++ b/spec/lib/gitlab/email/handler/create_note_handler_spec.rb
@@ -143,6 +143,7 @@ describe Gitlab::Email::Handler::CreateNoteHandler, lib: true do
expect(new_note.author).to eq(sent_notification.recipient)
expect(new_note.position).to eq(note.position)
expect(new_note.note).to include("I could not disagree more.")
+ expect(new_note.in_reply_to?(note)).to be_truthy
end
it "adds all attachments" do
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 6a89b007f96..e6f0a3b5920 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -63,7 +63,7 @@ describe Notify do
it 'contains a link to note author' do
is_expected.to have_html_escaped_body_text(issue.author_name)
- is_expected.to have_body_text 'wrote:'
+ is_expected.to have_body_text 'created an issue:'
end
end
end
@@ -215,7 +215,7 @@ describe Notify do
it 'contains a link to note author' do
is_expected.to have_html_escaped_body_text merge_request.author_name
- is_expected.to have_body_text 'wrote:'
+ is_expected.to have_body_text 'created a merge request:'
end
end
end
@@ -554,7 +554,7 @@ describe Notify do
end
it 'does not contain note author' do
- is_expected.not_to have_body_text 'wrote:'
+ is_expected.not_to have_body_text note.author_name
end
context 'when enabled email_author_in_body' do
@@ -564,7 +564,6 @@ describe Notify do
it 'contains a link to note author' do
is_expected.to have_html_escaped_body_text note.author_name
- is_expected.to have_body_text 'wrote:'
end
end
end
@@ -637,7 +636,7 @@ describe Notify do
end
end
- context 'items that are noteable, emails for a note on a diff' do
+ context 'items that are noteable, the email for a discussion note' do
let(:project) { create(:project, :repository) }
let(:note_author) { create(:user, name: 'author_name') }
@@ -645,8 +644,118 @@ describe Notify do
allow(Note).to receive(:find).with(note.id).and_return(note)
end
- shared_examples 'a note email on a diff' do |model|
- let(:note) { create(model, project: project, author: note_author) }
+ shared_examples 'a discussion note email' do |model|
+ it_behaves_like 'it should have Gmail Actions links'
+
+ it 'is sent to the given recipient as the author' do
+ sender = subject.header[:from].addrs[0]
+
+ aggregate_failures do
+ expect(sender.display_name).to eq(note_author.name)
+ expect(sender.address).to eq(gitlab_sender)
+ expect(subject).to deliver_to(recipient.notification_email)
+ end
+ end
+
+ it 'contains the message from the note' do
+ is_expected.to have_body_text note.note
+ end
+
+ it 'contains an introduction' do
+ is_expected.to have_body_text 'started a new discussion'
+ end
+
+ context 'when a comment on an existing discussion' do
+ let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) }
+
+ it 'contains an introduction' do
+ is_expected.to have_body_text 'commented on a'
+ end
+ end
+ end
+
+ describe 'on a commit' do
+ let(:commit) { project.commit }
+ let(:note) { create(:discussion_note_on_commit, commit_id: commit.id, project: project, author: note_author) }
+
+ before(:each) { allow(note).to receive(:noteable).and_return(commit) }
+
+ subject { Notify.note_commit_email(recipient.id, note.id) }
+
+ it_behaves_like 'a discussion note email', :discussion_note_on_commit
+ it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+ let(:model) { commit }
+ end
+ it_behaves_like 'it should show Gmail Actions View Commit link'
+ it_behaves_like 'a user cannot unsubscribe through footer link'
+
+ it 'has the correct subject' do
+ is_expected.to have_subject "Re: #{project.name} | #{commit.title.strip} (#{commit.short_id})"
+ end
+
+ it 'contains a link to the commit' do
+ is_expected.to have_body_text commit.short_id
+ end
+ end
+
+ describe 'on a merge request' do
+ let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
+ let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, author: note_author) }
+ let(:note_on_merge_request_path) { namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: "note_#{note.id}") }
+ before(:each) { allow(note).to receive(:noteable).and_return(merge_request) }
+
+ subject { Notify.note_merge_request_email(recipient.id, note.id) }
+
+ it_behaves_like 'a discussion note email', :discussion_note_on_merge_request
+ it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+ let(:model) { merge_request }
+ end
+ it_behaves_like 'it should show Gmail Actions View Merge request link'
+ it_behaves_like 'an unsubscribeable thread'
+
+ it 'has the correct subject' do
+ is_expected.to have_referable_subject(merge_request, reply: true)
+ end
+
+ it 'contains a link to the merge request note' do
+ is_expected.to have_body_text note_on_merge_request_path
+ end
+ end
+
+ describe 'on an issue' do
+ let(:issue) { create(:issue, project: project) }
+ let(:note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: note_author) }
+ let(:note_on_issue_path) { namespace_project_issue_path(project.namespace, project, issue, anchor: "note_#{note.id}") }
+ before(:each) { allow(note).to receive(:noteable).and_return(issue) }
+
+ subject { Notify.note_issue_email(recipient.id, note.id) }
+
+ it_behaves_like 'a discussion note email', :discussion_note_on_issue
+ it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do
+ let(:model) { issue }
+ end
+ it_behaves_like 'it should show Gmail Actions View Issue link'
+ it_behaves_like 'an unsubscribeable thread'
+
+ it 'has the correct subject' do
+ is_expected.to have_referable_subject(issue, reply: true)
+ end
+
+ it 'contains a link to the issue note' do
+ is_expected.to have_body_text note_on_issue_path
+ end
+ end
+ end
+
+ context 'items that are noteable, the email for a diff discussion note' do
+ let(:note_author) { create(:user, name: 'author_name') }
+
+ before :each do
+ allow(Note).to receive(:find).with(note.id).and_return(note)
+ end
+
+ shared_examples 'an email for a note on a diff discussion' do |model|
+ let(:note) { create(model, author: note_author) }
it "includes diffs with character-level highlighting" do
is_expected.to have_body_text '<span class="p">}</span></span>'
@@ -672,18 +781,15 @@ describe Notify do
is_expected.to have_html_escaped_body_text note.note
end
- it 'does not contain note author' do
- is_expected.not_to have_body_text 'wrote:'
+ it 'contains an introduction' do
+ is_expected.to have_body_text 'started a new discussion on'
end
- context 'when enabled email_author_in_body' do
- before do
- stub_application_setting(email_author_in_body: true)
- end
+ context 'when a comment on an existing discussion' do
+ let!(:second_note) { create(model, author: note_author, noteable: nil, in_reply_to: note) }
- it 'contains a link to note author' do
- is_expected.to have_html_escaped_body_text note.author_name
- is_expected.to have_body_text 'wrote:'
+ it 'contains an introduction' do
+ is_expected.to have_body_text 'commented on a discussion on'
end
end
end
@@ -694,7 +800,7 @@ describe Notify do
subject { Notify.note_commit_email(recipient.id, note.id) }
- it_behaves_like 'a note email on a diff', :diff_note_on_commit
+ it_behaves_like 'an email for a note on a diff discussion', :diff_note_on_commit
it_behaves_like 'it should show Gmail Actions View Commit link'
it_behaves_like 'a user cannot unsubscribe through footer link'
end
@@ -705,7 +811,7 @@ describe Notify do
subject { Notify.note_merge_request_email(recipient.id, note.id) }
- it_behaves_like 'a note email on a diff', :diff_note_on_merge_request
+ it_behaves_like 'an email for a note on a diff discussion', :diff_note_on_merge_request
it_behaves_like 'it should show Gmail Actions View Merge request link'
it_behaves_like 'an unsubscribeable thread'
end
diff --git a/spec/mailers/previews/notify_preview.rb b/spec/mailers/previews/notify_preview.rb
index 0e1ccb5b847..580f0d56a92 100644
--- a/spec/mailers/previews/notify_preview.rb
+++ b/spec/mailers/previews/notify_preview.rb
@@ -1,4 +1,100 @@
class NotifyPreview < ActionMailer::Preview
+ def note_merge_request_email_for_individual_note
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is an individual note on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - The note contents (that's what you're looking at)
+ - A link to view this note on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, note: note)
+ end
+ end
+
+ def note_merge_request_email_for_discussion
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is a new discussion on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - A line saying who started this discussion
+ - The note contents (that's what you're looking at)
+ - A link to view this discussion on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiscussionNote', note: note)
+ end
+ end
+
+ def note_merge_request_email_for_diff_discussion
+ note_email(:note_merge_request_email) do
+ note = <<-MD.strip_heredoc
+ This is a new discussion on a merge request :smiley:
+
+ In this notification email, we expect to see:
+
+ - A line saying who started this discussion and on what file
+ - The diff
+ - The note contents (that's what you're looking at)
+ - A link to view this discussion on Gitlab
+ - An explanation for why the user is receiving this notification
+ MD
+
+ position = Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: nil,
+ new_line: 14,
+ diff_refs: merge_request.diff_refs
+ )
+
+ create_note(noteable_type: 'merge_request', noteable_id: merge_request.id, type: 'DiffNote', position: position, note: note)
+ end
+ end
+
+ private
+
+ def project
+ @project ||= Project.find_by_full_path('gitlab-org/gitlab-test')
+ end
+
+ def merge_request
+ @merge_request ||= project.merge_requests.find_by(source_branch: 'master', target_branch: 'feature')
+ end
+
+ def user
+ @user ||= User.last
+ end
+
+ def create_note(params)
+ Notes::CreateService.new(project, user, params).execute
+ end
+
+ def note_email(method)
+ cleanup do
+ note = yield
+
+ Notify.public_send(method, user.id, note)
+ end
+ end
+
+ def cleanup
+ email = nil
+
+ ActiveRecord::Base.transaction do
+ email = yield
+ raise ActiveRecord::Rollback
+ end
+
+ email
+ end
+
def pipeline_success_email
pipeline = Ci::Pipeline.last
Notify.pipeline_success_email(pipeline, pipeline.user.try(:email))
diff --git a/spec/models/concerns/discussion_on_diff_spec.rb b/spec/models/concerns/discussion_on_diff_spec.rb
new file mode 100644
index 00000000000..0002a00770f
--- /dev/null
+++ b/spec/models/concerns/discussion_on_diff_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe DiffDiscussion, DiscussionOnDiff, model: true do
+ subject { create(:diff_note_on_merge_request).to_discussion }
+
+ describe "#truncated_diff_lines" do
+ let(:truncated_lines) { subject.truncated_diff_lines }
+
+ context "when diff is greater than allowed number of truncated diff lines " do
+ it "returns fewer lines" do
+ expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+
+ expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
+ end
+ end
+
+ context "when some diff lines are meta" do
+ it "returns no meta lines" do
+ expect(subject.diff_lines).to include(be_meta)
+ expect(truncated_lines).not_to include(be_meta)
+ end
+ end
+ end
+end
diff --git a/spec/models/concerns/noteable_spec.rb b/spec/models/concerns/noteable_spec.rb
new file mode 100644
index 00000000000..92cc8859a8c
--- /dev/null
+++ b/spec/models/concerns/noteable_spec.rb
@@ -0,0 +1,261 @@
+require 'spec_helper'
+
+describe MergeRequest, Noteable, model: true do
+ let!(:active_diff_note1) { create(:diff_note_on_merge_request) }
+ let(:project) { active_diff_note1.project }
+ subject { active_diff_note1.noteable }
+ let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: active_diff_note1) }
+ let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: active_position2) }
+ let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: subject, position: outdated_position) }
+ let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: subject, in_reply_to: outdated_diff_note1) }
+ let!(:discussion_note1) { create(:discussion_note_on_merge_request, project: project, noteable: subject) }
+ let!(:discussion_note2) { create(:discussion_note_on_merge_request, in_reply_to: discussion_note1) }
+ let!(:commit_diff_note1) { create(:diff_note_on_commit, project: project) }
+ let!(:commit_diff_note2) { create(:diff_note_on_commit, project: project, in_reply_to: commit_diff_note1) }
+ let!(:commit_note1) { create(:note_on_commit, project: project) }
+ let!(:commit_note2) { create(:note_on_commit, project: project) }
+ let!(:commit_discussion_note1) { create(:discussion_note_on_commit, project: project) }
+ let!(:commit_discussion_note2) { create(:discussion_note_on_commit, in_reply_to: commit_discussion_note1) }
+ let!(:commit_discussion_note3) { create(:discussion_note_on_commit, project: project) }
+ let!(:note1) { create(:note, project: project, noteable: subject) }
+ let!(:note2) { create(:note, project: project, noteable: subject) }
+
+ let(:active_position2) do
+ Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: 16,
+ new_line: 22,
+ diff_refs: subject.diff_refs
+ )
+ end
+
+ let(:outdated_position) do
+ Gitlab::Diff::Position.new(
+ old_path: "files/ruby/popen.rb",
+ new_path: "files/ruby/popen.rb",
+ old_line: nil,
+ new_line: 9,
+ diff_refs: project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs
+ )
+ end
+
+ describe '#discussions' do
+ let(:discussions) { subject.discussions }
+
+ it 'includes discussions for diff notes, commit diff notes, commit notes, and regular notes' do
+ expect(discussions).to eq([
+ DiffDiscussion.new([active_diff_note1, active_diff_note2], subject),
+ DiffDiscussion.new([active_diff_note3], subject),
+ DiffDiscussion.new([outdated_diff_note1, outdated_diff_note2], subject),
+ Discussion.new([discussion_note1, discussion_note2], subject),
+ DiffDiscussion.new([commit_diff_note1, commit_diff_note2], subject),
+ OutOfContextDiscussion.new([commit_note1, commit_note2], subject),
+ Discussion.new([commit_discussion_note1, commit_discussion_note2], subject),
+ Discussion.new([commit_discussion_note3], subject),
+ IndividualNoteDiscussion.new([note1], subject),
+ IndividualNoteDiscussion.new([note2], subject)
+ ])
+ end
+ end
+
+ describe '#grouped_diff_discussions' do
+ let(:grouped_diff_discussions) { subject.grouped_diff_discussions }
+
+ it "includes active discussions" do
+ discussions = grouped_diff_discussions.values.flatten
+
+ expect(discussions.count).to eq(2)
+ expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
+ expect(discussions.all?(&:active?)).to be true
+
+ expect(discussions.first.notes).to eq([active_diff_note1, active_diff_note2])
+ expect(discussions.last.notes).to eq([active_diff_note3])
+ end
+
+ it "doesn't include outdated discussions" do
+ expect(grouped_diff_discussions.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+ end
+
+ it "groups the discussions by line code" do
+ expect(grouped_diff_discussions[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
+ expect(grouped_diff_discussions[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
+ end
+ end
+
+ context "discussion status" do
+ let(:first_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+ let(:second_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+ let(:third_discussion) { build_stubbed(:discussion_note_on_merge_request, noteable: subject, project: project).to_discussion }
+
+ before do
+ allow(subject).to receive(:resolvable_discussions).and_return([first_discussion, second_discussion, third_discussion])
+ end
+
+ describe "#discussions_resolvable?" do
+ context "when all discussions are unresolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(false)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolvable?).to be false
+ end
+ end
+
+ context "when some discussions are unresolvable and some discussions are resolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolvable?).to be true
+ end
+ end
+
+ context "when all discussions are resolvable" do
+ before do
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(true)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolvable?).to be true
+ end
+ end
+ end
+
+ describe "#discussions_resolved?" do
+ context "when discussions are not resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolved?).to be false
+ end
+ end
+
+ context "when discussions are resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable discussions are resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_resolved?).to be true
+ end
+ end
+
+ context "when some resolvable discussions are not resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_resolved?).to be false
+ end
+ end
+ end
+ end
+
+ describe "#discussions_to_be_resolved?" do
+ context "when discussions are not resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_to_be_resolved?).to be false
+ end
+ end
+
+ context "when discussions are resolvable" do
+ before do
+ allow(subject).to receive(:discussions_resolvable?).and_return(true)
+
+ allow(first_discussion).to receive(:resolvable?).and_return(true)
+ allow(second_discussion).to receive(:resolvable?).and_return(false)
+ allow(third_discussion).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable discussions are resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns false" do
+ expect(subject.discussions_to_be_resolved?).to be false
+ end
+ end
+
+ context "when some resolvable discussions are not resolved" do
+ before do
+ allow(first_discussion).to receive(:resolved?).and_return(true)
+ allow(third_discussion).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns true" do
+ expect(subject.discussions_to_be_resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#discussions_to_be_resolved" do
+ before do
+ allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
+ allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
+ allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
+ end
+
+ it 'includes only discussions that need to be resolved' do
+ expect(subject.discussions_to_be_resolved).to eq([first_discussion])
+ end
+ end
+
+ describe '#discussions_can_be_resolved_by?' do
+ let(:user) { build(:user) }
+
+ context 'all discussions can be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
+ end
+ end
+
+ context 'one discussion cannot be resolved by the user' do
+ before do
+ allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
+ allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
+ end
+
+ it 'allows a user to resolve the discussions' do
+ expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
+ end
+ end
+ end
+ end
+end
diff --git a/spec/models/concerns/resolvable_discussion_spec.rb b/spec/models/concerns/resolvable_discussion_spec.rb
new file mode 100644
index 00000000000..18327fe262d
--- /dev/null
+++ b/spec/models/concerns/resolvable_discussion_spec.rb
@@ -0,0 +1,548 @@
+require 'spec_helper'
+
+describe Discussion, ResolvableDiscussion, models: true do
+ subject { described_class.new([first_note, second_note, third_note]) }
+
+ let(:first_note) { create(:discussion_note_on_merge_request) }
+ let(:merge_request) { first_note.noteable }
+ let(:project) { first_note.project }
+ let(:second_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+ let(:third_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+
+ describe "#resolvable?" do
+ context "when potentially resolvable" do
+ before do
+ allow(subject).to receive(:potentially_resolvable?).and_return(true)
+ end
+
+ context "when all notes are unresolvable" do
+ before do
+ allow(first_note).to receive(:resolvable?).and_return(false)
+ allow(second_note).to receive(:resolvable?).and_return(false)
+ allow(third_note).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.resolvable?).to be false
+ end
+ end
+
+ context "when some notes are unresolvable and some notes are resolvable" do
+ before do
+ allow(first_note).to receive(:resolvable?).and_return(true)
+ allow(second_note).to receive(:resolvable?).and_return(false)
+ allow(third_note).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.resolvable?).to be true
+ end
+ end
+
+ context "when all notes are resolvable" do
+ before do
+ allow(first_note).to receive(:resolvable?).and_return(true)
+ allow(second_note).to receive(:resolvable?).and_return(true)
+ allow(third_note).to receive(:resolvable?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.resolvable?).to be true
+ end
+ end
+ end
+
+ context "when not potentially resolvable" do
+ before do
+ allow(subject).to receive(:potentially_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.resolvable?).to be false
+ end
+ end
+ end
+
+ describe "#resolved?" do
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.resolved?).to be false
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+
+ allow(first_note).to receive(:resolvable?).and_return(true)
+ allow(second_note).to receive(:resolvable?).and_return(false)
+ allow(third_note).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable notes are resolved" do
+ before do
+ allow(first_note).to receive(:resolved?).and_return(true)
+ allow(third_note).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns true" do
+ expect(subject.resolved?).to be true
+ end
+ end
+
+ context "when some resolvable notes are not resolved" do
+ before do
+ allow(first_note).to receive(:resolved?).and_return(true)
+ allow(third_note).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.resolved?).to be false
+ end
+ end
+ end
+ end
+
+ describe "#to_be_resolved?" do
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.to_be_resolved?).to be false
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+
+ allow(first_note).to receive(:resolvable?).and_return(true)
+ allow(second_note).to receive(:resolvable?).and_return(false)
+ allow(third_note).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable notes are resolved" do
+ before do
+ allow(first_note).to receive(:resolved?).and_return(true)
+ allow(third_note).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns false" do
+ expect(subject.to_be_resolved?).to be false
+ end
+ end
+
+ context "when some resolvable notes are not resolved" do
+ before do
+ allow(first_note).to receive(:resolved?).and_return(true)
+ allow(third_note).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns true" do
+ expect(subject.to_be_resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#can_resolve?" do
+ let(:current_user) { create(:user) }
+
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.can_resolve?(current_user)).to be false
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when not signed in" do
+ let(:current_user) { nil }
+
+ it "returns false" do
+ expect(subject.can_resolve?(current_user)).to be false
+ end
+ end
+
+ context "when signed in" do
+ context "when the signed in user is the noteable author" do
+ before do
+ subject.noteable.author = current_user
+ end
+
+ it "returns true" do
+ expect(subject.can_resolve?(current_user)).to be true
+ end
+ end
+
+ context "when the signed in user can push to the project" do
+ before do
+ subject.project.team << [current_user, :master]
+ end
+
+ it "returns true" do
+ expect(subject.can_resolve?(current_user)).to be true
+ end
+ end
+
+ context "when the signed in user is a random user" do
+ it "returns false" do
+ expect(subject.can_resolve?(current_user)).to be false
+ end
+ end
+ end
+ end
+ end
+
+ describe "#resolve!" do
+ let(:current_user) { create(:user) }
+
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns nil" do
+ expect(subject.resolve!(current_user)).to be_nil
+ end
+
+ it "doesn't set resolved_at" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_at).to be_nil
+ end
+
+ it "doesn't set resolved_by" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_by).to be_nil
+ end
+
+ it "doesn't mark as resolved" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved?).to be false
+ end
+ end
+
+ context "when resolvable" do
+ let(:user) { create(:user) }
+ let(:second_note) { create(:diff_note_on_commit) } # unresolvable
+
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable notes are resolved" do
+ before do
+ first_note.resolve!(user)
+ third_note.resolve!(user)
+
+ first_note.reload
+ third_note.reload
+ end
+
+ it "doesn't change resolved_at on the resolved notes" do
+ expect(first_note.resolved_at).not_to be_nil
+ expect(third_note.resolved_at).not_to be_nil
+
+ expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at }
+ expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_at }
+ end
+
+ it "doesn't change resolved_by on the resolved notes" do
+ expect(first_note.resolved_by).to eq(user)
+ expect(third_note.resolved_by).to eq(user)
+
+ expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by }
+ expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_by }
+ end
+
+ it "doesn't change the resolved state on the resolved notes" do
+ expect(first_note.resolved?).to be true
+ expect(third_note.resolved?).to be true
+
+ expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? }
+ expect { subject.resolve!(current_user) }.not_to change { third_note.resolved? }
+ end
+
+ it "doesn't change resolved_at" do
+ expect(subject.resolved_at).not_to be_nil
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at }
+ end
+
+ it "doesn't change resolved_by" do
+ expect(subject.resolved_by).to eq(user)
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by }
+ end
+
+ it "doesn't change resolved state" do
+ expect(subject.resolved?).to be true
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved? }
+ end
+ end
+
+ context "when some resolvable notes are resolved" do
+ before do
+ first_note.resolve!(user)
+ end
+
+ it "doesn't change resolved_at on the resolved note" do
+ expect(first_note.resolved_at).not_to be_nil
+
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload.resolved_at }
+ end
+
+ it "doesn't change resolved_by on the resolved note" do
+ expect(first_note.resolved_by).to eq(user)
+
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload && first_note.resolved_by }
+ end
+
+ it "doesn't change the resolved state on the resolved note" do
+ expect(first_note.resolved?).to be true
+
+ expect { subject.resolve!(current_user) }.
+ not_to change { first_note.reload && first_note.resolved? }
+ end
+
+ it "sets resolved_at on the unresolved note" do
+ subject.resolve!(current_user)
+ third_note.reload
+
+ expect(third_note.resolved_at).not_to be_nil
+ end
+
+ it "sets resolved_by on the unresolved note" do
+ subject.resolve!(current_user)
+ third_note.reload
+
+ expect(third_note.resolved_by).to eq(current_user)
+ end
+
+ it "marks the unresolved note as resolved" do
+ subject.resolve!(current_user)
+ third_note.reload
+
+ expect(third_note.resolved?).to be true
+ end
+
+ it "sets resolved_at" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_at).not_to be_nil
+ end
+
+ it "sets resolved_by" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_by).to eq(current_user)
+ end
+
+ it "marks as resolved" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved?).to be true
+ end
+ end
+
+ context "when no resolvable notes are resolved" do
+ it "sets resolved_at on the unresolved notes" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved_at).not_to be_nil
+ expect(third_note.resolved_at).not_to be_nil
+ end
+
+ it "sets resolved_by on the unresolved notes" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved_by).to eq(current_user)
+ expect(third_note.resolved_by).to eq(current_user)
+ end
+
+ it "marks the unresolved notes as resolved" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved?).to be true
+ expect(third_note.resolved?).to be true
+ end
+
+ it "sets resolved_at" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(subject.resolved_at).not_to be_nil
+ end
+
+ it "sets resolved_by" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(subject.resolved_by).to eq(current_user)
+ end
+
+ it "marks as resolved" do
+ subject.resolve!(current_user)
+ first_note.reload
+ third_note.reload
+
+ expect(subject.resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#unresolve!" do
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns nil" do
+ expect(subject.unresolve!).to be_nil
+ end
+ end
+
+ context "when resolvable" do
+ let(:user) { create(:user) }
+
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+
+ allow(first_note).to receive(:resolvable?).and_return(true)
+ allow(second_note).to receive(:resolvable?).and_return(false)
+ allow(third_note).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when all resolvable notes are resolved" do
+ before do
+ first_note.resolve!(user)
+ third_note.resolve!(user)
+ end
+
+ it "unsets resolved_at on the resolved notes" do
+ subject.unresolve!
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved_at).to be_nil
+ expect(third_note.resolved_at).to be_nil
+ end
+
+ it "unsets resolved_by on the resolved notes" do
+ subject.unresolve!
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved_by).to be_nil
+ expect(third_note.resolved_by).to be_nil
+ end
+
+ it "unmarks the resolved notes as resolved" do
+ subject.unresolve!
+ first_note.reload
+ third_note.reload
+
+ expect(first_note.resolved?).to be false
+ expect(third_note.resolved?).to be false
+ end
+
+ it "unsets resolved_at" do
+ subject.unresolve!
+ first_note.reload
+ third_note.reload
+
+ expect(subject.resolved_at).to be_nil
+ end
+
+ it "unsets resolved_by" do
+ subject.unresolve!
+ first_note.reload
+ third_note.reload
+
+ expect(subject.resolved_by).to be_nil
+ end
+
+ it "unmarks as resolved" do
+ subject.unresolve!
+
+ expect(subject.resolved?).to be false
+ end
+ end
+
+ context "when some resolvable notes are resolved" do
+ before do
+ first_note.resolve!(user)
+ end
+
+ it "unsets resolved_at on the resolved note" do
+ subject.unresolve!
+
+ expect(subject.first_note.resolved_at).to be_nil
+ end
+
+ it "unsets resolved_by on the resolved note" do
+ subject.unresolve!
+
+ expect(subject.first_note.resolved_by).to be_nil
+ end
+
+ it "unmarks the resolved note as resolved" do
+ subject.unresolve!
+
+ expect(subject.first_note.resolved?).to be false
+ end
+ end
+ end
+ end
+
+ describe "#first_note_to_resolve" do
+ it "returns the first note that still needs to be resolved" do
+ allow(first_note).to receive(:to_be_resolved?).and_return(false)
+ allow(second_note).to receive(:to_be_resolved?).and_return(true)
+
+ expect(subject.first_note_to_resolve).to eq(second_note)
+ end
+ end
+
+ describe "#last_resolved_note" do
+ let(:current_user) { create(:user) }
+
+ before do
+ first_note.resolve!(current_user)
+ third_note.resolve!(current_user)
+ second_note.resolve!(current_user)
+ end
+
+ it "returns the last note that was resolved" do
+ expect(subject.last_resolved_note).to eq(second_note)
+ end
+ end
+end
diff --git a/spec/models/concerns/resolvable_note_spec.rb b/spec/models/concerns/resolvable_note_spec.rb
new file mode 100644
index 00000000000..1503ccdff11
--- /dev/null
+++ b/spec/models/concerns/resolvable_note_spec.rb
@@ -0,0 +1,329 @@
+require 'spec_helper'
+
+describe Note, ResolvableNote, models: true do
+ let(:project) { create(:project) }
+ let(:merge_request) { create(:merge_request, source_project: project) }
+ subject { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+
+ context 'resolvability scopes' do
+ let!(:note1) { create(:note, project: project) }
+ let!(:note2) { create(:diff_note_on_commit, project: project) }
+ let!(:note3) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+ let!(:note4) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+ let!(:note5) { create(:discussion_note_on_issue, project: project) }
+ let!(:note6) { create(:discussion_note_on_merge_request, :system, noteable: merge_request, project: project) }
+
+ describe '.potentially_resolvable' do
+ it 'includes diff and discussion notes on merge requests' do
+ expect(Note.potentially_resolvable).to match_array([note3, note4, note6])
+ end
+ end
+
+ describe '.resolvable' do
+ it 'includes non-system diff and discussion notes on merge requests' do
+ expect(Note.resolvable).to match_array([note3, note4])
+ end
+ end
+
+ describe '.resolved' do
+ it 'includes resolved non-system diff and discussion notes on merge requests' do
+ expect(Note.resolved).to match_array([note3])
+ end
+ end
+
+ describe '.unresolved' do
+ it 'includes non-resolved non-system diff and discussion notes on merge requests' do
+ expect(Note.unresolved).to match_array([note4])
+ end
+ end
+ end
+
+ describe ".resolve!" do
+ let(:current_user) { create(:user) }
+ let!(:commit_note) { create(:diff_note_on_commit, project: project) }
+ let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+ let!(:unresolved_note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) }
+
+ before do
+ described_class.resolve!(current_user)
+
+ commit_note.reload
+ resolved_note.reload
+ unresolved_note.reload
+ end
+
+ it 'resolves only the resolvable, not yet resolved notes' do
+ expect(commit_note.resolved_at).to be_nil
+ expect(resolved_note.resolved_by).not_to eq(current_user)
+ expect(unresolved_note.resolved_at).not_to be_nil
+ expect(unresolved_note.resolved_by).to eq(current_user)
+ end
+ end
+
+ describe ".unresolve!" do
+ let!(:resolved_note) { create(:discussion_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
+
+ before do
+ described_class.unresolve!
+
+ resolved_note.reload
+ end
+
+ it 'unresolves the resolved notes' do
+ expect(resolved_note.resolved_by).to be_nil
+ expect(resolved_note.resolved_at).to be_nil
+ end
+ end
+
+ describe '#resolvable?' do
+ context "when potentially resolvable" do
+ before do
+ allow(subject).to receive(:potentially_resolvable?).and_return(true)
+ end
+
+ context "when a system note" do
+ before do
+ subject.system = true
+ end
+
+ it "returns false" do
+ expect(subject.resolvable?).to be false
+ end
+ end
+
+ context "when a regular note" do
+ it "returns true" do
+ expect(subject.resolvable?).to be true
+ end
+ end
+ end
+
+ context "when not potentially resolvable" do
+ before do
+ allow(subject).to receive(:potentially_resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.resolvable?).to be false
+ end
+ end
+ end
+
+ describe "#to_be_resolved?" do
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns false" do
+ expect(subject.to_be_resolved?).to be false
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when resolved" do
+ before do
+ allow(subject).to receive(:resolved?).and_return(true)
+ end
+
+ it "returns false" do
+ expect(subject.to_be_resolved?).to be false
+ end
+ end
+
+ context "when not resolved" do
+ before do
+ allow(subject).to receive(:resolved?).and_return(false)
+ end
+
+ it "returns true" do
+ expect(subject.to_be_resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#resolved?" do
+ let(:current_user) { create(:user) }
+
+ context 'when not resolvable' do
+ before do
+ subject.resolve!(current_user)
+
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it 'returns false' do
+ expect(subject.resolved?).to be_falsey
+ end
+ end
+
+ context 'when resolvable' do
+ context 'when the note has been resolved' do
+ before do
+ subject.resolve!(current_user)
+ end
+
+ it 'returns true' do
+ expect(subject.resolved?).to be_truthy
+ end
+ end
+
+ context 'when the note has not been resolved' do
+ it 'returns false' do
+ expect(subject.resolved?).to be_falsey
+ end
+ end
+ end
+ end
+
+ describe "#resolve!" do
+ let(:current_user) { create(:user) }
+
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns nil" do
+ expect(subject.resolve!(current_user)).to be_nil
+ end
+
+ it "doesn't set resolved_at" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_at).to be_nil
+ end
+
+ it "doesn't set resolved_by" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_by).to be_nil
+ end
+
+ it "doesn't mark as resolved" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved?).to be false
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when already resolved" do
+ let(:user) { create(:user) }
+
+ before do
+ subject.resolve!(user)
+ end
+
+ it "returns nil" do
+ expect(subject.resolve!(current_user)).to be_nil
+ end
+
+ it "doesn't change resolved_at" do
+ expect(subject.resolved_at).not_to be_nil
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at }
+ end
+
+ it "doesn't change resolved_by" do
+ expect(subject.resolved_by).to eq(user)
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by }
+ end
+
+ it "doesn't change resolved status" do
+ expect(subject.resolved?).to be true
+
+ expect { subject.resolve!(current_user) }.not_to change { subject.resolved? }
+ end
+ end
+
+ context "when not yet resolved" do
+ it "returns true" do
+ expect(subject.resolve!(current_user)).to be true
+ end
+
+ it "sets resolved_at" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_at).not_to be_nil
+ end
+
+ it "sets resolved_by" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved_by).to eq(current_user)
+ end
+
+ it "marks as resolved" do
+ subject.resolve!(current_user)
+
+ expect(subject.resolved?).to be true
+ end
+ end
+ end
+ end
+
+ describe "#unresolve!" do
+ context "when not resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(false)
+ end
+
+ it "returns nil" do
+ expect(subject.unresolve!).to be_nil
+ end
+ end
+
+ context "when resolvable" do
+ before do
+ allow(subject).to receive(:resolvable?).and_return(true)
+ end
+
+ context "when resolved" do
+ let(:user) { create(:user) }
+
+ before do
+ subject.resolve!(user)
+ end
+
+ it "returns true" do
+ expect(subject.unresolve!).to be true
+ end
+
+ it "unsets resolved_at" do
+ subject.unresolve!
+
+ expect(subject.resolved_at).to be_nil
+ end
+
+ it "unsets resolved_by" do
+ subject.unresolve!
+
+ expect(subject.resolved_by).to be_nil
+ end
+
+ it "unmarks as resolved" do
+ subject.unresolve!
+
+ expect(subject.resolved?).to be false
+ end
+ end
+
+ context "when not resolved" do
+ it "returns nil" do
+ expect(subject.unresolve!).to be_nil
+ end
+ end
+ end
+ end
+end
diff --git a/spec/models/diff_discussion_spec.rb b/spec/models/diff_discussion_spec.rb
new file mode 100644
index 00000000000..48e7c0a822c
--- /dev/null
+++ b/spec/models/diff_discussion_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe DiffDiscussion, model: true do
+ subject { described_class.new([first_note, second_note, third_note]) }
+
+ let(:first_note) { create(:diff_note_on_merge_request) }
+ let(:merge_request) { first_note.noteable }
+ let(:project) { first_note.project }
+ let(:second_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+ let(:third_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project, in_reply_to: first_note) }
+
+ describe '#reply_attributes' do
+ it 'includes position and original_position' do
+ attributes = subject.reply_attributes
+ expect(attributes[:position]).to eq(first_note.position.to_json)
+ expect(attributes[:original_position]).to eq(first_note.original_position.to_json)
+ end
+ end
+end
diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb
index 9ea3a4b7020..fb80b74b226 100644
--- a/spec/models/diff_note_spec.rb
+++ b/spec/models/diff_note_spec.rb
@@ -31,43 +31,6 @@ describe DiffNote, models: true do
subject { create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) }
- describe ".resolve!" do
- let(:current_user) { create(:user) }
- let!(:commit_note) { create(:diff_note_on_commit) }
- let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
- let!(:unresolved_note) { create(:diff_note_on_merge_request) }
-
- before do
- described_class.resolve!(current_user)
-
- commit_note.reload
- resolved_note.reload
- unresolved_note.reload
- end
-
- it 'resolves only the resolvable, not yet resolved notes' do
- expect(commit_note.resolved_at).to be_nil
- expect(resolved_note.resolved_by).not_to eq(current_user)
- expect(unresolved_note.resolved_at).not_to be_nil
- expect(unresolved_note.resolved_by).to eq(current_user)
- end
- end
-
- describe ".unresolve!" do
- let!(:resolved_note) { create(:diff_note_on_merge_request, :resolved) }
-
- before do
- described_class.unresolve!
-
- resolved_note.reload
- end
-
- it 'unresolves the resolved notes' do
- expect(resolved_note.resolved_by).to be_nil
- expect(resolved_note.resolved_at).to be_nil
- end
- end
-
describe "#position=" do
context "when provided a string" do
it "sets the position" do
@@ -94,6 +57,32 @@ describe DiffNote, models: true do
end
end
+ describe "#original_position=" do
+ context "when provided a string" do
+ it "sets the original position" do
+ subject.original_position = new_position.to_json
+
+ expect(subject.original_position).to eq(new_position)
+ end
+ end
+
+ context "when provided a hash" do
+ it "sets the original position" do
+ subject.original_position = new_position.to_h
+
+ expect(subject.original_position).to eq(new_position)
+ end
+ end
+
+ context "when provided a position object" do
+ it "sets the original position" do
+ subject.original_position = new_position
+
+ expect(subject.original_position).to eq(new_position)
+ end
+ end
+ end
+
describe "#diff_file" do
it "returns the correct diff file" do
diff_file = subject.diff_file
@@ -226,252 +215,6 @@ describe DiffNote, models: true do
end
end
- describe "#resolvable?" do
- context "when noteable is a commit" do
- subject { create(:diff_note_on_commit, project: project, position: position) }
-
- it "returns false" do
- expect(subject.resolvable?).to be false
- end
- end
-
- context "when noteable is a merge request" do
- context "when a system note" do
- before do
- subject.system = true
- end
-
- it "returns false" do
- expect(subject.resolvable?).to be false
- end
- end
-
- context "when a regular note" do
- it "returns true" do
- expect(subject.resolvable?).to be true
- end
- end
- end
- end
-
- describe "#to_be_resolved?" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.to_be_resolved?).to be false
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when resolved" do
- before do
- allow(subject).to receive(:resolved?).and_return(true)
- end
-
- it "returns false" do
- expect(subject.to_be_resolved?).to be false
- end
- end
-
- context "when not resolved" do
- before do
- allow(subject).to receive(:resolved?).and_return(false)
- end
-
- it "returns true" do
- expect(subject.to_be_resolved?).to be true
- end
- end
- end
- end
-
- describe "#resolve!" do
- let(:current_user) { create(:user) }
-
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns nil" do
- expect(subject.resolve!(current_user)).to be_nil
- end
-
- it "doesn't set resolved_at" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_at).to be_nil
- end
-
- it "doesn't set resolved_by" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_by).to be_nil
- end
-
- it "doesn't mark as resolved" do
- subject.resolve!(current_user)
-
- expect(subject.resolved?).to be false
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when already resolved" do
- let(:user) { create(:user) }
-
- before do
- subject.resolve!(user)
- end
-
- it "returns nil" do
- expect(subject.resolve!(current_user)).to be_nil
- end
-
- it "doesn't change resolved_at" do
- expect(subject.resolved_at).not_to be_nil
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at }
- end
-
- it "doesn't change resolved_by" do
- expect(subject.resolved_by).to eq(user)
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by }
- end
-
- it "doesn't change resolved status" do
- expect(subject.resolved?).to be true
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved? }
- end
- end
-
- context "when not yet resolved" do
- it "returns true" do
- expect(subject.resolve!(current_user)).to be true
- end
-
- it "sets resolved_at" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_at).not_to be_nil
- end
-
- it "sets resolved_by" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_by).to eq(current_user)
- end
-
- it "marks as resolved" do
- subject.resolve!(current_user)
-
- expect(subject.resolved?).to be true
- end
- end
- end
- end
-
- describe "#unresolve!" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns nil" do
- expect(subject.unresolve!).to be_nil
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when resolved" do
- let(:user) { create(:user) }
-
- before do
- subject.resolve!(user)
- end
-
- it "returns true" do
- expect(subject.unresolve!).to be true
- end
-
- it "unsets resolved_at" do
- subject.unresolve!
-
- expect(subject.resolved_at).to be_nil
- end
-
- it "unsets resolved_by" do
- subject.unresolve!
-
- expect(subject.resolved_by).to be_nil
- end
-
- it "unmarks as resolved" do
- subject.unresolve!
-
- expect(subject.resolved?).to be false
- end
- end
-
- context "when not resolved" do
- it "returns nil" do
- expect(subject.unresolve!).to be_nil
- end
- end
- end
- end
-
- describe "#discussion" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns nil" do
- expect(subject.discussion).to be_nil
- end
- end
-
- context "when resolvable" do
- let!(:diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: subject.position) }
- let!(:diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) }
-
- let(:active_position2) do
- Gitlab::Diff::Position.new(
- old_path: "files/ruby/popen.rb",
- new_path: "files/ruby/popen.rb",
- old_line: 16,
- new_line: 22,
- diff_refs: merge_request.diff_refs
- )
- end
-
- it "returns the discussion this note is in" do
- discussion = subject.discussion
-
- expect(discussion.id).to eq(subject.discussion_id)
- expect(discussion.notes).to eq([subject, diff_note2])
- end
- end
- end
-
describe "#discussion_id" do
let(:note) { create(:diff_note_on_merge_request) }
@@ -496,29 +239,4 @@ describe DiffNote, models: true do
end
end
end
-
- describe "#original_discussion_id" do
- let(:note) { create(:diff_note_on_merge_request) }
-
- context "when it is newly created" do
- it "has a discussion id" do
- expect(note.original_discussion_id).not_to be_nil
- expect(note.original_discussion_id).to match(/\A\h{40}\z/)
- end
- end
-
- context "when it didn't store a discussion id before" do
- before do
- note.update_column(:original_discussion_id, nil)
- end
-
- it "has a discussion id" do
- # The original_discussion_id is set in `after_initialize`, so `reload` won't work
- reloaded_note = Note.find(note.id)
-
- expect(reloaded_note.original_discussion_id).not_to be_nil
- expect(reloaded_note.original_discussion_id).to match(/\A\h{40}\z/)
- end
- end
- end
end
diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb
index bc32fadd391..0221e23ced8 100644
--- a/spec/models/discussion_spec.rb
+++ b/spec/models/discussion_spec.rb
@@ -4,618 +4,27 @@ describe Discussion, model: true do
subject { described_class.new([first_note, second_note, third_note]) }
let(:first_note) { create(:diff_note_on_merge_request) }
- let(:second_note) { create(:diff_note_on_merge_request) }
+ let(:merge_request) { first_note.noteable }
+ let(:second_note) { create(:diff_note_on_merge_request, in_reply_to: first_note) }
let(:third_note) { create(:diff_note_on_merge_request) }
- describe "#resolvable?" do
- context "when a diff discussion" do
- before do
- allow(subject).to receive(:diff_discussion?).and_return(true)
- end
-
- context "when all notes are unresolvable" do
- before do
- allow(first_note).to receive(:resolvable?).and_return(false)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.resolvable?).to be false
- end
- end
-
- context "when some notes are unresolvable and some notes are resolvable" do
- before do
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.resolvable?).to be true
- end
- end
-
- context "when all notes are resolvable" do
- before do
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(true)
- allow(third_note).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.resolvable?).to be true
- end
- end
- end
-
- context "when not a diff discussion" do
- before do
- allow(subject).to receive(:diff_discussion?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.resolvable?).to be false
- end
- end
- end
-
- describe "#resolved?" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.resolved?).to be false
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
-
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable notes are resolved" do
- before do
- allow(first_note).to receive(:resolved?).and_return(true)
- allow(third_note).to receive(:resolved?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.resolved?).to be true
- end
- end
-
- context "when some resolvable notes are not resolved" do
- before do
- allow(first_note).to receive(:resolved?).and_return(true)
- allow(third_note).to receive(:resolved?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.resolved?).to be false
- end
- end
- end
- end
-
- describe "#to_be_resolved?" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.to_be_resolved?).to be false
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
-
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable notes are resolved" do
- before do
- allow(first_note).to receive(:resolved?).and_return(true)
- allow(third_note).to receive(:resolved?).and_return(true)
- end
-
- it "returns false" do
- expect(subject.to_be_resolved?).to be false
- end
- end
-
- context "when some resolvable notes are not resolved" do
- before do
- allow(first_note).to receive(:resolved?).and_return(true)
- allow(third_note).to receive(:resolved?).and_return(false)
- end
-
- it "returns true" do
- expect(subject.to_be_resolved?).to be true
- end
- end
- end
- end
-
- describe "#can_resolve?" do
- let(:current_user) { create(:user) }
-
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.can_resolve?(current_user)).to be false
- end
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when not signed in" do
- let(:current_user) { nil }
-
- it "returns false" do
- expect(subject.can_resolve?(current_user)).to be false
- end
- end
-
- context "when signed in" do
- context "when the signed in user is the noteable author" do
- before do
- subject.noteable.author = current_user
- end
-
- it "returns true" do
- expect(subject.can_resolve?(current_user)).to be true
- end
- end
-
- context "when the signed in user can push to the project" do
- before do
- subject.project.team << [current_user, :master]
- end
-
- it "returns true" do
- expect(subject.can_resolve?(current_user)).to be true
- end
- end
-
- context "when the signed in user is a random user" do
- it "returns false" do
- expect(subject.can_resolve?(current_user)).to be false
- end
- end
- end
- end
- end
-
- describe "#resolve!" do
- let(:current_user) { create(:user) }
-
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns nil" do
- expect(subject.resolve!(current_user)).to be_nil
- end
-
- it "doesn't set resolved_at" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_at).to be_nil
- end
-
- it "doesn't set resolved_by" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_by).to be_nil
- end
-
- it "doesn't mark as resolved" do
- subject.resolve!(current_user)
-
- expect(subject.resolved?).to be false
- end
- end
-
- context "when resolvable" do
- let(:user) { create(:user) }
- let(:second_note) { create(:diff_note_on_commit) } # unresolvable
-
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable notes are resolved" do
- before do
- first_note.resolve!(user)
- third_note.resolve!(user)
-
- first_note.reload
- third_note.reload
- end
-
- it "doesn't change resolved_at on the resolved notes" do
- expect(first_note.resolved_at).not_to be_nil
- expect(third_note.resolved_at).not_to be_nil
-
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_at }
- expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_at }
- end
-
- it "doesn't change resolved_by on the resolved notes" do
- expect(first_note.resolved_by).to eq(user)
- expect(third_note.resolved_by).to eq(user)
-
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved_by }
- expect { subject.resolve!(current_user) }.not_to change { third_note.resolved_by }
- end
-
- it "doesn't change the resolved state on the resolved notes" do
- expect(first_note.resolved?).to be true
- expect(third_note.resolved?).to be true
-
- expect { subject.resolve!(current_user) }.not_to change { first_note.resolved? }
- expect { subject.resolve!(current_user) }.not_to change { third_note.resolved? }
- end
-
- it "doesn't change resolved_at" do
- expect(subject.resolved_at).not_to be_nil
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved_at }
- end
-
- it "doesn't change resolved_by" do
- expect(subject.resolved_by).to eq(user)
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved_by }
- end
-
- it "doesn't change resolved state" do
- expect(subject.resolved?).to be true
-
- expect { subject.resolve!(current_user) }.not_to change { subject.resolved? }
- end
- end
-
- context "when some resolvable notes are resolved" do
- before do
- first_note.resolve!(user)
- end
-
- it "doesn't change resolved_at on the resolved note" do
- expect(first_note.resolved_at).not_to be_nil
-
- expect { subject.resolve!(current_user) }.
- not_to change { first_note.reload.resolved_at }
- end
-
- it "doesn't change resolved_by on the resolved note" do
- expect(first_note.resolved_by).to eq(user)
-
- expect { subject.resolve!(current_user) }.
- not_to change { first_note.reload && first_note.resolved_by }
- end
-
- it "doesn't change the resolved state on the resolved note" do
- expect(first_note.resolved?).to be true
-
- expect { subject.resolve!(current_user) }.
- not_to change { first_note.reload && first_note.resolved? }
- end
-
- it "sets resolved_at on the unresolved note" do
- subject.resolve!(current_user)
- third_note.reload
-
- expect(third_note.resolved_at).not_to be_nil
- end
-
- it "sets resolved_by on the unresolved note" do
- subject.resolve!(current_user)
- third_note.reload
-
- expect(third_note.resolved_by).to eq(current_user)
- end
-
- it "marks the unresolved note as resolved" do
- subject.resolve!(current_user)
- third_note.reload
-
- expect(third_note.resolved?).to be true
- end
-
- it "sets resolved_at" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_at).not_to be_nil
- end
-
- it "sets resolved_by" do
- subject.resolve!(current_user)
-
- expect(subject.resolved_by).to eq(current_user)
- end
-
- it "marks as resolved" do
- subject.resolve!(current_user)
-
- expect(subject.resolved?).to be true
- end
- end
-
- context "when no resolvable notes are resolved" do
- it "sets resolved_at on the unresolved notes" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved_at).not_to be_nil
- expect(third_note.resolved_at).not_to be_nil
- end
-
- it "sets resolved_by on the unresolved notes" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved_by).to eq(current_user)
- expect(third_note.resolved_by).to eq(current_user)
- end
-
- it "marks the unresolved notes as resolved" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved?).to be true
- expect(third_note.resolved?).to be true
- end
-
- it "sets resolved_at" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(subject.resolved_at).not_to be_nil
- end
-
- it "sets resolved_by" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(subject.resolved_by).to eq(current_user)
- end
-
- it "marks as resolved" do
- subject.resolve!(current_user)
- first_note.reload
- third_note.reload
-
- expect(subject.resolved?).to be true
- end
- end
- end
- end
-
- describe "#unresolve!" do
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- it "returns nil" do
- expect(subject.unresolve!).to be_nil
- end
- end
-
- context "when resolvable" do
- let(:user) { create(:user) }
-
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
-
- allow(first_note).to receive(:resolvable?).and_return(true)
- allow(second_note).to receive(:resolvable?).and_return(false)
- allow(third_note).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable notes are resolved" do
- before do
- first_note.resolve!(user)
- third_note.resolve!(user)
- end
-
- it "unsets resolved_at on the resolved notes" do
- subject.unresolve!
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved_at).to be_nil
- expect(third_note.resolved_at).to be_nil
- end
-
- it "unsets resolved_by on the resolved notes" do
- subject.unresolve!
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved_by).to be_nil
- expect(third_note.resolved_by).to be_nil
- end
-
- it "unmarks the resolved notes as resolved" do
- subject.unresolve!
- first_note.reload
- third_note.reload
-
- expect(first_note.resolved?).to be false
- expect(third_note.resolved?).to be false
- end
-
- it "unsets resolved_at" do
- subject.unresolve!
- first_note.reload
- third_note.reload
-
- expect(subject.resolved_at).to be_nil
- end
-
- it "unsets resolved_by" do
- subject.unresolve!
- first_note.reload
- third_note.reload
-
- expect(subject.resolved_by).to be_nil
- end
-
- it "unmarks as resolved" do
- subject.unresolve!
-
- expect(subject.resolved?).to be false
- end
- end
-
- context "when some resolvable notes are resolved" do
- before do
- first_note.resolve!(user)
- end
-
- it "unsets resolved_at on the resolved note" do
- subject.unresolve!
-
- expect(subject.first_note.resolved_at).to be_nil
- end
-
- it "unsets resolved_by on the resolved note" do
- subject.unresolve!
-
- expect(subject.first_note.resolved_by).to be_nil
- end
-
- it "unmarks the resolved note as resolved" do
- subject.unresolve!
-
- expect(subject.first_note.resolved?).to be false
- end
- end
+ describe '.build' do
+ it 'returns a discussion of the right type' do
+ discussion = described_class.build([first_note, second_note], merge_request)
+ expect(discussion).to be_a(DiffDiscussion)
+ expect(discussion.notes.count).to be(2)
+ expect(discussion.first_note).to be(first_note)
+ expect(discussion.noteable).to be(merge_request)
end
end
- describe "#first_note_to_resolve" do
- it "returns the first not that still needs to be resolved" do
- allow(first_note).to receive(:to_be_resolved?).and_return(false)
- allow(second_note).to receive(:to_be_resolved?).and_return(true)
-
- expect(subject.first_note_to_resolve).to eq(second_note)
- end
- end
-
- describe "#collapsed?" do
- context "when a diff discussion" do
- before do
- allow(subject).to receive(:diff_discussion?).and_return(true)
- end
-
- context "when resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(true)
- end
-
- context "when resolved" do
- before do
- allow(subject).to receive(:resolved?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.collapsed?).to be true
- end
- end
-
- context "when not resolved" do
- before do
- allow(subject).to receive(:resolved?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.collapsed?).to be false
- end
- end
- end
-
- context "when not resolvable" do
- before do
- allow(subject).to receive(:resolvable?).and_return(false)
- end
-
- context "when active" do
- before do
- allow(subject).to receive(:active?).and_return(true)
- end
-
- it "returns false" do
- expect(subject.collapsed?).to be false
- end
- end
-
- context "when outdated" do
- before do
- allow(subject).to receive(:active?).and_return(false)
- end
-
- it "returns true" do
- expect(subject.collapsed?).to be true
- end
- end
- end
- end
-
- context "when not a diff discussion" do
- before do
- allow(subject).to receive(:diff_discussion?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.collapsed?).to be false
- end
- end
- end
-
- describe "#truncated_diff_lines" do
- let(:truncated_lines) { subject.truncated_diff_lines }
-
- context "when diff is greater than allowed number of truncated diff lines " do
- it "returns fewer lines" do
- expect(subject.diff_lines.count).to be > described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
-
- expect(truncated_lines.count).to be <= described_class::NUMBER_OF_TRUNCATED_DIFF_LINES
- end
- end
-
- context "when some diff lines are meta" do
- it "returns no meta lines" do
- expect(subject.diff_lines).to include(be_meta)
- expect(truncated_lines).not_to include(be_meta)
- end
+ describe '.build_collection' do
+ it 'returns an array of discussions of the right type' do
+ discussions = described_class.build_collection([first_note, second_note, third_note], merge_request)
+ expect(discussions).to eq([
+ DiffDiscussion.new([first_note, second_note], merge_request),
+ DiffDiscussion.new([third_note], merge_request)
+ ])
end
end
end
diff --git a/spec/models/legacy_diff_discussion_spec.rb b/spec/models/legacy_diff_discussion_spec.rb
new file mode 100644
index 00000000000..153e757a0ef
--- /dev/null
+++ b/spec/models/legacy_diff_discussion_spec.rb
@@ -0,0 +1,11 @@
+require 'spec_helper'
+
+describe LegacyDiffDiscussion, models: true do
+ subject { create(:legacy_diff_note_on_merge_request).to_discussion }
+
+ describe '#reply_attributes' do
+ it 'includes line_code' do
+ expect(subject.reply_attributes[:line_code]).to eq(subject.line_code)
+ end
+ end
+end
diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb
deleted file mode 100644
index 81517a18b74..00000000000
--- a/spec/models/legacy_diff_note_spec.rb
+++ /dev/null
@@ -1,101 +0,0 @@
-require 'spec_helper'
-
-describe LegacyDiffNote, models: true do
- describe "Commit diff line notes" do
- let!(:note) { create(:legacy_diff_note_on_commit, note: "+1 from me") }
- let!(:commit) { note.noteable }
-
- it "saves a valid note" do
- expect(note.commit_id).to eq(commit.id)
- expect(note.noteable.id).to eq(commit.id)
- end
-
- it "is recognized by #legacy_diff_note?" do
- expect(note).to be_legacy_diff_note
- end
- end
-
- describe '#active?' do
- it 'is always true when the note has no associated diff line' do
- note = build(:legacy_diff_note_on_merge_request)
-
- expect(note).to receive(:diff_line).and_return(nil)
-
- expect(note).to be_active
- end
-
- it 'is never true when the note has no noteable associated' do
- note = build(:legacy_diff_note_on_merge_request)
-
- expect(note).to receive(:diff_line).and_return(double)
- expect(note).to receive(:noteable).and_return(nil)
-
- expect(note).not_to be_active
- end
-
- it 'returns the memoized value if defined' do
- note = build(:legacy_diff_note_on_merge_request)
-
- note.instance_variable_set(:@active, 'foo')
- expect(note).not_to receive(:find_noteable_diff)
-
- expect(note.active?).to eq 'foo'
- end
-
- context 'for a merge request noteable' do
- it 'is false when noteable has no matching diff' do
- merge = build_stubbed(:merge_request, :simple)
- note = build(:legacy_diff_note_on_merge_request, noteable: merge)
-
- allow(note).to receive(:diff_line).and_return(double)
- expect(note).to receive(:find_noteable_diff).and_return(nil)
-
- expect(note).not_to be_active
- end
-
- it 'is true when noteable has a matching diff' do
- merge = create(:merge_request, :simple)
-
- # Generate a real line_code value so we know it will match. We use a
- # random line from a random diff just for funsies.
- diff = merge.raw_diffs.to_a.sample
- line = Gitlab::Diff::Parser.new.parse(diff.diff.each_line).to_a.sample
- code = Gitlab::Diff::LineCode.generate(diff.new_path, line.new_pos, line.old_pos)
-
- # We're persisting in order to trigger the set_diff callback
- note = create(:legacy_diff_note_on_merge_request, noteable: merge,
- line_code: code,
- project: merge.source_project)
-
- # Make sure we don't get a false positive from a guard clause
- expect(note).to receive(:find_noteable_diff).and_call_original
- expect(note).to be_active
- end
- end
- end
-
- describe "#discussion_id" do
- let(:note) { create(:note) }
-
- context "when it is newly created" do
- it "has a discussion id" do
- expect(note.discussion_id).not_to be_nil
- expect(note.discussion_id).to match(/\A\h{40}\z/)
- end
- end
-
- context "when it didn't store a discussion id before" do
- before do
- note.update_column(:discussion_id, nil)
- end
-
- it "has a discussion id" do
- # The discussion_id is set in `after_initialize`, so `reload` won't work
- reloaded_note = Note.find(note.id)
-
- expect(reloaded_note.discussion_id).not_to be_nil
- expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
- end
- end
- end
-end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 2f6614c133e..90b3a2ba42d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1224,182 +1224,6 @@ describe MergeRequest, models: true do
end
end
- context "discussion status" do
- let(:first_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) }
- let(:second_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) }
- let(:third_discussion) { Discussion.new([create(:diff_note_on_merge_request)]) }
-
- before do
- allow(subject).to receive(:diff_discussions).and_return([first_discussion, second_discussion, third_discussion])
- end
-
- describe '#resolvable_discussions' do
- before do
- allow(first_discussion).to receive(:to_be_resolved?).and_return(true)
- allow(second_discussion).to receive(:to_be_resolved?).and_return(false)
- allow(third_discussion).to receive(:to_be_resolved?).and_return(false)
- end
-
- it 'includes only discussions that need to be resolved' do
- expect(subject.resolvable_discussions).to eq([first_discussion])
- end
- end
-
- describe '#discussions_can_be_resolved_by? user' do
- let(:user) { build(:user) }
-
- context 'all discussions can be resolved by the user' do
- before do
- allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(third_discussion).to receive(:can_resolve?).with(user).and_return(true)
- end
-
- it 'allows a user to resolve the discussions' do
- expect(subject.discussions_can_be_resolved_by?(user)).to be(true)
- end
- end
-
- context 'one discussion cannot be resolved by the user' do
- before do
- allow(first_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(second_discussion).to receive(:can_resolve?).with(user).and_return(true)
- allow(third_discussion).to receive(:can_resolve?).with(user).and_return(false)
- end
-
- it 'allows a user to resolve the discussions' do
- expect(subject.discussions_can_be_resolved_by?(user)).to be(false)
- end
- end
- end
-
- describe "#discussions_resolvable?" do
- context "when all discussions are unresolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(false)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolvable?).to be false
- end
- end
-
- context "when some discussions are unresolvable and some discussions are resolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolvable?).to be true
- end
- end
-
- context "when all discussions are resolvable" do
- before do
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(true)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolvable?).to be true
- end
- end
- end
-
- describe "#discussions_resolved?" do
- context "when discussions are not resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolved?).to be false
- end
- end
-
- context "when discussions are resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable discussions are resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(true)
- end
-
- it "returns true" do
- expect(subject.discussions_resolved?).to be true
- end
- end
-
- context "when some resolvable discussions are not resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_resolved?).to be false
- end
- end
- end
- end
-
- describe "#discussions_to_be_resolved?" do
- context "when discussions are not resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(false)
- end
-
- it "returns false" do
- expect(subject.discussions_to_be_resolved?).to be false
- end
- end
-
- context "when discussions are resolvable" do
- before do
- allow(subject).to receive(:discussions_resolvable?).and_return(true)
-
- allow(first_discussion).to receive(:resolvable?).and_return(true)
- allow(second_discussion).to receive(:resolvable?).and_return(false)
- allow(third_discussion).to receive(:resolvable?).and_return(true)
- end
-
- context "when all resolvable discussions are resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(true)
- end
-
- it "returns false" do
- expect(subject.discussions_to_be_resolved?).to be false
- end
- end
-
- context "when some resolvable discussions are not resolved" do
- before do
- allow(first_discussion).to receive(:resolved?).and_return(true)
- allow(third_discussion).to receive(:resolved?).and_return(false)
- end
-
- it "returns true" do
- expect(subject.discussions_to_be_resolved?).to be true
- end
- end
- end
- end
- end
-
describe '#conflicts_can_be_resolved_in_ui?' do
def create_merge_request(source_branch)
create(:merge_request, source_branch: source_branch, target_branch: 'conflict-start') do |mr|
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 33536487c41..3c4bf3f4ddb 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -245,14 +245,28 @@ describe Note, models: true do
end
end
+ describe '.find_discussion' do
+ let!(:note) { create(:discussion_note_on_merge_request) }
+ let!(:note2) { create(:discussion_note_on_merge_request, in_reply_to: note) }
+ let(:merge_request) { note.noteable }
+
+ it 'returns a discussion with multiple notes' do
+ discussion = merge_request.notes.find_discussion(note.discussion_id)
+
+ expect(discussion).not_to be_nil
+ expect(discussion.notes).to match_array([note, note2])
+ expect(discussion.first_note.discussion_id).to eq(note.discussion_id)
+ end
+ end
+
describe ".grouped_diff_discussions" do
let!(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let!(:active_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) }
- let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) }
+ let!(:active_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: active_diff_note1) }
let!(:active_diff_note3) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: active_position2) }
let!(:outdated_diff_note1) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) }
- let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position) }
+ let!(:outdated_diff_note2) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, in_reply_to: outdated_diff_note1) }
let(:active_position2) do
Gitlab::Diff::Position.new(
@@ -277,7 +291,7 @@ describe Note, models: true do
subject { merge_request.notes.grouped_diff_discussions }
it "includes active discussions" do
- discussions = subject.values
+ discussions = subject.values.flatten
expect(discussions.count).to eq(2)
expect(discussions.map(&:id)).to eq([active_diff_note1.discussion_id, active_diff_note3.discussion_id])
@@ -288,37 +302,12 @@ describe Note, models: true do
end
it "doesn't include outdated discussions" do
- expect(subject.values.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
+ expect(subject.values.flatten.map(&:id)).not_to include(outdated_diff_note1.discussion_id)
end
it "groups the discussions by line code" do
- expect(subject[active_diff_note1.line_code].id).to eq(active_diff_note1.discussion_id)
- expect(subject[active_diff_note3.line_code].id).to eq(active_diff_note3.discussion_id)
- end
- end
-
- describe "#discussion_id" do
- let(:note) { create(:note) }
-
- context "when it is newly created" do
- it "has a discussion id" do
- expect(note.discussion_id).not_to be_nil
- expect(note.discussion_id).to match(/\A\h{40}\z/)
- end
- end
-
- context "when it didn't store a discussion id before" do
- before do
- note.update_column(:discussion_id, nil)
- end
-
- it "has a discussion id" do
- # The discussion_id is set in `after_initialize`, so `reload` won't work
- reloaded_note = Note.find(note.id)
-
- expect(reloaded_note.discussion_id).not_to be_nil
- expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
- end
+ expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id)
+ expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id)
end
end
@@ -388,6 +377,248 @@ describe Note, models: true do
end
end
+ describe '#can_be_discussion_note?' do
+ context 'for a note on a merge request' do
+ it 'returns true' do
+ note = build(:note_on_merge_request)
+
+ expect(note.can_be_discussion_note?).to be_truthy
+ end
+ end
+
+ context 'for a note on an issue' do
+ it 'returns true' do
+ note = build(:note_on_issue)
+
+ expect(note.can_be_discussion_note?).to be_truthy
+ end
+ end
+
+ context 'for a note on a commit' do
+ it 'returns true' do
+ note = build(:note_on_commit)
+
+ expect(note.can_be_discussion_note?).to be_truthy
+ end
+ end
+
+ context 'for a note on a snippet' do
+ it 'returns true' do
+ note = build(:note_on_project_snippet)
+
+ expect(note.can_be_discussion_note?).to be_truthy
+ end
+ end
+
+ context 'for a diff note on merge request' do
+ it 'returns false' do
+ note = build(:diff_note_on_merge_request)
+
+ expect(note.can_be_discussion_note?).to be_falsey
+ end
+ end
+
+ context 'for a diff note on commit' do
+ it 'returns false' do
+ note = build(:diff_note_on_commit)
+
+ expect(note.can_be_discussion_note?).to be_falsey
+ end
+ end
+
+ context 'for a discussion note' do
+ it 'returns false' do
+ note = build(:discussion_note_on_merge_request)
+
+ expect(note.can_be_discussion_note?).to be_falsey
+ end
+ end
+ end
+
+ describe '#discussion_class' do
+ let(:note) { build(:note_on_commit) }
+ let(:merge_request) { create(:merge_request) }
+
+ context 'when the note is displayed out of context' do
+ it 'returns OutOfContextDiscussion' do
+ expect(note.discussion_class(merge_request)).to be(OutOfContextDiscussion)
+ end
+ end
+
+ context 'when the note is displayed in the original context' do
+ it 'returns IndividualNoteDiscussion' do
+ expect(note.discussion_class(note.noteable)).to be(IndividualNoteDiscussion)
+ end
+ end
+ end
+
+ describe "#discussion_id" do
+ let(:note) { create(:note_on_commit) }
+
+ context "when it is newly created" do
+ it "has a discussion id" do
+ expect(note.discussion_id).not_to be_nil
+ expect(note.discussion_id).to match(/\A\h{40}\z/)
+ end
+ end
+
+ context "when it didn't store a discussion id before" do
+ before do
+ note.update_column(:discussion_id, nil)
+ end
+
+ it "has a discussion id" do
+ # The discussion_id is set in `after_initialize`, so `reload` won't work
+ reloaded_note = Note.find(note.id)
+
+ expect(reloaded_note.discussion_id).not_to be_nil
+ expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
+ end
+ end
+
+ context 'when the note is displayed out of context' do
+ let(:merge_request) { create(:merge_request) }
+
+ it 'overrides the discussion id' do
+ expect(note.discussion_id(merge_request)).not_to eq(note.discussion_id)
+ end
+ end
+ end
+
+ describe '#to_discussion' do
+ subject { create(:discussion_note_on_merge_request) }
+ let!(:note2) { create(:discussion_note_on_merge_request, project: subject.project, noteable: subject.noteable, in_reply_to: subject) }
+
+ it "returns a discussion with just this note" do
+ discussion = subject.to_discussion
+
+ expect(discussion.id).to eq(subject.discussion_id)
+ expect(discussion.notes).to eq([subject])
+ end
+ end
+
+ describe "#discussion" do
+ let!(:note1) { create(:discussion_note_on_merge_request) }
+ let!(:note2) { create(:diff_note_on_merge_request, project: note1.project, noteable: note1.noteable) }
+
+ context 'when the note is part of a discussion' do
+ subject { create(:discussion_note_on_merge_request, project: note1.project, noteable: note1.noteable, in_reply_to: note1) }
+
+ it "returns the discussion this note is in" do
+ discussion = subject.discussion
+
+ expect(discussion.id).to eq(subject.discussion_id)
+ expect(discussion.notes).to eq([note1, subject])
+ end
+ end
+
+ context 'when the note is not part of a discussion' do
+ subject { create(:note) }
+
+ it "returns a discussion with just this note" do
+ discussion = subject.discussion
+
+ expect(discussion.id).to eq(subject.discussion_id)
+ expect(discussion.notes).to eq([subject])
+ end
+ end
+ end
+
+ describe "#part_of_discussion?" do
+ context 'for a regular note' do
+ let(:note) { build(:note) }
+
+ it 'returns false' do
+ expect(note.part_of_discussion?).to be_falsey
+ end
+ end
+
+ context 'for a diff note' do
+ let(:note) { build(:diff_note_on_commit) }
+
+ it 'returns true' do
+ expect(note.part_of_discussion?).to be_truthy
+ end
+ end
+
+ context 'for a discussion note' do
+ let(:note) { build(:discussion_note_on_merge_request) }
+
+ it 'returns true' do
+ expect(note.part_of_discussion?).to be_truthy
+ end
+ end
+ end
+
+ describe '#in_reply_to?' do
+ context 'for a note' do
+ context 'when part of a discussion' do
+ subject { create(:discussion_note_on_issue) }
+ let(:note) { create(:discussion_note_on_issue, in_reply_to: subject) }
+
+ it 'checks if the note is in reply to the other discussion' do
+ expect(subject).to receive(:in_reply_to?).with(note).and_call_original
+ expect(subject).to receive(:in_reply_to?).with(note.noteable).and_call_original
+ expect(subject).to receive(:in_reply_to?).with(note.to_discussion).and_call_original
+
+ subject.in_reply_to?(note)
+ end
+ end
+
+ context 'when not part of a discussion' do
+ subject { create(:note) }
+ let(:note) { create(:note, in_reply_to: subject) }
+
+ it 'checks if the note is in reply to the other noteable' do
+ expect(subject).to receive(:in_reply_to?).with(note).and_call_original
+ expect(subject).to receive(:in_reply_to?).with(note.noteable).and_call_original
+
+ subject.in_reply_to?(note)
+ end
+ end
+ end
+
+ context 'for a discussion' do
+ context 'when part of the same discussion' do
+ subject { create(:diff_note_on_merge_request) }
+ let(:note) { create(:diff_note_on_merge_request, in_reply_to: subject) }
+
+ it 'returns true' do
+ expect(subject.in_reply_to?(note.to_discussion)).to be_truthy
+ end
+ end
+
+ context 'when not part of the same discussion' do
+ subject { create(:diff_note_on_merge_request) }
+ let(:note) { create(:diff_note_on_merge_request) }
+
+ it 'returns false' do
+ expect(subject.in_reply_to?(note.to_discussion)).to be_falsey
+ end
+ end
+ end
+
+ context 'for a noteable' do
+ context 'when a comment on the same noteable' do
+ subject { create(:note) }
+ let(:note) { create(:note, in_reply_to: subject) }
+
+ it 'returns true' do
+ expect(subject.in_reply_to?(note.noteable)).to be_truthy
+ end
+ end
+
+ context 'when not a comment on the same noteable' do
+ subject { create(:note) }
+ let(:note) { create(:note) }
+
+ it 'returns false' do
+ expect(subject.in_reply_to?(note.noteable)).to be_falsey
+ end
+ end
+ end
+ end
+
describe 'expiring ETag cache' do
let(:note) { build(:note_on_issue) }
diff --git a/spec/models/sent_notification_spec.rb b/spec/models/sent_notification_spec.rb
new file mode 100644
index 00000000000..6b7eef388be
--- /dev/null
+++ b/spec/models/sent_notification_spec.rb
@@ -0,0 +1,166 @@
+require 'spec_helper'
+
+describe SentNotification, model: true do
+ describe 'validation' do
+ describe 'note validity' do
+ context "when the project doesn't match the noteable's project" do
+ subject { build(:sent_notification, noteable: create(:issue)) }
+
+ it "is invalid" do
+ expect(subject).not_to be_valid
+ end
+ end
+
+ context "when the project doesn't match the discussion project" do
+ let(:discussion_id) { create(:note).discussion_id }
+ subject { build(:sent_notification, in_reply_to_discussion_id: discussion_id) }
+
+ it "is invalid" do
+ expect(subject).not_to be_valid
+ end
+ end
+
+ context "when the noteable project and discussion project match" do
+ let(:project) { create(:project) }
+ let(:issue) { create(:issue, project: project) }
+ let(:discussion_id) { create(:note, project: project, noteable: issue).discussion_id }
+ subject { build(:sent_notification, project: project, noteable: issue, in_reply_to_discussion_id: discussion_id) }
+
+ it "is valid" do
+ expect(subject).to be_valid
+ end
+ end
+ end
+ end
+
+ describe '.record' do
+ let(:user) { create(:user) }
+ let(:issue) { create(:issue) }
+
+ it 'creates a new SentNotification' do
+ expect { described_class.record(issue, user.id) }.to change { SentNotification.count }.by(1)
+ end
+ end
+
+ describe '.record_note' do
+ let(:user) { create(:user) }
+ let(:note) { create(:diff_note_on_merge_request) }
+
+ it 'creates a new SentNotification' do
+ expect { described_class.record_note(note, user.id) }.to change { SentNotification.count }.by(1)
+ end
+ end
+
+ describe '#create_reply' do
+ context 'for issue' do
+ let(:issue) { create(:issue) }
+ subject { described_class.record(issue, issue.author.id) }
+
+ it 'creates a comment on the issue' do
+ note = subject.create_reply('Test')
+ expect(note.in_reply_to?(issue)).to be_truthy
+ end
+ end
+
+ context 'for issue comment' do
+ let(:note) { create(:note_on_issue) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a comment on the issue' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for issue discussion' do
+ let(:note) { create(:discussion_note_on_issue) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a reply on the discussion' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for merge request' do
+ let(:merge_request) { create(:merge_request) }
+ subject { described_class.record(merge_request, merge_request.author.id) }
+
+ it 'creates a comment on the merge_request' do
+ note = subject.create_reply('Test')
+ expect(note.in_reply_to?(merge_request)).to be_truthy
+ end
+ end
+
+ context 'for merge request comment' do
+ let(:note) { create(:note_on_merge_request) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a comment on the merge request' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for merge request diff discussion' do
+ let(:note) { create(:diff_note_on_merge_request) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a reply on the discussion' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for merge request non-diff discussion' do
+ let(:note) { create(:discussion_note_on_merge_request) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a reply on the discussion' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for commit' do
+ let(:project) { create(:project) }
+ let(:commit) { project.commit }
+ subject { described_class.record(commit, project.creator.id) }
+
+ it 'creates a comment on the commit' do
+ note = subject.create_reply('Test')
+ expect(note.in_reply_to?(commit)).to be_truthy
+ end
+ end
+
+ context 'for commit comment' do
+ let(:note) { create(:note_on_commit) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a comment on the commit' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for commit diff discussion' do
+ let(:note) { create(:diff_note_on_commit) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a reply on the discussion' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'for commit non-diff discussion' do
+ let(:note) { create(:discussion_note_on_commit) }
+ subject { described_class.record_note(note, note.author.id) }
+
+ it 'creates a reply on the discussion' do
+ new_note = subject.create_reply('Test')
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+ end
+end
diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb
index 2b27ce6390a..551aae7d701 100644
--- a/spec/requests/api/issues_spec.rb
+++ b/spec/requests/api/issues_spec.rb
@@ -840,7 +840,7 @@ describe API::Issues, api: true do
end
context 'resolving discussions' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb
index b1b398a897e..91d9057075f 100644
--- a/spec/requests/api/v3/issues_spec.rb
+++ b/spec/requests/api/v3/issues_spec.rb
@@ -824,7 +824,7 @@ describe API::V3::Issues, api: true do
end
context 'resolving issues in a merge request' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
before do
diff --git a/spec/services/discussions/resolve_service_spec.rb b/spec/services/discussions/resolve_service_spec.rb
index 12c3cdf28c6..ab8df7b74cd 100644
--- a/spec/services/discussions/resolve_service_spec.rb
+++ b/spec/services/discussions/resolve_service_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
describe Discussions::ResolveService do
describe '#execute' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:project) { merge_request.project }
let(:merge_request) { discussion.noteable }
let(:user) { create(:user) }
@@ -41,7 +41,7 @@ describe Discussions::ResolveService do
end
it 'can resolve multiple discussions at once' do
- other_discussion = Discussion.for_diff_notes([create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project)]).first
+ other_discussion = create(:diff_note_on_merge_request, noteable: discussion.noteable, project: discussion.noteable.source_project).to_discussion
service.execute([discussion, other_discussion])
diff --git a/spec/services/issues/build_service_spec.rb b/spec/services/issues/build_service_spec.rb
index 17990f41b3b..55d635235b0 100644
--- a/spec/services/issues/build_service_spec.rb
+++ b/spec/services/issues/build_service_spec.rb
@@ -11,7 +11,7 @@ describe Issues::BuildService, services: true do
context 'for a single discussion' do
describe '#execute' do
let(:merge_request) { create(:merge_request, title: "Hello world", source_project: project) }
- let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done")]) }
+ let(:discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, note: "Almost done").to_discussion }
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid, discussion_to_resolve: discussion.id) }
it 'references the noteable title in the issue title' do
@@ -47,7 +47,7 @@ describe Issues::BuildService, services: true do
let(:service) { described_class.new(project, user, merge_request_to_resolve_discussions_of: merge_request.iid) }
it 'mentions the author of the note' do
- discussion = Discussion.new([create(:diff_note_on_merge_request, author: create(:user, username: 'author'))])
+ discussion = create(:diff_note_on_merge_request, author: create(:user, username: 'author')).to_discussion
expect(service.item_for_discussion(discussion)).to include('@author')
end
@@ -60,7 +60,7 @@ describe Issues::BuildService, services: true do
note_result = " > This is a string\n"\
" > > with a blockquote\n"\
" > > > That has a quote\n"
- discussion = Discussion.new([create(:diff_note_on_merge_request, note: note_text)])
+ discussion = create(:diff_note_on_merge_request, note: note_text).to_discussion
expect(service.item_for_discussion(discussion)).to include(note_result)
end
end
@@ -91,25 +91,23 @@ describe Issues::BuildService, services: true do
end
describe 'with multiple discussions' do
- before do
- create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15)
- end
+ let!(:diff_note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.target_project, line_number: 15) }
it 'mentions all the authors in the description' do
- authors = merge_request.diff_discussions.map(&:author)
+ authors = merge_request.resolvable_discussions.map(&:author)
expect(issue.description).to include(*authors.map(&:to_reference))
end
it 'has a link for each unresolved discussion in the description' do
- notes = merge_request.diff_discussions.map(&:first_note)
+ notes = merge_request.resolvable_discussions.map(&:first_note)
links = notes.map { |note| Gitlab::UrlBuilder.build(note) }
expect(issue.description).to include(*links)
end
it 'mentions additional notes' do
- create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, line_number: 15)
+ create_list(:diff_note_on_merge_request, 2, noteable: merge_request, project: merge_request.target_project, in_reply_to: diff_note)
expect(issue.description).to include('(+2 comments)')
end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 776cbc4296b..80bfb731550 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -141,7 +141,7 @@ describe Issues::CreateService, services: true do
it_behaves_like 'new issuable record that supports slash commands'
context 'resolving discussions' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
diff --git a/spec/services/issues/resolve_discussions_spec.rb b/spec/services/issues/resolve_discussions_spec.rb
index 3a72f92383c..4a4929daefc 100644
--- a/spec/services/issues/resolve_discussions_spec.rb
+++ b/spec/services/issues/resolve_discussions_spec.rb
@@ -18,7 +18,7 @@ describe DummyService, services: true do
end
describe "for resolving discussions" do
- let(:discussion) { Discussion.new([create(:diff_note_on_merge_request, project: project, note: "Almost done")]) }
+ let(:discussion) { create(:diff_note_on_merge_request, project: project, note: "Almost done").to_discussion }
let(:merge_request) { discussion.noteable }
let(:other_merge_request) { create(:merge_request, source_project: project, source_branch: "other") }
diff --git a/spec/services/notes/build_service_spec.rb b/spec/services/notes/build_service_spec.rb
new file mode 100644
index 00000000000..f9dd5541b10
--- /dev/null
+++ b/spec/services/notes/build_service_spec.rb
@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe Notes::BuildService, services: true do
+ let(:note) { create(:discussion_note_on_issue) }
+ let(:project) { note.project }
+ let(:author) { note.author }
+
+ describe '#execute' do
+ context 'when in_reply_to_discussion_id is specified' do
+ context 'when a note with that original discussion ID exists' do
+ it 'sets the note up to be in reply to that note' do
+ new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
+ expect(new_note).to be_valid
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'when a note with that discussion ID exists' do
+ it 'sets the note up to be in reply to that note' do
+ new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: note.discussion_id).execute
+ expect(new_note).to be_valid
+ expect(new_note.in_reply_to?(note)).to be_truthy
+ end
+ end
+
+ context 'when no note with that discussion ID exists' do
+ it 'sets an error' do
+ new_note = described_class.new(project, author, note: 'Test', in_reply_to_discussion_id: 'foo').execute
+ expect(new_note.errors[:base]).to include('Discussion to reply to cannot be found')
+ end
+ end
+ end
+
+ it 'builds a note without saving it' do
+ new_note = described_class.new(project, author, noteable_type: note.noteable_type, noteable_id: note.noteable_id, note: 'Test').execute
+ expect(new_note).to be_valid
+ expect(new_note).not_to be_persisted
+ end
+ end
+end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index e3146a56495..617c8eaf3d5 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -439,7 +439,7 @@ describe NotificationService, services: true do
notification.new_note(note)
- expect(SentNotification.last.position).to eq(note.position)
+ expect(SentNotification.last.in_reply_to_discussion_id).to eq(note.discussion_id)
end
end
end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 5ec1ed8237b..42d63a9f9ba 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -796,7 +796,7 @@ describe SystemNoteService, services: true do
end
describe '.discussion_continued_in_issue' do
- let(:discussion) { Discussion.for_diff_notes([create(:diff_note_on_merge_request)]).first }
+ let(:discussion) { create(:diff_note_on_merge_request).to_discussion }
let(:merge_request) { discussion.noteable }
let(:project) { merge_request.source_project }
let(:issue) { create(:issue, project: project) }
diff --git a/spec/support/slash_commands_helpers.rb b/spec/support/slash_commands_helpers.rb
index 0d91fe5fd5d..4bfe481115f 100644
--- a/spec/support/slash_commands_helpers.rb
+++ b/spec/support/slash_commands_helpers.rb
@@ -3,7 +3,7 @@ module SlashCommandsHelpers
Sidekiq::Testing.fake! do
page.within('.js-main-target-form') do
fill_in 'note[note]', with: text
- find('.comment-btn').trigger('click')
+ find('.js-comment-submit-button').trigger('click')
end
end
end
diff --git a/spec/support/time_tracking_shared_examples.rb b/spec/support/time_tracking_shared_examples.rb
index 52f4fabdc47..01bc80f957e 100644
--- a/spec/support/time_tracking_shared_examples.rb
+++ b/spec/support/time_tracking_shared_examples.rb
@@ -77,6 +77,6 @@ end
def submit_time(slash_command)
fill_in 'note[note]', with: slash_command
- find('.comment-btn').trigger('click')
+ find('.js-comment-submit-button').trigger('click')
wait_for_ajax
end
diff --git a/spec/views/projects/notes/_form.html.haml_spec.rb b/spec/views/projects/notes/_form.html.haml_spec.rb
index b61f016967f..a364f9bce92 100644
--- a/spec/views/projects/notes/_form.html.haml_spec.rb
+++ b/spec/views/projects/notes/_form.html.haml_spec.rb
@@ -4,7 +4,7 @@ describe 'projects/notes/_form' do
include Devise::Test::ControllerHelpers
let(:user) { create(:user) }
- let(:project) { create(:empty_project) }
+ let(:project) { create(:project, :repository) }
before do
project.team << [user, :master]
@@ -20,7 +20,7 @@ describe 'projects/notes/_form' do
context "with a note on #{noteable}" do
let(:note) { build(:"note_on_#{noteable}", project: project) }
- it 'says that only markdown is supported, not slash commands' do
+ it 'says that markdown and slash commands are supported' do
expect(rendered).to have_content('Markdown and slash commands are supported')
end
end