Improve user experience for outdated comments (#29050)

Try to improve #28949

1. Make `ctx.Data["ShowOutdatedComments"] = true` by default: it brings
consistent user experience, and sometimes the "outdated (source
changed)" comments are still valuable.
2. Show a friendly message if the comment won't show, then the end users
won't fell that "the comment disappears" (it is the special case when
`ShowOutdatedComments = false`)

diff --git a/modules/contexttest/context_tests.go b/modules/contexttest/context_tests.go
index 9ca028bb6ec26643ba3fad1de3979e93ff846945..c9bacf259f530012bc054adaf16466b83784fa9a 100644 (file)
--- a/modules/contexttest/context_tests.go
+++ b/modules/contexttest/context_tests.go
@@ -40,8 +40,19 @@ func mockRequest(t *testing.T, reqPath string) *http.Request {
        return req
 }
 
+type MockContextOption struct {
+       Render context.Render
+}
+
 // MockContext mock context for unit tests
-func MockContext(t *testing.T, reqPath string) (*context.Context, *httptest.ResponseRecorder) {
+func MockContext(t *testing.T, reqPath string, opts ...MockContextOption) (*context.Context, *httptest.ResponseRecorder) {
+       var opt MockContextOption
+       if len(opts) > 0 {
+               opt = opts[0]
+       }
+       if opt.Render == nil {
+               opt.Render = &MockRender{}
+       }
        resp := httptest.NewRecorder()
        req := mockRequest(t, reqPath)
        base, baseCleanUp := context.NewBaseContext(resp, req)
@@ -49,7 +60,7 @@ func MockContext(t *testing.T, reqPath string) (*context.Context, *httptest.Resp
        base.Data = middleware.GetContextData(req.Context())
        base.Locale = &translation.MockLocale{}
 
-       ctx := context.NewWebContext(base, &MockRender{}, nil)
+       ctx := context.NewWebContext(base, opt.Render, nil)
 
        chiCtx := chi.NewRouteContext()
        ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)

diff --git a/routers/web/repo/middlewares.go b/routers/web/repo/middlewares.go
index ee49649654ca3e3a1b6dec07791299d8ae2b6441..d70a53030e659ee35827d172469898775f24812b 100644 (file)
--- a/routers/web/repo/middlewares.go
+++ b/routers/web/repo/middlewares.go
@@ -98,23 +98,15 @@ func SetWhitespaceBehavior(ctx *context.Context) {
 // SetShowOutdatedComments set the show outdated comments option as context variable
 func SetShowOutdatedComments(ctx *context.Context) {
        showOutdatedCommentsValue := ctx.FormString("show-outdated")
-       // var showOutdatedCommentsValue string
-
        if showOutdatedCommentsValue != "true" && showOutdatedCommentsValue != "false" {
                // invalid or no value for this form string -> use default or stored user setting
+               showOutdatedCommentsValue = "true"
                if ctx.IsSigned {
-                       showOutdatedCommentsValue, _ = user_model.GetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, "false")
-               } else {
-                       // not logged in user -> use the default value
-                       showOutdatedCommentsValue = "false"
+                       showOutdatedCommentsValue, _ = user_model.GetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
                }
-       } else {
+       } else if ctx.IsSigned {
                // valid value -> update user setting if user is logged in
-               if ctx.IsSigned {
-                       _ = user_model.SetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
-               }
+               _ = user_model.SetUserSetting(ctx, ctx.Doer.ID, user_model.SettingsKeyShowOutdatedComments, showOutdatedCommentsValue)
        }
-
-       showOutdatedComments, _ := strconv.ParseBool(showOutdatedCommentsValue)
-       ctx.Data["ShowOutdatedComments"] = showOutdatedComments
+       ctx.Data["ShowOutdatedComments"], _ = strconv.ParseBool(showOutdatedCommentsValue)
 }

diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go
index a6b3bd1c8d77f7ae1afb04ffb1b898d75bc0aa2e..b93460d1696ae8cabc1d0526ce26082249aec546 100644 (file)
--- a/routers/web/repo/pull_review.go
+++ b/routers/web/repo/pull_review.go
@@ -22,6 +22,7 @@ import (
 
 const (
        tplDiffConversation     base.TplName = "repo/diff/conversation"
+       tplConversationOutdated base.TplName = "repo/diff/conversation_outdated"
        tplTimelineConversation base.TplName = "repo/issue/view_content/conversation"
        tplNewComment           base.TplName = "repo/diff/new_comment"
 )
@@ -161,8 +162,8 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
                return
        }
        if len(comments) == 0 {
-               // if the comments are empty (deleted, outdated, etc), it doesn't need to render anything, just return an empty body to replace "conversation-holder" on the page
-               ctx.Resp.WriteHeader(http.StatusOK)
+               // if the comments are empty (deleted, outdated, etc), it's better to tell the users that it is outdated
+               ctx.HTML(http.StatusOK, tplConversationOutdated)
                return
        }
 

diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go
new file mode 100644 (file)
index 0000000..65019af
--- /dev/null
+++ b/routers/web/repo/pull_review_test.go
@@ -0,0 +1,76 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package repo
+
+import (
+       "net/http/httptest"
+       "testing"
+
+       "code.gitea.io/gitea/models/db"
+       issues_model "code.gitea.io/gitea/models/issues"
+       "code.gitea.io/gitea/models/unittest"
+       "code.gitea.io/gitea/modules/context"
+       "code.gitea.io/gitea/modules/contexttest"
+       "code.gitea.io/gitea/modules/templates"
+       "code.gitea.io/gitea/services/pull"
+
+       "github.com/stretchr/testify/assert"
+)
+
+func TestRenderConversation(t *testing.T) {
+       unittest.PrepareTestEnv(t)
+
+       pr, _ := issues_model.GetPullRequestByID(db.DefaultContext, 2)
+       _ = pr.LoadIssue(db.DefaultContext)
+       _ = pr.Issue.LoadPoster(db.DefaultContext)
+       _ = pr.Issue.LoadRepo(db.DefaultContext)
+
+       run := func(name string, cb func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder)) {
+               t.Run(name, func(t *testing.T) {
+                       ctx, resp := contexttest.MockContext(t, "/", contexttest.MockContextOption{Render: templates.HTMLRenderer()})
+                       contexttest.LoadUser(t, ctx, pr.Issue.PosterID)
+                       contexttest.LoadRepo(t, ctx, pr.BaseRepoID)
+                       contexttest.LoadGitRepo(t, ctx)
+                       defer ctx.Repo.GitRepo.Close()
+                       cb(t, ctx, resp)
+               })
+       }
+
+       var preparedComment *issues_model.Comment
+       run("prepare", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+               comment, err := pull.CreateCodeComment(ctx, pr.Issue.Poster, ctx.Repo.GitRepo, pr.Issue, 1, "content", "", false, 0, pr.HeadCommitID)
+               if !assert.NoError(t, err) {
+                       return
+               }
+               comment.Invalidated = true
+               err = issues_model.UpdateCommentInvalidate(ctx, comment)
+               if !assert.NoError(t, err) {
+                       return
+               }
+               preparedComment = comment
+       })
+       if !assert.NotNil(t, preparedComment) {
+               return
+       }
+       run("diff with outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+               ctx.Data["ShowOutdatedComments"] = true
+               renderConversation(ctx, preparedComment, "diff")
+               assert.Contains(t, resp.Body.String(), `<div class="content comment-container"`)
+       })
+       run("diff without outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+               ctx.Data["ShowOutdatedComments"] = false
+               renderConversation(ctx, preparedComment, "diff")
+               assert.Contains(t, resp.Body.String(), `conversation-not-existing`)
+       })
+       run("timeline with outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+               ctx.Data["ShowOutdatedComments"] = true
+               renderConversation(ctx, preparedComment, "timeline")
+               assert.Contains(t, resp.Body.String(), `<div id="code-comments-`)
+       })
+       run("timeline without outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+               ctx.Data["ShowOutdatedComments"] = false
+               renderConversation(ctx, preparedComment, "timeline")
+               assert.Contains(t, resp.Body.String(), `conversation-not-existing`)
+       })
+}

diff --git a/templates/repo/diff/conversation_outdated.tmpl b/templates/repo/diff/conversation_outdated.tmpl
new file mode 100644 (file)
index 0000000..c6a4bf1
--- /dev/null
+++ b/templates/repo/diff/conversation_outdated.tmpl
@@ -0,0 +1,3 @@
+<div class="ui segment conversation-holder conversation-not-existing">
+       {{ctx.Locale.Tr "repo.issues.review.outdated_description"}}
+</div>