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.
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
```
|
tboerger
added
the
lgtm/need 2
|
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. |
tboerger
added
lgtm/need 1
|
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.
Could we add unit tests for this new function?
There was a problem hiding this comment.
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. |
tboerger
added
lgtm/done
|
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... |
strk
force-pushed
the
mailnotify-issue-partecipants
branch
from
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)
strk
force-pushed
the
mailnotify-issue-partecipants
branch
from
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.
What does happen if participants are more than 5?
|
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