diff options
16 files changed, 133 insertions, 95 deletions
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 index 694932e01bd..cd696a3dae3 100644 --- a/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.js.es6 @@ -32,12 +32,12 @@ const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`); this.textareaVal = $textarea.val(); - $textarea.on('input', () => { + $textarea.on('input.comment-and-resolve-btn', () => { this.textareaVal = $textarea.val(); }); }, destroyed: function () { - $(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input'); + $(`#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 index 82cdc5a7772..75c4376acd4 100644 --- a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 +++ b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6 @@ -14,66 +14,77 @@ return discussion.isResolved(); }, discussionsCount: function () { - return Object.keys(this.discussions).length; + return CommentsStore.discussionCount(); + }, + unresolvedDiscussionCount: function () { + let unresolvedCount = 0; + for (const discussionId in this.discussions) { + const discussion = this.discussions[discussionId]; + + if (!discussion.isResolved()) { + unresolvedCount++; + } + } + + return unresolvedCount; }, showButton: function () { - return this.discussionsCount > 0 && (this.discussionsCount > 1 || !this.discussionId); + if (this.discussionId) { + if (this.unresolvedDiscussionCount > 1) { + return true; + } else { + return this.discussionId !== this.lastResolvedId(); + } + } else { + return this.unresolvedDiscussionCount >= 1; + } } }, methods: { + lastResolvedId: function () { + let lastId; + for (const discussionId in this.discussions) { + const discussion = this.discussions[discussionId]; + + if (!discussion.isResolved()) { + lastId = discussion.id; + } + } + return lastId; + }, jumpToNextUnresolvedDiscussion: function () { let nextUnresolvedDiscussionId, - firstUnresolvedDiscussionId; + firstUnresolvedDiscussionId, + useNextDiscussionId = false, + i = 0; - if (!this.discussionId) { - let i = 0; - for (const discussionId in this.discussions) { - const discussion = this.discussions[discussionId]; - const isResolved = discussion.isResolved(); + for (const discussionId in this.discussions) { + const discussion = this.discussions[discussionId]; - if (!firstUnresolvedDiscussionId && !isResolved) { - firstUnresolvedDiscussionId = discussionId; + if (!discussion.isResolved()) { + if (i === 0) { + firstUnresolvedDiscussionId = discussion.id; } - if (!isResolved) { - nextUnresolvedDiscussionId = discussionId; + if (useNextDiscussionId) { + nextUnresolvedDiscussionId = discussion.id; break; } - i++; - } - } else { - let nextDiscussionId; - const discussionKeys = Object.keys(this.discussions), - indexOfDiscussion = discussionKeys.indexOf(this.discussionId); - nextDiscussionIds = discussionKeys.splice(indexOfDiscussion); - - nextDiscussionIds.forEach((discussionId) => { - if (discussionId !== this.discussionId) { - const discussion = this.discussions[discussionId]; - - if (!discussion.isResolved()) { - nextDiscussionId = discussion.id; - } + if (this.discussionId && discussion.id === this.discussionId) { + useNextDiscussionId = true; } - }); - if (nextDiscussionId) { - nextUnresolvedDiscussionId = nextDiscussionId; - } else { - firstUnresolvedDiscussionId = discussionKeys[0]; + i++; } } - if (firstUnresolvedDiscussionId) { - // Jump to first unresolved discussion + if (!nextUnresolvedDiscussionId && firstUnresolvedDiscussionId) { nextUnresolvedDiscussionId = firstUnresolvedDiscussionId; } if (nextUnresolvedDiscussionId) { - $('#notes').addClass('active'); - $('#commits, #builds, #diffs').removeClass('active'); - mrTabs.setCurrentAction('notes'); + mrTabs.activateTab('notes'); $.scrollTo(`.discussion[data-discussion-id="${nextUnresolvedDiscussionId}"]`, { offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight()) diff --git a/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 index 01f51d2c20c..d6101c2d035 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 @@ -39,7 +39,7 @@ return this.note.resolved; }, resolvedByName: function () { - return this.note.user; + return this.note.resolved_by; }, }, methods: { @@ -63,13 +63,13 @@ } promise.then((response) => { - const data = response.data; - const user = data ? data.resolved_by : null; + const data = response.json(); this.loading = false; if (response.status === 200) { - CommentsStore.update(this.discussionId, this.noteId, !this.isResolved, user); + const resolved_by = data ? data.resolved_by : null; + CommentsStore.update(this.discussionId, this.noteId, !this.isResolved, resolved_by); ResolveService.updateUpdatedHtml(this.discussionId, data); } else { new Flash('An error occurred when trying to resolve a comment. Please try again.', 'alert'); diff --git a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 index 4d9e1dd8df8..51674e0e06d 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 @@ -1,5 +1,8 @@ ((w) => { w.ResolveCount = Vue.extend({ + props: { + loggedOut: Boolean + }, data: function () { return { discussions: CommentsStore.state, 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 index 47ea5a3cc04..1eaabdaf81e 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6 @@ -15,8 +15,11 @@ }; }, computed: { + discussion: function () { + return this.discussions[this.discussionId]; + }, allResolved: function () { - return this.discussions[this.discussionId].isResolved(); + return this.discussion.isResolved(); }, buttonText: function () { if (this.allResolved) { @@ -26,7 +29,7 @@ } }, loading: function () { - return this.discussions[this.discussionId].loading; + return this.discussion.loading; } }, methods: { diff --git a/app/assets/javascripts/diff_notes/models/discussion.js.es6 b/app/assets/javascripts/diff_notes/models/discussion.js.es6 index d4de6e2d6dc..8f0d77a62f5 100644 --- a/app/assets/javascripts/diff_notes/models/discussion.js.es6 +++ b/app/assets/javascripts/diff_notes/models/discussion.js.es6 @@ -5,8 +5,8 @@ class DiscussionModel { this.loading = false; } - createNote (noteId, resolved, user) { - Vue.set(this.notes, noteId, new NoteModel(this.id, noteId, resolved, user)); + createNote (noteId, resolved, resolved_by) { + Vue.set(this.notes, noteId, new NoteModel(this.id, noteId, resolved, resolved_by)); } deleteNote (noteId) { @@ -17,6 +17,10 @@ class DiscussionModel { return this.notes[noteId]; } + notesCount() { + return Object.keys(this.notes).length; + } + isResolved () { for (const noteId in this.notes) { const note = this.notes[noteId]; @@ -28,24 +32,24 @@ class DiscussionModel { return true; } - resolveAllNotes (user) { + resolveAllNotes (resolved_by) { for (const noteId in this.notes) { const note = this.notes[noteId]; if (!note.resolved) { note.resolved = true; - note.user = user; + note.resolved_by = resolved_by; } } } - unResolveAllNotes (user) { + unResolveAllNotes () { for (const noteId in this.notes) { const note = this.notes[noteId]; if (note.resolved) { note.resolved = false; - note.user = null; + note.resolved_by = null; } } } diff --git a/app/assets/javascripts/diff_notes/models/note.js.es6 b/app/assets/javascripts/diff_notes/models/note.js.es6 index 359c364cd90..4cbd57e6ff4 100644 --- a/app/assets/javascripts/diff_notes/models/note.js.es6 +++ b/app/assets/javascripts/diff_notes/models/note.js.es6 @@ -1,8 +1,8 @@ class NoteModel { - constructor (discussionId, noteId, resolved, user) { + constructor (discussionId, noteId, resolved, resolved_by) { this.discussionId = discussionId; this.id = noteId; this.resolved = resolved; - this.user = user; + 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 index d15959c0909..a1ddd260c65 100644 --- a/app/assets/javascripts/diff_notes/services/resolve.js.es6 +++ b/app/assets/javascripts/diff_notes/services/resolve.js.es6 @@ -11,14 +11,18 @@ resolve(namespace, noteId) { this.setCSRF(); - Vue.http.options.root = `/${namespace}`; + if (Vue.http.options.root !== `/${namespace}`) { + Vue.http.options.root = `/${namespace}`; + } return this.noteResource.save({ noteId }, {}); } unresolve(namespace, noteId) { this.setCSRF(); - Vue.http.options.root = `/${namespace}`; + if (Vue.http.options.root !== `/${namespace}`) { + Vue.http.options.root = `/${namespace}`; + } return this.noteResource.delete({ noteId }, {}); } @@ -37,18 +41,22 @@ const discussion = CommentsStore.state[discussionId]; this.setCSRF(); - Vue.http.options.root = `/${namespace}`; + + if (Vue.http.options.root !== `/${namespace}`) { + Vue.http.options.root = `/${namespace}`; + } discussion.loading = true; + console.log(discussion.loading); return this.discussionResource.save({ mergeRequestId, discussionId }, {}).then((response) => { if (response.status === 200) { - const data = response.data; - const user = data ? data.resolved_by : null; - discussion.resolveAllNotes(user); + const data = response.json(); + const resolved_by = data ? data.resolved_by : null; + discussion.resolveAllNotes(resolved_by); discussion.loading = false; this.updateUpdatedHtml(discussionId, data); @@ -71,7 +79,7 @@ discussionId }, {}).then((response) => { if (response.status === 200) { - const data = response.data; + const data = response.json(); discussion.unResolveAllNotes(); discussion.loading = false; diff --git a/app/assets/javascripts/diff_notes/stores/comments.js.es6 b/app/assets/javascripts/diff_notes/stores/comments.js.es6 index cbbad68bc5b..967568efa38 100644 --- a/app/assets/javascripts/diff_notes/stores/comments.js.es6 +++ b/app/assets/javascripts/diff_notes/stores/comments.js.es6 @@ -4,28 +4,31 @@ get: function (discussionId, noteId) { return this.state[discussionId].getNote(noteId); }, - create: function (discussionId, noteId, resolved, user) { + create: function (discussionId, noteId, resolved, resolved_by) { let discussion = this.state[discussionId]; if (!this.state[discussionId]) { discussion = new DiscussionModel(discussionId); Vue.set(this.state, discussionId, discussion); } - discussion.createNote(noteId, resolved, user); + discussion.createNote(noteId, resolved, resolved_by); }, - update: function (discussionId, noteId, resolved, user) { + update: function (discussionId, noteId, resolved, resolved_by) { const discussion = this.state[discussionId]; const note = discussion.getNote(noteId); note.resolved = resolved; - note.user = user; + note.resolved_by = resolved_by; }, delete: function (discussionId, noteId) { const discussion = this.state[discussionId]; discussion.deleteNote(noteId); - if (Object.keys(discussion.notes).length === 0) { + if (discussion.notesCount() === 0) { Vue.delete(this.state, discussionId); } + }, + discussionCount: function () { + return Object.keys(this.state).length } }; })(window); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 9ed87a4a1be..3c93d86994d 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -514,7 +514,7 @@ notes = note.closest(".notes"); if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) { - ref = DiffNotesApp.$refs['' + noteId + '']; + ref = DiffNotesApp.$refs[noteId]; if (ref) { ref.$destroy(true); @@ -591,7 +591,7 @@ form.find('.js-note-target-close').remove(); this.setupNoteForm(form); - if (canResolve === 'false') { + if (canResolve === 'false' || !form.closest('.notes_content').find('.note:not(.system-note)').length) { form.find('comment-and-resolve-btn').remove(); } else if (DiffNotesApp) { var $commentBtn = form.find('comment-and-resolve-btn'); diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js index b9ae497b0e5..70e80b57f91 100644 --- a/app/assets/javascripts/single_file_diff.js +++ b/app/assets/javascripts/single_file_diff.js @@ -35,10 +35,12 @@ this.isOpen = !this.isOpen; if (!this.isOpen && !this.hasError) { this.content.hide(); - return this.collapsedContent.show(); + this.collapsedContent.show(); + compileVueComponentsForDiffNotes(); } else if (this.content) { this.collapsedContent.hide(); - return this.content.show(); + this.content.show(); + compileVueComponentsForDiffNotes(); } else { return this.getContentHTML(); } @@ -53,6 +55,7 @@ if (data.html) { _this.content = $(data.html); _this.content.syntaxHighlight(); + compileVueComponentsForDiffNotes(); } else { _this.hasError = true; _this.content = $(ERROR_HTML); diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index c5bf454ccfe..fbe470bed2c 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,15 +1,15 @@ %ul.notes{ data: { discussion_id: discussion.id } } = render partial: "projects/notes/note", collection: discussion.notes, as: :note -.discussion-reply-holder - - if discussion.diff_discussion? - - line_type = local_assigns.fetch(:line_type, nil) +- 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) - .btn-group{ role: "group" } + .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) + = render "discussions/jump_to_next", discussion: discussion + - else + = link_to_reply_discussion(discussion) diff --git a/app/views/discussions/_resolve_all.html.haml b/app/views/discussions/_resolve_all.html.haml index 9eef367716f..5d751c60d2a 100644 --- a/app/views/discussions/_resolve_all.html.haml +++ b/app/views/discussions/_resolve_all.html.haml @@ -1,10 +1,11 @@ - if discussion.can_resolve?(current_user) - %resolve-discussion-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'", - ":project-path" => "'#{discussion.project.path}'", - ":discussion-id" => "'#{discussion.id}'", - ":merge-request-id" => "#{discussion.noteable.iid}", - "inline-template" => true, - "v-cloak" => true } - %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading" } - = icon("spinner spin", "v-show" => "loading") - {{ buttonText }} + .btn-group{ role: "group" } + %resolve-discussion-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'", + ":project-path" => "'#{discussion.project.path}'", + ":discussion-id" => "'#{discussion.id}'", + ":merge-request-id" => "#{discussion.noteable.iid}", + "inline-template" => true, + "v-cloak" => true } + %button.btn.btn-default{ type: "button", "@click" => "resolve", ":disabled" => "loading" } + = icon("spinner spin", "v-show" => "loading") + {{ buttonText }} diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index ec9869a4279..fa0288cff6d 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -45,9 +45,9 @@ = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" #resolve-count-app.line-resolve-all-container.prepend-top-10{ "v-cloak" => true } - %resolve-count{ "inline-template" => true } + %resolve-count{ "inline-template" => true, ":logged-out" => "#{current_user.nil?}" } .line-resolve-all{ "v-show" => "discussionCount > 0", - ":class" => "{ 'has-next-btn': resolved !== discussionCount }" } + ":class" => "{ 'has-next-btn': !loggedOut && resolved !== discussionCount }" } %span.line-resolve-btn.is-disabled{ type: "button", ":class" => "{ 'is-active': resolved === discussionCount }" } = icon("check") diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 044119e841d..17c5ef18cd9 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} } @@ -28,16 +29,16 @@ ":discussion-id" => "'#{note.discussion_id}'", ":note-id" => note.id, ":resolved" => note.resolved?, - ":can-resolve" => can?(current_user, :resolve_note, note), + ":can-resolve" => can_resolve, ":resolved-by" => "'#{note.resolved_by.try(:name)}'", - "v-show" => "#{(note.resolvable? && can?(current_user, :resolve_note, note)) || (note.resolved? && !can?(current_user, :resolve_note, note))}", + "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?(current_user, :resolve_note, note)), + class: ("is-disabled" unless can_resolve), ":class" => "{ 'is-active': isResolved }", ":aria-label" => "buttonText", "@click" => "resolve", diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb index 0216a8f91af..79264a1f26a 100644 --- a/spec/features/merge_requests/diff_notes_resolve_spec.rb +++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb @@ -116,15 +116,16 @@ feature 'Diff notes resolve', feature: true, js: true do 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 'Resolve discussion' + click_button 'Unresolve discussion' end page.within '.line-resolve-all-container' do expect(page).to have_content('0/1 discussion resolved') - expect(page).to have_selector('.line-resolve-btn.is-active') + expect(page).not_to have_selector('.line-resolve-btn.is-active') end end |