Show review summary in pull requests (#5132)

Author: kolaente

models/fixtures/review.yml

@@ -30,3 +30,43 @@
   content: "Pending Review"
   updated_unix: 946684810
   created_unix: 946684810
+-
+  id: 5
+  type: 2
+  reviewer_id: 1
+  issue_id: 3
+  content: "New review 1"
+  updated_unix: 946684810
+  created_unix: 946684810
+-
+  id: 6
+  type: 0
+  reviewer_id: 2
+  issue_id: 3
+  content: "New review 3"
+  updated_unix: 946684810
+  created_unix: 946684810
+-
+  id: 7
+  type: 3
+  reviewer_id: 3
+  issue_id: 3
+  content: "New review 4"
+  updated_unix: 946684810
+  created_unix: 946684810
+-
+  id: 8
+  type: 1
+  reviewer_id: 4
+  issue_id: 3
+  content: "New review 5"
+  updated_unix: 946684810
+  created_unix: 946684810
+-
+  id: 9
+  type: 3
+  reviewer_id: 2
+  issue_id: 3
+  content: "New review 3 rejected"
+  updated_unix: 946684810
+  created_unix: 946684810

models/review.go

@@ -255,3 +255,36 @@ func UpdateReview(r *Review) error {
 	}
 	return nil
 }
+
+// PullReviewersWithType represents the type used to display a review overview
+type PullReviewersWithType struct {
+	User              `xorm:"extends"`
+	Type              ReviewType
+	ReviewUpdatedUnix util.TimeStamp `xorm:"review_updated_unix"`
+}
+
+// GetReviewersByPullID gets all reviewers for a pull request with the statuses
+func GetReviewersByPullID(pullID int64) (issueReviewers []*PullReviewersWithType, err error) {
+	irs := []*PullReviewersWithType{}
+	err = x.Select("`user`.*, review.type, 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").
+		OrderBy("review_updated_unix DESC").
+		Find(&irs)
+
+	// We need to group our results by user id _and_ review type, otherwise the query fails when using postgresql.
+	// 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
+		}
+	}
+
+	return
+}

models/review_test.go

@@ -74,7 +74,7 @@ func TestGetCurrentReview(t *testing.T) {
 	assert.Equal(t, ReviewTypePending, review.Type)
 	assert.Equal(t, "Pending Review", review.Content)
 
-	user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
+	user2 := AssertExistsAndLoadBean(t, &User{ID: 7}).(*User)
 	review2, err := GetCurrentReview(user2, issue)
 	assert.Error(t, err)
 	assert.True(t, IsErrReviewNotExist(err))
@@ -105,3 +105,33 @@ func TestUpdateReview(t *testing.T) {
 	assert.NoError(t, UpdateReview(review))
 	AssertExistsAndLoadBean(t, &Review{ID: 1, Content: "Updated Review"})
 }
+
+func TestGetReviewersByPullID(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,
+	},
+		&PullReviewersWithType{
+			User:              *user3,
+			Type:              ReviewTypeReject,
+			ReviewUpdatedUnix: 946684810,
+		},
+		&PullReviewersWithType{
+			User:              *user4,
+			Type:              ReviewTypeApprove,
+			ReviewUpdatedUnix: 946684810,
+		})
+
+	allReviews, err := GetReviewersByPullID(issue.ID)
+	assert.NoError(t, err)
+	assert.Equal(t, expectedReviews, allReviews)
+}

options/locale/locale_en-US.ini

@@ -832,6 +832,7 @@ issues.review.content.empty = You need to leave a comment indicating the request
 issues.review.reject = "rejected these changes %s"
 issues.review.pending = Pending
 issues.review.review = Review
+issues.review.reviewers = Reviewers
 issues.review.show_outdated = Show outdated
 issues.review.hide_outdated = Hide outdated
 

public/css/index.css

@@ -556,6 +556,38 @@
                     margin-top: 10px;
                 }
             }
+            .review-item {
+
+                .avatar, .type-icon{
+                    float: none;
+                    display: inline-block;
+                    text-align: center;
+                    vertical-align: middle;
+
+                    .octicon {
+                        width: 23px;
+                        font-size: 23px;
+                        margin-top: 0.45em;
+                    }
+                }
+
+                .text {
+                    margin: .3em 0 .5em .5em
+                }
+
+                .type-icon{
+                    float: right;
+                    margin-right: 1em;
+                }
+
+                .divider{
+                    margin: .5rem 0;
+                }
+
+                .review-content {
+                    padding: 1em 0 1em 3.8em;
+                }
+            }
         }
         .comment-list {
             &:before {

public/less/_repository.less

@@ -802,6 +802,12 @@ 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)
+		if err != nil {
+			ctx.ServerError("GetReviewersByPullID", err)
+			return
+		}
 	}
 
 	// Get Dependencies

routers/repo/issue.go

@@ -1,3 +1,38 @@
+{{if gt (len .PullReviewersWithType) 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 }}
+					<div class="ui divider"></div>
+					<div class="review-item">
+						<span class="type-icon text {{if eq .Type 1}}green
+							{{else if eq .Type 2}}grey
+							{{else if eq .Type 3}}red
+							{{else}}grey{{end}}">
+							<span class="octicon octicon-{{.Type.Icon}}"></span>
+						</span>
+						<a class="ui avatar image" href="{{.HomeLink}}">
+							<img src="{{.RelAvatarLink}}">
+						</a>
+						<span class="text grey"><a href="{{.HomeLink}}">{{.Name}}</a>
+							{{if eq .Type 1}}
+								{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
+							{{else if eq .Type 2}}
+								{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
+							{{else if eq .Type 3}}
+								{{$.i18n.Tr "repo.issues.review.reject" $createdStr | Safe}}
+							{{else}}
+								{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
+							{{end}}
+						</span>
+					</div>
+				{{end}}
+			</div>
+		</div>
+	</div>
+{{end}}
 <div class="comment merge box">
 	<a class="avatar text
 	{{if .Issue.PullRequest.HasMerged}}purple