Enable assignee e-mail notification #2003
Conversation
|
It seems it's difficult to add a test for it. |
tboerger
added
the
lgtm/need 2
|
I would like to close this PR and create new one, because the doer.ID check is missing so assignee receive self comments too. Or I could simply push the fix in this PR? |
|
Just to push. |
|
One more mistake, because assignee is not mandatory it maybe empty, issue.Assignee != nil check enough or issue.AssigneID != 0 or better to use GetUserByID()? |
models/issue_mail.go
Outdated
| @@ -42,6 +42,11 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, comment *Comment, | |||
| participants = append(participants, issue.Poster) | |||
| } | |||
|
|
|||
| // Assignee must receive any communications | |||
| if issue.AssigneeID != doer.ID { | |||
| participants = append(participants, issue.Assignee) | |||
There was a problem hiding this comment.
Only when no assignee defined, so sanity check with GetUserByID() pushed.
There was a problem hiding this comment.
I have not checked too deeply but if there is error in issue loadAttributes, it would probably continue and leave issue.Assignee as nil. I would suggest adding additional check for && issue.Assignee != nil to be on safe side just in case
There was a problem hiding this comment.
Check against nil added with latest commit.
models/issue_mail.go
Outdated
| @@ -42,6 +42,15 @@ func mailIssueCommentToParticipants(issue *Issue, doer *User, comment *Comment, | |||
| participants = append(participants, issue.Poster) | |||
| } | |||
|
|
|||
| // Assignee must receive any communications | |||
| if issue.AssigneeID != doer.ID { | |||
| to, err := GetUserByID(issue.AssigneeID) | |||
There was a problem hiding this comment.
What if the assignee has been deleted? GetUserByID(issue.AssigneeID) will return an error, but we would want to use a ghost user in this case.
Maybe we should add an Issue.loadAssignee(..) method similar to Issue.loadPoster(..). This would also save us from loading the assignee if the assignee has already been loaded.
There was a problem hiding this comment.
Commited the loadAssignee() method.
|
LGTM |
tboerger
added
lgtm/need 1
|
LGTM |
tboerger
added
lgtm/done
|
@ethantkoenig needs your approval |
Assignee right now doesn't get any notifications when new comment or when got a new issue.