Render change edits on change and diff screens

ORIGINALLY: Iafd4b80af53624027c347f0f443b4cdfde292e29

This change makes the change view and the diff view change edit aware.
Mutation operations on edit itself, like editing files in editor, are
beyond the scope of this change.

Change edits must be fetched with a separate change edit endpoint and
merged into the change.revisions object. The _number of an editInfo is
'edit'. It has a special property called 'basePatchNum' that marks
which patch set the edit is based on. In patch set selectors, the edit
is sorted right after its basePatchNum.

Alternative implementation considerations:

It could be easier to handle edits on the Polygerrit UI, if

  GET /changes/<id>/detail endpoint

would optionally include the change edit in the resulting
change.revsions map.

TODOs:
* Overwrite change.current_revision with change edit commit in some
  use cases
* Mark the change edit as Change Edit in the header of the change view
* Allow for modification of the edit in the change/diff view
* Disable commenting on files in edit patchsets
* Modify file list rows to have appropriate actions when edit is
  selected (e.g. allow revert, disable marking as reviewed)
* Identify and whitelist valid change/revision actions for an edit 

Bug: Issue 4437
Change-Id: Ia4690d20954de730cd625ac76920e849beb12f7c

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

@@ -30,6 +30,8 @@ limitations under the License.
 
   /** @polymerBehavior Gerrit.PatchSetBehavior */
   const PatchSetBehavior = {
+    // Corresponds to the patchNum for an edit patchset.
+    EDIT_NAME: 'edit',
 
     /**
      * As patchNum can be either a string (e.g. 'edit', 'PARENT') OR a number,
@@ -57,6 +59,55 @@ limitations under the License.
       }
     },
 
+    /**
+     * Find change edit base revision if change edit exists.
+     *
+     * @param {!Array<!Object>} revisions The revisions array.
+     * @return {Object} change edit parent revision or null if change edit
+     *     doesn't exist.
+     */
+    findEditParentRevision(revisions) {
+      const editInfo =
+          revisions.find(info => info._number === PatchSetBehavior.EDIT_NAME);
+
+      if (!editInfo) { return null; }
+
+      return revisions.find(info => info._number === editInfo.basePatchNum) ||
+          null;
+    },
+
+    /**
+     * Find change edit base patch set number if change edit exists.
+     *
+     * @param {!Array<!Object>} revisions The revisions array.
+     * @return {number} Change edit patch set number or -1.
+     */
+    findEditParentPatchNum(revisions) {
+      const revisionInfo = PatchSetBehavior.findEditParentRevision(revisions);
+      return revisionInfo ? revisionInfo._number : -1;
+    },
+
+    /**
+     * Sort given revisions array according to the patch set number. The sort
+     * algorithm is change edit aware. Change edit has patch set number equals
+     * 0, but must appear after the patch set it was based on. Example: change
+     * edit is based on patch set 2, and another patch set was uploaded after
+     * change edit creation, the sorted order should be: 1, 2, (0|edit), 3.
+     *
+     * @param {!Array<!Object>} revisions The revisions array
+     * @return {!Array<!Object>} The sorted {revisions} array
+     */
+    sortRevisions(revisions) {
+      const editParent = PatchSetBehavior.findEditParentPatchNum(revisions);
+      // Map a normal patchNum to 2 * (patchNum - 1) + 1... I.e. 1 -> 1,
+      // 2 -> 3, 3 -> 5, etc.
+      // Map an edit to the patchNum of parent*2... I.e. edit on 2 -> 4.
+      const num = r => r._number === PatchSetBehavior.EDIT_NAME ?
+          2 * editParent :
+          2 * (r._number - 1) + 1;
+      return revisions.sort((a, b) => num(a) - num(b));
+    },
+
     /**
      * Construct a chronological list of patch sets derived from change details.
      * Each element of this list is an object with the following properties:
@@ -69,21 +120,26 @@ limitations under the License.
      * property and its log of change messages.
      *
      * @param {Object} change The change details
-     * @return {Array<Object>} Sorted list of patch set objects, as described
+     * @return {!Array<!Object>} Sorted list of patch set objects, as described
      *     above
      */
     computeAllPatchSets(change) {
       if (!change) { return []; }
-      const patchNums = [];
+      let patchNums = [];
-      for (const commit in change.revisions) {
+      if (change.revisions &&
-        if (change.revisions.hasOwnProperty(commit)) {
+          Object.keys(change.revisions).length) {
-          patchNums.push({
+        patchNums =
-            num: change.revisions[commit]._number,
+          PatchSetBehavior.sortRevisions(Object.values(change.revisions))
-            desc: change.revisions[commit].description,
+              .map(e => {
+                // TODO(kaspern): Mark which patchset an edit was made on, if an
+                // edit exists -- perhaps with a temporary description.
+                return {
+                  num: e._number,
+                  desc: e.description,
+                };
               });
       }
-      }
+      patchNums.sort((a, b) => a.num - b.num);
-      patchNums.sort((a, b) => { return a.num - b.num; });
       return PatchSetBehavior._computeWipForPatchSets(change, patchNums);
     },
 
@@ -91,9 +147,9 @@ limitations under the License.
      * Populate the wip properties of the given list of patch sets.
      *
      * @param {Object} change The change details
-     * @param {Array<Object>} patchNums Sorted list of patch set objects, as
+     * @param {!Array<!Object>} patchNums Sorted list of patch set objects, as
      *     generated by computeAllPatchSets
-     * @return {Array<Object>} The given list of patch set objects, with the
+     * @return {!Array<!Object>} The given list of patch set objects, with the
      *     wip property set on each of them
      */
     _computeWipForPatchSets(change, patchNums) {
@@ -122,15 +178,20 @@ limitations under the License.
 
     computeLatestPatchNum(allPatchSets) {
       if (!allPatchSets || !allPatchSets.length) { return undefined; }
+      if (allPatchSets[allPatchSets.length - 1].num ===
+          PatchSetBehavior.EDIT_NAME) {
+        return allPatchSets[allPatchSets.length - 2].num;
+      }
       return allPatchSets[allPatchSets.length - 1].num;
     },
 
     /**
      * Check whether there is no newer patch than the latest patch that was
      * available when this change was loaded.
-     * @return {Promise} A promise that yields true if the latest patch has been
+     *
-     *     loaded, and false if a newer patch has been uploaded in the meantime.
+     * @return {Promise<boolean>} A promise that yields true if the latest patch
-     *     The promise is rejected on network error.
+     *     has been loaded, and false if a newer patch has been uploaded in the
+     *     meantime. The promise is rejected on network error.
      */
     fetchIsLatestKnown(change, restAPI) {
       const knownLatest = PatchSetBehavior.computeLatestPatchNum(
@@ -145,6 +206,18 @@ limitations under the License.
             return actualLatest <= knownLatest;
           });
     },
+
+    /**
+     * @param {number|string} patchNum
+     * @param {!Array<!Object>} revisions A sorted array of revisions.
+     *
+     * @return {number} The index of the revision with the given patchNum.
+     */
+    findSortedIndex(patchNum, revisions) {
+      revisions = revisions || [];
+      const findNum = rev => rev._number + '' === patchNum + '';
+      return revisions.findIndex(findNum);
+    },
   };
 
   window.Gerrit = window.Gerrit || {};

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

@@ -173,5 +173,63 @@ limitations under the License.
       assert.isTrue(equals('edit', 'edit'));
       assert.isTrue(equals('PARENT', 'PARENT'));
     });
+
+    test('findEditParentRevision', () => {
+      const findParent = Gerrit.PatchSetBehavior.findEditParentRevision;
+      let revisions = [
+        {_number: 0},
+        {_number: 1},
+        {_number: 2},
+      ];
+      assert.strictEqual(findParent(revisions), null);
+
+      revisions = [...revisions, {_number: 'edit', basePatchNum: 3}];
+      assert.strictEqual(findParent(revisions), null);
+
+      revisions = [...revisions, {_number: 3}];
+      assert.deepEqual(findParent(revisions), {_number: 3});
+    });
+
+    test('findEditParentPatchNum', () => {
+      const findNum = Gerrit.PatchSetBehavior.findEditParentPatchNum;
+      let revisions = [
+        {_number: 0},
+        {_number: 1},
+        {_number: 2},
+      ];
+      assert.equal(findNum(revisions), -1);
+
+      revisions =
+          [...revisions, {_number: 'edit', basePatchNum: 3}, {_number: 3}];
+      assert.deepEqual(findNum(revisions), 3);
+    });
+
+    test('sortRevisions', () => {
+      const sort = Gerrit.PatchSetBehavior.sortRevisions;
+      const revisions = [
+        {_number: 0},
+        {_number: 2},
+        {_number: 1},
+      ];
+      const sorted = [
+        {_number: 0},
+        {_number: 1},
+        {_number: 2},
+      ];
+
+      assert.deepEqual(sort(revisions), sorted);
+
+      // Edit patchset should follow directly after its basePatchNum.
+      revisions.push({_number: 'edit', basePatchNum: 2});
+      sorted.push({_number: 'edit', basePatchNum: 2});
+      assert.deepEqual(sort(revisions), sorted);
+
+      revisions[3].basePatchNum = 0;
+      const edit = sorted.pop();
+      edit.basePatchNum = 0;
+      // Edit patchset should be at index 1.
+      sorted.splice(1, 0, edit);
+      assert.deepEqual(sort(revisions), sorted);
+    });
   });
 </script>

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html

