public/gitea
1
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-09-09 22:21:42 +09:00
Code Issues Packages Projects Releases Wiki Activity

fix: allow re-requesting uncounted review approvals (#38988)

Browse Source
This commit is contained in:
Harsh Sharma
2026-09-08 21:29:06 +05:30
committed by GitHub
parent c9193adb68
commit d93bd06d0c
9 changed files with 160 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
models/issues/review.go
+1 -1
View File
@@ -223,7 +223,7 @@ func (r *Review) HTMLTypeColorClass() string {
case ReviewTypeReject:
return "tw-text-red"
case ReviewTypeRequest:
return "tw-text-yellow"
return util.Iif(r.Official, "tw-text-yellow", "tw-text-text-light")
}
return "tw-text-text-light"
}
routers/web/repo/issue_page_meta.go
+3 -7
View File
@@ -351,13 +351,9 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) {
tmp.ItemID = -review.ReviewerTeamID
}
if data.CanChooseReviewer {
// Users who can choose reviewers can also remove review requests
tmp.CanChange = true
} else if ctx.Doer != nil && ctx.Doer.ID == review.ReviewerID && review.Type == issues_model.ReviewTypeRequest {
// A user can refuse review requests
tmp.CanChange = true
}
tmp.CanChange = data.CanChooseReviewer || (ctx.Doer != nil && ctx.Doer.ID == review.ReviewerID &&
(review.Type == issues_model.ReviewTypeRequest ||
(review.Type == issues_model.ReviewTypeApprove && !isClosed && !repo.IsArchived)))
pullReviews = append(pullReviews, tmp)
services/issue/pull.go
+1 -1
View File
@@ -182,7 +182,7 @@ func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.Protected
// owner lacks write access, or isn't on the approvals whitelist). Unlike the other
// branch-protection checks below, this one only cares whether a listed code owner approved.
approvingReviews, err := issues_model.FindLatestReviews(ctx, issues_model.FindReviewOptions{
Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove, issues_model.ReviewTypeReject},
Types: []issues_model.ReviewType{issues_model.ReviewTypeApprove, issues_model.ReviewTypeReject, issues_model.ReviewTypeRequest},
IssueID: pr.IssueID,
OfficialOnly: false,
Dismissed: optional.Some(false),
services/issue/review_request.go
+4
View File
@@ -103,6 +103,10 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User,
return nil
}
if !issue.Repo.IsArchived && doer.ID == reviewer.ID && lastReview != nil && lastReview.Type == issues_model.ReviewTypeApprove {
return nil
}
return issues_model.ErrNotValidReviewRequest{
Reason: "Doer can't choose reviewer",
UserID: doer.ID,
services/issue/review_request_test.go
+57
View File
@@ -0,0 +1,57 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package issue
import (
"testing"
issues_model "gitea.dev/models/issues"
"gitea.dev/models/unittest"
user_model "gitea.dev/models/user"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
func TestReviewRequestRetractOwnApproval(t *testing.T) {
for _, tc := range []struct {
name string
reviewType issues_model.ReviewType
archived bool
allowed bool
}{
{"approval", issues_model.ReviewTypeApprove, false, true},
{"comment", issues_model.ReviewTypeComment, false, false},
{"archived repository", issues_model.ReviewTypeApprove, true, false},
} {
t.Run(tc.name, func(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
pull := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: 2})
pull.HasMerged = false
require.NoError(t, pull.UpdateCols(t.Context(), "has_merged"))
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
require.NoError(t, issue.LoadRepo(t.Context()))
require.NoError(t, issue.Repo.LoadOwner(t.Context()))
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
review, _, err := issues_model.SubmitReview(t.Context(), reviewer, issue, tc.reviewType, "review", "", false, nil)
require.NoError(t, err)
assert.False(t, review.Official)
issue.Repo.IsArchived = tc.archived
_, err = ReviewRequest(t.Context(), issue, reviewer, nil, reviewer, true)
if !tc.allowed {
assert.True(t, issues_model.IsErrNotValidReviewRequest(err))
return
}
require.NoError(t, err)
review, err = issues_model.GetReviewByIssueIDAndUserID(t.Context(), issue.ID, reviewer.ID)
require.NoError(t, err)
assert.Equal(t, issues_model.ReviewTypeRequest, review.Type)
assert.False(t, review.Official)
})
}
}
templates/repo/issue/sidebar/reviewer_list.tmpl
+1 -1
View File
@@ -66,7 +66,7 @@
{{if .Review.Stale}}
<span data-tooltip-content="{{ctx.Locale.Tr "repo.issues.is_stale"}}">{{svg "octicon-hourglass" 16}}</span>
{{end}}
{{if and .CanChange $data.CanChooseReviewer}}
{{if .CanChange}}
{{if .Requested}}
<a href="#" class="ui muted icon link-action"
data-tooltip-content="{{ctx.Locale.Tr "repo.issues.remove_request_review"}}"
tests/e2e/pr-review.test.ts
+70 -9
View File
@@ -1,15 +1,68 @@
import {test, expect} from '@playwright/test';
import {apiCreateFiles, apiCreatePR, apiCreateRepo, apiCreateReview, apiCreateUser, apiUserHeaders, loginUser, randomString} from './utils.ts';
import {
apiAddCollaborator,
apiCreateFiles,
apiCreatePR,
apiCreateRepo,
apiCreateReview,
apiCreateUser,
apiUserHeaders,
loginUser,
randomString,
} from './utils.ts';
test('pr review flow', async ({page, request}) => {
const poster = `rv-poster-${randomString(8)}`;
const reviewer = `rv-reviewer-${randomString(8)}`;
await Promise.all([apiCreateUser(request, poster), apiCreateUser(request, reviewer)]);
const officialReviewer = `rv-official-${randomString(8)}`;
await Promise.all([
apiCreateUser(request, poster),
apiCreateUser(request, reviewer),
apiCreateUser(request, officialReviewer),
]);
const posterHeaders = apiUserHeaders(poster);
const repoName = `e2e-prreview-${randomString(8)}`;
await apiCreateRepo(request, {name: repoName, headers: posterHeaders});
await apiCreateFiles(request, poster, repoName, [{path: 'added.txt', content: 'new content\n'}], {branch: 'main', newBranch: 'feat', headers: posterHeaders});
await Promise.all([
apiAddCollaborator(
request,
poster,
repoName,
officialReviewer,
{headers: posterHeaders},
),
apiCreateFiles(
request,
poster,
repoName,
[{path: 'added.txt', content: 'new content\n'}],
{branch: 'main', newBranch: 'feat', headers: posterHeaders},
),
]);
const prIndex = await apiCreatePR(request, poster, repoName, 'feat', 'main', 'review test', {headers: posterHeaders});
const pullUrl = `/${poster}/${repoName}/pulls/${prIndex}`;
const reRequestAndApprove = async (
username: string,
approvalTooltip: string,
pendingStateClass: string,
official: boolean,
) => {
await page.goto(pullUrl);
const reviewerItem = page.locator('.issue-sidebar-combo .ui.relaxed.list > .item', {
has: page.locator(`a[href="/${username}"]`),
});
const reviewState = reviewerItem.locator('span[data-tooltip-content]');
await expect(reviewState).toHaveAttribute('data-tooltip-content', approvalTooltip);
await reviewerItem.locator('a[data-tooltip-content="Re-request review"]').click();
await expect(reviewState).toHaveAttribute('data-tooltip-content', 'Review pending');
await expect(reviewState.locator(`.octicon-dot-fill.${pendingStateClass}`)).toBeVisible();
expect(await apiCreateReview(request, poster, repoName, prIndex, {
event: 'APPROVED',
headers: apiUserHeaders(username),
})).toMatchObject({state: 'APPROVED', official});
};
// reviewer seeds an inline comment via API so the poster's UI reply exercises the reply-to-review path (#35994)
await Promise.all([
@@ -17,10 +70,14 @@ test('pr review flow', async ({page, request}) => {
comments: [{path: 'added.txt', body: 'inline to reply to', new_position: 1}],
headers: apiUserHeaders(reviewer),
}),
apiCreateReview(request, poster, repoName, prIndex, {
event: 'APPROVED',
headers: apiUserHeaders(officialReviewer),
}),
loginUser(page, poster),
]);
await page.goto(`/${poster}/${repoName}/pulls/${prIndex}/files`);
await page.goto(`${pullUrl}/files`);
// diff viewer renders the added file with its header and one added-line row
const fileBox = page.locator('.diff-file-box[data-new-filename="added.txt"]');
@@ -40,14 +97,18 @@ test('pr review flow', async ({page, request}) => {
await replyForm.getByRole('button', {name: 'Reply', exact: true}).click();
await expect(conversation.locator('.comment-body')).toContainText(['inline to reply to', 'my reply body']);
// switch to reviewer and submit an approve review
await page.context().clearCookies();
await loginUser(page, reviewer);
await page.goto(`/${poster}/${repoName}/pulls/${prIndex}/files`);
await page.goto(`${pullUrl}/files`);
await page.locator('#review-box .js-btn-review').click();
const panel = page.locator('.review-box-panel');
await panel.locator('textarea[name="content"]').fill('LGTM');
await panel.locator('textarea[name="content"]').fill(`First approval from ${reviewer}`);
await panel.getByRole('button', {name: 'Approve', exact: true}).click();
await expect(page.locator('.timeline-item .octicon-check').first()).toBeVisible();
await expect(page.locator('.timeline-item').filter({hasText: 'LGTM'})).toBeVisible();
await expect(page.locator('.timeline-item').filter({hasText: `First approval from ${reviewer}`})).toBeVisible();
await reRequestAndApprove(reviewer, 'Uncounted approval', 'tw-text-text-light', false);
await page.context().clearCookies();
await loginUser(page, officialReviewer);
await reRequestAndApprove(officialReviewer, 'Approved', 'tw-text-yellow', true);
});
tests/e2e/utils.ts
+18 -14
View File
@@ -1,6 +1,6 @@
import {env} from 'node:process';
import {expect} from '@playwright/test';
import type {APIRequestContext, Locator, Page} from '@playwright/test';
import type {APIRequestContext, APIResponse, Locator, Page} from '@playwright/test';
/** Generate a random alphanumeric string. */
export function randomString(length: number): string {
@@ -24,11 +24,11 @@ export function apiHeaders() {
return apiAuthHeader(env.GITEA_TEST_E2E_USER, env.GITEA_TEST_E2E_PASSWORD);
}
async function apiRetry(fn: () => Promise<{ok: () => boolean; status: () => number; text: () => Promise<string>}>, label: string) {
async function apiRetry(fn: () => Promise<APIResponse>, label: string): Promise<APIResponse> {
const maxAttempts = 5;
for (let attempt = 0; attempt < maxAttempts; attempt++) {
const response = await fn();
if (response.ok()) return;
if (response.ok()) return response;
if ([500, 502, 503].includes(response.status()) && attempt < maxAttempts - 1) {
const jitter = Math.random() * 500;
await new Promise((resolve) => setTimeout(resolve, 1000 * (attempt + 1) + jitter));
@@ -36,6 +36,7 @@ async function apiRetry(fn: () => Promise<{ok: () => boolean; status: () => numb
}
throw new Error(`${label} failed: ${response.status()} ${await response.text()}`);
}
throw new Error(`${label} failed after ${maxAttempts} attempts`);
}
export async function apiCreateRepo(requestContext: APIRequestContext, {name, autoInit = true, headers}: {name: string; autoInit?: boolean; headers?: Record<string, string>}) {
@@ -45,6 +46,13 @@ export async function apiCreateRepo(requestContext: APIRequestContext, {name, au
}), 'apiCreateRepo');
}
export async function apiAddCollaborator(requestContext: APIRequestContext, owner: string, repo: string, collaborator: string, {headers}: {headers?: Record<string, string>} = {}) {
await apiRetry(() => requestContext.put(`${baseUrl()}/api/v1/repos/${owner}/${repo}/collaborators/${collaborator}`, {
headers: headers || apiHeaders(),
data: {permission: 'write'},
}), 'apiAddCollaborator');
}
export async function apiCreateOrg(requestContext: APIRequestContext, name: string, {headers}: {headers?: Record<string, string>} = {}) {
await apiRetry(() => requestContext.post(`${baseUrl()}/api/v1/orgs`, {
headers: headers || apiHeaders(),
@@ -91,24 +99,20 @@ export async function apiCloseIssue(requestContext: APIRequestContext, owner: st
/** Create a PR via API. Returns the PR index for subsequent operations. */
export async function apiCreatePR(requestContext: APIRequestContext, owner: string, repo: string, head: string, base: string, title: string, {headers}: {headers?: Record<string, string>} = {}): Promise<number> {
let prIndex = 0;
await apiRetry(async () => {
const response = await requestContext.post(`${baseUrl()}/api/v1/repos/${owner}/${repo}/pulls`, {
headers: headers || apiHeaders(),
data: {head, base, title},
});
if (response.ok()) prIndex = (await response.json()).number;
return response;
}, 'apiCreatePR');
return prIndex;
const response = await apiRetry(() => requestContext.post(`${baseUrl()}/api/v1/repos/${owner}/${repo}/pulls`, {
headers: headers || apiHeaders(),
data: {head, base, title},
}), 'apiCreatePR');
return (await response.json()).number;
}
/** Create a review on a PR. `event: "COMMENT"` submits immediately without a pending review. */
export async function apiCreateReview(requestContext: APIRequestContext, owner: string, repo: string, index: number, {event = 'COMMENT', body, comments = [], headers}: {event?: string; body?: string; comments?: Array<{path: string; body: string; new_position?: number; old_position?: number}>; headers?: Record<string, string>} = {}) {
await apiRetry(() => requestContext.post(`${baseUrl()}/api/v1/repos/${owner}/${repo}/pulls/${index}/reviews`, {
const response = await apiRetry(() => requestContext.post(`${baseUrl()}/api/v1/repos/${owner}/${repo}/pulls/${index}/reviews`, {
headers: headers || apiHeaders(),
data: {event, body, comments},
}), 'apiCreateReview');
return response.json();
}
export async function createProjectColumn(requestContext: APIRequestContext, owner: string, repo: string, projectID: string, title: string) {
tests/integration/pull_review_test.go
+5
View File
@@ -161,6 +161,11 @@ func TestPullView_CodeOwner(t *testing.T) {
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.True(t, hasCodeownerReviews)
_, err = issue_service.ReviewRequest(t.Context(), pr.Issue, user5, nil, user5, true)
assert.NoError(t, err)
hasCodeownerReviews = issue_service.HasAllRequiredCodeownerReviews(t.Context(), &protectBranch, pr)
assert.False(t, hasCodeownerReviews)
// a later requested-changes review from a required code owner must override
// that user's earlier approval and no longer satisfy the requirement
_, _, err = issues_model.SubmitReview(t.Context(), user5, pr.Issue, issues_model.ReviewTypeReject, "Needs changes", resp.Commit.SHA, false, make([]string, 0))