opendev/gerrit
Code Issues Proposed changes

Replace patchNum compare with utility function

Browse Source 
In preparation for implementation of in-app editing, the instances of
parseInt(patchNum) must be swapped out, as a patchNum may now be either
a number or a string.

This change adds the patchNumEquals function to gr-patch-set-behavior,
and uses it everywhere patchNum is compared.

Bug: Issue 4437
Change-Id: Ib1176508cd88d60c79e952b99dd5f57b994baa77
This commit is contained in:
Kasper Nilsson
2017-07-21 14:07:48 -07:00
parent ddc0b75785
commit 9060cfd1a5
16 changed files with 80 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html
View File

@@ -30,6 +30,17 @@ limitations under the License.
/** @polymerBehavior Gerrit.PatchSetBehavior */
const PatchSetBehavior = {
/**
* As patchNum can be either a string (e.g. 'edit', 'PARENT') OR a number,
* this function checks for patchNum equality.
*
* @param {string|number} a
* @param {string|number} b
*/
patchNumEquals(a, b) {
return a + '' === b + '';
},
/**
* Given an object of revisions, get a particular revision based on patch
* num.
@@ -39,11 +50,9 @@ limitations under the License.
* @return {Object} The correspondent revision obj from {revisions}
*/
getRevisionByPatchNum(revisions, patchNum) {
patchNum = parseInt(patchNum, 10);
for (const rev in revisions) {
if (revisions.hasOwnProperty(rev) &&
revisions[rev]._number === patchNum) {
return revisions[rev];
for (const rev of Object.values(revisions || {})) {
if (PatchSetBehavior.patchNumEquals(rev._number, patchNum)) {
return rev;
}
}
},
@@ -75,7 +84,7 @@ limitations under the License.
}
}
patchNums.sort((a, b) => { return a.num - b.num; });
return this._computeWipForPatchSets(change, patchNums);
return PatchSetBehavior._computeWipForPatchSets(change, patchNums);
},
/**
@@ -124,15 +133,15 @@ limitations under the License.
* The promise is rejected on network error.
*/
fetchIsLatestKnown(change, restAPI) {
const knownLatest = this.computeLatestPatchNum(
this.computeAllPatchSets(change));
const knownLatest = PatchSetBehavior.computeLatestPatchNum(
PatchSetBehavior.computeAllPatchSets(change));
return restAPI.getChangeDetail(change._number)
.then(detail => {
if (!detail) {
return Promise.reject('Unable to check for latest patchset.');
}
const actualLatest = this.computeLatestPatchNum(
this.computeAllPatchSets(detail));
const actualLatest = PatchSetBehavior.computeLatestPatchNum(
PatchSetBehavior.computeAllPatchSets(detail));
return actualLatest <= knownLatest;
});
},

12
polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior_test.html
View File

@@ -161,5 +161,17 @@ limitations under the License.
.assertWip(5, false) // PS5 was marked ready for review
.assertWip(6, true); // PS6 was uploaded with WIP option
});
test('patchNumEquals', () => {
const equals = Gerrit.PatchSetBehavior.patchNumEquals;
assert.isFalse(equals('edit', 'PARENT'));
assert.isFalse(equals('edit', NaN));
assert.isFalse(equals(1, '2'));
assert.isTrue(equals(1, '1'));
assert.isTrue(equals(1, 1));
assert.isTrue(equals('edit', 'edit'));
assert.isTrue(equals('PARENT', 'PARENT'));
});
});
</script>

3
polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
View File

