Backport #29050 by wxiaoguang 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`) Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>tags/v1.21.6
@@ -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) |
@@ -92,23 +92,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) | |||
} |
@@ -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 | |||
} | |||
@@ -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`) | |||
}) | |||
} |
@@ -0,0 +1,3 @@ | |||
<div class="ui segment conversation-holder conversation-not-existing"> | |||
{{ctx.Locale.Tr "repo.issues.review.outdated_description"}} | |||
</div> |