Introduce `Owned Files` tab to the change screen This is the initial change that introduced the `Owned Files` tab to the change screen. At the moment tab is shown only when user owns any file (both `ownedFiles` and `shouldHide` functions are covered in units) and change is not in the `EDIT` mode. All files are displayed in the tab content (TODO provide links to diffs and style it appropriately). Changes: * `change-view-tab=[header|content]` were used to register new plugin components - note that Gerrit doesn't highlight tab without content but one can still hover over its place and select it :/ TODO: investigate if it can be improved with CSS and made no selectable * `getLoggedInUserRole` service's function was modified to `getLoggedInUser` so that user account and role are returned (existing functionality and tests were adjusted) * `getRandom` function was moved to `test-utils.ts` so that it gets reused (note that function is available only in tests) Bug: Issue 376837413 Change-Id: Ica25f84596d0d7d2dfe268e97a4e3477ce605a5f
diff --git a/owners/web/BUILD b/owners/web/BUILD index 620f1b2..56ab697 100644 --- a/owners/web/BUILD +++ b/owners/web/BUILD
@@ -21,7 +21,10 @@ name = "owners-ts", srcs = glob( ["**/*.ts"], - exclude = ["**/*test*"], + exclude = [ + "**/*test*", + "**/test-utils.ts", + ], ), incremental = True, out_dir = "_bazel_ts_out", @@ -36,7 +39,10 @@ ts_project( name = "owners-ts-tests", - srcs = glob(["**/*.ts"]), + srcs = glob([ + "**/*.ts", + "**/test-utils.ts", + ]), incremental = True, out_dir = "_bazel_ts_out_tests", tsc = "//tools/node_tools:tsc-bin",
diff --git a/owners/web/gr-files.ts b/owners/web/gr-files.ts index 93e3113..1f0c7ef 100644 --- a/owners/web/gr-files.ts +++ b/owners/web/gr-files.ts
@@ -63,7 +63,7 @@ this.change, this.patchRange, this.allFilesApproved, - this.userRole + this.user?.role ); }
diff --git a/owners/web/gr-files_test.ts b/owners/web/gr-files_test.ts index 5368fd6..0da0a35 100644 --- a/owners/web/gr-files_test.ts +++ b/owners/web/gr-files_test.ts
@@ -35,6 +35,7 @@ } from '@gerritcodereview/typescript-api/rest-api'; import {FilesOwners, OwnersLabels} from './owners-service'; import {deepEqual} from './utils'; +import {getRandom} from './test-utils'; suite('owners status tests', () => { const allFilesApproved = true; @@ -402,11 +403,6 @@ }); }); -function getRandom<T>(...values: T[]): T { - const idx = Math.floor(Math.random() * values.length); - return values[idx]; -} - function account(id: number) { return { _account_id: id,
diff --git a/owners/web/gr-owned-files.ts b/owners/web/gr-owned-files.ts new file mode 100644 index 0000000..b329d42 --- /dev/null +++ b/owners/web/gr-owned-files.ts
@@ -0,0 +1,142 @@ +/** + * @license + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {html, LitElement, PropertyValues, nothing, CSSResult} from 'lit'; +import {OwnersMixin} from './owners-mixin'; +import {customElement, property} from 'lit/decorators'; +import { + AccountInfo, + ChangeInfo, + ChangeStatus, + RevisionInfo, + EDIT, +} from '@gerritcodereview/typescript-api/rest-api'; +import {User, UserRole} from './owners-model'; +import {isOwner, OwnedFiles, OWNERS_SUBMIT_REQUIREMENT} from './owners-service'; + +const common = OwnersMixin(LitElement); + +class OwnedFilesCommon extends common { + @property({type: Object}) + revision?: RevisionInfo; + + protected ownedFiles?: string[]; + + protected override willUpdate(changedProperties: PropertyValues): void { + super.willUpdate(changedProperties); + this.computeOwnedFiles(); + + this.hidden = shouldHide( + this.change, + this.revision, + this.allFilesApproved, + this.user, + this.ownedFiles + ); + } + + protected static commonStyles(): CSSResult[] { + return [window?.Gerrit?.styles.font as CSSResult]; + } + + private computeOwnedFiles() { + this.ownedFiles = ownedFiles(this.user?.account, this.filesOwners?.files); + } +} + +export const OWNED_FILES_TAB_HEADER = 'owned-files-tab-header'; +@customElement(OWNED_FILES_TAB_HEADER) +export class OwnedFilesTabHeader extends OwnedFilesCommon { + static override get styles() { + return [...OwnedFilesCommon.commonStyles()]; + } + + override render() { + if (this.hidden) return nothing; + return html`<div>Owned Files</div>`; + } +} + +export const OWNED_FILES_TAB_CONTENT = 'owned-files-tab-content'; +@customElement(OWNED_FILES_TAB_CONTENT) +export class OwnedFilesTabContent extends OwnedFilesCommon { + static override get styles() { + return [...OwnedFilesCommon.commonStyles()]; + } + + override render() { + if (this.hidden || !this.ownedFiles) return nothing; + return html`<div> + ${this.ownedFiles.map(ownedFile => html`<div>${ownedFile}</div>`)} + </div>`; + } +} + +export function shouldHide( + change?: ChangeInfo, + revision?: RevisionInfo, + allFilesApproved?: boolean, + user?: User, + ownedFiles?: string[] +) { + // don't show owned files when no change or change is abandoned/merged or being edited + if ( + change === undefined || + change.status === ChangeStatus.ABANDONED || + change.status === ChangeStatus.MERGED || + revision === undefined || + revision._number === EDIT + ) { + return true; + } + + // show owned files if user owns anything + if ( + !allFilesApproved && + change.submit_requirements && + change.submit_requirements.find(r => r.name === OWNERS_SUBMIT_REQUIREMENT) + ) { + return ( + !user || + user.role === UserRole.ANONYMOUS || + (ownedFiles ?? []).length === 0 + ); + } + return true; +} + +export function ownedFiles( + owner?: AccountInfo, + files?: OwnedFiles +): string[] | undefined { + if (!owner || !files) { + return; + } + + const ownedFiles = []; + for (const file of Object.keys(files)) { + if ( + files[file].find( + fileOwner => isOwner(fileOwner) && fileOwner.id === owner._account_id + ) + ) { + ownedFiles.push(file); + } + } + + return ownedFiles; +}
diff --git a/owners/web/gr-owned-files_test.ts b/owners/web/gr-owned-files_test.ts new file mode 100644 index 0000000..0437ab9 --- /dev/null +++ b/owners/web/gr-owned-files_test.ts
@@ -0,0 +1,185 @@ +/** + * @license + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import {assert} from '@open-wc/testing'; +import { + AccountInfo, + ChangeInfo, + ChangeStatus, + RevisionInfo, + EDIT, + SubmitRequirementResultInfo, +} from '@gerritcodereview/typescript-api/rest-api'; +import {ownedFiles, shouldHide} from './gr-owned-files'; +import {OwnedFiles, Owner} from './owners-service'; +import {deepEqual} from './utils'; +import {User, UserRole} from './owners-model'; +import {getRandom} from './test-utils'; + +suite('owned files tests', () => { + suite('ownedFiles tests', () => { + const ownerAccountId = 1; + const owner = account(ownerAccountId); + + const ownedFile = 'README.md'; + const files = { + [ownedFile]: [fileOwner(1)], + 'some.text': [fileOwner(6)], + } as unknown as OwnedFiles; + + test('ownedFiles - should be `undefined` when owner is `undefined`', () => { + const undefinedOwner = undefined; + assert.equal(ownedFiles(undefinedOwner, files), undefined); + }); + + test('ownedFiles - should be `undefined` when files are `undefined`', () => { + const undefinedFiles = undefined; + assert.equal(ownedFiles(owner, undefinedFiles), undefined); + }); + + test('ownedFiles - should return empty owned file when no files are owned by user', () => { + const user = account(2); + assert.equal(deepEqual(ownedFiles(user, files), []), true); + }); + + test('ownedFiles - should return owned files', () => { + assert.equal(deepEqual(ownedFiles(owner, files), [ownedFile]), true); + }); + }); + + suite('shouldHide tests', () => { + const change = { + status: ChangeStatus.NEW, + submit_requirements: [ + {name: 'Owner-Approval'}, + ] as unknown as SubmitRequirementResultInfo[], + } as unknown as ChangeInfo; + const revisionInfo = {_number: 1} as unknown as RevisionInfo; + const allFilesApproved = true; + const user = {account: account(1), role: UserRole.OTHER}; + const ownedFiles = ['README.md']; + + test('shouldHide - should be `true` when change is `undefined`', () => { + const undefinedChange = undefined; + assert.equal( + shouldHide( + undefinedChange, + revisionInfo, + !allFilesApproved, + user, + ownedFiles + ), + true + ); + }); + + test('shouldHide - should be `true` when change is `ABANDONED` or `MERGED`', () => { + const abandonedOrMergedChange = { + ...change, + status: getRandom(ChangeStatus.ABANDONED, ChangeStatus.MERGED), + }; + assert.equal( + shouldHide( + abandonedOrMergedChange, + revisionInfo, + !allFilesApproved, + user, + ownedFiles + ), + true + ); + }); + + test('shouldHide - should be `true` when revisionInfo is `undefined` or in `EDIT` mode', () => { + const undefinedOrEditRevisionInfo = getRandom(undefined, { + _number: EDIT, + } as unknown as RevisionInfo); + assert.equal( + shouldHide( + change, + undefinedOrEditRevisionInfo, + !allFilesApproved, + user, + ownedFiles + ), + true + ); + }); + + test('shouldHide - should be `true` when change has different submit requirements', () => { + const changeWithOtherSubmitRequirements = { + ...change, + submit_requirements: [ + {name: 'Other'}, + ] as unknown as SubmitRequirementResultInfo[], + }; + assert.equal( + shouldHide( + changeWithOtherSubmitRequirements, + revisionInfo, + !allFilesApproved, + user, + ownedFiles + ), + true + ); + }); + + test('shouldHide - should be `true` when all files are approved', () => { + assert.equal( + shouldHide(change, revisionInfo, allFilesApproved, user, ownedFiles), + true + ); + }); + + test('shouldHide - should be `true` when user is `undefined` or `ANONYMOUS`', () => { + const undefinedOrAnonymousUser = getRandom(undefined, { + role: UserRole.ANONYMOUS, + } as unknown as User); + assert.equal( + shouldHide( + change, + revisionInfo, + !allFilesApproved, + undefinedOrAnonymousUser, + ownedFiles + ), + true + ); + }); + + test('shouldHide - should be `false` when user owns files', () => { + assert.equal( + shouldHide(change, revisionInfo, !allFilesApproved, user, ownedFiles), + false + ); + }); + }); +}); + +function account(id: number): AccountInfo { + return { + _account_id: id, + } as unknown as AccountInfo; +} + +function fileOwner(id: number): Owner { + return { + id, + name: `name for account: ${id}`, + } as unknown as Owner; +}
diff --git a/owners/web/owners-mixin.ts b/owners/web/owners-mixin.ts index 0dde0d2..0bc813a 100644 --- a/owners/web/owners-mixin.ts +++ b/owners/web/owners-mixin.ts
@@ -21,7 +21,7 @@ import {ChangeInfo} from '@gerritcodereview/typescript-api/rest-api'; import {FilesOwners, OwnersService} from './owners-service'; import {RestPluginApi} from '@gerritcodereview/typescript-api/rest'; -import {ModelLoader, OwnersModel, PatchRange, UserRole} from './owners-model'; +import {ModelLoader, OwnersModel, PatchRange, User} from './owners-model'; // Lit mixin definition as described in https://lit.dev/docs/composition/mixins/ // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -31,7 +31,7 @@ change?: ChangeInfo; patchRange?: PatchRange; restApi?: RestPluginApi; - userRole?: UserRole; + user?: User; allFilesApproved?: boolean; filesOwners?: FilesOwners; @@ -52,7 +52,7 @@ restApi?: RestPluginApi; @state() - userRole?: UserRole; + user?: User; @state() allFilesApproved?: boolean; @@ -81,7 +81,7 @@ this.subscriptions.push( model.state$.subscribe(s => { - this.userRole = s.userRole; + this.user = s.user; }) ); @@ -120,7 +120,7 @@ } protected onModelUpdate() { - this.modelLoader?.loadUserRole(); + this.modelLoader?.loadUser(); this.modelLoader?.loadAllFilesApproved(); this.modelLoader?.loadFilesOwners(); }
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts index 5ed7df3..6d048e0 100644 --- a/owners/web/owners-model.ts +++ b/owners/web/owners-model.ts
@@ -17,6 +17,7 @@ import {BehaviorSubject, Observable} from 'rxjs'; import { + AccountInfo, BasePatchSetNum, ChangeInfo, RevisionPatchSetNum, @@ -35,8 +36,13 @@ OTHER = 'OTHER', } +export interface User { + account?: AccountInfo; + role: UserRole; +} + export interface OwnersState { - userRole?: UserRole; + user?: User; allFilesApproved?: boolean; filesOwners?: FilesOwners; } @@ -80,10 +86,10 @@ this.subject$.next(Object.freeze(state)); } - setUserRole(userRole: UserRole) { + setUser(user: User) { const current = this.subject$.getValue(); - if (current.userRole === userRole) return; - this.setState({...current, userRole}); + if (current.user === user) return; + this.setState({...current, user}); } setAllFilesApproved(allFilesApproved: boolean | undefined) { @@ -112,11 +118,11 @@ private readonly model: OwnersModel ) {} - async loadUserRole() { + async loadUser() { await this._loadProperty( - 'userRole', - () => this.service.getLoggedInUserRole(), - value => this.model.setUserRole(value) + 'user', + () => this.service.getLoggedInUser(), + value => this.model.setUser(value) ); }
diff --git a/owners/web/owners-service.ts b/owners/web/owners-service.ts index 8d517a9..f1bb4fd 100644 --- a/owners/web/owners-service.ts +++ b/owners/web/owners-service.ts
@@ -24,7 +24,7 @@ RepoName, SubmitRequirementStatus, } from '@gerritcodereview/typescript-api/rest-api'; -import {UserRole} from './owners-model'; +import {User, UserRole} from './owners-model'; export interface GroupOwner { name: string; @@ -126,15 +126,16 @@ this.api = new OwnersApi(restApi); } - async getLoggedInUserRole(): Promise<UserRole> { + async getLoggedInUser(): Promise<User> { const account = await this.api.getAccount(); if (!account) { - return UserRole.ANONYMOUS; + return {role: UserRole.ANONYMOUS} as unknown as User; } - if (this.change.owner._account_id === account._account_id) { - return UserRole.CHANGE_OWNER; - } - return UserRole.OTHER; + const role = + this.change.owner._account_id === account._account_id + ? UserRole.CHANGE_OWNER + : UserRole.OTHER; + return {account, role} as unknown as User; } async getAllFilesApproved(): Promise<boolean | undefined> { @@ -176,8 +177,8 @@ } private async isLoggedIn(): Promise<boolean> { - const userRole = await this.getLoggedInUserRole(); - return userRole && userRole !== UserRole.ANONYMOUS; + const user = await this.getLoggedInUser(); + return user && user.role !== UserRole.ANONYMOUS; } static getOwnersService(restApi: RestPluginApi, change: ChangeInfo) {
diff --git a/owners/web/owners-service_test.ts b/owners/web/owners-service_test.ts index 3ac8590..0b77f66 100644 --- a/owners/web/owners-service_test.ts +++ b/owners/web/owners-service_test.ts
@@ -21,6 +21,7 @@ RestPluginApi, } from '@gerritcodereview/typescript-api/rest'; import { + AccountInfo, ChangeInfo, ChangeStatus, HttpMethod, @@ -51,7 +52,7 @@ }); suite('user role tests', () => { - test('getLoggedInUserRole - returns ANONYMOUS when user not logged in', async () => { + test('getLoggedInUser - returns ANONYMOUS when user not logged in', async () => { const notLoggedInApi = { getLoggedIn() { return Promise.resolve(false); @@ -62,43 +63,48 @@ notLoggedInApi, fakeChange ); - const userRole = await service.getLoggedInUserRole(); - assert.equal(userRole, UserRole.ANONYMOUS); + const user = await service.getLoggedInUser(); + assert.equal(user.role, UserRole.ANONYMOUS); + assert.equal(user.account, undefined); }); - test('getLoggedInUserRole - returns OTHER for logged in user that is NOT change owner', async () => { + test('getLoggedInUser - returns OTHER for logged in user that is NOT change owner', async () => { + const loggedUser = account(2); const userLoggedInApi = { getLoggedIn() { return Promise.resolve(true); }, getAccount() { - return Promise.resolve(account(2)); + return Promise.resolve(loggedUser); }, } as unknown as RestPluginApi; const change = {owner: account(1)} as unknown as ChangeInfo; const service = OwnersService.getOwnersService(userLoggedInApi, change); - const userRole = await service.getLoggedInUserRole(); - assert.equal(userRole, UserRole.OTHER); + const user = await service.getLoggedInUser(); + assert.equal(user.role, UserRole.OTHER); + assert.equal(user.account, loggedUser); }); - test('getLoggedInUserRole - returns CHANGE_OWNER for logged in user that is a change owner', async () => { + test('getLoggedInUser - returns CHANGE_OWNER for logged in user that is a change owner', async () => { + const owner = account(1); const changeOwnerLoggedInApi = { getLoggedIn() { return Promise.resolve(true); }, getAccount() { - return Promise.resolve(account(1)); + return Promise.resolve(owner); }, } as unknown as RestPluginApi; - const change = {owner: account(1)} as unknown as ChangeInfo; + const change = {owner} as unknown as ChangeInfo; const service = OwnersService.getOwnersService( changeOwnerLoggedInApi, change ); - const userRole = await service.getLoggedInUserRole(); - assert.equal(userRole, UserRole.CHANGE_OWNER); + const user = await service.getLoggedInUser(); + assert.equal(user.role, UserRole.CHANGE_OWNER); + assert.equal(user.account, owner); }); }); @@ -289,10 +295,10 @@ }); }); -function account(id: number) { +function account(id: number): AccountInfo { return { _account_id: id, - }; + } as unknown as AccountInfo; } function flush() {
diff --git a/owners/web/plugin.ts b/owners/web/plugin.ts index 8dcba05..596c8ab 100644 --- a/owners/web/plugin.ts +++ b/owners/web/plugin.ts
@@ -21,6 +21,12 @@ FilesColumnContent, FilesColumnHeader, } from './gr-files'; +import { + OWNED_FILES_TAB_CONTENT, + OWNED_FILES_TAB_HEADER, + OwnedFilesTabContent, + OwnedFilesTabHeader, +} from './gr-owned-files'; window.Gerrit.install(plugin => { const restApi = plugin.restApi(); @@ -41,4 +47,20 @@ .onAttached(view => { (view as unknown as FilesColumnContent).restApi = restApi; }); + plugin + .registerDynamicCustomComponent( + 'change-view-tab-header', + OWNED_FILES_TAB_HEADER + ) + .onAttached(view => { + (view as unknown as OwnedFilesTabHeader).restApi = restApi; + }); + plugin + .registerDynamicCustomComponent( + 'change-view-tab-content', + OWNED_FILES_TAB_CONTENT + ) + .onAttached(view => { + (view as unknown as OwnedFilesTabContent).restApi = restApi; + }); });
diff --git a/owners/web/test-utils.ts b/owners/web/test-utils.ts new file mode 100644 index 0000000..8f0c8a6 --- /dev/null +++ b/owners/web/test-utils.ts
@@ -0,0 +1,21 @@ +/** + * @license + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +export function getRandom<T>(...values: T[]): T { + const idx = Math.floor(Math.random() * values.length); + return values[idx]; +}