diff --git a/models/fixtures/TestAddTeamReviewRequest/issue.yml b/models/fixtures/TestAddTeamReviewRequest/issue.yml new file mode 100644 index 0000000000..a1bcf2921f --- /dev/null +++ b/models/fixtures/TestAddTeamReviewRequest/issue.yml @@ -0,0 +1,16 @@ +- + id: 23 + repo_id: 2 + index: 3 + poster_id: 2 + original_author_id: 0 + name: protected branch pull + content: pull request to a protected branch + milestone_id: 0 + priority: 0 + is_pull: true + is_closed: false + num_comments: 0 + created_unix: 1707270422 + updated_unix: 1707270422 + is_locked: false \ No newline at end of file diff --git a/models/fixtures/TestAddTeamReviewRequest/protected_branch.yml b/models/fixtures/TestAddTeamReviewRequest/protected_branch.yml new file mode 100644 index 0000000000..93909bd991 --- /dev/null +++ b/models/fixtures/TestAddTeamReviewRequest/protected_branch.yml @@ -0,0 +1,28 @@ +- id: 1 + repo_id: 2 + branch_name: protected-main + can_push: false + enable_whitelist: true + whitelist_user_i_ds: [1] + whitelist_team_i_ds: [] + enable_merge_whitelist: true + whitelist_deploy_keys: false + merge_whitelist_user_i_ds: [1] + merge_whitelist_team_i_ds: [] + enable_status_check: false + status_check_contexts: [] + enable_approvals_whitelist: true + approvals_whitelist_user_i_ds: [] + approvals_whitelist_team_i_ds: [1] + required_approvals: 1 + block_on_rejected_reviews: true + block_on_official_review_requests: true + block_on_outdated_branch: true + dismiss_stale_approvals: true + ignore_stale_approvals: false + require_signed_commits: false + protected_file_patterns: "" + unprotected_file_patterns: "" + apply_to_admins: true + created_unix: 1752513073 + updated_unix: 1752513073 \ No newline at end of file diff --git a/models/fixtures/TestAddTeamReviewRequest/pull_request.yml b/models/fixtures/TestAddTeamReviewRequest/pull_request.yml new file mode 100644 index 0000000000..067bb01324 --- /dev/null +++ b/models/fixtures/TestAddTeamReviewRequest/pull_request.yml @@ -0,0 +1,12 @@ +- + id: 11 + type: 0 # gitea pull request + status: 2 # mergeable + issue_id: 23 + index: 3 + head_repo_id: 2 + base_repo_id: 2 + head_branch: feature/protected-branch-pr + base_branch: protected-main + merge_base: 4a357436d925b5c974181ff12a994538ddc5a269 + has_merged: false \ No newline at end of file diff --git a/models/issues/review.go b/models/issues/review.go index 584704d3e8..5370117a81 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -781,10 +781,6 @@ func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organizat official, err := IsOfficialReviewerTeam(ctx, issue, reviewer) if err != nil { return nil, fmt.Errorf("isOfficialReviewerTeam(): %w", err) - } else if !official { - if official, err = IsOfficialReviewer(ctx, issue, doer); err != nil { - return nil, fmt.Errorf("isOfficialReviewer(): %w", err) - } } if review, err = CreateReview(ctx, CreateReviewOptions{ @@ -797,12 +793,6 @@ func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organizat return nil, err } - if official { - if _, err := db.Exec(ctx, "UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id=?", false, issue.ID, reviewer.ID); err != nil { - return nil, err - } - } - comment, err := CreateComment(ctx, &CreateCommentOptions{ Type: CommentTypeReviewRequest, Doer: doer, diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 33d131c225..6e2f6ddfa8 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -8,6 +8,7 @@ import ( "forgejo.org/models/db" issues_model "forgejo.org/models/issues" + organization_model "forgejo.org/models/organization" repo_model "forgejo.org/models/repo" "forgejo.org/models/unittest" user_model "forgejo.org/models/user" @@ -319,3 +320,80 @@ func TestAddReviewRequest(t *testing.T) { require.Error(t, err) assert.True(t, issues_model.IsErrReviewRequestOnClosedPR(err)) } + +func TestAddTeamReviewRequest(t *testing.T) { + defer unittest.OverrideFixtures("models/fixtures/TestAddTeamReviewRequest")() + require.NoError(t, unittest.PrepareTestDatabase()) + + setupForProtectedBranch := func() (*issues_model.Issue, *user_model.User) { + // From override models/fixtures/TestAddTeamReviewRequest/issue.yml; issue #23 is a PR into a protected branch + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 23}) + require.NoError(t, issue.LoadRepo(db.DefaultContext)) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + return issue, doer + } + + t.Run("Protected branch, not official team", func(t *testing.T) { + issue, doer := setupForProtectedBranch() + // Team 2 is not part of the whitelist for this protected branch + team := unittest.AssertExistsAndLoadBean(t, &organization_model.Team{ID: 2}) + + comment, err := issues_model.AddTeamReviewRequest(db.DefaultContext, issue, team, doer) + require.NoError(t, err) + require.NotNil(t, comment) + + review, err := issues_model.GetTeamReviewerByIssueIDAndTeamID(db.DefaultContext, issue.ID, team.ID) + require.NoError(t, err) + require.NotNil(t, review) + assert.Equal(t, issues_model.ReviewTypeRequest, review.Type) + assert.Equal(t, team.ID, review.ReviewerTeamID) + // This review request should not be marked official because it is not a request for a team in the branch + // protection rule's whitelist... + assert.False(t, review.Official) + }) + + t.Run("Protected branch, official team", func(t *testing.T) { + issue, doer := setupForProtectedBranch() + // Team 1 is part of the whitelist for this protected branch + team := unittest.AssertExistsAndLoadBean(t, &organization_model.Team{ID: 1}) + + comment, err := issues_model.AddTeamReviewRequest(db.DefaultContext, issue, team, doer) + require.NoError(t, err) + require.NotNil(t, comment) + + review, err := issues_model.GetTeamReviewerByIssueIDAndTeamID(db.DefaultContext, issue.ID, team.ID) + require.NoError(t, err) + require.NotNil(t, review) + assert.Equal(t, issues_model.ReviewTypeRequest, review.Type) + assert.Equal(t, team.ID, review.ReviewerTeamID) + // Expected to be considered official because team 1 is in the review whitelist for this protected branch + assert.True(t, review.Official) + }) + + t.Run("Unprotected branch, official team", func(t *testing.T) { + // Working on a PR into a branch that is not protected, issue #2 + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) + require.NoError(t, issue.LoadRepo(db.DefaultContext)) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + // team is a team that has write perms against the repo + team := unittest.AssertExistsAndLoadBean(t, &organization_model.Team{ID: 1}) + + comment, err := issues_model.AddTeamReviewRequest(db.DefaultContext, issue, team, doer) + require.NoError(t, err) + require.NotNil(t, comment) + + review, err := issues_model.GetTeamReviewerByIssueIDAndTeamID(db.DefaultContext, issue.ID, team.ID) + require.NoError(t, err) + require.NotNil(t, review) + assert.Equal(t, issues_model.ReviewTypeRequest, review.Type) + assert.Equal(t, team.ID, review.ReviewerTeamID) + // Will not be marked as official because PR #2 there's no branch protection rule that enables whitelist + // approvals (verifying logic in `IsOfficialReviewerTeam` indirectly) + assert.False(t, review.Official) + + // Adding the same team review request again should be a noop + comment, err = issues_model.AddTeamReviewRequest(db.DefaultContext, issue, team, doer) + require.NoError(t, err) + require.Nil(t, comment) + }) +}