diff --git a/models/issues/review.go b/models/issues/review.go index 58ad532da78..f2f73daec8c 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -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" } diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index 9253686eb3a..76382f18aed 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -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) diff --git a/services/issue/pull.go b/services/issue/pull.go index a7d3ca1b22a..bb877736740 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -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), diff --git a/services/issue/review_request.go b/services/issue/review_request.go index d2826f85d65..fe9fdb7118d 100644 --- a/services/issue/review_request.go +++ b/services/issue/review_request.go @@ -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, diff --git a/services/issue/review_request_test.go b/services/issue/review_request_test.go new file mode 100644 index 00000000000..e01385948d0 --- /dev/null +++ b/services/issue/review_request_test.go @@ -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) + }) + } +} diff --git a/templates/repo/issue/sidebar/reviewer_list.tmpl b/templates/repo/issue/sidebar/reviewer_list.tmpl index 99e033bcfde..b78d9561d81 100644 --- a/templates/repo/issue/sidebar/reviewer_list.tmpl +++ b/templates/repo/issue/sidebar/reviewer_list.tmpl @@ -66,7 +66,7 @@ {{if .Review.Stale}} {{svg "octicon-hourglass" 16}} {{end}} - {{if and .CanChange $data.CanChooseReviewer}} + {{if .CanChange}} {{if .Requested}} { 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); }); diff --git a/tests/e2e/utils.ts b/tests/e2e/utils.ts index e230acc3ce0..16079f66df6 100644 --- a/tests/e2e/utils.ts +++ b/tests/e2e/utils.ts @@ -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}>, label: string) { +async function apiRetry(fn: () => Promise, label: string): Promise { 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}) { @@ -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} = {}) { + 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} = {}) { 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} = {}): Promise { - 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} = {}) { - 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) { diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 15157c1562f..4d905b3f0c0 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -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))