Add review request api (#11355)

* Add review request api

* add : POST /repos/{owner}/{repo}/pulls/{index}/requested_reviewers
* Remove : DELET /repos/{owner}/{repo}/pulls/{index}/requested_reviewers
* fix some request review bug
* block delet request review by models/DeleteReview()

Signed-off-by: a1012112796 <[email protected]>

* make fmt

* fix bug

* fix test code

* fix typo

* Apply suggestion from code review @jonasfranz

* fix swagger ref

* fix typo

Co-authored-by: Lauris BH <[email protected]>

* fix comment

* Change response message

* chang response so some simplfy

* Add ErrIllLegalReviewRequest
fix some nits

* make fmt

* Apply suggestions from code review

Co-authored-by: silverwind <[email protected]>

* * Add team support
* fix test
* fix an known bug

* fix nit

* fix test

* Apply suggestions from code review

Co-authored-by: zeripath <[email protected]>

* update get api and add test

Co-authored-by: Lauris BH <[email protected]>
Co-authored-by: silverwind <[email protected]>
Co-authored-by: zeripath <[email protected]>

integrations/api_issue_test.go

@@ -153,15 +153,15 @@ func TestAPISearchIssues(t *testing.T) {
 	var apiIssues []*api.Issue
 	DecodeJSON(t, resp, &apiIssues)
 
-	assert.Len(t, apiIssues, 9)
+	assert.Len(t, apiIssues, 10)
 
 	query := url.Values{}
 	query.Add("token", token)
 	link.RawQuery = query.Encode()
 	req = NewRequest(t, "GET", link.String())
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	DecodeJSON(t, resp, &apiIssues)
-	assert.Len(t, apiIssues, 9)
+	assert.Len(t, apiIssues, 10)
 
 	query.Add("state", "closed")
 	link.RawQuery = query.Encode()
@@ -182,7 +182,7 @@ func TestAPISearchIssues(t *testing.T) {
 	req = NewRequest(t, "GET", link.String())
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	DecodeJSON(t, resp, &apiIssues)
-	assert.Len(t, apiIssues, 1)
+	assert.Len(t, apiIssues, 2)
 }
 
 func TestAPISearchIssuesWithLabels(t *testing.T) {
@@ -197,15 +197,15 @@ func TestAPISearchIssuesWithLabels(t *testing.T) {
 	var apiIssues []*api.Issue
 	DecodeJSON(t, resp, &apiIssues)
 
-	assert.Len(t, apiIssues, 9)
+	assert.Len(t, apiIssues, 10)
 
 	query := url.Values{}
 	query.Add("token", token)
 	link.RawQuery = query.Encode()
 	req = NewRequest(t, "GET", link.String())
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	DecodeJSON(t, resp, &apiIssues)
-	assert.Len(t, apiIssues, 9)
+	assert.Len(t, apiIssues, 10)
 
 	query.Add("labels", "label1")
 	link.RawQuery = query.Encode()

integrations/api_pull_review_test.go

@@ -122,4 +122,110 @@ func TestAPIPullReview(t *testing.T) {
 	assert.EqualValues(t, 0, review.CodeCommentsCount)
 	req = NewRequestf(t, http.MethodDelete, "/api/v1/repos/%s/%s/pulls/%d/reviews/%d?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, review.ID, token)
 	resp = session.MakeRequest(t, req, http.StatusNoContent)
+
+	// test get review requests
+	// to make it simple, use same api with get review
+	pullIssue12 := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 12}).(*models.Issue)
+	assert.NoError(t, pullIssue12.LoadAttributes())
+	repo3 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: pullIssue12.RepoID}).(*models.Repository)
+
+	req = NewRequestf(t, http.MethodGet, "/api/v1/repos/%s/%s/pulls/%d/reviews?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token)
+	resp = session.MakeRequest(t, req, http.StatusOK)
+	DecodeJSON(t, resp, &reviews)
+	assert.EqualValues(t, 11, reviews[0].ID)
+	assert.EqualValues(t, "REQUEST_REVIEW", reviews[0].State)
+	assert.EqualValues(t, 0, reviews[0].CodeCommentsCount)
+	assert.EqualValues(t, false, reviews[0].Stale)
+	assert.EqualValues(t, true, reviews[0].Official)
+	assert.EqualValues(t, "test_team", reviews[0].ReviewerTeam.Name)
+
+	assert.EqualValues(t, 12, reviews[1].ID)
+	assert.EqualValues(t, "REQUEST_REVIEW", reviews[1].State)
+	assert.EqualValues(t, 0, reviews[0].CodeCommentsCount)
+	assert.EqualValues(t, false, reviews[1].Stale)
+	assert.EqualValues(t, true, reviews[1].Official)
+	assert.EqualValues(t, 1, reviews[1].Reviewer.ID)
+}
+
+func TestAPIPullReviewRequest(t *testing.T) {
+	defer prepareTestEnv(t)()
+	pullIssue := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 3}).(*models.Issue)
+	assert.NoError(t, pullIssue.LoadAttributes())
+	repo := models.AssertExistsAndLoadBean(t, &models.Repository{ID: pullIssue.RepoID}).(*models.Repository)
+
+	// Test add Review Request
+	session := loginUser(t, "user2")
+	token := getTokenForLoggedInUser(t, session)
+	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.PullReviewRequestOptions{
+		Reviewers: []string{"[email protected]", "user8"},
+	})
+	session.MakeRequest(t, req, http.StatusCreated)
+
+	// poster of pr can't be reviewer
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user1"},
+	})
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	// test user not exist
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.PullReviewRequestOptions{
+		Reviewers: []string{"testOther"},
+	})
+	session.MakeRequest(t, req, http.StatusNotFound)
+
+	// Test Remove Review Request
+	session2 := loginUser(t, "user4")
+	token2 := getTokenForLoggedInUser(t, session2)
+
+	req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token2), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user4"},
+	})
+	session.MakeRequest(t, req, http.StatusNoContent)
+
+	// doer is not admin
+	req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token2), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user8"},
+	})
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo.OwnerName, repo.Name, pullIssue.Index, token), &api.PullReviewRequestOptions{
+		Reviewers: []string{"user8"},
+	})
+	session.MakeRequest(t, req, http.StatusNoContent)
+
+	// Test team review request
+	pullIssue12 := models.AssertExistsAndLoadBean(t, &models.Issue{ID: 12}).(*models.Issue)
+	assert.NoError(t, pullIssue12.LoadAttributes())
+	repo3 := models.AssertExistsAndLoadBean(t, &models.Repository{ID: pullIssue12.RepoID}).(*models.Repository)
+
+	// Test add Team Review Request
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{
+		TeamReviewers: []string{"team1", "owners"},
+	})
+	session.MakeRequest(t, req, http.StatusCreated)
+
+	// Test add Team Review Request to not allowned
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{
+		TeamReviewers: []string{"test_team"},
+	})
+	session.MakeRequest(t, req, http.StatusUnprocessableEntity)
+
+	// Test add Team Review Request to not exist
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{
+		TeamReviewers: []string{"not_exist_team"},
+	})
+	session.MakeRequest(t, req, http.StatusNotFound)
+
+	// Test Remove team Review Request
+	req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{
+		TeamReviewers: []string{"team1"},
+	})
+	session.MakeRequest(t, req, http.StatusNoContent)
+
+	// empty request test
+	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{})
+	session.MakeRequest(t, req, http.StatusCreated)
+
+	req = NewRequestWithJSON(t, http.MethodDelete, fmt.Sprintf("/api/v1/repos/%s/%s/pulls/%d/requested_reviewers?token=%s", repo3.OwnerName, repo3.Name, pullIssue12.Index, token), &api.PullReviewRequestOptions{})
+	session.MakeRequest(t, req, http.StatusNoContent)
 }

