diff --git a/models/issues/comment.go b/models/issues/comment.go index fd0f595945..bfad3935fb 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -663,21 +663,6 @@ func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) error { return nil } -// LoadResolveDoer if comment.Type is CommentTypeCode and ResolveDoerID not zero, then load resolveDoer -func (c *Comment) LoadResolveDoer(ctx context.Context) (err error) { - if c.ResolveDoerID == 0 || c.Type != CommentTypeCode { - return nil - } - c.ResolveDoer, err = user_model.GetUserByID(ctx, c.ResolveDoerID) - if err != nil { - if user_model.IsErrUserNotExist(err) { - c.ResolveDoer = user_model.NewGhostUser() - err = nil - } - } - return err -} - // IsResolved check if an code comment is resolved func (c *Comment) IsResolved() bool { return c.ResolveDoerID != 0 && c.Type == CommentTypeCode diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 3c87a1b41a..800d1e830e 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -133,7 +133,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } - n := 0 + readyComments := make(CommentList, 0, len(comments)) for _, comment := range comments { 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) @@ -143,17 +143,18 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu } comment.Review = re } - comments[n] = comment - n++ + readyComments = append(readyComments, comment) + } - if err := comment.LoadResolveDoer(ctx); err != nil { - return nil, err - } + if err := readyComments.LoadResolveDoers(ctx); err != nil { + return nil, err + } - if err := comment.LoadReactions(ctx, issue.Repo); err != nil { - return nil, err - } + if err := readyComments.LoadReactions(ctx, issue.Repo); err != nil { + return nil, err + } + for _, comment := range readyComments { var err error if comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{ Ctx: ctx, @@ -165,7 +166,8 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } } - return comments[:n], nil + + return readyComments, nil } // FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 53903bea31..9a5c22244b 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -5,6 +5,7 @@ package issues import ( "context" + "errors" "forgejo.org/models/db" repo_model "forgejo.org/models/repo" @@ -390,6 +391,84 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { return nil } +func (comments CommentList) LoadResolveDoers(ctx context.Context) (err error) { + relevant := func(c *Comment) bool { + return c.ResolveDoerID != 0 && c.Type == CommentTypeCode + } + userIDs := make(container.Set[int64]) + for _, comment := range comments { + if relevant(comment) { + userIDs.Add(comment.ResolveDoerID) + } + } + + if len(userIDs) == 0 { + return nil + } + + userMap := make(map[int64]*user_model.User) + users, err := user_model.GetUsersByIDs(ctx, userIDs.Slice()) + if err != nil { + return err + } + for _, user := range users { + userMap[user.ID] = user + } + + for _, comment := range comments { + if !relevant(comment) { + continue + } + resolveDoer, ok := userMap[comment.ResolveDoerID] + if !ok { + comment.ResolveDoer = user_model.NewGhostUser() + } else { + comment.ResolveDoer = resolveDoer + } + } + + return nil +} + +func (comments CommentList) LoadReactions(ctx context.Context, repo *repo_model.Repository) (err error) { + loadIssueID := int64(0) + loadCommentIDs := make([]int64, 0, len(comments)) + + for _, comment := range comments { + if loadIssueID == 0 { + loadIssueID = comment.IssueID + } else if loadIssueID != comment.IssueID { + return errors.New("unable to load reactions from comments on different issues than each other") + } + if comment.Reactions == nil { + loadCommentIDs = append(loadCommentIDs, comment.ID) + } + } + + if loadIssueID == 0 { + return nil + } + + reactions, err := getReactionsForComments(ctx, loadIssueID, loadCommentIDs) + if err != nil { + return err + } + + allReactions := make(ReactionList, 0, len(reactions)) + for _, comment := range comments { + if comment.Reactions == nil { + comment.Reactions = reactions[comment.ID] + allReactions = append(allReactions, comment.Reactions...) + } + } + + if _, err := allReactions.LoadUsers(ctx, repo); err != nil { + return err + } + + return nil +} + func (comments CommentList) getReviewIDs() []int64 { return container.FilterSlice(comments, func(comment *Comment) (int64, bool) { return comment.ReviewID, comment.ReviewID > 0 diff --git a/models/issues/comment_list_test.go b/models/issues/comment_list_test.go index 062a710b84..12a9144722 100644 --- a/models/issues/comment_list_test.go +++ b/models/issues/comment_list_test.go @@ -84,3 +84,111 @@ func TestCommentListLoadUser(t *testing.T) { }) } } + +func TestCommentListLoadResolveDoers(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &Issue{}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + empty := CommentList{} + require.NoError(t, empty.LoadResolveDoers(t.Context())) + + comment1, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello", + }) + require.NoError(t, err) + require.NoError(t, MarkConversation(t.Context(), comment1, doer, true)) + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) // reload after change + comment1List := CommentList{comment1} + require.NoError(t, comment1List.LoadResolveDoers(t.Context())) + require.NotNil(t, comment1.ResolveDoer) + assert.Equal(t, doer.ID, comment1.ResolveDoer.ID) + + comment2, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello again", + }) + require.NoError(t, err) + require.NoError(t, MarkConversation(t.Context(), comment2, user_model.NewGhostUser(), true)) + + // Reload for fresh objects + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) + comment2 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment2.ID}) + + comment2List := CommentList{comment1, comment2} + require.NoError(t, comment2List.LoadResolveDoers(t.Context())) + require.NotNil(t, comment1.ResolveDoer) + assert.Equal(t, doer.ID, comment1.ResolveDoer.ID) + require.NotNil(t, comment2.ResolveDoer) + assert.EqualValues(t, -1, comment2.ResolveDoer.ID) +} + +func TestCommentListLoadReactions(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &Issue{}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + empty := CommentList{} + require.NoError(t, empty.LoadReactions(t.Context(), repo)) + + comment1, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello", + }) + require.NoError(t, err) + _, err = CreateReaction(t.Context(), &ReactionOptions{ + Type: "eyes", + DoerID: doer.ID, + IssueID: issue.ID, + CommentID: comment1.ID, + }) + require.NoError(t, err) + + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) // reload after change + comment1List := CommentList{comment1} + require.NoError(t, comment1List.LoadReactions(t.Context(), repo)) + require.Len(t, comment1.Reactions, 1) + assert.Equal(t, "eyes", comment1.Reactions[0].Type) + assert.NotNil(t, comment1.Reactions[0].User) + + comment2, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello again", + }) + require.NoError(t, err) + _, err = CreateReaction(t.Context(), &ReactionOptions{ + Type: "rocket", + DoerID: doer.ID, + IssueID: issue.ID, + CommentID: comment2.ID, + }) + require.NoError(t, err) + + // Reload for fresh objects + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) + comment2 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment2.ID}) + + comment2List := CommentList{comment1, comment2} + require.NoError(t, comment2List.LoadReactions(t.Context(), repo)) + require.Len(t, comment1.Reactions, 1) + require.Len(t, comment2.Reactions, 1) + assert.Equal(t, "rocket", comment2.Reactions[0].Type) + assert.NotNil(t, comment2.Reactions[0].User) +} diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index c7adf6f62e..da87b8ec2f 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -52,12 +52,29 @@ func TestFetchCodeConversations(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + _, err := issues_model.CreateReaction(t.Context(), &issues_model.ReactionOptions{ + Type: "eyes", + DoerID: 2, + IssueID: issue.ID, + CommentID: 4, + }) + require.NoError(t, err) + require.NoError(t, issues_model.MarkConversation(t.Context(), + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}), + user, true)) + res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false) require.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][0].ID) + require.Contains(t, res, "README.md") + require.Contains(t, res["README.md"], int64(4)) + require.Len(t, res["README.md"][4], 1) + require.Len(t, res["README.md"][4][0], 1) + comment := res["README.md"][4][0][0] + assert.Equal(t, int64(4), comment.ID) + assert.NotNil(t, comment.ResolveDoer) + require.Len(t, comment.Reactions, 1) + r := comment.Reactions[0] + assert.NotNil(t, r.User) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false) diff --git a/models/issues/reaction.go b/models/issues/reaction.go index 522040c022..9a277a8c12 100644 --- a/models/issues/reaction.go +++ b/models/issues/reaction.go @@ -176,6 +176,37 @@ func FindReactions(ctx context.Context, opts FindReactionsOptions) (ReactionList return reactions, count, err } +func getReactionsForComments(ctx context.Context, issueID int64, commentIDs []int64) (map[int64]ReactionList, error) { + reactions := make(map[int64]ReactionList, len(commentIDs)) + left := len(commentIDs) + for left > 0 { + limit := min(left, db.DefaultMaxInSize) + rows, err := db.GetEngine(ctx). + Where(builder.Eq{"issue_id": issueID}). + In("reaction.`type`", setting.UI.Reactions). + In("comment_id", commentIDs[:limit]). + Rows(&Reaction{}) + if err != nil { + return nil, err + } + + for rows.Next() { + var reaction Reaction + err = rows.Scan(&reaction) + if err != nil { + _ = rows.Close() + return nil, err + } + reactions[reaction.CommentID] = append(reactions[reaction.CommentID], &reaction) + } + + _ = rows.Close() + left -= limit + commentIDs = commentIDs[limit:] + } + return reactions, nil +} + func createReaction(ctx context.Context, opts *ReactionOptions) (*Reaction, error) { reaction := &Reaction{ Type: opts.Type, diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index ceebb41f9e..3f58e4e271 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -215,6 +215,11 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) { return } + if err := comments.LoadResolveDoers(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadResolveDoers", err) + return + } + var apiComments []*api.TimelineComment for _, comment := range comments { if comment.Type != issues_model.CommentTypeCode && isXRefCommentAccessible(ctx, ctx.Doer, comment, issue.RepoID, ctx.Reducer) { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 3852c4c18f..bb4185f785 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1663,6 +1663,11 @@ func ViewIssue(ctx *context.Context) { return } + if err := issue.Comments.LoadResolveDoers(ctx); err != nil { + ctx.ServerError("LoadResolveDoers", err) + return + } + for commentIdx, comment = range issue.Comments { comment.Issue = issue metas := ctx.Repo.Repository.ComposeMetas(ctx) @@ -1801,10 +1806,6 @@ func ViewIssue(ctx *context.Context) { } } } - if err = comment.LoadResolveDoer(ctx); err != nil { - ctx.ServerError("LoadResolveDoer", err) - return - } } else if comment.Type == issues_model.CommentTypePullRequestPush { participants = addParticipant(comment.Poster, participants) if err = comment.LoadPushCommits(ctx); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c4b8583e2d..e9cdfc5ac7 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1104,21 +1104,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } - for _, file := range diff.Files { - for _, section := range file.Sections { - for _, line := range section.Lines { - for _, comments := range line.Conversations { - for _, comment := range comments { - if err := comment.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } - } - } - } - } - } - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) if err != nil { ctx.ServerError("LoadProtectedBranch", err) diff --git a/services/convert/issue_comment.go b/services/convert/issue_comment.go index 9ea315aee6..2f9af9be7c 100644 --- a/services/convert/issue_comment.go +++ b/services/convert/issue_comment.go @@ -43,12 +43,6 @@ func ToTimelineComment(ctx context.Context, repo *repo_model.Repository, c *issu return nil } - err = c.LoadResolveDoer(ctx) - if err != nil { - log.Error("LoadResolveDoer: %v", err) - return nil - } - err = c.LoadDepIssueDetails(ctx) if err != nil { log.Error("LoadDepIssueDetails: %v", err)