Simplify function GetReviewersByIssueID

Author: davidsvantesson

models/review.go

@@ -10,7 +10,6 @@ import (
 	"code.gitea.io/gitea/modules/timeutil"
 
 	"xorm.io/builder"
-	"xorm.io/core"
 )
 
 // ReviewType defines the sort of feedback a review gives
@@ -332,47 +331,25 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 	return review, comm, sess.Commit()
 }
 
-// PullReviewersWithType represents the type used to display a review overview
-type PullReviewersWithType struct {
-	User              `xorm:"extends"`
-	Type              ReviewType
-	Official          bool
-	ReviewUpdatedUnix timeutil.TimeStamp `xorm:"review_updated_unix"`
-}
+// GetReviewersByIssueID gets the latest review of each reviewer for a pull request
+func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
+	sess := x.NewSession()
+	defer sess.Close()
+	if err := sess.Begin(); err != nil {
+		return nil, err
+	}
 
-// GetReviewersByPullID gets all reviewers for a pull request with the statuses
-func GetReviewersByPullID(pullID int64) (issueReviewers []*PullReviewersWithType, err error) {
-	irs := []*PullReviewersWithType{}
-	if x.Dialect().DBType() == core.MSSQL {
-		err = x.SQL(`SELECT [user].*, review.type, review.official, review.review_updated_unix FROM
-(SELECT review.id, review.type, review.reviewer_id, review.official, max(review.updated_unix) as review_updated_unix
-FROM review WHERE review.issue_id=? AND (review.type = ? OR review.type = ?)
-GROUP BY review.id, review.type, review.reviewer_id, review.official) as review
-INNER JOIN [user] ON review.reviewer_id = [user].id ORDER BY review_updated_unix DESC`,
-			pullID, ReviewTypeApprove, ReviewTypeReject).
-			Find(&irs)
-	} else {
-		err = x.Select("`user`.*, review.type, review.official, max(review.updated_unix) as review_updated_unix").
-			Table("review").
-			Join("INNER", "`user`", "review.reviewer_id = `user`.id").
-			Where("review.issue_id = ? AND (review.type = ? OR review.type = ?)",
-				pullID, ReviewTypeApprove, ReviewTypeReject).
-			GroupBy("`user`.id, review.type, review.official").
-			OrderBy("review_updated_unix DESC").
-			Find(&irs)
+	if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
+		issueID, ReviewTypeApprove, ReviewTypeReject).
+		Find(&reviews); err != nil {
+		return nil, err
 	}
 
-	// We need to group our results by the fields we want, otherwise some databases will fail (only_full_group_by).
-	// But becaus we're doing this, we need to manually filter out multiple reviews of different types by the
-	// same person because we only want to show the newest review grouped by user. Thats why we're using a map here.
-	issueReviewers = []*PullReviewersWithType{}
-	usersInArray := make(map[int64]bool)
-	for _, ir := range irs {
-		if !usersInArray[ir.ID] {
-			issueReviewers = append(issueReviewers, ir)
-			usersInArray[ir.ID] = true
+	for _, review := range reviews {
+		if err := review.loadReviewer(sess); err != nil {
+			return nil, err
 		}
 	}
 
-	return
+	return reviews, nil
 }

models/review_test.go

@@ -98,32 +98,36 @@ func TestCreateReview(t *testing.T) {
 	AssertExistsAndLoadBean(t, &Review{Content: "New Review"})
 }
 
-func TestGetReviewersByPullID(t *testing.T) {
+func TestGetReviewersByIssueID(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	issue := AssertExistsAndLoadBean(t, &Issue{ID: 3}).(*Issue)
 	user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
 	user3 := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
 	user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
 
-	expectedReviews := []*PullReviewersWithType{}
-	expectedReviews = append(expectedReviews, &PullReviewersWithType{
-		User:              *user2,
-		Type:              ReviewTypeReject,
-		ReviewUpdatedUnix: 946684810,
+	expectedReviews := []*Review{}
+	expectedReviews = append(expectedReviews, &Review{
+		Reviewer:    user2,
+		Type:        ReviewTypeReject,
+		UpdatedUnix: 946684810,
 	},
-		&PullReviewersWithType{
-			User:              *user3,
-			Type:              ReviewTypeReject,
-			ReviewUpdatedUnix: 946684810,
+		&Review{
+			Reviewer:    user3,
+			Type:        ReviewTypeReject,
+			UpdatedUnix: 946684810,
 		},
-		&PullReviewersWithType{
-			User:              *user4,
-			Type:              ReviewTypeApprove,
-			ReviewUpdatedUnix: 946684810,
+		&Review{
+			Reviewer:    user4,
+			Type:        ReviewTypeApprove,
+			UpdatedUnix: 946684810,
 		})
 
-	allReviews, err := GetReviewersByPullID(issue.ID)
+	allReviews, err := GetReviewersByIssueID(issue.ID)
 	assert.NoError(t, err)
-	assert.Equal(t, expectedReviews, allReviews)
+	for i, review := range allReviews {
+		assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer)
+		assert.Equal(t, expectedReviews[i].Type, review.Type)
+		assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix)
+	}
 }

routers/repo/issue.go

@@ -934,9 +934,9 @@ func ViewIssue(ctx *context.Context) {
 		}
 		ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
 
-		ctx.Data["PullReviewersWithType"], err = models.GetReviewersByPullID(issue.ID)
+		ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID)
 		if err != nil {
-			ctx.ServerError("GetReviewersByPullID", err)
+			ctx.ServerError("GetReviewersByIssueID", err)
 			return
 		}
 	}

templates/repo/issue/view_content/pull.tmpl

@@ -1,10 +1,10 @@
-{{if gt (len .PullReviewersWithType) 0}}
+{{if gt (len .PullReviewers) 0}}
 	<div class="comment box">
 		<div class="content">
 			<div class="ui segment">
 				<h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4>
-				{{range .PullReviewersWithType}}
-					{{ $createdStr:= TimeSinceUnix .ReviewUpdatedUnix $.Lang }}
+				{{range .PullReviewers}}
+					{{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }}
 					<div class="ui divider"></div>
 					<div class="review-item">
 						<span class="type-icon text {{if eq .Type 1}}green
@@ -13,10 +13,10 @@
 							{{else}}grey{{end}}">
 							<span class="octicon octicon-{{.Type.Icon}}"></span>
 						</span>
-						<a class="ui avatar image" href="{{.HomeLink}}">
-							<img src="{{.RelAvatarLink}}">
+						<a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
+							<img src="{{.Reviewer.RelAvatarLink}}">
 						</a>
-						<span class="text grey"><a href="{{.HomeLink}}">{{.Name}}</a>
+						<span class="text grey"><a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
 							{{if eq .Type 1}}
 								{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
 							{{else if eq .Type 2}}