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

Delete related notifications on issue deletion too #18953

Merged
merged 4 commits into from
Mar 17, 2022
Merged

Delete related notifications on issue deletion too #18953

merged 4 commits into from
Mar 17, 2022

Conversation

fnetX
Copy link
Contributor

@fnetX fnetX commented Mar 1, 2022

as title

@lunny lunny added the type/bug label Mar 1, 2022
models/repo.go Outdated Show resolved Hide resolved
models/issue_comment.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 1, 2022
lunny
lunny previously requested changes Mar 1, 2022
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.

@fnetX fnetX requested a review from lunny March 17, 2022 13:12
@fnetX
Copy link
Contributor Author

fnetX commented Mar 17, 2022

Hey, I dropped the Action removal (will do this in a separate PR). The only thing left I wanted to do in this PR is to remove the notifications for removed issues.

Please note this is actually a bug I'd either backport that or the whole PR). I thought with #18954 this wouldn't be too weird, but we are facing the bug on Codeberg.

image
DySpgXOoQykPZLKeOVsgDYUR

I wonder if I should attempt writing a migration to remove eventual left over notifications. But I'm not sure how migrations can be backported (if at all), because they have a linear counter. If a migration is backported, it probably either shares an id with another important upcoming migration, or increments the counter so that all migrations until then are skipped. Or do I misunderstand the system?

Also note that notifications are also not removed for deleted comments already. I noticed this in the past already, it's a bug on its own (and I'm not going to address this, because I currently can't think of a solution to see if a still needs a notification for the issue (previous post) or has already read the notification. Probably with the issue read state?).

@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 Mar 17, 2022
@6543 6543 changed the title Tiny code adjustments Delete related notifications on issue deletion too Mar 17, 2022
@6543 6543 added this to the 1.17.0 milestone Mar 17, 2022
@6543 6543 dismissed lunny’s stale review March 17, 2022 16:02

outdated

@6543
Copy link
Member

6543 commented Mar 17, 2022

I should attempt writing a migration to remove eventual left over notifications

@fnetX the doctor subcommand is for this ...

@fnetX
Copy link
Contributor Author

fnetX commented Mar 17, 2022

But most people don't run it, do they? Anyway, I'm fine with not bothering. After all, since it's not live for long and not even released yet, it's unlikely many people have clicked the button.

@6543
Copy link
Member

6543 commented Mar 17, 2022

well yes - that's why a doctor task would be the best way to do so ... - we can merge this fix first and work in that afterwards

@fnetX
Copy link
Contributor Author

fnetX commented Mar 17, 2022

Oh, by the way, you can remove the backport label again. It's not in 1.16. I was confused about that earlier, too, because it's deployed on Codeberg. (So I'll backport anyway, but not for Gitea).

@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 Mar 17, 2022
@6543 6543 merged commit 04fcf23 into go-gitea:main Mar 17, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 19, 2022
* giteaoffical/main:
  remove not needed (go-gitea#19128)
  Add warning to set SENDMAIL_ARGS to --  (go-gitea#19102)
  Do not send activation email if manual confirm is set (go-gitea#19119)
  Update tool dependencies (go-gitea#19120)
  Delete related notifications on issue deletion too (go-gitea#18953)
  nit fix (go-gitea#19116)
  Store the foreign ID of issues during migration  (go-gitea#18446)
  Remove italics for `due_date_not_set` (go-gitea#19113)
Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Mar 20, 2022
* use .Decr for issue comment counting

* Remove notification on issue removal
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* use .Decr for issue comment counting

* Remove notification on issue removal
@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants