Skip to content

Ensure delete user deletes all comments #21067

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
Merged
lafriks merged 3 commits into from
Sep 5, 2022
Merged

Ensure delete user deletes all comments #21067

lafriks merged 3 commits into from
Sep 5, 2022

Conversation

@lunny
Copy link
Member

@lunny lunny commented Sep 5, 2022 •
edited by zeripath
Loading

There is a mistake in the batched delete comments part of DeleteUser which causes some comments to not be deleted

The code incorrectly updates the start of the limit clause resulting in most comments not being deleted.

			if err = e.Where("type=? AND poster_id=?", issues_model.CommentTypeComment, u.ID).Limit(batchSize, start).Find(&comments); err != nil {

should be:

			if err = e.Where("type=? AND poster_id=?", issues_model.CommentTypeComment, u.ID).Limit(batchSize, 0).Find(&comments); err != nil {

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. backport/v1.17 labels Sep 5, 2022
@lunny lunny added this to the 1.18.0 milestone Sep 5, 2022
@lunny lunny requested a review from 6543 September 5, 2022 02:59
@lunny lunny mentioned this pull request Sep 5, 2022
Merged
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 5, 2022
zeripath
zeripath reviewed Sep 5, 2022
View reviewed changes
@zeripath zeripath added the backport/done All backports for this PR have been created label Sep 5, 2022
@zeripath
Copy link
Contributor

zeripath commented Sep 5, 2022

The DeleteComment code is really quite inefficient, especially the neuter cross-references code. This really should all be done within the DB. There is no need to get this stuff out of the DB.

zeripath
zeripath approved these changes Sep 5, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Approved as this fixes a serious bug - but see my comment about the serious inefficiencies in this code.

@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 Sep 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2022

Codecov Report

Merging #21067 (4a6c96e) into main (b42aaf2) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #21067      +/-   ##
==========================================
- Coverage   47.13%   47.03%   -0.10%     
==========================================
  Files        1008     1007       -1     
  Lines      137718   137581     -137     
==========================================
- Hits        64911    64716     -195     
- Misses      64870    64935      +65     
+ Partials     7937     7930       -7
Impacted Files Coverage Δ
models/user.go 32.84% <0.00%> (ø)
modules/sync/status_pool.go 64.00% <0.00%> (-36.00%) ⬇️
services/user/user.go 17.36% <0.00%> (-30.00%) ⬇️
modules/process/manager_exec.go 58.13% <0.00%> (-27.91%) ⬇️
modules/context/xsrf.go 78.04% <0.00%> (-14.64%) ⬇️
modules/secret/secret.go 52.00% <0.00%> (-12.00%) ⬇️
modules/charset/escape_stream.go 58.68% <0.00%> (-8.93%) ⬇️
modules/util/filebuffer/file_backed_buffer.go 52.50% <0.00%> (-6.25%) ⬇️
modules/charset/charset.go 71.73% <0.00%> (-2.18%) ⬇️
modules/log/file.go 68.78% <0.00%> (-1.92%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lafriks lafriks merged commit bc4cce1 into go-gitea:main Sep 5, 2022
@lunny lunny deleted the lunny/fix_delete_user branch September 5, 2022 22:47
lunny added a commit that referenced this pull request Sep 5, 2022
@lunny
Fix delete user missed some comments (#21067) (#21068)
084797b
@zeripath zeripath changed the title Fix delete user missed some comments Ensure delete user deletes all comments Sep 6, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 6, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
b38d134 
* upstream/main:
  Fix sub folder in repository missing add file dropdown (go-gitea#21069)
  [skip ci] Updated translations via Crowdin
  Add missing volume to test-e2e (go-gitea#21079)
  Fix delete user missed some comments (go-gitea#21067)
  Remove insecure flag from curl (go-gitea#21074)
  Update curl usage in API docs (go-gitea#21071)
  Move go-licenses to generate and separate generate into a frontend and backend component (go-gitea#21061)
vanhoang1107 added a commit to vanhoang1107/gitea that referenced this pull request Oct 31, 2022
@vanhoang1107
Merge remote-tracking branch 'src/release/v1.17' into release/v1.17
d7bf1a9 
* src/release/v1.17: (26 commits)
  Fix reaction of issues (go-gitea#21185) (go-gitea#21196)
  Fix CSV diff for added/deleted files (go-gitea#21189) (go-gitea#21193)
  Fix pagination limit parameter problem (go-gitea#21111)
  Add MD5 back to template helper functions to avoid breaking (go-gitea#21102)
  Add changelog for v1.17.2 (go-gitea#21089)
  Fix sub folder in repository missing add file dropdown (go-gitea#21069) (go-gitea#21083)
  Fix hard-coded timeout and error panic in API archive download endpoint (go-gitea#20925) (go-gitea#21051)
  Fix delete user missed some comments (go-gitea#21067) (go-gitea#21068)
  Delete unreferenced packages when deleting a package version (go-gitea#20977) (go-gitea#21060)
  Redirect if user does not exist on admin pages (go-gitea#20981) (go-gitea#21059)
  Set uploadpack.allowFilter etc on gitea serv to enable partial clones with ssh (go-gitea#20902) (go-gitea#21058)
  Fix 500 on time in timeline API (go-gitea#21052) (go-gitea#21057)
  Fill the specified ref in webhook test payload (go-gitea#20961) (go-gitea#21055)
  Add another index for Action table on postgres (go-gitea#21033) (go-gitea#21054)
  fix broken insecureskipverify handling in rediss connection uris (go-gitea#20967) (go-gitea#21053)
  Add more checks in migration code (go-gitea#21011) (go-gitea#21050)
  Add Dev, Peer and Optional dependencies to npm PackageMetadataVersion (go-gitea#21017) (go-gitea#21044)
  Improve arc-green code theme (go-gitea#21039) (go-gitea#21042)
  Add down key check has tribute container (go-gitea#21016) (go-gitea#21038)
  Do not add links to Posters or Assignees with ID < 0 (go-gitea#20577) (go-gitea#21037)
  ...
@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.

Reviewers

@jolheiser jolheiser jolheiser approved these changes

@6543 6543 Awaiting requested review from 6543

+1 more reviewer

@zeripath zeripath zeripath approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug

Projects

None yet

Milestone

1.18.0

Development

Successfully merging this pull request may close these issues.

6 participants

@lunny @zeripath @codecov-commenter @jolheiser @lafriks @GiteaBot