From d5bb14de66d9243e8d9899a8ab3b6ce1b88b96a4 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Fri, 2 Feb 2024 10:18:42 +0100 Subject: [PATCH 1/2] [GITEA] add test showing bug on resolving invalidated review comment --- tests/integration/integration_test.go | 18 +++++++ tests/integration/pull_review_test.go | 70 +++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index cb6269cea9..7313ffe3f4 100644 --- a/tests/integration/integration_test.go +++ b/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 diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index 68d80a1021..f0cacbfa58 100644 --- a/tests/integration/pull_review_test.go +++ b/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) +} From ad67d9ef1a219b21309f811c14e7353cbc4982e3 Mon Sep 17 00:00:00 2001 From: oliverpool Date: Fri, 2 Feb 2024 10:40:27 +0100 Subject: [PATCH 2/2] [GITEA] always load outdated comments --- routers/web/repo/pull_review.go | 2 +- templates/repo/issue/view_content/conversation.tmpl | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index 156b70a999..e399176a4a 100644 --- a/routers/web/repo/pull_review.go +++ b/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 diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index f12fb0c965..29f8504e36 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/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))}}
@@ -14,7 +15,7 @@
{{if or $invalid $resolved}} - -