Commit 78c59270 authored by Douwe Maan's avatar Douwe Maan Committed by Ruben Davila

Merge branch 'diff-line-comment-vuejs' into 'master'

Diff line comments resolve

## What does this MR do?

Diff line comments can be resolved.

Part of #10325 

To do:

- [x] Backend (@DouweM)
  - [x] Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022#note_13319326. Will be made easier by https://gitlab.com/gitlab-org/gitlab-ce/issues/17237#note_13370331
  - [x] System note when all discussions are resolved
  - [x] Notification when all discussions are resolved
  - [x] Write unit tests
  - [x] Look at resolve time https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022#note_13912743 - Fixed by 4a13aa9f
- [x] Frontend (@iamphill)
  - [x] Fix bugs
  - [x] Write more feature tests 
- [x] Frontend (@connorshea)
  - [x] Address frontend feedback
  - [x] Feature specs for Jump feature
  - [x] Documentation
  - [x] Add Vue.js in a standard way

See merge request !5022
parent d02b1689
......@@ -29,6 +29,7 @@ v 8.11.0 (unreleased)
- Expand commit message width in repo view (ClemMakesApps)
- Cache highlighted diff lines for merge requests
- Pre-create all builds for a Pipeline when the new Pipeline is created !5295
- Allow merge request diff notes and discussions to be explicitly marked as resolved
- API: Add deployment endpoints
- API: Add Play endpoint on Builds
- Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
......
......@@ -224,10 +224,13 @@
});
$body.on("click", ".js-toggle-diff-comments", function(e) {
var $this = $(this);
var showComments = $this.hasClass('active');
$this.toggleClass('active');
$this.closest(".diff-file").find(".notes_holder").toggle(showComments);
var notesHolders = $this.closest('.diff-file').find('.notes_holder');
if ($this.hasClass('active')) {
notesHolders.show();
} else {
notesHolders.hide();
}
return e.preventDefault();
});
$document.off("click", '.js-confirm-danger');
......
((w) => {
w.CommentAndResolveBtn = Vue.extend({
props: {
discussionId: String,
textareaIsEmpty: Boolean
},
computed: {
discussion: function () {
return CommentsStore.state[this.discussionId];
},
showButton: function () {
if (this.discussion) {
return this.discussion.isResolvable();
} else {
return false;
}
},
isDiscussionResolved: function () {
return this.discussion.isResolved();
},
buttonText: function () {
if (this.isDiscussionResolved) {
if (this.textareaIsEmpty) {
return "Unresolve discussion";
} else {
return "Comment & unresolve discussion";
}
} else {
if (this.textareaIsEmpty) {
return "Resolve discussion";
} else {
return "Comment & resolve discussion";
}
}
}
},
ready: function () {
const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`);
this.textareaIsEmpty = $textarea.val() === '';
$textarea.on('input.comment-and-resolve-btn', () => {
this.textareaIsEmpty = $textarea.val() === '';
});
},
destroyed: function () {
$(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn');
}
});
})(window);
(() => {
JumpToDiscussion = Vue.extend({
mixins: [DiscussionMixins],
props: {
discussionId: String
},
data: function () {
return {
discussions: CommentsStore.state,
};
},
computed: {
discussion: function () {
return this.discussions[this.discussionId];
},
allResolved: function () {
return this.unresolvedDiscussionCount === 0;
},
showButton: function () {
if (this.discussionId) {
if (this.unresolvedDiscussionCount > 1) {
return true;
} else {
return this.discussionId !== this.lastResolvedId;
}
} else {
return this.unresolvedDiscussionCount >= 1;
}
},
lastResolvedId: function () {
let lastId;
for (const discussionId in this.discussions) {
const discussion = this.discussions[discussionId];
if (!discussion.isResolved()) {
lastId = discussion.id;
}
}
return lastId;
}
},
methods: {
jumpToNextUnresolvedDiscussion: function () {
let discussionsSelector,
discussionIdsInScope,
firstUnresolvedDiscussionId,
nextUnresolvedDiscussionId,
activeTab = window.mrTabs.currentAction,
hasDiscussionsToJumpTo = true,
jumpToFirstDiscussion = !this.discussionId;
const discussionIdsForElements = function(elements) {
return elements.map(function() {
return $(this).attr('data-discussion-id');
}).toArray();
};
const discussions = this.discussions;
if (activeTab === 'diffs') {
discussionsSelector = '.diffs .notes[data-discussion-id]';
discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
let unresolvedDiscussionCount = 0;
for (let i = 0; i < discussionIdsInScope.length; i++) {
const discussionId = discussionIdsInScope[i];
const discussion = discussions[discussionId];
if (discussion && !discussion.isResolved()) {
unresolvedDiscussionCount++;
}
}
if (this.discussionId && !this.discussion.isResolved()) {
// If this is the last unresolved discussion on the diffs tab,
// there are no discussions to jump to.
if (unresolvedDiscussionCount === 1) {
hasDiscussionsToJumpTo = false;
}
} else {
// If there are no unresolved discussions on the diffs tab at all,
// there are no discussions to jump to.
if (unresolvedDiscussionCount === 0) {
hasDiscussionsToJumpTo = false;
}
}
} else if (activeTab !== 'notes') {
// If we are on the commits or builds tabs,
// there are no discussions to jump to.
hasDiscussionsToJumpTo = false;
}
if (!hasDiscussionsToJumpTo) {
// If there are no discussions to jump to on the current page,
// switch to the notes tab and jump to the first disucssion there.
window.mrTabs.activateTab('notes');
activeTab = 'notes';
jumpToFirstDiscussion = true;
}
if (activeTab === 'notes') {
discussionsSelector = '.discussion[data-discussion-id]';
discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
}
let currentDiscussionFound = false;
for (let i = 0; i < discussionIdsInScope.length; i++) {
const discussionId = discussionIdsInScope[i];
const discussion = discussions[discussionId];
if (!discussion) {
// Discussions for comments on commits in this MR don't have a resolved status.
continue;
}
if (!firstUnresolvedDiscussionId && !discussion.isResolved()) {
firstUnresolvedDiscussionId = discussionId;
if (jumpToFirstDiscussion) {
break;
}
}
if (!jumpToFirstDiscussion) {
if (currentDiscussionFound) {
if (!discussion.isResolved()) {
nextUnresolvedDiscussionId = discussionId;
break;
}
else {
continue;
}
}
if (discussionId === this.discussionId) {
currentDiscussionFound = true;
}
}
}
nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || firstUnresolvedDiscussionId;
if (!nextUnresolvedDiscussionId) {
return;
}
let $target = $(`${discussionsSelector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`);
if (activeTab === 'notes') {
$target = $target.closest('.note-discussion');
// If the next discussion is closed, toggle it open.
if ($target.find('.js-toggle-content').is(':hidden')) {
$target.find('.js-toggle-button i').trigger('click')
}
} else if (activeTab === 'diffs') {
// Resolved discussions are hidden in the diffs tab by default.
// If they are marked unresolved on the notes tab, they will still be hidden on the diffs tab.
// When jumping between unresolved discussions on the diffs tab, we show them.
$target.closest(".content").show();
$target = $target.closest("tr.notes_holder");
$target.show();
// If we are on the diffs tab, we don't scroll to the discussion itself, but to
// 4 diff lines above it: the line the discussion was in response to + 3 context
let prevEl;
for (let i = 0; i < 4; i++) {
prevEl = $target.prev();
// If the discussion doesn't have 4 lines above it, we'll have to do with fewer.
if (!prevEl.hasClass("line_holder")) {
break;
}
$target = prevEl;
}
}
$.scrollTo($target, {
offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight())
});
}
}
});
Vue.component('jump-to-discussion', JumpToDiscussion);
})();
((w) => {
w.ResolveBtn = Vue.extend({
mixins: [
ButtonMixins
],
props: {
noteId: Number,
discussionId: String,
resolved: Boolean,
namespacePath: String,
projectPath: String,
canResolve: Boolean,
resolvedBy: String
},
data: function () {
return {
discussions: CommentsStore.state,
loading: false
};
},
watch: {
'discussions': {
handler: 'updateTooltip',
deep: true
}
},
computed: {
discussion: function () {
return this.discussions[this.discussionId];
},
note: function () {
if (this.discussion) {
return this.discussion.getNote(this.noteId);
} else {
return undefined;
}
},
buttonText: function () {
if (this.isResolved) {
return `Resolved by ${this.resolvedByName}`;
} else if (this.canResolve) {
return 'Mark as resolved';
} else {
return 'Unable to resolve';
}
},
isResolved: function () {
if (this.note) {
return this.note.resolved;
} else {
return false;
}
},
resolvedByName: function () {
return this.note.resolved_by;
},
},
methods: {
updateTooltip: function () {
$(this.$els.button)
.tooltip('hide')
.tooltip('fixTitle');
},
resolve: function () {
if (!this.canResolve) return;
let promise;
this.loading = true;
if (this.isResolved) {
promise = ResolveService
.unresolve(this.namespace, this.noteId);
} else {
promise = ResolveService
.resolve(this.namespace, this.noteId);
}
promise.then((response) => {
this.loading = false;
if (response.status === 200) {
const data = response.json();
const resolved_by = data ? data.resolved_by : null;
CommentsStore.update(this.discussionId, this.noteId, !this.isResolved, resolved_by);
this.discussion.updateHeadline(data);
} else {
new Flash('An error occurred when trying to resolve a comment. Please try again.', 'alert');
}
this.$nextTick(this.updateTooltip);
});
}
},
compiled: function () {
$(this.$els.button).tooltip({
container: 'body'
});
},
beforeDestroy: function () {
CommentsStore.delete(this.discussionId, this.noteId);
},
created: function () {
CommentsStore.create(this.discussionId, this.noteId, this.canResolve, this.resolved, this.resolvedBy);
}
});
})(window);
((w) => {
w.ResolveCount = Vue.extend({
mixins: [DiscussionMixins],
props: {
loggedOut: Boolean
},
data: function () {
return {
discussions: CommentsStore.state
};
},
computed: {
allResolved: function () {
return this.resolvedDiscussionCount === this.discussionCount;
}
}
});
})(window);
((w) => {
w.ResolveDiscussionBtn = Vue.extend({
mixins: [
ButtonMixins
],
props: {
discussionId: String,
mergeRequestId: Number,
namespacePath: String,
projectPath: String,
canResolve: Boolean,
},
data: function() {
return {
discussions: CommentsStore.state
};
},
computed: {
discussion: function () {
return this.discussions[this.discussionId];
},
showButton: function () {
if (this.discussion) {
return this.discussion.isResolvable();
} else {
return false;
}
},
isDiscussionResolved: function () {
if (this.discussion) {
return this.discussion.isResolved();
} else {
return false;
}
},
buttonText: function () {
if (this.isDiscussionResolved) {
return "Unresolve discussion";
} else {
return "Resolve discussion";
}
},
loading: function () {
if (this.discussion) {
return this.discussion.loading;
} else {
return false;
}
}
},
methods: {
resolve: function () {
ResolveService.toggleResolveForDiscussion(this.namespace, this.mergeRequestId, this.discussionId);
}
},
created: function () {
CommentsStore.createDiscussion(this.discussionId, this.canResolve);
}
});
})(window);
//= require vue
//= require vue-resource
//= require_directory ./models
//= require_directory ./stores
//= require_directory ./services
//= require_directory ./mixins
//= require_directory ./components
$(() => {
window.DiffNotesApp = new Vue({
el: '#diff-notes-app',
components: {
'resolve-btn': ResolveBtn,
'resolve-discussion-btn': ResolveDiscussionBtn,
'comment-and-resolve-btn': CommentAndResolveBtn
},
methods: {
compileComponents: function () {
const $components = $('resolve-btn, resolve-discussion-btn, jump-to-discussion');
if ($components.length) {
$components.each(function () {
DiffNotesApp.$compile($(this).get(0));
});
}
}
}
});
new Vue({
el: '#resolve-count-app',
components: {
'resolve-count': ResolveCount
}
});
});
((w) => {
w.DiscussionMixins = {
computed: {
discussionCount: function () {
return Object.keys(this.discussions).length;
},
resolvedDiscussionCount: function () {
let resolvedCount = 0;
for (const discussionId in this.discussions) {
const discussion = this.discussions[discussionId];
if (discussion.isResolved()) {
resolvedCount++;
}
}
return resolvedCount;
},
unresolvedDiscussionCount: function () {
let unresolvedCount = 0;
for (const discussionId in this.discussions) {
const discussion = this.discussions[discussionId];
if (!discussion.isResolved()) {
unresolvedCount++;
}
}
return unresolvedCount;
}
}
};
})(window);
((w) => {
w.ButtonMixins = {
computed: {
namespace: function () {
return `${this.namespacePath}/${this.projectPath}`;
}
}
};
})(window);
class DiscussionModel {
constructor (discussionId) {
this.id = discussionId;
this.notes = {};
this.loading = false;
this.canResolve = false;
}
createNote (noteId, canResolve, resolved, resolved_by) {
Vue.set(this.notes, noteId, new NoteModel(this.id, noteId, canResolve, resolved, resolved_by));
}
deleteNote (noteId) {
Vue.delete(this.notes, noteId);
}
getNote (noteId) {
return this.notes[noteId];
}
notesCount() {
return Object.keys(this.notes).length;
}
isResolved () {
for (const noteId in this.notes) {
const note = this.notes[noteId];
if (!note.resolved) {
return false;
}
}
return true;
}
resolveAllNotes (resolved_by) {
for (const noteId in this.notes) {
const note = this.notes[noteId];
if (!note.resolved) {
note.resolved = true;
note.resolved_by = resolved_by;
}
}
}
unResolveAllNotes () {
for (const noteId in this.notes) {
const note = this.notes[noteId];
if (note.resolved) {
note.resolved = false;
note.resolved_by = null;
}
}
}
updateHeadline (data) {
const $discussionHeadline = $(`.discussion[data-discussion-id="${this.id}"] .js-discussion-headline`);
if (data.discussion_headline_html) {
if ($discussionHeadline.length) {
$discussionHeadline.replaceWith(data.discussion_headline_html);
} else {
$(`.discussion[data-discussion-id="${this.id}"] .discussion-header`).append(data.discussion_headline_html);
}
} else {
$discussionHeadline.remove();
}
}
isResolvable () {
if (!this.canResolve) {
return false;
}
for (const noteId in this.notes) {
const note = this.notes[noteId];
if (note.canResolve) {
return true;
}
}
return false;
}
}
class NoteModel {
constructor (discussionId, noteId, canResolve, resolved, resolved_by) {
this.discussionId = discussionId;
this.id = noteId;
this.canResolve = canResolve;
this.resolved = resolved;
this.resolved_by = resolved_by;
}
}
((w) => {
class ResolveServiceClass {
constructor() {
this.noteResource = Vue.resource('notes{/noteId}/resolve');
this.discussionResource = Vue.resource('merge_requests{/mergeRequestId}/discussions{/discussionId}/resolve');
}
setCSRF() {
Vue.http.headers.common['X-CSRF-Token'] = $.rails.csrfToken();
}
prepareRequest(namespace) {
this.setCSRF();
Vue.http.options.root = `/${namespace}`;
}
resolve(namespace, noteId) {
this.prepareRequest(namespace);
return this.noteResource.save({ noteId }, {});
}
unresolve(namespace, noteId) {
this.prepareRequest(namespace);
return this.noteResource.delete({ noteId }, {});
}
toggleResolveForDiscussion(namespace, mergeRequestId, discussionId) {
const discussion = CommentsStore.state[discussionId],
isResolved = discussion.isResolved();
let promise;
if (isResolved) {
promise = this.unResolveAll(namespace, mergeRequestId, discussionId);
} else {
promise = this.resolveAll(namespace, mergeRequestId, discussionId);
}
promise.then((response) => {
discussion.loading = false;
if (response.status === 200) {
const data = response.json();
const resolved_by = data ? data.resolved_by : null;
if (isResolved) {
discussion.unResolveAllNotes();
} else {
discussion.resolveAllNotes(resolved_by);
}
discussion.updateHeadline(data);
} else {
new Flash('An error occurred when trying to resolve a discussion. Please try again.', 'alert');
}
})
}
resolveAll(namespace, mergeRequestId, discussionId) {
const discussion = CommentsStore.state[discussionId];
this.prepareRequest(namespace);
discussion.loading = true;
return this.discussionResource.save({
mergeRequestId,
discussionId
}, {});
}
unResolveAll(namespace, mergeRequestId, discussionId) {
const discussion = CommentsStore.state[discussionId];
this.prepareRequest(namespace);
discussion.loading = true;
return this.discussionResource.delete({
mergeRequestId,
discussionId
}, {});
}
}
w.ResolveService = new ResolveServiceClass();
})(window);
((w) => {
w.CommentsStore = {
state: {},
get: function (discussionId, noteId) {
return this.state[discussionId].getNote(noteId);
},
createDiscussion: function (discussionId, canResolve) {
let discussion = this.state[discussionId];
if (!this.state[discussionId]) {
discussion = new DiscussionModel(discussionId);
Vue.set(this.state, discussionId, discussion);
}
if (canResolve !== undefined) {
discussion.canResolve = canResolve;
}
return discussion;
},
create: function (discussionId, noteId, canResolve, resolved, resolved_by) {
const discussion = this.createDiscussion(discussionId);
discussion.createNote(noteId, canResolve, resolved, resolved_by);
},
update: function (discussionId, noteId, resolved, resolved_by) {
const discussion = this.state[discussionId];
const note = discussion.getNote(noteId);
note.resolved = resolved;
note.resolved_by = resolved_by;
},
delete: function (discussionId, noteId) {
const discussion = this.state[discussionId];
discussion.deleteNote(noteId);
if (discussion.notesCount() === 0) {
Vue.delete(this.state, discussionId);
}
},
unresolvedDiscussionIds: function () {
let ids = [];
for (const discussionId in this.state) {
const discussion = this.state[discussionId];
if (!discussion.isResolved()) {
ids.push(discussion.id);
}
}
return ids;
}
};
})(window);
......@@ -34,7 +34,7 @@
MergeRequest.prototype.initTabs = function() {
if (this.opts.action !== 'new') {
return new MergeRequestTabs(this.opts);
window.mrTabs = new MergeRequestTabs(this.opts);
} else {
return $('.merge-request-tabs a[data-toggle="tab"]:first').tab('show');
}
......
......@@ -89,6 +89,7 @@
if (action === 'show') {
action = 'notes';
}
this.currentAction = action;
new_state = this._location.pathname.replace(/\/(commits|diffs|builds|pipelines)(\.html)?\/?$/, '');
if (action !== 'notes') {
new_state += "/" + action;
......@@ -127,6 +128,11 @@
success: (function(_this) {
return function(data) {
$('#diffs').html(data.html);
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'));
$('#diffs .js-syntax-highlight').syntaxHighlight();
$('#diffs .diff-file').singleFileDiff();
......
......@@ -68,6 +68,7 @@
$(document).on("click", ".note-edit-cancel", this.cancelEdit);
$(document).on("click", ".js-comment-button", this.updateCloseButton);
$(document).on("keyup input", ".js-note-text", this.updateTargetButtons);
$(document).on('click', '.js-comment-resolve-button', this.resolveDiscussion);
$(document).on("click", ".js-note-delete", this.removeNote);
$(document).on("click", ".js-note-attachment-delete", this.removeAttachment);
$(document).on("ajax:complete", ".js-main-target-form", this.reenableTargetFormSubmitButton);
......@@ -100,6 +101,7 @@
$(document).off("click", ".js-note-target-close");
$(document).off("click", ".js-note-discard");
$(document).off("keydown", ".js-note-text");
$(document).off('click', '.js-comment-resolve-button');
$('.note .js-task-list-container').taskList('disable');
return $(document).off('tasklist:changed', '.note .js-task-list-container');
};
......@@ -304,6 +306,11 @@
} else {
discussionContainer.append(note_html);
}
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
gl.utils.localTimeAgo($('.js-timeago', note_html), false);
return this.updateNotesCount(1);
};
......@@ -350,6 +357,7 @@
form.find("#note_line_code").remove();
form.find("#note_position").remove();
form.find("#note_type").remove();
form.find('.js-comment-resolve-button').closest('comment-and-resolve-btn').remove();
return this.parentTimeline = form.parents('.timeline');
};
......@@ -393,8 +401,22 @@
*/
Notes.prototype.addDiscussionNote = function(xhr, note, status) {
var $form = $(xhr.target);
if ($form.attr('data-resolve-all') != null) {
var namespacePath = $form.attr('data-namespace-path'),
projectPath = $form.attr('data-project-path')
discussionId = $form.attr('data-discussion-id'),
mergeRequestId = $form.attr('data-noteable-iid'),
namespace = namespacePath + '/' + projectPath;
if (ResolveService != null) {
ResolveService.toggleResolveForDiscussion(namespace, mergeRequestId, discussionId);
}
}
this.renderDiscussionNote(note);
return this.removeDiscussionNoteForm($(xhr.target));
this.removeDiscussionNoteForm($form);
};
......@@ -411,7 +433,12 @@
$html.syntaxHighlight();
$html.find('.js-task-list-container').taskList('enable');
$note_li = $('.note-row-' + note.id);
return $note_li.replaceWith($html);
$note_li.replaceWith($html);
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
};
......@@ -492,6 +519,15 @@
var note, notes;
note = $(el);
notes = note.closest(".notes");
if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) {
ref = DiffNotesApp.$refs[noteId];
if (ref) {
ref.$destroy(true);
}
}
if (notes.find(".note").length === 1) {
notes.closest(".timeline-entry").remove();
notes.closest("tr").remove();
......@@ -530,8 +566,10 @@
var form, replyLink;
form = this.formClone.clone();
replyLink = $(e.target).closest(".js-discussion-reply-button");
replyLink.hide();
replyLink.after(form);
replyLink
.closest('.discussion-reply-holder')
.hide()
.after(form);
return this.setupDiscussionNoteForm(replyLink, form);
};
......@@ -556,9 +594,23 @@
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();
this.setupNoteForm(form);
if (typeof DiffNotesApp !== 'undefined') {
var $commentBtn = form.find('comment-and-resolve-btn');
$commentBtn
.attr(':discussion-id', "'" + dataHolder.data('discussionId') + "'");
DiffNotesApp.$compile($commentBtn.get(0));
}
form.find(".js-note-text").focus();
return form.removeClass('js-main-target-form').addClass("discussion-form js-discussion-note-form");
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");
};
......@@ -577,16 +629,19 @@
nextRow = row.next();
hasNotes = nextRow.is(".notes_holder");
addForm = false;
targetContent = ".notes_content";
rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"></td></tr>";
notesContentSelector = ".notes_content";
rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"><div class=\"content\"></div></td></tr>";
if (this.isParallelView()) {
lineType = $link.data("lineType");
targetContent += "." + lineType;
rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\"></td><td class=\"notes_content parallel old\"></td><td class=\"notes_line\"></td><td class=\"notes_content parallel new\"></td></tr>";
notesContentSelector += "." + lineType;
rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line old\"></td><td class=\"notes_content parallel old\"><div class=\"content\"></div></td><td class=\"notes_line new\"></td><td class=\"notes_content parallel new\"><div class=\"content\"></div></td></tr>";
}
notesContentSelector += " .content";
if (hasNotes) {
notesContent = nextRow.find(targetContent);
nextRow.show();
notesContent = nextRow.find(notesContentSelector);
if (notesContent.length) {
notesContent.show();
replyButton = notesContent.find(".js-discussion-reply-button:visible");
if (replyButton.length) {
e.target = replyButton[0];
......@@ -600,11 +655,13 @@
}
} else {
row.after(rowCssToAdd);
nextRow = row.next();
notesContent = nextRow.find(notesContentSelector);
addForm = true;
}
if (addForm) {
newForm = this.formClone.clone();
newForm.appendTo(row.next().find(targetContent));
newForm.appendTo(notesContent);
return this.setupDiscussionNoteForm($link, newForm);
}
};
......@@ -623,7 +680,9 @@
glForm = form.data('gl-form');
glForm.destroy();
form.find(".js-note-text").data("autosave").reset();
form.prev(".js-discussion-reply-button").show();
form
.prev('.discussion-reply-holder')
.show();
if (row.is(".js-temp-notes-holder")) {
return row.remove();
} else {
......@@ -732,6 +791,18 @@
return this.notesCountBadge.text(parseInt(this.notesCountBadge.text()) + updateCount);
};
Notes.prototype.resolveDiscussion = function () {
var $this = $(this),
discussionId = $this.attr('data-discussion-id');
$this
.closest('form')
.attr('data-discussion-id', discussionId)
.attr('data-resolve-all', 'true')
.attr('data-namespace-path', $this.attr('data-namespace-path'))
.attr('data-project-path', $this.attr('data-project-path'));
};
return Notes;
})();
......
......@@ -35,10 +35,16 @@
this.isOpen = !this.isOpen;
if (!this.isOpen && !this.hasError) {
this.content.hide();
return this.collapsedContent.show();
this.collapsedContent.show();
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
} else if (this.content) {
this.collapsedContent.hide();
return this.content.show();
this.content.show();
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
} else {
return this.getContentHTML();
}
......@@ -57,7 +63,11 @@
_this.hasError = true;
_this.content = $(ERROR_HTML);
}
return _this.collapsedContent.after(_this.content);
_this.collapsedContent.after(_this.content);
if (typeof DiffNotesApp !== 'undefined') {
DiffNotesApp.compileComponents();
}
};
})(this));
};
......
......@@ -21,7 +21,7 @@
}
}
[v-cloak] {
display: none;
// Hide element if Vue is still working on rendering it fully.
[v-cloak="true"] {
display: none !important;
}
......@@ -159,6 +159,32 @@
}
}
.discussion-with-resolve-btn {
display: table;
width: 100%;
border-collapse: separate;
table-layout: auto;
.btn-group {
display: table-cell;
float: none;
width: 1%;
&:first-child {
width: 100%;
padding-right: 5px;
}
&:last-child {
padding-left: 5px;
}
}
.btn {
width: 100%;
}
}
.discussion-notes-count {
font-size: 16px;
}
......
......@@ -383,3 +383,80 @@ ul.notes {
color: $gl-link-color;
}
}
.line-resolve-all-container {
.btn-group {
margin-top: -1px;
margin-left: -4px;
}
.discussion-next-btn {
border-top-left-radius: 0;
border-bottom-left-radius: 0;
}
}
.line-resolve-all {
display: inline-block;
padding: 5px 10px;
background-color: $background-color;
border: 1px solid $border-color;
border-radius: $border-radius-default;
&.has-next-btn {
border-top-right-radius: 0;
border-bottom-right-radius: 0;
}
.line-resolve-btn {
vertical-align: middle;
margin-right: 5px;
}
}
.line-resolve-text {
vertical-align: middle;
}
.line-resolve-btn {
display: inline-block;
position: relative;
top: 2px;
padding: 0;
background-color: transparent;
border: none;
outline: 0;
&.is-disabled {
cursor: default;
}
&:not(.is-disabled):hover,
&:not(.is-disabled):focus,
&.is-active {
color: $gl-text-green;
svg path {
fill: $gl-text-green;
}
}
svg {
position: relative;
color: $notes-action-color;
path {
fill: $notes-action-color;
}
}
}
.discussion-next-btn {
svg {
margin: 0;
path {
fill: $gray-darkest;
}
}
}
class Projects::DiscussionsController < Projects::ApplicationController
before_action :module_enabled
before_action :merge_request
before_action :discussion
before_action :authorize_resolve_discussion!
def resolve
discussion.resolve!(current_user)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request)
render json: {
resolved_by: discussion.resolved_by.try(:name),
discussion_headline_html: view_to_html_string('discussions/_headline', discussion: discussion)
}
end
def unresolve
discussion.unresolve!
render json: {
discussion_headline_html: view_to_html_string('discussions/_headline', discussion: discussion)
}
end
private
def merge_request
@merge_request ||= @project.merge_requests.find_by!(iid: params[:merge_request_id])
end
def discussion
@discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404
end
def authorize_resolve_discussion!
access_denied! unless discussion.can_resolve?(current_user)
end
def module_enabled
render_404 unless @project.merge_requests_enabled
end
end
......@@ -435,12 +435,9 @@ class Projects::MergeRequestsController < Projects::ApplicationController
# :show, :diff, :commits, :builds. but not when request the data through AJAX
def define_discussion_vars
# Build a note object for comment form
@note = @project.notes.new(noteable: @noteable)
@note = @project.notes.new(noteable: @merge_request)
@discussions = @noteable.mr_and_commit_notes.
inc_author_project_award_emoji.
fresh.
discussions
@discussions = @merge_request.discussions
preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
......@@ -474,7 +471,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
}
@use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
@grouped_diff_discussions = @merge_request.notes.inc_author_project_award_emoji.grouped_diff_discussions
@grouped_diff_discussions = @merge_request.notes.inc_relations_for_view.grouped_diff_discussions
Banzai::NoteRenderer.render(
@grouped_diff_discussions.values.flat_map(&:notes),
......
......@@ -5,6 +5,7 @@ class Projects::NotesController < Projects::ApplicationController
before_action :authorize_read_note!
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
......@@ -66,6 +67,33 @@ class Projects::NotesController < Projects::ApplicationController
end
end
def resolve
return render_404 unless note.resolvable?
note.resolve!(current_user)
MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
discussion = note.discussion
render json: {
resolved_by: note.resolved_by.try(:name),
discussion_headline_html: (view_to_html_string('discussions/_headline', discussion: discussion) if discussion)
}
end
def unresolve
return render_404 unless note.resolvable?
note.unresolve!
discussion = note.discussion
render json: {
discussion_headline_html: (view_to_html_string('discussions/_headline', discussion: discussion) if discussion)
}
end
private
def note
......@@ -138,7 +166,7 @@ class Projects::NotesController < Projects::ApplicationController
}
if note.diff_note?
discussion = Discussion.new([note])
discussion = note.to_discussion
attrs.merge!(
diff_discussion_html: diff_discussion_html(discussion),
......@@ -175,6 +203,10 @@ class Projects::NotesController < Projects::ApplicationController
return access_denied! unless can?(current_user, :admin_note, note)
end
def authorize_resolve_note!
return access_denied! unless can?(current_user, :resolve_note, note)
end
def note_params
params.require(:note).permit(
:note, :noteable, :noteable_id, :noteable_type, :project_id,
......
......@@ -32,6 +32,8 @@ module AppearancesHelper
end
def custom_icon(icon_name, size: 16)
# We can't simply do the below, because there are some .erb SVGs.
# File.read(Rails.root.join("app/views/shared/icons/_#{icon_name}.svg")).html_safe
render "shared/icons/#{icon_name}.svg", size: size
end
end
......@@ -49,7 +49,7 @@ module NotesHelper
}
if use_legacy_diff_note
discussion_id = LegacyDiffNote.build_discussion_id(
discussion_id = LegacyDiffNote.discussion_id(
@comments_target[:noteable_type],
@comments_target[:noteable_id] || @comments_target[:commit_id],
line_code
......@@ -60,7 +60,7 @@ module NotesHelper
discussion_id: discussion_id
)
else
discussion_id = DiffNote.build_discussion_id(
discussion_id = DiffNote.discussion_id(
@comments_target[:noteable_type],
@comments_target[:noteable_id] || @comments_target[:commit_id],
position
......@@ -81,11 +81,9 @@ module NotesHelper
data = discussion.reply_attributes.merge(line_type: line_type)
content_tag(:div, class: "discussion-reply-holder") do
button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
data: data, title: 'Add a reply'
end
end
def preload_max_access_for_authors(notes, project)
user_ids = notes.map(&:author_id)
......
......@@ -47,6 +47,13 @@ module Emails
mail_answer_thread(@merge_request, merge_request_thread_options(updated_by_user_id, recipient_id))
end
def resolved_all_discussions_email(recipient_id, merge_request_id, resolved_by_user_id)
setup_merge_request_mail(merge_request_id, recipient_id)
@resolved_by = User.find(resolved_by_user_id)
mail_answer_thread(@merge_request, merge_request_thread_options(resolved_by_user_id, recipient_id))
end
private
def setup_merge_request_mail(merge_request_id, recipient_id)
......
......@@ -276,6 +276,7 @@ class Ability
:create_merge_request,
:create_wiki,
:push_code,
:resolve_note,
:create_container_image,
:update_container_image,
:create_environment,
......@@ -457,7 +458,8 @@ class Ability
rules += [
:read_note,
:update_note,
:admin_note
:admin_note,
:resolve_note
]
end
......@@ -465,6 +467,10 @@ class Ability
rules += project_abilities(user, note.project)
end
if note.for_merge_request? && note.noteable.author == user
rules << :resolve_note
end
rules
end
......
......@@ -9,11 +9,13 @@ class DiffNote < Note
validates :diff_line, presence: true
validates :line_code, presence: true, line_code: true
validates :noteable_type, inclusion: { in: ['Commit', 'MergeRequest'] }
validates :resolved_by, presence: true, if: :resolved?
validate :positions_complete
validate :verify_supported
after_initialize :ensure_original_discussion_id
before_validation :set_original_position, :update_position, on: :create
before_validation :set_line_code
before_validation :set_line_code, :set_original_discussion_id
after_save :keep_around_commits
class << self
......@@ -30,14 +32,6 @@ class DiffNote < Note
{ position: position.to_json }
end
def discussion_id
@discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
end
def original_discussion_id
@original_discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
end
def position=(new_position)
if new_position.is_a?(String)
new_position = JSON.parse(new_position) rescue nil
......@@ -72,10 +66,48 @@ class DiffNote < Note
self.position.diff_refs == diff_refs
end
def resolvable?
!system? && for_merge_request?
end
def resolved?
return false unless resolvable?
self.resolved_at.present?
end
def resolve!(current_user)
return unless resolvable?
return if resolved?
self.resolved_at = Time.now
self.resolved_by = current_user
save!
end
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
def to_discussion
Discussion.new([self])
end
private
def supported?
!self.for_merge_request? || self.noteable.has_complete_diff_refs?
for_commit? || self.noteable.has_complete_diff_refs?
end
def noteable_diff_refs
......@@ -94,6 +126,26 @@ class DiffNote < Note
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?
......
class Discussion
NUMBER_OF_TRUNCATED_DIFF_LINES = 16
attr_reader :first_note, :notes
attr_reader :first_note, :last_note, :notes
delegate :created_at,
:project,
......@@ -18,6 +18,12 @@ class Discussion
to: :first_note
delegate :resolved_at,
:resolved_by,
to: :last_resolved_note,
allow_nil: true
delegate :blob, :highlighted_diff_lines, to: :diff_file, allow_nil: true
def self.for_notes(notes)
......@@ -30,13 +36,30 @@ class Discussion
def initialize(notes)
@first_note = notes.first
@last_note = notes.last
@notes = notes
end
def last_resolved_note
return unless resolved?
@last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
end
def last_updated_at
last_note.created_at
end
def last_updated_by
last_note.author
end
def id
first_note.discussion_id
end
alias_method :to_param, :id
def diff_discussion?
first_note.diff_note?
end
......@@ -45,6 +68,50 @@ class Discussion
notes.any?(&:legacy_diff_note?)
end
def resolvable?
return @resolvable if defined?(@resolvable)
@resolvable = diff_discussion? && notes.any?(&:resolvable?)
end
def resolved?
return @resolved if defined?(@resolved)
@resolved = resolvable? && notes.none?(&:to_be_resolved?)
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?
notes.each do |note|
note.resolve!(current_user) if note.resolvable?
end
end
def unresolve!
return unless resolvable?
notes.each do |note|
note.unresolve! if note.resolvable?
end
end
def for_target?(target)
self.noteable == target && !diff_discussion?
end
......@@ -55,8 +122,20 @@ class Discussion
@active = first_note.active?
end
def collapsed?
return false unless diff_discussion?
if resolvable?
# New diff discussions only disappear once they are marked resolved
resolved?
else
# Old diff discussions disappear once they become outdated
!active?
end
end
def expanded?
!diff_discussion? || active?
!collapsed?
end
def reply_attributes
......
......@@ -8,8 +8,8 @@ class LegacyDiffNote < Note
before_create :set_diff
class << self
def build_discussion_id(noteable_type, noteable_id, line_code, active = true)
[super(noteable_type, noteable_id), line_code, active].join("-")
def build_discussion_id(noteable_type, noteable_id, line_code)
[super(noteable_type, noteable_id), line_code].join("-")
end
end
......@@ -21,10 +21,6 @@ class LegacyDiffNote < Note
{ line_code: line_code }
end
def discussion_id
@discussion_id ||= self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
end
def project_repository
if RequestStore.active?
RequestStore.fetch("project:#{project_id}:repository") { self.project.repository }
......@@ -119,4 +115,8 @@ 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
......@@ -418,6 +418,32 @@ class MergeRequest < ActiveRecord::Base
)
end
def discussions
@discussions ||= self.mr_and_commit_notes.
inc_relations_for_view.
fresh.
discussions
end
def diff_discussions
@diff_discussions ||= self.notes.diff_notes.discussions
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 hook_attrs
attrs = {
source: source_project.try(:hook_attrs),
......
......@@ -25,6 +25,9 @@ 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
......@@ -59,7 +62,7 @@ class Note < ActiveRecord::Base
scope :fresh, ->{ order(created_at: :asc, id: :asc) }
scope :inc_author_project, ->{ includes(:project, :author) }
scope :inc_author, ->{ includes(:author) }
scope :inc_author_project_award_emoji, ->{ includes(:project, :author, :award_emoji) }
scope :inc_relations_for_view, ->{ includes(:project, :author, :updated_by, :resolved_by, :award_emoji) }
scope :diff_notes, ->{ where(type: ['LegacyDiffNote', 'DiffNote']) }
scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
......@@ -70,7 +73,9 @@ class Note < ActiveRecord::Base
project: [:project_members, { group: [:group_members] }])
end
after_initialize :ensure_discussion_id
before_validation :nullify_blank_type, :nullify_blank_line_code
before_validation :set_discussion_id
after_save :keep_around_commit
class << self
......@@ -82,13 +87,18 @@ class Note < ActiveRecord::Base
[:discussion, noteable_type.try(:underscore), noteable_id].join("-")
end
def discussion_id(*args)
Digest::SHA1.hexdigest(build_discussion_id(*args))
end
def discussions
Discussion.for_notes(all)
end
def grouped_diff_discussions
notes = diff_notes.fresh.select(&:active?)
Discussion.for_diff_notes(notes).map { |d| [d.line_code, d] }.to_h
active_notes = diff_notes.fresh.select(&:active?)
Discussion.for_diff_notes(active_notes).
map { |d| [d.line_code, d] }.to_h
end
# Searches for notes matching the given query.
......@@ -129,13 +139,16 @@ class Note < ActiveRecord::Base
true
end
def discussion_id
@discussion_id ||=
if for_merge_request?
[:discussion, :note, id].join("-")
else
self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
def resolvable?
false
end
def resolved?
false
end
def to_be_resolved?
resolvable? && !resolved?
end
def max_attachment_size
......@@ -243,4 +256,26 @@ class Note < ActiveRecord::Base
def nullify_blank_line_code
self.line_code = nil if self.line_code.blank?
end
def ensure_discussion_id
return unless self.persisted?
return if self.discussion_id
set_discussion_id
update_column(:discussion_id, self.discussion_id)
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
end
end
module MergeRequests
class ResolvedDiscussionNotificationService < MergeRequests::BaseService
def execute(merge_request)
return unless merge_request.discussions_resolved?
SystemNoteService.resolve_all_discussions(merge_request, project, current_user)
notification_service.resolve_all_discussions(merge_request, current_user)
end
end
end
......@@ -148,6 +148,14 @@ class NotificationService
)
end
def resolve_all_discussions(merge_request, current_user)
recipients = build_recipients(merge_request, merge_request.target_project, current_user, action: "resolve_all_discussions")
recipients.each do |recipient|
mailer.resolved_all_discussions_email(recipient.id, merge_request.id, current_user.id).deliver_later
end
end
# Notify new user with email after creation
def new_user(user, token = nil)
# Don't email omniauth created users
......
......@@ -158,6 +158,12 @@ module SystemNoteService
create_note(noteable: noteable, project: project, author: author, note: body)
end
def self.resolve_all_discussions(merge_request, project, author)
body = "Resolved all discussions"
create_note(noteable: merge_request, project: project, author: author, note: body)
end
# Called when the title of a Noteable is changed
#
# noteable - Noteable object that responds to `title`
......
%tr.notes_holder
- expanded = local_assigns.fetch(:expanded, true)
%tr.notes_holder{class: ('hide' unless expanded)}
%td.notes_line{ colspan: 2 }
%td.notes_content
%ul.notes{ data: { discussion_id: discussion.id } }
= render partial: "projects/notes/note", collection: discussion.notes, as: :note
= link_to_reply_discussion(discussion)
.content
= render "discussions/notes", discussion: discussion
......@@ -7,8 +7,11 @@
.diff-content.code.js-syntax-highlight
%table
- discussion.truncated_diff_lines.each do |line|
= render "projects/diffs/line", line: line, diff_file: diff_file, plain: true
- if discussion.for_line?(line)
= render "discussions/diff_discussion", discussion: discussion
- discussions = { discussion.line_code => discussion }
= render partial: "projects/diffs/line",
collection: discussion.truncated_diff_lines,
as: :line,
locals: { diff_file: diff_file,
discussions: discussions,
discussion_expanded: true,
plain: true }
......@@ -5,8 +5,17 @@
= 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 }
.discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } }
.discussion-header
.discussion-actions
= link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
- if expanded
= icon("chevron-up")
- else
= icon("chevron-down")
Toggle discussion
= link_to_member(@project, discussion.author, avatar: false)
.inline.discussion-headline-light
......@@ -29,17 +38,11 @@
= time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
.discussion-actions
= link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
- if expanded
= icon("chevron-up")
- else
= icon("chevron-down")
Toggle discussion
= render "discussions/headline", discussion: discussion
.discussion-body.js-toggle-content{ class: ("hide" unless expanded) }
- if discussion.diff_discussion? && discussion.diff_file
= render "discussions/diff_with_notes", discussion: discussion
- else
.panel.panel-default
= render "discussions/notes", discussion: discussion
- if discussion.resolved?
.discussion-headline-light.js-discussion-headline
Resolved
- if discussion.resolved_by
by
= link_to_member(@project, discussion.resolved_by, avatar: false)
= time_ago_with_tooltip(discussion.resolved_at, placement: "bottom")
- elsif discussion.last_updated_at != discussion.created_at
.discussion-headline-light.js-discussion-headline
Last updated
- if discussion.last_updated_by
by
= link_to_member(@project, discussion.last_updated_by, avatar: false)
= time_ago_with_tooltip(discussion.last_updated_at, placement: "bottom")
- discussion = local_assigns.fetch(:discussion, nil)
- if current_user
%jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" }
.btn-group{ role: "group", "v-show" => "!allResolved", "v-if" => "showButton" }
%button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion",
title: "Jump to next unresolved discussion",
"aria-label" => "Jump to next unresolved discussion",
data: { container: "body" } }
= custom_icon("next_discussion")
.panel.panel-default
.notes{ data: { discussion_id: discussion.id } }
%ul.notes.timeline
%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)
.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
= render "discussions/jump_to_next", discussion: discussion
- else
= link_to_reply_discussion(discussion)
%tr.notes_holder
- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?)
%tr.notes_holder{class: ('hide' unless expanded)}
- if discussion_left
%td.notes_line.old
%td.notes_content.parallel.old
%ul.notes{ data: { discussion_id: discussion_left.id } }
= render partial: "projects/notes/note", collection: discussion_left.notes, as: :note
= link_to_reply_discussion(discussion_left, 'old')
.content{class: ('hide' unless discussion_left.expanded?)}
= render "discussions/notes", discussion: discussion_left, line_type: 'old'
- else
%td.notes_line.old= ""
%td.notes_content.parallel.old= ""
%td.notes_content.parallel.old
.content
- if discussion_right
%td.notes_line.new
%td.notes_content.parallel.new
%ul.notes{ data: { discussion_id: discussion_right.id } }
= render partial: "projects/notes/note", collection: discussion_right.notes, as: :note
= link_to_reply_discussion(discussion_right, 'new')
.content{class: ('hide' unless discussion_right.expanded?)}
= render "discussions/notes", discussion: discussion_right, line_type: 'new'
- else
%td.notes_line.new= ""
%td.notes_content.parallel.new= ""
%td.notes_content.parallel.new
.content
- if discussion.for_merge_request?
%resolve-discussion-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'",
":project-path" => "'#{discussion.project.path}'",
":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" }
= icon("spinner spin", "v-show" => "loading")
{{ buttonText }}
......@@ -75,8 +75,7 @@
- blob = diff_file.blob
- if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob)
%table.code.white
- diff_file.highlighted_diff_lines.each do |line|
= render "projects/diffs/line", line: line, diff_file: diff_file, plain: true, email: true
= render partial: "projects/diffs/line", collection: diff_file.highlighted_diff_lines, as: :line, locals: { diff_file: diff_file, plain: true, email: true }
- else
No preview for this file type
%br
%p
All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{@resolved_by.name}
All discussions on Merge Request <%= @merge_request.to_reference %> were resolved by <%= @resolved_by.name %>
<%= url_for(namespace_project_merge_request_url(@merge_request.target_project.namespace, @merge_request.target_project, @merge_request)) %>
- email = local_assigns.fetch(:email, false)
- plain = local_assigns.fetch(:plain, false)
- type = line.type
- line_code = diff_file.line_code(line) unless plain
- line_code = diff_file.line_code(line)
%tr.line_holder{ plain ? { class: type} : { class: type, id: line_code } }
- case type
- when 'match'
......@@ -28,3 +28,10 @@
%pre= diff_line_content(line.text, type)
- else
= diff_line_content(line.text, type)
- discussions = local_assigns.fetch(:discussions, nil)
- if discussions && !line.meta?
- discussion = discussions[line_code]
- if discussion
- discussion_expanded = local_assigns.fetch(:discussion_expanded, discussion.expanded?)
= render "discussions/diff_discussion", discussion: discussion, expanded: discussion_expanded
......@@ -5,15 +5,12 @@
%table.text-file.code.js-syntax-highlight{ data: diff_view_data, class: too_big ? 'hide' : '' }
- last_line = 0
- diff_file.highlighted_diff_lines.each do |line|
- last_line = line.new_pos
= render "projects/diffs/line", line: line, diff_file: diff_file
- unless @diff_notes_disabled
- line_code = diff_file.line_code(line)
- discussion = @grouped_diff_discussions[line_code] if line_code
- if discussion
= render "discussions/diff_discussion", discussion: discussion
- discussions = @grouped_diff_discussions unless @diff_notes_disabled
= render partial: "projects/diffs/line",
collection: diff_file.highlighted_diff_lines,
as: :line,
locals: { diff_file: diff_file, discussions: discussions }
- last_line = diff_file.highlighted_diff_lines.last.new_pos
- if !diff_file.new_file && last_line > 0
= diff_match_line last_line, last_line, bottom: true
......@@ -4,5 +4,8 @@
= 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.closed?
= 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"}
%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: { namespace_path: "#{@merge_request.project.namespace.path}", project_path: "#{@merge_request.project.path}" } }
{{ buttonText }}
#notes= render "projects/notes/notes_with_form"
- page_title "#{@merge_request.title} (#{@merge_request.to_reference})", "Merge Requests"
- page_description @merge_request.description
- page_card_attributes @merge_request.card_attributes
- content_for :page_specific_javascripts do
= page_specific_javascript_tag('diff_notes/diff_notes_bundle.js')
- if diff_view == :parallel
- fluid_layout true
......@@ -65,8 +67,18 @@
= link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: { target: 'div#diffs', action: 'diffs', toggle: 'tab' } do
Changes
%span.badge= @merge_request.diff_size
%li#resolve-count-app.line-resolve-all-container.pull-right.prepend-top-10.hidden-xs{ "v-cloak" => true }
%resolve-count{ "inline-template" => true, ":logged-out" => "#{current_user.nil?}" }
.line-resolve-all{ "v-show" => "discussionCount > 0",
":class" => "{ 'has-next-btn': !loggedOut && resolvedDiscussionCount !== discussionCount }" }
%span.line-resolve-btn.is-disabled{ type: "button",
":class" => "{ 'is-active': resolvedDiscussionCount === discussionCount }" }
= render "shared/icons/icon_status_success.svg"
%span.line-resolve-text
{{ resolvedDiscussionCount }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved
= render "discussions/jump_to_next"
.tab-content
.tab-content#diff-notes-app
#notes.notes.tab-pane.voting_notes
.content-block.content-block-small.oneline-block
= render 'award_emoji/awards_block', awardable: @merge_request, inline: true
......
= form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new-note js-new-note-form js-quick-submit common-note-form" }, authenticity_token: true do |f|
= form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new-note js-new-note-form js-quick-submit common-note-form", "data-noteable-iid" => @note.noteable.try(:iid), }, authenticity_token: true do |f|
= hidden_field_tag :view, diff_view
= hidden_field_tag :line_type
= note_target_fields(@note)
......
- return unless note.author
- return if note.cross_reference_not_visible_for?(current_user)
- can_resolve = can?(current_user, :resolve_note, note)
- note_editable = note_editable?(note)
%li.timeline-entry{ id: dom_id(note), class: ["note", "note-row-#{note.id}", ('system-note' if note.system)], data: {author_id: note.author.id, editable: note_editable} }
......@@ -16,14 +17,43 @@
commented
%a{ href: "##{dom_id(note)}" }
= time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
- unless note.system?
.note-actions
- access = note_max_access_for_user(note)
- if access and not note.system
- if access
%span.note-role.hidden-xs= access
- if current_user and not note.system
- if note.resolvable?
%resolve-btn{ ":namespace-path" => "'#{note.project.namespace.path}'",
":project-path" => "'#{note.project.path}'",
":discussion-id" => "'#{note.discussion_id}'",
":note-id" => note.id,
":resolved" => note.resolved?,
":can-resolve" => can_resolve,
":resolved-by" => "'#{note.resolved_by.try(:name)}'",
"v-show" => "#{can_resolve || note.resolved?}",
"inline-template" => true,
"v-ref:note_#{note.id}" => true }
.note-action-button
= icon("spin spinner", "v-show" => "loading")
%button.line-resolve-btn{ type: "button",
class: ("is-disabled" unless can_resolve),
":class" => "{ 'is-active': isResolved }",
":aria-label" => "buttonText",
"@click" => "resolve",
":title" => "buttonText",
"v-show" => "!loading",
"v-el:button" => true }
= render "shared/icons/icon_status_success.svg"
- if current_user
- if note.emoji_awardable?
= link_to '#', title: 'Award Emoji', class: 'note-action-button note-emoji-button js-add-award js-note-emoji', data: { position: 'right' } do
= icon('spinner spin')
= icon('smile-o')
- if note_editable
= link_to '#', title: 'Edit comment', class: 'note-action-button js-note-edit' do
= icon('pencil')
......
<svg viewBox="0 0 20 19" ><path d="M15.21 7.783h-3.317c-.268 0-.472.218-.472.486v.953c0 .28.212.486.473.486h3.318v1.575c0 .36.233.452.52.23l3.06-2.37c.274-.213.286-.582 0-.804l-3.06-2.37c-.275-.213-.52-.12-.52.23v1.583zm.57-3.66c-1.558-1.22-3.783-1.98-6.254-1.98C4.816 2.143 1 4.91 1 8.333c0 1.964 1.256 3.715 3.216 4.846-.447 1.615-1.132 2.195-1.732 2.882-.142.174-.304.32-.256.56v.01c.047.213.218.368.41.368h.046c.37-.048.743-.116 1.085-.213 1.645-.425 3.13-1.22 4.377-2.34.447.048.913.077 1.38.077 2.092 0 4.01-.546 5.492-1.454-.416-.208-.798-.475-1.134-.792-1.227.63-2.743 1.008-4.36 1.008-.41 0-.828-.03-1.237-.078l-.543-.058-.41.368c-.78.696-1.655 1.248-2.616 1.654.248-.445.486-.977.667-1.664l.257-.928-.828-.484c-1.646-.948-2.598-2.32-2.598-3.763 0-2.69 3.35-4.952 7.308-4.952 1.893 0 3.647.518 4.962 1.353.393-.266.827-.473 1.29-.61z" /></svg>
......@@ -85,6 +85,7 @@ module Gitlab
config.assets.precompile << "users/users_bundle.js"
config.assets.precompile << "network/network_bundle.js"
config.assets.precompile << "profile/profile_bundle.js"
config.assets.precompile << "diff_notes/diff_notes_bundle.js"
config.assets.precompile << "boards/boards_bundle.js"
config.assets.precompile << "boards/test_utils/simulate_drag.js"
config.assets.precompile << "lib/utils/*.js"
......
......@@ -744,6 +744,13 @@ Rails.application.routes.draw do
get :update_branches
get :diff_for_path
end
resources :discussions, only: [], constraints: { id: /\h{40}/ } do
member do
post :resolve
delete :resolve, action: :unresolve
end
end
end
resources :branches, only: [:index, :new, :create, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
......@@ -853,6 +860,8 @@ Rails.application.routes.draw do
member do
post :toggle_award_emoji
delete :delete_attachment
post :resolve
delete :resolve, action: :unresolve
end
end
......
class AddResolvedToNotes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :notes, :resolved_at, :datetime
add_column :notes, :resolved_by_id, :integer
end
end
# See http://doc.gitlab.com/ce/development/migration_style_guide.html
# for more information on how to write migrations for GitLab.
class AddDiscussionIdsToNotes < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def change
add_column :notes, :discussion_id, :string
add_column :notes, :original_discussion_id, :string
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20160816161312) do
ActiveRecord::Schema.define(version: 20160817154936) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -690,6 +690,10 @@ ActiveRecord::Schema.define(version: 20160816161312) do
t.string "type"
t.text "position"
t.text "original_position"
t.datetime "resolved_at"
t.integer "resolved_by_id"
t.string "discussion_id"
t.string "original_discussion_id"
end
add_index "notes", ["author_id"], name: "index_notes_on_author_id", using: :btree
......
# Merge Request discussion resolution
> [Introduced][ce-5022] in GitLab 8.11.
Discussion resolution helps keep track of progress during code review.
Resolving comments prevents you from forgetting to address feedback and lets you
hide discussions that are no longer relevant.
!["A discussion between two people on a piece of code"][discussion-view]
Comments and discussions can be resolved by anyone with at least Developer
access to the project, as well as by the author of the merge request.
## Marking a comment or discussion as resolved
You can mark a discussion as resolved by clicking the "Resolve discussion"
button at the bottom of the discussion.
!["Resolve discussion" button][resolve-discussion-button]
Alternatively, you can mark each comment as resolved individually.
!["Resolve comment" button][resolve-comment-button]
## Jumping between unresolved discussions
When a merge request has a large number of comments it can be difficult to track
what remains unresolved. You can jump between unresolved discussions with the
Jump button next to the Reply field on a discussion.
You can also jump to the first unresolved discussion from the button next to the
resolved discussions tracker.
!["3/4 discussions resolved"][discussions-resolved]
[ce-5022]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5022
[resolve-discussion-button]: img/resolve_discussion_button.png
[resolve-comment-button]: img/resolve_comment_button.png
[discussion-view]: img/discussion_view.png
[discussions-resolved]: img/discussions_resolved.png
require 'spec_helper'
describe Projects::DiscussionsController do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project) }
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
let(:discussion) { note.discussion }
let(:request_params) do
{
namespace_id: project.namespace,
project_id: project,
merge_request_id: merge_request,
id: note.discussion_id
}
end
describe 'POST resolve' do
before do
sign_in user
end
context "when the user is not authorized to resolve the discussion" do
it "returns status 404" do
post :resolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the user is authorized to resolve the discussion" do
before do
project.team << [user, :developer]
end
context "when the discussion is not resolvable" do
before do
note.update(system: true)
end
it "returns status 404" do
post :resolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the discussion is resolvable" do
it "resolves the discussion" do
post :resolve, request_params
expect(note.reload.discussion.resolved?).to be true
expect(note.reload.discussion.resolved_by).to eq(user)
end
it "sends notifications if all discussions are resolved" do
expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
post :resolve, request_params
end
it "returns the name of the resolving user" do
post :resolve, request_params
expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
end
it "returns status 200" do
post :resolve, request_params
expect(response).to have_http_status(200)
end
end
end
end
describe 'DELETE unresolve' do
before do
sign_in user
note.discussion.resolve!(user)
end
context "when the user is not authorized to resolve the discussion" do
it "returns status 404" do
delete :unresolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the user is authorized to resolve the discussion" do
before do
project.team << [user, :developer]
end
context "when the discussion is not resolvable" do
before do
note.update(system: true)
end
it "returns status 404" do
delete :unresolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the discussion is resolvable" do
it "unresolves the discussion" do
delete :unresolve, request_params
expect(note.reload.discussion.resolved?).to be false
end
it "returns status 200" do
delete :unresolve, request_params
expect(response).to have_http_status(200)
end
end
end
end
end
require('spec_helper')
require 'spec_helper'
describe Projects::NotesController do
let(:user) { create(:user) }
......@@ -6,7 +6,15 @@ describe Projects::NotesController do
let(:issue) { create(:issue, project: project) }
let(:note) { create(:note, noteable: issue, project: project) }
describe 'POST #toggle_award_emoji' do
let(:request_params) do
{
namespace_id: project.namespace,
project_id: project,
id: note
}
end
describe 'POST toggle_award_emoji' do
before do
sign_in(user)
project.team << [user, :developer]
......@@ -14,23 +22,132 @@ describe Projects::NotesController do
it "toggles the award emoji" do
expect do
post(:toggle_award_emoji, namespace_id: project.namespace.path,
project_id: project.path, id: note.id, name: "thumbsup")
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
end.to change { note.award_emoji.count }.by(1)
expect(response).to have_http_status(200)
end
it "removes the already awarded emoji" do
post(:toggle_award_emoji, namespace_id: project.namespace.path,
project_id: project.path, id: note.id, name: "thumbsup")
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
expect do
post(:toggle_award_emoji, namespace_id: project.namespace.path,
project_id: project.path, id: note.id, name: "thumbsup")
post(:toggle_award_emoji, request_params.merge(name: "thumbsup"))
end.to change { AwardEmoji.count }.by(-1)
expect(response).to have_http_status(200)
end
end
describe "resolving and unresolving" do
let(:merge_request) { create(:merge_request, source_project: project) }
let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
describe 'POST resolve' do
before do
sign_in user
end
context "when the user is not authorized to resolve the note" do
it "returns status 404" do
post :resolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the user is authorized to resolve the note" do
before do
project.team << [user, :developer]
end
context "when the note is not resolvable" do
before do
note.update(system: true)
end
it "returns status 404" do
post :resolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the note is resolvable" do
it "resolves the note" do
post :resolve, request_params
expect(note.reload.resolved?).to be true
expect(note.reload.resolved_by).to eq(user)
end
it "sends notifications if all discussions are resolved" do
expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
post :resolve, request_params
end
it "returns the name of the resolving user" do
post :resolve, request_params
expect(JSON.parse(response.body)["resolved_by"]).to eq(user.name)
end
it "returns status 200" do
post :resolve, request_params
expect(response).to have_http_status(200)
end
end
end
end
describe 'DELETE unresolve' do
before do
sign_in user
note.resolve!(user)
end
context "when the user is not authorized to resolve the note" do
it "returns status 404" do
delete :unresolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the user is authorized to resolve the note" do
before do
project.team << [user, :developer]
end
context "when the note is not resolvable" do
before do
note.update(system: true)
end
it "returns status 404" do
delete :unresolve, request_params
expect(response).to have_http_status(404)
end
end
context "when the note is resolvable" do
it "unresolves the note" do
delete :unresolve, request_params
expect(note.reload.resolved?).to be false
end
it "returns status 200" do
delete :unresolve, request_params
expect(response).to have_http_status(200)
end
end
end
end
end
end
This diff is collapsed.
//= require vue
//= require diff_notes/models/discussion
//= require diff_notes/models/note
//= require diff_notes/stores/comments
(() => {
function createDiscussion(noteId = 1, resolved = true) {
CommentsStore.create('a', noteId, true, resolved, 'test');
};
beforeEach(() => {
CommentsStore.state = {};
});
describe('New discussion', () => {
it('creates new discussion', () => {
expect(Object.keys(CommentsStore.state).length).toBe(0);
createDiscussion();
expect(Object.keys(CommentsStore.state).length).toBe(1);
});
it('creates new note in discussion', () => {
createDiscussion();
createDiscussion(2);
const discussion = CommentsStore.state['a'];
expect(Object.keys(discussion.notes).length).toBe(2);
});
});
describe('Get note', () => {
beforeEach(() => {
expect(Object.keys(CommentsStore.state).length).toBe(0);
createDiscussion();
});
it('gets note by ID', () => {
const note = CommentsStore.get('a', 1);
expect(note).toBeDefined();
expect(note.id).toBe(1);
});
});
describe('Delete discussion', () => {
beforeEach(() => {
expect(Object.keys(CommentsStore.state).length).toBe(0);
createDiscussion();
});
it('deletes discussion by ID', () => {
CommentsStore.delete('a', 1);
expect(Object.keys(CommentsStore.state).length).toBe(0);
});
it('deletes discussion when no more notes', () => {
createDiscussion();
createDiscussion(2);
expect(Object.keys(CommentsStore.state).length).toBe(1);
expect(Object.keys(CommentsStore.state['a'].notes).length).toBe(2);
CommentsStore.delete('a', 1);
CommentsStore.delete('a', 2);
expect(Object.keys(CommentsStore.state).length).toBe(0);
});
});
describe('Update note', () => {
beforeEach(() => {
expect(Object.keys(CommentsStore.state).length).toBe(0);
createDiscussion();
});
it('updates note to be unresolved', () => {
CommentsStore.update('a', 1, false, 'test');
const note = CommentsStore.get('a', 1);
expect(note.resolved).toBe(false);
});
});
describe('Discussion resolved', () => {
beforeEach(() => {
expect(Object.keys(CommentsStore.state).length).toBe(0);
createDiscussion();
});
it('is resolved with single note', () => {
const discussion = CommentsStore.state['a'];
expect(discussion.isResolved()).toBe(true);
});
it('is unresolved with 2 notes', () => {
const discussion = CommentsStore.state['a'];
createDiscussion(2, false);
console.log(discussion.isResolved());
expect(discussion.isResolved()).toBe(false);
});
it('is resolved with 2 notes', () => {
const discussion = CommentsStore.state['a'];
createDiscussion(2);
expect(discussion.isResolved()).toBe(true);
});
it('resolve all notes', () => {
const discussion = CommentsStore.state['a'];
createDiscussion(2, false);
discussion.resolveAllNotes();
expect(discussion.isResolved()).toBe(true);
});
it('unresolve all notes', () => {
const discussion = CommentsStore.state['a'];
createDiscussion(2);
discussion.unResolveAllNotes();
expect(discussion.isResolved()).toBe(false);
});
});
})();
require 'spec_helper'
require 'email_spec'
require 'mailers/shared/notify'
describe Notify, "merge request notifications" do
include EmailSpec::Matchers
describe "#resolved_all_discussions_email" do
let(:user) { create(:user) }
let(:merge_request) { create(:merge_request) }
let(:current_user) { create(:user) }
subject { Notify.resolved_all_discussions_email(user.id, merge_request.id, current_user.id) }
it "includes the name of the resolver" do
expect(subject).to have_body_text current_user.name
end
end
end
......@@ -103,7 +103,7 @@ describe DiffNote, models: true do
describe "#active?" do
context "when noteable is a commit" do
subject { create(:diff_note_on_commit, project: project, position: position) }
subject { build(:diff_note_on_commit, project: project, position: position) }
it "returns true" do
expect(subject.active?).to be true
......@@ -188,4 +188,300 @@ describe DiffNote, models: true do
end
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) }
context "when it is newly created" do
it "has a discussion id" do
expect(note.discussion_id).not_to be_nil
expect(note.discussion_id).to match(/\A\h{40}\z/)
end
end
context "when it didn't store a discussion id before" do
before do
note.update_column(:discussion_id, nil)
end
it "has a discussion id" do
# The discussion_id is set in `after_initialize`, so `reload` won't work
reloaded_note = Note.find(note.id)
expect(reloaded_note.discussion_id).not_to be_nil
expect(reloaded_note.discussion_id).to match(/\A\h{40}\z/)
end
end
end
describe "#original_discussion_id" do
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
This diff is collapsed.
......@@ -73,4 +73,29 @@ describe LegacyDiffNote, models: true do
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
......@@ -784,6 +784,98 @@ 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 "#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
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|
......
require 'spec_helper'
describe Note, models: true do
include RepoHelpers
describe 'associations' do
it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:noteable).touch(true) }
......@@ -267,4 +269,81 @@ describe Note, models: true do
expect(note.participants).to include(note.author)
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_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(: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
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
subject { merge_request.notes.grouped_diff_discussions }
it "includes active discussions" do
discussions = subject.values
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(subject.values.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
end
end
end
require 'spec_helper'
describe MergeRequests::ResolvedDiscussionNotificationService, services: true do
let(:merge_request) { create(:merge_request) }
let(:user) { create(:user) }
let(:project) { merge_request.project }
subject { described_class.new(project, user) }
describe "#execute" do
context "when not all discussions are resolved" do
before do
allow(merge_request).to receive(:discussions_resolved?).and_return(false)
end
it "doesn't add a system note" do
expect(SystemNoteService).not_to receive(:resolve_all_discussions)
subject.execute(merge_request)
end
it "doesn't send a notification email" do
expect_any_instance_of(NotificationService).not_to receive(:resolve_all_discussions)
subject.execute(merge_request)
end
end
context "when all discussions are resolved" do
before do
allow(merge_request).to receive(:discussions_resolved?).and_return(true)
end
it "adds a system note" do
expect(SystemNoteService).to receive(:resolve_all_discussions).with(merge_request, project, user)
subject.execute(merge_request)
end
it "sends a notification email" do
expect_any_instance_of(NotificationService).to receive(:resolve_all_discussions).with(merge_request, user)
subject.execute(merge_request)
end
end
end
end
......@@ -1042,6 +1042,52 @@ describe NotificationService, services: true do
end
end
end
describe "#resolve_all_discussions" do
it do
notification.resolve_all_discussions(merge_request, @u_disabled)
should_email(merge_request.assignee)
should_email(@u_watcher)
should_email(@u_participant_mentioned)
should_email(@subscriber)
should_email(@watcher_and_subscriber)
should_email(@u_guest_watcher)
should_not_email(@unsubscriber)
should_not_email(@u_participating)
should_not_email(@u_disabled)
should_not_email(@u_lazy_participant)
end
context 'participating' do
context 'by assignee' do
before do
merge_request.update_attribute(:assignee, @u_lazy_participant)
notification.resolve_all_discussions(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
context 'by note' do
let!(:note) { create(:note_on_issue, noteable: merge_request, project_id: project.id, note: 'anything', author: @u_lazy_participant) }
before { notification.resolve_all_discussions(merge_request, @u_disabled) }
it { should_email(@u_lazy_participant) }
end
context 'by author' do
before do
merge_request.author = @u_lazy_participant
merge_request.save
notification.resolve_all_discussions(merge_request, @u_disabled)
end
it { should_email(@u_lazy_participant) }
end
end
end
end
describe 'Projects' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment