-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 to partecipants in issue comments #1217
Conversation
@@ -48,6 +54,16 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, mentions []string) | |||
tos = append(tos, to.Email) | |||
names = append(names, to.Name) | |||
} | |||
for i := range participants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore self on watchers too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already done in the file, a few lines above (see the full file)
It's done one line below:
```
if participants[i].ID == doer.ID {
continue
```
|
I mean |
Done also for `watchers`:
```
for i := range watchers {
if watchers[i].UserID == doer.ID {
continue
}
```
|
Where is the commit? |
It was already in the file, so not changed by the commit. |
Did some testing on 'issue notifications'. For the most part, it worked, although I wasn't able to reproduce an issue on master, some emails were delayed and issue counts for the issue creator were out of sync (but eventually synchronized). Using two accounts & two email accounts (one gmail, one yahoo, sparkpost relay), creator of issue is a participant, repo owner is a participant. confirmed that emails are sent to the other participant when a change is made (creator opens an issue- email sent to repo owner, not to creator; repo owner comments, email sent to creator, not to repo owner). Let me know if there's something else you'd like tested. |
LGTM, I just made some simple tests, it works. |
But maybe you didn't consider one situation. Issue creators seem will not receive emails? Except this. |
line 86 should has a bug. the poster maybe the comment poster not the issue poster. So poster should be a param to func (issue *Issue) MailParticipants() (err error) { |
@lunny actually I don't understand the issue. Are you saying that the issue poster should not receive an email notification when someone comments on the issue ? |
@@ -1133,6 +1133,23 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) { | |||
return issues, nil | |||
} | |||
|
|||
// GetParticipantsByIssueID returns all users who are participated in comments of an issue. | |||
func GetParticipantsByIssueID(issueID int64) ([]*User, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add unit tests for this new function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look into it as time permits (possibly next week)
@lunny @unkwon actually confirmed the bug and reports it as fixed by gogs/gogs@5f058c3 Great reviews, thanks everyone ! |
Trusted LGTM |
@strk I don't think that will fix the problem entirely, I will test this tomorrow again. |
192ca213f08b4d613c45bce22b78d7497f342ff5 stubs an initial test, and fails revealing still a bug ! I don't know what @unknwon idea was for that function (GetParticipantsByIssueID), but to me it does make sense for the original poster to be among the partecipants... |
e25eb54
to
d3b2555
Compare
So basically what's the failing test telling us is that the current |
Closes go-gitea#1216 Includes test (still failing)
98ce0b7
to
7770335
Compare
Fix test to expect what GetParticipants return
@@ -1134,6 +1134,24 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) { | |||
return issues, nil | |||
} | |||
|
|||
// GetParticipantsByIssueID returns all users who are participated in comments of an issue. | |||
func GetParticipantsByIssueID(issueID int64) ([]*User, error) { | |||
userIDs := make([]int64, 0, 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does happen if participants are more than 5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing.
let L-G-T-M work |
Closes #1216
NOTE: I didn't test this with GUI notifications so might need a review
from @andreynering to deal with that -- this is just a straight port
from the Gogs commit