summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-08-19 02:24:34 +0000
committerDouwe Maan <douwe@gitlab.com>2016-08-19 02:24:34 +0000
commit579951720afcfa7d2fbef6f3dc2d2ca0d2816025 (patch)
tree3ac0b583374e231a23ea8fc8af7424ac81946bc3
parentc5aa31c83145366d88ce6d8d91e68467cf5baed4 (diff)
parent10c5ec3e57d9a1a5a0477102dd7aa4cba0645b64 (diff)
downloadgitlab-ce-579951720afcfa7d2fbef6f3dc2d2ca0d2816025.tar.gz
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
-rw-r--r--CHANGELOG1
-rw-r--r--app/assets/javascripts/application.js9
-rw-r--r--app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es649
-rw-r--r--app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6188
-rw-r--r--app/assets/javascripts/diff_notes/components/resolve_btn.js.es6107
-rw-r--r--app/assets/javascripts/diff_notes/components/resolve_count.js.es618
-rw-r--r--app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es660
-rw-r--r--app/assets/javascripts/diff_notes/diff_notes_bundle.js.es635
-rw-r--r--app/assets/javascripts/diff_notes/mixins/discussion.js.es635
-rw-r--r--app/assets/javascripts/diff_notes/mixins/namespace.js.es69
-rw-r--r--app/assets/javascripts/diff_notes/models/discussion.js.es687
-rw-r--r--app/assets/javascripts/diff_notes/models/note.js.es69
-rw-r--r--app/assets/javascripts/diff_notes/services/resolve.js.es688
-rw-r--r--app/assets/javascripts/diff_notes/stores/comments.js.es653
-rw-r--r--app/assets/javascripts/merge_request.js2
-rw-r--r--app/assets/javascripts/merge_request_tabs.js6
-rw-r--r--app/assets/javascripts/notes.js95
-rw-r--r--app/assets/javascripts/single_file_diff.js16
-rw-r--r--app/assets/stylesheets/behaviors.scss6
-rw-r--r--app/assets/stylesheets/pages/note_form.scss26
-rw-r--r--app/assets/stylesheets/pages/notes.scss77
-rw-r--r--app/controllers/projects/discussions_controller.rb43
-rw-r--r--app/controllers/projects/merge_requests_controller.rb9
-rw-r--r--app/controllers/projects/notes_controller.rb34
-rw-r--r--app/helpers/appearances_helper.rb2
-rw-r--r--app/helpers/notes_helper.rb10
-rw-r--r--app/mailers/emails/merge_requests.rb7
-rw-r--r--app/models/ability.rb8
-rw-r--r--app/models/diff_note.rb72
-rw-r--r--app/models/discussion.rb83
-rw-r--r--app/models/legacy_diff_note.rb12
-rw-r--r--app/models/merge_request.rb26
-rw-r--r--app/models/note.rb55
-rw-r--r--app/services/merge_requests/resolved_discussion_notification_service.rb10
-rw-r--r--app/services/notification_service.rb8
-rw-r--r--app/services/system_note_service.rb6
-rw-r--r--app/views/discussions/_diff_discussion.html.haml8
-rw-r--r--app/views/discussions/_diff_with_notes.html.haml13
-rw-r--r--app/views/discussions/_discussion.html.haml23
-rw-r--r--app/views/discussions/_headline.html.haml14
-rw-r--r--app/views/discussions/_jump_to_next.html.haml9
-rw-r--r--app/views/discussions/_notes.html.haml20
-rw-r--r--app/views/discussions/_parallel_diff_discussion.html.haml21
-rw-r--r--app/views/discussions/_resolve_all.html.haml11
-rw-r--r--app/views/notify/repository_push_email.html.haml3
-rw-r--r--app/views/notify/resolved_all_discussions_email.html.haml2
-rw-r--r--app/views/notify/resolved_all_discussions_email.text.erb3
-rw-r--r--app/views/projects/diffs/_line.html.haml9
-rw-r--r--app/views/projects/diffs/_text_file.html.haml15
-rw-r--r--app/views/projects/merge_requests/_discussion.html.haml3
-rw-r--r--app/views/projects/merge_requests/_show.html.haml14
-rw-r--r--app/views/projects/notes/_form.html.haml2
-rw-r--r--app/views/projects/notes/_note.html.haml56
-rw-r--r--app/views/shared/icons/_next_discussion.svg1
-rw-r--r--config/application.rb1
-rw-r--r--config/routes.rb9
-rw-r--r--db/migrate/20160724205507_add_resolved_to_notes.rb10
-rw-r--r--db/migrate/20160817154936_add_discussion_ids_to_notes.rb13
-rw-r--r--db/schema.rb8
-rw-r--r--doc/user/project/merge_requests/img/discussion_view.pngbin0 -> 292754 bytes
-rw-r--r--doc/user/project/merge_requests/img/discussions_resolved.pngbin0 -> 12840 bytes
-rw-r--r--doc/user/project/merge_requests/img/resolve_comment_button.pngbin0 -> 14075 bytes
-rw-r--r--doc/user/project/merge_requests/img/resolve_discussion_button.pngbin0 -> 18405 bytes
-rw-r--r--doc/user/project/merge_requests/merge_request_discussion_resolution.md40
-rw-r--r--spec/controllers/projects/discussions_controller_spec.rb125
-rw-r--r--spec/controllers/projects/notes_controller_spec.rb133
-rw-r--r--spec/features/merge_requests/diff_notes_resolve_spec.rb497
-rw-r--r--spec/javascripts/diff_comments_store_spec.js.es6122
-rw-r--r--spec/mailers/emails/merge_requests_spec.rb19
-rw-r--r--spec/models/diff_note_spec.rb298
-rw-r--r--spec/models/discussion_spec.rb615
-rw-r--r--spec/models/legacy_diff_note_spec.rb25
-rw-r--r--spec/models/merge_request_spec.rb92
-rw-r--r--spec/models/note_spec.rb79
-rw-r--r--spec/services/merge_requests/resolved_discussion_notification_service.rb46
-rw-r--r--spec/services/notification_service_spec.rb46
76 files changed, 3629 insertions, 137 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 98638b8eb28..7ea2631f9f4 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -30,6 +30,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 f6e8e770527..a122fa2d637 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -225,10 +225,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 5c48bf233d6..66f77aee06a 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -749,6 +749,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 }
@@ -858,6 +865,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 fa56db332b3..82d4590f6b5 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
new file mode 100644
index 00000000000..83bb60acce2
--- /dev/null
+++ b/doc/user/project/merge_requests/img/discussion_view.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/discussions_resolved.png b/doc/user/project/merge_requests/img/discussions_resolved.png
new file mode 100644
index 00000000000..85428129ac8
--- /dev/null
+++ b/doc/user/project/merge_requests/img/discussions_resolved.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/resolve_comment_button.png b/doc/user/project/merge_requests/img/resolve_comment_button.png
new file mode 100644
index 00000000000..2c4ab2f5d53
--- /dev/null
+++ b/doc/user/project/merge_requests/img/resolve_comment_button.png
Binary files differ
diff --git a/doc/user/project/merge_requests/img/resolve_discussion_button.png b/doc/user/project/merge_requests/img/resolve_discussion_button.png
new file mode 100644
index 00000000000..73f265bb101
--- /dev/null
+++ b/doc/user/project/merge_requests/img/resolve_discussion_button.png
Binary files differ
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