Indicate file that needs file owner's review When logged in user opens details screen then: * if any file has missing file owner's approval then show and populate the `Status` column (prior the `File Status` column) with an icon for each file that needs approval from owner * if all files are approved or repository has no owners etc.. then no statuses are shown or populated TODO: ATM `Missing owner approval` tooltip is displayed when one hovers over the file that has addtional icon. In the follow up change(s) it will be extended to display the file owners. Bug: Issue 373151160 Change-Id: Ice722ec49710377991018b151a1521217c5ed0c1
diff --git a/owners/web/gr-owners.ts b/owners/web/gr-owners.ts index 85e92dc..ca761fd 100644 --- a/owners/web/gr-owners.ts +++ b/owners/web/gr-owners.ts
@@ -16,7 +16,7 @@ */ import {Subscription} from 'rxjs'; -import {LitElement, nothing, PropertyValues} from 'lit'; +import {css, html, LitElement, nothing, PropertyValues} from 'lit'; import {customElement, property, state} from 'lit/decorators'; import { ChangeInfo, @@ -28,13 +28,40 @@ OwnersService, } from './owners-service'; import {RestPluginApi} from '@gerritcodereview/typescript-api/rest'; -import {ModelLoader, OwnersModel, PatchRange, UserRole} from './owners-model'; +import { + FileOwnership, + FileStatus, + ModelLoader, + OwnersModel, + PatchRange, + UserRole, +} from './owners-model'; + +const STATUS_CODE = { + MISSING: 'missing', +}; + +const STATUS_ICON = { + [STATUS_CODE.MISSING]: 'schedule', +}; + +const STATUS_SUMMARY = { + [STATUS_CODE.MISSING]: 'Missing', +}; + +const STATUS_TOOLTIP = { + [STATUS_CODE.MISSING]: 'Missing owner approval', +}; + +const FILE_STATUS = { + [FileStatus.NEEDS_APPROVAL]: STATUS_CODE.MISSING, +}; // Lit mixin definition as described in https://lit.dev/docs/composition/mixins/ // eslint-disable-next-line @typescript-eslint/no-explicit-any export type Constructor<T> = new (...args: any[]) => T; -interface CommonInterface { +interface CommonInterface extends LitElement { change?: ChangeInfo; patchRange?: PatchRange; restApi?: RestPluginApi; @@ -123,7 +150,12 @@ this.model = model; } - this.hidden = shouldHide(this.change, this.patchRange, this.userRole); + this.hidden = shouldHide( + this.change, + this.patchRange, + this.allFilesApproved, + this.userRole + ); } protected onModelUpdate() { @@ -147,14 +179,23 @@ @customElement(FILE_OWNERS_COLUMN_HEADER) export class FileOwnersColumnHeader extends common { static override get styles() { - return []; + return [ + css` + :host() { + display: block; + padding-right: var(--spacing-m); + width: 4em; + } + :host[hidden] { + display: none; + } + `, + ]; } override render() { - console.log( - `hidden: ${this.hidden}, userRole: ${this.userRole}, allFilesApproved: ${this.allFilesApproved}` - ); - return nothing; + if (this.hidden) return nothing; + return html`<div>Status</div>`; } } @@ -167,25 +208,78 @@ @property({type: String}) oldPath?: string; + @property({type: String, reflect: true, attribute: 'file-status'}) + fileStatus?: string; + static override get styles() { - return []; + return [ + css` + :host { + display: flex; + padding-right: var(--spacing-m); + width: 4em; + text-align: center; + } + :host[hidden] { + display: none; + } + gr-icon { + padding: var(--spacing-xs) 0px; + } + :host([file-status='missing']) gr-icon.status { + color: #ffa62f; + } + `, + ]; } override render() { - console.log( - `hidden: ${this.hidden}, userRole: ${this.userRole}, path: ${ - this.path - }, oldPath: ${this.oldPath}, filesOwners: ${JSON.stringify( - this.filesOwners - )}` + if (this.hidden || !this.fileStatus) { + return nothing; + } + + const info = STATUS_SUMMARY[this.fileStatus]; + const tooltip = STATUS_TOOLTIP[this.fileStatus]; + const icon = STATUS_ICON[this.fileStatus]; + return html` + <gr-tooltip-content + title=${tooltip} + aria-label=${info} + aria-description=${info} + has-tooltip + > + <gr-icon class="status" icon=${icon} aria-hidden="true"></gr-icon> + </gt-tooltip-content> + `; + } + + protected override willUpdate(changedProperties: PropertyValues): void { + super.willUpdate(changedProperties); + this.computeFileState(); + } + + private computeFileState(): void { + const fileOwnership = getFileOwnership( + this.path, + this.allFilesApproved, + this.filesOwners ); - return nothing; + if ( + !fileOwnership || + fileOwnership.fileStatus === FileStatus.NOT_OWNED_OR_APPROVED + ) { + this.fileStatus = undefined; + return; + } + + this.fileStatus = FILE_STATUS[fileOwnership.fileStatus]; } } export function shouldHide( change?: ChangeInfo, patchRange?: PatchRange, + allFilesApproved?: boolean, userRole?: UserRole ): boolean { // don't show owners when no change or change is merged @@ -212,6 +306,7 @@ // show owners when they apply to the change and for logged in user if ( + !allFilesApproved && change.submit_requirements && change.submit_requirements.find(r => r.name === OWNERS_SUBMIT_REQUIREMENT) ) { @@ -219,3 +314,19 @@ } return true; } + +export function getFileOwnership( + path?: string, + allFilesApproved?: boolean, + filesOwners?: FilesOwners +): FileOwnership | undefined { + if (path === undefined || filesOwners === undefined) { + return undefined; + } + + const status = + allFilesApproved || !(filesOwners.files ?? {})[path] + ? FileStatus.NOT_OWNED_OR_APPROVED + : FileStatus.NEEDS_APPROVAL; + return {fileStatus: status} as unknown as FileOwnership; +}
diff --git a/owners/web/gr-owners_test.ts b/owners/web/gr-owners_test.ts index bf29042..ff76c9c 100644 --- a/owners/web/gr-owners_test.ts +++ b/owners/web/gr-owners_test.ts
@@ -17,15 +17,19 @@ import {assert} from '@open-wc/testing'; -import {shouldHide} from './gr-owners'; -import {PatchRange, UserRole} from './owners-model'; +import {getFileOwnership, shouldHide} from './gr-owners'; +import {FileOwnership, FileStatus, PatchRange, UserRole} from './owners-model'; import { ChangeInfo, ChangeStatus, SubmitRequirementResultInfo, } from '@gerritcodereview/typescript-api/rest-api'; +import {FilesOwners} from './owners-service'; +import {deepEqual} from './utils'; suite('owners status tests', () => { + const allFilesApproved = true; + suite('shouldHide tests', () => { const loggedIn = getRandom(UserRole.CHANGE_OWNER, UserRole.OTHER); @@ -33,7 +37,12 @@ const undefinedChange = undefined; const definedPatchRange = {} as unknown as PatchRange; assert.equal( - shouldHide(undefinedChange, definedPatchRange, loggedIn), + shouldHide( + undefinedChange, + definedPatchRange, + allFilesApproved, + loggedIn + ), true ); }); @@ -42,7 +51,12 @@ const definedChange = {} as unknown as ChangeInfo; const undefinedPatchRange = undefined; assert.equal( - shouldHide(definedChange, undefinedPatchRange, loggedIn), + shouldHide( + definedChange, + undefinedPatchRange, + allFilesApproved, + loggedIn + ), true ); }); @@ -53,7 +67,12 @@ } as unknown as ChangeInfo; const definedPatchRange = {} as unknown as PatchRange; assert.equal( - shouldHide(abandonedChange, definedPatchRange, loggedIn), + shouldHide( + abandonedChange, + definedPatchRange, + allFilesApproved, + loggedIn + ), true ); }); @@ -63,7 +82,10 @@ status: ChangeStatus.MERGED, } as unknown as ChangeInfo; const definedPatchRange = {} as unknown as PatchRange; - assert.equal(shouldHide(mergedChange, definedPatchRange, loggedIn), true); + assert.equal( + shouldHide(mergedChange, definedPatchRange, allFilesApproved, loggedIn), + true + ); }); test('shouldHide - should be `true` if not on the latest PS', () => { @@ -75,7 +97,10 @@ current_revision: 'current_rev', } as unknown as ChangeInfo; const patchRangeOnPs1 = {patchNum: 1} as unknown as PatchRange; - assert.equal(shouldHide(changeWithPs2, patchRangeOnPs1, loggedIn), true); + assert.equal( + shouldHide(changeWithPs2, patchRangeOnPs1, allFilesApproved, loggedIn), + true + ); }); const change = { @@ -88,7 +113,10 @@ const patchRange = {patchNum: 1} as unknown as PatchRange; test('shouldHide - should be `true` when change has no submit requirements', () => { - assert.equal(shouldHide(change, patchRange, loggedIn), true); + assert.equal( + shouldHide(change, patchRange, !allFilesApproved, loggedIn), + true + ); }); test('shouldHide - should be `true` when change has no `Owner-Approval` submit requirements', () => { @@ -99,34 +127,56 @@ ] as unknown as SubmitRequirementResultInfo[], }; assert.equal( - shouldHide(changeWithDifferentSubmitReqs, patchRange, loggedIn), + shouldHide( + changeWithDifferentSubmitReqs, + patchRange, + !allFilesApproved, + loggedIn + ), true ); }); - test('shouldHide - should be `true` when user is not logged in', () => { - const changeWithSubmitRequirements = { - ...change, - submit_requirements: [ - {name: 'Owner-Approval'}, - ] as unknown as SubmitRequirementResultInfo[], - }; + const changeWithSubmitRequirements = { + ...change, + submit_requirements: [ + {name: 'Owner-Approval'}, + ] as unknown as SubmitRequirementResultInfo[], + }; + + test('shouldHide - should be `true` when user is not change owner', () => { const anonymous = UserRole.ANONYMOUS; assert.equal( - shouldHide(changeWithSubmitRequirements, patchRange, anonymous), + shouldHide( + changeWithSubmitRequirements, + patchRange, + !allFilesApproved, + anonymous + ), true ); }); - test('shouldHide - should be `false` when change has submit requirements and user is logged in', () => { - const changeWithSubmitRequirements = { - ...change, - submit_requirements: [ - {name: 'Owner-Approval'}, - ] as unknown as SubmitRequirementResultInfo[], - }; + test('shouldHide - should be `true` when change has submit requirements and has all files approved even if user is logged in', () => { assert.equal( - shouldHide(changeWithSubmitRequirements, patchRange, loggedIn), + shouldHide( + changeWithSubmitRequirements, + patchRange, + allFilesApproved, + loggedIn + ), + true + ); + }); + + test('shouldHide - should be `false` when change has submit requirements, has no all files approved and user is logged in', () => { + assert.equal( + shouldHide( + changeWithSubmitRequirements, + patchRange, + !allFilesApproved, + loggedIn + ), false ); }); @@ -134,11 +184,69 @@ test('shouldHide - should be `false` when in edit mode', () => { const patchRangeWithoutPatchNum = {} as unknown as PatchRange; assert.equal( - shouldHide(change, patchRangeWithoutPatchNum, loggedIn), + shouldHide( + change, + patchRangeWithoutPatchNum, + allFilesApproved, + loggedIn + ), false ); }); }); + + suite('getFileOwnership tests', () => { + const path = 'readme.md'; + const emptyFilesOwners = {} as unknown as FilesOwners; + const fileOwnersWithPath = { + files: {[path]: [{name: 'John', id: 1}]}, + } as unknown as FilesOwners; + + test('getFileOwnership - should be `undefined` when path is `undefined', () => { + const undefinedPath = undefined; + assert.equal( + getFileOwnership(undefinedPath, allFilesApproved, emptyFilesOwners), + undefined + ); + }); + + test('getFileOwnership - should be `undefined` when file owners are `undefined', () => { + const undefinedFileOwners = undefined; + assert.equal( + getFileOwnership(path, allFilesApproved, undefinedFileOwners), + undefined + ); + }); + + test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED_OR_APPROVED` fileStatus when `allFilesApproved`', () => { + assert.equal( + deepEqual( + getFileOwnership(path, allFilesApproved, fileOwnersWithPath), + {fileStatus: FileStatus.NOT_OWNED_OR_APPROVED} as FileOwnership + ), + true + ); + }); + + test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED_OR_APPROVED` fileStatus when file has no owner', () => { + assert.equal( + deepEqual(getFileOwnership(path, !allFilesApproved, emptyFilesOwners), { + fileStatus: FileStatus.NOT_OWNED_OR_APPROVED, + } as FileOwnership), + true + ); + }); + + test('getFileOwnership - should return `FileOwnership` with `NEEDS_APPROVAL` fileStatus when file has owner', () => { + assert.equal( + deepEqual( + getFileOwnership(path, !allFilesApproved, fileOwnersWithPath), + {fileStatus: FileStatus.NEEDS_APPROVAL} as FileOwnership + ), + true + ); + }); + }); }); function getRandom<T>(...values: T[]): T {
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts index 61ced72..d1ecaab 100644 --- a/owners/web/owners-model.ts +++ b/owners/web/owners-model.ts
@@ -41,6 +41,23 @@ filesOwners?: FilesOwners; } +/** + * TODO: The plugin's REST endpoint returns only files that still need to be reviewed which means that file can only have two states: + * * needs approval + * * was not subject of OWNERS file or is already approved + */ +export enum FileStatus { + NEEDS_APPROVAL = 'NEEDS_APPROVAL', + NOT_OWNED_OR_APPROVED = 'NOT_OWNED_OR_APPROVED', +} + +/** + * TODO: extend FileOwnership with owners when it will be used by UI elements + */ +export interface FileOwnership { + fileStatus: FileStatus; +} + let ownersModel: OwnersModel | undefined; export class OwnersModel extends EventTarget {