diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-08-19 05:24:34 +0300 |
---|---|---|
committer | Ruben Davila <rdavila84@gmail.com> | 2016-08-19 09:22:27 +0300 |
commit | 78c5927031b8181e4fa3824676a552ebfaf42bab (patch) | |
tree | cb8d953af9d71e8c0ceaff43f4e5055aa722f74b | |
parent | d02b168979a35ca48ffe843a3adc92d58f394d9b (diff) |
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 4a13aa9
- [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
76 files changed, 3629 insertions, 137 deletions
diff --git a/CHANGELOG b/CHANGELOG index 21737738558..41c065cf7b7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -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' diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 2b331546e4b..4cc5af76e47 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -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'); diff --git a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es6 b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es6 new file mode 100644 index 00000000000..48bc7d77805 --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es6 @@ -0,0 +1,49 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 new file mode 100644 index 00000000000..ad80d1118df --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 @@ -0,0 +1,188 @@ +(() => { + 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); +})(); diff --git a/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 new file mode 100644 index 00000000000..be6ebc77947 --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 @@ -0,0 +1,107 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 new file mode 100644 index 00000000000..9e383b14a3e --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 @@ -0,0 +1,18 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6 new file mode 100644 index 00000000000..e373b06b1eb --- /dev/null +++ b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6 @@ -0,0 +1,60 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 new file mode 100644 index 00000000000..22d9cf6c857 --- /dev/null +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 @@ -0,0 +1,35 @@ +//= 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 + } + }); +}); diff --git a/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 b/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 new file mode 100644 index 00000000000..a05f885201d --- /dev/null +++ b/app/assets/javascripts/diff_notes/mixins/discussion.js.es6 @@ -0,0 +1,35 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/mixins/namespace.js.es6 b/app/assets/javascripts/diff_notes/mixins/namespace.js.es6 new file mode 100644 index 00000000000..d278678085b --- /dev/null +++ b/app/assets/javascripts/diff_notes/mixins/namespace.js.es6 @@ -0,0 +1,9 @@ +((w) => { + w.ButtonMixins = { + computed: { + namespace: function () { + return `${this.namespacePath}/${this.projectPath}`; + } + } + }; +})(window); diff --git a/app/assets/javascripts/diff_notes/models/discussion.js.es6 b/app/assets/javascripts/diff_notes/models/discussion.js.es6 new file mode 100644 index 00000000000..488714e4870 --- /dev/null +++ b/app/assets/javascripts/diff_notes/models/discussion.js.es6 @@ -0,0 +1,87 @@ +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; + } +} diff --git a/app/assets/javascripts/diff_notes/models/note.js.es6 b/app/assets/javascripts/diff_notes/models/note.js.es6 new file mode 100644 index 00000000000..f2d2d389c38 --- /dev/null +++ b/app/assets/javascripts/diff_notes/models/note.js.es6 @@ -0,0 +1,9 @@ +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; + } +} diff --git a/app/assets/javascripts/diff_notes/services/resolve.js.es6 b/app/assets/javascripts/diff_notes/services/resolve.js.es6 new file mode 100644 index 00000000000..de771ff814b --- /dev/null +++ b/app/assets/javascripts/diff_notes/services/resolve.js.es6 @@ -0,0 +1,88 @@ +((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); diff --git a/app/assets/javascripts/diff_notes/stores/comments.js.es6 b/app/assets/javascripts/diff_notes/stores/comments.js.es6 new file mode 100644 index 00000000000..69522e1dac5 --- /dev/null +++ b/app/assets/javascripts/diff_notes/stores/comments.js.es6 @@ -0,0 +1,53 @@ +((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); diff --git a/app/assets/javascripts/merge_request.js b/app/assets/javascripts/merge_request.js index 47e6dd1084d..56ebf84c4f6 100644 --- a/app/assets/javascripts/merge_request.js +++ b/app/assets/javascripts/merge_request.js @@ -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'); } diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 4e2273f5aa8..ad08209d61e 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -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(); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 2484a07f363..d0d5cad813a 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -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; })(); diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js index b9ae497b0e5..156b9b8abec 100644 --- a/app/assets/javascripts/single_file_diff.js +++ b/app/assets/javascripts/single_file_diff.js @@ -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)); }; diff --git a/app/assets/stylesheets/behaviors.scss b/app/assets/stylesheets/behaviors.scss index a6b9efc49c9..897bc49e7df 100644 --- a/app/assets/stylesheets/behaviors.scss +++ b/app/assets/stylesheets/behaviors.scss @@ -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; } diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 3784010348a..bd875b9823f 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -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; } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index a2b5437e503..08d1692c888 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -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; + } + } +} diff --git a/app/controllers/projects/discussions_controller.rb b/app/controllers/projects/discussions_controller.rb new file mode 100644 index 00000000000..b2e8733ccb7 --- /dev/null +++ b/app/controllers/projects/discussions_controller.rb @@ -0,0 +1,43 @@ +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 diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 0f52a5f96a3..d3fe441c4d2 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -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), diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index f2422729364..0948ad21649 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -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, diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index e12a1052988..de13e7a1fc2 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -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 diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 26bde2230a9..da230f76bae 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -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,10 +81,8 @@ 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 + 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) diff --git a/app/mailers/emails/merge_requests.rb b/app/mailers/emails/merge_requests.rb index 95810b0ac6e..ec27ac517db 100644 --- a/app/mailers/emails/merge_requests.rb +++ b/app/mailers/emails/merge_requests.rb @@ -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) diff --git a/app/models/ability.rb b/app/models/ability.rb index 55265c3cfcb..07f703f205d 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -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 diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index e02a3d54c36..f56c3d74ae3 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -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? diff --git a/app/models/discussion.rb b/app/models/discussion.rb index e2218a5f02b..3fddc084af2 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,7 +1,7 @@ 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 diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index 6ed66001513..8e26cbe9835 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 4304ef04767..5330a07ee35 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -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), diff --git a/app/models/note.rb b/app/models/note.rb index ddcd7f9d034..3bbf5db0b70 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -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) - end + 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 diff --git a/app/services/merge_requests/resolved_discussion_notification_service.rb b/app/services/merge_requests/resolved_discussion_notification_service.rb new file mode 100644 index 00000000000..3a09350c847 --- /dev/null +++ b/app/services/merge_requests/resolved_discussion_notification_service.rb @@ -0,0 +1,10 @@ +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 diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 2291bc0f127..66a838b3d13 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -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 diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index e13dc9265b8..546a8f11330 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -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` diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml index fa1ad9efa73..1411daeb4a6 100644 --- a/app/views/discussions/_diff_discussion.html.haml +++ b/app/views/discussions/_diff_discussion.html.haml @@ -1,6 +1,6 @@ -%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 diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 02b159ffd45..b2e55f7647a 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -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 } diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml index 49702e048aa..077e8e64e5f 100644 --- a/app/views/discussions/_discussion.html.haml +++ b/app/views/discussions/_discussion.html.haml @@ -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 - = render "discussions/notes", discussion: discussion + .panel.panel-default + = render "discussions/notes", discussion: discussion diff --git a/app/views/discussions/_headline.html.haml b/app/views/discussions/_headline.html.haml new file mode 100644 index 00000000000..c1dabeed387 --- /dev/null +++ b/app/views/discussions/_headline.html.haml @@ -0,0 +1,14 @@ +- 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") diff --git a/app/views/discussions/_jump_to_next.html.haml b/app/views/discussions/_jump_to_next.html.haml new file mode 100644 index 00000000000..69bd416c4de --- /dev/null +++ b/app/views/discussions/_jump_to_next.html.haml @@ -0,0 +1,9 @@ +- 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") diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index a2642b839f6..fbe470bed2c 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,5 +1,15 @@ -.panel.panel-default - .notes{ data: { discussion_id: discussion.id } } - %ul.notes.timeline - = render partial: "projects/notes/note", collection: discussion.notes, as: :note - = link_to_reply_discussion(discussion) +%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) diff --git a/app/views/discussions/_parallel_diff_discussion.html.haml b/app/views/discussions/_parallel_diff_discussion.html.haml index a798c438ea0..f1072ce0feb 100644 --- a/app/views/discussions/_parallel_diff_discussion.html.haml +++ b/app/views/discussions/_parallel_diff_discussion.html.haml @@ -1,22 +1,21 @@ -%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 diff --git a/app/views/discussions/_resolve_all.html.haml b/app/views/discussions/_resolve_all.html.haml new file mode 100644 index 00000000000..7a8767ddba0 --- /dev/null +++ b/app/views/discussions/_resolve_all.html.haml @@ -0,0 +1,11 @@ +- 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 }} diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index 4dc39a72225..c0c07d65daa 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -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 diff --git a/app/views/notify/resolved_all_discussions_email.html.haml b/app/views/notify/resolved_all_discussions_email.html.haml new file mode 100644 index 00000000000..522421b7cc3 --- /dev/null +++ b/app/views/notify/resolved_all_discussions_email.html.haml @@ -0,0 +1,2 @@ +%p + All discussions on Merge Request #{@merge_request.to_reference} were resolved by #{@resolved_by.name} diff --git a/app/views/notify/resolved_all_discussions_email.text.erb b/app/views/notify/resolved_all_discussions_email.text.erb new file mode 100644 index 00000000000..b0d380af8fc --- /dev/null +++ b/app/views/notify/resolved_all_discussions_email.text.erb @@ -0,0 +1,3 @@ +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)) %> diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index 891b2bd9802..7042e9f1fc9 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -1,7 +1,7 @@ - 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 diff --git a/app/views/projects/diffs/_text_file.html.haml b/app/views/projects/diffs/_text_file.html.haml index ab5463ba89d..f1d2d4bf268 100644 --- a/app/views/projects/diffs/_text_file.html.haml +++ b/app/views/projects/diffs/_text_file.html.haml @@ -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 diff --git a/app/views/projects/merge_requests/_discussion.html.haml b/app/views/projects/merge_requests/_discussion.html.haml index 53dd300c35c..d070979bcfe 100644 --- a/app/views/projects/merge_requests/_discussion.html.haml +++ b/app/views/projects/merge_requests/_discussion.html.haml @@ -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" diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index a1313064725..f8025fc1dbe 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -1,6 +1,8 @@ - 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 diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index 759c72b2477..402f5b52f5e 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -1,4 +1,4 @@ -= 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) diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 71da8ac9d7c..d2ac1ce2b9a 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -1,5 +1,6 @@ - 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,19 +17,48 @@ commented %a{ href: "##{dom_id(note)}" } = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago') - .note-actions - - access = note_max_access_for_user(note) - - if access and not note.system - %span.note-role.hidden-xs= access - - if current_user and not note.system - = 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') - = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'note-action-button hidden-xs js-note-delete danger' do - = icon('trash-o') + - unless note.system? + .note-actions + - access = note_max_access_for_user(note) + - if access + %span.note-role.hidden-xs= access + + - 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') + = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'note-action-button hidden-xs js-note-delete danger' do + = icon('trash-o') .note-body{class: note_editable ? 'js-task-list-container' : ''} .note-text.md = preserve do diff --git a/app/views/shared/icons/_next_discussion.svg b/app/views/shared/icons/_next_discussion.svg new file mode 100644 index 00000000000..43559a60cb0 --- /dev/null +++ b/app/views/shared/icons/_next_discussion.svg @@ -0,0 +1 @@ +<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> diff --git a/config/application.rb b/config/application.rb index 0c136623477..6b80f8ddafa 100644 --- a/config/application.rb +++ b/config/application.rb @@ -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" diff --git a/config/routes.rb b/config/routes.rb index 867f3dac50b..ff4b8f25cc7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -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 diff --git a/db/migrate/20160724205507_add_resolved_to_notes.rb b/db/migrate/20160724205507_add_resolved_to_notes.rb new file mode 100644 index 00000000000..b8ebcdbd156 --- /dev/null +++ b/db/migrate/20160724205507_add_resolved_to_notes.rb @@ -0,0 +1,10 @@ +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 diff --git a/db/migrate/20160817154936_add_discussion_ids_to_notes.rb b/db/migrate/20160817154936_add_discussion_ids_to_notes.rb new file mode 100644 index 00000000000..61facce665a --- /dev/null +++ b/db/migrate/20160817154936_add_discussion_ids_to_notes.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index d73c045c57c..1af6c805c24 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -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" @@ -684,12 +684,16 @@ ActiveRecord::Schema.define(version: 20160816161312) do t.string "line_code" t.string "commit_id" t.integer "noteable_id" - t.boolean "system", default: false, null: false + t.boolean "system", default: false, null: false t.text "st_diff" t.integer "updated_by_id" 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 diff --git a/doc/user/project/merge_requests/img/discussion_view.png b/doc/user/project/merge_requests/img/discussion_view.png Binary files differnew file mode 100644 index 00000000000..83bb60acce2 --- /dev/null +++ b/doc/user/project/merge_requests/img/discussion_view.png diff --git a/doc/user/project/merge_requests/img/discussions_resolved.png b/doc/user/project/merge_requests/img/discussions_resolved.png Binary files differnew file mode 100644 index 00000000000..85428129ac8 --- /dev/null +++ b/doc/user/project/merge_requests/img/discussions_resolved.png diff --git a/doc/user/project/merge_requests/img/resolve_comment_button.png b/doc/user/project/merge_requests/img/resolve_comment_button.png Binary files differnew file mode 100644 index 00000000000..2c4ab2f5d53 --- /dev/null +++ b/doc/user/project/merge_requests/img/resolve_comment_button.png diff --git a/doc/user/project/merge_requests/img/resolve_discussion_button.png b/doc/user/project/merge_requests/img/resolve_discussion_button.png Binary files differnew file mode 100644 index 00000000000..73f265bb101 --- /dev/null +++ b/doc/user/project/merge_requests/img/resolve_discussion_button.png diff --git a/doc/user/project/merge_requests/merge_request_discussion_resolution.md b/doc/user/project/merge_requests/merge_request_discussion_resolution.md new file mode 100644 index 00000000000..2559f5f5250 --- /dev/null +++ b/doc/user/project/merge_requests/merge_request_discussion_resolution.md @@ -0,0 +1,40 @@ +# 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 diff --git a/spec/controllers/projects/discussions_controller_spec.rb b/spec/controllers/projects/discussions_controller_spec.rb new file mode 100644 index 00000000000..ff617fea847 --- /dev/null +++ b/spec/controllers/projects/discussions_controller_spec.rb @@ -0,0 +1,125 @@ +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 diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb index 75590c1ed4f..92e38b02615 100644 --- a/spec/controllers/projects/notes_controller_spec.rb +++ b/spec/controllers/projects/notes_controller_spec.rb @@ -1,4 +1,4 @@ -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 diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb new file mode 100644 index 00000000000..c6adf7e4c56 --- /dev/null +++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb @@ -0,0 +1,497 @@ +require 'spec_helper' + +feature 'Diff notes resolve', feature: true, js: true do + let(:user) { create(:user) } + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request_with_diffs, source_project: project, author: user, title: "Bug NS-04") } + let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request) } + let(:path) { "files/ruby/popen.rb" } + let(:position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + old_line: nil, + new_line: 9, + diff_refs: merge_request.diff_refs + ) + end + + context 'no discussions' do + before do + project.team << [user, :master] + login_as user + note.destroy + visit_merge_request + end + + it 'displays no discussion resolved data' do + expect(page).not_to have_content('discussion resolved') + expect(page).not_to have_selector('.discussion-next-btn') + end + end + + context 'as authorized user' do + before do + project.team << [user, :master] + login_as user + visit_merge_request + end + + context 'single discussion' do + it 'shows text with how many discussions' do + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'allows user to mark a note as resolved' do + page.within '.diff-content .note' do + find('.line-resolve-btn').click + + expect(page).to have_selector('.line-resolve-btn.is-active') + expect(find('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") + end + + page.within '.diff-content' do + expect(page).to have_selector('.btn', text: 'Unresolve discussion') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to mark discussion as resolved' do + page.within '.diff-content' do + click_button 'Resolve discussion' + end + + page.within '.diff-content .note' do + expect(page).to have_selector('.line-resolve-btn.is-active') + + expect(find('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to unresolve discussion' do + page.within '.diff-content' do + click_button 'Resolve discussion' + click_button 'Unresolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'hides resolved discussion' do + page.within '.diff-content' do + click_button 'Resolve discussion' + end + + visit_merge_request + + expect(page).to have_selector('.discussion-body', visible: false) + end + + it 'allows user to resolve from reply form without a comment' do + page.within '.diff-content' do + click_button 'Reply...' + + click_button 'Resolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to unresolve from reply form without a comment' do + page.within '.diff-content' do + click_button 'Resolve discussion' + sleep 1 + + click_button 'Reply...' + + click_button 'Unresolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + expect(page).not_to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to comment & resolve discussion' do + page.within '.diff-content' do + click_button 'Reply...' + + find('.js-note-text').set 'testing' + + click_button 'Comment & resolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to comment & unresolve discussion' do + page.within '.diff-content' do + click_button 'Resolve discussion' + + click_button 'Reply...' + + find('.js-note-text').set 'testing' + + click_button 'Comment & unresolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'allows user to quickly scroll to next unresolved discussion' do + page.within '.line-resolve-all-container' do + page.find('.discussion-next-btn').click + end + + expect(page.evaluate_script("$('body').scrollTop()")).to be > 0 + end + + it 'hides jump to next button when all resolved' do + page.within '.diff-content' do + click_button 'Resolve discussion' + end + + expect(page).to have_selector('.discussion-next-btn', visible: false) + end + + it 'updates updated text after resolving note' do + page.within '.diff-content .note' do + find('.line-resolve-btn').click + end + + expect(page).to have_content("Resolved by #{user.name}") + end + + it 'hides jump to next discussion button' do + page.within '.discussion-reply-holder' do + expect(page).not_to have_selector('.discussion-next-btn') + end + end + end + + context 'multiple notes' do + before do + create(:diff_note_on_merge_request, project: project, noteable: merge_request) + end + + it 'does not mark discussion as resolved when resolving single note' do + page.within '.diff-content .note' do + first('.line-resolve-btn').click + sleep 1 + expect(first('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") + end + + expect(page).to have_content('Last updated') + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'resolves discussion' do + page.all('.note').each do |note| + note.find('.line-resolve-btn').click + end + + expect(page).to have_content('Resolved by') + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + end + end + end + + context 'muliple discussions' do + before do + create(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) + visit_merge_request + end + + it 'shows text with how many discussions' do + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/2 discussions resolved') + end + end + + it 'allows user to mark a single note as resolved' do + click_button('Resolve discussion', match: :first) + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/2 discussions resolved') + end + end + + it 'allows user to mark all notes as resolved' do + page.all('.line-resolve-btn').each do |btn| + btn.click + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('2/2 discussions resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user user to mark all discussions as resolved' do + page.all('.discussion-reply-holder').each do |reply_holder| + page.within reply_holder do + click_button 'Resolve discussion' + end + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('2/2 discussions resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to quickly scroll to next unresolved discussion' do + page.within first('.discussion-reply-holder') do + click_button 'Resolve discussion' + end + + page.within '.line-resolve-all-container' do + page.find('.discussion-next-btn').click + end + + expect(page.evaluate_script("$('body').scrollTop()")).to be > 0 + end + + it 'updates updated text after resolving note' do + page.within first('.diff-content .note') do + find('.line-resolve-btn').click + end + + expect(page).to have_content("Resolved by #{user.name}") + end + + it 'shows jump to next discussion button' do + page.all('.discussion-reply-holder').each do |holder| + expect(holder).to have_selector('.discussion-next-btn') + end + end + + it 'displays next discussion even if hidden' do + page.all('.note-discussion').each do |discussion| + page.within discussion do + click_link 'Toggle discussion' + end + end + + page.within('.issuable-discussion #notes') do + expect(page).not_to have_selector('.btn', text: 'Resolve discussion') + end + + page.within '.line-resolve-all-container' do + page.find('.discussion-next-btn').click + end + + expect(find('.discussion-with-resolve-btn')).to have_selector('.btn', text: 'Resolve discussion') + end + end + + context 'changes tab' do + it 'shows text with how many discussions' do + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'allows user to mark a note as resolved' do + page.within '.diff-content .note' do + find('.line-resolve-btn').click + + expect(page).to have_selector('.line-resolve-btn.is-active') + end + + page.within '.diff-content' do + expect(page).to have_selector('.btn', text: 'Unresolve discussion') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to mark discussion as resolved' do + page.within '.diff-content' do + click_button 'Resolve discussion' + end + + page.within '.diff-content .note' do + expect(page).to have_selector('.line-resolve-btn.is-active') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to unresolve discussion' do + page.within '.diff-content' do + click_button 'Resolve discussion' + click_button 'Unresolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'allows user to comment & resolve discussion' do + page.within '.diff-content' do + click_button 'Reply...' + + find('.js-note-text').set 'testing' + + click_button 'Comment & resolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + + it 'allows user to comment & unresolve discussion' do + page.within '.diff-content' do + click_button 'Resolve discussion' + + click_button 'Reply...' + + find('.js-note-text').set 'testing' + + click_button 'Comment & unresolve discussion' + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + end + end + + context 'as a guest' do + let(:guest) { create(:user) } + + before do + project.team << [guest, :guest] + login_as guest + end + + context 'someone elses merge request' do + before do + visit_merge_request + end + + it 'does not allow user to mark note as resolved' do + page.within '.diff-content .note' do + expect(page).not_to have_selector('.line-resolve-btn') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + + it 'does not allow user to mark discussion as resolved' do + page.within '.diff-content .note' do + expect(page).not_to have_selector('.btn', text: 'Resolve discussion') + end + end + end + + context 'guest users merge request' do + before do + mr = create(:merge_request_with_diffs, source_project: project, source_branch: 'markdown', author: guest, title: "Bug") + create(:diff_note_on_merge_request, project: project, noteable: mr) + visit_merge_request(mr) + end + + it 'allows user to mark a note as resolved' do + page.within '.diff-content .note' do + find('.line-resolve-btn').click + + expect(page).to have_selector('.line-resolve-btn.is-active') + end + + page.within '.diff-content' do + expect(page).to have_selector('.btn', text: 'Unresolve discussion') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('1/1 discussion resolved') + expect(page).to have_selector('.line-resolve-btn.is-active') + end + end + end + end + + context 'unauthorized user' do + context 'no resolved comments' do + before do + visit_merge_request + end + + it 'does not allow user to mark note as resolved' do + page.within '.diff-content .note' do + expect(page).not_to have_selector('.line-resolve-btn') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + end + end + end + + context 'resolved comment' do + before do + note.resolve!(user) + visit_merge_request + end + + it 'shows resolved icon' do + expect(page).to have_content '1/1 discussion resolved' + + click_link 'Toggle discussion' + expect(page).to have_selector('.line-resolve-btn.is-active') + end + + it 'does not allow user to click resolve button' do + expect(page).to have_selector('.line-resolve-btn.is-disabled') + click_link 'Toggle discussion' + + expect(page).to have_selector('.line-resolve-btn.is-disabled') + end + end + end + + def visit_merge_request(mr = nil) + mr = mr || merge_request + visit namespace_project_merge_request_path(mr.project.namespace, mr.project, mr) + end +end diff --git a/spec/javascripts/diff_comments_store_spec.js.es6 b/spec/javascripts/diff_comments_store_spec.js.es6 new file mode 100644 index 00000000000..22293d4de87 --- /dev/null +++ b/spec/javascripts/diff_comments_store_spec.js.es6 @@ -0,0 +1,122 @@ +//= 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); + }); + }); +})(); diff --git a/spec/mailers/emails/merge_requests_spec.rb b/spec/mailers/emails/merge_requests_spec.rb new file mode 100644 index 00000000000..4d3811af254 --- /dev/null +++ b/spec/mailers/emails/merge_requests_spec.rb @@ -0,0 +1,19 @@ +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 diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 1fa96eb1f15..6a640474cfe 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -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 diff --git a/spec/models/discussion_spec.rb b/spec/models/discussion_spec.rb new file mode 100644 index 00000000000..179f2e73662 --- /dev/null +++ b/spec/models/discussion_spec.rb @@ -0,0 +1,615 @@ +require 'spec_helper' + +describe Discussion, model: true do + subject { described_class.new([first_note, second_note, third_note]) } + + let(:first_note) { create(:diff_note_on_merge_request) } + let(:second_note) { create(:diff_note_on_merge_request) } + let(:third_note) { create(:diff_note_on_merge_request) } + + 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) } + + 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 "calls resolve! on every resolvable note" do + expect(first_note).to receive(:resolve!).with(current_user) + expect(second_note).not_to receive(:resolve!) + expect(third_note).to receive(:resolve!).with(current_user) + + subject.resolve!(current_user) + 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 "calls resolve! on every resolvable note" do + expect(first_note).to receive(:resolve!).with(current_user) + expect(second_note).not_to receive(:resolve!) + expect(third_note).to receive(:resolve!).with(current_user) + + subject.resolve!(current_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.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.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.resolved? } + end + + it "sets resolved_at on the unresolved note" do + subject.resolve!(current_user) + + expect(third_note.resolved_at).not_to be_nil + end + + it "sets resolved_by on the unresolved note" do + subject.resolve!(current_user) + + expect(third_note.resolved_by).to eq(current_user) + end + + it "marks the unresolved note as resolved" do + subject.resolve!(current_user) + + 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 "calls resolve! on every resolvable note" do + expect(first_note).to receive(:resolve!).with(current_user) + expect(second_note).not_to receive(:resolve!) + expect(third_note).to receive(:resolve!).with(current_user) + + subject.resolve!(current_user) + end + + it "sets resolved_at on the unresolved notes" do + subject.resolve!(current_user) + + 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) + + 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) + + expect(first_note.resolved?).to be true + 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 + 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 "calls unresolve! on every resolvable note" do + expect(first_note).to receive(:unresolve!) + expect(second_note).not_to receive(:unresolve!) + expect(third_note).to receive(:unresolve!) + + subject.unresolve! + end + + it "unsets resolved_at on the resolved notes" do + subject.unresolve! + + 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! + + 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! + + expect(first_note.resolved?).to be false + expect(third_note.resolved?).to be false + 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 some resolvable notes are resolved" do + before do + first_note.resolve!(user) + end + + it "calls unresolve! on every resolvable note" do + expect(first_note).to receive(:unresolve!) + expect(second_note).not_to receive(:unresolve!) + expect(third_note).to receive(:unresolve!) + + subject.unresolve! + end + + it "unsets resolved_at on the resolved note" do + subject.unresolve! + + expect(first_note.resolved_at).to be_nil + end + + it "unsets resolved_by on the resolved note" do + subject.unresolve! + + expect(first_note.resolved_by).to be_nil + end + + it "unmarks the resolved note as resolved" do + subject.unresolve! + + expect(first_note.resolved?).to be false + end + end + + context "when no resolvable notes are resolved" do + it "calls unresolve! on every resolvable note" do + expect(first_note).to receive(:unresolve!) + expect(second_note).not_to receive(:unresolve!) + expect(third_note).to receive(:unresolve!) + + subject.unresolve! + end + end + 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 +end diff --git a/spec/models/legacy_diff_note_spec.rb b/spec/models/legacy_diff_note_spec.rb index 2cfd26419ca..81517a18b74 100644 --- a/spec/models/legacy_diff_note_spec.rb +++ b/spec/models/legacy_diff_note_spec.rb @@ -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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index f83dbefedc0..64c56d922ff 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -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| diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 53733d253f7..ef2747046b9 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -1,6 +1,8 @@ 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 diff --git a/spec/services/merge_requests/resolved_discussion_notification_service.rb b/spec/services/merge_requests/resolved_discussion_notification_service.rb new file mode 100644 index 00000000000..7ddd812e513 --- /dev/null +++ b/spec/services/merge_requests/resolved_discussion_notification_service.rb @@ -0,0 +1,46 @@ +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 diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 62c97e09288..18da3b1b453 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -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 |