Updated issues with jump tp button

Fixed styling bugs

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

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;
             }
 
+            if (this.discussionId && discussion.id === this.discussionId) {
+              useNextDiscussionId = true;
+            }
+
             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 (nextDiscussionId) {
-            nextUnresolvedDiscussionId = nextDiscussionId;
-          } else {
-            firstUnresolvedDiscussionId = discussionKeys[0];
-          }
         }
 
-        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())

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

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,

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

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

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

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;
 

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

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

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

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)

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

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

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

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