From efb74875cfe1a1e2b3696f0129b819fc8e204876 Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Fri, 29 Jul 2016 11:19:56 +0100
Subject: [PATCH] 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
---
.../components/jump_to_discussion.js.es6 | 42 ++++++++-------
.../diff_notes/components/resolve_btn.js.es6 | 17 ++++++-
.../components/resolve_comment_btn.js.es6 | 16 ++----
.../components/resolve_count.js.es6 | 13 +----
...n.js.es6 => resolve_discussion_btn.js.es6} | 12 +----
.../diff_notes/diff_notes_bundle.js.es6 | 14 ++++-
.../diff_notes/models/disucssion.js.es6 | 51 +++++++++++++++++++
.../javascripts/diff_notes/models/note.js | 8 +++
.../diff_notes/services/resolve.js.es6 | 21 +++-----
.../diff_notes/stores/comments.js.es6 | 49 +++++-------------
app/assets/javascripts/merge_request_tabs.js | 6 +--
app/assets/javascripts/notes.js | 26 +++-------
app/helpers/notes_helper.rb | 2 +-
app/views/discussions/_resolve_all.html.haml | 2 +-
app/views/projects/notes/_form.html.haml | 3 +-
app/views/projects/notes/_note.html.haml | 4 +-
16 files changed, 146 insertions(+), 140 deletions(-)
rename app/assets/javascripts/diff_notes/components/{resolve_all_btn.js.es6 => resolve_discussion_btn.js.es6} (72%)
create mode 100644 app/assets/javascripts/diff_notes/models/disucssion.js.es6
create mode 100644 app/assets/javascripts/diff_notes/models/note.js
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 a0c859ab7f6..3c30f9df058 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
@@ -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;
+ const discussion = this.discussions[discussionId];
+ 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) {
- nextUnresolvedDiscussionId = discussionId;
- break;
- }
+ if (!isResolved) {
+ nextUnresolvedDiscussionId = discussionId;
+ break;
}
- if (nextUnresolvedDiscussionId) break;
+ i++;
}
} else {
const discussionKeys = Object.keys(this.discussions),
@@ -52,9 +43,16 @@
if (nextDiscussionId) {
nextUnresolvedDiscussionId = nextDiscussionId;
+ } else {
+ firstUnresolvedDiscussionId = discussionKeys[0];
}
}
+ if (firstUnresolvedDiscussionId) {
+ // Jump to first unresolved discussion
+ nextUnresolvedDiscussionId = firstUnresolvedDiscussionId;
+ }
+
if (nextUnresolvedDiscussionId) {
$.scrollTo(`.discussion[data-discussion-id="${nextUnresolvedDiscussionId}"]`, {
offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight())
diff --git a/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6
index e077c3ba395..c8cf6b8ad3a 100644
--- a/app/assets/javascripts/diff_notes/components/resolve_btn.js.es6
+++ b/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 () {
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 56514afa6ce..5832435dcfe 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
@@ -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) {
diff --git a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_count.js.es6
index b8df89491ad..4d9e1dd8df8 100644
--- a/app/assets/javascripts/diff_notes/components/resolve_count.js.es6
+++ b/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++;
}
}
diff --git a/app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6 b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6
similarity index 72%
rename from app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6
rename to app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6
index 543ed8625f3..47a317a07c4 100644
--- a/app/assets/javascripts/diff_notes/components/resolve_all_btn.js.es6
+++ b/app/assets/javascripts/diff_notes/components/resolve_discussion_btn.js.es6
@@ -1,5 +1,5 @@
((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) {
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 b856c718034..33b7648f0b6 100644
--- a/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6
+++ b/app/assets/javascripts/diff_notes/diff_notes_bundle.js.es6
@@ -1,5 +1,6 @@
//= 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))
+ });
+ }
+ }
});
diff --git a/app/assets/javascripts/diff_notes/models/disucssion.js.es6 b/app/assets/javascripts/diff_notes/models/disucssion.js.es6
new file mode 100644
index 00000000000..bbc940214dd
--- /dev/null
+++ b/app/assets/javascripts/diff_notes/models/disucssion.js.es6
@@ -0,0 +1,51 @@
+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;
+ }
+ }
+ }
+}
diff --git a/app/assets/javascripts/diff_notes/models/note.js b/app/assets/javascripts/diff_notes/models/note.js
new file mode 100644
index 00000000000..2772991ce8b
--- /dev/null
+++ b/app/assets/javascripts/diff_notes/models/note.js
@@ -0,0 +1,8 @@
+class NoteModel {
+ constructor (discussionId, noteId, resolved, user) {
+ this.discussionId = discussionId;
+ this.noteId = noteId;
+ this.resolved = resolved;
+ this.user = user;
+ }
+}
diff --git a/app/assets/javascripts/diff_notes/services/resolve.js.es6 b/app/assets/javascripts/diff_notes/services/resolve.js.es6
index 50392e76fbe..0be938448b2 100644
--- a/app/assets/javascripts/diff_notes/services/resolve.js.es6
+++ b/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);
});
diff --git a/app/assets/javascripts/diff_notes/stores/comments.js.es6 b/app/assets/javascripts/diff_notes/stores/comments.js.es6
index 6e2040144a6..fbb71ca30af 100644
--- a/app/assets/javascripts/diff_notes/stores/comments.js.es6
+++ b/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);
diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js
index 2343ace3b68..8574a4e3370 100644
--- a/app/assets/javascripts/merge_request_tabs.js
+++ b/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'));
diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js
index aeaba5ae03e..0ffd3e0882b 100644
--- a/app/assets/javascripts/notes.js
+++ b/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') + "'");
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 9d9b0dba57f..3761855efc7 100644
--- a/app/helpers/notes_helper.rb
+++ b/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'
diff --git a/app/views/discussions/_resolve_all.html.haml b/app/views/discussions/_resolve_all.html.haml
index 9a75a573828..9eef367716f 100644
--- a/app/views/discussions/_resolve_all.html.haml
+++ b/app/views/discussions/_resolve_all.html.haml
@@ -1,5 +1,5 @@
- 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}",
diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml
index 032aeec70fd..b915d0133f9 100644
--- a/app/views/projects/notes/_form.html.haml
+++ b/app/views/projects/notes/_form.html.haml
@@ -1,11 +1,10 @@
-= 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
diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml
index 69b770b62b7..c708bd46387 100644
--- a/app/views/projects/notes/_note.html.haml
+++ b/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",
--
GitLab