@@ -459,8 +459,9 @@ limitations under the License.
               <select>
                 <template is="dom-repeat" items="[[_allPatchSets]]"
                     as="patchNum">
-                  <option value$="[[patchNum.num]]"
+                  <option
-                      disabled$="[[_computePatchSetDisabled(patchNum.num, _patchRange.basePatchNum)]]">
+                      value$="[[patchNum.num]]"
+                      disabled$="[[_computePatchSetDisabled(patchNum.num, _patchRange.basePatchNum, _sortedRevisions)]]">
                     [[patchNum.num]]
                     /
                     [[computeLatestPatchNum(_allPatchSets)]]
@@ -511,7 +512,7 @@ limitations under the License.
             patch-range="{{_patchRange}}"
             comments="[[_comments]]"
             drafts="[[_diffDrafts]]"
-            revisions="[[_change.revisions]]"
+            revisions="[[_sortedRevisions]]"
             project-config="[[_projectConfig]]"
             selected-index="{{viewState.selectedFileIndex}}"
             diff-view-mode="{{viewState.diffMode}}"

polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js

@@ -181,6 +181,7 @@
         value: true,
       },
       _updateCheckTimerHandle: Number,
+      _sortedRevisions: Array,
     },
 
     behaviors: [
@@ -195,6 +196,7 @@
     observers: [
       '_labelsChanged(_change.labels.*)',
       '_paramsAndChangeChanged(params, _change)',
+      '_updateSortedRevisions(_change.revisions.*)',
     ],
 
     keyBindings: {
@@ -243,6 +245,11 @@
       }
     },
 
