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

Send notifications for mentions in pull request code comments #14218

Merged
merged 4 commits into from
Jan 2, 2021

Conversation

jpraet
Copy link
Member

@jpraet jpraet commented Jan 2, 2021

Fixes #14187: mention handling extracted from email notification code
Fixes #14013: add notification for mentions in pull request code comments
Fixes #13450: Not receiving any emails with setting "Only Email on Mention"

@lunny lunny added this to the 1.14.0 milestone Jan 2, 2021
@lunny lunny added the type/enhancement An improvement of existing functionality label Jan 2, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 2, 2021
@codecov-io
Copy link

codecov-io commented Jan 2, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@ac88b0e). Click here to learn what that means.
The diff coverage is 48.99%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #14218   +/-   ##
=========================================
  Coverage          ?   41.98%           
=========================================
  Files             ?      735           
  Lines             ?    78881           
  Branches          ?        0           
=========================================
  Hits              ?    33117           
  Misses            ?    40313           
  Partials          ?     5451           
Impacted Files Coverage Δ
modules/notification/ui/ui.go 62.96% <12.50%> (ø)
modules/notification/base/null.go 77.77% <40.00%> (ø)
modules/notification/mail/mail.go 38.88% <42.85%> (ø)
services/mailer/mail_issue.go 62.65% <44.44%> (ø)
models/issue.go 57.01% <50.00%> (ø)
services/comments/comments.go 69.23% <50.00%> (ø)
services/issue/issue.go 32.87% <50.00%> (ø)
services/pull/pull.go 42.28% <50.00%> (ø)
services/pull/review.go 50.98% <50.00%> (ø)
services/mailer/mail_comment.go 75.00% <69.56%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac88b0e...c48006d. Read the comment docs.

@lunny
Copy link
Member

lunny commented Jan 2, 2021

May also resolved #13450

@jpraet
Copy link
Member Author

jpraet commented Jan 2, 2021

May also resolved #13450

I tested and it doesn't solve that problem. It is because the visited map[int64]bool is populated even when the notification wasn't sent. So when sending the mail to the mentioned users it thinks an email notification has already been sent. Easiest fix is probably to just switch the order around in mailIssueCommentToParticipants: first send to mentions, then to other participants?

I notice something else: you can explicitly unsubscribe from an issue to no longer receive notifications from it. However, in the case where someone explicitly mentions you, shouldn't we still send a notification?

@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 Jan 2, 2021
@lunny
Copy link
Member

lunny commented Jan 2, 2021

Unsubcribe should not affect mention. But user has an option about mail notification. It could be all, mentioned and none.

@jpraet
Copy link
Member Author

jpraet commented Jan 2, 2021

#13450 should be fixed now as well. And unsubscribe no longer affects mentions.

@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 Jan 2, 2021
@6543 6543 merged commit e6acce6 into go-gitea:master Jan 2, 2021
@6543
Copy link
Member

6543 commented Jan 2, 2021

@jpraet pleace send backport :)

jpraet added a commit to jpraet/gitea that referenced this pull request Jan 2, 2021
@techknowlogick techknowlogick added the backport/done All backports for this PR have been created label Jan 2, 2021
lafriks pushed a commit that referenced this pull request Jan 3, 2021
@jpraet jpraet deleted the mentions branch January 22, 2021 18:38
@go-gitea go-gitea locked and limited conversation to collaborators Feb 11, 2021
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 type/enhancement An improvement of existing functionality
Projects
None yet
6 participants