From f081d7236f5ae4c04f9fb078b77353b331f94f46 Mon Sep 17 00:00:00 2001
From: Kasper Nilsson <kaspern@google.com>
Date: Fri, 18 Nov 2016 10:54:24 -0800
Subject: [PATCH] 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
---
 .../gr-patch-set-behavior.html                |  8 +-
 .../gr-patch-set-behavior_test.html           |  4 +-
 .../gr-change-actions/gr-change-actions.js    |  2 +-
 .../change/gr-change-view/gr-change-view.html | 24 ++++--
 .../change/gr-change-view/gr-change-view.js   | 73 ++++++++++++++++---
 .../gr-change-view/gr-change-view_test.html   | 60 ++++++++++++++-
 .../change/gr-file-list/gr-file-list.html     | 12 +--
 .../change/gr-file-list/gr-file-list.js       | 14 +++-
 .../gr-file-list/gr-file-list_test.html       | 21 ++++--
 .../gr-patch-range-select.html                |  2 +-
 .../gr-patch-range-select.js                  |  2 +-
 .../gr-editable-label/gr-editable-label.html  | 26 ++++---
 .../gr-rest-api-interface.js                  |  6 ++
 13 files changed, 199 insertions(+), 55 deletions(-)

diff --git a/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html b/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html
index c52c12eb31..acf3a62955 100644
--- a/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior.html
+++ b/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 {number|string} patchNum
-     * @return {Object}
+     * @param {Object} revisions The object of revisions given by the API
+     * @param {number|string} patchNum The number index of the revision
+     * @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) &&
diff --git a/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior_test.html b/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior_test.html
index d9e98f4c58..7ff93719df 100644
--- a/polygerrit-ui/app/behaviors/gr-patch-set-behavior/gr-patch-set-behavior_test.html
+++ b/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() {
-      var get = Gerrit.PatchSetBehavior.getRevisionNumber;
+    test('getRevisionByPatchNum', function() {
+      var get = Gerrit.PatchSetBehavior.getRevisionByPatchNum;
       var revisions = [
         {_number: 0},
         {_number: 1},
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
index ccfb124a88..12c3afd53b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js
@@ -454,7 +454,7 @@
           if (!this._canSubmitChange()) {
             return;
           }
-          /* falls through */ // required by JSHint
+        /* falls through */ // required by JSHint
         default:
           this._fireAction(this._prependSlash(key),
               this.revisionActions[key], true);
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index a1b7852708..85c3fb8962 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/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">
-                <option value$="[[patchNumber]]">
-                  [[patchNumber]]
+                  as="patchNum">
+                <option value$="[[patchNum.num]]" label>
+                  [[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
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 688cd0a117..cd5a35aa4e 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
@@ -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) {
-        patchNums.push(change.revisions[rev]._number);
+      for (var commit in change.revisions) {
+        if (change.revisions.hasOwnProperty(commit)) {
+          patchNums.push({
+            num: change.revisions[commit]._number,
+            desc: change.revisions[commit].description,
+          });
+        }
       }
-      return patchNums.sort(function(a, b) {
-        return a - b;
-      });
+      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));
+    },
   });
 })();
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 6b5849a555..15fd4f9a14 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
@@ -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];
-      assert.equal(element._computePatchInfoClass('1', allPatcheSets),
+      var allPatchSets = [{num: 1}, {num: 2}, {num: 4}];
+      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() {
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 31427339ae..6425acb7a6 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/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]]"
-                  disabled$="[[_computePatchSetDisabled(patchNum, patchRange.patchNum)]]">
-                [[patchNum]]
-                [[_computePatchSetDescription(revisions, patchNum)]]
+              <option value$="[[patchNum.num]]"
+                  disabled$="[[_computePatchSetDisabled(patchNum.num, patchRange.patchNum)]]">
+                [[patchNum.num]]
+                [[patchNum.desc]]
               </option>
             </template>
           </select>
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 b423f50c48..22a07b0222 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
@@ -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 : '';
     },
   });
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 bc7c0c7b04..5da29957b8 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
@@ -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},
-        rev1: {_number: 1},
-        rev4: {_number: 4},
-        rev2: {_number: 2},
+        base: {
+          rev3: {_number: 3, description: 'test'},
+          rev1: {_number: 1, description: 'test'},
+          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() {
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
index fe9efd6157..759b3a2a6d 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.html
+++ b/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:
diff --git a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js b/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
index 58a3b00899..ca38a8c090 100644
--- a/polygerrit-ui/app/elements/diff/gr-patch-range-select/gr-patch-range-select.js
+++ b/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 : '';
     },
   });
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html b/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html
index 4fec27f12a..9b6c572124 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-label/gr-editable-label.html
+++ b/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 {
@@ -43,16 +45,16 @@ limitations under the License.
         text-decoration: underline;
       }
     </style>
-    <input
-        is="iron-input"
-        id="input"
-        hidden$="[[!editing]]"
-        on-keydown="_handleInputKeydown"
-        bind-value="{{_inputText}}">
-    <label
-        hidden$="[[editing]]"
-        class$="[[_computeLabelClass(readOnly, value, placeholder)]]"
-        on-tap="_open">[[_computeLabel(value, placeholder)]]</label>
+      <input
+          is="iron-input"
+          id="input"
+          hidden$="[[!editing]]"
+          on-keydown="_handleInputKeydown"
+          bind-value="{{_inputText}}">
+      <label
+          hidden$="[[editing]]"
+          class$="[[_computeLabelClass(readOnly, value, placeholder)]]"
+          on-tap="_open">[[_computeLabel(value, placeholder)]]</label>
   </template>
   <script src="gr-editable-label.js"></script>
 </dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 8a66533580..48682e93f2 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/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});
+    },
   });
 })();