Merge "Revert "Revert "Show author and committer when relevant""" into stable-2.16

4 files changed, 178 insertions, 112 deletions

diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
index 3c5adde96e..bec08a8188 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.html
@@ -163,11 +163,28 @@ limitations under the License.
           </template>
         </span>
       </section>
-      <section class$="[[_computeShowUploaderHide(change)]]">
+      <section class$="[[_computeShowRoleClass(change, _CHANGE_ROLE.UPLOADER)]]">
         <span class="title">Uploader</span>
         <span class="value">
           <gr-account-link
-              account="[[_computeShowUploader(change)]]"></gr-account-link>
+              account="[[_getNonOwnerRole(change, _CHANGE_ROLE.UPLOADER)]]"
+              ></gr-account-link>
+        </span>
+      </section>
+      <section class$="[[_computeShowRoleClass(change, _CHANGE_ROLE.AUTHOR)]]">
+        <span class="title">Author</span>
+        <span class="value">
+          <gr-account-link
+              account="[[_getNonOwnerRole(change, _CHANGE_ROLE.AUTHOR)]]"
+              ></gr-account-link>
+        </span>
+      </section>
+      <section class$="[[_computeShowRoleClass(change, _CHANGE_ROLE.COMMITTER)]]">
+        <span class="title">Committer</span>
+        <span class="value">
+          <gr-account-link
+              account="[[_getNonOwnerRole(change, _CHANGE_ROLE.COMMITTER)]]"
+              ></gr-account-link>
         </span>
       </section>
       <section class="assignee">
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
index 82af321f38..d3fc7e0df7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.js
@@ -133,6 +133,18 @@
         type: Array,
         computed: '_computeParents(change)',
       },
+
+      /** @type {?} */
+      _CHANGE_ROLE: {
+        type: Object,
+        readOnly: true,
+        value: {
+          OWNER: 'owner',
+          UPLOADER: 'uploader',
+          AUTHOR: 'author',
+          COMMITTER: 'committer',
+        },
+      },
     },
 
     behaviors: [
@@ -388,24 +400,45 @@
       return !!change.work_in_progress;
     },
 
