From f95fb8cc44d790e0ae71d3f879124a6ee9b07f66 Mon Sep 17 00:00:00 2001 From: Jimmy Praet Date: Sun, 25 Feb 2024 07:00:55 +0100 Subject: [PATCH 1/3] 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 | 20 ++++ routers/web/repo/pull_review_test.go | 76 ++++++++++++ services/forms/repo_form.go | 1 + services/mailer/incoming/incoming_handler.go | 1 + services/pull/review.go | 9 +- 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 | 113 +++++++++--------- web_src/js/features/repo-issue.js | 7 ++ 15 files changed, 227 insertions(+), 74 deletions(-) create mode 100644 routers/web/repo/pull_review_test.go diff --git a/models/issues/comment.go b/models/issues/comment.go index 286a3a2f33..49c3159f6f 100644 --- a/models/issues/comment.go +++ b/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 - attachments, err := repo_model.GetAttachmentsByUUIDs(ctx, opts.Attachments) - if err != nil { - return fmt.Errorf("getAttachmentsByUUIDs [uuids: %v]: %w", opts.Attachments, err) + if err = updateAttachments(ctx, opts, comment); err != nil { + return 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 diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index ab8deab362..efb077cbae 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/routers/api/v1/repo/pull_review.go @@ -508,6 +508,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 05db107545..963c6289b2 100644 --- a/routers/web/repo/issue.go +++ b/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 { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 949962ad68..229016fcb5 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -986,6 +986,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 6c193b9aea..798d3d5454 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" @@ -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) diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go new file mode 100644 index 0000000000..8fc9cecaf3 --- /dev/null +++ b/routers/web/repo/pull_review_test.go @@ -0,0 +1,76 @@ +// 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, "/", contexttest.MockContextOption{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(), `
+ {{template "repo/upload" .}} +
+ {{end}}
diff --git a/templates/repo/diff/comment_form.tmpl b/templates/repo/diff/comment_form.tmpl index 767c2613a0..54817d4740 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 7cabeabd57..e567417fa6 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 dbc8249068..1bad0e9b55 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 e8b546970f..cd0fc6d6a9 100644 --- a/web_src/js/features/common-global.js +++ b/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); - 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}), - }); - } - }); - this.on('error', function (file, message) { - showErrorToast(message); - this.removeFile(file); - }); - }, - }); + 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); + }); + 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. diff --git a/web_src/js/features/repo-issue.js b/web_src/js/features/repo-issue.js index 3437565c80..10faeb135d 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(); } From 3c9b176348e32c7d7356e175b083752f06ff53aa Mon Sep 17 00:00:00 2001 From: oliverpool Date: Tue, 27 Feb 2024 16:05:59 +0100 Subject: [PATCH 2/3] fix cherry-pick --- routers/web/repo/pull.go | 10 ++++++---- routers/web/repo/pull_review_test.go | 6 ++++-- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 229016fcb5..561039c413 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -989,10 +989,12 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi 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 + for _, comments := range line.Conversations { + for _, comment := range comments { + if err := comment.LoadAttachments(ctx); err != nil { + ctx.ServerError("LoadAttachments", err) + return + } } } } diff --git a/routers/web/repo/pull_review_test.go b/routers/web/repo/pull_review_test.go index 8fc9cecaf3..68f68d7bb0 100644 --- a/routers/web/repo/pull_review_test.go +++ b/routers/web/repo/pull_review_test.go @@ -28,7 +28,8 @@ func TestRenderConversation(t *testing.T) { 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, "/", contexttest.MockContextOption{Render: templates.HTMLRenderer()}) + 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) @@ -61,7 +62,8 @@ func TestRenderConversation(t *testing.T) { run("diff without outdated", func(t *testing.T, ctx *context.Context, resp *httptest.ResponseRecorder) { ctx.Data["ShowOutdatedComments"] = false renderConversation(ctx, preparedComment, "diff") - assert.Contains(t, resp.Body.String(), `conversation-not-existing`) + // 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 From e154655c8777d4587113b9992291b623f70b81da Mon Sep 17 00:00:00 2001 From: oliverpool Date: Tue, 27 Feb 2024 16:18:30 +0100 Subject: [PATCH 3/3] fix missing argument --- routers/api/v1/repo/pull_review.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/api/v1/repo/pull_review.go b/routers/api/v1/repo/pull_review.go index efb077cbae..e39a096add 100644 --- a/routers/api/v1/repo/pull_review.go +++ b/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)