Added tests for resolving comments feature

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: {

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) {

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) {

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);
       }
     },

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))
               });
             }

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));
       }
 

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;
   }
 }

app/views/discussions/_jump_to_next.html.haml

 - 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",

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

spec/features/merge_requests/diff_notes_resolve_spec.rb

+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