-    _computeShowUploaderHide(change) {
-      return this._computeShowUploader(change) ? '' : 'hideDisplay';
+    _computeShowRoleClass(change, role) {
+      return this._getNonOwnerRole(change, role) ? '' : 'hideDisplay';
     },
 
-    _computeShowUploader(change) {
+    /**
+     * Get the user with the specified role on the change. Returns null if the
+     * user with that role is the same as the owner.
+     * @param {!Object} change
+     * @param {string} role One of the values from _CHANGE_ROLE
+     * @return {Object|null} either an accound or null.
+     */
+    _getNonOwnerRole(change, role) {
       if (!change.current_revision ||
           !change.revisions[change.current_revision]) {
         return null;
       }
 
       const rev = change.revisions[change.current_revision];
+      if (!rev) { return null; }
 
-      if (!rev || !rev.uploader ||
-        change.owner._account_id === rev.uploader._account_id) {
-        return null;
+      if (role === this._CHANGE_ROLE.UPLOADER &&
+          rev.uploader &&
+          change.owner._account_id !== rev.uploader._account_id) {
+        return rev.uploader;
+      }
+
+      if (role === this._CHANGE_ROLE.AUTHOR &&
+          rev.commit && rev.commit.author &&
+          change.owner.email !== rev.commit.author.email) {
+        return rev.commit.author;
+      }
+
+      if (role === this._CHANGE_ROLE.COMMITTER &&
+          rev.commit && rev.commit.committer &&
+          change.owner.email !== rev.commit.committer.email) {
+        return rev.commit.committer;
       }
 
-      return rev.uploader;
+      return null;
     },
 
     _computeParents(change) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
index 2884a68880..c5a569e653 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.html
@@ -185,115 +185,130 @@ limitations under the License.
       assert.equal(element._computeWebLinks(element.commitInfo).length, 1);
     });
 
-    test('_computeShowUploader test for uploader', () => {
-      const change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        owner: {
-          _account_id: 1019328,
-        },
-        revisions: {
-          rev1: {
-            _number: 1,
-            uploader: {
-              _account_id: 1011123,
-            },
-          },
-        },
-        current_revision: 'rev1',
-        status: 'NEW',
-        labels: {},
-        mergeable: true,
-      };
-      assert.deepEqual(element._computeShowUploader(change),
-          {_account_id: 1011123});
-    });
+    suite('_getNonOwnerRole', () => {
+      let change;
 
-    test('_computeShowUploader test that it does not return uploader', () => {
-      const change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        owner: {
-          _account_id: 1011123,
-        },
-        revisions: {
-          rev1: {
-            _number: 1,
-            uploader: {
-              _account_id: 1011123,
-            },
+      setup(() => {
+        change = {
+          owner: {
+            email: 'abc@def',
+            _account_id: 1019328,
           },
-        },
-        current_revision: 'rev1',
-        status: 'NEW',
-        labels: {},
-        mergeable: true,
-      };
-      assert.isNotOk(element._computeShowUploader(change));
-    });
-
-    test('no current_revision makes _computeShowUploader return null', () => {
-      const change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        owner: {
-          _account_id: 1011123,
-        },
-        revisions: {
-          rev1: {
-            _number: 1,
-            uploader: {
-              _account_id: 1011123,
+          revisions: {
+            rev1: {
+              _number: 1,
+              uploader: {
+                email: 'ghi@def',
+                _account_id: 1011123,
+              },
+              commit: {
+                author: {email: 'jkl@def'},
+                committer: {email: 'ghi@def'},
+              },
             },
           },
-        },
-        status: 'NEW',
-        labels: {},
-        mergeable: true,
-      };
-      assert.isNotOk(element._computeShowUploader(change));
-    });
+          current_revision: 'rev1',
+        };
+      });
 
-    test('_computeShowUploaderHide test for string which equals true', () => {
-      const change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        owner: {
-          _account_id: 1019328,
-        },
-        revisions: {
-          rev1: {
-            _number: 1,
-            uploader: {
-              _account_id: 1011123,
-            },
-          },
-        },
-        current_revision: 'rev1',
-        status: 'NEW',
-        labels: {},
-        mergeable: true,
-      };
-      assert.equal(element._computeShowUploaderHide(change), '');
-    });
+      suite('role=uploader', () => {
+        test('_getNonOwnerRole for uploader', () => {
+          assert.deepEqual(
+              element._getNonOwnerRole(change, element._CHANGE_ROLE.UPLOADER),
+              {email: 'ghi@def', _account_id: 1011123});
+        });
 
-    test('_computeShowUploaderHide test for hideDisplay', () => {
-      const change = {
-        change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
-        owner: {
-          _account_id: 1011123,
-        },
-        revisions: {
-          rev1: {
-            _number: 1,
-            uploader: {
-              _account_id: 1011123,
-            },
-          },
-        },
-        current_revision: 'rev1',
-        status: 'NEW',
-        labels: {},
-        mergeable: true,
-      };
-      assert.equal(
-          element._computeShowUploaderHide(change), 'hideDisplay');
+        test('_getNonOwnerRole that it does not return uploader', () => {
+          // Set the uploader email to be the same as the owner.
+          change.revisions.rev1.uploader._account_id = 1019328;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.UPLOADER));
+        });
+
+        test('_getNonOwnerRole null for uploader with no current rev', () => {
+          delete change.current_revision;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.UPLOADER));
+        });
+
+        test('_computeShowRoleClass show uploader', () => {
+          assert.equal(element._computeShowRoleClass(
+              change, element._CHANGE_ROLE.UPLOADER), '');
+        });
+
+        test('_computeShowRoleClass hide uploader', () => {
+          // Set the uploader email to be the same as the owner.
+          change.revisions.rev1.uploader._account_id = 1019328;
+          assert.equal(element._computeShowRoleClass(change,
+              element._CHANGE_ROLE.UPLOADER), 'hideDisplay');
+        });
+      });
+
+      suite('role=committer', () => {
+        test('_getNonOwnerRole for committer', () => {
+          assert.deepEqual(
+              element._getNonOwnerRole(change, element._CHANGE_ROLE.COMMITTER),
+              {email: 'ghi@def'});
+        });
+
+        test('_getNonOwnerRole that it does not return committer', () => {
+          // Set the committer email to be the same as the owner.
+          change.revisions.rev1.commit.committer.email = 'abc@def';
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.COMMITTER));
+        });
+
+        test('_getNonOwnerRole null for committer with no current rev', () => {
+          delete change.current_revision;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.COMMITTER));
+        });
+
+        test('_getNonOwnerRole null for committer with no commit', () => {
+          delete change.revisions.rev1.commit;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.COMMITTER));
+        });
+
+        test('_getNonOwnerRole null for committer with no committer', () => {
+          delete change.revisions.rev1.commit.committer;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.COMMITTER));
+        });
+      });
+
+      suite('role=author', () => {
+        test('_getNonOwnerRole for author', () => {
+          assert.deepEqual(
+              element._getNonOwnerRole(change, element._CHANGE_ROLE.AUTHOR),
+              {email: 'jkl@def'});
+        });
+
+        test('_getNonOwnerRole that it does not return author', () => {
+          // Set the author email to be the same as the owner.
+          change.revisions.rev1.commit.author.email = 'abc@def';
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.AUTHOR));
+        });
+
+        test('_getNonOwnerRole null for author with no current rev', () => {
+          delete change.current_revision;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.AUTHOR));
+        });
+
+        test('_getNonOwnerRole null for author with no commit', () => {
+          delete change.revisions.rev1.commit;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.AUTHOR));
+        });
+
+        test('_getNonOwnerRole null for author with no author', () => {
+          delete change.revisions.rev1.commit.author;
+          assert.isNull(element._getNonOwnerRole(change,
+              element._CHANGE_ROLE.AUTHOR));
+        });
+      });
     });
 
     test('Push Certificate Validation test BAD', () => {
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 4296bd4ab2..f6acef68a1 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
@@ -575,6 +575,7 @@ limitations under the License.
       };
       element._change = {
         change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
+        owner: {email: 'abc@def'},
         revisions: {
           rev2: {_number: 2, commit: {parents: []}},
           rev1: {_number: 1, commit: {parents: []}},