Merge pull request '[gitea] add attachment support for code review comments gitea#29220' (#2501) from oliverpool/forgejo:review_attachment_support into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/2501
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/issues/comment.go

@@ -863,6 +863,9 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 	// Check comment type.
 	switch opts.Type {
 	case CommentTypeCode:
+		if err = updateAttachments(ctx, opts, comment); err != nil {
+			return err
+		}
 		if comment.ReviewID != 0 {
 			if comment.Review == nil {
 				if err := comment.loadReview(ctx); err != nil {
@@ -880,22 +883,9 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 		}
 		fallthrough
 	case CommentTypeReview:
-		// Check attachments
+		if err = updateAttachments(ctx, opts, comment); err != nil {
-		attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments)
+			return err
-		if err != nil {
-			return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err)
 		}
-
-		for i := range attachments {
-			attachments[i].IssueID = opts.Issue.ID
-			attachments[i].CommentID = comment.ID
-			// No assign value could be 0, so ignore AllCols().
-			if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil {
-				return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err)
-			}
-		}
-
-		comment.Attachments = attachments
 	case CommentTypeReopen, CommentTypeClose:
 		if err = repo_model.UpdateRepoIssueNumbers(ctx, opts.Issue.RepoID, opts.Issue.IsPull, true); err != nil {
 			return err
@@ -905,6 +895,23 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment
 	return UpdateIssueCols(ctx, opts.Issue, "updated_unix")
 }
 
+func updateAttachments(ctx context.Context, opts *CreateCommentOptions, comment *Comment) error {
+	attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments)
+	if err != nil {
+		return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err)
+	}
+	for i := range attachments {
+		attachments[i].IssueID = opts.Issue.ID
+		attachments[i].CommentID = comment.ID
+		// No assign value could be 0, so ignore AllCols().
+		if _, err = db.GetEngine(ctx).ID(attachments[i].ID).Update(attachments[i]); err != nil {
+			return fmt.Errorf("update attachment [%d]: %w", attachments[i].ID, err)
+		}
+	}
+	comment.Attachments = attachments
+	return nil
+}
+
 func createDeadlineComment(ctx context.Context, doer *user_model.User, issue *Issue, newDeadlineUnix timeutil.TimeStamp) (*Comment, error) {
 	var content string
 	var commentType CommentType

routers/api/v1/repo/pull_review.go

@@ -339,6 +339,7 @@ func CreatePullReviewComment(ctx *context.APIContext) {
 		opts.Path,
 		line,
 		review.ID,
+		nil,
 	)
 	if err != nil {
 		ctx.InternalServerError(err)
@@ -508,6 +509,7 @@ func CreatePullReview(ctx *context.APIContext) {
 			true, // pending review
 			0,    // no reply
 			opts.CommitID,
+			nil,
 		); err != nil {
 			ctx.Error(http.StatusInternalServerError, "CreateCodeComment", err)
 			return

routers/web/repo/issue.go

@@ -1727,6 +1727,10 @@ func ViewIssue(ctx *context.Context) {
 			for _, codeComments := range comment.Review.CodeComments {
 				for _, lineComments := range codeComments {
 					for _, c := range lineComments {
+						if err := c.LoadAttachments(ctx); err != nil {
+							ctx.ServerError("LoadAttachments", err)
+							return
+						}
 						// Check tag.
 						role, ok = marked[c.PosterID]
 						if ok {

routers/web/repo/pull.go

@@ -986,6 +986,21 @@ 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)

routers/web/repo/pull_review.go

@@ -15,6 +15,7 @@ import (
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/upload"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/services/forms"
 	pull_service "code.gitea.io/gitea/services/pull"
@@ -49,6 +50,8 @@ func RenderNewCodeCommentForm(ctx *context.Context) {
 		return
 	}
 	ctx.Data["AfterCommitID"] = pullHeadCommitID
+	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
+	upload.AddUploadContext(ctx, "comment")
 	ctx.HTML(http.StatusOK, tplNewComment)
 }
 
@@ -74,6 +77,11 @@ func CreateCodeComment(ctx *context.Context) {
 		signedLine *= -1
 	}
 
+	var attachments []string
+	if setting.Attachment.Enabled {
+		attachments = form.Files
+	}
+
 	comment, err := pull_service.CreateCodeComment(ctx,
 		ctx.Doer,
 		ctx.Repo.GitRepo,
@@ -84,6 +92,7 @@ func CreateCodeComment(ctx *context.Context) {
 		!form.SingleReview,
 		form.Reply,
 		form.LatestCommitID,
+		attachments,
 	)
 	if err != nil {
 		ctx.ServerError("CreateCodeComment", err)
@@ -159,6 +168,17 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
 		return
 	}
 	ctx.Data["PageIsPullFiles"] = (origin == "diff")
+
+	for _, c := range comments {
+		if err := c.LoadAttachments(ctx); err != nil {
+			ctx.ServerError("LoadAttachments", err)
+			return
+		}
+	}
+
+	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
+	upload.AddUploadContext(ctx, "comment")
+
 	ctx.Data["comments"] = comments
 	if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, comment.Issue, ctx.Doer); err != nil {
 		ctx.ServerError("CanMarkConversation", err)

routers/web/repo/pull_review_test.go

@@ -0,0 +1,78 @@
+// 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, "/")
+			ctx.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, nil)
+		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")
+		// unlike gitea, Forgejo renders the conversation (with the "outdated" label)
+		assert.Contains(t, resp.Body.String(), `repo.issues.review.outdated_description`)
+	})
+	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 is not affected by ShowOutdatedComments=false", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) {
+		ctx.Data["ShowOutdatedComments"] = false
+		renderConversation(ctx, preparedComment, "timeline")
+		assert.Contains(t, resp.Body.String(), `<div id="code-comments-`)
+	})
+}

services/forms/repo_form.go

@@ -637,6 +637,7 @@ type CodeCommentForm struct {
 	SingleReview   bool   `form:"single_review"`
 	Reply          int64  `form:"reply"`
 	LatestCommitID string
+	Files          []string
 }
 
 // Validate validates the fields

services/mailer/incoming/incoming_handler.go

@@ -130,6 +130,7 @@ func (h *ReplyHandler) Handle(ctx context.Context, content *MailContent, doer *u
 				false, // not pending review but a single review
 				comment.ReviewID,
 				"",
+				nil,
 			)
 			if err != nil {
 				return fmt.Errorf("CreateCodeComment failed: %w", err)

services/pull/review.go

@@ -71,7 +71,7 @@ func InvalidateCodeComments(ctx context.Context, prs issues_model.PullRequestLis
 }
 
 // CreateCodeComment creates a comment on the code line
-func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string) (*issues_model.Comment, error) {
+func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, line int64, content, treePath string, pendingReview bool, replyReviewID int64, latestCommitID string, attachments []string) (*issues_model.Comment, error) {
 	var (
 		existsReview bool
 		err          error
@@ -104,6 +104,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 			treePath,
 			line,
 			replyReviewID,
+			attachments,
 		)
 		if err != nil {
 			return nil, err
@@ -144,6 +145,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 		treePath,
 		line,
 		review.ID,
+		attachments,
 	)
 	if err != nil {
 		return nil, err
@@ -161,8 +163,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git.
 	return comment, nil
 }
 
-// createCodeComment creates a plain code comment at the specified line / path
+// CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path
-func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64) (*issues_model.Comment, error) {
+func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) {
 	var commitID, patch string
 	if err := issue.LoadPullRequest(ctx); err != nil {
 		return nil, fmt.Errorf("LoadPullRequest: %w", err)
@@ -260,6 +262,7 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
 		ReviewID:    reviewID,
 		Patch:       patch,
 		Invalidated: invalidated,
+		Attachments: attachments,
 	})
 }
 

templates/repo/diff/box.tmpl

@@ -240,6 +240,11 @@
 					"TextareaName" "content"
 					"DropzoneParentContainer" ".ui.form"
 				)}}
