[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 <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: oliverpool <git@olivier.pfad.fr>
Co-committed-by: oliverpool <git@olivier.pfad.fr>
This commit is contained in:
oliverpool 2024-02-16 12:16:11 +00:00
parent 45c0fa4905
commit 07bc099401
11 changed files with 348 additions and 144 deletions

View file

@ -14,15 +14,58 @@ import (
"xorm.io/builder" "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 // CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
type CodeComments map[string]map[int64][]*Comment type CodeComments map[string]map[int64][]*Comment
// FetchCodeComments will return a 2d-map: ["Path"]["Line"] = Comments at line func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) (CodeComments, error) {
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) {
pathToLineToComment := make(CodeComments) pathToLineToComment := make(CodeComments)
if review == nil { if review == nil {
review = &Review{ID: 0} review = &Review{ID: 0}
@ -33,7 +76,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u
ReviewID: review.ID, ReviewID: review.ID,
} }
comments, err := findCodeComments(ctx, opts, issue, currentUser, review, showOutdatedComments) comments, err := findCodeComments(ctx, opts, issue, doer, review, showOutdatedComments)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -47,7 +90,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, currentUser *u
return pathToLineToComment, nil 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 var comments CommentList
if review == nil { if review == nil {
review = &Review{ID: 0} 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 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 the review is pending only the author can see the comments (except if the review is set)
if review.ID == 0 && re.Type == ReviewTypePending && if review.ID == 0 && re.Type == ReviewTypePending &&
(currentUser == nil || currentUser.ID != re.ReviewerID) { (doer == nil || doer.ID != re.ReviewerID) {
continue continue
} }
comment.Review = re comment.Review = re
@ -121,13 +164,14 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
return comments[:n], nil return comments[:n], nil
} }
// FetchCodeCommentsByLine fetches the code comments for a given treePath and line number // FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number)
func FetchCodeCommentsByLine(ctx context.Context, issue *Issue, currentUser *user_model.User, treePath string, line int64, showOutdatedComments bool) ([]*Comment, error) { func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) ([]*Comment, error) {
opts := FindCommentsOptions{ opts := FindCommentsOptions{
Type: CommentTypeCode, Type: CommentTypeCode,
IssueID: issue.ID, IssueID: comment.IssueID,
TreePath: treePath, ReviewID: comment.ReviewID,
Line: line, TreePath: comment.TreePath,
Line: comment.Line,
} }
return findCodeComments(ctx, opts, issue, currentUser, nil, showOutdatedComments) return findCodeComments(ctx, opts, comment.Issue, doer, nil, true)
} }

View file

@ -45,20 +45,20 @@ func TestCreateComment(t *testing.T) {
unittest.AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix)) unittest.AssertInt64InRange(t, now, then, int64(updatedIssue.UpdatedUnix))
} }
func TestFetchCodeComments(t *testing.T) { func TestFetchCodeConversations(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase()) assert.NoError(t, unittest.PrepareTestDatabase())
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) 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.NoError(t, err)
assert.Contains(t, res, "README.md") assert.Contains(t, res, "README.md")
assert.Contains(t, res["README.md"], int64(4)) assert.Contains(t, res["README.md"], int64(4))
assert.Len(t, res["README.md"][4], 1) 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}) 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.NoError(t, err)
assert.Len(t, res, 1) assert.Len(t, res, 1)
} }

View file

