From 1e3853d5932ec67d26a1937bdc62c2ed1c73e673 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Sun, 6 Oct 2019 15:47:21 +0000 Subject: [PATCH] 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 --- .../gr-change-list/gr-change-list.js | 3 ++- .../gr-change-metadata/gr-change-metadata.js | 1 + .../change/gr-change-view/gr-change-view.js | 1 + .../gr-download-dialog/gr-download-dialog.js | 22 +++++++++++++++++-- .../change/gr-file-list/gr-file-list.js | 5 +++++ .../gr-file-list/gr-file-list_test.html | 16 -------------- .../gr-label-score-row/gr-label-score-row.js | 5 ++++- .../elements/change/gr-message/gr-message.js | 5 +++++ .../gr-messages-list/gr-messages-list.js | 20 ++++++++++++++++- .../gr-related-changes-list.js | 6 +++++ .../diff/gr-diff-host/gr-diff-host.js | 1 + .../diff/gr-diff-view/gr-diff-view.js | 9 +++++++- .../app/elements/diff/gr-diff/gr-diff.js | 1 + .../gr-settings-view/gr-settings-view.js | 2 +- .../elements/shared/gr-comment/gr-comment.js | 2 +- .../shared/gr-label-info/gr-label-info.js | 2 +- .../shared/gr-linked-text/gr-linked-text.js | 1 + .../gr-linked-text/gr-linked-text_test.html | 22 ------------------- .../shared/gr-linked-text/link-text-parser.js | 8 ++++--- 19 files changed, 82 insertions(+), 50 deletions(-) diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js index 8500e3b6cf..8803c83908 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js @@ -179,6 +179,7 @@ }, _computeColspan(changeTableColumns, labelNames) { + if (!changeTableColumns || !labelNames) return; return changeTableColumns.length + labelNames.length + NUMBER_FIXED_COLUMNS; }, @@ -250,7 +251,7 @@ _computeItemHighlight(account, change) { // Do not show the assignee highlight if the change is not open. - if (!change.assignee || + if (!change ||!change.assignee || !account || CLOSED_STATUS.indexOf(change.status) !== -1) { return false; diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js index dbb41f6154..9788975747 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js @@ -353,6 +353,7 @@ }, _computeBranchURL(project, branch) { + if (!this.change || !this.change.status) return ''; return Gerrit.Nav.getUrlForBranch(branch, project, this.change.status == this.ChangeStatus.NEW ? 'open' : this.change.status.toLowerCase()); diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js index 7a0700ff3e..4ffcc9bf1b 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js @@ -524,6 +524,7 @@ }, _computeTotalCommentCounts(unresolvedCount, changeComments) { + if (!changeComments) return undefined; const draftCount = changeComments.computeDraftCount(); const unresolvedString = GrCountStringFormatter.computeString( unresolvedCount, 'unresolved'); diff --git a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.js b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.js index ddac3be829..4fe902bd87 100644 --- a/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.js +++ b/polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.js @@ -71,16 +71,17 @@ _computeDownloadCommands(change, patchNum, _selectedScheme) { let commandObj; + if (!change) return []; for (const rev of Object.values(change.revisions || {})) { if (this.patchNumEquals(rev._number, patchNum) && - rev.fetch.hasOwnProperty(_selectedScheme)) { + rev && rev.fetch && rev.fetch.hasOwnProperty(_selectedScheme)) { commandObj = rev.fetch[_selectedScheme].commands; break; } } const commands = []; for (const title in commandObj) { - if (!commandObj.hasOwnProperty(title)) { continue; } + if (!commandObj || !commandObj.hasOwnProperty(title)) { continue; } commands.push({ title, command: commandObj[title], @@ -117,6 +118,10 @@ * @return {string} Not sure why there was a mismatch */ _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) + '/patch?' + (opt_zip ? 'zip' : 'download'); }, @@ -130,6 +135,11 @@ * @return {string} */ _computeDownloadFilename(change, patchNum, opt_zip) { + // Polymer 2: check for undefined + if ([change, patchNum].some(arg => arg === undefined)) { + return ''; + } + let shortRev = ''; for (const rev in change.revisions) { if (this.patchNumEquals(change.revisions[rev]._number, patchNum)) { @@ -141,6 +151,10 @@ }, _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 || {})) { if (this.patchNumEquals(rev._number, patchNum)) { const parentLength = rev.commit && rev.commit.parents ? @@ -152,6 +166,10 @@ }, _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) + '/archive?format=' + format; }, diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js index 180d1a371d..ef69ae4ad3 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js @@ -760,6 +760,11 @@ }, _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. if (editMode && path !== this.COMMIT_MESSAGE_PATH && path !== this.MERGE_LIST_PATH) { diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html index 50e7701638..22df07109c 100644 --- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html +++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html @@ -1213,22 +1213,6 @@ limitations under the License. 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', () => { test('_computeSizeBarLayout', () => { assert.isUndefined(element._computeSizeBarLayout(null)); diff --git a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row.js b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row.js index 9c3d69a4ed..a77b62421b 100644 --- a/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row.js +++ b/polygerrit-ui/app/elements/change/gr-label-score-row/gr-label-score-row.js @@ -110,6 +110,9 @@ }, _computeLabelValue(labels, permittedLabels, label) { + if ([labels, permittedLabels, label].some(arg => arg === undefined)) { + return null; + } if (!labels[label.name]) { return null; } const labelValue = this._getLabelValue(labels, permittedLabels, label); const len = permittedLabels[label.name] != null ? @@ -138,7 +141,7 @@ }, _computeAnyPermittedLabelValues(permittedLabels, label) { - return permittedLabels.hasOwnProperty(label) && + return permittedLabels && permittedLabels.hasOwnProperty(label) && permittedLabels[label].length; }, diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.js b/polygerrit-ui/app/elements/change/gr-message/gr-message.js index 4072508d65..7305cdb9d4 100644 --- a/polygerrit-ui/app/elements/change/gr-message/gr-message.js +++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.js @@ -203,6 +203,10 @@ }, _computeScoreClass(score, labelExtremes) { + // Polymer 2: check for undefined + if ([score, labelExtremes].some(arg => arg === undefined)) { + return ''; + } const classes = []; if (score.value > 0) { classes.push('positive'); @@ -211,6 +215,7 @@ } const extremes = labelExtremes[score.label]; if (extremes) { + const extremes = labelExtremes[score.label]; const intScore = parseInt(score.value, 10); if (intScore === extremes.max) { classes.push('max'); diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js index 80115c6131..9fa0290dd9 100644 --- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js +++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list.js @@ -223,6 +223,9 @@ * @return {!Object} Hash of arrays of comments, filename as key. */ _computeCommentsForMessage(changeComments, message) { + if ([changeComments, message].some(arg => arg === undefined)) { + return []; + } const comments = changeComments.getAllPublishedComments(); if (message._index === undefined || !comments || !this.messages) { return []; @@ -271,8 +274,13 @@ * more visible messages in the list. */ _getDelta(visibleMessages, messages, hideAutomated) { + if ([visibleMessages, messages].some(arg => arg === undefined)) { + return 0; + } + let delta = MESSAGES_INCREMENT; const msgsRemaining = messages.length - visibleMessages.length; + if (hideAutomated) { let counter = 0; let i; @@ -289,6 +297,10 @@ * exist in _visibleMessages. */ _numRemaining(visibleMessages, messages, hideAutomated) { + if ([visibleMessages, messages].some(arg => arg === undefined)) { + return 0; + } + if (hideAutomated) { return this._getHumanMessages(messages).length - this._getHumanMessages(visibleMessages).length; @@ -311,6 +323,10 @@ _computeShowHideTextHidden(visibleMessages, messages, hideAutomated) { + if ([visibleMessages, messages].some(arg => arg === undefined)) { + return 0; + } + if (hideAutomated) { messages = this._getHumanMessages(messages); visibleMessages = this._getHumanMessages(visibleMessages); @@ -334,7 +350,9 @@ }, _processedMessagesChanged(messages) { - this._visibleMessages = messages.slice(-MAX_INITIAL_SHOWN_MESSAGES); + if (messages) { + this._visibleMessages = messages.slice(-MAX_INITIAL_SHOWN_MESSAGES); + } }, _computeNumMessagesText(visibleMessages, messages, diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js index aa565503db..97241aea2a 100644 --- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js +++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js @@ -208,6 +208,9 @@ _computeChangeContainerClass(currentChange, relatedChange) { const classes = ['changeContainer']; + if ([relatedChange, currentChange].some(arg => arg === undefined)) { + return classes; + } if (this._changesEqual(relatedChange, currentChange)) { classes.push('thisChange'); } @@ -242,6 +245,9 @@ * @return {number} */ _getChangeNumber(change) { + // Default to 0 if change property is not defined. + if (!change) return 0; + if (change.hasOwnProperty('_change_number')) { return change._change_number; } diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js index a538dcfc75..228433337c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js @@ -891,6 +891,7 @@ * than SYNTAX_MAX_LINE_LENGTH. */ _anyLineTooLong(diff) { + if (!diff) return false; return diff.content.some(section => { const lines = section.ab ? section.ab : diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js index 854e3257fd..eb298c814c 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js @@ -265,7 +265,8 @@ _getFiles(changeNum, patchRangeRecord) { // Polymer 2: check for undefined - if ([changeNum, patchRangeRecord].some(arg => arg === undefined)) { + if ([changeNum, patchRangeRecord, patchRangeRecord.base] + .some(arg => arg === undefined)) { return; } @@ -745,6 +746,9 @@ }, _getDiffUrl(change, patchRange, path) { + if ([change, patchRange, path].some(arg => arg === undefined)) { + return ''; + } return Gerrit.Nav.getUrlForDiff(change, path, patchRange.patchNum, patchRange.basePatchNum); }, @@ -783,6 +787,9 @@ }, _getChangePath(change, patchRange, revisions) { + if ([change, patchRange].some(arg => arg === undefined)) { + return ''; + } const range = this._getChangeUrlRange(patchRange, revisions); return Gerrit.Nav.getUrlForChange(change, range.patchNum, range.basePatchNum); diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js index 7606928281..540d6e6d2b 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js @@ -967,6 +967,7 @@ * @return {number} */ getDiffLength(diff) { + if (!diff) return 0; return diff.content.reduce((sum, sec) => { if (sec.hasOwnProperty('ab')) { return sum + sec.ab.length; diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js index 7f0a754298..cc93c2ced5 100644 --- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js +++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js @@ -393,7 +393,7 @@ }, _isNewEmailValid(newEmail) { - return newEmail.includes('@'); + return newEmail && newEmail.includes('@'); }, _computeAddEmailButtonEnabled(newEmail, addingEmail) { diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js index cf2f57d647..50dfdce746 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.js @@ -354,7 +354,7 @@ _computeSaveDisabled(draft, comment, resolved) { // 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 !draft || draft.trim() === ''; diff --git a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.js b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.js index 2036f2a067..fa4a154d53 100644 --- a/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.js +++ b/polygerrit-ui/app/elements/shared/gr-label-info/gr-label-info.js @@ -38,7 +38,7 @@ */ _mapLabelInfo(labelInfo, account, changeLabelsRecord) { const result = []; - if (!labelInfo) { return result; } + if (!labelInfo || !account) { return result; } if (!labelInfo.values) { if (labelInfo.rejected || labelInfo.approved) { const ok = labelInfo.approved || !labelInfo.rejected; diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js index b1bd5cdc6b..e9d6e84f8f 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text.js @@ -61,6 +61,7 @@ * commentLink patterns */ _contentOrConfigChanged(content, config) { + if (!Gerrit.Nav || !Gerrit.Nav.mapCommentlinks) return; config = Gerrit.Nav.mapCommentlinks(config); const output = Polymer.dom(this.$.output); output.textContent = ''; diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html index 3344e1d356..f01a75c978 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/gr-linked-text_test.html @@ -314,26 +314,4 @@ limitations under the License. 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); - }); - }); diff --git a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js index cff345d155..b87aeefbf6 100644 --- a/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js +++ b/polygerrit-ui/app/elements/shared/gr-linked-text/link-text-parser.js @@ -228,9 +228,11 @@ * @param {string} text */ GrLinkTextParser.prototype.parse = function(text) { - linkify(text, { - callback: this.parseChunk.bind(this), - }); + if (text) { + linkify(text, { + callback: this.parseChunk.bind(this), + }); + } }; /**