Merge pull request '[GITEA] Internal Server Error when resolving comments' (#2289) from oliverpool/forgejo:forgejo-bp-2282 into v1.21/forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2289
Reviewed-by: crystal <crystal@noreply.codeberg.org>
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

routers/web/repo/pull_review.go

@@ -153,7 +153,7 @@ func UpdateResolveConversation(ctx *context.Context) {
 }
 
 func renderConversation(ctx *context.Context, comment *issues_model.Comment, origin string) {
-	comments, err := issues_model.FetchCodeCommentsByLine(ctx, comment.Issue, ctx.Doer, comment.TreePath, comment.Line, ctx.Data["ShowOutdatedComments"].(bool))
+	comments, err := issues_model.FetchCodeCommentsByLine(ctx, comment.Issue, ctx.Doer, comment.TreePath, comment.Line, true)
 	if err != nil {
 		ctx.ServerError("FetchCodeCommentsByLine", err)
 		return

templates/repo/issue/view_content/conversation.tmpl

@@ -1,6 +1,7 @@
 {{$invalid := (index .comments 0).Invalidated}}
 {{$resolved := (index .comments 0).IsResolved}}
 {{$resolveDoer := (index .comments 0).ResolveDoer}}
+{{$hideByDefault := or $resolved (and $invalid (not .ShowOutdatedComments))}}
 {{$isNotPending := (not (eq (index .comments 0).Review.Type 0))}}
 <div class="ui segments conversation-holder">
 	<div class="ui segment collapsible-comment-box gt-py-3 gt-df gt-ac gt-sb">
@@ -14,7 +15,7 @@
 		</div>
 		<div>
 			{{if or $invalid $resolved}}
-				<button id="show-outdated-{{(index .comments 0).ID}}" data-comment="{{(index .comments 0).ID}}" class="{{if not $resolved}}gt-hidden {{end}}ui compact labeled button show-outdated gt-df gt-ac">
+				<button id="show-outdated-{{(index .comments 0).ID}}" data-comment="{{(index .comments 0).ID}}" class="{{if not $hideByDefault}}gt-hidden {{end}}ui compact labeled button show-outdated gt-df gt-ac">
 					{{svg "octicon-unfold" 16 "gt-mr-3"}}
 					{{if $resolved}}
 						{{ctx.Locale.Tr "repo.issues.review.show_resolved"}}
@@ -22,7 +23,7 @@
 						{{ctx.Locale.Tr "repo.issues.review.show_outdated"}}
 					{{end}}
 				</button>
-				<button id="hide-outdated-{{(index .comments 0).ID}}" data-comment="{{(index .comments 0).ID}}" class="{{if $resolved}}gt-hidden {{end}}ui compact labeled button hide-outdated gt-df gt-ac">
+				<button id="hide-outdated-{{(index .comments 0).ID}}" data-comment="{{(index .comments 0).ID}}" class="{{if $hideByDefault}}gt-hidden {{end}}ui compact labeled button hide-outdated gt-df gt-ac">
 					{{svg "octicon-fold" 16 "gt-mr-3"}}
 					{{if $resolved}}
 						{{ctx.Locale.Tr "repo.issues.review.hide_resolved"}}
@@ -36,7 +37,7 @@
 	{{$diff := (CommentMustAsDiff (index .comments 0))}}
 	{{if $diff}}
 		{{$file := (index $diff.Files 0)}}
-		<div id="code-preview-{{(index .comments 0).ID}}" class="ui table segment{{if $resolved}} gt-hidden{{end}}">
+		<div id="code-preview-{{(index .comments 0).ID}}" class="ui table segment{{if $hideByDefault}} gt-hidden{{end}}">
 			<div class="diff-file-box diff-box file-content {{TabSizeClass $.Editorconfig $file.Name}}">
 				<div class="file-body file-code code-view code-diff code-diff-unified unicode-escaped">
 					<table>
@@ -48,7 +49,7 @@
 			</div>
 		</div>
 	{{end}}
-	<div id="code-comments-{{(index .comments 0).ID}}" class="comment-code-cloud ui segment{{if $resolved}} gt-hidden{{end}}">
+	<div id="code-comments-{{(index .comments 0).ID}}" class="comment-code-cloud ui segment{{if $hideByDefault}} gt-hidden{{end}}">
 		<div class="ui comments gt-mb-0">
 			{{range .comments}}
 				{{$createdSubStr:= TimeSinceUnix .CreatedUnix ctx.Locale}}

tests/integration/integration_test.go

@@ -485,6 +485,24 @@ func logUnexpectedResponse(t testing.TB, recorder *httptest.ResponseRecorder) {
 	t.Helper()
 	respBytes := recorder.Body.Bytes()
 	if len(respBytes) == 0 {
+		// log the content of the flash cookie
+		for _, cookie := range recorder.Result().Cookies() {
+			if cookie.Name != gitea_context.CookieNameFlash {
+				continue
+			}
+			flash, _ := url.ParseQuery(cookie.Value)
+			for key, value := range flash {
+				// the key is itself url-encoded
+				if flash, err := url.ParseQuery(key); err == nil {
+					for key, value := range flash {
+						t.Logf("FlashCookie %q: %q", key, value)
+					}
+				} else {
+					t.Logf("FlashCookie %q: %q", key, value)
+				}
+			}
+		}
+
 		return
 	} else if len(respBytes) < 500 {
 		// if body is short, just log the whole thing

tests/integration/pull_review_test.go

@@ -4,10 +4,15 @@
 package integration
 
 import (
+	"context"
 	"net/http"
+	"strconv"
 	"testing"
 
+	"code.gitea.io/gitea/models/issues"
 	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestPullView_ReviewerMissed(t *testing.T) {
@@ -20,3 +25,68 @@ func TestPullView_ReviewerMissed(t *testing.T) {
 	req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
 	session.MakeRequest(t, req, http.StatusOK)
 }
+
+func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+	session := loginUser(t, "user1")
+
+	req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files")
+	session.MakeRequest(t, req, http.StatusOK)
+
+	req = NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment")
+	resp := session.MakeRequest(t, req, http.StatusOK)
+
+	doc := NewHTMLParser(t, resp.Body)
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{
+		"_csrf":            doc.GetInputValueByName("_csrf"),
+		"origin":           doc.GetInputValueByName("origin"),
+		"latest_commit_id": doc.GetInputValueByName("latest_commit_id"),
+		"side":             "proposed",
+		"line":             "1",
+		"path":             "iso-8859-1.txt",
+		"diff_start_cid":   doc.GetInputValueByName("diff_start_cid"),
+		"diff_end_cid":     doc.GetInputValueByName("diff_end_cid"),
+		"diff_base_cid":    doc.GetInputValueByName("diff_base_cid"),
+		"content":          "nitpicking comment",
+		"pending_review":   "",
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/submit", map[string]string{
+		"_csrf":     doc.GetInputValueByName("_csrf"),
+		"commit_id": doc.GetInputValueByName("latest_commit_id"),
+		"content":   "looks good",
+		"type":      "comment",
+	})
+	session.MakeRequest(t, req, http.StatusOK)
+
+	// retrieve comment_id by reloading the comment page
+	req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
+	resp = session.MakeRequest(t, req, http.StatusOK)
+	doc = NewHTMLParser(t, resp.Body)
+	commentID, ok := doc.Find(`[data-action="Resolve"]`).Attr("data-comment-id")
+	assert.True(t, ok)
+
+	// adjust the database to mark the comment as invalidated
+	// (to invalidate it properly, one should push a commit which should trigger this logic,
+	// in the meantime, use this quick-and-dirty trick)
+	id, err := strconv.ParseInt(commentID, 10, 64)
+	assert.NoError(t, err)
+	assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{
+		ID:          id,
+		Invalidated: true,
+	}))
+
+	req = NewRequestWithValues(t, "POST", "/user2/repo1/issues/resolve_conversation", map[string]string{
+		"_csrf":      doc.GetInputValueByName("_csrf"),
+		"origin":     "timeline",
+		"action":     "Resolve",
+		"comment_id": commentID,
+	})
+	resp = session.MakeRequest(t, req, http.StatusOK)
+
+	// even on template error, the page returns HTTP 200
+	// search the button to mark the comment as unresolved to ensure success.
+	doc = NewHTMLParser(t, resp.Body)
+	assert.Len(t, doc.Find(`[data-action="UnResolve"][data-comment-id="`+commentID+`"]`).Nodes, 1)
+}