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

Mail assignee when issue/pull request is assigned #8546

Merged
merged 23 commits into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
fb0bb14
Send email to assigned user
davidsvantesson Oct 12, 2019
385c14d
Only send mail if enabled
davidsvantesson Oct 16, 2019
27bbc9d
Mail also when assigned through API
davidsvantesson Oct 16, 2019
b063676
Need to refactor functions from models to issue service
davidsvantesson Oct 16, 2019
1374db5
Refer to issue index rather than ID
davidsvantesson Oct 16, 2019
a3556c9
Merge branch 'master' into mail-assigned
davidsvantesson Oct 16, 2019
630dceb
Disable email notifications completly at initalization if global disable
davidsvantesson Oct 17, 2019
18187fa
Check of user enbled mail shall be in mail notification function only
davidsvantesson Oct 17, 2019
99f8db4
Initialize notifications from routers init function.
davidsvantesson Oct 18, 2019
ba31fed
Use the assigned comment when sending assigned mail
davidsvantesson Oct 18, 2019
04450f9
Refactor so that assignees always added as separate step when new iss…
davidsvantesson Oct 18, 2019
fea106d
Check error from AddAssignees
davidsvantesson Oct 18, 2019
5f6f77f
Check if user can be assiged to issue or pull request
davidsvantesson Oct 18, 2019
15b84ed
Missing return
davidsvantesson Oct 18, 2019
a7562d1
Refactor of CanBeAssigned check.
davidsvantesson Oct 19, 2019
075c7c6
Clarify function names (toggle rather than update/change), and clean up.
davidsvantesson Oct 19, 2019
e0b2648
Merge remote-tracking branch 'upstream/master' into mail-assigned
davidsvantesson Oct 19, 2019
0d5202c
Fix review comments.
davidsvantesson Oct 20, 2019
e761790
Merge branch 'master' into mail-assigned
lunny Oct 20, 2019
abc21a5
Flash error if assignees was not added when creating issue/pr
davidsvantesson Oct 21, 2019
430a3c8
Generate error if assignee users doesn't exist
davidsvantesson Oct 23, 2019
4c10b70
Merge branch 'master' into mail-assigned
lunny Oct 25, 2019
152fef9
Merge branch 'master' into mail-assigned
davidsvantesson Oct 25, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion models/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {

// Insert the assignees
for _, assigneeID := range opts.AssigneeIDs {
err = opts.Issue.changeAssignee(e, doer, assigneeID, true)
_, err = opts.Issue.changeAssignee(e, doer, assigneeID, true)
if err != nil {
return err
}
Expand Down
106 changes: 20 additions & 86 deletions models/issue_assignees.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,73 +110,62 @@ func clearAssigneeByUserID(sess *xorm.Session, userID int64) (err error) {
return
}

// AddAssigneeIfNotAssigned adds an assignee only if he isn't aleady assigned to the issue
func AddAssigneeIfNotAssigned(issue *Issue, doer *User, assigneeID int64) (err error) {
// Check if the user is already assigned
isAssigned, err := IsUserAssignedToIssue(issue, &User{ID: assigneeID})
if err != nil {
return err
}

if !isAssigned {
return issue.ChangeAssignee(doer, assigneeID)
}
return nil
}

// UpdateAssignee deletes or adds an assignee to an issue
func UpdateAssignee(issue *Issue, doer *User, assigneeID int64) (err error) {
return issue.ChangeAssignee(doer, assigneeID)
_, err = issue.ChangeAssignee(doer, assigneeID)
return err
}

// ChangeAssignee changes the Assignee of this issue.
func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) {
func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (removed bool, err error) {
sess := x.NewSession()
davidsvantesson marked this conversation as resolved.
Show resolved Hide resolved
defer sess.Close()

if err := sess.Begin(); err != nil {
return err
return false, err
}

if err := issue.changeAssignee(sess, doer, assigneeID, false); err != nil {
return err
removed, err = issue.changeAssignee(sess, doer, assigneeID, false)
if err != nil {
return false, err
}

if err := sess.Commit(); err != nil {
return err
return false, err
}

go HookQueue.Add(issue.RepoID)
return nil

return removed, nil
}

func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (err error) {
func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (removed bool, err error) {
// Update the assignee
removed, err := updateIssueAssignee(sess, issue, assigneeID)
removed, err = updateIssueAssignee(sess, issue, assigneeID)
davidsvantesson marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return fmt.Errorf("UpdateIssueUserByAssignee: %v", err)
return false, fmt.Errorf("UpdateIssueUserByAssignee: %v", err)
}

// Repo infos
if err = issue.loadRepo(sess); err != nil {
return fmt.Errorf("loadRepo: %v", err)
return false, fmt.Errorf("loadRepo: %v", err)
}

// Comment
if _, err = createAssigneeComment(sess, doer, issue.Repo, issue, assigneeID, removed); err != nil {
return fmt.Errorf("createAssigneeComment: %v", err)
return false, fmt.Errorf("createAssigneeComment: %v", err)
}

// if pull request is in the middle of creation - don't call webhook
if isCreate {
return nil
return removed, err
}

if issue.IsPull {
mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypePullRequests)

if err = issue.loadPullRequest(sess); err != nil {
return fmt.Errorf("loadPullRequest: %v", err)
return false, fmt.Errorf("loadPullRequest: %v", err)
}
issue.PullRequest.Issue = issue
apiPullRequest := &api.PullRequestPayload{
Expand All @@ -192,7 +181,7 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in
}
if err := prepareWebhooks(sess, issue.Repo, HookEventPullRequest, apiPullRequest); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The webhooks part doesn't seem related with changing assignees. I thihk it should be moved to another function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is, below is help text for PR trigger event. 😉

Pull request opened, closed, reopened, edited, approved, rejected, review comment, assigned, unassigned, label updated, label cleared or synchronized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is, below is help text for PR trigger event. 😉

Pull request opened, closed, reopened, edited, approved, rejected, review comment, assigned, unassigned, label updated, label cleared or synchronized.

Oh, that's not what I meant. 😄 What I meant is that this code seems to belong to services rather than models. Anyway, it was here before, so refactoring should be for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, yes you are probably right, but I will leave out that refactoring from this PR. I think we should have some design guidelines what type of code should be where. I think I've started to understand Gitea's code structure now....

log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err)
return nil
return false, err
}
} else {
mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues)
Expand All @@ -210,65 +199,10 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in
}
if err := prepareWebhooks(sess, issue.Repo, HookEventIssues, apiIssue); err != nil {
log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err)
return nil
}
}
return nil
}

