Merge changes Icf5efac2,I0110348a * changes: Update URL generation in gr-change-view Accept PARENT as base patch number in Gerrit.Nav
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 f8a1ea2..5822ff9 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
@@ -619,13 +619,12 @@ } if (patchNum === currentPatchNum && this._patchRange.basePatchNum === 'PARENT') { - page.show(this.changePath(this._changeNum)); + Gerrit.Nav.navigateToChange(this._change); return; } } - const patchExpr = this._patchRange.basePatchNum === 'PARENT' ? patchNum : - this._patchRange.basePatchNum + '..' + patchNum; - page.show(this.changePath(this._changeNum) + '/' + patchExpr); + Gerrit.Nav.navigateToChange(this._change, patchNum, + this._patchRange.basePatchNum); }, _computeChangeUrl(change) { @@ -790,7 +789,7 @@ _handleCapitalRKey(e) { if (this.shouldSuppressKeyboardShortcut(e)) { return; } e.preventDefault(); - page.show('/c/' + this._change._number); + Gerrit.Nav.navigateToChange(this._change); }, _handleSKey(e) { @@ -835,7 +834,7 @@ _determinePageBack() { // Default backPage to '/' if user came to change view page // via an email link, etc. - page.show(this.backPage || '/'); + Gerrit.Nav.navigateToRelativeUrl(this.backPage || '/'); }, _handleLabelRemoved(splices, path) { @@ -883,7 +882,7 @@ // If the change was rebased, we need to reload the page with the // latest patch. if (e.detail.action === 'rebase') { - page.show(this.changePath(this._changeNum)); + Gerrit.Nav.navigateToChange(this._change); } }); },
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html index 797b6b3..dcbab09 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -37,12 +37,12 @@ suite('gr-change-view tests', () => { let element; let sandbox; - let showStub; + let navigateToChangeStub; const TEST_SCROLL_TOP_PX = 100; setup(() => { sandbox = sinon.sandbox.create(); - showStub = sandbox.stub(page, 'show'); + navigateToChangeStub = sandbox.stub(Gerrit.Nav, 'navigateToChange'); stub('gr-rest-api-interface', { getConfig() { return Promise.resolve({test: 'config'}); }, getAccount() { return Promise.resolve(null); }, @@ -65,14 +65,21 @@ }); test('U should navigate to / if no backPage set', () => { + const relativeNavStub = sandbox.stub(Gerrit.Nav, + 'navigateToRelativeUrl'); MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u'); - assert(showStub.lastCall.calledWithExactly('/')); + assert.isTrue(relativeNavStub.called); + assert.isTrue(relativeNavStub.lastCall.calledWithExactly('/')); }); test('U should navigate to backPage if set', () => { + const relativeNavStub = sandbox.stub(Gerrit.Nav, + 'navigateToRelativeUrl'); element.backPage = '/dashboard/self'; MockInteractions.pressAndReleaseKeyOn(element, 85, null, 'u'); - assert(showStub.lastCall.calledWithExactly('/dashboard/self')); + assert.isTrue(relativeNavStub.called); + assert.isTrue(relativeNavStub.lastCall.calledWithExactly( + '/dashboard/self')); }); test('A fires an error event when not logged in', done => { @@ -145,11 +152,14 @@ sandbox.stub(element.$.actions, 'reload'); - showStub.restore(); - showStub = sandbox.stub(page, 'show', arg => { - assert.equal(arg, '/c/42'); - done(); - }); + navigateToChangeStub.restore(); + navigateToChangeStub = sandbox.stub(Gerrit.Nav, 'navigateToChange', + (change, patchNum, basePatchNum) => { + assert.equal(change, element._change); + assert.isUndefined(patchNum); + assert.isUndefined(basePatchNum); + done(); + }); MockInteractions.pressAndReleaseKeyOn(element, 82, 'shift', 'r'); }); @@ -528,13 +538,13 @@ assert.equal(element.viewState.diffMode, 'UNIFIED'); numEvents++; if (numEvents == 1) { - assert(showStub.lastCall.calledWithExactly('/c/42/1'), - 'Should navigate to /c/42/1'); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 1, 'PARENT')); selectEl.value = '3'; element.fire('change', {}, {node: selectEl}); } else if (numEvents == 2) { - assert(showStub.lastCall.calledWithExactly('/c/42/3'), - 'Should navigate to /c/42/3'); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 3, 'PARENT')); done(); } }); @@ -577,13 +587,13 @@ selectEl.addEventListener('change', e => { numEvents++; if (numEvents == 1) { - assert(showStub.lastCall.calledWithExactly('/c/42/1'), - 'Should navigate to /c/42/1'); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 1, 'PARENT')); selectEl.value = '3'; element.fire('change', {}, {node: selectEl}); } else if (numEvents == 2) { - assert(showStub.lastCall.calledWithExactly('/c/42/3'), - 'Should navigate to /c/42/3'); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 3, 'PARENT')); done(); } }); @@ -650,12 +660,14 @@ }; element._changePatchNum(13); - assert(showStub.lastCall.calledWithExactly('/c/42/2..13')); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 13, '2')); element._patchRange.basePatchNum = 'PARENT'; element._changePatchNum(3); - assert(showStub.lastCall.calledWithExactly('/c/42/3')); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change, 3, 'PARENT')); }); test('related changes are updated and new patch selected after rebase', @@ -668,7 +680,8 @@ () => { return Promise.resolve(); }); const e = {detail: {action: 'rebase'}}; element._handleReloadChange(e).then(() => { - assert.isTrue(showStub.lastCall.calledWithExactly('/c/42')); + assert.isTrue(navigateToChangeStub.lastCall.calledWithExactly( + element._change)); done(); }); }); @@ -679,7 +692,7 @@ sandbox.stub(element.$.relatedChanges, 'reload'); const e = {detail: {action: 'abandon'}}; element._handleReloadChange(e).then(() => { - assert.isFalse(showStub.called); + assert.isFalse(navigateToChangeStub.called); done(); }); });
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 7607a44..d7efb8f 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
@@ -238,7 +238,7 @@ patchRange.basePatchNum = Polymer.dom(e).rootTarget.value; Gerrit.Nav.navigateToChange(this.change, patchRange.patchNum, - this._getBasePatchNum(patchRange)); + patchRange.basePatchNum); }, _updateDiffPreferences() { @@ -601,7 +601,7 @@ _openCursorFile() { const diff = this.$.diffCursor.getTargetDiffElement(); Gerrit.Nav.navigateToDiff(this.change, diff.path, - diff.patchRange.patchNum, this._getBasePatchNum(this.patchRange)); + diff.patchRange.patchNum, this.patchRange.basePatchNum); }, _openSelectedFile(opt_index) { @@ -611,7 +611,7 @@ if (!this._files[this.$.fileCursor.index]) { return; } Gerrit.Nav.navigateToDiff(this.change, this._files[this.$.fileCursor.index].__path, this.patchRange.patchNum, - this._getBasePatchNum(this.patchRange)); + this.patchRange.basePatchNum); }, _addDraftAtTarget() { @@ -637,12 +637,7 @@ _computeDiffURL(change, patchRange, path) { return Gerrit.Nav.getUrlForDiff(change, path, patchRange.patchNum, - this._getBasePatchNum(patchRange)); - }, - - _getBasePatchNum(patchRange) { - return patchRange.basePatchNum === 'PARENT' ? - undefined : patchRange.basePatchNum; + patchRange.basePatchNum); }, _computeFileDisplayName(path) {
diff --git a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html index 47655cf..f1c3618 100644 --- a/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html +++ b/polygerrit-ui/app/elements/core/gr-navigation/gr-navigation.html
@@ -52,6 +52,8 @@ console.warn('Use of uninitialized routing'); }; + const PARENT_PATCHNUM = 'PARENT'; + window.Gerrit.Nav = { View: { @@ -90,15 +92,15 @@ /** * Generate a URL for the given route parameters. * @param {Object} params - * @return {String} + * @return {string} */ _getUrlFor(params) { return this._generateUrl(params); }, /** - * @param {String} project The name of the project. - * @return {String} + * @param {string} project The name of the project. + * @return {string} */ getUrlForProject(project) { return this._getUrlFor({ @@ -108,10 +110,10 @@ }, /** - * @param {String} branch The name of the branch. - * @param {String} project The name of the project. - * @param {String} status The status to search. - * @return {String} + * @param {string} branch The name of the branch. + * @param {string} project The name of the project. + * @param {string} status The status to search. + * @return {string} */ getUrlForBranch(branch, project, status) { return this._getUrlFor({ @@ -123,8 +125,8 @@ }, /** - * @param {String} topic The name of the topic. - * @return {String} + * @param {string} topic The name of the topic. + * @return {string} */ getUrlForTopic(topic) { return this._getUrlFor({ @@ -136,11 +138,16 @@ /** * @param {!Object} change The change object. - * @param {Number} opt_patchNum - * @param {Number} opt_basePatchNum - * @return {String} + * @param {number} opt_patchNum + * @param {number|string} opt_basePatchNum The string 'PARENT' can be used + * for none. + * @return {string} */ getUrlForChange(change, opt_patchNum, opt_basePatchNum) { + if (opt_basePatchNum === PARENT_PATCHNUM) { + opt_basePatchNum = undefined; + } + this._checkPatchRange(opt_patchNum, opt_basePatchNum); return this._getUrlFor({ view: Gerrit.Nav.View.CHANGE, @@ -152,9 +159,10 @@ /** * @param {!Object} change The change object. - * @param {Number} opt_patchNum - * @param {Number} opt_basePatchNum - * @return {String} + * @param {number} opt_patchNum + * @param {number|string} opt_basePatchNum The string 'PARENT' can be used + * for none. + * @return {string} */ navigateToChange(change, opt_patchNum, opt_basePatchNum) { this._navigate(this.getUrlForChange(change, opt_patchNum, @@ -164,11 +172,16 @@ /** * @param {!Object} change The change object. * @param {!String} path The file path. - * @param {Number} opt_patchNum - * @param {Number} opt_basePatchNum - * @return {String} + * @param {number} opt_patchNum + * @param {number|string} opt_basePatchNum The string 'PARENT' can be used + * for none. + * @return {string} */ getUrlForDiff(change, path, opt_patchNum, opt_basePatchNum) { + if (opt_basePatchNum === PARENT_PATCHNUM) { + opt_basePatchNum = undefined; + } + this._checkPatchRange(opt_patchNum, opt_basePatchNum); return this._getUrlFor({ view: Gerrit.Nav.View.DIFF, @@ -182,8 +195,9 @@ /** * @param {!Object} change The change object. * @param {!String} path The file path. - * @param {Number} opt_patchNum - * @param {Number} opt_basePatchNum + * @param {number} opt_patchNum + * @param {number|string} opt_basePatchNum The string 'PARENT' can be used + * for none. */ navigateToDiff(change, path, opt_patchNum, opt_basePatchNum) { this._navigate(this.getUrlForDiff(change, path, opt_patchNum, @@ -191,8 +205,8 @@ }, /** - * @param {String} owner The name of the owner. - * @return {String} + * @param {string} owner The name of the owner. + * @return {string} */ getUrlForOwner(owner) { return this._getUrlFor({ @@ -200,6 +214,17 @@ owner, }); }, + + /** + * Navigate to an arbitrary relative URL. + * @param {!string} relativeUrl + */ + navigateToRelativeUrl(relativeUrl) { + if (!relativeUrl.startsWith('/')) { + throw new Error('navigateToRelativeUrl with non-relative URL'); + } + this._navigate(relativeUrl); + }, }; })(window); </script>