fix(gerrit): not auto-approving if change already had a Code-Review +1

This commit is contained in:
Felipe Santos 2024-11-29 19:42:54 -03:00
parent 5459f59030
commit 46f8eed01f
5 changed files with 103 additions and 28 deletions

View file

@ -393,10 +393,20 @@ describe('modules/platform/gerrit/client', () => {
describe('approveChange()', () => {
it('already approved - do nothing', async () => {
const change = partial<GerritChange>({});
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 2 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
@ -405,17 +415,53 @@ describe('modules/platform/gerrit/client', () => {
const change = partial<GerritChange>({ labels: {} });
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
});
it('not already approved - approve now', async () => {
const change = partial<GerritChange>({ labels: { 'Code-Review': {} } });
const change = partial<GerritChange>({
labels: { 'Code-Review': { all: [] } },
});
httpMock
.scope(gerritEndpointUrl)
.get((url) => url.includes('/a/changes/123456?o='))
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
.post('/a/changes/123456/revisions/current/review', {
labels: { 'Code-Review': +2 },
})
.reply(200, gerritRestResponse(''), jsonResultHeader);
await expect(client.approveChange(123456)).toResolve();
expect(approveMock.isDone()).toBeTrue();
});
it('not already approved because of +1 - approve now', async () => {
const change = partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 1 }],
},
},
});
httpMock
.scope(gerritEndpointUrl)
.get(
(url) =>
url.includes('/a/changes/123456?o=') &&
url.includes('o=DETAILED_LABELS'),
)
.reply(200, gerritRestResponse(change), jsonResultHeader);
const approveMock = httpMock
.scope(gerritEndpointUrl)
@ -432,7 +478,7 @@ describe('modules/platform/gerrit/client', () => {
it('label not exists', () => {
expect(
client.wasApprovedBy(partial<GerritChange>({}), 'user'),
).toBeUndefined();
).toBeFalse();
});
it('not approved by anyone', () => {
@ -440,12 +486,29 @@ describe('modules/platform/gerrit/client', () => {
client.wasApprovedBy(
partial<GerritChange>({
labels: {
'Code-Review': {},
'Code-Review': {
all: [],
},
},
}),
'user',
),
).toBeUndefined();
).toBeFalse();
});
it('not approved but with +1', () => {
expect(
client.wasApprovedBy(
partial<GerritChange>({
labels: {
'Code-Review': {
all: [{ value: 1, username: 'user' }],
},
},
}),
'user',
),
).toBeFalse();
});
it('approved by given user', () => {
@ -454,10 +517,7 @@ describe('modules/platform/gerrit/client', () => {
partial<GerritChange>({
labels: {
'Code-Review': {
approved: {
_account_id: 1,
username: 'user',
},
all: [{ value: 2, username: 'user' }],
},
},
}),
@ -472,10 +532,7 @@ describe('modules/platform/gerrit/client', () => {
partial<GerritChange>({
labels: {
'Code-Review': {
approved: {
_account_id: 1,
username: 'other',
},
all: [{ value: 2, username: 'other' }],
},
},
}),

View file

@ -58,12 +58,14 @@ class GerritClient {
repository: string,
findPRConfig: GerritFindPRConfig,
refreshCache?: boolean,
extraOptions?: string[],
): Promise<GerritChange[]> {
const filters = GerritClient.buildSearchFilters(repository, findPRConfig);
const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange[]>(
`a/changes/?q=` +
filters.join('+') +
this.requestDetails.map((det) => `&o=${det}`).join(''),
options.map((det) => `&o=${det}`).join(''),
{ memCache: !refreshCache },
);
logger.trace(
@ -72,10 +74,13 @@ class GerritClient {
return changes.body;
}
async getChange(changeNumber: number): Promise<GerritChange> {
async getChange(
changeNumber: number,
extraOptions?: string[],
): Promise<GerritChange> {
const options = [...this.requestDetails, ...(extraOptions ?? [])];
const changes = await this.gerritHttp.getJson<GerritChange>(
`a/changes/${changeNumber}?` +
this.requestDetails.map((det) => `o=${det}`).join('&'),
`a/changes/${changeNumber}?` + options.map((det) => `o=${det}`).join('&'),
);
return changes.body;
}
@ -189,15 +194,20 @@ class GerritClient {
}
async checkIfApproved(changeId: number): Promise<boolean> {
const change = await client.getChange(changeId);
const reviewLabels = change?.labels?.['Code-Review'];
return reviewLabels === undefined || reviewLabels.approved !== undefined;
const change = await client.getChange(changeId, ['DETAILED_LABELS']);
const reviewLabel = change?.labels?.['Code-Review'];
return (
reviewLabel === undefined ||
Boolean(reviewLabel.all?.some((label) => label.value === 2))
);
}
wasApprovedBy(change: GerritChange, username: string): boolean | undefined {
return (
change.labels?.['Code-Review'].approved &&
change.labels['Code-Review'].approved.username === username
wasApprovedBy(change: GerritChange, username: string): boolean {
const reviewLabel = change?.labels?.['Code-Review'];
return Boolean(
reviewLabel?.all?.some(
(label) => label.value === 2 && label.username === username,
),
);
}

View file

@ -277,6 +277,7 @@ describe('modules/platform/gerrit/scm', () => {
targetBranch: 'main',
},
true,
['DETAILED_LABELS'],
);
});

View file

@ -101,7 +101,7 @@ export class GerritScm extends DefaultGitScm {
targetBranch: commit.baseBranch,
};
const existingChange = await client
.findChanges(repository, searchConfig, true)
.findChanges(repository, searchConfig, true, ['DETAILED_LABELS'])
.then((res) => res.pop());
let hasChanges = true;

View file

@ -77,6 +77,8 @@ export interface GerritChangeMessageInfo {
export interface GerritLabelInfo {
approved?: GerritAccountInfo;
rejected?: GerritAccountInfo;
/** List of votes. Only set when o=DETAILED_LABELS. */
all?: GerritApprovalInfo[];
}
export interface GerritActionInfo {
@ -99,3 +101,8 @@ export interface GerritMergeableInfo {
| 'CHERRY_PICK';
mergeable: boolean;
}
export interface GerritApprovalInfo {
value?: number;
username?: string;
}