@@ -553,9 +553,8 @@
},
_getRevision(change, patchNum) {
const num = window.parseInt(patchNum, 10);
for (const rev of Object.values(change.revisions)) {
if (rev._number === num) {
if (this.patchNumEquals(rev._number, patchNum)) {
return rev;
}
}

15
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -360,7 +360,7 @@
},
_handlePatchChange(e) {
this._changePatchNum(parseInt(e.target.value, 10), true);
this._changePatchNum(e.target.value, true);
},
_handleReplyTap(e) {
@@ -595,7 +595,7 @@
/**
* Change active patch to the provided patch num.
* @param {number} patchNum the patchn number to be viewed.
* @param {number|string} patchNum the patchn number to be viewed.
* @param {boolean} opt_forceParams When set to true, the resulting URL will
* always include the patch range, even if the requested patchNum is
* known to be the latest.
@@ -609,7 +609,7 @@
} else {
currentPatchNum = this.computeLatestPatchNum(this._allPatchSets);
}
if (patchNum === currentPatchNum &&
if (this.patchNumEquals(patchNum, currentPatchNum) &&
this._patchRange.basePatchNum === 'PARENT') {
Gerrit.Nav.navigateToChange(this._change);
return;
@@ -674,8 +674,8 @@
},
_computePatchInfoClass(patchNum, allPatchSets) {
if (parseInt(patchNum, 10) ===
this.computeLatestPatchNum(allPatchSets)) {
const latestNum = this.computeLatestPatchNum(allPatchSets);
if (this.patchNumEquals(patchNum, latestNum)) {
return '';
}
return 'patchInfo--oldPatchSet';
@@ -931,8 +931,9 @@
this._change = change;
if (!this._patchRange || !this._patchRange.patchNum ||
this._patchRange.patchNum === currentRevision._number) {
// CommitInfo.commit is optional, and may need patching.
this.patchNumEquals(this._patchRange.patchNum,
currentRevision._number)) {
// CommitInfo.commit is optional, and may need patching.
if (!currentRevision.commit.commit) {
currentRevision.commit.commit = latestRevisionSha;
}

8
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -543,12 +543,12 @@ limitations under the License.
numEvents++;
if (numEvents == 1) {
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 1, 'PARENT'));
element._change, '1', 'PARENT'));
selectEl.nativeSelect.value = '3';
element.fire('change', {}, {node: selectEl.nativeSelect});
} else if (numEvents == 2) {
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 3, 'PARENT'));
element._change, '3', 'PARENT'));
done();
}
});
@@ -592,12 +592,12 @@ limitations under the License.
numEvents++;
if (numEvents == 1) {
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 1, 'PARENT'));
element._change, '1', 'PARENT'));
selectEl.nativeSelect.value = '3';
element.fire('change', {}, {node: selectEl.nativeSelect});
} else if (numEvents == 2) {
assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly(
element._change, 3, 'PARENT'));
element._change, '3', 'PARENT'));
done();
}
});

4
polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.html
View File

@@ -14,8 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
-->
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../shared/gr-download-commands/gr-download-commands.html">
<link rel="import" href="../../../styles/shared-styles.html">

13
polygerrit-ui/app/elements/change/gr-download-dialog/gr-download-dialog.js
View File

@@ -42,6 +42,7 @@
},
behaviors: [
Gerrit.PatchSetBehavior,
Gerrit.RESTClientBehavior,
],
@@ -63,10 +64,10 @@
_computeDownloadCommands(change, patchNum, _selectedScheme) {
let commandObj;
for (const rev in change.revisions) {
if (change.revisions[rev]._number === parseInt(patchNum, 10) &&
change.revisions[rev].fetch.hasOwnProperty(_selectedScheme)) {
commandObj = change.revisions[rev].fetch[_selectedScheme].commands;
for (const rev of Object.values(change.revisions || {})) {
if (this.patchNumEquals(rev._number, patchNum) &&
rev.fetch.hasOwnProperty(_selectedScheme)) {
commandObj = rev.fetch[_selectedScheme].commands;
break;
}
}
@@ -97,7 +98,7 @@
_computeDownloadFilename(change, patchNum, zip) {
let shortRev;
for (const rev in change.revisions) {
if (change.revisions[rev]._number === parseInt(patchNum, 10)) {
if (this.patchNumEquals(change.revisions[rev]._number, patchNum)) {
shortRev = rev.substr(0, 7);
break;
}
@@ -112,7 +113,7 @@
_computeSchemes(change, patchNum) {
for (const rev of Object.values(change.revisions || {})) {
if (rev._number === parseInt(patchNum, 10)) {
if (this.patchNumEquals(rev._number, patchNum)) {
const fetch = rev.fetch;
if (fetch) {
return Object.keys(fetch).sort();

2
polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
View File

@@ -305,7 +305,7 @@
getCommentsForPath(comments, patchNum, path) {
return (comments[path] || []).filter(c => {
return parseInt(c.patch_set, 10) === parseInt(patchNum, 10);
return this.patchNumEquals(c.patch_set, patchNum);
});
},

3
polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.html
View File

@@ -13,10 +13,11 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/base-url-behavior/base-url-behavior.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../../../styles/shared-styles.html">

3
polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.js
View File

@@ -63,6 +63,7 @@
behaviors: [
Gerrit.BaseUrlBehavior,
Gerrit.PatchSetBehavior,
Gerrit.RESTClientBehavior,
],
@@ -246,7 +247,7 @@
const connected = [];
let changeRevision;
for (const rev in change.revisions) {
if (change.revisions[rev]._number == patchNum) {
if (this.patchNumEquals(change.revisions[rev]._number, patchNum)) {
changeRevision = rev;
}
}

6
polygerrit-ui/app/elements/core/gr-router/gr-router.html
View File

@@ -13,9 +13,11 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<link rel="import" href="../../../behaviors/base-url-behavior/base-url-behavior.html">
<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/base-url-behavior/base-url-behavior.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../behaviors/gr-url-encoding-behavior.html">
<link rel="import" href="../../core/gr-navigation/gr-navigation.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-reporting/gr-reporting.html">

5
polygerrit-ui/app/elements/core/gr-router/gr-router.js
View File

@@ -38,6 +38,7 @@
});
const encode = window.Gerrit.URLEncodingBehavior.encodeURL;
const patchNumEquals = window.Gerrit.PatchSetBehavior.patchNumEquals;
function startRouter(generateUrl) {
const base = window.Gerrit.BaseUrlBehavior.getBaseUrl();
@@ -302,7 +303,8 @@
});
const normalizePatchRangeParams = params => {
if (params.basePatchNum && params.basePatchNum === params.patchNum) {
if (params.basePatchNum &&
patchNumEquals(params.basePatchNum, params.patchNum)) {
params.basePatchNum = null;
history.replaceState(null, null, generateUrl(params));
} else if (params.basePatchNum && !params.patchNum) {
@@ -416,6 +418,7 @@
Polymer({
is: 'gr-router',
behaviors: [Gerrit.PatchSetBehavior],
start() {
if (!app) { return; }
startRouter(this._generateUrl.bind(this));

1
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html
View File

@@ -15,6 +15,7 @@ limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../../behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html">
<link rel="import" href="../../../behaviors/rest-client-behavior/rest-client-behavior.html">
<link rel="import" href="../../../bower_components/iron-dropdown/iron-dropdown.html">

10
polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
View File

@@ -115,6 +115,7 @@
behaviors: [
Gerrit.KeyboardShortcutBehavior,
Gerrit.PatchSetBehavior,
Gerrit.RESTClientBehavior,
],
@@ -714,11 +715,10 @@
* @return {Promise} A promise that yields a comment map object.
*/
_loadCommentMap() {
const filterByRange = comment => {
const patchNum = comment.patch_set + '';
return patchNum === this._patchRange.patchNum ||
patchNum === this._patchRange.basePatchNum;
};
const filterByRange = comment =>
this.patchNumEquals(comment.patch_set, this._patchRange.patchNum) ||
this.patchNumEquals(comment.patch_set,
this._patchRange.basePatchNum);
return Promise.all([
this.$.restAPI.getDiffComments(this._changeNum),

2
polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.html
View File

@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<link rel="import" href="../../../behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html">
<link rel="import" href="../../core/gr-reporting/gr-reporting.html">
<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
<link rel="import" href="../gr-rest-api-interface/gr-rest-api-interface.html">

8
polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.js
View File

@@ -44,6 +44,8 @@
},
},
behaviors: [Gerrit.PatchSetBehavior],
Element,
EventType,
@@ -121,9 +123,9 @@
const change = detail.change;
const patchNum = detail.patchNum;
let revision;
for (const rev in change.revisions) {
if (change.revisions[rev]._number == patchNum) {
revision = change.revisions[rev];
for (const rev of Object.values(change.revisions || {})) {
if (this.patchNumEquals(rev._number, patchNum)) {
revision = rev;
break;
}
}