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 e85562454e38d623df9eefb4e1bd3441e9b8411c..3f107e26421dd7b0ff43382d1afed290a4e12f08 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 @@ -24,12 +24,6 @@ } return allResolved; - }, - isLast: function () { - const discussionKeys = Object.keys(this.discussions), - indexOfDiscussion = discussionKeys.indexOf(this.discussionId); - - return discussionKeys.length - 1 === indexOfDiscussion; } }, methods: { diff --git a/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 index 1ffe4cf99d6fb246a158cd7d4b95192d19cbddc8..897a7657bf6409cef727944dafacdb12aa3e0fc4 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 +++ b/app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6 @@ -9,7 +9,8 @@ discussion = CommentsStore.state[this.discussionId]; let allResolved = true; - for (const noteId of notes) { + for (let i = 0; i < notes.length; i++) { + const noteId = notes[i]; const note = discussion[noteId]; if (!note.resolved) { diff --git a/app/assets/javascripts/diff_notes/services/resolve.js.es6 b/app/assets/javascripts/diff_notes/services/resolve.js.es6 index 29de46f2dc77f36b2c243a6178f4cb300153c843..28830f4af4ebd8e4f89e8a3db905a1eafc4e811a 100644 --- a/app/assets/javascripts/diff_notes/services/resolve.js.es6 +++ b/app/assets/javascripts/diff_notes/services/resolve.js.es6 @@ -27,7 +27,8 @@ const noteIds = CommentsStore.notesForDiscussion(discussionId); let isResolved = true; - for (const noteId of noteIds) { + for (let i = 0; i < noteIds.length; i++) { + const noteId = noteIds[i]; const resolved = CommentsStore.state[discussionId][noteId].resolved; if (!resolved) { diff --git a/app/assets/javascripts/diff_notes/stores/comments.js.es6 b/app/assets/javascripts/diff_notes/stores/comments.js.es6 index f42ed29b619a493d6427a21196ad29aa55a9240c..e199b774f59005a8080be8871637c68ff2cfea03 100644 --- a/app/assets/javascripts/diff_notes/stores/comments.js.es6 +++ b/app/assets/javascripts/diff_notes/stores/comments.js.es6 @@ -28,7 +28,8 @@ updateCommentsForDiscussion: function (discussionId, resolve, user) { const noteIds = CommentsStore.resolvedNotesForDiscussion(discussionId, resolve); - for (const noteId of noteIds) { + for (let i = 0; i < noteIds.length; i++) { + const noteId = noteIds[i]; CommentsStore.update(discussionId, noteId, resolve, user); } }, diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 332288dcf8e2b727e21aed7c3ba6cfb6164761a1..2343ace3b68e001b5822aaa724ab39e40d25acbf 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-btn').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { - $('resolve-btn, resolve-all-btn').each(function () { + if ($('resolve-btn, resolve-all-btn, jump-to-discussion').length && (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null)) { + $('resolve-btn, resolve-all-btn, jump-to-discussion').each(function () { DiffNotesApp.$compile($(this).get(0)) }); } diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 67ea47e67b2a4e7ac2c647d90f320b198ee97edf..5293e5f8dda41e969e94dbe8b3480b68bf4bf776 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -429,7 +429,7 @@ $html.find('.js-task-list-container').taskList('enable'); $note_li = $('.note-row-' + note.id); - if (DiffNotesApp != null) { + if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) { ref = DiffNotesApp.$refs['' + note.id + '']; if (ref) { @@ -525,7 +525,7 @@ note = $(el); notes = note.closest(".notes"); - if (DiffNotesApp != null) { + if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) { ref = DiffNotesApp.$refs['' + noteId + '']; if (ref) { @@ -604,10 +604,10 @@ if (canResolve === 'false') { form.find('resolve-comment-btn').remove(); - } else if (DiffNotesApp) { + } else if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) { var $commentBtn = form.find('resolve-comment-btn'); $commentBtn - .attr(':discussion-id', dataHolder.data('discussionId')); + .attr(':discussion-id', "'" + dataHolder.data('discussionId') + "'"); DiffNotesApp.$compile($commentBtn.get(0)); } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 6b34ab30121c3e1f5eac2d8374463afea9e5bdc6..c2762b3e3a12fa9f0faf64479cd28688624e2137 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -454,13 +454,13 @@ ul.notes { position: relative; top: 2px; font-size: 8px; - color: #c3c3c3; + color: $notes-action-color; vertical-align: top; } } .discussion-next-btn { path { - fill: #7E7E7E; + fill: $gray-darkest; } } diff --git a/app/views/discussions/_jump_to_next.html.haml b/app/views/discussions/_jump_to_next.html.haml index 83c94b65ea35cf75fd2df2403ed9ee5749790dd5..dfdf6e600519ce8d7fced21d862a15d7f0f46c8e 100644 --- a/app/views/discussions/_jump_to_next.html.haml +++ b/app/views/discussions/_jump_to_next.html.haml @@ -1,8 +1,7 @@ - discussion = local_assigns.fetch(:discussion, false) %jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" } .btn-group{ role: "group", - "v-show" => "!allResolved", - "v-if" => "!isLast" } + "v-show" => "!allResolved" } %button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion", title: "Jump to next unresolved discussion", "aria-label" => "Jump to next unresolved discussion", diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml index 6ad3b289c5491b5f862b26be7bb5eef014d5b912..4e090e005c3d5cac8c6dee2000e04fe54f4f0637 100644 --- a/app/views/projects/merge_requests/_show.html.haml +++ b/app/views/projects/merge_requests/_show.html.haml @@ -44,17 +44,16 @@ = succeed '.' do = link_to "command line", "#modal_merge_info", class: "how_to_merge_link vlink", title: "How To Merge", "data-toggle" => "modal" - - if current_user - #resolve-count-app.line-resolve-all-container{ "v-cloak" => true } - %resolve-count{ "inline-template" => true } - .line-resolve-all{ "v-show" => "discussionCount > 0", - ":class" => "{ 'has-next-btn': resolved !== discussionCount }" } - %span.line-resolve-btn.is-disabled{ type: "button", - ":class" => "{ 'is-active': resolved === discussionCount }" } - = icon("check") - %span.line-resolve-text - {{ resolved }}/{{ discussionCount }} discussions resolved - = render "discussions/jump_to_next" + #resolve-count-app.line-resolve-all-container{ "v-cloak" => true } + %resolve-count{ "inline-template" => true } + .line-resolve-all{ "v-show" => "discussionCount > 0", + ":class" => "{ 'has-next-btn': resolved !== discussionCount }" } + %span.line-resolve-btn.is-disabled{ type: "button", + ":class" => "{ 'is-active': resolved === discussionCount }" } + = icon("check") + %span.line-resolve-text + {{ resolved }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved + = render "discussions/jump_to_next" - if @commits_count.nonzero? %ul.merge-request-tabs.nav-links.no-top.no-bottom 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 0000000000000000000000000000000000000000..ead07cee7f7a3c2c4f11154fceaa1a916bc24db1 --- /dev/null +++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb @@ -0,0 +1,358 @@ +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 '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') + 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 '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 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 495 + 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 + 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 495 + 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).to have_selector('.line-resolve-btn.is-disabled') + + find('.line-resolve-btn').click + + expect(page).to have_selector('.line-resolve-btn.is-disabled') + 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 + 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).to have_selector('.line-resolve-btn.is-disabled') + + find('.line-resolve-btn').click + + expect(page).to have_selector('.line-resolve-btn.is-disabled') + end + + page.within '.line-resolve-all-container' do + expect(page).to have_content('0/1 discussion resolved') + 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