From 6f94f62f721d4300e538acbc75b0c7e422a6fdbd Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 29 Jul 2016 13:31:32 +0100 Subject: [PATCH] Correctly cycles the unresolved discussions Switches back to discussion tab when clicking jump to next unresolved button --- ...ment_btn.js.es6 => comment_resolve_btn.es6} | 2 +- .../components/jump_to_discussion.js.es6 | 18 ++++++++++++++++-- .../diff_notes/diff_notes_bundle.js.es6 | 2 +- .../diff_notes/models/disucssion.js.es6 | 4 ++-- .../javascripts/diff_notes/models/note.js.es6 | 2 +- .../merge_requests/_discussion.html.haml | 2 +- .../merge_requests/diff_notes_resolve_spec.rb | 5 +++++ 7 files changed, 27 insertions(+), 8 deletions(-) rename app/assets/javascripts/diff_notes/components/{resolve_comment_btn.js.es6 => comment_resolve_btn.es6} (92%) diff --git a/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.es6 similarity index 92% rename from app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 rename to app/assets/javascripts/diff_notes/components/comment_resolve_btn.es6 index cf006c460c8..e0e79e1c7e1 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/comment_resolve_btn.es6 @@ -1,5 +1,5 @@ ((w) => { - w.ResolveCommentBtn = Vue.extend({ + w.CommentAndResolveBtn = Vue.extend({ props: { discussionId: String }, 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 3c30f9df058..641d6502d74 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 @@ -37,9 +37,20 @@ i++; } } else { + let nextDiscussionId; const discussionKeys = Object.keys(this.discussions), - indexOfDiscussion = discussionKeys.indexOf(this.discussionId), - nextDiscussionId = discussionKeys[indexOfDiscussion + 1]; + 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 (nextDiscussionId) { nextUnresolvedDiscussionId = nextDiscussionId; @@ -54,6 +65,9 @@ } if (nextUnresolvedDiscussionId) { + $('#notes').addClass('active'); + $('#commits, #builds, #diffs').removeClass('active'); + $.scrollTo(`.discussion[data-discussion-id="${nextUnresolvedDiscussionId}"]`, { offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight()) }); diff --git a/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 index ed96af66c50..b0cce29fa04 100644 --- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 +++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6 @@ -12,7 +12,7 @@ $(() => { components: { 'resolve-btn': ResolveBtn, 'resolve-discussion-btn': ResolveDiscussionBtn, - 'resolve-comment-btn': ResolveCommentBtn + 'comment-and-resolve-btn': CommentAndResolveBtn } }); diff --git a/app/assets/javascripts/diff_notes/models/disucssion.js.es6 b/app/assets/javascripts/diff_notes/models/disucssion.js.es6 index 58e8529d1d3..d4de6e2d6dc 100644 --- a/app/assets/javascripts/diff_notes/models/disucssion.js.es6 +++ b/app/assets/javascripts/diff_notes/models/disucssion.js.es6 @@ -1,12 +1,12 @@ class DiscussionModel { constructor (discussionId) { - this.discussionId = discussionId; + this.id = discussionId; this.notes = {}; this.loading = false; } createNote (noteId, resolved, user) { - Vue.set(this.notes, noteId, new NoteModel(this.discussionId, noteId, resolved, user)); + Vue.set(this.notes, noteId, new NoteModel(this.id, noteId, resolved, user)); } deleteNote (noteId) { diff --git a/app/assets/javascripts/diff_notes/models/note.js.es6 b/app/assets/javascripts/diff_notes/models/note.js.es6 index 2772991ce8b..359c364cd90 100644 --- a/app/assets/javascripts/diff_notes/models/note.js.es6 +++ b/app/assets/javascripts/diff_notes/models/note.js.es6 @@ -1,7 +1,7 @@ class NoteModel { constructor (discussionId, noteId, resolved, user) { this.discussionId = discussionId; - this.noteId = noteId; + this.id = noteId; this.resolved = resolved; this.user = user; } diff --git a/app/views/projects/merge_requests/_discussion.html.haml b/app/views/projects/merge_requests/_discussion.html.haml index d36d60be32e..218dbbb7951 100644 --- a/app/views/projects/merge_requests/_discussion.html.haml +++ b/app/views/projects/merge_requests/_discussion.html.haml @@ -4,7 +4,7 @@ = 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"} - %resolve-comment-btn{ "inline-template" => true, ":discussion-id" => "" } + %comment-and-resolve-btn{ "inline-template" => true, ":discussion-id" => "" } %button.btn.btn-nr.btn-default.append-right-10.js-comment-resolve-button{ type: "submit", data: { namespace_path: "#{@merge_request.project.namespace.path}", project_path: "#{@merge_request.project.path}" } } {{ buttonText }} diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb index c7ab9fac126..dc8bcd04aeb 100644 --- a/spec/features/merge_requests/diff_notes_resolve_spec.rb +++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb @@ -35,6 +35,7 @@ feature 'Diff notes resolve', feature: true, js: true 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 @@ -54,6 +55,8 @@ feature 'Diff notes resolve', feature: true, js: true do 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 @@ -147,6 +150,8 @@ feature 'Diff notes resolve', feature: true, js: true do 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')