Mail assignee when issue/pull request is assigned (#8546)

* Send email to assigned user

* Only send mail if enabled

* Mail also when assigned through API

* Need to refactor functions from models to issue service

* Refer to issue index rather than ID

* Disable email notifications completly at initalization if global disable

* Check of user enbled mail shall be in mail notification function only

* Initialize notifications from routers init function.

* Use the assigned comment when sending assigned mail

* Refactor so that assignees always added as separate step when new issue/pr.

* Check error from AddAssignees

* Check if user can be assiged to issue or pull request

* Missing return

* Refactor of CanBeAssigned check.

CanBeAssigned shall have same check as UI.

* Clarify function names (toggle rather than update/change), and clean up.

* Fix review comments.

* Flash error if assignees was not added when creating issue/pr

* Generate error if assignee users doesn't exist

Author: davidsvantesson

models/issue.go

@@ -896,7 +896,6 @@ type NewIssueOptions struct {
 	Repo        *Repository
 	Issue       *Issue
 	LabelIDs    []int64
-	AssigneeIDs []int64
 	Attachments []string // In UUID format.
 	IsPull      bool
 }
@@ -918,40 +917,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 		}
 	}
 
-	// Keep the old assignee id thingy for compatibility reasons
-	if opts.Issue.AssigneeID > 0 {
-		isAdded := false
-		// Check if the user has already been passed to issue.AssigneeIDs, if not, add it
-		for _, aID := range opts.AssigneeIDs {
-			if aID == opts.Issue.AssigneeID {
-				isAdded = true
-				break
-			}
-		}
-
-		if !isAdded {
-			opts.AssigneeIDs = append(opts.AssigneeIDs, opts.Issue.AssigneeID)
-		}
-	}
-
-	// Check for and validate assignees
-	if len(opts.AssigneeIDs) > 0 {
-		for _, assigneeID := range opts.AssigneeIDs {
-			user, err := getUserByID(e, assigneeID)
-			if err != nil {
-				return fmt.Errorf("getUserByID [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err)
-			}
-			valid, err := canBeAssigned(e, user, opts.Repo)
-			if err != nil {
-				return fmt.Errorf("canBeAssigned [user_id: %d, repo_id: %d]: %v", assigneeID, opts.Repo.ID, err)
-			}
-			if !valid {
-				return ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: opts.Repo.Name}
-			}
-		}
-	}
-
-	// Milestone and assignee validation should happen before insert actual object.
+	// Milestone validation should happen before insert actual object.
 	if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").
 		Where("repo_id=?", opts.Issue.RepoID).
 		Insert(opts.Issue); err != nil {
@@ -976,14 +942,6 @@ 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)
-		if err != nil {
-			return err
-		}
-	}
-
 	if opts.IsPull {
 		_, err = e.Exec("UPDATE `repository` SET num_pulls = num_pulls + 1 WHERE id = ?", opts.Issue.RepoID)
 	} else {
@@ -1041,11 +999,11 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 }
 
 // NewIssue creates new issue with labels for repository.
-func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) (err error) {
+func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
 	// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
 	i := 0
 	for {
-		if err = newIssueAttempt(repo, issue, labelIDs, assigneeIDs, uuids); err == nil {
+		if err = newIssueAttempt(repo, issue, labelIDs, uuids); err == nil {
 			return nil
 		}
 		if !IsErrNewIssueInsert(err) {
@@ -1059,7 +1017,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in
 	return fmt.Errorf("NewIssue: too many errors attempting to insert the new issue. Last error was: %v", err)
 }
 
-func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) (err error) {
+func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, uuids []string) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {
@@ -1071,7 +1029,6 @@ func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, assigneeI
 		Issue:       issue,
 		LabelIDs:    labelIDs,
 		Attachments: uuids,
-		AssigneeIDs: assigneeIDs,
 	}); err != nil {
 		if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
 			return err

models/issue_assignees.go

@@ -58,8 +58,11 @@ func getAssigneesByIssue(e Engine, issue *Issue) (assignees []*User, err error)
 
 // IsUserAssignedToIssue returns true when the user is assigned to the issue
 func IsUserAssignedToIssue(issue *Issue, user *User) (isAssigned bool, err error) {
-	isAssigned, err = x.Exist(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID})
-	return
+	return isUserAssignedToIssue(x, issue, user)
+}
+
+func isUserAssignedToIssue(e Engine, issue *Issue, user *User) (isAssigned bool, err error) {
+	return e.Get(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID})
 }
 
 // DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array