+				{{if .IsAttachmentEnabled}}
+					<div class="field">
+						{{template "repo/upload" .}}
+					</div>
+				{{end}}
 				<div class="text right edit buttons">
 					<button class="ui cancel button">{{ctx.Locale.Tr "repo.issues.cancel"}}</button>
 					<button class="ui primary save button">{{ctx.Locale.Tr "repo.issues.save"}}</button>

templates/repo/diff/comment_form.tmpl

@@ -19,6 +19,12 @@
 			"DisableAutosize" "true"
 		)}}
 
+		{{if $.root.IsAttachmentEnabled}}
+			<div class="field">
+				{{template "repo/upload" $.root}}
+			</div>
+		{{end}}
+
 		<div class="field footer gt-mx-3">
 			<span class="markup-info">{{svg "octicon-markup"}} {{ctx.Locale.Tr "repo.diff.comment.markdown_info"}}</span>
 			<div class="gt-text-right">

templates/repo/diff/comments.tmpl

@@ -61,7 +61,10 @@
 			{{end}}
 			</div>
 			<div id="issuecomment-{{.ID}}-raw" class="raw-content gt-hidden">{{.Content}}</div>
-			<div class="edit-content-zone gt-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-context="{{$.root.RepoLink}}"></div>
+			<div class="edit-content-zone gt-hidden" data-update-url="{{$.root.RepoLink}}/comments/{{.ID}}" data-context="{{$.root.RepoLink}}" data-attachment-url="{{$.root.RepoLink}}/comments/{{.ID}}/attachments"></div>
+			{{if .Attachments}}
+				{{template "repo/issue/view_content/attachments" dict "ctxData" $ "Attachments" .Attachments "Content" .RenderedContent}}
+			{{end}}
 		</div>
 		{{$reactions := .Reactions.GroupByType}}
 		{{if $reactions}}