integrations/gitea-repositories-meta/user3/repo3.git/refs/heads/test_branch

@@ -0,0 +1 @@
+d22b4d4daa5be07329fcef6ed458f00cf3392da0

models/error.go

@@ -2003,7 +2003,7 @@ type ErrNotValidReviewRequest struct {
 
 // IsErrNotValidReviewRequest checks if an error is a ErrNotValidReviewRequest.
 func IsErrNotValidReviewRequest(err error) bool {
-	_, ok := err.(ErrReviewNotExist)
+	_, ok := err.(ErrNotValidReviewRequest)
 	return ok
 }
 

models/fixtures/issue.yml

@@ -135,3 +135,15 @@
   is_pull: true
   created_unix: 1579194806
   updated_unix: 1579194806
+
+-
+  id: 12
+  repo_id: 3
+  index: 2
+  poster_id: 2
+  name: pull6
+  content: content for the a pull request
+  is_closed: false
+  is_pull: true
+  created_unix: 1602935696
+  updated_unix: 1602935696

models/fixtures/pull_request.yml

@@ -63,3 +63,16 @@
   base_branch: branch1
   merge_base: 1234567890abcdef
   has_merged: false
+
+-
+  id: 6
+  type: 0 # gitea pull request
+  status: 2 # mergable
+  issue_id: 12
+  index: 2
+  head_repo_id: 3
+  base_repo_id: 3
+  head_branch: test_branch
+  base_branch: master
+  merge_base: 2a47ca4b614a9f5a
+  has_merged: false

models/fixtures/repository.yml

@@ -41,7 +41,7 @@
   is_private: true
   num_issues: 1
   num_closed_issues: 0
-  num_pulls: 0
+  num_pulls: 1
   num_closed_pulls: 0
   num_watches: 0
   num_projects: 1

models/fixtures/review.yml

@@ -86,3 +86,22 @@
   official: true
   updated_unix: 946684815
   created_unix: 946684815
+
+-
+  id: 11
+  type: 4
+  reviewer_id: 0
+  reviewer_team_id: 7
+  issue_id: 12
+  official: true
+  updated_unix: 1602936509
+  created_unix: 1602936509
+
+-
+  id: 12
+  type: 4
+  reviewer_id: 1
+  issue_id: 12
+  official: true
+  updated_unix: 1603196749
+  created_unix: 1603196749

models/review.go

@@ -627,13 +627,14 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		}
 	}
 
