Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API] Add issue and comment attachments #14601

Conversation

andrebruch
Copy link

@andrebruch andrebruch commented Feb 7, 2021

The following API routes have been added:

Issues

  • GET /repos/{owner}/{repo}/issues/assets/{attachment_id}
  • GET /repos/{owner}/{repo}/issues/{index}/assets
  • POST /repos/{owner}/{repo}/issues/{index}/assets
  • PATCH /repos/{owner}/{repo}/issues/assets/{attachment_id}
  • DELETE /repos/{owner}/{repo}/issues/assets/{attachment_id}

Comments

  • GET /repos/{owner}/{repo}/issues/comments/assets/{attachment_id}
  • GET /repos/{owner}/{repo}/issues/comments/{id}/assets
  • POST /repos/{owner}/{repo}/issues/comments/{id}/assets
  • PATCH /repos/{owner}/{repo}/issues/comments/assets/{attachment_id}
  • DELETE /repos/{owner}/{repo}/issues/comments/assets/{attachment_id}

close #3690

@6543 6543 added modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Feb 7, 2021
@6543 6543 added this to the 1.15.0 milestone Feb 7, 2021
@zeripath
Copy link
Contributor

zeripath commented Feb 7, 2021

Please run make generate-swagger and commit the changes

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 7, 2021
modules/convert/issue.go Outdated Show resolved Hide resolved
}

if attach.IssueID == 0 {
log.Info("User requested attachment is not in issue, attachment_id: %v", attachID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure about this logging level. I suspect it's too high but I can see you don't want people to crawl it...

@andrebruch
Copy link
Author

Please run make generate-swagger and commit the changes

The swagger spec is already there.

@jolheiser
Copy link
Member

Currently the linter is mad because it's detecting duplicate code.
https://drone.gitea.io/go-gitea/gitea/35748/1/4

@andrebruch
Copy link
Author

The code is quite similar, but not the same.

@@ -5,6 +5,7 @@
package repo

import (
issue_service "code.gitea.io/gitea/services/issue"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import order.

@@ -1673,17 +1673,17 @@ func UpdateIssueContent(ctx *context.Context) {
return
}

files := ctx.QueryStrings("files[]")
if err := updateAttachments(issue, files); err != nil {
ctx.ServerError("UpdateAttachments", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return missed

@andrebruch andrebruch closed this Mar 13, 2021
@andrebruch andrebruch force-pushed the feature-add-issue-and-comment-attachments-api branch from 29e9315 to 240fea8 Compare March 13, 2021 01:58
@andrebruch andrebruch reopened this Mar 13, 2021
@andrebruch
Copy link
Author

It looks like a new line in the v1_json.tmpl file solved the duplicate code and make generate-swagger issues.

The make generate-swagger and make swagger-check commands remove the last line in the v1_json.tmpl file on my system.

@andrebruch
Copy link
Author

The following test fails:
testing-amd64 > test-mssql > TestGit/HTTP/MergeFork/CreatePRAndMerge

I just run the tests locally on a MSSQL database and there were no problems.

@6543
Copy link
Member

6543 commented Apr 15, 2021

@andrebruch would be nice if you could resolve conflicts and adopt related changes :)

@mattrpav
Copy link

Checking in-- how's this looking for inclusion with a 1.17.0 or a 1.16.x release?

@6543
Copy link
Member

6543 commented Apr 21, 2022

1.17.0 ...

@6543
Copy link
Member

6543 commented Apr 21, 2022

... I can have a look at finishing it - next week?!?

modules/context/api.go Outdated Show resolved Hide resolved
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow my comments.

}

func canUserWriteIssueCommentAttachment(ctx *context.APIContext, c *models.Comment) (success bool) {
canEditComment := ctx.Doer.ID == c.PosterID || ctx.IsUserRepoAdmin() || ctx.IsUserSiteAdmin()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check if user has a write permission to the issue.

@lunny lunny modified the milestones: 1.17.0, 1.18.0 May 25, 2022
@mattrpav
Copy link

Friendly comment to bubble up. Thanks all!

@mattrpav
Copy link

Hi @6543 @lunny checking in. Any update to the plans for this feature request?

@lunny
Copy link
Member

lunny commented Oct 12, 2022

The review comments should be followed and conflict files should be resolved.

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022
@6543
Copy link
Member

6543 commented Oct 19, 2022

original contributor is not responding so everybody looking at this pull and want it ...

if you have time, checkout to the pull-head and create a new branch on you own fork and submit a pull ... then fix all open mentioned issues and we will merge that one :)

@mattrpav
Copy link

Hi @6543 @lunny @andrebruch - Thanks for the follow-ups. I'm not a Go programmer, so I don't think anyone wants to see me tackle the patch at this point ;-). Is there a tip jar or other option to financially back this feature?

I have committed to writing a JIRA-to-Gitea migration tool and publish blog for others to benefit-- and we are happy to kick in money for a Gitea/Go programmer to finish out this PR.

Also, this feature will be needed by any organization that wants to use Gitea and maintain automated security compliance (SOC2, ISO27001, etc).

Thanks!

@lunny
Copy link
Member

lunny commented Nov 12, 2022

Hi @6543 @lunny @andrebruch - Thanks for the follow-ups. I'm not a Go programmer, so I don't think anyone wants to see me tackle the patch at this point ;-). Is there a tip jar or other option to financially back this feature?

I have committed to writing a JIRA-to-Gitea migration tool and publish blog for others to benefit-- and we are happy to kick in money for a Gitea/Go programmer to finish out this PR.

Also, this feature will be needed by any organization that wants to use Gitea and maintain automated security compliance (SOC2, ISO27001, etc).

Thanks!

Thank you very much! You can contribute in bountysource for #3690 but even that I cannot guarantee this could be resolved in a short days.

@mattrpav
Copy link

mattrpav commented Nov 12, 2022

@lunny This looks like the Bountysource link for #3690 and I contributed $100 (USD).

How much $ do you think to have it completed by December 31st?

@lunny
Copy link
Member

lunny commented Nov 14, 2022

Let's continue in #21783

@lunny lunny closed this Nov 14, 2022
@lunny lunny removed this from the 1.19.0 milestone Nov 14, 2022
lunny added a commit that referenced this pull request Dec 9, 2022
Close #14601
Fix #3690

Revive of #14601.
Updated to current code, cleanup and added more read/write checks.

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andre Bruch <ab@andrebruch.com>
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Norwin <git@nroo.de>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue API and Attachments