UI side of patchset descriptions

Adds an editable label allowing the change owner to edit patchset
descriptions within the UI.

Also re-styles the dropdowns pertaining to patchsets for better UX.

Feature: Issue 4544
Change-Id: I15c0f8ab6a60c3a06482fe152539615d31eeae0f

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

@@ -23,11 +23,11 @@ limitations under the License.
      * Given an object of revisions, get a particular revision based on patch
      * num.
      *
-     * @param {Object} revisions
+     * @param {Object} revisions The object of revisions given by the API
-     * @param {number|string} patchNum
+     * @param {number|string} patchNum The number index of the revision
-     * @return {Object}
+     * @return {Object} The correspondent revision obj from {revisions}
      */
-    getRevisionNumber: function(revisions, patchNum) {
+    getRevisionByPatchNum: function(revisions, patchNum) {
       patchNum = parseInt(patchNum, 10);
       for (var rev in revisions) {
         if (revisions.hasOwnProperty(rev) &&

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

@@ -23,8 +23,8 @@ limitations under the License.
 
 <script>
   suite('gr-path-list-behavior tests', function() {
-    test('getRevisionNumber', function() {
+    test('getRevisionByPatchNum', function() {
-      var get = Gerrit.PatchSetBehavior.getRevisionNumber;
+      var get = Gerrit.PatchSetBehavior.getRevisionByPatchNum;
       var revisions = [
         {_number: 0},
         {_number: 1},

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

@@ -24,6 +24,7 @@ limitations under the License.
 <link rel="import" href="../../shared/gr-change-star/gr-change-star.html">
 <link rel="import" href="../../shared/gr-date-formatter/gr-date-formatter.html">
 <link rel="import" href="../../shared/gr-editable-content/gr-editable-content.html">
+<link rel="import" href="../../shared/gr-editable-label/gr-editable-label.html">
 <link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
 <link rel="import" href="../../shared/gr-linked-text/gr-linked-text.html">
 <link rel="import" href="../../shared/gr-overlay/gr-overlay.html">
@@ -161,7 +162,10 @@ limitations under the License.
         display: none;
       }
       .patchSetSelect {
-        max-width: 25em;
+        max-width: 8em;
+      }
+      gr-editable-label.descriptionLabel {
+        max-width: 15em;
       }
       @media screen and (max-width: 50em) {
         .header {
@@ -296,15 +300,25 @@ limitations under the License.
                 class="patchSetSelect"
                 on-change="_handlePatchChange">
               <template is="dom-repeat" items="[[_allPatchSets]]"
-                  as="patchNumber">
+                  as="patchNum">
-                <option value$="[[patchNumber]]">
+                <option value$="[[patchNum.num]]" label>
-                  [[patchNumber]]
+                  [[patchNum.num]]
                   /
                   [[_computeLatestPatchNum(_allPatchSets)]]
-                  [[_computePatchSetDescription(_change, patchNumber)]]
+                  [[patchNum.desc]]
                 </option>
               </template>
             </select>
+            <span class="descriptionContainer">
+              /
+              <gr-editable-label
+                  id="descriptionLabel"
+                  class="descriptionLabel"
+                  value="[[_computePatchSetDescription(_change, _selectedPatchSet)]]"
+                  placeholder="[[_computeDescriptionPlaceholder(_descriptionReadOnly)]]"
+                  read-only="[[_descriptionReadOnly]]"
+                  on-changed="_handleDescriptionChanged"></gr-editable-label>
+            </span>
             <span class="downloadContainer">
               /
               <gr-button link

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

@@ -49,6 +49,10 @@
         value: function() { return document.body; },
       },
 
+      _account: {
+        type: Object,
+        value: {},
+      },
       _comments: Object,
       _change: {
         type: Object,
@@ -81,7 +85,7 @@
       _currentRevisionActions: Object,
       _allPatchSets: {
         type: Array,
-        computed: '_computeAllPatchSets(_change)',
+        computed: '_computeAllPatchSets(_change, _change.revisions.*)',
       },
       _loggedIn: {
         type: Boolean,
@@ -99,6 +103,10 @@
         type: Boolean,
         value: false,
       },
+      _descriptionReadOnly: {
+        type: Boolean,
+        computed: '_computeDescriptionReadOnly(_loggedIn, _change, _account)',
+      },
     },
 
     behaviors: [
@@ -122,6 +130,11 @@
     attached: function() {
       this._getLoggedIn().then(function(loggedIn) {
         this._loggedIn = loggedIn;
+        if (loggedIn) {
+          this.$.restAPI.getAccount().then(function(acct) {
+            this._account = acct;
+          }.bind(this));
+        }
       }.bind(this));
 
       this.addEventListener('comment-save', this._handleCommentSave.bind(this));
@@ -505,7 +518,7 @@
     _computeChangeStatus: function(change, patchNum) {
       var statusString;
       if (change.status === this.ChangeStatus.NEW) {
-        var rev = this.getRevisionNumber(change.revisions, patchNum);
+        var rev = this.getRevisionByPatchNum(change.revisions, patchNum);
         if (rev && rev.draft === true) {
           statusString = 'Draft';
         }
@@ -516,7 +529,7 @@
     },
 
     _computeLatestPatchNum: function(allPatchSets) {
-      return allPatchSets[allPatchSets.length - 1];
+      return allPatchSets[allPatchSets.length - 1].num;
     },
 
     _computePatchInfoClass: function(patchNum, allPatchSets) {
@@ -529,12 +542,15 @@
 
     _computeAllPatchSets: function(change) {
       var patchNums = [];
-      for (var rev in change.revisions) {
+      for (var commit in change.revisions) {
-        patchNums.push(change.revisions[rev]._number);
+        if (change.revisions.hasOwnProperty(commit)) {
-      }
+          patchNums.push({
-      return patchNums.sort(function(a, b) {
+            num: change.revisions[commit]._number,
-        return a - b;
+            desc: change.revisions[commit].description,
           });
+        }
+      }
+      return patchNums.sort(function(a, b) { return a.num - b.num; });
     },
 
     _computeLabelNames: function(labels) {
@@ -581,7 +597,7 @@
 
     _switchToMostRecentPatchNum: function() {
       this._getChangeDetail().then(function() {
-        var patchNum = this._allPatchSets[this._allPatchSets.length - 1];
+        var patchNum = this._computeLatestPatchNum(this._allPatchSets);
         if (patchNum !== this._patchRange.patchNum) {
           this._changePatchNum(patchNum);
         }
@@ -811,8 +827,45 @@
     },
 
     _computePatchSetDescription: function(change, patchNum) {
-      var rev = this.getRevisionNumber(change.revisions, patchNum);
+      var rev = this.getRevisionByPatchNum(change.revisions, patchNum);
       return (rev && rev.description) ? rev.description : '';
     },
+
+    _computeDescriptionPlaceholder: function(readOnly) {
+      return (readOnly ? 'No' : 'Add a') + ' patch set description';
+    },
+
+    _handleDescriptionChanged: function(e) {
+      var desc = e.detail.trim();
+      var rev = this.getRevisionByPatchNum(this._change.revisions,
+          this._selectedPatchSet);
+      var sha = this._getPatchsetHash(this._change.revisions, rev);
+      this.$.restAPI.setDescription(this._changeNum,
+          this._selectedPatchSet, desc)
+          .then(function(res) {
+            if (res.ok) {
+              this.set(['_change', 'revisions', sha, 'description'], desc);
+            }
+        }.bind(this));
+    },
+
+
+    /**
+     * @param {Object} revisions The revisions object keyed by revision hashes
+     * @param {Object} patchSet A revision already fetched from {revisions}
+     * @return {string} the SHA hash corresponding to the revision.
+     */
+    _getPatchsetHash: function(revisions, patchSet) {
+      for (var rev in revisions) {
+        if (revisions.hasOwnProperty(rev) &&
+            revisions[rev] === patchSet) {
+          return rev;
+        }
+      }
+    },
+
+    _computeDescriptionReadOnly: function(loggedIn, change, account) {
+      return !(loggedIn && (account._account_id === change.owner._account_id));
+    },
   });
 })();

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

@@ -127,6 +127,58 @@ limitations under the License.
       });
     });
 
+    test('_computeDescriptionReadOnly', function() {
+      assert.equal(element._computeDescriptionReadOnly(false,
+          {owner: {_account_id: 1}}, {_account_id: 1}), true);
+      assert.equal(element._computeDescriptionReadOnly(true,
+          {owner: {_account_id: 0}}, {_account_id: 1}), true);
+      assert.equal(element._computeDescriptionReadOnly(true,
+          {owner: {_account_id: 1}}, {_account_id: 1}), false);
+    });
+
+    test('_computeDescriptionPlaceholder', function() {
+      assert.equal(element._computeDescriptionPlaceholder(true),
+          'No patch set description');
+      assert.equal(element._computeDescriptionPlaceholder(false),
+          'Add a patch set description');
+    });
+
+    test('_handleDescriptionChanged', function() {
+      var putDescStub = sandbox.stub(element.$.restAPI, 'setDescription')
+          .returns(Promise.resolve({ok: true}));
+      sandbox.stub(element, '_computeDescriptionReadOnly');
+
+      element._changeNum = '42';
+      element._patchRange = {
+        basePatchNum: 'PARENT',
+        patchNum: 1,
+      };
+      element._selectedPatchNum = '1';
+      element._change = {
+        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
+        revisions: {
+          rev1: {_number: 1, description: 'test', commit: {commit: 'rev1'}},
+        },
+        current_revision: 'rev1',
+        status: 'NEW',
+        labels: {},
+        actions: {},
+        owner: {_account_id: 1},
+      };
+      element._account = {_account_id: 1};
+      element._loggedIn = true;
+
+      flushAsynchronousOperations();
+      var label = element.$.descriptionLabel;
+      assert.equal(label.value, 'test');
+      label.editing = true;
+      label._inputText = 'test2';
+      label._save();
+      flushAsynchronousOperations();
+      assert.isTrue(putDescStub.called);
+      assert.equal(putDescStub.args[0][2], 'test2');
+    });
+
     test('_reload is called when an approved label is removed', function() {
       var vote = {_account_id: 1, name: 'bojack', value: 1};
       element._changeNum = '42';
@@ -531,12 +583,12 @@ limitations under the License.
     });
 
     test('class is applied to file list on old patch set', function() {
-      var allPatcheSets = [1, 2, 4];
+      var allPatchSets = [{num: 1}, {num: 2}, {num: 4}];
-      assert.equal(element._computePatchInfoClass('1', allPatcheSets),
+      assert.equal(element._computePatchInfoClass('1', allPatchSets),
           'patchInfo--oldPatchSet');
-      assert.equal(element._computePatchInfoClass('2', allPatcheSets),
+      assert.equal(element._computePatchInfoClass('2', allPatchSets),
           'patchInfo--oldPatchSet');
-      assert.equal(element._computePatchInfoClass('4', allPatcheSets), '');
+      assert.equal(element._computePatchInfoClass('4', allPatchSets), '');
     });
 
     test('getUrlParameter functionality', function() {

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

@@ -143,7 +143,7 @@ limitations under the License.
         margin: .25em 0 1em;
       }
       .patchSetSelect {
-        max-width: 25em;
+        max-width: 8em;
       }
       @media screen and (max-width: 50em) {
         .row.selected {
@@ -192,12 +192,12 @@ limitations under the License.
             <option value="PARENT">Base</option>
             <template
                 is="dom-repeat"
-                items="[[_computePatchSets(revisions, patchRange.*)]]"
+                items="[[_computePatchSets(revisions.*, patchRange.*)]]"
                 as="patchNum">
-              <option value$="[[patchNum]]"
+              <option value$="[[patchNum.num]]"
-                  disabled$="[[_computePatchSetDisabled(patchNum, patchRange.patchNum)]]">
+                  disabled$="[[_computePatchSetDisabled(patchNum.num, patchRange.patchNum)]]">
-                [[patchNum]]
+                [[patchNum.num]]
-                [[_computePatchSetDescription(revisions, patchNum)]]
+                [[patchNum.desc]]
               </option>
             </template>
           </select>

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

@@ -199,12 +199,18 @@
       return this.$.restAPI.getPreferences();
     },
 
-    _computePatchSets: function(revisions) {
+    _computePatchSets: function(revisionRecord) {
+      var revisions = revisionRecord.base;
       var patchNums = [];
       for (var commit in revisions) {
-        patchNums.push(revisions[commit]._number);
+        if (revisions.hasOwnProperty(commit)) {
+          patchNums.push({
+            num: revisions[commit]._number,
+            desc: revisions[commit].description,
+          });
         }
-      return patchNums.sort(function(a, b) { return a - b; });
+      }
+      return patchNums.sort(function(a, b) { return a.num - b.num; });
     },
 
     _computePatchSetDisabled: function(patchNum, currentPatchNum) {
@@ -617,7 +623,7 @@
     },
 
     _computePatchSetDescription: function(revisions, patchNum) {
-      var rev = this.getRevisionNumber(revisions, patchNum);
+      var rev = this.getRevisionByPatchNum(revisions, patchNum);
       return (rev && rev.description) ? rev.description : '';
     },
   });

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

@@ -446,13 +446,24 @@ limitations under the License.
     });
 
     test('patch set from revisions', function() {
+      var expected = [
+        {num: 1, desc: 'test'},
+        {num: 2, desc: 'test'},
+        {num: 3, desc: 'test'},
+        {num: 4, desc: 'test'},
+      ];
       var patchNums = element._computePatchSets({
-        rev3: {_number: 3},
+        base: {
-        rev1: {_number: 1},
+          rev3: {_number: 3, description: 'test'},
-        rev4: {_number: 4},
+          rev1: {_number: 1, description: 'test'},
-        rev2: {_number: 2},
+          rev4: {_number: 4, description: 'test'},
+          rev2: {_number: 2, description: 'test'},
+        }
       });
-      assert.deepEqual(patchNums, [1, 2, 3, 4]);
+      assert.equal(patchNums.length, expected.length);
+      for (var i = 0; i < expected.length; i++) {
+        assert.deepEqual(patchNums[i], expected[i]);
+      }
     });
 
     test('patch range string', function() {

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

@@ -28,7 +28,7 @@ limitations under the License.
         display: inline-block;
       }
       select {
-        max-width: 25em;
+        max-width: 8em;
       }
     </style>
     Patch set:

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

@@ -72,7 +72,7 @@
     },
 
     _computePatchSetDescription: function(revisions, patchNum) {
-      var rev = this.getRevisionNumber(revisions, patchNum);
+      var rev = this.getRevisionByPatchNum(revisions, patchNum);
       return (rev && rev.description) ? rev.description : '';
     },
   });

polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html

@@ -19,10 +19,12 @@ limitations under the License.
   <template>
     <style>
       :host {
-        display: inline-block;
+        align-items: center;
+        display: inline-flex;
       }
       input,
-      label {
+      label,
+      .container {
         width: 100%;
       }
       input {

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

@@ -905,5 +905,11 @@
       return this.send('DELETE', '/changes/' + changeID +
           '/reviewers/' + account + '/votes/' + encodeURIComponent(label));
     },
+
+    setDescription: function(changeNum, patchNum, desc) {
+      return this.send('PUT',
+          this.getChangeActionURL(changeNum, patchNum, '/description'),
+          {description: desc});
+    },
   });
 })();