-	if _, err = createReview(sess, CreateReviewOptions{
+	review, err = createReview(sess, CreateReviewOptions{
 		Type:     ReviewTypeRequest,
 		Issue:    issue,
 		Reviewer: reviewer,
 		Official: official,
 		Stale:    false,
-	}); err != nil {
+	})
+	if err != nil {
 		return nil, err
 	}
 
@@ -644,6 +645,7 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeID:      reviewer.ID, // Use AssigneeID as reviewer ID
+		ReviewID:        review.ID,
 	})
 	if err != nil {
 		return nil, err
@@ -732,7 +734,7 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 		}
 	}
 
-	if _, err = createReview(sess, CreateReviewOptions{
+	if review, err = createReview(sess, CreateReviewOptions{
 		Type:         ReviewTypeRequest,
 		Issue:        issue,
 		ReviewerTeam: reviewer,
@@ -755,6 +757,7 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
 		Issue:           issue,
 		RemovedAssignee: false,       // Use RemovedAssignee as !isRequest
 		AssigneeTeamID:  reviewer.ID, // Use AssigneeTeamID as reviewer team ID
+		ReviewID:        review.ID,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("createComment(): %v", err)
@@ -894,6 +897,10 @@ func DeleteReview(r *Review) error {
 		return fmt.Errorf("review is not allowed to be 0")
 	}
 
+	if r.Type == ReviewTypeRequest {
+		return fmt.Errorf("review request can not be deleted using this method")
+	}
+
 	opts := FindCommentsOptions{
 		Type:     CommentTypeCode,
 		IssueID:  r.IssueID,

modules/convert/convert.go

@@ -284,6 +284,10 @@ func ToOrganization(org *models.User) *api.Organization {
 
 // ToTeam convert models.Team to api.Team
 func ToTeam(team *models.Team) *api.Team {
+	if team == nil {
+		return nil
+	}
+
 	return &api.Team{
 		ID:                      team.ID,
 		Name:                    team.Name,

modules/convert/pull_review.go

@@ -28,6 +28,7 @@ func ToPullReview(r *models.Review, doer *models.User) (*api.PullReview, error)
 	result := &api.PullReview{
 		ID:                r.ID,
 		Reviewer:          ToUser(r.Reviewer, doer != nil, auth),
+		ReviewerTeam:      ToTeam(r.ReviewerTeam),
 		State:             api.ReviewStateUnknown,
 		Body:              r.Content,
 		CommitID:          r.CommitID,

modules/structs/pull_review.go

@@ -30,6 +30,7 @@ const (
 type PullReview struct {
 	ID                int64           `json:"id"`
 	Reviewer          *User           `json:"user"`
+	ReviewerTeam      *Team           `json:"team"`
 	State             ReviewStateType `json:"state"`
 	Body              string          `json:"body"`
 	CommitID          string          `json:"commit_id"`
@@ -90,3 +91,9 @@ type SubmitPullReviewOptions struct {
 	Event ReviewStateType `json:"event"`
 	Body  string          `json:"body"`
 }
+
+// PullReviewRequestOptions are options to add or remove pull review requests
+type PullReviewRequestOptions struct {
+	Reviewers     []string `json:"reviewers"`
+	TeamReviewers []string `json:"team_reviewers"`
+}

routers/api/v1/api.go

@@ -827,7 +827,9 @@ func RegisterRoutes(m *macaron.Macaron) {
 									Get(repo.GetPullReviewComments)
 							})
 						})
-
+						m.Combo("/requested_reviewers").
+							Delete(reqToken(), bind(api.PullReviewRequestOptions{}), repo.DeleteReviewRequests).
+							Post(reqToken(), bind(api.PullReviewRequestOptions{}), repo.CreateReviewRequests)
 					})
 				}, mustAllowPulls, reqRepoReader(models.UnitTypeCode), context.ReferencesGitRepo(false))
 				m.Group("/statuses", func() {