From 6aa3f8bc29cb1ed3a1b165cbf526079a751c8c71 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 25 Oct 2019 16:46:37 +0200 Subject: [PATCH] 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 --- models/issue.go | 51 +--------- models/issue_assignees.go | 138 ++++++++++---------------- models/issue_assignees_test.go | 6 +- models/issue_test.go | 2 +- models/issue_user.go | 38 ------- models/pull.go | 7 +- models/repo_permission.go | 14 ++- models/user.go | 10 +- modules/notification/base/notifier.go | 2 +- modules/notification/base/null.go | 2 +- modules/notification/mail/mail.go | 10 ++ modules/notification/notification.go | 12 ++- options/locale/locale_en-US.ini | 1 + routers/api/v1/repo/issue.go | 30 +++++- routers/api/v1/repo/pull.go | 30 +++++- routers/init.go | 2 + routers/repo/issue.go | 44 ++++++-- routers/repo/pull.go | 8 +- services/issue/issue.go | 106 +++++++++++++++++++- services/mailer/mail.go | 11 +- services/mailer/mail_issue.go | 4 - services/pull/pull.go | 4 +- templates/mail/issue/assigned.tmpl | 17 ++++ 23 files changed, 333 insertions(+), 216 deletions(-) create mode 100644 templates/mail/issue/assigned.tmpl diff --git a/models/issue.go b/models/issue.go index 52af872cccf..0315580c31b 100644 --- a/models/issue.go +++ b/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 diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 00ee4988605..ed0576b38b0 100644 --- a/models/issue_assignees.go +++ b/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 - } - } + // Check if the user exists + assignee, err := getUserByID(e, assigneeID) + if err != nil { + return false, err + } - if !isDouble { - multipleAssignees = append(multipleAssignees, oneAssignee) + // 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 } } - // Loop through all assignees to add them - for _, assigneeName := range multipleAssignees { - assignee, err := GetUserByName(assigneeName) + assigneeIn := IssueAssignees{AssigneeID: assigneeID, IssueID: issue.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 } - - 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) + } else { + issue.Assignees = append(issue.Assignees, assignee) + _, err = e.Insert(assigneeIn) if err != nil { - return err + 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 } diff --git a/models/issue_assignees_test.go b/models/issue_assignees_test.go index d32f41737a3..1c5b5e7a22f 100644 --- a/models/issue_assignees_test.go +++ b/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 diff --git a/models/issue_test.go b/models/issue_test.go index 7614c2f5544..592a0e3d777 100644 --- a/models/issue_test.go +++ b/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 diff --git a/models/issue_user.go b/models/issue_user.go index 6974a4d3cc1..67a118fe579 100644 --- a/models/issue_user.go +++ b/models/issue_user.go @@ -6,8 +6,6 @@ package models import ( "fmt" - - "xorm.io/xorm" ) // IssueUser represents an issue-user relation. @@ -51,42 +49,6 @@ func newIssueUsers(e Engine, repo *Repository, issue *Issue) error { return nil } -func updateIssueAssignee(e *xorm.Session, issue *Issue, assigneeID int64) (removed bool, err error) { - - // Check if the user exists - assignee, err := getUserByID(e, assigneeID) - if err != nil { - return false, err - } - - // Check if the submitted user is already assigne, 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 - } - } - - assigneeIn := IssueAssignees{AssigneeID: assigneeID, IssueID: issue.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 toBeDeleted, err - } - } else { - issue.Assignees = append(issue.Assignees, assignee) - _, err = e.Insert(assigneeIn) - if err != nil { - return toBeDeleted, err - } - } - - return toBeDeleted, nil -} - // UpdateIssueUserByRead updates issue-user relation for reading. func UpdateIssueUserByRead(uid, issueID int64) error { _, err := x.Exec("UPDATE `issue_user` SET is_read=? WHERE uid=? AND issue_id=?", true, uid, issueID) diff --git a/models/pull.go b/models/pull.go index a3554a1b3da..c6da63ec557 100644 --- a/models/pull.go +++ b/models/pull.go @@ -686,11 +686,11 @@ func (pr *PullRequest) testPatch(e Engine) (err error) { } // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte, assigneeIDs []int64) (err error) { +func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) { // Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887 i := 0 for { - if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch, assigneeIDs); err == nil { + if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch); err == nil { return nil } if !IsErrNewIssueInsert(err) { @@ -704,7 +704,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err) } -func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte, assigneeIDs []int64) (err error) { +func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte) (err error) { sess := x.NewSession() defer sess.Close() if err = sess.Begin(); err != nil { @@ -717,7 +717,6 @@ func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuid LabelIDs: labelIDs, Attachments: uuids, IsPull: true, - AssigneeIDs: assigneeIDs, }); err != nil { if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) { return err diff --git a/models/repo_permission.go b/models/repo_permission.go index 916678d1685..fad29bd1695 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -329,10 +329,18 @@ func HasAccessUnit(user *User, repo *Repository, unitType UnitType, testMode Acc return hasAccessUnit(x, user, repo, unitType, testMode) } -// canBeAssigned return true if user could be assigned to a repo +// CanBeAssigned return true if user can be assigned to issue or pull requests in repo +// Currently any write access (code, issues or pr's) is assignable, to match assignee list in user interface. // FIXME: user could send PullRequest also could be assigned??? -func canBeAssigned(e Engine, user *User, repo *Repository) (bool, error) { - return hasAccessUnit(e, user, repo, UnitTypeCode, AccessModeWrite) +func CanBeAssigned(user *User, repo *Repository, isPull bool) (bool, error) { + if user.IsOrganization() { + return false, fmt.Errorf("Organization can't be added as assignee [user_id: %d, repo_id: %d]", user.ID, repo.ID) + } + perm, err := GetUserRepoPermission(repo, user) + if err != nil { + return false, err + } + return perm.CanAccessAny(AccessModeWrite, UnitTypeCode, UnitTypeIssues, UnitTypePullRequests), nil } func hasAccess(e Engine, userID int64, repo *Repository) (bool, error) { diff --git a/models/user.go b/models/user.go index a3679c9a5b0..7aa1e143e83 100644 --- a/models/user.go +++ b/models/user.go @@ -1320,16 +1320,20 @@ func GetUsersByIDs(ids []int64) ([]*User, error) { } // GetUserIDsByNames returns a slice of ids corresponds to names. -func GetUserIDsByNames(names []string) []int64 { +func GetUserIDsByNames(names []string, ignoreNonExistent bool) ([]int64, error) { ids := make([]int64, 0, len(names)) for _, name := range names { u, err := GetUserByName(name) if err != nil { - continue + if ignoreNonExistent { + continue + } else { + return nil, err + } } ids = append(ids, u.ID) } - return ids + return ids, nil } // UserCommit represents a commit with validation of user. diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index e44f3cc6321..c74bb52014d 100644 --- a/modules/notification/base/notifier.go +++ b/modules/notification/base/notifier.go @@ -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, comment *models.Comment) 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) diff --git a/modules/notification/base/null.go b/modules/notification/base/null.go index 12be1999f90..9fb08884a19 100644 --- a/modules/notification/base/null.go +++ b/modules/notification/base/null.go @@ -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, comment *models.Comment) { } // NotifyIssueClearLabels places a place holder function diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index e1ae391f783..0900c6dcdf1 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -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" @@ -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, comment *models.Comment) { + // 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, comment, []string{assignee.Email}) + } +} diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 06220ecb040..0f1b63cf672 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -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 ( @@ -24,9 +25,12 @@ func RegisterNotifier(notifier base.Notifier) { notifiers = append(notifiers, notifier) } -func init() { +// NewContext registers notification handlers +func NewContext() { RegisterNotifier(ui.NewNotifier()) - RegisterNotifier(mail.NewNotifier()) + if setting.Service.EnableNotifyMail { + RegisterNotifier(mail.NewNotifier()) + } RegisterNotifier(indexer.NewNotifier()) RegisterNotifier(webhook.NewNotifier()) } @@ -138,9 +142,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, comment *models.Comment) { for _, notifier := range notifiers { - notifier.NotifyIssueChangeAssignee(doer, issue, removed) + notifier.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) } } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 83047ef6f1c..f8e25a85f98 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -977,6 +977,7 @@ issues.review.review = Review issues.review.reviewers = Reviewers issues.review.show_outdated = Show outdated issues.review.hide_outdated = Hide outdated +issues.assignee.error = Not all assignees was added due to an unexpected error. pulls.desc = Enable pull requests and code reviews. pulls.new = New Pull Request diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index aab167bc681..9529e09b290 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -213,12 +213,31 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { } return } + + // Check if the passed assignees is assignable + for _, aID := range assigneeIDs { + assignee, err := models.GetUserByID(aID) + if err != nil { + ctx.Error(500, "GetUserByID", err) + return + } + + valid, err := models.CanBeAssigned(assignee, ctx.Repo.Repository, false) + if err != nil { + ctx.Error(500, "canBeAssigned", err) + return + } + if !valid { + ctx.Error(422, "canBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: ctx.Repo.Repository.Name}) + return + } + } } else { // setting labels is not allowed if user is not a writer form.Labels = make([]int64, 0) } - if err := issue_service.NewIssue(ctx.Repo.Repository, issue, form.Labels, assigneeIDs, nil); err != nil { + if err := issue_service.NewIssue(ctx.Repo.Repository, issue, form.Labels, nil); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err) return @@ -227,6 +246,11 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { return } + if err := issue_service.AddAssignees(issue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } + notification.NotifyNewIssue(issue) if form.Closed { @@ -336,9 +360,9 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { oneAssignee = *form.Assignee } - err = models.UpdateAPIAssignee(issue, oneAssignee, form.Assignees, ctx.User) + err = issue_service.UpdateAssignees(issue, oneAssignee, form.Assignees, ctx.User) if err != nil { - ctx.Error(500, "UpdateAPIAssignee", err) + ctx.Error(500, "UpdateAssignees", err) return } } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 16ddd10c60f..6b20844c51f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -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" ) @@ -285,8 +286,26 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption } return } + // Check if the passed assignees is assignable + for _, aID := range assigneeIDs { + assignee, err := models.GetUserByID(aID) + if err != nil { + ctx.Error(500, "GetUserByID", err) + return + } - if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, patch, assigneeIDs); err != nil { + valid, err := models.CanBeAssigned(assignee, repo, true) + if err != nil { + ctx.Error(500, "canBeAssigned", err) + return + } + if !valid { + ctx.Error(422, "canBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name}) + return + } + } + + if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, patch); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err) return @@ -298,6 +317,11 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption return } + if err := issue_service.AddAssignees(prIssue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } + notification.NotifyNewPullRequest(pr) log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID) @@ -387,12 +411,12 @@ 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.UpdateAssignees(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)) } else { - ctx.Error(500, "UpdateAPIAssignee", err) + ctx.Error(500, "UpdateAssignees", err) } return } diff --git a/routers/init.go b/routers/init.go index e4e880dbbb2..bdd978d717a 100644 --- a/routers/init.go +++ b/routers/init.go @@ -18,6 +18,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup" "code.gitea.io/gitea/modules/markup/external" + "code.gitea.io/gitea/modules/notification" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/ssh" "code.gitea.io/gitea/modules/task" @@ -44,6 +45,7 @@ func NewServices() { setting.NewServices() mailer.NewContext() _ = cache.NewContext() + notification.NewContext() } // In case of problems connecting to DB, retry connection. Eg, PGSQL in Docker Container on Synology diff --git a/routers/repo/issue.go b/routers/repo/issue.go index dee2c6e698d..94c39ae2240 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -503,21 +503,21 @@ func ValidateRepoMetas(ctx *context.Context, form auth.CreateIssueForm, isPull b return nil, nil, 0 } - // Check if the passed assignees actually exists and has write access to the repo + // Check if the passed assignees actually exists and is assignable for _, aID := range assigneeIDs { - user, err := models.GetUserByID(aID) + assignee, err := models.GetUserByID(aID) if err != nil { ctx.ServerError("GetUserByID", err) return nil, nil, 0 } - perm, err := models.GetUserRepoPermission(repo, user) + valid, err := models.CanBeAssigned(assignee, repo, isPull) if err != nil { - ctx.ServerError("GetUserRepoPermission", err) + ctx.ServerError("canBeAssigned", err) return nil, nil, 0 } - if !perm.CanWriteIssuesOrPulls(isPull) { - ctx.ServerError("CanWriteIssuesOrPulls", fmt.Errorf("No permission for %s", user.Name)) + if !valid { + ctx.ServerError("canBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: aID, RepoName: repo.Name}) return nil, nil, 0 } } @@ -574,7 +574,7 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { Content: form.Content, Ref: form.Ref, } - if err := issue_service.NewIssue(repo, issue, labelIDs, assigneeIDs, attachments); err != nil { + if err := issue_service.NewIssue(repo, issue, labelIDs, attachments); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error()) return @@ -583,6 +583,11 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { return } + if err := issue_service.AddAssignees(issue, ctx.User, assigneeIDs); err != nil { + log.Error("AddAssignees: %v", err) + ctx.Flash.Error(ctx.Tr("issues.assignee.error")) + } + notification.NotifyNewIssue(issue) log.Trace("Issue created: %d/%d", repo.ID, issue.ID) @@ -1112,7 +1117,7 @@ func UpdateIssueMilestone(ctx *context.Context) { }) } -// UpdateIssueAssignee change issue's assignee +// UpdateIssueAssignee change issue's or pull's assignee func UpdateIssueAssignee(ctx *context.Context) { issues := getActionIssues(ctx) if ctx.Written() { @@ -1130,10 +1135,29 @@ func UpdateIssueAssignee(ctx *context.Context) { return } default: - if err := issue.ChangeAssignee(ctx.User, assigneeID); err != nil { - ctx.ServerError("ChangeAssignee", err) + assignee, err := models.GetUserByID(assigneeID) + if err != nil { + ctx.ServerError("GetUserByID", err) return } + + valid, err := models.CanBeAssigned(assignee, issue.Repo, issue.IsPull) + if err != nil { + ctx.ServerError("canBeAssigned", err) + return + } + if !valid { + ctx.ServerError("canBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}) + return + } + + removed, comment, err := issue.ToggleAssignee(ctx.User, assigneeID) + if err != nil { + ctx.ServerError("ToggleAssignee", err) + return + } + + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed, comment) } } ctx.JSON(200, map[string]interface{}{ diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 4b73571e663..23d97e7b7e2 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -24,6 +24,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/gitdiff" + issue_service "code.gitea.io/gitea/services/issue" pull_service "code.gitea.io/gitea/services/pull" "github.com/unknwon/com" @@ -770,7 +771,7 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt // instead of 500. - if err := pull_service.NewPullRequest(repo, pullIssue, labelIDs, attachments, pullRequest, patch, assigneeIDs); err != nil { + if err := pull_service.NewPullRequest(repo, pullIssue, labelIDs, attachments, pullRequest, patch); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error()) return @@ -782,6 +783,11 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } + if err := issue_service.AddAssignees(pullIssue, ctx.User, assigneeIDs); err != nil { + log.Error("AddAssignees: %v", err) + ctx.Flash.Error(ctx.Tr("issues.assignee.error")) + } + notification.NotifyNewPullRequest(pullRequest) log.Trace("Pull request created: %d/%d", repo.ID, pullIssue.ID) diff --git a/services/issue/issue.go b/services/issue/issue.go index a28916a7f9b..a5f725ab702 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -9,12 +9,13 @@ 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" ) // NewIssue creates new issue with labels for repository. -func NewIssue(repo *models.Repository, issue *models.Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) error { - if err := models.NewIssue(repo, issue, labelIDs, assigneeIDs, uuids); err != nil { +func NewIssue(repo *models.Repository, issue *models.Issue, labelIDs []int64, uuids []string) error { + if err := models.NewIssue(repo, issue, labelIDs, uuids); err != nil { return err } @@ -96,3 +97,104 @@ func ChangeTitle(issue *models.Issue, doer *models.User, title string) (err erro return nil } + +// UpdateAssignees 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 UpdateAssignees(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 already assigned to the issue. +// Also checks for access of assigned user +func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID int64) (err error) { + assignee, err := models.GetUserByID(assigneeID) + if err != nil { + return err + } + + // Check if the user is already assigned + isAssigned, err := models.IsUserAssignedToIssue(issue, assignee) + if err != nil { + return err + } + if isAssigned { + // nothing to to + return nil + } + + valid, err := models.CanBeAssigned(assignee, issue.Repo, issue.IsPull) + if err != nil { + return err + } + if !valid { + return models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} + } + + removed, comment, err := issue.ToggleAssignee(doer, assigneeID) + if err != nil { + return err + } + + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) + + return nil +} + +// AddAssignees adds a list of assignes (from IDs) to an issue +func AddAssignees(issue *models.Issue, doer *models.User, assigneeIDs []int64) (err error) { + for _, assigneeID := range assigneeIDs { + if err = AddAssigneeIfNotAssigned(issue, doer, assigneeID); err != nil { + return err + } + } + return nil +} diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 3f0a789dc4c..bc2aff7314f 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -28,8 +28,9 @@ const ( mailAuthResetPassword base.TplName = "auth/reset_passwd" mailAuthRegisterNotify base.TplName = "auth/register_notify" - mailIssueComment base.TplName = "issue/comment" - mailIssueMention base.TplName = "issue/mention" + mailIssueComment base.TplName = "issue/comment" + mailIssueMention base.TplName = "issue/mention" + mailIssueAssigned base.TplName = "issue/assigned" mailNotifyCollaborator base.TplName = "notify/collaborator" ) @@ -183,6 +184,7 @@ func composeIssueCommentMessage(issue *models.Issue, doer *models.User, content data = composeTplData(subject, body, issue.HTMLURL()) } data["Doer"] = doer + data["Issue"] = issue var mailBody bytes.Buffer @@ -220,3 +222,8 @@ func SendIssueMentionMail(issue *models.Issue, doer *models.User, content string } SendAsync(composeIssueCommentMessage(issue, doer, content, comment, mailIssueMention, tos, "issue mention")) } + +// SendIssueAssignedMail composes and sends issue assigned email +func SendIssueAssignedMail(issue *models.Issue, doer *models.User, content string, comment *models.Comment, tos []string) { + SendAsync(composeIssueCommentMessage(issue, doer, content, comment, mailIssueAssigned, tos, "issue assigned")) +} diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index b16323909ca..a5f3251807a 100644 --- a/services/mailer/mail_issue.go +++ b/services/mailer/mail_issue.go @@ -10,7 +10,6 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/references" - "code.gitea.io/gitea/modules/setting" "github.com/unknwon/com" ) @@ -24,9 +23,6 @@ func mailSubject(issue *models.Issue) string { // 1. Repository watchers and users who are participated in comments. // 2. Users who are not in 1. but get mentioned in current issue/comment. func mailIssueCommentToParticipants(issue *models.Issue, doer *models.User, content string, comment *models.Comment, mentions []string) error { - if !setting.Service.EnableNotifyMail { - return nil - } watchers, err := models.GetWatchers(issue.RepoID) if err != nil { diff --git a/services/pull/pull.go b/services/pull/pull.go index 3c584fce748..959da674052 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -14,8 +14,8 @@ import ( ) // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(repo *models.Repository, pull *models.Issue, labelIDs []int64, uuids []string, pr *models.PullRequest, patch []byte, assigneeIDs []int64) error { - if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr, patch, assigneeIDs); err != nil { +func NewPullRequest(repo *models.Repository, pull *models.Issue, labelIDs []int64, uuids []string, pr *models.PullRequest, patch []byte) error { + if err := models.NewPullRequest(repo, pull, labelIDs, uuids, pr, patch); err != nil { return err } diff --git a/templates/mail/issue/assigned.tmpl b/templates/mail/issue/assigned.tmpl new file mode 100644 index 00000000000..ab06ade1f4c --- /dev/null +++ b/templates/mail/issue/assigned.tmpl @@ -0,0 +1,17 @@ + + + + + {{.Subject}} + + + +

@{{.Doer.Name}} assigned you to the {{if eq .Issue.IsPull true}}pull request{{else}}issue{{end}} #{{.Issue.Index}} in repository {{.Issue.Repo.FullName}}.

+

+ --- +
+ View it on Gitea. +

+ + +