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.

Sign up for GitHub

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

Jump to bottom

[API] Allow removing issues #18879

Merged
merged 21 commits into from
Mar 1, 2022
Merged

[API] Allow removing issues #18879

merged 21 commits into from
Mar 1, 2022

Conversation

fnetX
Copy link
Contributor

@fnetX fnetX commented Feb 24, 2022

First steps to address #18056 and #923. I hope to get to a simple UI, too, but I'm looking for feedback for this technical aspects.

fnetX added 2 commits February 24, 2022 17:09
@fnetX
Allow removing issues via API
5da7adc 
Signed-off-by: fnetx <git@fralix.ovh>
@fnetX
Fixup: Swagger
166002b 
Signed-off-by: fnetx <git@fralix.ovh>
fnetX
fnetX commented Feb 24, 2022
View reviewed changes
models/issue.go Outdated Show resolved Hide resolved
models/issue.go Outdated Show resolved Hide resolved
services/issue/issue.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 24, 2022
@fnetX
Copy link
Contributor Author

fnetX commented Feb 24, 2022

grafik

Please note that references like these are not cleaned up, I looked in every plausible database table, but didn't find the place they are stored. Are they stored in the issue indexer? Is it important to clean them up, too?

@fnetX
Copy link
Contributor Author

fnetX commented Feb 24, 2022

Ah it's also in the comments table, my database viewer didn't properly reload. Still, I wonder why this isn't removed.

@6543 6543 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 24, 2022
@6543 6543 added this to the 1.17.0 milestone Feb 24, 2022
fnetX added 3 commits February 25, 2022 12:49
@fnetX
Simple test case for most important failures
d66325e 
Leftover attachments and issues that can't be closed would probably be
the most important issues. Everything else should be cleaned up by
consistency checks in case it really fails.
@fnetX
Merge 'gitea/main' into fnetx-delete-issue
23470b3
@fnetX
remove more leftover data
fe4812e
@fnetX
Copy link
Contributor Author

fnetX commented Feb 25, 2022

Currently not properly cleaned up: actions. They show up as broken in the user dashboard ("500 when get issue"). In the deleteComment function they are set to "is_deleted" here: https://github.com/go-gitea/gitea/blob/main/models/issue_comment.go#L1172

This touches my first review question. Should I

  • reuse the deleteComment function and remove the redundant calls to removing comment attachments, reactions etc
  • move some or all cleanup tasks to the AfterDelete function for the comments
  • set the is_deleted flag manually for the actions
  • completely drop the actions (what use to clutter the db with "is_deleted" comments?)

@lunny
Copy link
Member

lunny commented Feb 26, 2022

I have sent a commit ref. https://github.com/fnetX/gitea/pull/2/commits/8cfb88cd594509c7bbcbd94f2e4db8db0b062c03

Gusted
Gusted approved these changes Feb 26, 2022
View reviewed changes
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 26, 2022
@Gusted
Copy link
Contributor

Gusted commented Feb 26, 2022

Dammit, mis-click. Didn't approve the PR.

@fnetX
Copy link
Contributor Author

fnetX commented Feb 27, 2022

@lunny thank you, just got to reading your commit. I left a comment in the commit regarding the deleteIn() function. I hope you can read it.

@fnetX fnetX requested a review from Gusted February 28, 2022 14:28
Gusted
Gusted reviewed Feb 28, 2022
View reviewed changes
routers/api/v1/api.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
@fnetX
Copy link
Contributor Author

fnetX commented Feb 28, 2022

The "Issue opened" action is not deleted from the table yet. I think it would be okay to keep it (it includes the title and doesn't fail, also it might be interesting that a user opened a certain issue. OTOH, it includes the title and thus maybe information people want to get rid of.

I don't really know how to cleanly remove this? Maybe the repo ID and then where content starts with "issue_id|" so it matches e.g. "5|Issue title"? Or is there already a convenient function for this? Didn't find one. Should I also try to match the op_type, or can this be any?

fnetX and others added 2 commits February 28, 2022 15:38
@fnetX
reqAdmin() for DeleteIssue route
117547b 
Co-authored-by: Gusted <williamzijl7@hotmail.com>
@fnetX
Update routers/api/v1/repo/issue.go
879e4f9 
Co-authored-by: Gusted <williamzijl7@hotmail.com>
fnetX
fnetX commented Feb 28, 2022
View reviewed changes
models/issue.go Outdated Show resolved Hide resolved
@Gusted
Copy link
Contributor

Gusted commented Feb 28, 2022

Should I also try to match the op_type, or can this be any?

I think the op_type's are the way to go. There a quite a few which relates to issues.

@fnetX fnetX changed the title WIP: [API] Allow removing issues [API] Allow removing issues Feb 28, 2022
6543
6543 requested changes Feb 28, 2022
View reviewed changes
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
services/issue/issue.go Outdated Show resolved Hide resolved
modules/notification/base/notifier.go Outdated Show resolved Hide resolved
@6543 6543 added the modifies/api This PR adds API routes or modifies them label Feb 28, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 28, 2022
@6543 6543 merged commit 062fd4c into go-gitea:main Mar 1, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 1, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
20bb80b 
* giteaofficial/main:
  [API] Allow removing issues (go-gitea#18879)
  Refactor SecToTime() function (go-gitea#18863)
  Improve mirror iterator (go-gitea#18928)
  Fix login with email panic when email is not exist (go-gitea#18941)
Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Mar 1, 2022
@fnetX @lunny @6543
CB/bp: [API] Allow removing issues (go-gitea#18879)
732d5e5 
Add new feature to delete issues and pulls via API

Co-authored-by: fnetx <git@fralix.ovh>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: 6543 <6543@obermui.de>
@fnetX fnetX mentioned this pull request Mar 8, 2022
Merged
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@fnetX @lunny @6543
[API] Allow removing issues (go-gitea#18879)
d7b6ab9 
Add new feature to delete issues and pulls via API

Co-authored-by: fnetx <git@fralix.ovh>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Gusted <williamzijl7@hotmail.com>
Co-authored-by: 6543 <6543@obermui.de>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Gusted Gusted Gusted approved these changes

@lunny lunny lunny left review comments

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
5 participants
@fnetX @lunny @Gusted @6543 @GiteaBot