// UpdateAPIAssignee is a helper function to add or delete one or multiple issue assignee(s)
// Deleting is done the GitHub way (quote from their api documentation):
// https://developer.github.com/v3/issues/#edit-an-issue
// "assignees" (array): Logins for Users to assign to this issue.
// Pass one or more user logins to replace the set of assignees on this Issue.
// Send an empty array ([]) to clear all assignees from the Issue.
func UpdateAPIAssignee(issue *Issue, oneAssignee string, multipleAssignees []string, doer *User) (err error) {
var allNewAssignees []*User

// Keep the old assignee thingy for compatibility reasons
if oneAssignee != "" {
// Prevent double adding assignees
var isDouble bool
for _, assignee := range multipleAssignees {
if assignee == oneAssignee {
isDouble = true
break
}
}

if !isDouble {
multipleAssignees = append(multipleAssignees, oneAssignee)
}
}

// Loop through all assignees to add them
for _, assigneeName := range multipleAssignees {
assignee, err := GetUserByName(assigneeName)
if err != nil {
return err
return false, err
}

allNewAssignees = append(allNewAssignees, assignee)
}

// Delete all old assignees not passed
if err = DeleteNotPassedAssignee(issue, doer, allNewAssignees); err != nil {
return err
}

// Add all new assignees
// Update the assignee. The function will check if the user exists, is already
// assigned (which he shouldn't as we deleted all assignees before) and
// has access to the repo.
for _, assignee := range allNewAssignees {
// Extra method to prevent double adding (which would result in removing)
err = AddAssigneeIfNotAssigned(issue, doer, assignee.ID)
if err != nil {
return err
}
}

return
return removed, nil
}

// MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type Notifier interface {
NotifyNewIssue(*models.Issue)
NotifyIssueChangeStatus(*models.User, *models.Issue, bool)
NotifyIssueChangeMilestone(doer *models.User, issue *models.Issue)
NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, removed bool)
NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool)
NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent string)
NotifyIssueClearLabels(doer *models.User, issue *models.Issue)
NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string)
Expand Down
2 changes: 1 addition & 1 deletion modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (*NullNotifier) NotifyIssueChangeContent(doer *models.User, issue *models.I
}

// NotifyIssueChangeAssignee places a place holder function
func (*NullNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, removed bool) {
func (*NullNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool) {
}

// NotifyIssueClearLabels places a place holder function
Expand Down
10 changes: 10 additions & 0 deletions modules/notification/mail/mail.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
package mail

import (
"fmt"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification/base"
Expand Down Expand Up @@ -88,3 +90,11 @@ func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models
log.Error("MailParticipants: %v", err)
}
}

func (m *mailNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool) {
// mail only sent to added assignees and not self-assignee
if !removed && doer.ID != assignee.ID && assignee.EmailNotifications() == models.EmailNotificationsEnabled {
ct := fmt.Sprintf("Assigned #%d.", issue.Index)
mailer.SendIssueAssignedMail(issue, doer, ct, []string{assignee.Email})
}
}
9 changes: 6 additions & 3 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"code.gitea.io/gitea/modules/notification/mail"
"code.gitea.io/gitea/modules/notification/ui"
"code.gitea.io/gitea/modules/notification/webhook"
"code.gitea.io/gitea/modules/setting"
)

var (
Expand All @@ -26,7 +27,9 @@ func RegisterNotifier(notifier base.Notifier) {

func init() {
RegisterNotifier(ui.NewNotifier())
RegisterNotifier(mail.NewNotifier())
if setting.Service.EnableNotifyMail {
davidsvantesson marked this conversation as resolved.
Show resolved Hide resolved
RegisterNotifier(mail.NewNotifier())
}
davidsvantesson marked this conversation as resolved.
Show resolved Hide resolved
RegisterNotifier(indexer.NewNotifier())
RegisterNotifier(webhook.NewNotifier())
}
Expand Down Expand Up @@ -138,9 +141,9 @@ func NotifyIssueChangeContent(doer *models.User, issue *models.Issue, oldContent
}

// NotifyIssueChangeAssignee notifies change content to notifiers
func NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, removed bool) {
func NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool) {
for _, notifier := range notifiers {
notifier.NotifyIssueChangeAssignee(doer, issue, removed)
notifier.NotifyIssueChangeAssignee(doer, issue, assignee, removed)
}
}

Expand Down
2 changes: 1 addition & 1 deletion routers/api/v1/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) {
oneAssignee = *form.Assignee
}

err = models.UpdateAPIAssignee(issue, oneAssignee, form.Assignees, ctx.User)
err = issue_service.UpdateAPIAssignee(issue, oneAssignee, form.Assignees, ctx.User)
if err != nil {
ctx.Error(500, "UpdateAPIAssignee", err)
return
Expand Down
3 changes: 2 additions & 1 deletion routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"code.gitea.io/gitea/modules/notification"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil"
issue_service "code.gitea.io/gitea/services/issue"
milestone_service "code.gitea.io/gitea/services/milestone"
pull_service "code.gitea.io/gitea/services/pull"
)
Expand Down Expand Up @@ -388,7 +389,7 @@ func EditPullRequest(ctx *context.APIContext, form api.EditPullRequestOption) {
// Send an empty array ([]) to clear all assignees from the Issue.

if ctx.Repo.CanWrite(models.UnitTypePullRequests) && (form.Assignees != nil || len(form.Assignee) > 0) {
err = models.UpdateAPIAssignee(issue, form.Assignee, form.Assignees, ctx.User)
err = issue_service.UpdateAPIAssignee(issue, form.Assignee, form.Assignees, ctx.User)
if err != nil {
if models.IsErrUserNotExist(err) {
ctx.Error(422, "", fmt.Sprintf("Assignee does not exist: [name: %s]", err))
Expand Down
12 changes: 11 additions & 1 deletion routers/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1130,10 +1130,20 @@ func UpdateIssueAssignee(ctx *context.Context) {
return
}
default:
if err := issue.ChangeAssignee(ctx.User, assigneeID); err != nil {
removed, err := issue.ChangeAssignee(ctx.User, assigneeID)
if err != nil {
ctx.ServerError("ChangeAssignee", err)
return
}

assignee, err := models.GetUserByID(assigneeID)
if err != nil {
ctx.ServerError("GetUserByID", err)
return
}
if !assignee.IsOrganization() {
notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed)
}
}
}
ctx.JSON(200, map[string]interface{}{
Expand Down
82 changes: 82 additions & 0 deletions services/issue/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
api "code.gitea.io/gitea/modules/structs"
)

Expand Down Expand Up @@ -96,3 +97,84 @@ func ChangeTitle(issue *models.Issue, doer *models.User, title string) (err erro

return nil
}

// UpdateAPIAssignee is a helper function to add or delete one or multiple issue assignee(s)
// Deleting is done the GitHub way (quote from their api documentation):
// https://developer.github.com/v3/issues/#edit-an-issue
// "assignees" (array): Logins for Users to assign to this issue.
// Pass one or more user logins to replace the set of assignees on this Issue.
// Send an empty array ([]) to clear all assignees from the Issue.
func UpdateAPIAssignee(issue *models.Issue, oneAssignee string, multipleAssignees []string, doer *models.User) (err error) {
var allNewAssignees []*models.User

// Keep the old assignee thingy for compatibility reasons
if oneAssignee != "" {
// Prevent double adding assignees
var isDouble bool
for _, assignee := range multipleAssignees {
if assignee == oneAssignee {
isDouble = true
break
}
}

if !isDouble {
multipleAssignees = append(multipleAssignees, oneAssignee)
}
}

// Loop through all assignees to add them
for _, assigneeName := range multipleAssignees {
assignee, err := models.GetUserByName(assigneeName)
if err != nil {
return err
}

allNewAssignees = append(allNewAssignees, assignee)
}

// Delete all old assignees not passed
if err = models.DeleteNotPassedAssignee(issue, doer, allNewAssignees); err != nil {
return err
}

// Add all new assignees
// Update the assignee. The function will check if the user exists, is already
// assigned (which he shouldn't as we deleted all assignees before) and
// has access to the repo.
for _, assignee := range allNewAssignees {
// Extra method to prevent double adding (which would result in removing)
err = AddAssigneeIfNotAssigned(issue, doer, assignee.ID)
if err != nil {
return err
}
}

return
}

// AddAssigneeIfNotAssigned adds an assignee only if he isn't aleady assigned to the issue
func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID int64) (err error) {
// Check if the user is already assigned
isAssigned, err := models.IsUserAssignedToIssue(issue, &models.User{ID: assigneeID})
if err != nil {
return err
}

if !isAssigned {
davidsvantesson marked this conversation as resolved.
Show resolved Hide resolved
removed, err := issue.ChangeAssignee(doer, assigneeID)
if err != nil {
return err
}

assignee, err := models.GetUserByID(assigneeID)
if err != nil {
return err
}

if !assignee.IsOrganization() {
notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed)
}
}
return nil
}
Loading