Moved most of the data handling into discussion & notes models

Reduced some duplicated code with compiling components
Fixed bug with resolve button tooltip not updating after resolving discussion

app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6

@@ -10,40 +10,31 @@
     },
     computed: {
       allResolved: function () {
-        let allResolved = true;
-        for (const discussionId in this.discussions) {
         const discussion = this.discussions[discussionId];
-
-          for (const noteId in discussion) {
-            const note = discussion[noteId];
-
-            if (!note.resolved) {
-              allResolved = false;
-            }
-          }
-        }
-
-        return allResolved;
+        return discussion.isResolved();
       }
     },
     methods: {
       jumpToNextUnresolvedDiscussion: function () {
-        let nextUnresolvedDiscussionId;
+        let nextUnresolvedDiscussionId,
+            firstUnresolvedDiscussionId;
 
         if (!this.discussionId) {
+          let i = 0;
           for (const discussionId in this.discussions) {
             const discussion = this.discussions[discussionId];
+            const isResolved = discussion.isResolved();
 
-            for (const noteId in discussion) {
-              const note = discussion[noteId];
+            if (!firstUnresolvedDiscussionId && !isResolved) {
+              firstUnresolvedDiscussionId = discussionId;
+            }
 
-              if (!note.resolved) {
+            if (!isResolved) {
               nextUnresolvedDiscussionId = discussionId;
               break;
             }
-            }
 
-            if (nextUnresolvedDiscussionId) break;
+            i++;
           }
         } else {
           const discussionKeys = Object.keys(this.discussions),
@@ -52,7 +43,14 @@
 
           if (nextDiscussionId) {
             nextUnresolvedDiscussionId = nextDiscussionId;
+          } else {
+            firstUnresolvedDiscussionId = discussionKeys[0];
+          }
         }
+
+        if (firstUnresolvedDiscussionId) {
+          // Jump to first unresolved discussion
+          nextUnresolvedDiscussionId = firstUnresolvedDiscussionId;
         }
 
         if (nextUnresolvedDiscussionId) {

app/assets/javascripts/diff_notes/components/resolve_btn.js.es6

@@ -18,7 +18,16 @@
         loading: false
       };
     },
+    watch: {
+      'discussions': {
+        handler: 'updateTooltip',
+        deep: true
+      }
+    },
     computed: {
+      note: function () {
+        return CommentsStore.get(this.discussionId, this.noteId);
+      },
       buttonText: function () {
         if (this.isResolved) {
           return `Resolved by ${this.resolvedByName}`;
@@ -26,8 +35,12 @@
           return 'Mark as resolved';
         }
       },
-      isResolved: function () { return CommentsStore.get(this.discussionId, this.noteId).resolved; },
-      resolvedByName: function () { return CommentsStore.get(this.discussionId, this.noteId).user; },
+      isResolved: function () {
+        return this.note.resolved;
+      },
+      resolvedByName: function () {
+        return this.note.user;
+      },
     },
     methods: {
       updateTooltip: function () {

app/assets/javascripts/diff_notes/components/resolve_comment_btn.js.es6

@@ -5,20 +5,10 @@
     },
     computed: {
       isDiscussionResolved: function () {
-        const notes = CommentsStore.notesForDiscussion(this.discussionId),
-              discussion = CommentsStore.state[this.discussionId];
-        let allResolved = true;
+        const discussion = CommentsStore.state[this.discussionId],
+              notes = CommentsStore.notesForDiscussion(this.discussionId);
 
-        for (let i = 0; i < notes.length; i++) {
-          const noteId = notes[i];
-          const note = discussion[noteId];
-
-          if (!note.resolved) {
-            allResolved = false;
-          }
-        }
-
-        return allResolved;
+        return discussion.isResolved();
       },
       buttonText: function () {
         if (this.isDiscussionResolved) {

app/assets/javascripts/diff_notes/components/resolve_count.js.es6

@@ -11,18 +11,9 @@
         let resolvedCount = 0;
 
         for (const discussionId in this.discussions) {
-          const comments = this.discussions[discussionId];
-          let resolved = true;
+          const discussion = this.discussions[discussionId];
 
-          for (const noteId in comments) {
-            const commentResolved = comments[noteId].resolved;
-
-            if (!commentResolved) {
-              resolved = false;
-            }
-          }
-
-          if (resolved) {
+          if (discussion.isResolved()) {
             resolvedCount++;
           }
         }

app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6 → app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6

 ((w) => {
-  w.ResolveAllBtn = Vue.extend({
+  w.ResolveDiscussionBtn = Vue.extend({
     mixins: [
       ButtonMixins
     ],
@@ -17,15 +17,7 @@
     },
     computed: {
       allResolved: function () {
-        let isResolved = true;
-        for (const noteId in this.discussions[this.discussionId]) {
-          const resolved = this.discussions[this.discussionId][noteId].resolved;
-
-          if (!resolved) {
-            isResolved = false;
-          }
-        }
-        return isResolved;
+        return this.discussions[this.discussionId].isResolved();
       },
       buttonText: function () {
         if (this.allResolved) {

app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6

 //= require vue
 //= require vue-resource
+//= require_directory ./models
 //= require_directory ./stores
 //= require_directory ./services
 //= require_directory ./mixins
@@ -10,8 +11,8 @@ $(() => {
     el: '#diff-notes-app',
     components: {
       'resolve-btn': ResolveBtn,
-      'resolve-all-btn': ResolveAllBtn,
-      'resolve-comment-btn': ResolveCommentBtn,
+      'resolve-discussion-btn': ResolveDiscussionBtn,
+      'resolve-comment-btn': ResolveCommentBtn
     }
   });
 
@@ -21,4 +22,13 @@ $(() => {
       'resolve-count': ResolveCount
     }
   });
+
+  window.compileVueComponentsForDiffNotes = function () {
+    const $components = $('resolve-btn, resolve-discussion-btn, jump-to-discussion');
+    if ($components.length) {
+      $components.each(function () {
+        DiffNotesApp.$compile($(this).get(0))
+      });
+    }
+  }
 });

app/assets/javascripts/diff_notes/models/disucssion.js.es6

+class DiscussionModel {
+  constructor (discussionId) {
+    this.discussionId = discussionId;
+    this.notes = {};
+  }
+
+  createNote (noteId, resolved, user) {
+    Vue.set(this.notes, noteId, new NoteModel(this.discussionId, noteId, resolved, user));
+  }
+
+  deleteNote (noteId) {
+    Vue.delete(this.notes, noteId);
+  }
+
+  getNote (noteId) {
+    return this.notes[noteId];
+  }
+
+  isResolved () {
+    for (const noteId in this.notes) {
+      const note = this.notes[noteId];
+
+      if (!note.resolved) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  resolveAllNotes (user) {
+    for (const noteId in this.notes) {
+      const note = this.notes[noteId];
+
+      if (!note.resolved) {
+        note.resolved = true;
+        note.user = user;
+      }
+    }
+  }
+
+  unResolveAllNotes (user) {
+    for (const noteId in this.notes) {
+      const note = this.notes[noteId];
+
+      if (note.resolved) {
+        note.resolved = false;
+        note.user = null;
+      }
+    }
+  }
+}

app/assets/javascripts/diff_notes/models/note.js

+class NoteModel {
+  constructor (discussionId, noteId, resolved, user) {
+    this.discussionId = discussionId;
+    this.noteId = noteId;
+    this.resolved = resolved;
+    this.user = user;
+  }
+}

app/assets/javascripts/diff_notes/services/resolve.js.es6

@@ -24,17 +24,7 @@
     }
 
     toggleResolveForDiscussion(namespace, mergeRequestId, discussionId) {
-      const noteIds = CommentsStore.notesForDiscussion(discussionId);
-      let isResolved = true;
-
-      for (let i = 0; i < noteIds.length; i++) {
-        const noteId = noteIds[i];
-        const resolved = CommentsStore.state[discussionId][noteId].resolved;
-
-        if (!resolved) {
-          isResolved = false;
-        }
-      }
+      const isResolved = CommentsStore.state[discussionId].isResolved();
 
       if (isResolved) {
         return this.unResolveAll(namespace, mergeRequestId, discussionId);
@@ -55,9 +45,11 @@
       }, {}).then((response) => {
         const data = response.data;
         const user = data ? data.resolved_by : null;
+        const discussion = CommentsStore.state[discussionId];
+        discussion.resolveAllNotes(user);
+
         CommentsStore.loading[discussionId] = false;
 
-        CommentsStore.updateCommentsForDiscussion(discussionId, true, user);
 
         this.updateUpdatedHtml(discussionId, data);
       });
@@ -74,9 +66,10 @@
         discussionId
       }, {}).then((response) => {
         const data = response.data;
-        CommentsStore.loading[discussionId] = false;
+        const discussion = CommentsStore.state[discussionId];
+        discussion.unResolveAllNotes();
 
-        CommentsStore.updateCommentsForDiscussion(discussionId, false);
+        CommentsStore.loading[discussionId] = false;
 
         this.updateUpdatedHtml(discussionId, data);
       });

app/assets/javascripts/diff_notes/stores/comments.js.es6

@@ -3,57 +3,32 @@
     state: {},
     loading: {},
     get: function (discussionId, noteId) {
-      return this.state[discussionId][noteId];
+      return this.state[discussionId].getNote(noteId);
     },
     create: function (discussionId, noteId, resolved, user) {
+      let discussion = this.state[discussionId];
       if (!this.state[discussionId]) {
-        Vue.set(this.state, discussionId, {});
+        discussion = new DiscussionModel(discussionId);
+        Vue.set(this.state, discussionId, discussion);
         Vue.set(this.loading, discussionId, false);
       }
 
-      Vue.set(this.state[discussionId], noteId, { resolved, user});
+      discussion.createNote(noteId, resolved, user);
     },
     update: function (discussionId, noteId, resolved, user) {
-      this.state[discussionId][noteId].resolved = resolved;
-      this.state[discussionId][noteId].user = user;
+      const discussion = this.state[discussionId];
+      const note = discussion.getNote(noteId);
+      note.resolved = resolved;
+      note.user = user;
     },
     delete: function (discussionId, noteId) {
-      Vue.delete(this.state[discussionId], noteId);
+      const discussion = this.state[discussionId];
+      discussion.deleteNote(noteId);
 
-      if (Object.keys(this.state[discussionId]).length === 0) {
+      if (Object.keys(discussion.notes).length === 0) {
         Vue.delete(this.state, discussionId);
         Vue.delete(this.loading, discussionId);
       }
-    },
-    updateCommentsForDiscussion: function (discussionId, resolve, user) {
-      const noteIds = CommentsStore.resolvedNotesForDiscussion(discussionId, resolve);
-
-      for (let i = 0; i < noteIds.length; i++) {
-        const noteId = noteIds[i];
-        CommentsStore.update(discussionId, noteId, resolve, user);
-      }
-    },
-    notesForDiscussion: function (discussionId) {
-      let ids = [];
-
-      for (const noteId in CommentsStore.state[discussionId]) {
-        ids.push(noteId);
-      }
-
-      return ids;
-    },
-    resolvedNotesForDiscussion: function (discussionId, resolve) {
-      let ids = [];
-
-      for (const noteId in CommentsStore.state[discussionId]) {
-        const resolved = CommentsStore.state[discussionId][noteId].resolved;
-
-        if (resolved !== resolve) {
-          ids.push(noteId);
-        }
-      }
-
-      return ids;
     }
   };
 })(window);

app/assets/javascripts/merge_request_tabs.js

@@ -120,10 +120,8 @@
           return function(data) {
             $('#diffs').html(data.html);
 
-            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))
-              });
+            if (compileVueComponentsForDiffNotes) {
+              compileVueComponentsForDiffNotes();
             }
 
             gl.utils.localTimeAgo($('.js-timeago', 'div#diffs'));

app/assets/javascripts/notes.js

@@ -300,10 +300,8 @@
         discussionContainer.append(note_html);
       }
 
-      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))
-        });
+      if (compileVueComponentsForDiffNotes) {
+        compileVueComponentsForDiffNotes();
       }
 
       gl.utils.localTimeAgo($('.js-timeago', note_html), false);
@@ -402,7 +400,7 @@
         var namespacePath = $form.attr('data-namespace-path'),
             projectPath = $form.attr('data-project-path')
             discussionId = $form.attr('data-discussion-id'),
-            mergeRequestId = $('input[name="noteable_iid"]', $form).val(),
+            mergeRequestId = $form.attr('data-noteable-iid'),
             namespace = namespacePath + '/' + projectPath;
 
         if (ResolveService != null) {
@@ -429,20 +427,10 @@
       $html.find('.js-task-list-container').taskList('enable');
       $note_li = $('.note-row-' + note.id);
 
-      if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) {
-        ref = DiffNotesApp.$refs['' + note.id + ''];
-
-        if (ref) {
-          ref.$destroy(true);
-        }
-      }
-
       $note_li.replaceWith($html);
 
-      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))
-        });
+      if (compileVueComponentsForDiffNotes) {
+        compileVueComponentsForDiffNotes();
       }
     };
 