templates/repo/issue/view_content/conversation.tmpl

@@ -94,6 +94,9 @@
 							</div>
 							<div id="issuecomment-{{.ID}}-raw" class="raw-content gt-hidden">{{.Content}}</div>
 							<div class="edit-content-zone gt-hidden" data-update-url="{{$.RepoLink}}/comments/{{.ID}}" data-context="{{$.RepoLink}}" data-attachment-url="{{$.RepoLink}}/comments/{{.ID}}/attachments"></div>
+							{{if .Attachments}}
+								{{template "repo/issue/view_content/attachments" dict "ctxData" $ "Attachments" .Attachments "Content" .RenderedContent}}
+							{{end}}
 						</div>
 						{{$reactions := .Reactions.GroupByType}}
 						{{if $reactions}}

web_src/js/features/common-global.js

@@ -200,65 +200,68 @@ export function initGlobalCommon() {
 }
 
 export function initGlobalDropzone() {
-  // Dropzone
   for (const el of document.querySelectorAll('.dropzone')) {
-    const $dropzone = $(el);
+    initDropzone(el);
-    const _promise = createDropzone(el, {
-      url: $dropzone.data('upload-url'),
-      headers: {'X-Csrf-Token': csrfToken},
-      maxFiles: $dropzone.data('max-file'),
-      maxFilesize: $dropzone.data('max-size'),
-      acceptedFiles: (['*/*', ''].includes($dropzone.data('accepts'))) ? null : $dropzone.data('accepts'),
-      addRemoveLinks: true,
-      dictDefaultMessage: $dropzone.data('default-message'),
-      dictInvalidFileType: $dropzone.data('invalid-input-type'),
-      dictFileTooBig: $dropzone.data('file-too-big'),
-      dictRemoveFile: $dropzone.data('remove-file'),
-      timeout: 0,
-      thumbnailMethod: 'contain',
-      thumbnailWidth: 480,
-      thumbnailHeight: 480,
-      init() {
-        this.on('success', (file, data) => {
-          file.uuid = data.uuid;
-          const input = $(`<input id="${data.uuid}" name="files" type="hidden">`).val(data.uuid);
-          $dropzone.find('.files').append(input);
-          // Create a "Copy Link" element, to conveniently copy the image
-          // or file link as Markdown to the clipboard
-          const copyLinkElement = document.createElement('div');
-          copyLinkElement.className = 'gt-text-center';
-          // The a element has a hardcoded cursor: pointer because the default is overridden by .dropzone
-          copyLinkElement.innerHTML = `<a href="#" style="cursor: pointer;">${svg('octicon-copy', 14, 'copy link')} Copy link</a>`;
-          copyLinkElement.addEventListener('click', async (e) => {
-            e.preventDefault();
-            let fileMarkdown = `[${file.name}](/attachments/${file.uuid})`;
-            if (file.type.startsWith('image/')) {
-              fileMarkdown = `!${fileMarkdown}`;
-            } else if (file.type.startsWith('video/')) {
-              fileMarkdown = `<video src="/attachments/${file.uuid}" title="${htmlEscape(file.name)}" controls></video>`;
-            }
-            const success = await clippie(fileMarkdown);
-            showTemporaryTooltip(e.target, success ? i18n.copy_success : i18n.copy_error);
-          });
-          file.previewTemplate.append(copyLinkElement);
-        });
-        this.on('removedfile', (file) => {
-          $(`#${file.uuid}`).remove();
-          if ($dropzone.data('remove-url')) {
-            POST($dropzone.data('remove-url'), {
-              data: new URLSearchParams({file: file.uuid}),
-            });
-          }
-        });
-        this.on('error', function (file, message) {
-          showErrorToast(message);
-          this.removeFile(file);
-        });
-      },
-    });
   }
 }
 