@ -153,7 +153,7 @@ func UpdateResolveConversation(ctx *context.Context) {
} }
func renderConversation(ctx *context.Context, comment *issues_model.Comment, origin string) { 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 { if err != nil {
ctx.ServerError("FetchCodeCommentsByLine", err) ctx.ServerError("FetchCodeCommentsByLine", err)
return return

View file

@ -13,7 +13,6 @@ import (
"html/template" "html/template"
"io" "io"
"net/url" "net/url"
"sort"
"strings" "strings"
"time" "time"
@ -80,7 +79,7 @@ type DiffLine struct {
Match int Match int
Type DiffLineType Type DiffLineType
Content string Content string
Comments []*issues_model.Comment Conversations []issues_model.CodeConversation
SectionInfo *DiffLineSectionInfo SectionInfo *DiffLineSectionInfo
} }
@ -118,15 +117,15 @@ func (d *DiffLine) GetHTMLDiffLineType() string {
// CanComment returns whether a line can get commented // CanComment returns whether a line can get commented
func (d *DiffLine) CanComment() bool { 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 // GetCommentSide returns the comment side of the first comment, if not set returns empty string
func (d *DiffLine) GetCommentSide() string { func (d *DiffLine) GetCommentSide() string {
if len(d.Comments) == 0 { if len(d.Conversations) == 0 || len(d.Conversations[0]) == 0 {
return "" return ""
} }
return d.Comments[0].DiffSide() return d.Conversations[0][0].DiffSide()
} }
// GetLineTypeMarker returns the line type marker // GetLineTypeMarker returns the line type marker
@ -467,23 +466,20 @@ type Diff struct {
// LoadComments loads comments into each line // LoadComments loads comments into each line
func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error { 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 { if err != nil {
return err return err
} }
for _, file := range diff.Files { 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 _, section := range file.Sections {
for _, line := range section.Lines { for _, line := range section.Lines {
if comments, ok := lineCommits[int64(line.LeftIdx*-1)]; ok { if conversations, ok := lineCommits[int64(line.LeftIdx*-1)]; ok {
line.Comments = append(line.Comments, comments...) line.Conversations = append(line.Conversations, conversations...)
} }
if comments, ok := lineCommits[int64(line.RightIdx)]; ok { 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
})
} }
} }
} }

View file

@ -601,7 +601,7 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) {
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
diff := setupDefaultDiff() diff := setupDefaultDiff()
assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false)) 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) { 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}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
diff := setupDefaultDiff() diff := setupDefaultDiff()
assert.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true)) 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) { func TestDiffLine_CanComment(t *testing.T) {
assert.False(t, (&DiffLine{Type: DiffLineSection}).CanComment()) 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: DiffLineAdd}).CanComment())
assert.True(t, (&DiffLine{Type: DiffLineDel}).CanComment()) assert.True(t, (&DiffLine{Type: DiffLineDel}).CanComment())
assert.True(t, (&DiffLine{Type: DiffLinePlain}).CanComment()) assert.True(t, (&DiffLine{Type: DiffLinePlain}).CanComment())
} }
func TestDiffLine_GetCommentSide(t *testing.T) { func TestDiffLine_GetCommentSide(t *testing.T) {
assert.Equal(t, "previous", (&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{Comments: []*issues_model.Comment{{Line: 3}}}).GetCommentSide()) assert.Equal(t, "proposed", (&DiffLine{Conversations: []issues_model.CodeConversation{{{Line: 3}}}}).GetCommentSide())
} }
func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) {

View file

@ -57,7 +57,7 @@ func TestGetDiffPreview(t *testing.T) {
RightIdx: 0, RightIdx: 0,
Type: 4, Type: 4,
Content: "@@ -1,3 +1,4 @@", Content: "@@ -1,3 +1,4 @@",
Comments: nil, Conversations: nil,
SectionInfo: &gitdiff.DiffLineSectionInfo{ SectionInfo: &gitdiff.DiffLineSectionInfo{
Path: "README.md", Path: "README.md",
LastLeftIdx: 0, LastLeftIdx: 0,
@ -73,14 +73,14 @@ func TestGetDiffPreview(t *testing.T) {
RightIdx: 1, RightIdx: 1,
Type: 1, Type: 1,
Content: " # repo1", Content: " # repo1",
Comments: nil, Conversations: nil,
}, },
{ {
LeftIdx: 2, LeftIdx: 2,
RightIdx: 2, RightIdx: 2,
Type: 1, Type: 1,
Content: " ", Content: " ",
Comments: nil, Conversations: nil,
}, },
{ {
LeftIdx: 3, LeftIdx: 3,
@ -88,7 +88,7 @@ func TestGetDiffPreview(t *testing.T) {
Match: 4, Match: 4,
Type: 3, Type: 3,
Content: "-Description for repo1", Content: "-Description for repo1",
Comments: nil, Conversations: nil,
}, },
{ {
LeftIdx: 0, LeftIdx: 0,
@ -96,7 +96,7 @@ func TestGetDiffPreview(t *testing.T) {
Match: 3, Match: 3,
Type: 2, Type: 2,
Content: "+Description for repo1", Content: "+Description for repo1",
Comments: nil, Conversations: nil,
}, },
{ {
LeftIdx: 0, LeftIdx: 0,
@ -104,7 +104,7 @@ func TestGetDiffPreview(t *testing.T) {
Match: -1, Match: -1,
Type: 2, Type: 2,
Content: "+this is a new line", Content: "+this is a new line",
Comments: nil, Conversations: nil,
}, },
}, },
}, },

View file

@ -0,0 +1,3 @@
{{range .conversations}}
{{template "repo/diff/conversation" dict "." $ "comments" .}}
{{end}}

View file

@ -108,44 +108,44 @@
</tr> </tr>
{{if and (eq .GetType 3) $hasmatch}} {{if and (eq .GetType 3) $hasmatch}}
{{$match := index $section.Lines $line.Match}} {{$match := index $section.Lines $line.Match}}
{{if or $line.Comments $match.Comments}} {{if or $line.Conversations $match.Conversations}}
<tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}"> <tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}">
<td class="add-comment-left" colspan="4"> <td class="add-comment-left" colspan="4">
{{if $line.Comments}} {{if $line.Conversations}}
{{if eq $line.GetCommentSide "previous"}} {{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}}
{{end}} {{end}}
{{if $match.Comments}} {{if $match.Conversations}}
{{if eq $match.GetCommentSide "previous"}} {{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}}
{{end}} {{end}}
</td> </td>
<td class="add-comment-right" colspan="4"> <td class="add-comment-right" colspan="4">
{{if $line.Comments}} {{if $line.Conversations}}
{{if eq $line.GetCommentSide "proposed"}} {{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}}
{{end}} {{end}}
{{if $match.Comments}} {{if $match.Conversations}}
{{if eq $match.GetCommentSide "proposed"}} {{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}} {{end}}
</td> </td>
</tr> </tr>
{{end}} {{end}}
{{else if $line.Comments}} {{else if $line.Conversations}}
<tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}"> <tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}">
<td class="add-comment-left" colspan="4"> <td class="add-comment-left" colspan="4">
{{if eq $line.GetCommentSide "previous"}} {{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}}
</td> </td>
<td class="add-comment-right" colspan="4"> <td class="add-comment-right" colspan="4">
{{if eq $line.GetCommentSide "proposed"}} {{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}}
</td> </td>
</tr> </tr>

