From 07bc099401023a3938570a0bc6267c1b69f4a48c Mon Sep 17 00:00:00 2001 From: oliverpool Date: Fri, 16 Feb 2024 12:16:11 +0000 Subject: [PATCH] [BUG] split code conversations in diff tab (#2306) Follow-up of #2282 and #2296 (which tried to address #2278) One of the issue with the previous PR is that when a conversation on the Files tab was marked as "resolved", it would fetch all the comments for that line (even the outdated ones, which should not be shown on this page - except when explicitly activated). To properly fix this, I have changed `FetchCodeCommentsByLine` to `FetchCodeConversation`. Its role is to fetch all comments related to a given (review, path, line) and reverted my changes in the template (which were based on a misunderstanding). Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2306 Reviewed-by: Earl Warren Reviewed-by: Gusted Co-authored-by: oliverpool Co-committed-by: oliverpool --- models/issues/comment_code.go | 74 ++++- models/issues/comment_test.go | 8 +- routers/web/repo/pull_review.go | 2 +- services/gitdiff/gitdiff.go | 34 +-- services/gitdiff/gitdiff_test.go | 12 +- services/repository/files/diff_test.go | 66 ++--- templates/repo/diff/conversations.tmpl | 3 + templates/repo/diff/section_split.tmpl | 24 +- templates/repo/diff/section_unified.tmpl | 4 +- .../repo/issue/view_content/conversation.tmpl | 9 +- tests/integration/pull_review_test.go | 256 ++++++++++++++---- 11 files changed, 348 insertions(+), 144 deletions(-) create mode 100644 templates/repo/diff/conversations.tmpl diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 27f6694134..0e6340c189 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -14,15 +14,58 @@ import ( "xorm.io/builder" ) +// CodeConversation contains the comment of a given review +type CodeConversation []*Comment + +// CodeConversationsAtLine contains the conversations for a given line +type CodeConversationsAtLine map[int64][]CodeConversation + +// CodeConversationsAtLineAndTreePath contains the conversations for a given TreePath and line +type CodeConversationsAtLineAndTreePath map[string]CodeConversationsAtLine + +func newCodeConversationsAtLineAndTreePath(comments []*Comment) CodeConversationsAtLineAndTreePath { + tree := make(CodeConversationsAtLineAndTreePath) + for _, comment := range comments { + tree.insertComment(comment) + } + return tree +} + +func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment) { + // attempt to append comment to existing conversations (i.e. list of comments belonging to the same review) + for i, conversation := range tree[comment.TreePath][comment.Line] { + if conversation[0].ReviewID == comment.ReviewID { + tree[comment.TreePath][comment.Line][i] = append(conversation, comment) + return + } + } + + // no previous conversation was found at this line, create it + if tree[comment.TreePath] == nil { + tree[comment.TreePath] = make(map[int64][]CodeConversation) + } + + tree[comment.TreePath][comment.Line] = append(tree[comment.TreePath][comment.Line], CodeConversation{comment}) +} + +// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this line +func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool) (CodeConversationsAtLineAndTreePath, error) { + opts := FindCommentsOptions{ + Type: CommentTypeCode, + IssueID: issue.ID, + } + comments, err := findCodeComments(ctx, opts, issue, doer, nil, showOutdatedComments) + if err != nil { + return nil, err + } + + return newCodeConversationsAtLineAndTreePath(comments), nil +} + // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS type CodeComments map[string]map[int64][]*Comment -// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line -func FetchCodeComments(ctx context.Context, issue *Issue, currentUser *user_model.User, showOutdatedComments bool) (CodeComments, error) { - return fetchCodeCommentsByReview(ctx, issue, currentUser, nil, showOutdatedComments) -} - -func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) { +func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) { pathToLineToComment := make(CodeComments) if review == nil { review = &Review{ID: 0} @@ -33,7 +76,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u ReviewID: review.ID, } - comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments) + comments, err := findCodeComments(ctx, opts, issue, doer, review, showOutdatedComments) if err != nil { return nil, err } @@ -47,7 +90,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u return pathToLineToComment, nil } -func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, currentUser *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) { +func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) { var comments CommentList if review == nil { review = &Review{ID: 0} @@ -91,7 +134,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu if re, ok := reviews[comment.ReviewID]; ok && re != nil { // If the review is pending only the author can see the comments (except if the review is set) if review.ID == 0 && re.Type == ReviewTypePending && - (currentUser == nil || currentUser.ID != re.ReviewerID) { + (doer == nil || doer.ID != re.ReviewerID) { continue } comment.Review = re @@ -121,13 +164,14 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return comments[:n], nil } -// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number -func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) ([]*Comment, error) { +// FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number) +func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) ([]*Comment, error) { opts := FindCommentsOptions{ Type: CommentTypeCode, - IssueID: issue.ID, - TreePath: treePath, - Line: line, + IssueID: comment.IssueID, + ReviewID: comment.ReviewID, + TreePath: comment.TreePath, + Line: comment.Line, } - return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments) + return findCodeComments(ctx, opts, comment.Issue, doer, nil, true) } diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index c5bbfdedc2..dd4bc62986 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -45,20 +45,20 @@ func TestCreateComment(t *testing.T) { unittest.AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix)) } -func TestFetchCodeComments(t *testing.T) { +func TestFetchCodeConversations(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) - res, err := issues_model.FetchCodeComments(db.DefaultContext, issue, user, false) + res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false) assert.NoError(t, err) assert.Contains(t, res, "README.md") assert.Contains(t, res["README.md"], int64(4)) assert.Len(t, res["README.md"][4], 1) - assert.Equal(t, int64(4), res["README.md"][4][0].ID) + assert.Equal(t, int64(4), res["README.md"][4][0][0].ID) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) - res, err = issues_model.FetchCodeComments(db.DefaultContext, issue, user2, false) + res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false) assert.NoError(t, err) assert.Len(t, res, 1) } diff --git a/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index e399176a4a..a5e0c69471 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, true) + comments, err := issues_model.FetchCodeConversation(ctx, comment, ctx.Doer) if err != nil { ctx.ServerError("FetchCodeCommentsByLine", err) return diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index cd9785f67c..b74e1180ea 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -13,7 +13,6 @@ import ( "html/template" "io" "net/url" - "sort" "strings" "time" @@ -75,13 +74,13 @@ const ( // DiffLine represents a line difference in a DiffSection. type DiffLine struct { - LeftIdx int - RightIdx int - Match int - Type DiffLineType - Content string - Comments []*issues_model.Comment - SectionInfo *DiffLineSectionInfo + LeftIdx int + RightIdx int + Match int + Type DiffLineType + Content string + Conversations []issues_model.CodeConversation + SectionInfo *DiffLineSectionInfo } // DiffLineSectionInfo represents diff line section meta data @@ -118,15 +117,15 @@ func (d *DiffLine) GetHTMLDiffLineType() string { // CanComment returns whether a line can get commented func (d *DiffLine) CanComment() bool { - return len(d.Comments) == 0 && d.Type != DiffLineSection + return len(d.Conversations) == 0 && d.Type != DiffLineSection } // GetCommentSide returns the comment side of the first comment, if not set returns empty string func (d *DiffLine) GetCommentSide() string { - if len(d.Comments) == 0 { + if len(d.Conversations) == 0 || len(d.Conversations[0]) == 0 { return "" } - return d.Comments[0].DiffSide() + return d.Conversations[0][0].DiffSide() } // GetLineTypeMarker returns the line type marker @@ -467,23 +466,20 @@ type Diff struct { // LoadComments loads comments into each line func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error { - allComments, err := issues_model.FetchCodeComments(ctx, issue, currentUser, showOutdatedComments) + allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments) if err != nil { return err } for _, file := range diff.Files { - if lineCommits, ok := allComments[file.Name]; ok { + if lineCommits, ok := allConversations[file.Name]; ok { for _, section := range file.Sections { for _, line := range section.Lines { - if comments, ok := lineCommits[int64(line.LeftIdx*-1)]; ok { - line.Comments = append(line.Comments, comments...) + if conversations, ok := lineCommits[int64(line.LeftIdx*-1)]; ok { + line.Conversations = append(line.Conversations, conversations...) } if comments, ok := lineCommits[int64(line.RightIdx)]; ok { - line.Comments = append(line.Comments, comments...) + line.Conversations = append(line.Conversations, comments...) } - sort.SliceStable(line.Comments, func(i, j int) bool { - return line.Comments[i].CreatedUnix < line.Comments[j].CreatedUnix - }) } } } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index e270e46fd4..e55433f1ec 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -601,7 +601,7 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) diff := setupDefaultDiff() assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false)) - assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 2) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2) } func TestDiff_LoadCommentsWithOutdated(t *testing.T) { @@ -611,20 +611,22 @@ func TestDiff_LoadCommentsWithOutdated(t *testing.T) { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) diff := setupDefaultDiff() assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true)) - assert.Len(t, diff.Files[0].Sections[0].Lines[0].Comments, 3) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[0], 2) + assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[1], 1) } func TestDiffLine_CanComment(t *testing.T) { assert.False(t, (&DiffLine{Type: DiffLineSection}).CanComment()) - assert.False(t, (&DiffLine{Type: DiffLineAdd, Comments: []*issues_model.Comment{{Content: "bla"}}}).CanComment()) + assert.False(t, (&DiffLine{Type: DiffLineAdd, Conversations: []issues_model.CodeConversation{{{Content: "bla"}}}}).CanComment()) assert.True(t, (&DiffLine{Type: DiffLineAdd}).CanComment()) assert.True(t, (&DiffLine{Type: DiffLineDel}).CanComment()) assert.True(t, (&DiffLine{Type: DiffLinePlain}).CanComment()) } func TestDiffLine_GetCommentSide(t *testing.T) { - assert.Equal(t, "previous", (&DiffLine{Comments: []*issues_model.Comment{{Line: -3}}}).GetCommentSide()) - assert.Equal(t, "proposed", (&DiffLine{Comments: []*issues_model.Comment{{Line: 3}}}).GetCommentSide()) + assert.Equal(t, "previous", (&DiffLine{Conversations: []issues_model.CodeConversation{{{Line: -3}}}}).GetCommentSide()) + assert.Equal(t, "proposed", (&DiffLine{Conversations: []issues_model.CodeConversation{{{Line: 3}}}}).GetCommentSide()) } func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { diff --git a/services/repository/files/diff_test.go b/services/repository/files/diff_test.go index 91c878e505..fbd2f3e70f 100644 --- a/services/repository/files/diff_test.go +++ b/services/repository/files/diff_test.go @@ -53,11 +53,11 @@ func TestGetDiffPreview(t *testing.T) { Name: "", Lines: []*gitdiff.DiffLine{ { - LeftIdx: 0, - RightIdx: 0, - Type: 4, - Content: "@@ -1,3 +1,4 @@", - Comments: nil, + LeftIdx: 0, + RightIdx: 0, + Type: 4, + Content: "@@ -1,3 +1,4 @@", + Conversations: nil, SectionInfo: &gitdiff.DiffLineSectionInfo{ Path: "README.md", LastLeftIdx: 0, @@ -69,42 +69,42 @@ func TestGetDiffPreview(t *testing.T) { }, }, { - LeftIdx: 1, - RightIdx: 1, - Type: 1, - Content: " # repo1", - Comments: nil, + LeftIdx: 1, + RightIdx: 1, + Type: 1, + Content: " # repo1", + Conversations: nil, }, { - LeftIdx: 2, - RightIdx: 2, - Type: 1, - Content: " ", - Comments: nil, + LeftIdx: 2, + RightIdx: 2, + Type: 1, + Content: " ", + Conversations: nil, }, { - LeftIdx: 3, - RightIdx: 0, - Match: 4, - Type: 3, - Content: "-Description for repo1", - Comments: nil, + LeftIdx: 3, + RightIdx: 0, + Match: 4, + Type: 3, + Content: "-Description for repo1", + Conversations: nil, }, { - LeftIdx: 0, - RightIdx: 3, - Match: 3, - Type: 2, - Content: "+Description for repo1", - Comments: nil, + LeftIdx: 0, + RightIdx: 3, + Match: 3, + Type: 2, + Content: "+Description for repo1", + Conversations: nil, }, { - LeftIdx: 0, - RightIdx: 4, - Match: -1, - Type: 2, - Content: "+this is a new line", - Comments: nil, + LeftIdx: 0, + RightIdx: 4, + Match: -1, + Type: 2, + Content: "+this is a new line", + Conversations: nil, }, }, }, diff --git a/templates/repo/diff/conversations.tmpl b/templates/repo/diff/conversations.tmpl new file mode 100644 index 0000000000..5945337cd3 --- /dev/null +++ b/templates/repo/diff/conversations.tmpl @@ -0,0 +1,3 @@ +{{range .conversations}} + {{template "repo/diff/conversation" dict "." $ "comments" .}} +{{end}} diff --git a/templates/repo/diff/section_split.tmpl b/templates/repo/diff/section_split.tmpl index 5b0d982e96..5137e0e838 100644 --- a/templates/repo/diff/section_split.tmpl +++ b/templates/repo/diff/section_split.tmpl @@ -108,44 +108,44 @@ {{if and (eq .GetType 3) $hasmatch}} {{$match := index $section.Lines $line.Match}} - {{if or $line.Comments $match.Comments}} + {{if or $line.Conversations $match.Conversations}} - {{if $line.Comments}} + {{if $line.Conversations}} {{if eq $line.GetCommentSide "previous"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}} {{end}} {{end}} - {{if $match.Comments}} + {{if $match.Conversations}} {{if eq $match.GetCommentSide "previous"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $match.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $match.Conversations}} {{end}} {{end}} - {{if $line.Comments}} + {{if $line.Conversations}} {{if eq $line.GetCommentSide "proposed"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}} {{end}} {{end}} - {{if $match.Comments}} + {{if $match.Conversations}} {{if eq $match.GetCommentSide "proposed"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $match.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $match.Conversations}} {{end}} {{end}} {{end}} - {{else if $line.Comments}} + {{else if $line.Conversations}} {{if eq $line.GetCommentSide "previous"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}} {{end}} {{if eq $line.GetCommentSide "proposed"}} - {{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}} {{end}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 2b901411e2..7e00677121 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -60,10 +60,10 @@ */}} {{end}} - {{if $line.Comments}} + {{if $line.Conversations}} - {{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} + {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}} {{end}} diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index 29f8504e36..f12fb0c965 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -1,7 +1,6 @@ {{$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))}}
@@ -15,7 +14,7 @@
{{if or $invalid $resolved}} - -