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)
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 4c73fcf..1db484c 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-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,13 +195,20 @@
override async updated() {
assertIsDefined(this.account, 'account');
+ if (isDetailedAccount(this.account)) return;
const account = await this.getAccountsModel().fillDetails(this.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 details to avoid
- // displaying incorrect author or committer email.
- if (account) this.account = Object.assign(account, this.account);
+ // 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 0802f06..6eedcbe8 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 @@
): 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 53c90a6..e84723c 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 @@
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 0853941..b93acc6 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -140,10 +140,10 @@
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 72fa791..b1ee50e 100644
--- a/polygerrit-ui/app/utils/account-util_test.ts
+++ b/polygerrit-ui/app/utils/account-util_test.ts
@@ -263,6 +263,7 @@
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 @@
email: 'abcd' as EmailAddress,
})
);
+ assert.isTrue(
+ isDetailedAccount({
+ _account_id: 12345 as AccountId,
+ name: 'Kermit',
+ })
+ );
});
test('fails gracefully when all is not included', async () => {