View file

@ -60,10 +60,10 @@
*/}}</td> */}}</td>
{{end}} {{end}}
</tr> </tr>
{{if $line.Comments}} {{if $line.Conversations}}
<tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}"> <tr class="add-comment" data-line-type="{{.GetHTMLDiffLineType}}">
<td class="add-comment-left add-comment-right" colspan="5"> <td class="add-comment-left add-comment-right" colspan="5">
{{template "repo/diff/conversation" dict "." $.root "comments" $line.Comments}} {{template "repo/diff/conversations" dict "." $.root "conversations" $line.Conversations}}
</td> </td>
</tr> </tr>
{{end}} {{end}}

View file

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

View file

@ -10,8 +10,10 @@ import (
"testing" "testing"
"code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/tests" "code.gitea.io/gitea/tests"
"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
) )
@ -26,6 +28,13 @@ func TestPullView_ReviewerMissed(t *testing.T) {
session.MakeRequest(t, req, http.StatusOK) session.MakeRequest(t, req, http.StatusOK)
} }
func loadComment(t *testing.T, commentID string) *issues.Comment {
t.Helper()
id, err := strconv.ParseInt(commentID, 10, 64)
assert.NoError(t, err)
return unittest.AssertExistsAndLoadBean(t, &issues.Comment{ID: id})
}
func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) { func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
defer tests.PrepareTestEnv(t)() defer tests.PrepareTestEnv(t)()
session := loginUser(t, "user1") session := loginUser(t, "user1")
@ -33,9 +42,10 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files") req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files")
session.MakeRequest(t, req, http.StatusOK) session.MakeRequest(t, req, http.StatusOK)
req = NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment") t.Run("single outdated review (line 1)", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment")
resp := session.MakeRequest(t, req, http.StatusOK) resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body) doc := NewHTMLParser(t, resp.Body)
req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{ req = NewRequestWithValues(t, "POST", "/user2/repo1/pulls/3/files/reviews/comments", map[string]string{
"_csrf": doc.GetInputValueByName("_csrf"), "_csrf": doc.GetInputValueByName("_csrf"),
@ -70,10 +80,9 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
// adjust the database to mark the comment as invalidated // adjust the database to mark the comment as invalidated
// (to invalidate it properly, one should push a commit which should trigger this logic, // (to invalidate it properly, one should push a commit which should trigger this logic,
// in the meantime, use this quick-and-dirty trick) // in the meantime, use this quick-and-dirty trick)
id, err := strconv.ParseInt(commentID, 10, 64) comment := loadComment(t, commentID)
assert.NoError(t, err)
assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{ assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{
ID: id, ID: comment.ID,
Invalidated: true, Invalidated: true,
})) }))
@ -86,7 +95,158 @@ func TestPullView_ResolveInvalidatedReviewComment(t *testing.T) {
resp = session.MakeRequest(t, req, http.StatusOK) resp = session.MakeRequest(t, req, http.StatusOK)
// even on template error, the page returns HTTP 200 // even on template error, the page returns HTTP 200
// search the button to mark the comment as unresolved to ensure success. // count the comments to ensure success.
doc = NewHTMLParser(t, resp.Body) doc = NewHTMLParser(t, resp.Body)
assert.Len(t, doc.Find(`[data-action="UnResolve"][data-comment-id="`+commentID+`"]`).Nodes, 1) assert.Len(t, doc.Find(`.comments > .comment`).Nodes, 1)
})
t.Run("outdated and newer review (line 2)", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files/reviews/new_comment")
resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body)
var firstReviewID int64
{
// first (outdated) review
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": "2",
"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)
comment := loadComment(t, commentID)
assert.NoError(t, issues.UpdateCommentInvalidate(context.Background(), &issues.Comment{
ID: comment.ID,
Invalidated: true,
}))
firstReviewID = comment.ReviewID
assert.NotZero(t, firstReviewID)
}
// ID of the first comment for the second (up-to-date) review
var commentID string
{
// second (up-to-date) review on the same line
// make a second review
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": "2",
"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 better",
"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)
commentIDs := doc.Find(`[data-action="Resolve"]`).Map(func(i int, elt *goquery.Selection) string {
v, _ := elt.Attr("data-comment-id")
return v
})
assert.Len(t, commentIDs, 2) // 1 for the outdated review, 1 for the current review
// check that the first comment is for the previous review
comment := loadComment(t, commentIDs[0])
assert.Equal(t, comment.ReviewID, firstReviewID)
// check that the second comment is for a different review
comment = loadComment(t, commentIDs[1])
assert.NotZero(t, comment.ReviewID)
assert.NotEqual(t, comment.ReviewID, firstReviewID)
commentID = commentIDs[1] // save commentID for later
}
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
// count the comments to ensure success.
doc = NewHTMLParser(t, resp.Body)
comments := doc.Find(`.comments > .comment`)
assert.Len(t, comments.Nodes, 1) // the outdated comment belongs to another review and should not be shown
})
t.Run("Files Changed tab", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
for _, c := range []struct {
style, outdated string
expectedCount int
}{
{"unified", "true", 3}, // 1 comment on line 1 + 2 comments on line 3
{"unified", "false", 1}, // 1 comment on line 3 is not outdated
{"split", "true", 3}, // 1 comment on line 1 + 2 comments on line 3
{"split", "false", 1}, // 1 comment on line 3 is not outdated
} {
t.Run(c.style+"+"+c.outdated, func(t *testing.T) {
req := NewRequest(t, "GET", "/user2/repo1/pulls/3/files?style="+c.style+"&show-outdated="+c.outdated)
resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body)
comments := doc.Find(`.comments > .comment`)
assert.Len(t, comments.Nodes, c.expectedCount)
})
}
})
t.Run("Conversation tab", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
req := NewRequest(t, "GET", "/user2/repo1/pulls/3")
resp := session.MakeRequest(t, req, http.StatusOK)
doc := NewHTMLParser(t, resp.Body)
comments := doc.Find(`.comments > .comment`)
assert.Len(t, comments.Nodes, 3) // 1 comment on line 1 + 2 comments on line 3
})
} }