diff --git a/models/fixtures/comment.yml b/models/fixtures/comment.yml index 17586caa219..74fc716180d 100644 --- a/models/fixtures/comment.yml +++ b/models/fixtures/comment.yml @@ -75,3 +75,11 @@ content: "comment in private pository" created_unix: 946684811 updated_unix: 946684811 + +- + id: 9 + type: 22 # review + poster_id: 2 + issue_id: 2 # in repo_id 1 + review_id: 20 + created_unix: 946684810 diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 7a880800684..ac97e24c2be 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -170,3 +170,12 @@ content: "review request for user15" updated_unix: 946684835 created_unix: 946684835 + +- + id: 20 + type: 22 + reviewer_id: 1 + issue_id: 2 + content: "Review Comment" + updated_unix: 946684810 + created_unix: 946684810 diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 7e6594774a8..af6e0ece184 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -4,6 +4,7 @@ package repo import ( + "net/http" "net/http/httptest" "testing" @@ -73,4 +74,20 @@ func TestRenderConversation(t *testing.T) { renderConversation(ctx, preparedComment, "timeline") assert.Contains(t, resp.Body.String(), `
{{end}} - {{if and .Review}} + {{if .Review}} {{if eq .Review.Type 0}}
{{ctx.Locale.Tr "repo.issues.review.pending"}} diff --git a/templates/repo/diff/conversation.tmpl b/templates/repo/diff/conversation.tmpl index aaeac3c550d..507f17fd94b 100644 --- a/templates/repo/diff/conversation.tmpl +++ b/templates/repo/diff/conversation.tmpl @@ -1,66 +1,72 @@ -{{$resolved := (index .comments 0).IsResolved}} -{{$invalid := (index .comments 0).Invalidated}} -{{$resolveDoer := (index .comments 0).ResolveDoer}} -{{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}} -{{$referenceUrl := printf "%s#%s" $.Issue.Link (index .comments 0).HashTag}} -
- {{if $resolved}} -
-
- {{svg "octicon-check" 16 "icon gt-mr-2"}} - {{$resolveDoer.Name}} {{ctx.Locale.Tr "repo.issues.review.resolved_by"}} - {{if $invalid}} - - - {{ctx.Locale.Tr "repo.issues.review.outdated"}} - - {{end}} +{{if len .comments}} + {{$comment := index .comments 0}} + {{$resolved := $comment.IsResolved}} + {{$invalid := $comment.Invalidated}} + {{$resolveDoer := $comment.ResolveDoer}} + {{$hasReview := and $comment.Review}} + {{$isReviewPending := and $hasReview (eq $comment.Review.Type 0)}} + {{$referenceUrl := printf "%s#%s" $.Issue.Link $comment.HashTag}} +
+ {{if $resolved}} +
+
+ {{svg "octicon-check" 16 "icon gt-mr-2"}} + {{$resolveDoer.Name}} {{ctx.Locale.Tr "repo.issues.review.resolved_by"}} + {{if $invalid}} + + + {{ctx.Locale.Tr "repo.issues.review.outdated"}} + + {{end}} +
+
+ + +
-
- - + {{end}} +
+
+ + {{template "repo/diff/comments" dict "root" $ "comments" .comments}} +
-
- {{end}} -
-
- - {{template "repo/diff/comments" dict "root" $ "comments" .comments}} - -
-
-
- - +
+
+ + +
+ {{if and $.CanMarkConversation $hasReview (not $isReviewPending)}} + + {{end}} + {{if and $.SignedUserID (not $.Repository.IsArchived)}} + + {{end}}
- {{if and $.CanMarkConversation $isNotPending}} - - {{end}} - {{if and $.SignedUserID (not $.Repository.IsArchived)}} - - {{end}} + {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" $comment.ReviewID "root" $ "comment" $comment}}
- {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index .comments 0).ReviewID "root" $ "comment" (index .comments 0)}}
-
+{{else}} + {{template "repo/diff/conversation_outdated"}} +{{end}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 4433d86fcc5..218efe50e54 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -365,16 +365,20 @@ {{else if eq .Type 22}}
+ {{$reviewType := -1}} + {{if .Review}}{{$reviewType = .Review.Type}}{{end}} {{if .OriginalAuthor}} {{else}} {{/* Some timeline avatars need a offset to correctly align with their speech bubble. The condition depends on review type and for positive reviews whether there is a comment element or not */}} - + {{ctx.AvatarUtils.Avatar .Poster 40}} {{end}} - {{svg (printf "octicon-%s" .Review.Type.Icon)}} + + {{if .Review}}{{svg (printf "octicon-%s" .Review.Type.Icon)}}{{end}} + {{if .OriginalAuthor}} @@ -387,16 +391,16 @@ {{template "shared/user/authorlink" .Poster}} {{end}} - {{if eq .Review.Type 1}} + {{if eq $reviewType 1}} {{ctx.Locale.Tr "repo.issues.review.approve" $createdStr | Safe}} - {{else if eq .Review.Type 2}} + {{else if eq $reviewType 2}} {{ctx.Locale.Tr "repo.issues.review.comment" $createdStr | Safe}} - {{else if eq .Review.Type 3}} + {{else if eq $reviewType 3}} {{ctx.Locale.Tr "repo.issues.review.reject" $createdStr | Safe}} {{else}} {{ctx.Locale.Tr "repo.issues.review.comment" $createdStr | Safe}} {{end}} - {{if .Review.Dismissed}} + {{if and .Review .Review.Dismissed}}
{{ctx.Locale.Tr "repo.issues.review.dismissed_label"}}
{{end}} @@ -456,7 +460,7 @@
{{end}} - {{if .Review.CodeComments}} + {{if and .Review .Review.CodeComments}}
{{range $filename, $lines := .Review.CodeComments}} {{range $line, $comms := $lines}} @@ -610,10 +614,12 @@ {{template "shared/user/authorlink" .Poster}} {{$reviewerName := ""}} - {{if eq .Review.OriginalAuthor ""}} - {{$reviewerName = .Review.Reviewer.Name}} - {{else}} - {{$reviewerName = .Review.OriginalAuthor}} + {{if .Review}} + {{if eq .Review.OriginalAuthor ""}} + {{$reviewerName = .Review.Reviewer.Name}} + {{else}} + {{$reviewerName = .Review.OriginalAuthor}} + {{end}} {{end}} {{ctx.Locale.Tr "repo.issues.review.dismissed" ($reviewerName | Escape) $createdStr | Safe}} diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index f12fb0c965f..206709f014b 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -1,133 +1,139 @@ -{{$invalid := (index .comments 0).Invalidated}} -{{$resolved := (index .comments 0).IsResolved}} -{{$resolveDoer := (index .comments 0).ResolveDoer}} -{{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}} -
-
-
- {{(index .comments 0).TreePath}} - {{if $invalid}} - - {{ctx.Locale.Tr "repo.issues.review.outdated"}} - - {{end}} -
-
- {{if or $invalid $resolved}} - - - {{end}} +{{if len .comments}} + {{$comment := index .comments 0}} + {{$invalid := $comment.Invalidated}} + {{$resolved := $comment.IsResolved}} + {{$resolveDoer := $comment.ResolveDoer}} + {{$hasReview := and $comment.Review}} + {{$isReviewPending := and $hasReview (eq $comment.Review.Type 0)}} +
+
+
+ {{$comment.TreePath}} + {{if $invalid}} + + {{ctx.Locale.Tr "repo.issues.review.outdated"}} + + {{end}} +
+
+ {{if or $invalid $resolved}} + + + {{end}} +
-
- {{$diff := (CommentMustAsDiff (index .comments 0))}} - {{if $diff}} - {{$file := (index $diff.Files 0)}} -
-
-
- - - {{template "repo/diff/section_unified" dict "file" $file "root" $}} - -
+ {{$diff := (CommentMustAsDiff $comment)}} + {{if $diff}} + {{$file := (index $diff.Files 0)}} +
+
+
+ + + {{template "repo/diff/section_unified" dict "file" $file "root" $}} + +
+
-
- {{end}} -
-
- {{range .comments}} - {{$createdSubStr:= TimeSinceUnix .CreatedUnix ctx.Locale}} -
-
-
-
- {{if not .OriginalAuthor}} - - {{ctx.AvatarUtils.Avatar .Poster 20}} - - {{end}} - - {{if .OriginalAuthor}} - - {{svg (MigrationIcon $.Repository.GetOriginalURLHostname)}} - {{.OriginalAuthor}} - - {{if $.Repository.OriginalURL}} - ({{ctx.Locale.Tr "repo.migrated_from" ($.Repository.OriginalURL|Escape) ($.Repository.GetOriginalURLHostname|Escape) | Safe}}){{end}} - {{else}} - {{template "shared/user/authorlink" .Poster}} + {{end}} +
+
+ {{range .comments}} + {{$createdSubStr:= TimeSinceUnix .CreatedUnix ctx.Locale}} +
+
+
+
+ {{if not .OriginalAuthor}} + + {{ctx.AvatarUtils.Avatar .Poster 20}} + {{end}} - {{ctx.Locale.Tr "repo.issues.commented_at" (.HashTag|Escape) $createdSubStr | Safe}} - + + {{if .OriginalAuthor}} + + {{svg (MigrationIcon $.Repository.GetOriginalURLHostname)}} + {{.OriginalAuthor}} + + {{if $.Repository.OriginalURL}} + ({{ctx.Locale.Tr "repo.migrated_from" ($.Repository.OriginalURL|Escape) ($.Repository.GetOriginalURLHostname|Escape) | Safe}}){{end}} + {{else}} + {{template "shared/user/authorlink" .Poster}} + {{end}} + {{ctx.Locale.Tr "repo.issues.commented_at" (.HashTag|Escape) $createdSubStr | Safe}} + +
+
+ {{template "repo/issue/view_content/show_role" dict "ShowRole" .ShowRole}} + {{if not $.Repository.IsArchived}} + {{template "repo/issue/view_content/add_reaction" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID)}} + {{template "repo/issue/view_content/context_menu" dict "ctxData" $ "item" . "delete" true "issue" true "diff" true "IsCommentPoster" (and $.IsSigned (eq $.SignedUserID .PosterID))}} + {{end}} +
-
- {{template "repo/issue/view_content/show_role" dict "ShowRole" .ShowRole}} - {{if not $.Repository.IsArchived}} - {{template "repo/issue/view_content/add_reaction" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID)}} - {{template "repo/issue/view_content/context_menu" dict "ctxData" $ "item" . "delete" true "issue" true "diff" true "IsCommentPoster" (and $.IsSigned (eq $.SignedUserID .PosterID))}} +
+
+ {{if .RenderedContent}} + {{.RenderedContent|Str2html}} + {{else}} + {{ctx.Locale.Tr "repo.issues.no_content"}} {{end}} +
+
{{.Content}}
+
-
-
-
- {{if .RenderedContent}} - {{.RenderedContent|Str2html}} - {{else}} - {{ctx.Locale.Tr "repo.issues.no_content"}} + {{$reactions := .Reactions.GroupByType}} + {{if $reactions}} + {{template "repo/issue/view_content/reactions" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID) "Reactions" $reactions}} {{end}} -
-
{{.Content}}
-
- {{$reactions := .Reactions.GroupByType}} - {{if $reactions}} - {{template "repo/issue/view_content/reactions" dict "ctxData" $ "ActionURL" (printf "%s/comments/%d/reactions" $.RepoLink .ID) "Reactions" $reactions}} - {{end}} -
-
- {{end}} -
-
-
- {{if $resolved}} -
- {{svg "octicon-check" 16 "gt-mr-2"}} - {{$resolveDoer.Name}} {{ctx.Locale.Tr "repo.issues.review.resolved_by"}}
{{end}}
-
- {{if and $.CanMarkConversation $isNotPending}} - - {{end}} - {{if and $.SignedUserID (not $.Repository.IsArchived)}} - - {{end}} +
+
+ {{if $resolved}} +
+ {{svg "octicon-check" 16 "gt-mr-2"}} + {{$resolveDoer.Name}} {{ctx.Locale.Tr "repo.issues.review.resolved_by"}} +
+ {{end}} +
+
+ {{if and $.CanMarkConversation $hasReview (not $isReviewPending)}} + + {{end}} + {{if and $.SignedUserID (not $.Repository.IsArchived)}} + + {{end}} +
+ {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" $comment.ReviewID "root" $ "comment" $comment}}
- {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index .comments 0).ReviewID "root" $ "comment" (index .comments 0)}}
-
+{{else}} + {{template "repo/diff/conversation_outdated"}} +{{end}} diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 8987ad304b2..c52fce353e7 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -13,6 +13,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/test" repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" "code.gitea.io/gitea/tests" @@ -25,10 +26,19 @@ func TestPullView_ReviewerMissed(t *testing.T) { session := loginUser(t, "user1") req := NewRequest(t, "GET", "/pulls") - session.MakeRequest(t, req, http.StatusOK) + resp := session.MakeRequest(t, req, http.StatusOK) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String())) req = NewRequest(t, "GET", "/user2/repo1/pulls/3") - session.MakeRequest(t, req, http.StatusOK) + resp = session.MakeRequest(t, req, http.StatusOK) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String())) + + // if some reviews are missing, the page shouldn't fail + err := db.TruncateBeans(db.DefaultContext, &issues_model.Review{}) + assert.NoError(t, err) + req = NewRequest(t, "GET", "/user2/repo1/pulls/2") + resp = session.MakeRequest(t, req, http.StatusOK) + assert.True(t, test.IsNormalPageCompleted(resp.Body.String())) } func TestPullView_CodeOwner(t *testing.T) {