+    _updateSortedRevisions(revisionsRecord) {
+      const revisions = revisionsRecord.base;
+      this._sortedRevisions = this.sortRevisions(Object.values(revisions));
+    },
+
     _computePrefsButtonHidden(prefs, loggedIn) {
       return !loggedIn || !prefs;
     },
@@ -684,13 +691,15 @@
     /**
      * Determines if a patch number should be disabled based on value of the
      * basePatchNum from gr-file-list.
-     * @param {Number} patchNum Patch number available in dropdown
+     * @param {number} patchNum Patch number available in dropdown
-     * @param {Number|String} basePatchNum Base patch number from file list
+     * @param {number|string} basePatchNum Base patch number from file list
-     * @return {Boolean}
+     * @return {boolean}
      */
     _computePatchSetDisabled(patchNum, basePatchNum) {
-      basePatchNum = basePatchNum === 'PARENT' ? 0 : basePatchNum;
+      if (basePatchNum === 'PARENT') { return false; }
-      return parseInt(patchNum, 10) <= parseInt(basePatchNum, 10);
+
+      return this.findSortedIndex(patchNum, this._sortedRevisions) <=
+          this.findSortedIndex(basePatchNum, this._sortedRevisions);
     },
 
     _computeLabelNames(labels) {
@@ -901,12 +910,24 @@
     },
 
     _getChangeDetail() {
-      return this.$.restAPI.getChangeDetail(this._changeNum,
+      const detailCompletes = this.$.restAPI.getChangeDetail(
-          this._handleGetChangeDetailError.bind(this)).then(change => {
+          this._changeNum, this._handleGetChangeDetailError.bind(this));
+      const editCompletes = this._getEdit();
+
+      return Promise.all([detailCompletes, editCompletes])
+          .then(([change, edit]) => {
             if (!change) {
               return '';
             }
             this._upgradeUrl(change, this.params);
+            if (edit) {
+              change.revisions[edit.commit.commit] = {
+                _number: this.EDIT_NAME,
+                basePatchNum: edit.base_patch_set_number,
+                commit: edit.commit,
+                fetch: edit.fetch,
+              };
+            }
             // Issue 4190: Coalesce missing topics to null.
             if (!change.topic) { change.topic = null; }
             if (!change.reviewer_updates) {
@@ -952,6 +973,10 @@
       });
     },
 
+    _getEdit() {
+      return this.$.restAPI.getChangeEdit(this._changeNum, true);
+    },
+
     _getLatestCommitMessage() {
       return this.$.restAPI.getChangeCommitInfo(this._changeNum,
           this.computeLatestPatchNum(this._allPatchSets)).then(commitInfo => {

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

@@ -58,6 +58,10 @@ limitations under the License.
     });
 
     suite('keyboard shortcuts', () => {
+      setup(() => {
+        sandbox.stub(element, '_updateSortedRevisions');
+      });
+
       test('S should toggle the CL star', () => {
         const starStub = sandbox.stub(element.$.changeStar, 'toggleStar');
         MockInteractions.pressAndReleaseKeyOn(element, 83, null, 's');
@@ -233,6 +237,12 @@ limitations under the License.
     });
 
     test('_computePatchSetDisabled', () => {
+      element._sortedRevisions = [
+        {_number: 1},
+        {_number: 2},
+        {_number: element.EDIT_NAME, basePatchNum: 2},
+        {_number: 3},
+      ];
       let basePatchNum = 'PARENT';
       let patchNum = 1;
       assert.equal(element._computePatchSetDisabled(patchNum, basePatchNum),
@@ -243,6 +253,12 @@ limitations under the License.
       patchNum = 2;
       assert.equal(element._computePatchSetDisabled(patchNum, basePatchNum),
           false);
+      basePatchNum = element.EDIT_NAME;
+      assert.equal(element._computePatchSetDisabled(patchNum, basePatchNum),
+          true);
+      patchNum = '3';
+      assert.equal(element._computePatchSetDisabled(patchNum, basePatchNum),
+          false);
     });
 
     test('_prepareCommitMsgForLinkify', () => {
@@ -470,6 +486,7 @@ limitations under the License.
     });
 
     test('change num change', () => {
+      sandbox.stub(element, '_updateSortedRevisions');
       element._changeNum = null;
       element._patchRange = {
         basePatchNum: 'PARENT',
@@ -852,6 +869,37 @@ limitations under the License.
       });
     });
 
+    test('edit is added to change', () => {
+      sandbox.stub(element, '_changeChanged');
+      sandbox.stub(element, '_upgradeUrl');
+      sandbox.stub(element.$.restAPI, 'getChangeDetail', () => {
+        return Promise.resolve({
+          id: '123456789',
+          labels: {},
+          current_revision: 'foo',
+          revisions: {foo: {commit: {}}},
+        });
+      });
+      sandbox.stub(element, '_getEdit', () => {
+        return Promise.resolve({
+          base_patch_set_number: 1,
+          commit: {commit: 'bar'},
+        });
+      });
+
+      return element._getChangeDetail().then(() => {
+        const revs = element._change.revisions;
+        assert.equal(Object.keys(revs).length, 2);
+        assert.deepEqual(revs['foo'], {commit: {commit: 'foo'}});
+        assert.deepEqual(revs['bar'], {
+          _number: element.EDIT_NAME,
+          basePatchNum: 1,
+          commit: {commit: 'bar'},
+          fetch: undefined,
+        });
+      });
+    });
+
     test('reply dialog focus can be controlled', () => {
       const FocusTarget = element.$.replyDialog.FocusTarget;
       const openStub = sandbox.stub(element, '_openReplyDialog');
@@ -966,6 +1014,7 @@ limitations under the License.
     suite('reply dialog tests', () => {
       setup(() => {
         sandbox.stub(element.$.replyDialog, '_draftChanged');
+        sandbox.stub(element, '_updateSortedRevisions');
         sandbox.stub(element.$.replyDialog, 'fetchIsLatestKnown',
             () => { return Promise.resolve(true); });
         element._change = {labels: {}};

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html

@@ -234,7 +234,7 @@ limitations under the License.
                   items="[[computeAllPatchSets(change)]]"
                   as="patchNum">
                 <option
-                    disabled$="[[_computePatchSetDisabled(patchNum.num, patchRange.patchNum)]]"
+                    disabled$="[[_computePatchSetDisabled(patchNum.num, patchRange.patchNum, revisions)]]"
                     value$="[[patchNum.num]]">
                   [[patchNum.num]]
                   [[patchNum.desc]]

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

@@ -41,7 +41,8 @@
       changeNum: String,
       comments: Object,
       drafts: Object,
-      revisions: Object,
+      // Already sorted by the change-view.
+      revisions: Array,
       projectConfig: Object,
       selectedIndex: {
         type: Number,
@@ -116,6 +117,7 @@
         type: Boolean,
         observer: '_loadingChanged',
       },
+      _sortedRevisions: Array,
     },
 
     behaviors: [
@@ -231,7 +233,8 @@
     },
 
     _computePatchSetDisabled(patchNum, currentPatchNum) {
-      return parseInt(patchNum, 10) >= parseInt(currentPatchNum, 10);
+      return this.findSortedIndex(patchNum, this.revisions) >=
+          this.findSortedIndex(currentPatchNum, this.revisions);
     },
 
     _togglePathExpanded(path) {

polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html

@@ -664,21 +664,29 @@ limitations under the License.
         revisions: {
           rev1: {_number: 1},
           rev2: {_number: 2},
-          rev3: {_number: 3},
+          rev3: {_number: 'edit', basePatchNum: 2},
+          rev4: {_number: 3},
         },
       };
+      element.revisions = [
+        {_number: 1},
+        {_number: 2},
+        {_number: 'edit', basePatchNum: 2},
+        {_number: 3},
+      ];
+
       flush(() => {
         const selectEl = element.$.patchChange;
         assert.equal(selectEl.nativeSelect.value, 'PARENT');
         assert.isTrue(element.$$('option[value="3"]').hasAttribute('disabled'));
         selectEl.addEventListener('change', () => {
-          assert.equal(selectEl.nativeSelect.value, '2');
+          assert.equal(selectEl.nativeSelect.value, 'edit');
-          assert(navStub.lastCall.calledWithExactly(element.change, '3', '2'),
+          assert(navStub.lastCall.calledWithExactly(element.change, '3', 'edit'),
-              'Should navigate to /c/42/2..3');
+              'Should navigate to /c/42/edit..3');
           navStub.restore();
           done();
         });
-        selectEl.nativeSelect.value = '2';
+        selectEl.nativeSelect.value = 'edit';
         element.fire('change', {}, {node: selectEl.nativeSelect});
       });
     });

polygerrit-ui/app/elements/core/gr-router/gr-router.js

@@ -39,6 +39,7 @@
 
   const encode = window.Gerrit.URLEncodingBehavior.encodeURL;
   const patchNumEquals = window.Gerrit.PatchSetBehavior.patchNumEquals;
+  const EDIT_NAME = window.Gerrit.PatchSetBehavior.EDIT_NAME;
 
   function startRouter(generateUrl) {
     const base = window.Gerrit.BaseUrlBehavior.getBaseUrl();
@@ -419,18 +420,27 @@
       if (params.basePatchNum &&
           patchNumEquals(params.basePatchNum, params.patchNum)) {
         params.basePatchNum = null;
-        upgradeUrl(params);
       } else if (params.basePatchNum && !params.patchNum) {
         params.patchNum = params.basePatchNum;
         params.basePatchNum = null;
       }
+      // In GWTUI, edits are represented in URLs with either 0 or 'edit'.
+      // TODO(kaspern): Remove this normalization when GWT UI is gone.
+      if (patchNumEquals(params.basePatchNum, 0)) {
+        params.basePatchNum = EDIT_NAME;
+      }
+      if (patchNumEquals(params.patchNum, 0)) {
+        params.patchNum = EDIT_NAME;
+      }
+      upgradeUrl(params);
     };
 
     // Matches
-    // /c/<project>/+/<changeNum>/[<basePatchNum>..][<patchNum>]/[path].
+    // /c/<project>/+/<changeNum>/[<basePatchNum|edit>..][<patchNum|edit>]/[path].
     // TODO(kaspern): Migrate completely to project based URLs, with backwards
     // compatibility for change-only.
-    page(/^\/c\/(.+)\/\+\/(\d+)(\/?((\d+)(\.\.(\d+))?(\/(.+))?))?\/?$/,
+    // eslint-disable-next-line max-len
+    page(/^\/c\/(.+)\/\+\/(\d+)(\/?((\d+|edit)(\.\.(\d+|edit))?(\/(.+))?))?\/?$/,
         ctx => {
           // Parameter order is based on the regex group number matched.
           const params = {
@@ -448,7 +458,7 @@
         });
 
     // Matches /c/<changeNum>/[<basePatchNum>..][<patchNum>][/].
-    page(/^\/c\/(\d+)\/?(((\d+)(\.\.(\d+))?))?\/?$/, ctx => {
+    page(/^\/c\/(\d+)\/?(((\d+|edit)(\.\.(\d+|edit))?))?\/?$/, ctx => {
       // Parameter order is based on the regex group number matched.
       const params = {
         changeNum: ctx.params[0],
@@ -462,7 +472,7 @@
     });
 
     // Matches /c/<changeNum>/[<basePatchNum>..]<patchNum>/<path>.
-    page(/^\/c\/(\d+)\/((\d+)(\.\.(\d+))?)\/(.+)/, ctx => {
+    page(/^\/c\/(\d+)\/((\d+|edit)(\.\.(\d+|edit))?)\/(.+)/, ctx => {
       // Check if path has an '@' which indicates it was using GWT style line
       // numbers. Even if the filename had an '@' in it, it would have already
       // been URI encoded. Redirect to hash version of path.

polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html

@@ -271,7 +271,7 @@ limitations under the License.
               change-num="[[_changeNum]]"
               patch-range="[[_patchRange]]"
               files-weblinks="[[_filesWeblinks]]"
-              available-patches="[[_computeAvailablePatches(_change.revisions)]]"
+              available-patches="[[_computeAvailablePatches(_change.revisions, _change.revisions.*)]]"
               revisions="[[_change.revisions]]">
           </gr-patch-range-select>
           <span class="download desktop">

polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js

@@ -176,6 +176,10 @@
       }
     },
 
+    _getChangeEdit(changeNum) {
+      return this.$.restAPI.getChangeEdit(this._changeNum);
+    },
+
     _getFiles(changeNum, patchRangeRecord) {
       const patchRange = patchRangeRecord.base;
       return this.$.restAPI.getChangeFilePathsAsSpeciallySortedArray(
@@ -495,7 +499,17 @@
 
       promises.push(this._loadComments());
 
-      Promise.all(promises).then(() => {
+      promises.push(this._getChangeEdit(this._changeNum));
+
+      Promise.all(promises).then(r => {
+        const edit = r[4];
+        if (edit) {
+          this.set('_change.revisions.' + edit.commit.commit, {
+            _number: this.EDIT_NAME,
+            basePatchNum: edit.base_patch_set_number,
+            commit: edit.commit,
+          });
+        }
         this._loading = false;
         this.$.diff.comments = this._commentsForDiff;
         this.$.diff.reload();
@@ -562,13 +576,8 @@
       return patchStr;
     },
 
-    _computeAvailablePatches(revisions) {
+    _computeAvailablePatches(revs) {
-      const patchNums = [];
+      return this.sortRevisions(Object.values(revs)).map(e => e._number);
-      if (!revisions) { return patchNums; }
-      for (const rev of Object.values(revisions)) {
-        patchNums.push(rev._number);
-      }
-      return patchNums.sort((a, b) => { return a - b; });
     },
 
     /**

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

@@ -30,10 +30,13 @@
         observer: '_updateSelected',
       },
       revisions: Object,
+      _sortedRevisions: Array,
       _rightSelected: String,
       _leftSelected: String,
     },
 
+    observers: ['_updateSortedRevisions(revisions.*)'],
+
     behaviors: [Gerrit.PatchSetBehavior],
 
     _updateSelected() {
@@ -41,6 +44,11 @@
       this._leftSelected = this.patchRange.basePatchNum;
     },
 
+    _updateSortedRevisions(revisionsRecord) {
+      const revisions = revisionsRecord.base;
+      this._sortedRevisions = this.sortRevisions(Object.values(revisions));
+    },
+
     _handlePatchChange(e) {
       const leftPatch = this._leftSelected;
       const rightPatch = this._rightSelected;
@@ -53,12 +61,15 @@
     },
 
     _computeLeftDisabled(patchNum, patchRange) {
-      return parseInt(patchNum, 10) >= parseInt(patchRange.patchNum, 10);
+      return this.findSortedIndex(patchNum, this._sortedRevisions) >=
+          this.findSortedIndex(patchRange.patchNum, this._sortedRevisions);
     },
 
     _computeRightDisabled(patchNum, patchRange) {
       if (patchRange.basePatchNum == 'PARENT') { return false; }
-      return parseInt(patchNum, 10) <= parseInt(patchRange.basePatchNum, 10);
+
+      return this.findSortedIndex(patchNum, this._sortedRevisions) <=
+          this.findSortedIndex(patchRange.basePatchNum, this._sortedRevisions);
     },
 
     // On page load, the dom-if for options getting added occurs after

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

@@ -36,16 +36,26 @@ limitations under the License.
 <script>
   suite('gr-patch-range-select tests', () => {
     let element;
+    let sandbox;
 
     setup(() => {
       element = fixture('basic');
+      sandbox = sinon.sandbox.create();
     });
 
+    teardown(() => sandbox.restore());
+
     test('enabled/disabled options', () => {
       const patchRange = {
         basePatchNum: 'PARENT',
         patchNum: '3',
       };
+      element._sortedRevisions = [
+        {_number: 1},
+        {_number: 2},
+        {_number: element.EDIT_NAME, basePatchNum: 2},
+        {_number: 3},
+      ];
       for (const patchNum of ['1', '2', '3']) {
         assert.isFalse(element._computeRightDisabled(patchNum, patchRange));
       }
@@ -54,18 +64,21 @@ limitations under the License.
       }
       assert.isTrue(element._computeLeftDisabled('3', patchRange));
 
-      patchRange.basePatchNum = '2';
+      patchRange.basePatchNum = element.EDIT_NAME;
       assert.isTrue(element._computeLeftDisabled('3', patchRange));
       assert.isTrue(element._computeRightDisabled('1', patchRange));
       assert.isTrue(element._computeRightDisabled('2', patchRange));
       assert.isFalse(element._computeRightDisabled('3', patchRange));
+      assert.isTrue(element._computeRightDisabled(element.EDIT_NAME, patchRange));
     });
 
     test('navigation', done => {
-      const showStub = sinon.stub(page, 'show');
+      sandbox.stub(element, '_computeLeftDisabled').returns(false);
+      sandbox.stub(element, '_computeRightDisabled').returns(false);
+      const showStub = sandbox.stub(page, 'show');
       const leftSelectEl = element.$.leftPatchSelect;
       const rightSelectEl = element.$.rightPatchSelect;
-      const blurSpy = sinon.spy(leftSelectEl, 'blur');
+      const blurSpy = sandbox.spy(leftSelectEl, 'blur');
       element.changeNum = '42';
       element.path = 'path/to/file.txt';
       element.availablePatches = ['1', '2', '3'];
@@ -89,7 +102,6 @@ limitations under the License.
           assert(showStub.lastCall.calledWithExactly(
               '/c/42/1..3/path/to/file.txt'),
               'Should navigate to /c/42/1..3/path/to/file.txt');
-          showStub.restore();
           done();
         }
       });

polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js

@@ -899,6 +899,16 @@
           .then(() => this.send('POST', url, review, opt_errFn, opt_ctx));
     },
 
+    getChangeEdit(changeNum, opt_download_commands) {
+      return this.getLoggedIn().then(loggedIn => {
+        return loggedIn ?
+          this.fetchJSON(
+              this.getChangeActionURL(changeNum, null, '/edit/'), null, null,
+              opt_download_commands ? {'download-commands': true} : null) :
+          false;
+      });
+    },
+
     getFileInChangeEdit(changeNum, path) {
       return this.send('GET',
           this.getChangeActionURL(changeNum, null,