opendev/gerrit
Code Issues Proposed changes

Fix comments sometimes not showing up in patch set select

Browse Source 
Previously, comments were not used as a parameter in the computed
function that calculated patch range values. Instead, it just used the
value of 'comments' when the patch dropdown was computed.

However, if comments were loaded after the dropdown values were
calculated, it was not updated to display the comment counts.

This change fixes the problem by using comments as a parameter in the
value calculations, and includes a default empty object in the case that
comments are not used as an input for the patch range selector
(currently the diff view).

Bug: Issue 7444
Change-Id: I2646800afc1632bf39b3c6a451c848669007ca18
This commit is contained in:
Becky Siegel 2017-10-16 16:34:31 -07:00
parent 40125966e1
commit 3a90cd92c2
2 changed files with 75 additions and 12 deletions
polygerrit-ui/app/elements/diff/gr-patch-range-select
gr-patch-range-select.jsgr-patch-range-select_test.html

21
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js

@ -34,15 +34,21 @@
_baseDropdownContent: {
type: Object,
computed: '_computeBaseDropdownContent(availablePatches, patchNum,' +
'_sortedRevisions, revisions)',
'_sortedRevisions, revisions, comments)',
},
_patchDropdownContent: {
type: Object,
computed: '_computePatchDropdownContent(availablePatches,' +
'basePatchNum, _sortedRevisions, revisions)',
'basePatchNum, _sortedRevisions, revisions, comments)',
},
changeNum: String,
comments: Array,
// In the case of a patch range select (like diff view) comments should
// be an empty array, so that the patch and base content computed values
// get triggered.
comments: {
type: Object,
value: () => { return {}; },
},
/** @type {{ meta_a: !Array, meta_b: !Array}} */
filesWeblinks: Object,
patchNum: String,
@ -58,7 +64,7 @@
behaviors: [Gerrit.PatchSetBehavior],
_computeBaseDropdownContent(availablePatches, patchNum, _sortedRevisions,
revisions) {
revisions, comments) {
const dropdownContent = [];
dropdownContent.push({
text: 'Base',
@ -72,7 +78,7 @@
triggerText: `Patchset ${basePatchNum}`,
text: `Patchset ${basePatchNum}` +
this._computePatchSetCommentsString(this.comments, basePatchNum),
mobileText: this._computeMobileText(basePatchNum, this.comments,
mobileText: this._computeMobileText(basePatchNum, comments,
revisions),
bottomText: `${this._computePatchSetDescription(
revisions, basePatchNum)}`,
@ -89,7 +95,7 @@
},
_computePatchDropdownContent(availablePatches, basePatchNum,
_sortedRevisions, revisions) {
_sortedRevisions, revisions, comments) {
const dropdownContent = [];
for (const patch of availablePatches) {
const patchNum = patch.num;
@ -101,8 +107,7 @@
text: `${patchNum === 'edit' ? '': 'Patchset '}${patchNum}` +
`${this._computePatchSetCommentsString(
this.comments, patchNum)}`,
mobileText: this._computeMobileText(patchNum, this.comments,
revisions),
mobileText: this._computeMobileText(patchNum, comments, revisions),
bottomText: `${this._computePatchSetDescription(
revisions, patchNum)}`,
value: patchNum,

66
polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select_test.html

@ -80,7 +80,7 @@ limitations under the License.
});
test('_computeBaseDropdownContent', () => {
element.comments = {};
const comments = {};
const availablePatches = [
{num: 1},
{num: 2},
@ -143,7 +143,7 @@ limitations under the License.
},
];
assert.deepEqual(element._computeBaseDropdownContent(availablePatches,
patchNum, sortedRevisions, revisions), expectedResult);
patchNum, sortedRevisions, revisions, comments), expectedResult);
});
test('_computeBaseDropdownContent called when patchNum updates', () => {
@ -170,6 +170,35 @@ limitations under the License.
assert.equal(element._computeBaseDropdownContent.callCount, 1);
});
test('_computeBaseDropdownContent called when comments update', () => {
element.revisions = [
{commit: {}},
{commit: {}},
{commit: {}},
{commit: {}},
];
element.availablePatches = [
{num: 1},
{num: 2},
{num: 3},
{num: 'edit'},
];
element.patchNum = 2;
element.basePatchNum = 'PARENT';
flushAsynchronousOperations();
// Should be recomputed for each available patch
sandbox.stub(element, '_computeBaseDropdownContent');
assert.equal(element._computeBaseDropdownContent.callCount, 0);
element.set('comments', {
file: [{
message: 'test',
patch_set: 2,
}],
});
assert.equal(element._computeBaseDropdownContent.callCount, 1);
});
test('_computePatchDropdownContent called when basePatchNum updates', () => {
element.revisions = [
{commit: {}},
@ -193,8 +222,37 @@ limitations under the License.
assert.equal(element._computePatchDropdownContent.callCount, 1);
});
test('_computePatchDropdownContent called when comments update', () => {
element.revisions = [
{commit: {}},
{commit: {}},
{commit: {}},
{commit: {}},
];
element.availablePatches = [
{num: 1},
{num: 2},
{num: 3},
{num: 'edit'},
];
element.patchNum = 2;
element.basePatchNum = 'PARENT';
flushAsynchronousOperations();
// Should be recomputed for each available patch
sandbox.stub(element, '_computePatchDropdownContent');
assert.equal(element._computePatchDropdownContent.callCount, 0);
element.set('comments', {
file: [{
message: 'test',
patch_set: 2,
}],
});
assert.equal(element._computePatchDropdownContent.callCount, 1);
});
test('_computePatchDropdownContent', () => {
element.comments = {};
const comments = {};
const availablePatches = [
{num: 1},
{num: 2},
@ -255,7 +313,7 @@ limitations under the License.
];
assert.deepEqual(element._computePatchDropdownContent(availablePatches,
basePatchNum, sortedRevisions, revisions), expectedResult);
basePatchNum, sortedRevisions, revisions, comments), expectedResult);
});
test('filesWeblinks', () => {