+export function initDropzone(el) {
+  const $dropzone = $(el);
+  const _promise = createDropzone(el, {
+    url: $dropzone.data('upload-url'),
+    headers: {'X-Csrf-Token': csrfToken},
+    maxFiles: $dropzone.data('max-file'),
+    maxFilesize: $dropzone.data('max-size'),
+    acceptedFiles: (['*/*', ''].includes($dropzone.data('accepts'))) ? null : $dropzone.data('accepts'),
+    addRemoveLinks: true,
+    dictDefaultMessage: $dropzone.data('default-message'),
+    dictInvalidFileType: $dropzone.data('invalid-input-type'),
+    dictFileTooBig: $dropzone.data('file-too-big'),
+    dictRemoveFile: $dropzone.data('remove-file'),
+    timeout: 0,
+    thumbnailMethod: 'contain',
+    thumbnailWidth: 480,
+    thumbnailHeight: 480,
+    init() {
+      this.on('success', (file, data) => {
+        file.uuid = data.uuid;
+        const input = $(`<input id="${data.uuid}" name="files" type="hidden">`).val(data.uuid);
+        $dropzone.find('.files').append(input);
+        // Create a "Copy Link" element, to conveniently copy the image
+        // or file link as Markdown to the clipboard
+        const copyLinkElement = document.createElement('div');
+        copyLinkElement.className = 'gt-text-center';
+        // The a element has a hardcoded cursor: pointer because the default is overridden by .dropzone
+        copyLinkElement.innerHTML = `<a href="#" style="cursor: pointer;">${svg('octicon-copy', 14, 'copy link')} Copy link</a>`;
+        copyLinkElement.addEventListener('click', async (e) => {
+          e.preventDefault();
+          let fileMarkdown = `[${file.name}](/attachments/${file.uuid})`;
+          if (file.type.startsWith('image/')) {
+            fileMarkdown = `!${fileMarkdown}`;
+          } else if (file.type.startsWith('video/')) {
+            fileMarkdown = `<video src="/attachments/${file.uuid}" title="${htmlEscape(file.name)}" controls></video>`;
+          }
+          const success = await clippie(fileMarkdown);
+          showTemporaryTooltip(e.target, success ? i18n.copy_success : i18n.copy_error);
+        });
+        file.previewTemplate.append(copyLinkElement);
+      });
+      this.on('removedfile', (file) => {
+        $(`#${file.uuid}`).remove();
+        if ($dropzone.data('remove-url')) {
+          POST($dropzone.data('remove-url'), {
+            data: new URLSearchParams({file: file.uuid}),
+          });
+        }
+      });
+      this.on('error', function (file, message) {
+        showErrorToast(message);
+        this.removeFile(file);
+      });
+    },
+  });
+}
+
 async function linkAction(e) {
   // A "link-action" can post AJAX request to its "data-url"
   // Then the browser is redirected to: the "redirect" in response, or "data-redirect" attribute, or current URL by reloading.

web_src/js/features/repo-issue.js

@@ -5,6 +5,7 @@ import {hideElem, showElem, toggleElem} from '../utils/dom.js';
 import {setFileFolding} from './file-fold.js';
 import {getComboMarkdownEditor, initComboMarkdownEditor} from './comp/ComboMarkdownEditor.js';
 import {toAbsoluteUrl} from '../utils.js';
+import {initDropzone} from './common-global.js';
 
 const {appSubUrl, csrfToken} = window.config;
 
@@ -382,6 +383,11 @@ export async function handleReply($el) {
   const $textarea = form.find('textarea');
   let editor = getComboMarkdownEditor($textarea);
   if (!editor) {
+    // FIXME: the initialization of the dropzone is not consistent.
+    // When the page is loaded, the dropzone is initialized by initGlobalDropzone, but the editor is not initialized.
+    // When the form is submitted and partially reload, none of them is initialized.
+    const dropzone = form.find('.dropzone')[0];
+    if (!dropzone.dropzone) initDropzone(dropzone);
     editor = await initComboMarkdownEditor(form.find('.combo-markdown-editor'));
   }
   editor.focus();
@@ -511,6 +517,7 @@ export function initRepoPullRequestReview() {
       td.find("input[name='side']").val(side === 'left' ? 'previous' : 'proposed');
       td.find("input[name='path']").val(path);
 
+      initDropzone(td.find('.dropzone')[0]);
       const editor = await initComboMarkdownEditor(td.find('.combo-markdown-editor'));
       editor.focus();
     }