Display owners details even when owners.expandGroups == false
When `owners.expandGroups = false` then neither groups nor accounts are
expaned to members (in case of groups) or account ids (in terms of
accounts) in the plugins REST API response. In addition, owner can be
configured as:
* email - in this case the `@domain` is dropped
* full user name - in this case full user name is conveyed
Considering the above when owner details are about to be displayed:
* check if it starts with `group/` prefix and display it as
Group: {name} [copy_icon]
* search change's accounts through either domain dropped email address
or full name and finally fallback to display
{name} [copy_icon]
Note that either account name or group name is copied when button is
clicked.
Bug: Issue 379269836
Change-Id: I7c87dd9b91ab45045536f86b80b0ec7c99ba3613
diff --git a/owners/web/gr-files.ts b/owners/web/gr-files.ts
index 567d84c..1e63863 100644
--- a/owners/web/gr-files.ts
+++ b/owners/web/gr-files.ts
@@ -22,6 +22,7 @@
ApprovalInfo,
ChangeInfo,
ChangeStatus,
+ GroupInfo,
LabelInfo,
isDetailedLabelInfo,
EmailAddress,
@@ -33,7 +34,13 @@
OwnersLabels,
OWNERS_SUBMIT_REQUIREMENT,
} from './owners-service';
-import {FileOwnership, FileStatus, PatchRange, UserRole} from './owners-model';
+import {
+ FileOwnership,
+ FileStatus,
+ OwnerOrGroupOwner,
+ PatchRange,
+ UserRole,
+} from './owners-model';
import {query} from './utils';
import {GrAccountLabel} from './gerrit-model';
import {OwnersMixin} from './owners-mixin';
@@ -114,6 +121,9 @@
owner?: AccountInfo;
@property({type: Object})
+ groupOwner?: GroupInfo;
+
+ @property({type: Object})
approval?: ApprovalInfo;
@property({type: Object})
@@ -141,31 +151,47 @@
}
override render() {
- if (!this.owner) {
+ if (!this.owner && !this.groupOwner) {
return nothing;
}
- const voteChip = this.approval
- ? html` <gr-vote-chip
- .vote=${this.approval}
- .label=${this.info}
- ></gr-vote-chip>`
- : nothing;
+ const isAccountOwner = this.owner !== undefined;
+ const ownerLabel = isAccountOwner
+ ? this.owner?._account_id
+ ? html`<gr-account-label .account=${this.owner}></gr-account-label>`
+ : html`<span>${this.owner?.display_name}</span>`
+ : html`<span>Group: ${this.groupOwner?.name}</span>`;
- const copyEmail = this.email
+ const voteChip =
+ isAccountOwner && this.approval
+ ? html` <gr-vote-chip
+ .vote=${this.approval}
+ .label=${this.info}
+ ></gr-vote-chip>`
+ : nothing;
+
+ // allow user to copy what is available:
+ // * email or name (if available) for AccountInfo owner type
+ // * group name for GroupInfo owner type
+ const [copyText, copyTooltip] = isAccountOwner
+ ? this.email || this.owner?.display_name
+ ? [
+ this.email ?? this.owner?.display_name,
+ this.email ? 'email' : 'name',
+ ]
+ : [nothing, nothing]
+ : [this.groupOwner?.name, 'group name'];
+ const copy = copyText
? html` <gr-copy-clipboard
- .text=${this.email}
+ .text=${copyText}
hasTooltip
hideinput
- buttonTitle=${"Copy owner's email to clipboard"}
+ buttonTitle="Copy owner's ${copyTooltip} to clipboard"
></gr-copy-clipboard>`
: nothing;
return html`
- <div class="container">
- <gr-account-label .account=${this.owner}></gr-account-label>
- ${voteChip} ${copyEmail}
- </div>
+ <div class="container">${ownerLabel} ${voteChip} ${copy}</div>
`;
}
@@ -200,7 +226,7 @@
@property({type: String, reflect: true, attribute: 'file-status'})
fileStatus?: string;
- private owners?: AccountInfo[];
+ private owners?: OwnerOrGroupOwner[];
// taken from Gerrit's common-util.ts
private uniqueId = Math.random().toString(36).substring(2);
@@ -328,10 +354,11 @@
: ''}"
>
<gr-owner
- .owner=${owner}
+ .owner=${owner.owner}
+ .groupOwner=${owner.groupOwner}
.approval=${approval}
.info=${info}
- .email=${owner.email}
+ .email=${owner.owner ? owner.owner.email : nothing}
></gr-owner>
</div>
`;
@@ -369,13 +396,34 @@
this.fileStatus = FILE_STATUS[fileOwnership.fileStatus];
const accounts = getChangeAccounts(this.change);
- // TODO for the time being filter out or group owners - to be decided what/how to display them
- this.owners = (fileOwnership.owners ?? [])
- .filter(isOwner)
- .map(
- o =>
- accounts.get(o.id) ?? ({_account_id: o.id} as unknown as AccountInfo)
- );
+ this.owners = (fileOwnership.owners ?? []).map(owner => {
+ if (isOwner(owner)) {
+ return {
+ owner:
+ accounts.byAccountId.get(owner.id) ??
+ ({_account_id: owner.id} as unknown as AccountInfo),
+ } as unknown as OwnerOrGroupOwner;
+ }
+
+ const groupPrefix = 'group/';
+ if (owner.name.startsWith(groupPrefix)) {
+ return {
+ groupOwner: {
+ name: owner.name.substring(groupPrefix.length),
+ } as unknown as GroupInfo,
+ } as unknown as OwnerOrGroupOwner;
+ }
+
+ // when `owners.expandGroups = false` then neither group nor account
+ // will be expanded therefore try to match account with change's available
+ // accounts through email without domain or by full name
+ // finally construct `AccountInfo` just with a `name` property
+ const accountOwner =
+ accounts.byEmailWithoutDomain.get(owner.name) ??
+ accounts.byFullName.get(owner.name) ??
+ ({display_name: owner.name} as unknown as AccountInfo);
+ return {owner: accountOwner} as unknown as OwnerOrGroupOwner;
+ });
}
}
@@ -447,14 +495,15 @@
}
export function computeApprovalAndInfo(
- fileOwner: AccountInfo,
+ fileOwner: OwnerOrGroupOwner,
labels: OwnersLabels,
change?: ChangeInfo
): [ApprovalInfo, LabelInfo] | undefined {
- if (!change?.labels) {
+ if (!change?.labels || !fileOwner?.owner) {
return;
}
- const ownersLabel = labels[`${fileOwner._account_id}`];
+ const accountId = fileOwner.owner._account_id;
+ const ownersLabel = labels[`${accountId}`];
if (!ownersLabel) {
return;
}
@@ -471,19 +520,26 @@
return;
}
- const approval = info.all?.filter(
- x => x._account_id === fileOwner._account_id
- )[0];
+ const approval = info.all?.filter(x => x._account_id === accountId)[0];
return approval ? [approval, info] : undefined;
}
return;
}
-export function getChangeAccounts(
- change?: ChangeInfo
-): Map<number, AccountInfo> {
- const accounts = new Map();
+export interface ChangeAccounts {
+ byAccountId: Map<number, AccountInfo>;
+ byEmailWithoutDomain: Map<string, AccountInfo>;
+ byFullName: Map<string, AccountInfo>;
+}
+
+export function getChangeAccounts(change?: ChangeInfo): ChangeAccounts {
+ const accounts = {
+ byAccountId: new Map(),
+ byEmailWithoutDomain: new Map(),
+ byFullName: new Map(),
+ } as unknown as ChangeAccounts;
+
if (!change) {
return accounts;
}
@@ -493,6 +549,24 @@
...(change.submitter ? [change.submitter] : []),
...(change.reviewers[ReviewerState.REVIEWER] ?? []),
...(change.reviewers[ReviewerState.CC] ?? []),
- ].forEach(account => accounts.set(account._account_id, account));
+ ].forEach(account => {
+ if (account._account_id) {
+ accounts.byAccountId.set(account._account_id, account);
+ }
+
+ if (account.email && account.email.indexOf('@') > 0) {
+ accounts.byEmailWithoutDomain.set(
+ account.email.substring(
+ 0,
+ account.email.indexOf('@')
+ ) as unknown as string,
+ account
+ );
+ }
+
+ if (account.name) {
+ accounts.byFullName.set(account.name, account);
+ }
+ });
return accounts;
}
diff --git a/owners/web/gr-files_test.ts b/owners/web/gr-files_test.ts
index e710b86..1797b20 100644
--- a/owners/web/gr-files_test.ts
+++ b/owners/web/gr-files_test.ts
@@ -23,7 +23,13 @@
getFileOwnership,
shouldHide,
} from './gr-files';
-import {FileOwnership, FileStatus, PatchRange, UserRole} from './owners-model';
+import {
+ FileOwnership,
+ FileStatus,
+ OwnerOrGroupOwner,
+ PatchRange,
+ UserRole,
+} from './owners-model';
import {
AccountInfo,
ApprovalInfo,
@@ -273,7 +279,9 @@
suite('computeApprovalAndInfo tests', () => {
const account = 1;
- const fileOwner = {_account_id: account} as unknown as AccountInfo;
+ const fileOwner = {
+ owner: {_account_id: account} as unknown as AccountInfo,
+ } as unknown as OwnerOrGroupOwner;
const label = 'Code-Review';
const crPlus1OwnersVote = {
[`${account}`]: {[label]: 1},
@@ -369,7 +377,10 @@
suite('getChangeAccounts tests', () => {
test('getChangeAccounts - should return empty map when change is `undefined', () => {
const undefinedChange = undefined;
- assert.equal(getChangeAccounts(undefinedChange).size, 0);
+ const accounts = getChangeAccounts(undefinedChange);
+ assert.equal(accounts.byAccountId.size, 0);
+ assert.equal(accounts.byEmailWithoutDomain.size, 0);
+ assert.equal(accounts.byFullName.size, 0);
});
test('getChangeAccounts - should return map with owner when change has only owner and empty reviewers defined', () => {
@@ -378,8 +389,17 @@
owner,
reviewers: {},
} as unknown as ChangeInfo;
+ const accounts = getChangeAccounts(changeWithOwner);
assert.equal(
- deepEqual(getChangeAccounts(changeWithOwner), new Map([[1, owner]])),
+ deepEqual(accounts.byAccountId, new Map([[1, owner]])),
+ true
+ );
+ assert.equal(
+ deepEqual(accounts.byEmailWithoutDomain, new Map([['1_email', owner]])),
+ true
+ );
+ assert.equal(
+ deepEqual(accounts.byFullName, new Map([['1_name', owner]])),
true
);
});
@@ -397,9 +417,10 @@
[ReviewerState.CC]: [ccReviewer],
},
} as unknown as ChangeInfo;
+ const accounts = getChangeAccounts(change);
assert.equal(
deepEqual(
- getChangeAccounts(change),
+ accounts.byAccountId,
new Map([
[1, owner],
[2, submitter],
@@ -409,6 +430,30 @@
),
true
);
+ assert.equal(
+ deepEqual(
+ accounts.byEmailWithoutDomain,
+ new Map([
+ ['1_email', owner],
+ ['2_email', submitter],
+ ['3_email', reviewer],
+ ['4_email', ccReviewer],
+ ])
+ ),
+ true
+ );
+ assert.equal(
+ deepEqual(
+ accounts.byFullName,
+ new Map([
+ ['1_name', owner],
+ ['2_name', submitter],
+ ['3_name', reviewer],
+ ['4_name', ccReviewer],
+ ])
+ ),
+ true
+ );
});
});
});
@@ -416,5 +461,7 @@
function account(id: number) {
return {
_account_id: id,
+ email: `${id}_email@example.com`,
+ name: `${id}_name`,
} as unknown as AccountInfo;
}
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts
index 83af4e3..4d32095 100644
--- a/owners/web/owners-model.ts
+++ b/owners/web/owners-model.ts
@@ -20,6 +20,7 @@
AccountInfo,
BasePatchSetNum,
ChangeInfo,
+ GroupInfo,
RevisionPatchSetNum,
} from '@gerritcodereview/typescript-api/rest-api';
import {FileOwner, FilesOwners, OwnersService} from './owners-service';
@@ -58,6 +59,11 @@
owners?: FileOwner[];
}
+export interface OwnerOrGroupOwner {
+ owner?: AccountInfo;
+ groupOwner?: GroupInfo;
+}
+
let ownersModel: OwnersModel | undefined;
export class OwnersModel extends EventTarget {
