Merge branch 'stable-3.9' into stable-3.10 * stable-3.9: Document Owned Files tab in the Change screen's owners UI Improve owners how-to-use.md Expire the owners cache after 1 minute and make it unlimited Optimize the 'path_owners_entries' cache eviction Introduce PathOwners cache from Ic7d61de07 Include Gerrit's `shared-styles` in `Owned Files` tab Document owners status on the change screen Add `Status` column to the `Owned Files` tab Change-Id: Icdac09cf936763ddd599e615032bea3a9abcb123
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java index 901dbc8..aea33ff 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntriesCache.java
@@ -26,6 +26,7 @@ import com.google.inject.Module; import com.google.inject.Singleton; import com.google.inject.TypeLiteral; +import java.time.Duration; import java.util.Objects; import java.util.Optional; import java.util.concurrent.Callable; @@ -38,7 +39,9 @@ return new CacheModule() { @Override protected void configure() { - cache(CACHE_NAME, Key.class, new TypeLiteral<Optional<OwnersConfig>>() {}); + cache(CACHE_NAME, Key.class, new TypeLiteral<Optional<OwnersConfig>>() {}) + .maximumWeight(Long.MAX_VALUE) + .expireAfterWrite(Duration.ofSeconds(60)); bind(PathOwnersEntriesCache.class).to(PathOwnersEntriesCacheImpl.class); DynamicSet.bind(binder(), GitBatchRefUpdateListener.class) .to(OwnersRefUpdateListener.class);
diff --git a/owners/src/main/resources/Documentation/files-owners-status-not-ready-hover.png b/owners/src/main/resources/Documentation/files-owners-status-not-ready-hover.png new file mode 100644 index 0000000..8c43565 --- /dev/null +++ b/owners/src/main/resources/Documentation/files-owners-status-not-ready-hover.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/files-owners-status-ready-hover.png b/owners/src/main/resources/Documentation/files-owners-status-ready-hover.png new file mode 100644 index 0000000..0d57ca6 --- /dev/null +++ b/owners/src/main/resources/Documentation/files-owners-status-ready-hover.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/files-owners-status.png b/owners/src/main/resources/Documentation/files-owners-status.png new file mode 100644 index 0000000..afdae26 --- /dev/null +++ b/owners/src/main/resources/Documentation/files-owners-status.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/how-to-use.md b/owners/src/main/resources/Documentation/how-to-use.md new file mode 100644 index 0000000..4a4d17f --- /dev/null +++ b/owners/src/main/resources/Documentation/how-to-use.md
@@ -0,0 +1,149 @@ +# How to use + +## Intro + +The `@PLUGIN@` plugin allows defining owners (individual accounts or groups) +of directories, sub-directories and files that require approval when changes +modify them. + +The rules defining which user/group needs to approve which file are specified +in the `OWNERS` file and are covered in the +[@PLUGIN@ configuration](config.html) guide. + +## Context + +The owners plugin can be used in three different modes: +1. Prolog rules only(deprecated since Gerrit 3.6). +2. With plugin-provided submit requirements. +3. With user defined custom submit requirements. + +When using the plugin in mode 1. the functionality is limited to the generation +of a Prolog-based submit rule with no extra UI features. + +On top of providing significantly better and more predictable performances, +using the plugin in either mode 2. or 3. provides extra capabilities like: +- A REST-api that exposes the owners approval status with single file granularity. +- Enhanced UI experience with much clearer explanation of who owns what files, + as explained below. + +## <a id="ownerStatus">Owner status on change page + +### <a id="ownerStatus.submitRequirements">Owners status for submit requirements + +When either `has:approval_owners` predicate is used in a submit requirement or +[enableSubmitRequirement](config.html#owners.enableSubmitRequirement) is enabled for a +project or any of its parents and, if applicable, owners status is displayed in two ways: + +* As an icon prepending the submit requirement and as a text next to it +\ + + +Which then displays more detail information in a tooltip when hovered over. + + + +* Next to all owned files, even if they are owned by someone else, (when the +newest patchset is selected) +\ + + +#### <a id="ownersStatus.submitRequirement.files">Per file owners statuses + +The @PLUGIN@ plugin also shows the owners statuses per file in the file list. +For each file, the owners status is shown as an icon. One can **hover over +the icon** to get additional information displayed as a tooltip. + +- **Needs Owners' Approval**\ + A file owner either has not voted or voted negatively or not with sufficient + score. +\ + + + + > **Notes:** + > + > The dialog contains up to **5** first file owners returned by the + > @PLUGIN@ plugin's REST API. + > + > Apart from the owner's name, their votes (if cast) and + > `Copy owner's email to clipboard` button are shown. + > + > Hovering over the owner's name results in their details being displayed + + +  + +- **Approved by Owners**\ + A file owner has approved the change. +\ + + + + > Note that again, apart from the owner's name, their vote and + > `Copy owner's email to clipboard` button are shown, and owner's details are + > displayed when one hovers over the name. + +### <a id="ownerStatus.submitRule">Owners status for submit rule + + `OWNERS` file can still be evaluated (without a need for the prolog +predicate being added to the project) by enabling the +[default submit requirements](config.html#owners.enableSubmitRequirement). +In this way, results of the `OWNERS` file evaluation are provided to +Gerrit's change through submit rule (as +[submit record](/Documentation/rest-api-changes.html#submit-record)) and, if +applicable, owners status is displayed: + +* As an icon prepending the `Code Review from owners` submit requirement +\ + + + + > Note that in replacement mode, owner's votes are displayed next to the + > `Code Review from owners` submit requirement instead of its status. + > It is a Gerrit's core representation of submit rule. + +* Next to all owned files (when the newest patchset is selected) +\ + + + + > There is no difference in this aspect between modes. + +#### <a id="ownersStatus.submitRule.owners">`Code Review from owners` submit requirement + +The `Code Review from owners` (that is available only in +[replacement mode](config.html#owners.enableSubmitRequirement)) submit +requirement provides general information if all owned files were approved +(requirement is satisfied). When hovered, a detailed description is shown + + + +#### <a id="ownersStatus.submitRule.files">Per file owners statuses + +The owners status per file in replacement mode doesn't differ from +[submit requirements mode](#ownersStatus.submitRequirement.files). + +### Files that are assigned to the current user based on the OWNERS + +When the current user is mentioned directly or indirectly via a group +ownership as the owner of one or more files, the change screen displays +an additional tab called _Owned Files_. + + + +The tab indicates whether the current user has review duties based on the +icon displayed on the right side of the tab. If the icon is an orange +clock, the current user is required to review the files indicated in the +tab; otherwise, if the icon is a green tick, then the current user review +input is not strictly required for the change to be merged. + +The list of files included in the _Owned Files_ tab, a subset of the full +list displayed in the _Files_ tab, are the ones assigned to the +current user through the OWNERS file. + +> **NOTE**: When the `owners.config` has `owners.expandGroups = false` +> then the association between files and the current user may not be +> accurate or not available because of the lack of group expansion and +> account resolution service on the backend. In this case, the +> _Owned Files_ tab may be absent even if the current user is effectively +> owner of one or more files in the change.
diff --git a/owners/src/main/resources/Documentation/owned-files.png b/owners/src/main/resources/Documentation/owned-files.png new file mode 100644 index 0000000..f88cbee --- /dev/null +++ b/owners/src/main/resources/Documentation/owned-files.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/owner-details.png b/owners/src/main/resources/Documentation/owner-details.png new file mode 100644 index 0000000..f5ba6fd --- /dev/null +++ b/owners/src/main/resources/Documentation/owner-details.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/submit-requirement-hover.png b/owners/src/main/resources/Documentation/submit-requirement-hover.png new file mode 100644 index 0000000..15b5f9c --- /dev/null +++ b/owners/src/main/resources/Documentation/submit-requirement-hover.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/submit-requirement.png b/owners/src/main/resources/Documentation/submit-requirement.png new file mode 100644 index 0000000..a9c27de --- /dev/null +++ b/owners/src/main/resources/Documentation/submit-requirement.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/submit-rule-hover.png b/owners/src/main/resources/Documentation/submit-rule-hover.png new file mode 100644 index 0000000..ab5995c --- /dev/null +++ b/owners/src/main/resources/Documentation/submit-rule-hover.png Binary files differ
diff --git a/owners/src/main/resources/Documentation/submit-rule.png b/owners/src/main/resources/Documentation/submit-rule.png new file mode 100644 index 0000000..b71e62f --- /dev/null +++ b/owners/src/main/resources/Documentation/submit-rule.png Binary files differ
diff --git a/owners/web/gr-owned-files.ts b/owners/web/gr-owned-files.ts index 96ea373..29f8b7a 100644 --- a/owners/web/gr-owned-files.ts +++ b/owners/web/gr-owned-files.ts
@@ -15,7 +15,15 @@ * limitations under the License. */ -import {css, html, LitElement, PropertyValues, nothing, CSSResult} from 'lit'; +import { + css, + html, + LitElement, + PropertyValues, + nothing, + CSSResult, + unsafeCSS, +} from 'lit'; import {ifDefined} from 'lit/directives/if-defined.js'; import {OwnersMixin} from './owners-mixin'; import {customElement, property} from 'lit/decorators'; @@ -42,18 +50,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; } @@ -79,7 +92,13 @@ } static commonStyles(): CSSResult[] { - return [window?.Gerrit?.styles.font as CSSResult]; + const template = document.querySelector( + 'dom-module#shared-styles > template' + ) as HTMLTemplateElement; + const sharedStyles = css` + ${unsafeCSS(template?.content?.querySelector('style')?.textContent)} + `; + return [sharedStyles, window?.Gerrit?.styles.font as CSSResult]; } private computeOwnedFiles() { @@ -91,7 +110,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 +144,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 +203,7 @@ } private buildInfoAndSummary( - filesStatus: string, + filesStatus: FileStatus, filesApproved: number, filesPending: number ): [string, string] { @@ -201,12 +220,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 +240,7 @@ revision?: RevisionInfo; @property({type: Array}) - ownedFiles?: string[]; + ownedFiles?: OwnedFileInfo[]; static override get styles() { return [ @@ -238,9 +257,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 +270,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 +335,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 +378,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 +438,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 +467,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 +485,7 @@ ); }) ) { - ownedFiles.push(file); + ownedFiles.push({file, status: fileStatus} as OwnedFileInfo); } } @@ -463,12 +510,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;
