Fix issues with account details filling

Users have reported an issue with bogus account labels when the
account label was trying to fill in the details. One issue was that
the account (even the detailed one) does not have an `email` property
set. So the underlying `isDetailedAccount()` utility was producing
false return values and initiated the account to be looked up again
and again.

Another issue was that `Object.assign()` would actually modify the
cached object in place and thus potentially mess up the account object
for all subsequent callers.

Release-Notes: Fix bogus account labels.
Change-Id: Ib91fe57ad8026ef2a1daf61ddc2af5954ffd39ab
(cherry picked from commit ac98b4d387004032658eb0a3e9e4152d82e32a8c)

5 files changed, 60 insertions, 8 deletions

diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
index cf7ff22090..1db484cab2 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
@@ -10,7 +10,11 @@ import '../../plugins/gr-endpoint-decorator/gr-endpoint-decorator';
 import '../../plugins/gr-endpoint-param/gr-endpoint-param';
 import {getAppContext} from '../../../services/app-context';
 import {getDisplayName} from '../../../utils/display-name-util';
-import {isSelf, isServiceUser} from '../../../utils/account-util';
+import {
+  isDetailedAccount,
+  isSelf,
+  isServiceUser,
+} from '../../../utils/account-util';
 import {ChangeInfo, AccountInfo, ServerInfo} from '../../../types/common';
 import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
 import {fire} from '../../../utils/event-util';
@@ -191,8 +195,20 @@ export class GrAccountLabel extends LitElement {
 
   override async updated() {
     assertIsDefined(this.account, 'account');
+    if (isDetailedAccount(this.account)) return;
     const account = await this.getAccountsModel().fillDetails(this.account);
-    if (account) this.account = account;
+    if (!isDetailedAccount(account)) return;
+    // AccountInfo returned by fillDetails has the email property set
+    // to the primary email of the account. This poses a problem in
+    // cases where a secondary email is used as the committer or author
+    // email. Therefore, only fill in the *missing* properties.
+    if (
+      account &&
+      account !== this.account &&
+      account._account_id === this.account._account_id
+    ) {
+      this.account = {...account, ...this.account};
+    }
   }
 
   override render() {
diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model.ts b/polygerrit-ui/app/models/accounts-model/accounts-model.ts
index 0802f0645b..6eedcbe808 100644
--- a/polygerrit-ui/app/models/accounts-model/accounts-model.ts
+++ b/polygerrit-ui/app/models/accounts-model/accounts-model.ts
@@ -42,7 +42,7 @@ export class AccountsModel extends Model<AccountsState> {
   ): Promise<AccountDetailInfo | AccountInfo> {
     const current = this.getState();
     const id = getUserId(partialAccount);
-    if (hasOwnProperty(current.accounts, id)) return current.accounts[id];
+    if (hasOwnProperty(current.accounts, id)) return {...current.accounts[id]};
     // It is possible to add emails to CC when they don't have a Gerrit
     // account. In this case getAccountDetails will return a 404 error then
     // we at least use what is in partialAccount.
diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts b/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts
index 53c90a67ea..e84723c6b2 100644
--- a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts
+++ b/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts
@@ -5,12 +5,23 @@
  */
 
 import '../../test/common-test-setup';
-import {EmailAddress} from '../../api/rest-api';
+import {
+  AccountDetailInfo,
+  AccountId,
+  EmailAddress,
+  Timestamp,
+} from '../../api/rest-api';
 import {getAppContext} from '../../services/app-context';
 import {stubRestApi} from '../../test/test-utils';
 import {AccountsModel} from './accounts-model';
 import {assert} from '@open-wc/testing';
 
+const KERMIT: AccountDetailInfo = {
+  _account_id: 1 as AccountId,
+  name: 'Kermit',
+  registered_on: '2015-03-12 18:32:08.000000000' as Timestamp,
+};
+
 suite('accounts-model tests', () => {
   let model: AccountsModel;
 
@@ -22,6 +33,24 @@ suite('accounts-model tests', () => {
     model.finalize();
   });
 
+  test('basic lookup', async () => {
+    const stub = stubRestApi('getAccountDetails').returns(
+      Promise.resolve(KERMIT)
+    );
+
+    let filled = await model.fillDetails({_account_id: 1 as AccountId});
+    assert.equal(filled.name, 'Kermit');
+    assert.equal(filled, KERMIT);
+    assert.equal(stub.callCount, 1);
+
+    filled = await model.fillDetails({_account_id: 1 as AccountId});
+    assert.equal(filled.name, 'Kermit');
+    // Cache objects are cloned on lookup, so this is a different object.
+    assert.notEqual(filled, KERMIT);
+    // Did not have to call the REST API again.
+    assert.equal(stub.callCount, 1);
+  });
+
   test('invalid account makes only one request', () => {
     const response = {...new Response(), status: 404};
     const getAccountDetails = stubRestApi('getAccountDetails').callsFake(
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index 08539411f0..b93acc61cc 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -140,10 +140,10 @@ export function uniqueAccountId(
 
 export function isDetailedAccount(account?: AccountInfo) {
   // In case ChangeInfo is requested without DetailedAccount option, the
-  // reviewer entry is returned as just {_account_id: 123}
-  // This object should also be treated as not detailed account if they have
-  // an AccountId and no email
-  return !!account?.email && !!account?._account_id;
+  // reviewer entry is returned as just {_account_id: 123}.
+  // At least a name or an email must be set for the account to be treated as
+  // "detailed".
+  return (!!account?.email || !!account?.name) && !!account?._account_id;
 }
 
 /**
diff --git a/polygerrit-ui/app/utils/account-util_test.ts b/polygerrit-ui/app/utils/account-util_test.ts
index 72fa7911a3..b1ee50ee4c 100644
--- a/polygerrit-ui/app/utils/account-util_test.ts
+++ b/polygerrit-ui/app/utils/account-util_test.ts
@@ -263,6 +263,7 @@ suite('account-util tests', () => {
   test('isDetailedAccount', () => {
     assert.isFalse(isDetailedAccount({_account_id: 12345 as AccountId}));
     assert.isFalse(isDetailedAccount({email: 'abcd' as EmailAddress}));
+    assert.isFalse(isDetailedAccount({name: 'Kermit'}));
 
     assert.isTrue(
       isDetailedAccount({
@@ -270,6 +271,12 @@ suite('account-util tests', () => {
         email: 'abcd' as EmailAddress,
       })
     );
+    assert.isTrue(
+      isDetailedAccount({
+        _account_id: 12345 as AccountId,
+        name: 'Kermit',
+      })
+    );
   });
 
   test('fails gracefully when all is not included', async () => {