fix: PR not blocked by review request for a whitelisted team (#8511)

Fixes #8491.

Previous behavior always updated the newly created review to set the `official` flag to false.  This logic is now removed.

The most likely reason that this logic existed was that team reviews were being marked as official based upon `doer`, rather than the target team, which seems undesirable.  The expected behavior was retained by removing the check for `IsOfficialReviewer(..., doer)`, ensuring that when making a review request for a team, it doesn't matter *who* makes the request.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8511
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>

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

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

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

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,

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)
+	})
+}