Show green ✔ (check mark) for file approved by owner(s) When a file that is subject of OWNERS gets approved then it is indicated by green ✔ (check mark) in the Status column of Files tab at change details screen. Hovering over the ✔ icon displays a tooltip window (titled `Approved by Owners`) that displays who approved the file in question (only first 5 approvers will be shown). Changes: * the status column is shown always in case when change is a subject of the OWNERS (is not merged|abandoned, plugin's REST API responds with `files_owners` defined) * `FileStatus` was modified and contains the following states: `NEEDS_APPROVAL|APPROVED|NOT_OWNED` * unit tests were adjusted to cover modified functionality Bug: Issue 377723880 Change-Id: I9162687259343afdc46cdbfa8ca3ad82429a35a3
diff --git a/owners/web/gr-files.ts b/owners/web/gr-files.ts index fb33778..567d84c 100644 --- a/owners/web/gr-files.ts +++ b/owners/web/gr-files.ts
@@ -40,14 +40,22 @@ const STATUS_CODE = { MISSING: 'missing', + APPROVED: 'approved', }; const STATUS_ICON = { [STATUS_CODE.MISSING]: 'schedule', + [STATUS_CODE.APPROVED]: 'check', }; const FILE_STATUS = { [FileStatus.NEEDS_APPROVAL]: STATUS_CODE.MISSING, + [FileStatus.APPROVED]: STATUS_CODE.APPROVED, +}; + +const HOVER_HEADING = { + [STATUS_CODE.MISSING]: "Needs Owners' Approval", + [STATUS_CODE.APPROVED]: 'Approved by Owners', }; const DISPLAY_OWNERS_FOR_FILE_LIMIT = 5; @@ -62,7 +70,7 @@ this.hidden = shouldHide( this.change, this.patchRange, - this.allFilesApproved, + this.filesOwners, this.user?.role ); } @@ -214,6 +222,9 @@ padding: var(--spacing-xs) 0px; margin-left: 9px; } + :host([file-status='approved']) gr-icon.status { + color: var(--positive-green-text-color); + } :host([file-status='missing']) gr-icon.status { color: #ffa62f; } @@ -246,6 +257,7 @@ const owners = this.owners ?? []; const splicedOwners = owners.splice(0, DISPLAY_OWNERS_FOR_FILE_LIMIT); const showEllipsis = owners.length > DISPLAY_OWNERS_FOR_FILE_LIMIT; + const heading = HOVER_HEADING[this.fileStatus ?? STATUS_CODE.MISSING]; // inlining <style> here is ugly but an alternative would be to copy the `HovercardMixin` from Gerrit and implement hoover from scratch return html`<gr-hovercard for="${this.pathId()}"> <style> @@ -296,7 +308,7 @@ </div> <div class="sectionContent"> <h3 class="name heading-3"> - <span>Needs Owners' Approval</span> + <span>${heading}</span> </h3> </div> </div> @@ -347,15 +359,8 @@ } private computeFileState(): void { - const fileOwnership = getFileOwnership( - this.path, - this.allFilesApproved, - this.filesOwners - ); - if ( - !fileOwnership || - fileOwnership.fileStatus === FileStatus.NOT_OWNED_OR_APPROVED - ) { + const fileOwnership = getFileOwnership(this.path, this.filesOwners); + if (!fileOwnership || fileOwnership.fileStatus === FileStatus.NOT_OWNED) { this.fileStatus = undefined; this.owners = undefined; return; @@ -377,7 +382,7 @@ export function shouldHide( change?: ChangeInfo, patchRange?: PatchRange, - allFilesApproved?: boolean, + filesOwners?: FilesOwners, userRole?: UserRole ): boolean { // don't show owners when no change or change is merged @@ -404,9 +409,12 @@ // 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) + change.submit_requirements.find( + r => r.name === OWNERS_SUBMIT_REQUIREMENT + ) && + filesOwners && + (filesOwners.files || filesOwners.files_approved) ) { return !userRole || userRole === UserRole.ANONYMOUS; } @@ -415,7 +423,6 @@ export function getFileOwnership( path?: string, - allFilesApproved?: boolean, filesOwners?: FilesOwners ): FileOwnership | undefined { if (path === undefined || filesOwners === undefined) { @@ -423,12 +430,20 @@ } const fileOwners = (filesOwners.files ?? {})[path]; - return (allFilesApproved || !fileOwners - ? {fileStatus: FileStatus.NOT_OWNED_OR_APPROVED} - : { - fileStatus: FileStatus.NEEDS_APPROVAL, - owners: fileOwners, - }) as unknown as FileOwnership; + const fileApprovers = (filesOwners.files_approved ?? {})[path]; + if (fileApprovers) { + return { + fileStatus: FileStatus.APPROVED, + owners: fileApprovers, + }; + } else if (fileOwners) { + return { + fileStatus: FileStatus.NEEDS_APPROVAL, + owners: fileOwners, + }; + } else { + return {fileStatus: FileStatus.NOT_OWNED}; + } } export function computeApprovalAndInfo(
diff --git a/owners/web/gr-files_test.ts b/owners/web/gr-files_test.ts index 0da0a35..e710b86 100644 --- a/owners/web/gr-files_test.ts +++ b/owners/web/gr-files_test.ts
@@ -38,7 +38,12 @@ import {getRandom} from './test-utils'; suite('owners status tests', () => { - const allFilesApproved = true; + const path = 'readme.md'; + const approvedPath = 'db.sql'; + const filesOwners = { + files: {[path]: [{name: 'John', id: 1}]}, + files_approved: {[approvedPath]: [{name: 'Merry', id: 2}]}, + } as unknown as FilesOwners; suite('shouldHide tests', () => { const loggedIn = getRandom(UserRole.CHANGE_OWNER, UserRole.OTHER); @@ -47,12 +52,7 @@ const undefinedChange = undefined; const definedPatchRange = {} as unknown as PatchRange; assert.equal( - shouldHide( - undefinedChange, - definedPatchRange, - allFilesApproved, - loggedIn - ), + shouldHide(undefinedChange, definedPatchRange, filesOwners, loggedIn), true ); }); @@ -61,12 +61,7 @@ const definedChange = {} as unknown as ChangeInfo; const undefinedPatchRange = undefined; assert.equal( - shouldHide( - definedChange, - undefinedPatchRange, - allFilesApproved, - loggedIn - ), + shouldHide(definedChange, undefinedPatchRange, filesOwners, loggedIn), true ); }); @@ -77,12 +72,7 @@ } as unknown as ChangeInfo; const definedPatchRange = {} as unknown as PatchRange; assert.equal( - shouldHide( - abandonedChange, - definedPatchRange, - allFilesApproved, - loggedIn - ), + shouldHide(abandonedChange, definedPatchRange, filesOwners, loggedIn), true ); }); @@ -93,7 +83,7 @@ } as unknown as ChangeInfo; const definedPatchRange = {} as unknown as PatchRange; assert.equal( - shouldHide(mergedChange, definedPatchRange, allFilesApproved, loggedIn), + shouldHide(mergedChange, definedPatchRange, filesOwners, loggedIn), true ); }); @@ -108,7 +98,7 @@ } as unknown as ChangeInfo; const patchRangeOnPs1 = {patchNum: 1} as unknown as PatchRange; assert.equal( - shouldHide(changeWithPs2, patchRangeOnPs1, allFilesApproved, loggedIn), + shouldHide(changeWithPs2, patchRangeOnPs1, filesOwners, loggedIn), true ); }); @@ -123,10 +113,7 @@ const patchRange = {patchNum: 1} as unknown as PatchRange; test('shouldHide - should be `true` when change has no submit requirements', () => { - assert.equal( - shouldHide(change, patchRange, !allFilesApproved, loggedIn), - true - ); + assert.equal(shouldHide(change, patchRange, filesOwners, loggedIn), true); }); test('shouldHide - should be `true` when change has no `Owner-Approval` submit requirements', () => { @@ -140,7 +127,7 @@ shouldHide( changeWithDifferentSubmitReqs, patchRange, - !allFilesApproved, + filesOwners, loggedIn ), true @@ -154,20 +141,39 @@ ] as unknown as SubmitRequirementResultInfo[], }; - test('shouldHide - should be `true` when user is not change owner', () => { + test('shouldHide - should be `true` when user is anonymous', () => { const anonymous = UserRole.ANONYMOUS; assert.equal( shouldHide( changeWithSubmitRequirements, patchRange, - !allFilesApproved, + filesOwners, anonymous ), true ); }); - test('shouldHide - should be `true` when change has submit requirements and has all files approved even if user is logged in', () => { + test('shouldHide - should be `true` when files owners are `undefined`', () => { + const undefinedFilesOwners = getRandom( + undefined, + {} + ) as unknown as FilesOwners; + assert.equal( + shouldHide( + changeWithSubmitRequirements, + patchRange, + undefinedFilesOwners, + loggedIn + ), + true + ); + }); + + test('shouldHide - should be `false` when change has all files approved', () => { + const allFilesApproved = { + approved_files: {[approvedPath]: [{name: 'Merry', id: 2}]}, + } as unknown as FilesOwners; assert.equal( shouldHide( changeWithSubmitRequirements, @@ -179,12 +185,27 @@ ); }); - test('shouldHide - should be `false` when change has submit requirements, has no all files approved and user is logged in', () => { + test('shouldHide - should be `false` when change has no files approved', () => { + const noApprovedFiles = { + files: {[path]: [{name: 'John', id: 1}]}, + } as unknown as FilesOwners; assert.equal( shouldHide( changeWithSubmitRequirements, patchRange, - !allFilesApproved, + noApprovedFiles, + loggedIn + ), + false + ); + }); + + test('shouldHide - should be `false` when change has both approved and not approved files', () => { + assert.equal( + shouldHide( + changeWithSubmitRequirements, + patchRange, + filesOwners, loggedIn ), false @@ -194,68 +215,57 @@ test('shouldHide - should be `false` when in edit mode', () => { const patchRangeWithoutPatchNum = {} as unknown as PatchRange; assert.equal( - shouldHide( - change, - patchRangeWithoutPatchNum, - allFilesApproved, - loggedIn - ), + shouldHide(change, patchRangeWithoutPatchNum, filesOwners, 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), + getFileOwnership(undefinedPath, emptyFilesOwners), undefined ); }); test('getFileOwnership - should be `undefined` when file owners are `undefined', () => { const undefinedFileOwners = undefined; - assert.equal( - getFileOwnership(path, allFilesApproved, undefinedFileOwners), - undefined - ); + assert.equal(getFileOwnership(path, undefinedFileOwners), undefined); }); - test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED_OR_APPROVED` fileStatus when `allFilesApproved`', () => { + test('getFileOwnership - should return `FileOwnership` with `APPROVED` fileStatus when file is approved', () => { assert.equal( - deepEqual( - getFileOwnership(path, allFilesApproved, fileOwnersWithPath), - {fileStatus: FileStatus.NOT_OWNED_OR_APPROVED} as FileOwnership - ), + deepEqual(getFileOwnership(approvedPath, filesOwners), { + fileStatus: FileStatus.APPROVED, + owners: [{name: 'Merry', id: 2}], + } as FileOwnership), true ); }); - test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED_OR_APPROVED` fileStatus when file has no owner', () => { + test('getFileOwnership - should return `FileOwnership` with `NOT_OWNED` fileStatus when file has no owner', () => { assert.equal( - deepEqual(getFileOwnership(path, !allFilesApproved, emptyFilesOwners), { - fileStatus: FileStatus.NOT_OWNED_OR_APPROVED, + deepEqual(getFileOwnership(path, emptyFilesOwners), { + fileStatus: FileStatus.NOT_OWNED, } as FileOwnership), true ); }); test('getFileOwnership - should return `FileOwnership` with `NEEDS_APPROVAL` fileStatus when file has owner', () => { + const fileOwnersWithPathOwner = { + files: {[path]: [{name: 'John', id: 1}]}, + } as unknown as FilesOwners; + assert.equal( - deepEqual( - getFileOwnership(path, !allFilesApproved, fileOwnersWithPath), - { - fileStatus: FileStatus.NEEDS_APPROVAL, - owners: [{name: 'John', id: 1}], - } as FileOwnership - ), + deepEqual(getFileOwnership(path, fileOwnersWithPathOwner), { + fileStatus: FileStatus.NEEDS_APPROVAL, + owners: [{name: 'John', id: 1}], + } as FileOwnership), true ); });
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts index 6d048e0..83af4e3 100644 --- a/owners/web/owners-model.ts +++ b/owners/web/owners-model.ts
@@ -47,19 +47,12 @@ 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', + APPROVED = 'APPROVED', + NOT_OWNED = 'NOT_OWNED', } -/** - * TODO: extend FileOwnership with owners when it will be used by UI elements - */ export interface FileOwnership { fileStatus: FileStatus; owners?: FileOwner[];
diff --git a/owners/web/owners-service.ts b/owners/web/owners-service.ts index f1bb4fd..7a295da 100644 --- a/owners/web/owners-service.ts +++ b/owners/web/owners-service.ts
@@ -53,6 +53,7 @@ export interface FilesOwners { files: OwnedFiles; + files_approved: OwnedFiles; owners_labels: OwnersLabels; } @@ -79,7 +80,7 @@ } /** - * Returns the list of owners associated to each file that needs a review, + * Returns the list of owners associated to each file that needs a review or were approved, * and, for each owner, its current labels and votes. * * @doc @@ -163,12 +164,6 @@ } async getFilesOwners(): Promise<FilesOwners | undefined> { - const allFilesApproved = await this.getAllFilesApproved(); - - if (allFilesApproved === undefined || allFilesApproved) { - return Promise.resolve(undefined); - } - return this.api.getFilesOwners( this.change.project, this.change._number,
diff --git a/owners/web/owners-service_test.ts b/owners/web/owners-service_test.ts index 0b77f66..1c01c07 100644 --- a/owners/web/owners-service_test.ts +++ b/owners/web/owners-service_test.ts
@@ -227,7 +227,7 @@ ); } - test('should have getAllFilesApproved `false` when no submit requirements are not satisfied', async () => { + test('should have getAllFilesApproved `false` when submit requirements are not satisfied', async () => { setupGetAllFilesApproved_false(); const response = await service.getAllFilesApproved(); @@ -237,32 +237,14 @@ function setupGetAllFilesApproved_true() { setup(isLoggedIn, changeNew, ownersSubmitRequirementsSatisfied); } - test('should have getAllFilesApproved `true` when no submit requirements are satisfied', async () => { + test('should have getAllFilesApproved `true` when submit requirements are satisfied', async () => { setupGetAllFilesApproved_true(); const response = await service.getAllFilesApproved(); assert.equal(response, true); }); - test('should not call getFilesOwners when getAllFilesApproved is `undefined`', async () => { - setupGetAllFilesApproved_undefined(); - - const response = await service.getFilesOwners(); - await flush(); - assert.equal(getApiStub.callCount, 0); - assert.equal(response, undefined); - }); - - test('should not call getFilesOwners when getAllFilesApproved is `true`', async () => { - setupGetAllFilesApproved_true(); - - const response = await service.getFilesOwners(); - await flush(); - assert.equal(getApiStub.callCount, 0); - assert.equal(response, undefined); - }); - - test('should call getFilesOwners when getAllFilesApproved is `false`', async () => { + test('should call getFilesOwners', async () => { const expected = { files: { 'AJavaFile.java': [{name: 'Bob', id: 1000001}],