Merge branch 'stable-3.9' into stable-3.10 * stable-3.9: Fix `Owned Files` status icon when some files are already approved Fix misleading log message when adding reviewers Add status icon to the `Owned Files` tab header Unify check if change contains owners submit requirements Show owners files statuses and `Owned Files` tab in replacment mode Remove `allFilesApproved` state from the `OwnersState` Show `Owned Files` even when all files were approved Don't display `Owned Files` when not current PS is viewed Ensure that `Owned Files` is not switchable when no files are owned Match Owned Files entries even when owners.expandGroups == false Change-Id: I13ca03f357426a8971a376dcc624f1f419929a32
diff --git a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java index 83ba531..bdfa7f6 100644 --- a/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java +++ b/owners-autoassign/src/main/java/com/googlesource/gerrit/owners/common/SyncReviewerManager.java
@@ -106,7 +106,10 @@ in.reviewers = new ArrayList<>(accountsIds.size()); Collection<Account.Id> validOwnersForAttentionSet = new ArrayList<>(accountsIds.size()); for (Account.Id account : accountsIds) { - if (!currentReviewers.contains(account.get()) && isVisibleTo(changeInfo, account)) { + if (currentReviewers.contains(account.get())) { + continue; + } + if (isVisibleTo(changeInfo, account)) { ReviewerInput addReviewerInput = new ReviewerInput(); addReviewerInput.reviewer = account.toString(); addReviewerInput.state = reviewerState;
diff --git a/owners/web/gr-files.ts b/owners/web/gr-files.ts index 1e63863..d0fe189 100644 --- a/owners/web/gr-files.ts +++ b/owners/web/gr-files.ts
@@ -32,7 +32,7 @@ FilesOwners, isOwner, OwnersLabels, - OWNERS_SUBMIT_REQUIREMENT, + hasOwnersSubmitRequirement, } from './owners-service'; import { FileOwnership, @@ -457,10 +457,7 @@ // show owners when they apply to the change and for logged in user if ( - change.submit_requirements && - change.submit_requirements.find( - r => r.name === OWNERS_SUBMIT_REQUIREMENT - ) && + hasOwnersSubmitRequirement(change) && filesOwners && (filesOwners.files || filesOwners.files_approved) ) {
diff --git a/owners/web/gr-files_test.ts b/owners/web/gr-files_test.ts index 1797b20..d05f6e7 100644 --- a/owners/web/gr-files_test.ts +++ b/owners/web/gr-files_test.ts
@@ -126,7 +126,7 @@ const changeWithDifferentSubmitReqs = { ...change, submit_requirements: [ - {name: 'other'}, + {submittability_expression_result: {expression: 'other'}}, ] as unknown as SubmitRequirementResultInfo[], }; assert.equal( @@ -140,18 +140,26 @@ ); }); - const changeWithSubmitRequirements = { - ...change, + const submitRequirements = getRandom({ submit_requirements: [ - {name: 'Owner-Approval'}, + {submittability_expression_result: {expression: 'has:approval_owners'}}, + { + submittability_expression_result: { + expression: 'rule:owners~OwnersSubmitRequirement', + }, + }, ] as unknown as SubmitRequirementResultInfo[], + }); + const changeWithSubmitRequirementOrRecord = { + ...change, + ...submitRequirements, }; test('shouldHide - should be `true` when user is anonymous', () => { const anonymous = UserRole.ANONYMOUS; assert.equal( shouldHide( - changeWithSubmitRequirements, + changeWithSubmitRequirementOrRecord, patchRange, filesOwners, anonymous @@ -167,7 +175,7 @@ ) as unknown as FilesOwners; assert.equal( shouldHide( - changeWithSubmitRequirements, + changeWithSubmitRequirementOrRecord, patchRange, undefinedFilesOwners, loggedIn @@ -182,7 +190,7 @@ } as unknown as FilesOwners; assert.equal( shouldHide( - changeWithSubmitRequirements, + changeWithSubmitRequirementOrRecord, patchRange, allFilesApproved, loggedIn @@ -197,7 +205,7 @@ } as unknown as FilesOwners; assert.equal( shouldHide( - changeWithSubmitRequirements, + changeWithSubmitRequirementOrRecord, patchRange, noApprovedFiles, loggedIn @@ -209,7 +217,7 @@ test('shouldHide - should be `false` when change has both approved and not approved files', () => { assert.equal( shouldHide( - changeWithSubmitRequirements, + changeWithSubmitRequirementOrRecord, patchRange, filesOwners, loggedIn
diff --git a/owners/web/gr-owned-files.ts b/owners/web/gr-owned-files.ts index 969d91c..96ea373 100644 --- a/owners/web/gr-owned-files.ts +++ b/owners/web/gr-owned-files.ts
@@ -23,11 +23,17 @@ AccountInfo, ChangeInfo, ChangeStatus, + EmailAddress, RevisionInfo, EDIT, } from '@gerritcodereview/typescript-api/rest-api'; import {User, UserRole} from './owners-model'; -import {isOwner, OwnedFiles, OWNERS_SUBMIT_REQUIREMENT} from './owners-service'; +import { + FilesOwners, + hasOwnersSubmitRequirement, + isOwner, + OwnedFiles, +} from './owners-service'; import { computeDisplayPath, diffFilePaths, @@ -36,13 +42,29 @@ truncatePath, } from './utils'; +const STATUS_CODE = { + MISSING: 'missing', + APPROVED: 'approved', +}; + +const STATUS_ICON = { + [STATUS_CODE.MISSING]: 'schedule', + [STATUS_CODE.APPROVED]: 'check', +}; + +export interface OwnedFilesInfo { + ownedFiles: string[]; + numberOfPending: number; + numberOfApproved: number; +} + const common = OwnersMixin(LitElement); class OwnedFilesCommon extends common { @property({type: Object}) revision?: RevisionInfo; - protected ownedFiles?: string[]; + protected ownedFilesInfo?: OwnedFilesInfo; protected override willUpdate(changedProperties: PropertyValues): void { super.willUpdate(changedProperties); @@ -51,9 +73,8 @@ this.hidden = shouldHide( this.change, this.revision, - this.allFilesApproved, this.user, - this.ownedFiles + this.ownedFilesInfo?.ownedFiles ); } @@ -62,20 +83,132 @@ } private computeOwnedFiles() { - this.ownedFiles = ownedFiles(this.user?.account, this.filesOwners?.files); + this.ownedFilesInfo = ownedFiles(this.user?.account, this.filesOwners); } } export const OWNED_FILES_TAB_HEADER = 'owned-files-tab-header'; @customElement(OWNED_FILES_TAB_HEADER) export class OwnedFilesTabHeader extends OwnedFilesCommon { + @property({type: String, reflect: true, attribute: 'files-status'}) + filesStatus?: string; + + private ownedFiles: number | undefined; + static override get styles() { - return [...OwnedFilesCommon.commonStyles()]; + return [ + ...OwnedFilesCommon.commonStyles(), + css` + [hidden] { + display: none; + } + gr-icon { + padding: var(--spacing-xs) 0px; + margin-left: 3px; + } + :host([files-status='approved']) gr-icon.status { + color: var(--positive-green-text-color); + } + :host([files-status='missing']) gr-icon.status { + color: #ffa62f; + } + `, + ]; + } + + protected override willUpdate(changedProperties: PropertyValues): void { + super.willUpdate(changedProperties); + + if (this.hidden || this.ownedFilesInfo === undefined) { + this.filesStatus = undefined; + this.ownedFiles = undefined; + } else { + [this.filesStatus, this.ownedFiles] = + this.ownedFilesInfo.numberOfPending > 0 + ? [STATUS_CODE.MISSING, this.ownedFilesInfo.numberOfPending] + : [STATUS_CODE.APPROVED, this.ownedFilesInfo.numberOfApproved]; + } } override render() { - if (this.hidden) return nothing; - return html`<div>Owned Files</div>`; + // even if `nothing` is returned Gerrit still shows the pointer and allows + // clicking at it, redirecting to the empty tab when done; traverse through + // the shadowRoots down to the tab and disable/enable it when needed + const tabParent = document + .querySelector('#pg-app') + ?.shadowRoot?.querySelector('#app-element') + ?.shadowRoot?.querySelector('main > gr-change-view') + ?.shadowRoot?.querySelector( + '#tabs > paper-tab[data-name="change-view-tab-header-owners"]' + ); + if (this.hidden) { + if (tabParent && !tabParent.getAttribute('disabled')) { + tabParent.setAttribute('disabled', 'disabled'); + } + return nothing; + } + if (tabParent && tabParent.getAttribute('disabled')) { + tabParent.removeAttribute('disabled'); + } + + if ( + this.ownedFilesInfo === undefined || + this.filesStatus === undefined || + this.ownedFiles === undefined + ) { + return html`<div>Owned Files</div>`; + } + + const icon = STATUS_ICON[this.filesStatus]; + const [info, summary] = this.buildInfoAndSummary( + this.filesStatus, + this.ownedFilesInfo.numberOfApproved, + this.ownedFilesInfo.numberOfPending + ); + return html` <div> + Owned Files + <gr-tooltip-content + title=${info} + aria-label=${summary} + aria-description=${info} + has-tooltip + > + <gr-icon + id="owned-files-status" + class="status" + icon=${icon} + aria-hidden="true" + ></gr-icon> + </gr-tooltip-content> + </div>`; + } + + private buildInfoAndSummary( + filesStatus: string, + filesApproved: number, + filesPending: number + ): [string, string] { + const pendingInfo = + filesPending > 0 + ? `Missing approval for ${filesPending} file${ + filesPending > 1 ? 's' : '' + }` + : ''; + const approvedInfo = + filesApproved > 0 + ? `${filesApproved} file${ + filesApproved > 1 ? 's' : '' + } already approved.` + : ''; + const info = `${ + STATUS_CODE.APPROVED === filesStatus + ? approvedInfo + : `${pendingInfo}${filesApproved > 0 ? ` and ${approvedInfo}` : '.'}` + }`; + const summary = `${ + STATUS_CODE.APPROVED === filesStatus ? 'Approved' : 'Missing' + }`; + return [info, summary]; } } @@ -248,7 +381,7 @@ id="ownedFilesList" .change=${this.change} .revision=${this.revision} - .ownedFiles=${this.ownedFiles} + .ownedFiles=${this.ownedFilesInfo?.ownedFiles} > </gr-owned-files-list> `; @@ -258,27 +391,23 @@ export function shouldHide( change?: ChangeInfo, revision?: RevisionInfo, - allFilesApproved?: boolean, user?: User, ownedFiles?: string[] ) { - // don't show owned files when no change or change is abandoned/merged or being edited + // don't show owned files when no change or change is abandoned/merged or being edited or viewing not current PS if ( change === undefined || change.status === ChangeStatus.ABANDONED || change.status === ChangeStatus.MERGED || revision === undefined || - revision._number === EDIT + revision._number === EDIT || + change.current_revision !== revision.commit?.commit ) { return true; } // show owned files if user owns anything - if ( - !allFilesApproved && - change.submit_requirements && - change.submit_requirements.find(r => r.name === OWNERS_SUBMIT_REQUIREMENT) - ) { + if (hasOwnersSubmitRequirement(change)) { return ( !user || user.role === UserRole.ANONYMOUS || @@ -288,20 +417,26 @@ return true; } -export function ownedFiles( - owner?: AccountInfo, - files?: OwnedFiles -): string[] | undefined { - if (!owner || !files) { - return; - } - +function collectOwnedFiles( + owner: AccountInfo, + groupPrefix: string, + files: OwnedFiles, + emailWithoutDomain?: string +): string[] { const ownedFiles = []; - for (const file of Object.keys(files)) { + for (const file of Object.keys(files ?? [])) { if ( - files[file].find( - fileOwner => isOwner(fileOwner) && fileOwner.id === owner._account_id - ) + files[file].find(fileOwner => { + if (isOwner(fileOwner)) { + return fileOwner.id === owner._account_id; + } + + return ( + !fileOwner.name?.startsWith(groupPrefix) && + (fileOwner.name === emailWithoutDomain || + fileOwner.name === owner.name) + ); + }) ) { ownedFiles.push(file); } @@ -310,6 +445,39 @@ return ownedFiles; } +export function ownedFiles( + owner?: AccountInfo, + filesOwners?: FilesOwners +): OwnedFilesInfo | undefined { + if ( + !owner || + !filesOwners || + (!filesOwners.files && !filesOwners.files_approved) + ) { + return; + } + + const groupPrefix = 'group/'; + const emailWithoutDomain = toEmailWithoutDomain(owner.email); + const pendingFiles = collectOwnedFiles( + owner, + groupPrefix, + filesOwners.files, + emailWithoutDomain + ); + const approvedFiles = collectOwnedFiles( + owner, + groupPrefix, + filesOwners.files_approved, + emailWithoutDomain + ); + return { + ownedFiles: [...pendingFiles, ...approvedFiles], + numberOfPending: pendingFiles.length, + numberOfApproved: approvedFiles.length, + } as OwnedFilesInfo; +} + export function computeDiffUrl( file: string, change?: ChangeInfo, @@ -329,3 +497,8 @@ return `${getBaseUrl()}/c/${repo}${change._number}${range}${path}`; } + +function toEmailWithoutDomain(email?: EmailAddress): string | undefined { + const startDomainIndex = email?.indexOf('@'); + return startDomainIndex ? email?.substring(0, startDomainIndex) : undefined; +}
diff --git a/owners/web/gr-owned-files_test.ts b/owners/web/gr-owned-files_test.ts index 0437ab9..d7bef24 100644 --- a/owners/web/gr-owned-files_test.ts +++ b/owners/web/gr-owned-files_test.ts
@@ -17,15 +17,17 @@ import {assert} from '@open-wc/testing'; import { + AccountId, AccountInfo, ChangeInfo, ChangeStatus, + CommitId, RevisionInfo, EDIT, SubmitRequirementResultInfo, } from '@gerritcodereview/typescript-api/rest-api'; -import {ownedFiles, shouldHide} from './gr-owned-files'; -import {OwnedFiles, Owner} from './owners-service'; +import {ownedFiles, OwnedFilesInfo, shouldHide} from './gr-owned-files'; +import {FilesOwners, GroupOwner, Owner} from './owners-service'; import {deepEqual} from './utils'; import {User, UserRole} from './owners-model'; import {getRandom} from './test-utils'; @@ -36,53 +38,116 @@ const owner = account(ownerAccountId); const ownedFile = 'README.md'; + const ownedApprovedFile = 'db.sql'; const files = { [ownedFile]: [fileOwner(1)], 'some.text': [fileOwner(6)], - } as unknown as OwnedFiles; + }; + const files_approved = { + [ownedApprovedFile]: [fileOwner(1)], + }; + const filesOwners = { + files, + files_approved, + } as unknown as FilesOwners; + const emptyOwnerFilesInfo = { + ownedFiles: [], + numberOfApproved: 0, + numberOfPending: 0, + } as unknown as OwnedFilesInfo; test('ownedFiles - should be `undefined` when owner is `undefined`', () => { const undefinedOwner = undefined; - assert.equal(ownedFiles(undefinedOwner, files), undefined); + assert.equal(ownedFiles(undefinedOwner, filesOwners), undefined); }); - test('ownedFiles - should be `undefined` when files are `undefined`', () => { - const undefinedFiles = undefined; - assert.equal(ownedFiles(owner, undefinedFiles), undefined); + test('ownedFiles - should be `undefined` when filesOwners are `undefined`', () => { + const undefinedFilesOwners = undefined; + assert.equal(ownedFiles(owner, undefinedFilesOwners), undefined); }); test('ownedFiles - should return empty owned file when no files are owned by user', () => { const user = account(2); - assert.equal(deepEqual(ownedFiles(user, files), []), true); + assert.equal( + deepEqual(ownedFiles(user, filesOwners), emptyOwnerFilesInfo), + true + ); }); test('ownedFiles - should return owned files', () => { - assert.equal(deepEqual(ownedFiles(owner, files), [ownedFile]), true); + assert.equal( + deepEqual(ownedFiles(owner, filesOwners), { + ownedFiles: [ownedFile, ownedApprovedFile], + numberOfApproved: 1, + numberOfPending: 1, + }), + true + ); + }); + + test('ownedFiles - should match file owner through email without domain name', () => { + const filesOwners = { + files: { + [ownedFile]: [fileOwnerWithNameOnly(`${ownerAccountId}_email`)], + 'some.text': [fileOwnerWithNameOnly('random_joe')], + }, + } as unknown as FilesOwners; + assert.equal( + deepEqual(ownedFiles(owner, filesOwners), { + ownedFiles: [ownedFile], + numberOfApproved: 0, + numberOfPending: 1, + }), + true + ); + }); + + test('ownedFiles - should match file owner through full name', () => { + const filesOwners = { + files_approved: { + [ownedFile]: [fileOwnerWithNameOnly(`${ownerAccountId}_name`)], + 'some.text': [fileOwnerWithNameOnly('random_joe')], + }, + } as unknown as FilesOwners; + assert.equal( + deepEqual(ownedFiles(owner, filesOwners), { + ownedFiles: [ownedFile], + numberOfApproved: 1, + numberOfPending: 0, + }), + true + ); + }); + + test('ownedFiles - should NOT match file owner over email without domain or full name when account id is different', () => { + const notFileOwner = {...owner, _account_id: 2 as unknown as AccountId}; + assert.equal( + deepEqual(ownedFiles(notFileOwner, filesOwners), emptyOwnerFilesInfo), + true + ); }); }); suite('shouldHide tests', () => { + const current_revision = 'commit-1' as unknown as CommitId; const change = { status: ChangeStatus.NEW, submit_requirements: [ - {name: 'Owner-Approval'}, + {submittability_expression_result: {expression: 'has:approval_owners'}}, ] as unknown as SubmitRequirementResultInfo[], + current_revision, } as unknown as ChangeInfo; - const revisionInfo = {_number: 1} as unknown as RevisionInfo; - const allFilesApproved = true; + const revisionInfo = { + _number: 1, + commit: {commit: current_revision}, + } as unknown as RevisionInfo; const user = {account: account(1), role: UserRole.OTHER}; const ownedFiles = ['README.md']; test('shouldHide - should be `true` when change is `undefined`', () => { const undefinedChange = undefined; assert.equal( - shouldHide( - undefinedChange, - revisionInfo, - !allFilesApproved, - user, - ownedFiles - ), + shouldHide(undefinedChange, revisionInfo, user, ownedFiles), true ); }); @@ -93,13 +158,7 @@ status: getRandom(ChangeStatus.ABANDONED, ChangeStatus.MERGED), }; assert.equal( - shouldHide( - abandonedOrMergedChange, - revisionInfo, - !allFilesApproved, - user, - ownedFiles - ), + shouldHide(abandonedOrMergedChange, revisionInfo, user, ownedFiles), true ); }); @@ -109,13 +168,18 @@ _number: EDIT, } as unknown as RevisionInfo); assert.equal( - shouldHide( - change, - undefinedOrEditRevisionInfo, - !allFilesApproved, - user, - ownedFiles - ), + shouldHide(change, undefinedOrEditRevisionInfo, user, ownedFiles), + true + ); + }); + + test('shouldHide - should be `true` when NOT current_revision is viewed', () => { + const notCurrentRevisionInfo = { + _number: 0, + commit: {commit: 'not-current' as unknown as CommitId}, + } as unknown as RevisionInfo; + assert.equal( + shouldHide(change, notCurrentRevisionInfo, user, ownedFiles), true ); }); @@ -124,14 +188,17 @@ const changeWithOtherSubmitRequirements = { ...change, submit_requirements: [ - {name: 'Other'}, + { + submittability_expression_result: { + expression: 'has:other_predicate', + }, + }, ] as unknown as SubmitRequirementResultInfo[], }; assert.equal( shouldHide( changeWithOtherSubmitRequirements, revisionInfo, - !allFilesApproved, user, ownedFiles ), @@ -139,34 +206,18 @@ ); }); - test('shouldHide - should be `true` when all files are approved', () => { - assert.equal( - shouldHide(change, revisionInfo, allFilesApproved, user, ownedFiles), - true - ); - }); - test('shouldHide - should be `true` when user is `undefined` or `ANONYMOUS`', () => { const undefinedOrAnonymousUser = getRandom(undefined, { role: UserRole.ANONYMOUS, } as unknown as User); assert.equal( - shouldHide( - change, - revisionInfo, - !allFilesApproved, - undefinedOrAnonymousUser, - ownedFiles - ), + shouldHide(change, revisionInfo, undefinedOrAnonymousUser, ownedFiles), true ); }); test('shouldHide - should be `false` when user owns files', () => { - assert.equal( - shouldHide(change, revisionInfo, !allFilesApproved, user, ownedFiles), - false - ); + assert.equal(shouldHide(change, revisionInfo, user, ownedFiles), false); }); }); }); @@ -174,12 +225,18 @@ function account(id: number): AccountInfo { return { _account_id: id, + email: `${id}_email@example.com`, + name: `${id}_name`, } as unknown as AccountInfo; } function fileOwner(id: number): Owner { return { id, - name: `name for account: ${id}`, + name: `${id}_name`, } as unknown as Owner; } + +function fileOwnerWithNameOnly(name: string): GroupOwner { + return {name} as unknown as GroupOwner; +}
diff --git a/owners/web/owners-mixin.ts b/owners/web/owners-mixin.ts index 0bc813a..cd9217a 100644 --- a/owners/web/owners-mixin.ts +++ b/owners/web/owners-mixin.ts
@@ -32,7 +32,6 @@ patchRange?: PatchRange; restApi?: RestPluginApi; user?: User; - allFilesApproved?: boolean; filesOwners?: FilesOwners; onModelUpdate(): void; @@ -55,9 +54,6 @@ user?: User; @state() - allFilesApproved?: boolean; - - @state() filesOwners?: FilesOwners; private _model?: OwnersModel; @@ -87,12 +83,6 @@ this.subscriptions.push( model.state$.subscribe(s => { - this.allFilesApproved = s.allFilesApproved; - }) - ); - - this.subscriptions.push( - model.state$.subscribe(s => { this.filesOwners = s.filesOwners; }) ); @@ -121,7 +111,6 @@ protected onModelUpdate() { this.modelLoader?.loadUser(); - this.modelLoader?.loadAllFilesApproved(); this.modelLoader?.loadFilesOwners(); }
diff --git a/owners/web/owners-model.ts b/owners/web/owners-model.ts index 4d32095..48e7e08 100644 --- a/owners/web/owners-model.ts +++ b/owners/web/owners-model.ts
@@ -44,7 +44,6 @@ export interface OwnersState { user?: User; - allFilesApproved?: boolean; filesOwners?: FilesOwners; } @@ -91,12 +90,6 @@ this.setState({...current, user}); } - setAllFilesApproved(allFilesApproved: boolean | undefined) { - const current = this.subject$.getValue(); - if (current.allFilesApproved === allFilesApproved) return; - this.setState({...current, allFilesApproved}); - } - setFilesOwners(filesOwners: FilesOwners | undefined) { const current = this.subject$.getValue(); if (deepEqual(current.filesOwners, filesOwners)) return; @@ -125,14 +118,6 @@ ); } - async loadAllFilesApproved() { - await this._loadProperty( - 'allFilesApproved', - () => this.service.getAllFilesApproved(), - value => this.model.setAllFilesApproved(value) - ); - } - async loadFilesOwners() { await this._loadProperty( 'filesOwners',
diff --git a/owners/web/owners-service.ts b/owners/web/owners-service.ts index 2e80458..0a57362 100644 --- a/owners/web/owners-service.ts +++ b/owners/web/owners-service.ts
@@ -19,10 +19,8 @@ import { AccountDetailInfo, ChangeInfo, - ChangeStatus, NumericChangeId, RepoName, - SubmitRequirementStatus, } from '@gerritcodereview/typescript-api/rest-api'; import {User, UserRole} from './owners-model'; @@ -57,7 +55,21 @@ owners_labels: OwnersLabels; } -export const OWNERS_SUBMIT_REQUIREMENT = 'Owner-Approval'; +const OWNERS_SUBMIT_REQUIREMENT = 'has:approval_owners'; +const OWNERS_SUBMIT_RULE = 'owners~OwnersSubmitRequirement'; + +export function hasOwnersSubmitRequirement(change: ChangeInfo): boolean { + return ( + change.submit_requirements !== undefined && + change.submit_requirements.find(r => { + const expression = r.submittability_expression_result.expression; + return ( + expression.includes(OWNERS_SUBMIT_REQUIREMENT) || + expression.includes(OWNERS_SUBMIT_RULE) + ); + }) !== undefined + ); +} class ResponseError extends Error { constructor(readonly response: Response) { @@ -181,39 +193,10 @@ return this.apiCache.getLoggedInUser(); } - async getAllFilesApproved(): Promise<boolean | undefined> { - if (!(await this.isLoggedIn())) { - return Promise.resolve(undefined); - } - - if ( - this.change?.status === ChangeStatus.ABANDONED || - this.change?.status === ChangeStatus.MERGED - ) { - return Promise.resolve(undefined); - } - - const ownersSr = this.change?.submit_requirements?.find( - r => r.name === OWNERS_SUBMIT_REQUIREMENT - ); - if (!ownersSr) { - return Promise.resolve(undefined); - } - - return Promise.resolve( - ownersSr.status === SubmitRequirementStatus.SATISFIED - ); - } - getFilesOwners(): Promise<FilesOwners | undefined> { return this.apiCache.getFilesOwners(); } - private async isLoggedIn(): Promise<boolean> { - const user = await this.getLoggedInUser(); - return user && user.role !== UserRole.ANONYMOUS; - } - static getOwnersService(restApi: RestPluginApi, change: ChangeInfo) { if (!service || service.change !== change) { service = new OwnersService(restApi, change);
diff --git a/owners/web/owners-service_test.ts b/owners/web/owners-service_test.ts index f98fddd..fbb9dc7 100644 --- a/owners/web/owners-service_test.ts +++ b/owners/web/owners-service_test.ts
@@ -15,7 +15,11 @@ * limitations under the License. */ -import {FilesOwners, OwnersService} from './owners-service'; +import { + FilesOwners, + hasOwnersSubmitRequirement, + OwnersService, +} from './owners-service'; import { RequestPayload, RestPluginApi, @@ -25,7 +29,6 @@ ChangeInfo, ChangeStatus, HttpMethod, - SubmitRequirementResultInfo, } from '@gerritcodereview/typescript-api/rest-api'; import {assert} from '@open-wc/testing'; import {UserRole} from './owners-model'; @@ -149,8 +152,6 @@ } as unknown as RestPluginApi; } - const notLoggedInRestApiService = setupRestApiForLoggedIn(false); - function loggedInRestApiService(acc: number): RestPluginApi { return { ...setupRestApiForLoggedIn(true), @@ -163,106 +164,29 @@ let service: OwnersService; let getApiStub: sinon.SinonStub; - function setup( - loggedIn: boolean, - changeStats: ChangeStatus = ChangeStatus.NEW, - submitRequirementsSatisfied?: boolean, - response = {} - ) { + function setup(response = {}) { const acc = account(1); const base_change = { ...fakeChange, _number: 1, - status: changeStats, + status: ChangeStatus.NEW, project: 'test_repo', owner: acc, } as unknown as ChangeInfo; - const change = - submitRequirementsSatisfied === undefined - ? base_change - : { - ...base_change, - submit_requirements: [ - { - name: 'Owner-Approval', - status: submitRequirementsSatisfied - ? 'SATISFIED' - : 'UNSATISFIED', - } as unknown as SubmitRequirementResultInfo, - ], - }; - const restApi = loggedIn - ? loggedInRestApiService(1) - : notLoggedInRestApiService; + const restApi = loggedInRestApiService(1); getApiStub = sinon.stub(restApi, 'send'); getApiStub .withArgs( sinon.match.any, - `/changes/${change.project}~${change._number}/revisions/current/owners~files-owners`, + `/changes/${base_change.project}~${base_change._number}/revisions/current/owners~files-owners`, sinon.match.any, sinon.match.any ) .returns(Promise.resolve(response)); - service = OwnersService.getOwnersService(restApi, change); + service = OwnersService.getOwnersService(restApi, base_change); } - const isLoggedIn = true; - const changeMerged = ChangeStatus.MERGED; - const changeNew = ChangeStatus.NEW; - const ownersSubmitRequirementsSatisfied = true; - - function setupGetAllFilesApproved_undefined() { - setup(!isLoggedIn); - } - - test('should have getAllFilesApproved `undefined` for no change owner', async () => { - setupGetAllFilesApproved_undefined(); - - const response = await service.getAllFilesApproved(); - assert.equal(response, undefined); - }); - - test('should have getAllFilesApproved `undefined` for `MERGED` change', async () => { - setup(isLoggedIn, changeMerged); - - const response = await service.getAllFilesApproved(); - assert.equal(response, undefined); - }); - - test('should have getAllFilesApproved `undefined` when no submit requirements', async () => { - setup(isLoggedIn, changeNew); - - const response = await service.getAllFilesApproved(); - assert.equal(response, undefined); - }); - - function setupGetAllFilesApproved_false(response = {}) { - setup( - isLoggedIn, - changeNew, - !ownersSubmitRequirementsSatisfied, - response - ); - } - - test('should have getAllFilesApproved `false` when submit requirements are not satisfied', async () => { - setupGetAllFilesApproved_false(); - - const response = await service.getAllFilesApproved(); - assert.equal(response, false); - }); - - function setupGetAllFilesApproved_true() { - setup(isLoggedIn, changeNew, ownersSubmitRequirementsSatisfied); - } - test('should have getAllFilesApproved `true` when submit requirements are satisfied', async () => { - setupGetAllFilesApproved_true(); - - const response = await service.getAllFilesApproved(); - assert.equal(response, true); - }); - test('should call getFilesOwners', async () => { const expected = { files: { @@ -283,7 +207,7 @@ }, }, }; - setupGetAllFilesApproved_false(expected); + setup(expected); const response = await service.getFilesOwners(); await flush(); @@ -295,7 +219,7 @@ }); test('should fetch response from plugin only once', async () => { - setupGetAllFilesApproved_true(); + setup(); await service.getFilesOwners(); await flush(); @@ -306,6 +230,59 @@ assert.equal(getApiStub.callCount, 1); }); }); + + suite('hasOwnersSubmitRequirement tests', () => { + test('hasOwnersSubmitRequirement - should be `false` when change has no owners submit requirement', () => { + const noSubmitRequirementOrRecordChange = {} as unknown as ChangeInfo; + assert.equal( + hasOwnersSubmitRequirement(noSubmitRequirementOrRecordChange), + false + ); + }); + + test('hasOwnersSubmitRequirement - should be `false` when change has different submit requirements', () => { + const differentSubmitRequirementAndRecord = { + submit_requirements: [ + { + submittability_expression_result: { + expression: 'has:other_predicated', + }, + }, + ], + } as unknown as ChangeInfo; + assert.equal( + hasOwnersSubmitRequirement(differentSubmitRequirementAndRecord), + false + ); + }); + + test('hasOwnersSubmitRequirement - should be `true` when change has owners submit requirement', () => { + const ownersSubmitRequirement = { + submit_requirements: [ + { + submittability_expression_result: { + expression: 'has:approval_owners', + }, + }, + ], + } as unknown as ChangeInfo; + assert.equal(hasOwnersSubmitRequirement(ownersSubmitRequirement), true); + }); + + test('hasOwnersSubmitRequirement - should be `true` when change has owners submit rule', () => { + const ownersSubmitRule = { + submit_requirements: [ + { + submittability_expression_result: { + expression: + 'label:Code-Review from owners\u003downers~OwnersSubmitRequirement', + }, + }, + ], + } as unknown as ChangeInfo; + assert.equal(hasOwnersSubmitRequirement(ownersSubmitRule), true); + }); + }); }); function account(id: number): AccountInfo {