@@ -78,7 +81,7 @@ func DeleteNotPassedAssignee(issue *Issue, doer *User, assignees []*User) (err e
 
 		if !found {
 			// This function also does comments and hooks, which is why we call it seperatly instead of directly removing the assignees here
-			if err := UpdateAssignee(issue, doer, assignee.ID); err != nil {
+			if _, _, err := issue.ToggleAssignee(doer, assignee.ID); err != nil {
 				return err
 			}
 		}
@@ -110,73 +113,56 @@ 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)
-}
-
-// ChangeAssignee changes the Assignee of this issue.
-func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (err error) {
+// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it.
+func (issue *Issue) ToggleAssignee(doer *User, assigneeID int64) (removed bool, comment *Comment, err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 
 	if err := sess.Begin(); err != nil {
-		return err
+		return false, nil, err
 	}
 
-	if err := issue.changeAssignee(sess, doer, assigneeID, false); err != nil {
-		return err
+	removed, comment, err = issue.toggleAssignee(sess, doer, assigneeID, false)
+	if err != nil {
+		return false, nil, err
 	}
 
 	if err := sess.Commit(); err != nil {
-		return err
+		return false, nil, err
 	}
 
 	go HookQueue.Add(issue.RepoID)
-	return nil
+
+	return removed, comment, nil
 }
 
-func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (err error) {
-	// Update the assignee
-	removed, err := updateIssueAssignee(sess, issue, assigneeID)
+func (issue *Issue) toggleAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (removed bool, comment *Comment, err error) {
+	removed, err = toggleUserAssignee(sess, issue, assigneeID)
 	if err != nil {
-		return fmt.Errorf("UpdateIssueUserByAssignee: %v", err)
+		return false, nil, fmt.Errorf("UpdateIssueUserByAssignee: %v", err)
 	}
 
 	// Repo infos
 	if err = issue.loadRepo(sess); err != nil {
-		return fmt.Errorf("loadRepo: %v", err)
+		return false, nil, fmt.Errorf("loadRepo: %v", err)
 	}
 
 	// Comment
-	if _, err = createAssigneeComment(sess, doer, issue.Repo, issue, assigneeID, removed); err != nil {
-		return fmt.Errorf("createAssigneeComment: %v", err)
+	comment, err = createAssigneeComment(sess, doer, issue.Repo, issue, assigneeID, removed)
+	if err != nil {
+		return false, nil, fmt.Errorf("createAssigneeComment: %v", err)
 	}
 
 	// if pull request is in the middle of creation - don't call webhook
 	if isCreate {
-		return nil
+		return removed, comment, 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, nil, fmt.Errorf("loadPullRequest: %v", err)
 		}
 		issue.PullRequest.Issue = issue
 		apiPullRequest := &api.PullRequestPayload{
@@ -190,9 +176,10 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in
 		} else {
 			apiPullRequest.Action = api.HookIssueAssigned
 		}
+		// Assignee comment triggers a webhook
 		if err := prepareWebhooks(sess, issue.Repo, HookEventPullRequest, apiPullRequest); err != nil {
 			log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err)
-			return nil
+			return false, nil, err
 		}
 	} else {
 		mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues)
@@ -208,67 +195,50 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in
 		} else {
 			apiIssue.Action = api.HookIssueAssigned
 		}
+		// Assignee comment triggers a webhook
 		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 false, nil, err
 		}
 	}
-	return nil
+	return removed, comment, 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
+// toggles user assignee state in database
+func toggleUserAssignee(e *xorm.Session, issue *Issue, assigneeID int64) (removed bool, err error) {
 
-	// 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)
-		}
+	// Check if the user exists
+	assignee, err := getUserByID(e, assigneeID)
+	if err != nil {
+		return false, err
 	}
 
-	// Loop through all assignees to add them
-	for _, assigneeName := range multipleAssignees {
-		assignee, err := GetUserByName(assigneeName)
-		if err != nil {
-			return err
+	// Check if the submitted user is already assigned, if yes delete him otherwise add him
+	var i int
+	for i = 0; i < len(issue.Assignees); i++ {
+		if issue.Assignees[i].ID == assigneeID {
+			break
 		}
-
-		allNewAssignees = append(allNewAssignees, assignee)
 	}
 
-	// Delete all old assignees not passed
-	if err = DeleteNotPassedAssignee(issue, doer, allNewAssignees); err != nil {
-		return err
-	}
+	assigneeIn := IssueAssignees{AssigneeID: assigneeID, IssueID: issue.ID}
 
-	// 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)
+	toBeDeleted := i < len(issue.Assignees)
+	if toBeDeleted {
+		issue.Assignees = append(issue.Assignees[:i], issue.Assignees[i:]...)
+		_, err = e.Delete(assigneeIn)
 		if err != nil {
-			return err
+			return toBeDeleted, err
+		}
+	} else {
+		issue.Assignees = append(issue.Assignees, assignee)
+		_, err = e.Insert(assigneeIn)
+		if err != nil {
+			return toBeDeleted, err
 		}
 	}
 
-	return
+	return toBeDeleted, nil
 }
 
 // MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs
@@ -292,7 +262,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string
 	}
 
 	// Get the IDs of all assignees
-	assigneeIDs = GetUserIDsByNames(multipleAssignees)
+	assigneeIDs, err = GetUserIDsByNames(multipleAssignees, false)
 
 	return
 }

models/issue_assignees_test.go

@@ -20,17 +20,17 @@ func TestUpdateAssignee(t *testing.T) {
 	// Assign multiple users
 	user2, err := GetUserByID(2)
 	assert.NoError(t, err)
-	err = UpdateAssignee(issue, &User{ID: 1}, user2.ID)
+	_, _, err = issue.ToggleAssignee(&User{ID: 1}, user2.ID)
 	assert.NoError(t, err)
 
 	user3, err := GetUserByID(3)
 	assert.NoError(t, err)
-	err = UpdateAssignee(issue, &User{ID: 1}, user3.ID)
+	_, _, err = issue.ToggleAssignee(&User{ID: 1}, user3.ID)
 	assert.NoError(t, err)
 
 	user1, err := GetUserByID(1) // This user is already assigned (see the definition in fixtures), so running  UpdateAssignee should unassign him
 	assert.NoError(t, err)
-	err = UpdateAssignee(issue, &User{ID: 1}, user1.ID)
+	_, _, err = issue.ToggleAssignee(&User{ID: 1}, user1.ID)
 	assert.NoError(t, err)
 
 	// Check if he got removed

models/issue_test.go

@@ -297,7 +297,7 @@ func testInsertIssue(t *testing.T, title, content string) {
 		Title:    title,
 		Content:  content,
 	}
-	err := NewIssue(repo, &issue, nil, nil, nil)
+	err := NewIssue(repo, &issue, nil, nil)
 	assert.NoError(t, err)
 
 	var newIssue Issue