From d5f2c9d74d63443cc2abbcabc268cf1121f58e8b Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Thu, 20 Apr 2023 16:21:10 +0800
Subject: [PATCH] Fix issue attachment handling (#24202) (#24221)
Backport #24202
Close #24195
Fix the bug:
1. The old code doesn't handle `removedfile` event correctly
2. The old code doesn't provide attachments for type=CommentTypeReview
---------
Co-authored-by: silverwind <me@silverwind.io>
---
models/issues/comment.go | 147 +++++++++---------
models/issues/comment_test.go | 5 +-
models/issues/issue.go | 2 +-
routers/api/v1/repo/issue_comment.go | 4 +-
routers/web/repo/issue.go | 28 ++--
services/issue/comments.go | 3 +-
.../repo/issue/view_content/attachments.tmpl | 18 +--
web_src/js/features/repo-legacy.js | 53 ++++---
8 files changed, 131 insertions(+), 129 deletions(-)
diff --git a/models/issues/comment.go b/models/issues/comment.go
index a47dc7c151..c7e815c8b6 100644
--- a/models/issues/comment.go
+++ b/models/issues/comment.go
@@ -52,84 +52,61 @@ func (err ErrCommentNotExist) Unwrap() error {
// CommentType defines whether a comment is just a simple comment, an action (like close) or a reference.
type CommentType int
-// define unknown comment type
-const (
- CommentTypeUnknown CommentType = -1
-)
+// CommentTypeUndefined is used to search for comments of any type
+const CommentTypeUndefined CommentType = -1
-// Enumerate all the comment types
const (
- // 0 Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0)
- CommentTypeComment CommentType = iota
- CommentTypeReopen // 1
- CommentTypeClose // 2
+ CommentTypeComment CommentType = iota // 0 Plain comment, can be associated with a commit (CommitID > 0) and a line (LineNum > 0)
+
+ CommentTypeReopen // 1
+ CommentTypeClose // 2
+
+ CommentTypeIssueRef // 3 References.
+ CommentTypeCommitRef // 4 Reference from a commit (not part of a pull request)
+ CommentTypeCommentRef // 5 Reference from a comment
+ CommentTypePullRef // 6 Reference from a pull request
+
+ CommentTypeLabel // 7 Labels changed
+ CommentTypeMilestone // 8 Milestone changed
+ CommentTypeAssignees // 9 Assignees changed
+ CommentTypeChangeTitle // 10 Change Title
+ CommentTypeDeleteBranch // 11 Delete Branch
+
+ CommentTypeStartTracking // 12 Start a stopwatch for time tracking
+ CommentTypeStopTracking // 13 Stop a stopwatch for time tracking
+ CommentTypeAddTimeManual // 14 Add time manual for time tracking
+ CommentTypeCancelTracking // 15 Cancel a stopwatch for time tracking
+ CommentTypeAddedDeadline // 16 Added a due date
+ CommentTypeModifiedDeadline // 17 Modified the due date
+ CommentTypeRemovedDeadline // 18 Removed a due date
+
+ CommentTypeAddDependency // 19 Dependency added
+ CommentTypeRemoveDependency // 20 Dependency removed
+
+ CommentTypeCode // 21 Comment a line of code
+ CommentTypeReview // 22 Reviews a pull request by giving general feedback
+
+ CommentTypeLock // 23 Lock an issue, giving only collaborators access
+ CommentTypeUnlock // 24 Unlocks a previously locked issue
+
+ CommentTypeChangeTargetBranch // 25 Change pull request's target branch
+
+ CommentTypeDeleteTimeManual // 26 Delete time manual for time tracking
+
+ CommentTypeReviewRequest // 27 add or remove Request from one
+ CommentTypeMergePull // 28 merge pull request
+ CommentTypePullRequestPush // 29 push to PR head branch
+
+ CommentTypeProject // 30 Project changed
+ CommentTypeProjectBoard // 31 Project board changed
+
+ CommentTypeDismissReview // 32 Dismiss Review
+
+ CommentTypeChangeIssueRef // 33 Change issue ref
+
+ CommentTypePRScheduledToAutoMerge // 34 pr was scheduled to auto merge when checks succeed
+ CommentTypePRUnScheduledToAutoMerge // 35 pr was un scheduled to auto merge when checks succeed
- // 3 References.
- CommentTypeIssueRef
- // 4 Reference from a commit (not part of a pull request)
- CommentTypeCommitRef
- // 5 Reference from a comment
- CommentTypeCommentRef
- // 6 Reference from a pull request
- CommentTypePullRef
- // 7 Labels changed
- CommentTypeLabel
- // 8 Milestone changed
- CommentTypeMilestone
- // 9 Assignees changed
- CommentTypeAssignees
- // 10 Change Title
- CommentTypeChangeTitle
- // 11 Delete Branch
- CommentTypeDeleteBranch
- // 12 Start a stopwatch for time tracking
- CommentTypeStartTracking
- // 13 Stop a stopwatch for time tracking
- CommentTypeStopTracking
- // 14 Add time manual for time tracking
- CommentTypeAddTimeManual
- // 15 Cancel a stopwatch for time tracking
- CommentTypeCancelTracking
- // 16 Added a due date
- CommentTypeAddedDeadline
- // 17 Modified the due date
- CommentTypeModifiedDeadline
- // 18 Removed a due date
- CommentTypeRemovedDeadline
- // 19 Dependency added
- CommentTypeAddDependency
- // 20 Dependency removed
- CommentTypeRemoveDependency
- // 21 Comment a line of code
- CommentTypeCode
- // 22 Reviews a pull request by giving general feedback
- CommentTypeReview
- // 23 Lock an issue, giving only collaborators access
- CommentTypeLock
- // 24 Unlocks a previously locked issue
- CommentTypeUnlock
- // 25 Change pull request's target branch
- CommentTypeChangeTargetBranch
- // 26 Delete time manual for time tracking
- CommentTypeDeleteTimeManual
- // 27 add or remove Request from one
- CommentTypeReviewRequest
- // 28 merge pull request
- CommentTypeMergePull
- // 29 push to PR head branch
- CommentTypePullRequestPush
- // 30 Project changed
- CommentTypeProject
- // 31 Project board changed
- CommentTypeProjectBoard
- // 32 Dismiss Review
- CommentTypeDismissReview
- // 33 Change issue ref
- CommentTypeChangeIssueRef
- // 34 pr was scheduled to auto merge when checks succeed
- CommentTypePRScheduledToAutoMerge
- // 35 pr was un scheduled to auto merge when checks succeed
- CommentTypePRUnScheduledToAutoMerge
)
var commentStrings = []string{
@@ -181,7 +158,23 @@ func AsCommentType(typeName string) CommentType {
return CommentType(index)
}
}
- return CommentTypeUnknown
+ return CommentTypeUndefined
+}
+
+func (t CommentType) HasContentSupport() bool {
+ switch t {
+ case CommentTypeComment, CommentTypeCode, CommentTypeReview:
+ return true
+ }
+ return false
+}
+
+func (t CommentType) HasAttachmentSupport() bool {
+ switch t {
+ case CommentTypeComment, CommentTypeCode, CommentTypeReview:
+ return true
+ }
+ return false
}
// RoleDescriptor defines comment tag type
@@ -1039,7 +1032,7 @@ func (opts *FindCommentsOptions) ToConds() builder.Cond {
if opts.Before > 0 {
cond = cond.And(builder.Lte{"comment.updated_unix": opts.Before})
}
- if opts.Type != CommentTypeUnknown {
+ if opts.Type != CommentTypeUndefined {
cond = cond.And(builder.Eq{"comment.type": opts.Type})
}
if opts.Line != 0 {
diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go
index f1232729f1..43bad1660f 100644
--- a/models/issues/comment_test.go
+++ b/models/issues/comment_test.go
@@ -64,8 +64,9 @@ func TestFetchCodeComments(t *testing.T) {
}
func TestAsCommentType(t *testing.T) {
- assert.Equal(t, issues_model.CommentTypeUnknown, issues_model.AsCommentType(""))
- assert.Equal(t, issues_model.CommentTypeUnknown, issues_model.AsCommentType("nonsense"))
+ assert.Equal(t, issues_model.CommentType(0), issues_model.CommentTypeComment)
+ assert.Equal(t, issues_model.CommentTypeUndefined, issues_model.AsCommentType(""))
+ assert.Equal(t, issues_model.CommentTypeUndefined, issues_model.AsCommentType("nonsense"))
assert.Equal(t, issues_model.CommentTypeComment, issues_model.AsCommentType("comment"))
assert.Equal(t, issues_model.CommentTypePRUnScheduledToAutoMerge, issues_model.AsCommentType("pull_cancel_scheduled_merge"))
}
diff --git a/models/issues/issue.go b/models/issues/issue.go
index c59e9d14e5..c8d148dd86 100644
--- a/models/issues/issue.go
+++ b/models/issues/issue.go
@@ -267,7 +267,7 @@ func (issue *Issue) LoadPullRequest(ctx context.Context) (err error) {
}
func (issue *Issue) loadComments(ctx context.Context) (err error) {
- return issue.loadCommentsByType(ctx, CommentTypeUnknown)
+ return issue.loadCommentsByType(ctx, CommentTypeUndefined)
}
// LoadDiscussComments loads discuss comments
diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go
index 3d14343d47..6ae6063303 100644
--- a/routers/api/v1/repo/issue_comment.go
+++ b/routers/api/v1/repo/issue_comment.go
@@ -173,7 +173,7 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) {
IssueID: issue.ID,
Since: since,
Before: before,
- Type: issues_model.CommentTypeUnknown,
+ Type: issues_model.CommentTypeUndefined,
}
comments, err := issues_model.FindComments(ctx, opts)
@@ -549,7 +549,7 @@ func editIssueComment(ctx *context.APIContext, form api.EditIssueCommentOption)
return
}
- if comment.Type != issues_model.CommentTypeComment && comment.Type != issues_model.CommentTypeReview && comment.Type != issues_model.CommentTypeCode {
+ if !comment.Type.HasContentSupport() {
ctx.Status(http.StatusNoContent)
return
}
diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go
index 1a17afeef3..018925b8d1 100644
--- a/routers/web/repo/issue.go
+++ b/routers/web/repo/issue.go
@@ -1563,7 +1563,7 @@ func ViewIssue(ctx *context.Context) {
return
}
}
- } else if comment.Type == issues_model.CommentTypeCode || comment.Type == issues_model.CommentTypeReview || comment.Type == issues_model.CommentTypeDismissReview {
+ } else if comment.Type.HasContentSupport() {
comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{
URLPrefix: ctx.Repo.RepoLink,
Metas: ctx.Repo.Repository.ComposeMetas(),
@@ -2800,7 +2800,7 @@ func UpdateCommentContent(ctx *context.Context) {
return
}
- if comment.Type != issues_model.CommentTypeComment && comment.Type != issues_model.CommentTypeReview && comment.Type != issues_model.CommentTypeCode {
+ if !comment.Type.HasContentSupport() {
ctx.Error(http.StatusNoContent)
return
}
@@ -2864,7 +2864,7 @@ func DeleteComment(ctx *context.Context) {
if !ctx.IsSigned || (ctx.Doer.ID != comment.PosterID && !ctx.Repo.CanWriteIssuesOrPulls(comment.Issue.IsPull)) {
ctx.Error(http.StatusForbidden)
return
- } else if comment.Type != issues_model.CommentTypeComment && comment.Type != issues_model.CommentTypeCode {
+ } else if !comment.Type.HasContentSupport() {
ctx.Error(http.StatusNoContent)
return
}
@@ -3010,7 +3010,7 @@ func ChangeCommentReaction(ctx *context.Context) {
return
}
- if comment.Type != issues_model.CommentTypeComment && comment.Type != issues_model.CommentTypeCode && comment.Type != issues_model.CommentTypeReview {
+ if !comment.Type.HasContentSupport() {
ctx.Error(http.StatusNoContent)
return
}
@@ -3126,15 +3126,19 @@ func GetCommentAttachments(ctx *context.Context) {
ctx.NotFoundOrServerError("GetCommentByID", issues_model.IsErrCommentNotExist, err)
return
}
+
+ if !comment.Type.HasAttachmentSupport() {
+ ctx.ServerError("GetCommentAttachments", fmt.Errorf("comment type %v does not support attachments", comment.Type))
+ return
+ }
+
attachments := make([]*api.Attachment, 0)
- if comment.Type == issues_model.CommentTypeComment {
- if err := comment.LoadAttachments(ctx); err != nil {
- ctx.ServerError("LoadAttachments", err)
- return
- }
- for i := 0; i < len(comment.Attachments); i++ {
- attachments = append(attachments, convert.ToAttachment(comment.Attachments[i]))
- }
+ if err := comment.LoadAttachments(ctx); err != nil {
+ ctx.ServerError("LoadAttachments", err)
+ return
+ }
+ for i := 0; i < len(comment.Attachments); i++ {
+ attachments = append(attachments, convert.ToAttachment(comment.Attachments[i]))
}
ctx.JSON(http.StatusOK, attachments)
}
diff --git a/services/issue/comments.go b/services/issue/comments.go
index 1323fb47aa..22eb31e3c4 100644
--- a/services/issue/comments.go
+++ b/services/issue/comments.go
@@ -90,8 +90,7 @@ func CreateIssueComment(ctx context.Context, doer *user_model.User, repo *repo_m
// UpdateComment updates information of comment.
func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_model.User, oldContent string) error {
- needsContentHistory := c.Content != oldContent &&
- (c.Type == issues_model.CommentTypeComment || c.Type == issues_model.CommentTypeReview || c.Type == issues_model.CommentTypeCode)
+ needsContentHistory := c.Content != oldContent && c.Type.HasContentSupport()
if needsContentHistory {
hasContentHistory, err := issues_model.HasIssueContentHistory(ctx, c.IssueID, c.ID)
if err != nil {
diff --git a/templates/repo/issue/view_content/attachments.tmpl b/templates/repo/issue/view_content/attachments.tmpl
index 86696ec62b..eaedeb06cf 100644
--- a/templates/repo/issue/view_content/attachments.tmpl
+++ b/templates/repo/issue/view_content/attachments.tmpl
@@ -1,11 +1,11 @@
<div class="dropzone-attachments">
{{if .Attachments}}
- <div class="ui clearing divider"></div>
+ <div class="ui divider"></div>
{{end}}
- <div class="ui middle aligned padded grid">
- {{$hasThumbnails := false}}
- {{- range .Attachments -}}
- <div class="twelve wide column" style="padding: 6px;">
+ {{$hasThumbnails := false}}
+ {{- range .Attachments -}}
+ <div class="gt-df">
+ <div class="gt-f1 gt-p-3">
<a target="_blank" rel="noopener noreferrer" href="{{.DownloadURL}}" title='{{$.ctx.locale.Tr "repo.issues.attachment.open_tab" .Name}}'>
{{if FilenameIsImage .Name}}
{{if not (containGeneric $.Content .UUID)}}
@@ -18,14 +18,14 @@
<span><strong>{{.Name}}</strong></span>
</a>
</div>
- <div class="four wide column" style="padding: 0px;">
+ <div class="gt-p-3 gt-df gt-ac">
<span class="ui text grey right">{{.Size | FileSize}}</span>
</div>
- {{end -}}
- </div>
+ </div>
+ {{end -}}
{{if $hasThumbnails}}
- <div class="ui clearing divider"></div>
+ <div class="ui divider"></div>
<div class="ui small thumbnails">
{{- range .Attachments -}}
{{if FilenameIsImage .Name}}
diff --git a/web_src/js/features/repo-legacy.js b/web_src/js/features/repo-legacy.js
index 6bb1a67ee6..9fda108813 100644
--- a/web_src/js/features/repo-legacy.js
+++ b/web_src/js/features/repo-legacy.js
@@ -333,20 +333,19 @@ async function onEditContent(event) {
let dz;
const $dropzone = $editContentZone.find('.dropzone');
if ($dropzone.length === 1) {
- $dropzone.data('saved', false);
-
- const fileUuidDict = {};
- dz = await createDropzone($dropzone[0], {
- url: $dropzone.data('upload-url'),
+ let disableRemovedfileEvent = false; // when resetting the dropzone (removeAllFiles), disable the "removedfile" event
+ let fileUuidDict = {}; // to record: if a comment has been saved, then the uploaded files won't be deleted from server when clicking the Remove in the dropzone
+ const dz = await createDropzone($dropzone[0], {
+ url: $dropzone.attr('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'),
+ maxFiles: $dropzone.attr('data-max-file'),
+ maxFilesize: $dropzone.attr('data-max-size'),
+ acceptedFiles: (['*/*', ''].includes($dropzone.attr('data-accepts'))) ? null : $dropzone.attr('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'),
+ dictDefaultMessage: $dropzone.attr('data-default-message'),
+ dictInvalidFileType: $dropzone.attr('data-invalid-input-type'),
+ dictFileTooBig: $dropzone.attr('data-file-too-big'),
+ dictRemoveFile: $dropzone.attr('data-remove-file'),
timeout: 0,
thumbnailMethod: 'contain',
thumbnailWidth: 480,
@@ -359,9 +358,10 @@ async function onEditContent(event) {
$dropzone.find('.files').append(input);
});
this.on('removedfile', (file) => {
+ if (disableRemovedfileEvent) return;
$(`#${file.uuid}`).remove();
- if ($dropzone.data('remove-url') && !fileUuidDict[file.uuid].submitted) {
- $.post($dropzone.data('remove-url'), {
+ if ($dropzone.attr('data-remove-url') && !fileUuidDict[file.uuid].submitted) {
+ $.post($dropzone.attr('data-remove-url'), {
file: file.uuid,
_csrf: csrfToken,
});
@@ -373,20 +373,25 @@ async function onEditContent(event) {
});
});
this.on('reload', () => {
- $.getJSON($editContentZone.data('attachment-url'), (data) => {
+ $.getJSON($editContentZone.attr('data-attachment-url'), (data) => {
+ // do not trigger the "removedfile" event, otherwise the attachments would be deleted from server
+ disableRemovedfileEvent = true;
dz.removeAllFiles(true);
$dropzone.find('.files').empty();
- $.each(data, function () {
- const imgSrc = `${$dropzone.data('link-url')}/${this.uuid}`;
- dz.emit('addedfile', this);
- dz.emit('thumbnail', this, imgSrc);
- dz.emit('complete', this);
- dz.files.push(this);
- fileUuidDict[this.uuid] = {submitted: true};
+ fileUuidDict = {};
+ disableRemovedfileEvent = false;
+
+ for (const attachment of data) {
+ const imgSrc = `${$dropzone.attr('data-link-url')}/${attachment.uuid}`;
+ dz.emit('addedfile', attachment);
+ dz.emit('thumbnail', attachment, imgSrc);
+ dz.emit('complete', attachment);
+ dz.files.push(attachment);
+ fileUuidDict[attachment.uuid] = {submitted: true};
$dropzone.find(`img[src='${imgSrc}']`).css('max-width', '100%');
- const input = $(`<input id="${this.uuid}" name="files" type="hidden">`).val(this.uuid);
+ const input = $(`<input id="${attachment.uuid}" name="files" type="hidden">`).val(attachment.uuid);
$dropzone.find('.files').append(input);
- });
+ }
});
});
},