Add team support for review request (#12039)

Add team support for review request

Block #11355

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

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

Author: 3 people

models/error.go

@@ -1994,6 +1994,26 @@ func (err ErrReviewNotExist) Error() string {
 	return fmt.Sprintf("review does not exist [id: %d]", err.ID)
 }
 
+// ErrNotValidReviewRequest an not allowed review request modify
+type ErrNotValidReviewRequest struct {
+	Reason string
+	UserID int64
+	RepoID int64
+}
+
+// IsErrNotValidReviewRequest checks if an error is a ErrNotValidReviewRequest.
+func IsErrNotValidReviewRequest(err error) bool {
+	_, ok := err.(ErrReviewNotExist)
+	return ok
+}
+
+func (err ErrNotValidReviewRequest) Error() string {
+	return fmt.Sprintf("%s [user_id: %d, repo_id: %d]",
+		err.Reason,
+		err.UserID,
+		err.RepoID)
+}
+
 //  ________      _____          __  .__
 //  \_____  \    /  _  \  __ ___/  |_|  |__
 //   /   |   \  /  /_\  \|  |  \   __\  |  \

models/fixtures/review.yml

@@ -44,6 +44,7 @@
   reviewer_id: 2
   issue_id: 3
   content: "New review 3"
+  original_author_id: 0
   updated_unix: 946684811
   created_unix: 946684811
 -
@@ -52,13 +53,15 @@
   reviewer_id: 3
   issue_id: 3
   content: "New review 4"
+  original_author_id: 0
   updated_unix: 946684812
   created_unix: 946684812
 -
   id: 8
   type: 1
   reviewer_id: 4
   issue_id: 3
+  original_author_id: 0
   content: "New review 5"
   commit_id: 8091a55037cd59e47293aca02981b5a67076b364
   stale: true
@@ -72,6 +75,7 @@
   content: "New review 3 rejected"
   updated_unix: 946684814
   created_unix: 946684814
+  original_author_id: 0
 
 -
   id: 10

models/issue_comment.go

@@ -137,6 +137,8 @@ type Comment struct {
 	AssigneeID       int64
 	RemovedAssignee  bool
 	Assignee         *User `xorm:"-"`
+	AssigneeTeamID   int64 `xorm:"NOT NULL DEFAULT 0"`
+	AssigneeTeam     *Team `xorm:"-"`
 	ResolveDoerID    int64
 	ResolveDoer      *User `xorm:"-"`
 	OldTitle         string
@@ -487,18 +489,37 @@ func (c *Comment) UpdateAttachments(uuids []string) error {
 	return sess.Commit()
 }
 
-// LoadAssigneeUser if comment.Type is CommentTypeAssignees, then load assignees
-func (c *Comment) LoadAssigneeUser() error {
+// LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees
+func (c *Comment) LoadAssigneeUserAndTeam() error {
 	var err error
 
-	if c.AssigneeID > 0 {
+	if c.AssigneeID > 0 && c.Assignee == nil {
 		c.Assignee, err = getUserByID(x, c.AssigneeID)
 		if err != nil {
 			if !IsErrUserNotExist(err) {
 				return err
 			}
 			c.Assignee = NewGhostUser()
 		}
+	} else if c.AssigneeTeamID > 0 && c.AssigneeTeam == nil {
+		if err = c.LoadIssue(); err != nil {
+			return err
+		}
+
+		if err = c.Issue.LoadRepo(); err != nil {
+			return err
+		}
+
+		if err = c.Issue.Repo.GetOwner(); err != nil {
+			return err
+		}
+
+		if c.Issue.Repo.Owner.IsOrganization() {
+			c.AssigneeTeam, err = GetTeamByID(c.AssigneeTeamID)
+			if err != nil && !IsErrTeamNotExist(err) {
+				return err
+			}
+		}
 	}
 	return nil
 }
@@ -685,6 +706,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
 		ProjectID:        opts.ProjectID,
 		RemovedAssignee:  opts.RemovedAssignee,
 		AssigneeID:       opts.AssigneeID,
+		AssigneeTeamID:   opts.AssigneeTeamID,
 		CommitID:         opts.CommitID,
 		CommitSHA:        opts.CommitSHA,
 		Line:             opts.LineNum,
@@ -849,6 +871,7 @@ type CreateCommentOptions struct {
 	OldProjectID     int64
 	ProjectID        int64
 	AssigneeID       int64
+	AssigneeTeamID   int64
 	RemovedAssignee  bool
 	OldTitle         string
 	NewTitle         string

models/migrations/migrations.go

@@ -240,6 +240,8 @@ var migrations = []Migration{
 	NewMigration("set default password algorithm to Argon2", setDefaultPasswordToArgon2),
 	// v152 -> v153
 	NewMigration("add TrustModel field to Repository", addTrustModelToRepository),
+	// v153 > v154
+	NewMigration("add Team review request support", addTeamReviewRequestSupport),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v153.go

@@ -0,0 +1,25 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func addTeamReviewRequestSupport(x *xorm.Engine) error {
+	type Review struct {
+		ReviewerTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	type Comment struct {
+		AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	if err := x.Sync2(new(Review)); err != nil {
+		return err
+	}
+
+	return x.Sync2(new(Comment))
+}

models/repo.go

@@ -694,32 +694,37 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) {
 	return repo.getAssignees(x)
 }
 
-func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (users []*User, err error) {
-	users = make([]*User, 0, 20)
-
-	if err = e.
-		SQL("SELECT * FROM `user` WHERE id in (SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)) ORDER BY name",
-			repo.ID, AccessModeRead,
-			doerID, posterID).
-		Find(&users); err != nil {
+func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, error) {
+	// Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries
+	if err := repo.getOwner(e); err != nil {
 		return nil, err
 	}
 
-	return users, nil
-}
-
-func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ []*User, err error) {
+	var users []*User
 
-	users := make([]*User, 0)
+	if repo.IsPrivate ||
+		(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
+		// This a private repository:
+		// Anyone who can read the repository is a requestable reviewer
+		if err := e.
+			SQL("SELECT * FROM `user` WHERE id in (SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)) ORDER BY name",
+				repo.ID, AccessModeRead,
+				doerID, posterID).
+			Find(&users); err != nil {
+			return nil, err
+		}
 
-	const SQLCmd = "SELECT * FROM `user` WHERE id IN ( " +
-		"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) " +
-		"UNION " +
-		"SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) " +
-		") ORDER BY name"
+		return users, nil
+	}
 
-	if err = e.
-		SQL(SQLCmd,
+	// This is a "public" repository:
+	// Any user that has write access or who is a watcher can be requested to review
+	if err := e.
+		SQL("SELECT * FROM `user` WHERE id IN ( "+
+			"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) "+
+			"UNION "+
+			"SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) "+
+			") ORDER BY name",
 			repo.ID, AccessModeRead, doerID, posterID,
 			repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto).
 		Find(&users); err != nil {
@@ -729,27 +734,30 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_
 	return users, nil
 }
 
-func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, err error) {
-	if err = repo.getOwner(e); err != nil {
+// GetReviewers get all users can be requested to review:
+// * for private repositories this returns all users that have read access or higher to the repository.
+// * for public repositories this returns all users that have write access or higher to the repository,
+// and all repo watchers.
+// TODO: may be we should hava a busy choice for users to block review request to them.
+func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, error) {
+	return repo.getReviewers(x, doerID, posterID)
+}
+
+// GetReviewerTeams get all teams can be requested to review
+func (repo *Repository) GetReviewerTeams() ([]*Team, error) {
+	if err := repo.GetOwner(); err != nil {
 		return nil, err
 	}
+	if !repo.Owner.IsOrganization() {
+		return nil, nil
+	}
 
-	if repo.IsPrivate ||
-		(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
-		users, err = repo.getReviewersPrivate(x, doerID, posterID)
-	} else {
-		users, err = repo.getReviewersPublic(x, doerID, posterID)
+	teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
+	if err != nil {
+		return nil, err
 	}
-	return
-}
 
-// GetReviewers get all users can be requested to review
-// for private rpo , that return all users that have read access or higher to the repository.
-// but for public rpo, that return all users that have write access or higher to the repository,
-// and all repo watchers.
-// TODO: may be we should hava a busy choice for users to block review request to them.
-func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, err error) {
-	return repo.getReviewers(x, doerID, posterID)
+	return teams, err
 }
 
 // GetMilestoneByID returns the milestone belongs to repository by given ID.

models/repo_test.go

@@ -193,3 +193,34 @@ func TestDoctorUserStarNum(t *testing.T) {
 
 	assert.NoError(t, DoctorUserStarNum())
 }
+
+func TestRepoGetReviewers(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	// test public repo
+	repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
+
+	reviewers, err := repo1.GetReviewers(2, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, 4, len(reviewers))
+
+	// test private repo
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
+	reviewers, err = repo2.GetReviewers(2, 2)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(reviewers))
+}
+
+func TestRepoGetReviewerTeams(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
+	teams, err := repo2.GetReviewerTeams()
+	assert.NoError(t, err)
+	assert.Empty(t, teams)
+
+	repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
+	teams, err = repo3.GetReviewerTeams()
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(teams))
+}