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

Ignore missing comment for user notifications #18954

Merged
merged 8 commits into from
Mar 3, 2022

Conversation

eladyn
Copy link
Contributor

@eladyn eladyn commented Mar 1, 2022

This extends #17550 to another place, where an internal server error may occur, when a notification is pointing to a deleted comment. (In fact, this fixes the original issue in #17499, which occurred in the user notification endpoint.)

Tested and is working for me.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 1, 2022
@Gusted Gusted added this to the 1.17.0 milestone Mar 1, 2022
routers/api/v1/notify/user.go Outdated Show resolved Hide resolved
@fnetX
Copy link
Contributor

fnetX commented Mar 1, 2022

What about instead making sure to remove notifications when deleting issues, comments, repos etc? I also didn't consider this with my recent issue removal move, but I think it's even better?

@eladyn
Copy link
Contributor Author

eladyn commented Mar 1, 2022

@fnetX This would definitely be cleaner in general, but I imagine that this would also involve a bit more than a one-line fix (:sweat_smile:), which my golang / gitea knowledge probably won't suffice to tackle.

You would also have to consider that deleting a comment doesn't necessarily mean that you can delete the whole notification, since the user might still have unread comments in that issue that were written before the deleted comment. So it may be necessary to keep track of the read status of individual items in the timeline of an issue / PR to do this properly.

Apart from all that, as I said, this is probably out of scope for me / this PR (which just aims to fix the 500).

@fnetX
Copy link
Contributor

fnetX commented Mar 1, 2022

Yeah, I mainly considered if I should try to get some tiny fixes into my #18953, but you are right about the read state. Removing notifications for removed repos and whole issues should be safe and somewhat easy, I can attempt that.

@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 2, 2022
@6543 6543 merged commit 7a893da into go-gitea:main Mar 3, 2022
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Mar 3, 2022
* main:
  ignore missing comment for user notifications (go-gitea#18954)
  allow overwrite artifacts for github releases (go-gitea#18987)
  fix & refactor (go-gitea#18973)
  Don't clean up hardcoded `tmp` (go-gitea#18983)
  git backend ignore replace objects (go-gitea#18979)
  Improve the deletion of issue (go-gitea#18945)
  Add note to GPG key response if user has no keys (go-gitea#18961)
  adds restore docs for docker based instances (go-gitea#18844)
  Refactor admin user filter query parameters (go-gitea#18965)
@eladyn eladyn deleted the fix_notifications_missing_comment branch March 3, 2022 16:10
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 6, 2022
* giteaofficial/main:
  Fix EasyMDE error when input Enter (go-gitea#19004)
  Fix update hint bug (go-gitea#18996)
  Fix the editor height in review box (go-gitea#19003)
  Add a "admin user generate-access-token" subcommand (go-gitea#17722)
  Fix potential assignee query for repo (go-gitea#18994)
  Add config option to disable "Update branch by rebase" (go-gitea#18745)
  Update `go-enry` to v2.8.0 (go-gitea#18993)
  homebrew updates via cron
  ignore missing comment for user notifications (go-gitea#18954)
  allow overwrite artifacts for github releases (go-gitea#18987)
  fix & refactor (go-gitea#18973)
zeripath pushed a commit to zeripath/gitea that referenced this pull request Mar 9, 2022
Backport go-gitea#18954

* ignore missing comment for user notifications

* instead fix bug in notifications model

* use local variable instead

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: 6543 <6543@obermui.de>
@zeripath zeripath changed the title ignore missing comment for user notifications Ignore missing comment for user notifications Mar 9, 2022
@zeripath zeripath added the backport/done All backports for this PR have been created label Mar 9, 2022
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* ignore missing comment for user notifications

* instead fix bug in notifications model

* use local variable instead

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.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.
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. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants