Add undefined check throughout app

Removes both "no execute _computeDiffURL before patchNum is knwon"
and "gr-linked-text with null config" tests.

This is due to behaviour changes in Polymer2 which means functions are
always called regardless if one of the params is undefined.

Change-Id: I4bdbe03f9dc46d03a8d0997055271327f0c2a2ea
This commit is contained in:
Paladox none
2019-10-06 15:47:21 +00:00
parent 097fc86bde
commit 1e3853d593
19 changed files with 82 additions and 50 deletions

View File

@@ -179,6 +179,7 @@
}, },
_computeColspan(changeTableColumns, labelNames) { _computeColspan(changeTableColumns, labelNames) {
if (!changeTableColumns || !labelNames) return;
return changeTableColumns.length + labelNames.length + return changeTableColumns.length + labelNames.length +
NUMBER_FIXED_COLUMNS; NUMBER_FIXED_COLUMNS;
}, },
@@ -250,7 +251,7 @@
_computeItemHighlight(account, change) { _computeItemHighlight(account, change) {
// Do not show the assignee highlight if the change is not open. // Do not show the assignee highlight if the change is not open.
if (!change.assignee || if (!change ||!change.assignee ||
!account || !account ||
CLOSED_STATUS.indexOf(change.status) !== -1) { CLOSED_STATUS.indexOf(change.status) !== -1) {
return false; return false;

View File

@@ -353,6 +353,7 @@
}, },
_computeBranchURL(project, branch) { _computeBranchURL(project, branch) {
if (!this.change || !this.change.status) return '';
return Gerrit.Nav.getUrlForBranch(branch, project, return Gerrit.Nav.getUrlForBranch(branch, project,
this.change.status == this.ChangeStatus.NEW ? 'open' : this.change.status == this.ChangeStatus.NEW ? 'open' :
this.change.status.toLowerCase()); this.change.status.toLowerCase());

View File

@@ -524,6 +524,7 @@
}, },
_computeTotalCommentCounts(unresolvedCount, changeComments) { _computeTotalCommentCounts(unresolvedCount, changeComments) {
if (!changeComments) return undefined;
const draftCount = changeComments.computeDraftCount(); const draftCount = changeComments.computeDraftCount();
const unresolvedString = GrCountStringFormatter.computeString( const unresolvedString = GrCountStringFormatter.computeString(
unresolvedCount, 'unresolved'); unresolvedCount, 'unresolved');

View File

@@ -71,16 +71,17 @@
_computeDownloadCommands(change, patchNum, _selectedScheme) { _computeDownloadCommands(change, patchNum, _selectedScheme) {
let commandObj; let commandObj;
if (!change) return [];
for (const rev of Object.values(change.revisions || {})) { for (const rev of Object.values(change.revisions || {})) {
if (this.patchNumEquals(rev._number, patchNum) && if (this.patchNumEquals(rev._number, patchNum) &&
rev.fetch.hasOwnProperty(_selectedScheme)) { rev && rev.fetch && rev.fetch.hasOwnProperty(_selectedScheme)) {
commandObj = rev.fetch[_selectedScheme].commands; commandObj = rev.fetch[_selectedScheme].commands;
break; break;
} }
} }
const commands = []; const commands = [];
for (const title in commandObj) { for (const title in commandObj) {
if (!commandObj.hasOwnProperty(title)) { continue; } if (!commandObj || !commandObj.hasOwnProperty(title)) { continue; }
commands.push({ commands.push({
title, title,
command: commandObj[title], command: commandObj[title],
@@ -117,6 +118,10 @@
* @return {string} Not sure why there was a mismatch * @return {string} Not sure why there was a mismatch
*/ */
_computeDownloadLink(change, patchNum, opt_zip) { _computeDownloadLink(change, patchNum, opt_zip) {
// Polymer 2: check for undefined
if ([change, patchNum].some(arg => arg === undefined)) {
return '';
}
return this.changeBaseURL(change.project, change._number, patchNum) + return this.changeBaseURL(change.project, change._number, patchNum) +
'/patch?' + (opt_zip ? 'zip' : 'download'); '/patch?' + (opt_zip ? 'zip' : 'download');
}, },
@@ -130,6 +135,11 @@
* @return {string} * @return {string}
*/ */
_computeDownloadFilename(change, patchNum, opt_zip) { _computeDownloadFilename(change, patchNum, opt_zip) {
// Polymer 2: check for undefined
if ([change, patchNum].some(arg => arg === undefined)) {
return '';
}
let shortRev = ''; let shortRev = '';
for (const rev in change.revisions) { for (const rev in change.revisions) {
if (this.patchNumEquals(change.revisions[rev]._number, patchNum)) { if (this.patchNumEquals(change.revisions[rev]._number, patchNum)) {
@@ -141,6 +151,10 @@
}, },
_computeHidePatchFile(change, patchNum) { _computeHidePatchFile(change, patchNum) {
// Polymer 2: check for undefined
if ([change, patchNum].some(arg => arg === undefined)) {
return false;
}
for (const rev of Object.values(change.revisions || {})) { for (const rev of Object.values(change.revisions || {})) {
if (this.patchNumEquals(rev._number, patchNum)) { if (this.patchNumEquals(rev._number, patchNum)) {
const parentLength = rev.commit && rev.commit.parents ? const parentLength = rev.commit && rev.commit.parents ?
@@ -152,6 +166,10 @@
}, },
_computeArchiveDownloadLink(change, patchNum, format) { _computeArchiveDownloadLink(change, patchNum, format) {
// Polymer 2: check for undefined
if ([change, patchNum, format].some(arg => arg === undefined)) {
return '';
}
return this.changeBaseURL(change.project, change._number, patchNum) + return this.changeBaseURL(change.project, change._number, patchNum) +
'/archive?format=' + format; '/archive?format=' + format;
}, },

View File

@@ -760,6 +760,11 @@
}, },
_computeDiffURL(change, patchNum, basePatchNum, path, editMode) { _computeDiffURL(change, patchNum, basePatchNum, path, editMode) {
// Polymer 2: check for undefined
if ([change, patchNum, basePatchNum, path, editMode]
.some(arg => arg === undefined)) {
return;
}
// TODO(kaspern): Fix editing for commit messages and merge lists. // TODO(kaspern): Fix editing for commit messages and merge lists.
if (editMode && path !== this.COMMIT_MESSAGE_PATH && if (editMode && path !== this.COMMIT_MESSAGE_PATH &&
path !== this.MERGE_LIST_PATH) { path !== this.MERGE_LIST_PATH) {

View File

@@ -1213,22 +1213,6 @@ limitations under the License.
assert.isFalse(element.classList.contains('loading')); assert.isFalse(element.classList.contains('loading'));
}); });
test('no execute _computeDiffURL before patchNum is knwon', done => {
const urlStub = sandbox.stub(element, '_computeDiffURL');
element.change = {_number: 123};
element.patchRange = {patchNum: undefined, basePatchNum: 'PARENT'};
element._filesByPath = {'foo/bar.cpp': {}};
element.editMode = false;
flush(() => {
assert.isFalse(urlStub.called);
element.set('patchRange.patchNum', 4);
flush(() => {
assert.isTrue(urlStub.called);
done();
});
});
});
suite('size bars', () => { suite('size bars', () => {
test('_computeSizeBarLayout', () => { test('_computeSizeBarLayout', () => {
assert.isUndefined(element._computeSizeBarLayout(null)); assert.isUndefined(element._computeSizeBarLayout(null));

View File

@@ -110,6 +110,9 @@
}, },
_computeLabelValue(labels, permittedLabels, label) { _computeLabelValue(labels, permittedLabels, label) {
if ([labels, permittedLabels, label].some(arg => arg === undefined)) {
return null;
}
if (!labels[label.name]) { return null; } if (!labels[label.name]) { return null; }
const labelValue = this._getLabelValue(labels, permittedLabels, label); const labelValue = this._getLabelValue(labels, permittedLabels, label);
const len = permittedLabels[label.name] != null ? const len = permittedLabels[label.name] != null ?
@@ -138,7 +141,7 @@
}, },
_computeAnyPermittedLabelValues(permittedLabels, label) { _computeAnyPermittedLabelValues(permittedLabels, label) {
return permittedLabels.hasOwnProperty(label) && return permittedLabels && permittedLabels.hasOwnProperty(label) &&
permittedLabels[label].length; permittedLabels[label].length;
}, },

View File

@@ -203,6 +203,10 @@
}, },
_computeScoreClass(score, labelExtremes) { _computeScoreClass(score, labelExtremes) {
// Polymer 2: check for undefined
if ([score, labelExtremes].some(arg => arg === undefined)) {
return '';
}
const classes = []; const classes = [];
if (score.value > 0) { if (score.value > 0) {
classes.push('positive'); classes.push('positive');
@@ -211,6 +215,7 @@
} }
const extremes = labelExtremes[score.label]; const extremes = labelExtremes[score.label];
if (extremes) { if (extremes) {
const extremes = labelExtremes[score.label];
const intScore = parseInt(score.value, 10); const intScore = parseInt(score.value, 10);
if (intScore === extremes.max) { if (intScore === extremes.max) {
classes.push('max'); classes.push('max');

View File

@@ -223,6 +223,9 @@
* @return {!Object} Hash of arrays of comments, filename as key. * @return {!Object} Hash of arrays of comments, filename as key.
*/ */
_computeCommentsForMessage(changeComments, message) { _computeCommentsForMessage(changeComments, message) {
if ([changeComments, message].some(arg => arg === undefined)) {
return [];
}
const comments = changeComments.getAllPublishedComments(); const comments = changeComments.getAllPublishedComments();
if (message._index === undefined || !comments || !this.messages) { if (message._index === undefined || !comments || !this.messages) {
return []; return [];
@@ -271,8 +274,13 @@
* more visible messages in the list. * more visible messages in the list.
*/ */
_getDelta(visibleMessages, messages, hideAutomated) { _getDelta(visibleMessages, messages, hideAutomated) {
if ([visibleMessages, messages].some(arg => arg === undefined)) {
return 0;
}
let delta = MESSAGES_INCREMENT; let delta = MESSAGES_INCREMENT;
const msgsRemaining = messages.length - visibleMessages.length; const msgsRemaining = messages.length - visibleMessages.length;
if (hideAutomated) { if (hideAutomated) {
let counter = 0; let counter = 0;
let i; let i;
@@ -289,6 +297,10 @@
* exist in _visibleMessages. * exist in _visibleMessages.
*/ */
_numRemaining(visibleMessages, messages, hideAutomated) { _numRemaining(visibleMessages, messages, hideAutomated) {
if ([visibleMessages, messages].some(arg => arg === undefined)) {
return 0;
}
if (hideAutomated) { if (hideAutomated) {
return this._getHumanMessages(messages).length - return this._getHumanMessages(messages).length -
this._getHumanMessages(visibleMessages).length; this._getHumanMessages(visibleMessages).length;
@@ -311,6 +323,10 @@
_computeShowHideTextHidden(visibleMessages, messages, _computeShowHideTextHidden(visibleMessages, messages,
hideAutomated) { hideAutomated) {
if ([visibleMessages, messages].some(arg => arg === undefined)) {
return 0;
}
if (hideAutomated) { if (hideAutomated) {
messages = this._getHumanMessages(messages); messages = this._getHumanMessages(messages);
visibleMessages = this._getHumanMessages(visibleMessages); visibleMessages = this._getHumanMessages(visibleMessages);
@@ -334,7 +350,9 @@
}, },
_processedMessagesChanged(messages) { _processedMessagesChanged(messages) {
this._visibleMessages = messages.slice(-MAX_INITIAL_SHOWN_MESSAGES); if (messages) {
this._visibleMessages = messages.slice(-MAX_INITIAL_SHOWN_MESSAGES);
}
}, },
_computeNumMessagesText(visibleMessages, messages, _computeNumMessagesText(visibleMessages, messages,

View File

@@ -208,6 +208,9 @@
_computeChangeContainerClass(currentChange, relatedChange) { _computeChangeContainerClass(currentChange, relatedChange) {
const classes = ['changeContainer']; const classes = ['changeContainer'];
if ([relatedChange, currentChange].some(arg => arg === undefined)) {
return classes;
}
if (this._changesEqual(relatedChange, currentChange)) { if (this._changesEqual(relatedChange, currentChange)) {
classes.push('thisChange'); classes.push('thisChange');
} }
@@ -242,6 +245,9 @@
* @return {number} * @return {number}
*/ */
_getChangeNumber(change) { _getChangeNumber(change) {
// Default to 0 if change property is not defined.
if (!change) return 0;
if (change.hasOwnProperty('_change_number')) { if (change.hasOwnProperty('_change_number')) {
return change._change_number; return change._change_number;
} }

View File

@@ -891,6 +891,7 @@
* than SYNTAX_MAX_LINE_LENGTH. * than SYNTAX_MAX_LINE_LENGTH.
*/ */
_anyLineTooLong(diff) { _anyLineTooLong(diff) {
if (!diff) return false;
return diff.content.some(section => { return diff.content.some(section => {
const lines = section.ab ? const lines = section.ab ?
section.ab : section.ab :

View File

@@ -265,7 +265,8 @@
_getFiles(changeNum, patchRangeRecord) { _getFiles(changeNum, patchRangeRecord) {
// Polymer 2: check for undefined // Polymer 2: check for undefined
if ([changeNum, patchRangeRecord].some(arg => arg === undefined)) { if ([changeNum, patchRangeRecord, patchRangeRecord.base]
.some(arg => arg === undefined)) {
return; return;
} }
@@ -745,6 +746,9 @@
}, },
_getDiffUrl(change, patchRange, path) { _getDiffUrl(change, patchRange, path) {
if ([change, patchRange, path].some(arg => arg === undefined)) {
return '';
}
return Gerrit.Nav.getUrlForDiff(change, path, patchRange.patchNum, return Gerrit.Nav.getUrlForDiff(change, path, patchRange.patchNum,
patchRange.basePatchNum); patchRange.basePatchNum);
}, },
@@ -783,6 +787,9 @@
}, },
_getChangePath(change, patchRange, revisions) { _getChangePath(change, patchRange, revisions) {
if ([change, patchRange].some(arg => arg === undefined)) {
return '';
}
const range = this._getChangeUrlRange(patchRange, revisions); const range = this._getChangeUrlRange(patchRange, revisions);
return Gerrit.Nav.getUrlForChange(change, range.patchNum, return Gerrit.Nav.getUrlForChange(change, range.patchNum,
range.basePatchNum); range.basePatchNum);

View File

@@ -967,6 +967,7 @@
* @return {number} * @return {number}
*/ */
getDiffLength(diff) { getDiffLength(diff) {
if (!diff) return 0;
return diff.content.reduce((sum, sec) => { return diff.content.reduce((sum, sec) => {
if (sec.hasOwnProperty('ab')) { if (sec.hasOwnProperty('ab')) {
return sum + sec.ab.length; return sum + sec.ab.length;

View File

@@ -393,7 +393,7 @@
}, },
_isNewEmailValid(newEmail) { _isNewEmailValid(newEmail) {
return newEmail.includes('@'); return newEmail && newEmail.includes('@');
}, },
_computeAddEmailButtonEnabled(newEmail, addingEmail) { _computeAddEmailButtonEnabled(newEmail, addingEmail) {

View File

@@ -354,7 +354,7 @@
_computeSaveDisabled(draft, comment, resolved) { _computeSaveDisabled(draft, comment, resolved) {
// If resolved state has changed and a msg exists, save should be enabled. // If resolved state has changed and a msg exists, save should be enabled.
if (comment.unresolved === resolved && draft) { if (!comment || comment.unresolved === resolved && draft) {
return false; return false;
} }
return !draft || draft.trim() === ''; return !draft || draft.trim() === '';

View File

@@ -38,7 +38,7 @@
*/ */
_mapLabelInfo(labelInfo, account, changeLabelsRecord) { _mapLabelInfo(labelInfo, account, changeLabelsRecord) {
const result = []; const result = [];
if (!labelInfo) { return result; } if (!labelInfo || !account) { return result; }
if (!labelInfo.values) { if (!labelInfo.values) {
if (labelInfo.rejected || labelInfo.approved) { if (labelInfo.rejected || labelInfo.approved) {
const ok = labelInfo.approved || !labelInfo.rejected; const ok = labelInfo.approved || !labelInfo.rejected;

View File

@@ -61,6 +61,7 @@
* commentLink patterns * commentLink patterns
*/ */
_contentOrConfigChanged(content, config) { _contentOrConfigChanged(content, config) {
if (!Gerrit.Nav || !Gerrit.Nav.mapCommentlinks) return;
config = Gerrit.Nav.mapCommentlinks(config); config = Gerrit.Nav.mapCommentlinks(config);
const output = Polymer.dom(this.$.output); const output = Polymer.dom(this.$.output);
output.textContent = ''; output.textContent = '';

View File

@@ -314,26 +314,4 @@ limitations under the License.
assert.isTrue(contentConfigStub.called); assert.isTrue(contentConfigStub.called);
}); });
}); });
suite('gr-linked-text with null config', () => {
let element;
let sandbox;
setup(() => {
element = fixture('basic');
sandbox = sinon.sandbox.create();
});
teardown(() => {
sandbox.restore();
});
test('_contentOrConfigChanged not called without config', () => {
const contentStub = sandbox.stub(element, '_contentChanged');
const contentConfigStub = sandbox.stub(element, '_contentOrConfigChanged');
element.content = 'some text';
assert.isTrue(contentStub.called);
assert.isFalse(contentConfigStub.called);
});
});
</script> </script>

View File

@@ -228,9 +228,11 @@
* @param {string} text * @param {string} text
*/ */
GrLinkTextParser.prototype.parse = function(text) { GrLinkTextParser.prototype.parse = function(text) {
linkify(text, { if (text) {
callback: this.parseChunk.bind(this), linkify(text, {
}); callback: this.parseChunk.bind(this),
});
}
}; };
/** /**