From 6537a4a8feac08031b3be2133f6756afb3310254 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 26 Jul 2016 13:44:51 +0100 Subject: [PATCH] Correctly resolves/unresolves discussions --- .../components/resolve_all_btn.js.es6} | 20 ++++-- .../components/resolve_btn.js.es6 | 31 +++++++-- .../components/resolve_count.js.es6 | 0 .../diff_notes_bundle.js.es6} | 5 +- .../diff_notes/mixins/namespace.js.es6 | 9 +++ .../diff_notes/services/resolve.js.es6 | 60 +++++++++++++++++ .../stores/comments.js.es6 | 20 ++++++ .../line_comments/services/resolve.js.es6 | 64 ------------------- app/assets/javascripts/merge_request_tabs.js | 4 +- app/assets/javascripts/notes.js | 4 +- app/views/discussions/_resolve_all.html.haml | 4 +- .../projects/merge_requests/_show.html.haml | 4 +- app/views/projects/notes/_note.html.haml | 5 +- config/application.rb | 2 +- 14 files changed, 145 insertions(+), 87 deletions(-) rename app/assets/javascripts/{line_comments/components/resolve_all.js.es6 => diff_notes/components/resolve_all_btn.js.es6} (65%) rename app/assets/javascripts/{line_comments => diff_notes}/components/resolve_btn.js.es6 (63%) rename app/assets/javascripts/{line_comments => diff_notes}/components/resolve_count.js.es6 (100%) rename app/assets/javascripts/{line_comments/line_comments_bundle.js.es6 => diff_notes/diff_notes_bundle.js.es6} (78%) create mode 100644 app/assets/javascripts/diff_notes/mixins/namespace.js.es6 create mode 100644 app/assets/javascripts/diff_notes/services/resolve.js.es6 rename app/assets/javascripts/{line_comments => diff_notes}/stores/comments.js.es6 (58%) delete mode 100644 app/assets/javascripts/line_comments/services/resolve.js.es6 diff --git a/app/assets/javascripts/line_comments/components/resolve_all.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6 similarity index 65% rename from app/assets/javascripts/line_comments/components/resolve_all.js.es6 rename to app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6 index a1564a68096..4c56386ad66 100644 --- a/app/assets/javascripts/line_comments/components/resolve_all.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6 @@ -1,8 +1,13 @@ ((w) => { - w.ResolveAll = Vue.extend({ + w.ResolveAllBtn = Vue.extend({ + mixins: [ + ButtonMixins + ], props: { discussionId: String, - namespace: String, + mergeRequestId: Number, + namespacePath: String, + projectPath: String, }, data: function() { return { @@ -35,8 +40,15 @@ }, methods: { resolve: function () { - ResolveService - .resolveAll(this.namespace, this.discussionId, this.allResolved); + let promise; + + if (this.allResolved) { + promise = ResolveService + .unResolveAll(this.namespace, this.mergeRequestId, this.discussionId); + } else { + promise = ResolveService + .resolveAll(this.namespace, this.mergeRequestId, this.discussionId); + } } } }); diff --git a/app/assets/javascripts/line_comments/components/resolve_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 similarity index 63% rename from app/assets/javascripts/line_comments/components/resolve_btn.js.es6 rename to app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 index fa0e727c6d4..b60b8424d41 100644 --- a/app/assets/javascripts/line_comments/components/resolve_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 @@ -1,10 +1,14 @@ ((w) => { w.ResolveBtn = Vue.extend({ + mixins: [ + ButtonMixins + ], props: { noteId: Number, discussionId: String, resolved: Boolean, - namespace: String + namespacePath: String, + projectPath: String, }, data: function () { return { @@ -29,13 +33,26 @@ .tooltip('fixTitle'); }, resolve: function () { + let promise; this.loading = true; - ResolveService - .resolve(this.namespace, this.discussionId, this.noteId, !this.isResolved) - .then(() => { - this.loading = false; - this.$nextTick(this.updateTooltip); - }); + + 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) { + CommentsStore.update(this.discussionId, this.noteId, !this.isResolved); + } + + this.$nextTick(this.updateTooltip); + }); } }, compiled: function () { diff --git a/app/assets/javascripts/line_comments/components/resolve_count.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 similarity index 100% rename from app/assets/javascripts/line_comments/components/resolve_count.js.es6 rename to app/assets/javascripts/diff_notes/components/resolve_count.js.es6 diff --git a/app/assets/javascripts/line_comments/line_comments_bundle.js.es6 b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 similarity index 78% rename from app/assets/javascripts/line_comments/line_comments_bundle.js.es6 rename to app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 index 6866e6b6f54..0605ad0e750 100644 --- a/app/assets/javascripts/line_comments/line_comments_bundle.js.es6 +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 @@ -2,14 +2,15 @@ //= require vue-resource //= require_directory ./stores //= require_directory ./services +//= require_directory ./mixins //= require_directory ./components $(() => { window.DiffNotesApp = new Vue({ - el: '#diff-comments-app', + el: '#diff-notes-app', components: { 'resolve-btn': ResolveBtn, - 'resolve-all': ResolveAll, + 'resolve-all-btn': ResolveAllBtn, } }); 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..5215e7c6be7 --- /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/services/resolve.js.es6 b/app/assets/javascripts/diff_notes/services/resolve.js.es6 new file mode 100644 index 00000000000..052f3c5f721 --- /dev/null +++ b/app/assets/javascripts/diff_notes/services/resolve.js.es6 @@ -0,0 +1,60 @@ +((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(); + } + + resolve(namespace, noteId) { + this.setCSRF(); + Vue.http.options.root = `/${namespace}`; + + return this.noteResource.save({ noteId }, {}); + } + + unresolve(namespace, noteId) { + this.setCSRF(); + Vue.http.options.root = `/${namespace}`; + + return this.noteResource.delete({ noteId }, {}); + } + + resolveAll(namespace, mergeRequestId, discussionId) { + this.setCSRF(); + Vue.http.options.root = `/${namespace}`; + + CommentsStore.loading[discussionId] = true; + + return this.discussionResource.save({ + mergeRequestId, + discussionId + }, {}).then((response) => { + CommentsStore.loading[discussionId] = false; + + CommentsStore.updateCommentsForDiscussion(discussionId, true); + }); + } + + unResolveAll(namespace, mergeRequestId, discussionId) { + this.setCSRF(); + Vue.http.options.root = `/${namespace}`; + + CommentsStore.loading[discussionId] = true; + + return this.discussionResource.delete({ + mergeRequestId, + discussionId + }, {}).then((response) => { + CommentsStore.loading[discussionId] = false; + + CommentsStore.updateCommentsForDiscussion(discussionId, false); + }); + } + } + + w.ResolveService = new ResolveServiceClass(); +}(window)); diff --git a/app/assets/javascripts/line_comments/stores/comments.js.es6 b/app/assets/javascripts/diff_notes/stores/comments.js.es6 similarity index 58% rename from app/assets/javascripts/line_comments/stores/comments.js.es6 rename to app/assets/javascripts/diff_notes/stores/comments.js.es6 index 709361372ac..cd981023944 100644 --- a/app/assets/javascripts/line_comments/stores/comments.js.es6 +++ b/app/assets/javascripts/diff_notes/stores/comments.js.es6 @@ -24,5 +24,25 @@ Vue.delete(this.loading, discussionId); } }, + updateCommentsForDiscussion: function (discussionId, resolve) { + const noteIds = CommentsStore.notesForDiscussion(discussionId, resolve); + + for (const noteId of noteIds) { + CommentsStore.update(discussionId, noteId, resolve); + } + }, + notesForDiscussion: function (discussionId, resolve) { + let ids = []; + + for (const noteId in CommentsStore.state[discussionId]) { + const resolved = CommentsStore.state[discussionId][noteId]; + + if (resolved !== resolve) { + ids.push(noteId); + } + } + + return ids; + } }; }(window)); diff --git a/app/assets/javascripts/line_comments/services/resolve.js.es6 b/app/assets/javascripts/line_comments/services/resolve.js.es6 deleted file mode 100644 index e2be311cb15..00000000000 --- a/app/assets/javascripts/line_comments/services/resolve.js.es6 +++ /dev/null @@ -1,64 +0,0 @@ -((w) => { - class ResolveServiceClass { - constructor() { - const actions = { - resolve: { - method: 'POST', - url: 'notes{/id}/resolve', - }, - all: { - method: 'POST', - url: 'notes/resolve_all', - } - }; - - this.resource = Vue.resource('notes{/id}', {}, actions); - } - - setCSRF() { - Vue.http.headers.common['X-CSRF-Token'] = $.rails.csrfToken(); - } - - resolve(namespace, discussionId, noteId, resolve) { - this.setCSRF(); - Vue.http.options.root = `/${namespace}`; - - return this.resource - .resolve({ id: noteId }, { discussion: discussionId, resolved: resolve }) - .then((response) => { - if (response.status === 200) { - CommentsStore.update(discussionId, noteId, resolve) - } - }); - } - - resolveAll(namespace, discussionId, allResolve) { - this.setCSRF(); - Vue.http.options.root = `/${namespace}`; - - let ids = [] - for (const noteId in CommentsStore.state[discussionId]) { - const resolved = CommentsStore.state[discussionId][noteId]; - - if (resolved === allResolve) { - ids.push(noteId); - } - } - - CommentsStore.loading[discussionId] = true; - return this.resource - .all({}, { ids: ids, discussion: discussionId, resolved: !allResolve }) - .then((response) => { - if (response.status === 200) { - for (const noteId in ids) { - CommentsStore.update(discussionId, noteId, !allResolve); - } - } - - CommentsStore.loading[discussionId] = false; - }); - } - } - - w.ResolveService = new ResolveServiceClass(); -}(window)); diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 61d05932b57..332288dcf8e 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -120,8 +120,8 @@ return function(data) { $('#diffs').html(data.html); - if ($('resolve-btn, resolve-all').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { - $('resolve-btn, resolve-all').each(function () { + if ($('resolve-btn, resolve-all-btn').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { + $('resolve-btn, resolve-all-btn').each(function () { DiffNotesApp.$compile($(this).get(0)) }); } diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index bb20c143a4d..b5fc21e331b 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -300,8 +300,8 @@ discussionContainer.append(note_html); } - if ($('resolve-btn, resolve-all').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { - $('resolve-btn, resolve-all').each(function () { + if ($('resolve-btn, resolve-all-btn').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { + $('resolve-btn, resolve-all-btn').each(function () { DiffNotesApp.$compile($(this).get(0)) }); } diff --git a/app/views/discussions/_resolve_all.html.haml b/app/views/discussions/_resolve_all.html.haml index 8b5655af2a1..6c4e37be854 100644 --- a/app/views/discussions/_resolve_all.html.haml +++ b/app/views/discussions/_resolve_all.html.haml @@ -1,6 +1,8 @@ - if discussion.can_resolve?(current_user) - %resolve-all{ ":namespace" => "'#{discussion.project.namespace.path}/#{discussion.project.path}'", + %resolve-all-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'", + ":project-path" => "'#{discussion.project.path}'", ":discussion-id" => "'#{discussion.id}'", + ":merge-request-id" => "#{discussion.first_note.noteable.iid}", "inline-template" => true, "v-cloak" => true } %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading" } diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 14092c38a95..cd71e1d6a38 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -2,7 +2,7 @@ - page_description @merge_request.description - page_card_attributes @merge_request.card_attributes - content_for :page_specific_javascripts do - = page_specific_javascript_tag('line_comments/line_comments_bundle.js') + = page_specific_javascript_tag('diff_notes/diff_notes_bundle.js') - if diff_view == 'parallel' - fluid_layout true @@ -71,7 +71,7 @@ Changes %span.badge= @merge_request.diff_size - .tab-content#diff-comments-app + .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/_note.html.haml b/app/views/projects/notes/_note.html.haml index 0fce8d06b5f..c90be872ff4 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -24,13 +24,14 @@ - if note.resolvable? - if can?(current_user, :resolve_note, note) - %resolve-btn{ ":namespace" => "'#{note.project.namespace.path}/#{note.project.path}'", + %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?, "inline-template" => true, "v-ref:note_#{note.id}" => true } - + .note-action-button = icon("spin spinner", "v-show" => "loading") %button.line-resolve-btn{ type: "button", diff --git a/config/application.rb b/config/application.rb index 5a4b7fa3ee4..1646461939d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -85,7 +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 << "line_comments/line_comments_bundle.js" + config.assets.precompile << "diff_notes/diff_notes_bundle.js" config.assets.precompile << "lib/utils/*.js" config.assets.precompile << "lib/*.js" config.assets.precompile << "u2f.js"