@@ -589,7 +577,7 @@
      */
 
     Notes.prototype.setupDiscussionNoteForm = function(dataHolder, form) {
-      var canResolve = dataHolder.attr('data-resolvable');
+      var canResolve = dataHolder.attr('data-can-resolve');
       form.attr('id', "new-discussion-note-form-" + (dataHolder.data("discussionId")));
       form.attr("data-line-code", dataHolder.data("lineCode"));
       form.find("#note_type").val(dataHolder.data("noteType"));
@@ -604,7 +592,7 @@
 
       if (canResolve === 'false') {
         form.find('resolve-comment-btn').remove();
-      } else if (typeof DiffNotesApp !== "undefined" && DiffNotesApp !== null) {
+      } else if (DiffNotesApp) {
         var $commentBtn = form.find('resolve-comment-btn');
         $commentBtn
           .attr(':discussion-id', "'" + dataHolder.data('discussionId') + "'");

app/helpers/notes_helper.rb

@@ -79,7 +79,7 @@ module NotesHelper
   def link_to_reply_discussion(discussion, line_type = nil)
     return unless current_user
 
-    data = discussion.reply_attributes.merge(line_type: line_type, resolvable: discussion.can_resolve?(current_user))
+    data = discussion.reply_attributes.merge(line_type: line_type, can_resolve: discussion.can_resolve?(current_user))
 
     button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
                            data: data, title: 'Add a reply'

app/views/discussions/_resolve_all.html.haml

 - if discussion.can_resolve?(current_user)
-  %resolve-all-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'",
+  %resolve-discussion-btn{ ":namespace-path" => "'#{discussion.project.namespace.path}'",
       ":project-path" => "'#{discussion.project.path}'",
       ":discussion-id" => "'#{discussion.id}'",
       ":merge-request-id" => "#{discussion.noteable.iid}",

app/views/projects/notes/_form.html.haml

-= form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new-note js-new-note-form js-quick-submit common-note-form" }, authenticity_token: true do |f|
+= form_for [@project.namespace.becomes(Namespace), @project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new-note js-new-note-form js-quick-submit common-note-form", "data-noteable-iid" => @note.noteable.try(:iid), }, authenticity_token: true do |f|
   = hidden_field_tag :view, diff_view
   = hidden_field_tag :line_type
   = note_target_fields(@note)
   = f.hidden_field :commit_id
   = f.hidden_field :line_code
   = f.hidden_field :noteable_id
-  = hidden_field_tag :noteable_iid, @note.noteable.try(:iid)
   = f.hidden_field :noteable_type
   = f.hidden_field :type
   = f.hidden_field :position

app/views/projects/notes/_note.html.haml

@@ -22,7 +22,7 @@
             - if access
               %span.note-role.hidden-xs= access
 
-            - if note.resolvable?
+            - if (note.resolvable? && can?(current_user, :resolve_note, note)) || (note.resolved? && !can?(current_user, :resolve_note, note))
               %resolve-btn{ ":namespace-path" => "'#{note.project.namespace.path}'",
                   ":project-path" => "'#{note.project.path}'",
                   ":discussion-id" => "'#{note.discussion_id}'",
@@ -36,7 +36,7 @@
                 .note-action-button
                   = icon("spin spinner", "v-show" => "loading")
                   %button.line-resolve-btn{ type: "button",
-                      class: ("is-disabled" if !can?(current_user, :resolve_note, note)),
+                      class: ("is-disabled" unless can?(current_user, :resolve_note, note)),
                       ":class" => "{ 'is-active': isResolved }",
                       ":aria-label" => "buttonText",
                       "@click" => "resolve",