Fix corner case where storage gets duplicated for patchsets
Previously, there was an issue where if you create a draft comment in
side by side view and switch to unified view, the comment thinks it's in
the later patch set rather than the earlier one and a second copy gets
added to local storage with the later patchset as a component of the
key.
This was because the the thread group assumed all threads inside of it
had the same patch number. This change fixes that, so in the event that
a user switches from side by side to unified, the patch number will get
taken from the comment rather than the thread group.
Bug: Issue 5493
Change-Id: I7f00997bcb2e6f1001a5d58ac206acf5af3367d2
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
index b094e1c..7b0eeb9 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html
@@ -69,11 +69,11 @@
});
suite('test show change number preference enabled', function() {
- return setup(function() {
- return stubRestAPI({legacycid_in_change_table: true,
- time_format: 'HHMM_12',
- change_table: [],
- }).then(function() {
+ setup(function() {
+ return stubRestAPI({legacycid_in_change_table: true,
+ time_format: 'HHMM_12',
+ change_table: [],
+ }).then(function() {
element = fixture('basic');
return element._loadPreferences();
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
index c27c57e..d62dbf8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.js
@@ -340,7 +340,7 @@
var threadGroupEl =
document.createElement('gr-diff-comment-thread-group');
threadGroupEl.changeNum = changeNum;
- threadGroupEl.patchNum = patchNum;
+ threadGroupEl.patchForNewThreads = patchNum;
threadGroupEl.path = path;
threadGroupEl.side = side;
threadGroupEl.projectConfig = projectConfig;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 4665bdf..e5fe1d7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -241,7 +241,7 @@
function checkThreadGroupProps(threadGroupEl, patchNum, side, comments) {
assert.equal(threadGroupEl.changeNum, '42');
- assert.equal(threadGroupEl.patchNum, patchNum);
+ assert.equal(threadGroupEl.patchForNewThreads, patchNum);
assert.equal(threadGroupEl.path, '/path/to/foo');
assert.equal(threadGroupEl.side, side);
assert.deepEqual(threadGroupEl.projectConfig, {foo: 'bar'});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
index adbb1a4..95de61f 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.html
@@ -28,14 +28,14 @@
margin-top: .2em;
}
</style>
- <template is="dom-repeat" items="[[_threadGroups]]"
+ <template is="dom-repeat" items="[[_threads]]"
as="thread">
<gr-diff-comment-thread
comments="[[thread.comments]]"
comment-side="[[thread.commentSide]]"
change-num="[[changeNum]]"
location-range="[[thread.locationRange]]"
- patch-num="[[patchNum]]"
+ patch-num="[[thread.patchNum]]"
path="[[path]]"
side="[[side]]"
project-config="[[projectConfig]]"></gr-diff-comment-thread>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
index b9fc9f7..ee4c5b3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group.js
@@ -23,14 +23,14 @@
type: Array,
value: function() { return []; },
},
- patchNum: String,
+ patchForNewThreads: String,
projectConfig: Object,
range: Object,
side: {
type: String,
value: 'REVISION',
},
- _threadGroups: {
+ _threads: {
type: Array,
value: function() { return []; },
},
@@ -40,17 +40,18 @@
'_commentsChanged(comments.*)',
],
- addNewThread: function(locationRange, commentSide) {
- this.push('_threadGroups', {
+ addNewThread: function(locationRange) {
+ this.push('_threads', {
comments: [],
locationRange: locationRange,
+ patchNum: this.patchForNewThreads,
});
},
removeThread: function(locationRange) {
- for (var i = 0; i < this._threadGroups.length; i++) {
- if (this._threadGroups[i].locationRange === locationRange) {
- this.splice('_threadGroups', i, 1);
+ for (var i = 0; i < this._threads.length; i++) {
+ if (this._threads[i].locationRange === locationRange) {
+ this.splice('_threads', i, 1);
return;
}
}
@@ -68,7 +69,7 @@
},
_commentsChanged: function() {
- this._threadGroups = this._getThreadGroups(this.comments);
+ this._threads = this._getThreadGroups(this.comments);
},
_sortByDate: function(threadGroups) {
@@ -95,6 +96,16 @@
comment.__commentSide;
},
+ /**
+ * Determines what the patchNum of a thread should be. Use patchNum from
+ * comment if it exists, otherwise the property of the thread group.
+ * This is needed for switching between side-by-side and unified views when
+ * there are unsaved drafts.
+ */
+ _getPatchNum: function(comment) {
+ return comment.patchNum || this.patchForNewThreads;
+ },
+
_getThreadGroups: function(comments) {
var threadGroups = {};
@@ -114,6 +125,7 @@
comments: [comment],
locationRange: locationRange,
commentSide: comment.__commentSide,
+ patchNum: this._getPatchNum(comment),
};
}
}.bind(this));
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
index 79e58ee..bc08bab 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread-group/gr-diff-comment-thread-group_test.html
@@ -49,6 +49,7 @@
});
test('_getThreadGroups', function() {
+ element.patchForNewThreads = 3;
var comments = [
{
id: 'sallys_confession',
@@ -79,12 +80,14 @@
__commentSide: 'left',
}],
locationRange: 'line-left',
+ patchNum: 3
},
];
assert.deepEqual(element._getThreadGroups(comments),
expectedThreadGroups);
+ // Patch num should get inherited from comment rather
comments.push({
id: 'betsys_confession',
message: 'i like you, jack',
@@ -113,6 +116,7 @@
updated: '2015-12-24 15:00:20.396000000',
__commentSide: 'left',
}],
+ patchNum: 3,
locationRange: 'line-left',
},
{
@@ -130,6 +134,7 @@
},
__commentSide: 'left',
}],
+ patchNum: 3,
locationRange: 'range-1-1-1-2-left',
},
];
@@ -218,21 +223,33 @@
test('addNewThread', function() {
var locationRange = 'range-1-2-3-4';
- element._threadGroups = [{locationRange: 'line'}];
+ element._threads = [{locationRange: 'line'}];
element.addNewThread(locationRange);
- assert(element._threadGroups.length, 2);
+ assert(element._threads.length, 2);
+ });
+
+ test('_getPatchNum', function() {
+ element.patchForNewThreads = 3;
+ var comment = {
+ id: 'sallys_confession',
+ message: 'i like you, jack',
+ updated: '2015-12-23 15:00:20.396000000',
+ };
+ assert.equal(element._getPatchNum(comment), 3);
+ comment.patchNum = 4;
+ assert.equal(element._getPatchNum(comment), 4);
});
test('removeThread', function() {
var locationRange = 'range-1-2-3-4';
- element._threadGroups = [
+ element._threads = [
{locationRange: 'range-1-2-3-4', comments: []},
{locationRange: 'line', comments: []}
];
flushAsynchronousOperations();
element.removeThread(locationRange);
flushAsynchronousOperations();
- assert(element._threadGroups.length, 1);
+ assert(element._threads.length, 1);
});
});
</script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
index fc130c7..c088b1a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread.js
@@ -271,6 +271,7 @@
__draftID: Math.random().toString(36),
__date: new Date(),
path: this.path,
+ patchNum: this.patchNum,
side: this.side,
__commentSide: this.commentSide,
};
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
index 424cef6..e0ae9ff 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment-thread/gr-diff-comment-thread_test.html
@@ -532,8 +532,10 @@
test('_newDraft', function() {
element.commentSide = 'left';
+ element.patchNum = 3;
var draft = element._newDraft();
assert.equal(draft.__commentSide, 'left');
+ assert.equal(draft.patchNum, 3);
});
test('new comment gets created', function() {
@@ -541,8 +543,8 @@
element.addOrEditDraft(1);
assert.equal(element.comments.length, 1);
// Mock a submitted comment.
- element.comments[0].__draft = false;
element.comments[0].id = element.comments[0].__draftID;
+ element.comments[0].__draft = false;
element.addOrEditDraft(1);
assert.equal(element.comments.length, 2);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index d2cdd07..117ff24 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -236,15 +236,18 @@
sandbox.stub(element.$.diffBuilder, 'createCommentThreadGroup',
function() {
- return document.createElement('gr-diff-comment-thread-group');
+ var threadGroup =
+ document.createElement('gr-diff-comment-thread-group');
+ threadGroup.patchForNewThreads = 1;
+ return threadGroup;
});
// No thread groups.
assert.isNotOk(element._getThreadGroupForLine(contentEl));
// A thread group gets created.
- assert.isOk(element._getOrCreateThreadAtLineRange(contentEl, patchNum,
- commentSide, side));
+ assert.isOk(element._getOrCreateThreadAtLineRange(contentEl,
+ patchNum, commentSide, side));
// Try to fetch a thread with a different range.
range = {
