Add `getAllFilesApproved` and `getFileOwners` functions to service
Introduce `getAllFilesApproved` function to determine if file owners
should be obtained from plugin and displayed to the user. It returns:
* `undefined` for all cases when file owners are not relevant to the
change in question: change is not viewed by logged user, change is
abandoned or merged and finally change has no `Owner-Approval`
submit requirement
* `true` when `Owner-Approval` is `SATISFIED` and `false` otherwise
Introduce the `getFileOwners` function to the `OwnersService` that
returns:
* `undefined` when `getAllFilesApproved` returns `undefined` or `true`
as there is no point for calling plugin REST api for not relevant or
already approved change
* files owners from plugin REST API
New functions are covered with unit tests (add `deepEqual` function to
compare objects and their content - it is copied from Gerrit utils).
Bug: Issue 373151160
Change-Id: Ie4cef9addc2af6963aea9000f195d19665e35dfa
diff --git a/owners/web/gr-owners.ts b/owners/web/gr-owners.ts
index b29fd27..76b89d9 100644
--- a/owners/web/gr-owners.ts
+++ b/owners/web/gr-owners.ts
@@ -22,7 +22,7 @@
ChangeInfo,
ChangeStatus,
} from '@gerritcodereview/typescript-api/rest-api';
-import {OwnersService} from './owners-service';
+import {OWNERS_SUBMIT_REQUIREMENT, OwnersService} from './owners-service';
import {RestPluginApi} from '@gerritcodereview/typescript-api/rest';
import {ModelLoader, OwnersModel, PatchRange, UserRole} from './owners-model';
@@ -187,7 +187,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 === 'Owner-Approval')
+ change.submit_requirements.find(r => r.name === OWNERS_SUBMIT_REQUIREMENT)
) {
return !userRole || userRole === UserRole.ANONYMOUS;
}
diff --git a/owners/web/owners-service.ts b/owners/web/owners-service.ts
index 6abf6e9..91107bf 100644
--- a/owners/web/owners-service.ts
+++ b/owners/web/owners-service.ts
@@ -15,13 +15,55 @@
* limitations under the License.
*/
-import {RestPluginApi} from '@gerritcodereview/typescript-api/rest';
+import {HttpMethod, RestPluginApi} from '@gerritcodereview/typescript-api/rest';
import {
AccountDetailInfo,
ChangeInfo,
+ ChangeStatus,
+ NumericChangeId,
+ RepoName,
+ SubmitRequirementStatus,
} from '@gerritcodereview/typescript-api/rest-api';
import {UserRole} from './owners-model';
+export interface GroupOwner {
+ name: string;
+}
+
+export interface Owner extends GroupOwner {
+ id: number;
+}
+
+export type FileOwner = Owner | GroupOwner;
+
+export interface OwnedFiles {
+ [fileName: string]: FileOwner[];
+}
+
+export interface OwnersLabels {
+ [id: string]: {
+ [label: string]: number;
+ };
+}
+
+export interface FilesOwners {
+ files: OwnedFiles;
+ owners_labels: OwnersLabels;
+}
+
+export const OWNERS_SUBMIT_REQUIREMENT = 'Owner-Approval';
+
+class ResponseError extends Error {
+ constructor(readonly response: Response) {
+ super();
+ }
+}
+
+async function getErrorMessage(response: Response) {
+ const text = await response.text();
+ return text ? `${response.status}: ${text}` : `${response.status}`;
+}
+
class OwnersApi {
constructor(readonly restApi: RestPluginApi) {}
@@ -30,6 +72,43 @@
if (!loggedIn) return undefined;
return await this.restApi.getAccount();
}
+
+ /**
+ * Returns the list of owners associated to each file that needs a review,
+ * and, for each owner, its current labels and votes.
+ *
+ * @doc
+ * https://gerrit.googlesource.com/plugins/owners/+/refs/heads/master/owners/src/main/resources/Documentation/rest-api.md
+ */
+ getFilesOwners(
+ repoName: RepoName,
+ changeId: NumericChangeId
+ ): Promise<FilesOwners> {
+ return this.get(
+ `/changes/${encodeURIComponent(
+ repoName
+ )}~${changeId}/revisions/current/owners~files-owners`
+ ) as Promise<FilesOwners>;
+ }
+
+ private async get(url: string): Promise<unknown> {
+ const errFn = (response?: Response | null, error?: Error) => {
+ if (error) throw error;
+ if (response) throw new ResponseError(response);
+ throw new Error('Generic REST API error');
+ };
+ try {
+ return await this.restApi.send(HttpMethod.GET, url, undefined, errFn);
+ } catch (err) {
+ if (err instanceof ResponseError && err.response.status === 409) {
+ getErrorMessage(err.response).then(msg => {
+ // TODO handle 409 in UI - show banner with error
+ console.error(`Plugin configuration errot: ${msg}`);
+ });
+ }
+ throw err;
+ }
+ }
}
let service: OwnersService | undefined;
@@ -52,6 +131,45 @@
return UserRole.OTHER;
}
+ 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
+ );
+ }
+
+ 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);
+ }
+
+ private async isLoggedIn(): Promise<boolean> {
+ const userRole = await this.getLoggedInUserRole();
+ return userRole && userRole !== 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 f593304..3ac8590 100644
--- a/owners/web/owners-service_test.ts
+++ b/owners/web/owners-service_test.ts
@@ -15,11 +15,20 @@
* limitations under the License.
*/
-import {OwnersService} from './owners-service';
-import {RestPluginApi} from '@gerritcodereview/typescript-api/rest';
-import {ChangeInfo} from '@gerritcodereview/typescript-api/rest-api';
+import {FilesOwners, OwnersService} from './owners-service';
+import {
+ RequestPayload,
+ RestPluginApi,
+} from '@gerritcodereview/typescript-api/rest';
+import {
+ ChangeInfo,
+ ChangeStatus,
+ HttpMethod,
+ SubmitRequirementResultInfo,
+} from '@gerritcodereview/typescript-api/rest-api';
import {assert} from '@open-wc/testing';
import {UserRole} from './owners-model';
+import {deepEqual} from './utils';
suite('owners service tests', () => {
const fakeRestApi = {} as unknown as RestPluginApi;
@@ -92,6 +101,192 @@
assert.equal(userRole, UserRole.CHANGE_OWNER);
});
});
+
+ suite('files owners tests', () => {
+ teardown(() => {
+ sinon.restore();
+ });
+
+ function setupRestApiForLoggedIn(loggedIn: boolean): RestPluginApi {
+ return {
+ getLoggedIn() {
+ return Promise.resolve(loggedIn);
+ },
+ send(
+ _method: HttpMethod,
+ _url: string,
+ _payload?: RequestPayload,
+ _errFn?: ErrorCallback,
+ _contentType?: string
+ ) {
+ return Promise.resolve({});
+ },
+ } as unknown as RestPluginApi;
+ }
+
+ const notLoggedInRestApiService = setupRestApiForLoggedIn(false);
+
+ function loggedInRestApiService(acc: number): RestPluginApi {
+ return {
+ ...setupRestApiForLoggedIn(true),
+ getAccount() {
+ return Promise.resolve(account(acc));
+ },
+ } as unknown as RestPluginApi;
+ }
+
+ let service: OwnersService;
+ let getApiStub: sinon.SinonStub;
+
+ function setup(
+ loggedIn: boolean,
+ changeStats: ChangeStatus = ChangeStatus.NEW,
+ submitRequirementsSatisfied?: boolean,
+ response = {}
+ ) {
+ const acc = account(1);
+ const base_change = {
+ ...fakeChange,
+ _number: 1,
+ status: changeStats,
+ 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;
+
+ getApiStub = sinon.stub(restApi, 'send');
+ getApiStub
+ .withArgs(
+ sinon.match.any,
+ `/changes/${change.project}~${change._number}/revisions/current/owners~files-owners`,
+ sinon.match.any,
+ sinon.match.any
+ )
+ .returns(Promise.resolve(response));
+ service = OwnersService.getOwnersService(restApi, 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 no 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 no 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 () => {
+ const expected = {
+ files: {
+ 'AJavaFile.java': [{name: 'Bob', id: 1000001}],
+ 'Aptyhonfileroot.py': [
+ {name: 'John', id: 1000002},
+ {name: 'Bob', id: 1000001},
+ {name: 'Jack', id: 1000003},
+ ],
+ },
+ owners_labels: {
+ '1000002': {
+ Verified: 1,
+ 'Code-Review': 0,
+ },
+ '1000001': {
+ 'Code-Review': 2,
+ },
+ },
+ };
+ setupGetAllFilesApproved_false(expected);
+
+ const response = await service.getFilesOwners();
+ await flush();
+ assert.equal(getApiStub.callCount, 1);
+ assert.equal(
+ deepEqual(response, {...expected} as unknown as FilesOwners),
+ true
+ );
+ });
+ });
});
function account(id: number) {
@@ -99,3 +294,9 @@
_account_id: id,
};
}
+
+function flush() {
+ return new Promise((resolve, _reject) => {
+ setTimeout(resolve, 0);
+ });
+}
diff --git a/owners/web/utils.ts b/owners/web/utils.ts
new file mode 100644
index 0000000..349f5fb
--- /dev/null
+++ b/owners/web/utils.ts
@@ -0,0 +1,56 @@
+/**
+ * @license
+ * Copyright (C) 2024 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export function deepEqual<T>(a: T, b: T): boolean {
+ if (a === b) return true;
+ if (a === undefined || b === undefined) return false;
+ if (a === null || b === null) return false;
+ if (a instanceof Date || b instanceof Date) {
+ if (!(a instanceof Date && b instanceof Date)) return false;
+ return a.getTime() === b.getTime();
+ }
+
+ if (a instanceof Set || b instanceof Set) {
+ if (!(a instanceof Set && b instanceof Set)) return false;
+ if (a.size !== b.size) return false;
+ for (const ai of a) if (!b.has(ai)) return false;
+ return true;
+ }
+ if (a instanceof Map || b instanceof Map) {
+ if (!(a instanceof Map && b instanceof Map)) return false;
+ if (a.size !== b.size) return false;
+ for (const [aKey, aValue] of a.entries()) {
+ if (!b.has(aKey) || !deepEqual(aValue, b.get(aKey))) return false;
+ }
+ return true;
+ }
+
+ if (typeof a === 'object') {
+ if (typeof b !== 'object') return false;
+ const aObj = a as Record<string, unknown>;
+ const bObj = b as Record<string, unknown>;
+ const aKeys = Object.keys(aObj);
+ const bKeys = Object.keys(bObj);
+ if (aKeys.length !== bKeys.length) return false;
+ for (const key of aKeys) {
+ if (!deepEqual(aObj[key], bObj[key])) return false;
+ }
+ return true;
+ }
+
+ return false;
+}
