Add `Status` column to the `Owned Files` tab Prior to this change it wasn't clear which owned files are already approved (for instance by other reviewers that own subset of files). Add a `Status` column (before `File`) that: * contains orange clock icon for each file that still needs to be approved * contains green ✓ icon for file that was approved by someone else Bug: Issue 384100207 Change-Id: I1dc8ce4b1d3bc90cdb58817a0f92a88c83611400
diff --git a/owners/web/gr-owned-files.ts b/owners/web/gr-owned-files.ts index 96ea373..8324225 100644 --- a/owners/web/gr-owned-files.ts +++ b/owners/web/gr-owned-files.ts
@@ -42,18 +42,23 @@ truncatePath, } from './utils'; -const STATUS_CODE = { - MISSING: 'missing', - APPROVED: 'approved', -}; +export enum FileStatus { + NEEDS_APPROVAL = 'missing', + APPROVED = 'approved', +} const STATUS_ICON = { - [STATUS_CODE.MISSING]: 'schedule', - [STATUS_CODE.APPROVED]: 'check', + [FileStatus.NEEDS_APPROVAL]: 'schedule', + [FileStatus.APPROVED]: 'check', }; +interface OwnedFileInfo { + status: FileStatus; + file: string; +} + export interface OwnedFilesInfo { - ownedFiles: string[]; + ownedFiles: OwnedFileInfo[]; numberOfPending: number; numberOfApproved: number; } @@ -91,7 +96,7 @@ @customElement(OWNED_FILES_TAB_HEADER) export class OwnedFilesTabHeader extends OwnedFilesCommon { @property({type: String, reflect: true, attribute: 'files-status'}) - filesStatus?: string; + filesStatus?: FileStatus; private ownedFiles: number | undefined; @@ -125,8 +130,8 @@ } else { [this.filesStatus, this.ownedFiles] = this.ownedFilesInfo.numberOfPending > 0 - ? [STATUS_CODE.MISSING, this.ownedFilesInfo.numberOfPending] - : [STATUS_CODE.APPROVED, this.ownedFilesInfo.numberOfApproved]; + ? [FileStatus.NEEDS_APPROVAL, this.ownedFilesInfo.numberOfPending] + : [FileStatus.APPROVED, this.ownedFilesInfo.numberOfApproved]; } } @@ -184,7 +189,7 @@ } private buildInfoAndSummary( - filesStatus: string, + filesStatus: FileStatus, filesApproved: number, filesPending: number ): [string, string] { @@ -201,12 +206,12 @@ } already approved.` : ''; const info = `${ - STATUS_CODE.APPROVED === filesStatus + FileStatus.APPROVED === filesStatus ? approvedInfo : `${pendingInfo}${filesApproved > 0 ? ` and ${approvedInfo}` : '.'}` }`; const summary = `${ - STATUS_CODE.APPROVED === filesStatus ? 'Approved' : 'Missing' + FileStatus.APPROVED === filesStatus ? 'Approved' : 'Missing' }`; return [info, summary]; } @@ -221,7 +226,7 @@ revision?: RevisionInfo; @property({type: Array}) - ownedFiles?: string[]; + ownedFiles?: OwnedFileInfo[]; static override get styles() { return [ @@ -238,9 +243,11 @@ padding: var(--spacing-xs) var(--spacing-l); } .header-row { + padding-left: var(--spacing-m); background-color: var(--background-color-secondary); } .file-row { + padding-left: var(--spacing-m); cursor: pointer; } .file-row:hover { @@ -249,13 +256,23 @@ .file-row.selected { background-color: var(--selection-background-color); } + .status { + padding: inherit; + } .path { + padding: inherit; cursor: pointer; flex: 1; /* Wrap it into multiple lines if too long. */ white-space: normal; word-break: break-word; } + gr-icon.status.approved { + color: var(--positive-green-text-color); + } + gr-icon.status.missing { + color: #ffa62f; + } .matchingFilePath { color: var(--deemphasized-text-color); } @@ -304,24 +321,39 @@ private renderOwnedFilesHeaderRow() { return html` <div class="header-row row" role="row"> + <div class="status" role="columnheader">Status</div> <div class="path" role="columnheader">File</div> </div> `; } - private renderOwnedFileRow(ownedFile: string, index: number) { + private renderOwnedFileRow(ownedFile: OwnedFileInfo, index: number) { return html` <div class="file-row row" tabindex="-1" role="row" - aria-label=${ownedFile} + aria-label=${ownedFile.file} > - ${this.renderFilePath(ownedFile, index)} + ${this.renderFileStatus(ownedFile.status)} + ${this.renderFilePath(ownedFile.file, index)} </div> `; } + private renderFileStatus(status: FileStatus) { + const icon = STATUS_ICON[status]; + return html` + <span class="status" role="gridcell"> + <gr-icon + class="status ${status}" + icon=${icon} + aria-hidden="true" + ></gr-icon> + </span> + `; + } + private renderFilePath(file: string, index: number) { const displayPath = computeDisplayPath(file); const previousFile = (this.ownedFiles ?? [])[index - 1]; @@ -332,7 +364,7 @@ href=${ifDefined(computeDiffUrl(file, this.change, this.revision))} > <span title=${displayPath} class="fullFileName"> - ${this.renderStyledPath(file, previousFile)} + ${this.renderStyledPath(file, previousFile?.file)} </span> <span title=${displayPath} class="truncatedFileName"> ${truncatePath(displayPath)} @@ -392,7 +424,7 @@ change?: ChangeInfo, revision?: RevisionInfo, user?: User, - ownedFiles?: string[] + ownedFiles?: OwnedFileInfo[] ) { // don't show owned files when no change or change is abandoned/merged or being edited or viewing not current PS if ( @@ -421,8 +453,9 @@ owner: AccountInfo, groupPrefix: string, files: OwnedFiles, + fileStatus: FileStatus, emailWithoutDomain?: string -): string[] { +): OwnedFileInfo[] { const ownedFiles = []; for (const file of Object.keys(files ?? [])) { if ( @@ -438,7 +471,7 @@ ); }) ) { - ownedFiles.push(file); + ownedFiles.push({file, status: fileStatus} as OwnedFileInfo); } } @@ -463,12 +496,14 @@ owner, groupPrefix, filesOwners.files, + FileStatus.NEEDS_APPROVAL, emailWithoutDomain ); const approvedFiles = collectOwnedFiles( owner, groupPrefix, filesOwners.files_approved, + FileStatus.APPROVED, emailWithoutDomain ); return {
diff --git a/owners/web/gr-owned-files_test.ts b/owners/web/gr-owned-files_test.ts index d7bef24..fd37f67 100644 --- a/owners/web/gr-owned-files_test.ts +++ b/owners/web/gr-owned-files_test.ts
@@ -26,7 +26,12 @@ EDIT, SubmitRequirementResultInfo, } from '@gerritcodereview/typescript-api/rest-api'; -import {ownedFiles, OwnedFilesInfo, shouldHide} from './gr-owned-files'; +import { + FileStatus, + ownedFiles, + OwnedFilesInfo, + shouldHide, +} from './gr-owned-files'; import {FilesOwners, GroupOwner, Owner} from './owners-service'; import {deepEqual} from './utils'; import {User, UserRole} from './owners-model'; @@ -77,7 +82,10 @@ test('ownedFiles - should return owned files', () => { assert.equal( deepEqual(ownedFiles(owner, filesOwners), { - ownedFiles: [ownedFile, ownedApprovedFile], + ownedFiles: [ + {file: ownedFile, status: FileStatus.NEEDS_APPROVAL}, + {file: ownedApprovedFile, status: FileStatus.APPROVED}, + ], numberOfApproved: 1, numberOfPending: 1, }), @@ -94,7 +102,7 @@ } as unknown as FilesOwners; assert.equal( deepEqual(ownedFiles(owner, filesOwners), { - ownedFiles: [ownedFile], + ownedFiles: [{file: ownedFile, status: FileStatus.NEEDS_APPROVAL}], numberOfApproved: 0, numberOfPending: 1, }), @@ -111,7 +119,7 @@ } as unknown as FilesOwners; assert.equal( deepEqual(ownedFiles(owner, filesOwners), { - ownedFiles: [ownedFile], + ownedFiles: [{file: ownedFile, status: FileStatus.APPROVED}], numberOfApproved: 1, numberOfPending: 0, }), @@ -142,7 +150,7 @@ commit: {commit: current_revision}, } as unknown as RevisionInfo; const user = {account: account(1), role: UserRole.OTHER}; - const ownedFiles = ['README.md']; + const ownedFiles = [{file: 'README.md', status: FileStatus.NEEDS_APPROVAL}]; test('shouldHide - should be `true` when change is `undefined`', () => { const undefinedChange = undefined;