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 {
