From 2e33671f2c1e98759e4fd2a90944c534cfdf5776 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 25 Feb 2024 07:00:55 +0100 Subject: [PATCH] Add attachment support for code review comments (#29220) Fixes #27960, #24411, #12183 --------- Co-authored-by: wxiaoguang --- models/issues/comment.go | 37 +++--- routers/api/v1/repo/pull_review.go | 1 + routers/web/repo/issue.go | 4 + routers/web/repo/pull.go | 13 ++ routers/web/repo/pull_review.go | 19 +++ routers/web/repo/pull_review_test.go | 2 +- services/forms/repo_form.go | 1 + services/mailer/incoming/incoming_handler.go | 1 + services/pull/review.go | 7 +- templates/repo/diff/box.tmpl | 5 + templates/repo/diff/comment_form.tmpl | 6 + templates/repo/diff/comments.tmpl | 5 +- .../repo/issue/view_content/conversation.tmpl | 3 + web_src/js/features/common-global.js | 111 +++++++++--------- web_src/js/features/repo-issue.js | 7 ++ 15 files changed, 149 insertions(+), 73 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index fa0eb3cc0f1..c7b22f3cca0 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -855,6 +855,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 { @@ -872,22 +875,9 @@ func updateCommentInfos(ctx context.Context, opts *CreateCommentOptions, comment } fallthrough case CommentTypeReview: - // Check attachments - 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) - } + if err = updateAttachments(ctx, opts, comment); err != nil { + return err } - - comment.Attachments = attachments case CommentTypeReopen, CommentTypeClose: if err = repo_model.UpdateRepoIssueNumbers(ctx, opts.Issue.RepoID, opts.Issue.IsPull, true); err != nil { return err @@ -897,6 +887,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 diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index 07d8f4877bb..6338651aae9 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -362,6 +362,7 @@ func CreatePullReview(ctx *context.APIContext) { true, // pending review 0, // no reply opts.CommitID, + nil, ); err != nil { ctx.Error(http.StatusInternalServerError, "CreateCodeComment", err) return diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 245ed2b2f2d..46d48c46381 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1718,6 +1718,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 { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7ab21f22b9e..af626dad303 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -970,6 +970,19 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } + for _, file := range diff.Files { + for _, section := range file.Sections { + for _, line := range section.Lines { + for _, comment := range line.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/routers/web/repo/pull_review.go b/routers/web/repo/pull_review.go index f84510b39d8..92665af7e78 100644 --- a/routers/web/repo/pull_review.go +++ b/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" @@ -50,6 +51,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) } @@ -75,6 +78,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, @@ -85,6 +93,7 @@ func CreateCodeComment(ctx *context.Context) { !form.SingleReview, form.Reply, form.LatestCommitID, + attachments, ) if err != nil { ctx.ServerError("CreateCodeComment", err) @@ -168,6 +177,16 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori return } + 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) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 7e6594774a8..8fc9cecaf35 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -39,7 +39,7 @@ func TestRenderConversation(t *testing.T) { 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) + 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 } diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 98d556b9461..98b8d610d0a 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -626,6 +626,7 @@ type CodeCommentForm struct { SingleReview bool `form:"single_review"` Reply int64 `form:"reply"` LatestCommitID string + Files []string } // Validate validates the fields diff --git a/services/mailer/incoming/incoming_handler.go b/services/mailer/incoming/incoming_handler.go index 9682c52456d..5ce2cd5fd57 100644 --- a/services/mailer/incoming/incoming_handler.go +++ b/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) diff --git a/services/pull/review.go b/services/pull/review.go index d4ea9756125..3ffc276778c 100644 --- a/services/pull/review.go +++ b/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 @@ -162,7 +164,7 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. } // createCodeComment creates a plain code comment at the specified line / path -func createCodeComment(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 createCodeComment(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 createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo ReviewID: reviewID, Patch: patch, Invalidated: invalidated, + Attachments: attachments, }) } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index abeeacead03..b9a43a06127 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -237,6 +237,11 @@ "TextareaName" "content" "DropzoneParentContainer" ".ui.form" )}} + {{if .IsAttachmentEnabled}} +
+ {{template "repo/upload" .}} +
+ {{end}}
diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 767c2613a06..54817d47401 100644 --- a/templates/repo/diff/comment_form.tmpl +++ b/templates/repo/diff/comment_form.tmpl @@ -19,6 +19,12 @@ "DisableAutosize" "true" )}} + {{if $.root.IsAttachmentEnabled}} +
+ {{template "repo/upload" $.root}} +
+ {{end}} +
{{svg "octicon-markup"}} {{ctx.Locale.Tr "repo.diff.comment.markdown_info"}}
diff --git a/templates/repo/diff/comments.tmpl b/templates/repo/diff/comments.tmpl index b3d06ed6bce..b795074e498 100644 --- a/templates/repo/diff/comments.tmpl +++ b/templates/repo/diff/comments.tmpl @@ -61,7 +61,10 @@ {{end}}
{{.Content}}
-
+
+ {{if .Attachments}} + {{template "repo/issue/view_content/attachments" dict "ctxData" $ "Attachments" .Attachments "Content" .RenderedContent}} + {{end}}
{{$reactions := .Reactions.GroupByType}} {{if $reactions}} diff --git a/templates/repo/issue/view_content/conversation.tmpl b/templates/repo/issue/view_content/conversation.tmpl index 1bc850d8cfe..56f1af19b2b 100644 --- a/templates/repo/issue/view_content/conversation.tmpl +++ b/templates/repo/issue/view_content/conversation.tmpl @@ -94,6 +94,9 @@
{{.Content}}
+ {{if .Attachments}} + {{template "repo/issue/view_content/attachments" dict "ctxData" $ "Attachments" .Attachments "Content" .RenderedContent}} + {{end}} {{$reactions := .Reactions.GroupByType}} {{if $reactions}} diff --git a/web_src/js/features/common-global.js b/web_src/js/features/common-global.js index e8b546970fc..cd0fc6d6a92 100644 --- a/web_src/js/features/common-global.js +++ b/web_src/js/features/common-global.js @@ -200,63 +200,66 @@ export function initGlobalCommon() { } export function initGlobalDropzone() { - // Dropzone for (const el of document.querySelectorAll('.dropzone')) { - 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 = $(``).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 = `${svg('octicon-copy', 14, 'copy link')} Copy link`; - 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 = ``; - } - 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}), - }); + initDropzone(el); + } +} + +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 = $(``).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 = `${svg('octicon-copy', 14, 'copy link')} Copy link`; + 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 = ``; } + const success = await clippie(fileMarkdown); + showTemporaryTooltip(e.target, success ? i18n.copy_success : i18n.copy_error); }); - this.on('error', function (file, message) { - showErrorToast(message); - this.removeFile(file); - }); - }, - }); - } + 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) { diff --git a/web_src/js/features/repo-issue.js b/web_src/js/features/repo-issue.js index 3437565c804..10faeb135d1 100644 --- a/web_src/js/features/repo-issue.js +++ b/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(); }