From fb0bb14e09ef93c8642fb149c0f2081596426852 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 12 Oct 2019 13:50:46 +0200 Subject: [PATCH 01/18] Send email to assigned user --- models/issue.go | 2 +- models/issue_assignees.go | 41 ++++++++++++++------------- modules/notification/base/notifier.go | 2 +- modules/notification/base/null.go | 2 +- modules/notification/mail/mail.go | 10 +++++++ modules/notification/notification.go | 4 +-- routers/repo/issue.go | 10 ++++++- services/mailer/mail.go | 11 +++++-- templates/mail/issue/assigned.tmpl | 17 +++++++++++ 9 files changed, 72 insertions(+), 27 deletions(-) create mode 100644 templates/mail/issue/assigned.tmpl diff --git a/models/issue.go b/models/issue.go index 6503a0618fb06..bc5440e05a29c 100644 --- a/models/issue.go +++ b/models/issue.go @@ -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 } diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 1f504a9950a82..28ebc7ee75966 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -119,64 +119,67 @@ func AddAssigneeIfNotAssigned(issue *Issue, doer *User, assigneeID int64) (err e } if !isAssigned { - return issue.ChangeAssignee(doer, assigneeID) + _, err = issue.ChangeAssignee(doer, assigneeID) } - return nil + return err } // 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() 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) 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{ @@ -192,7 +195,7 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in } 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, err } } else { mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues) @@ -210,10 +213,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 false, err } } - return nil + return removed, nil } // UpdateAPIAssignee is a helper function to add or delete one or multiple issue assignee(s) diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index e44f3cc63216f..54c24754ac01b 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) 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 12be1999f9019..9801944a6bf8f 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) { } // NotifyIssueClearLabels places a place holder function diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index e1ae391f783c4..52e2ee1b361c3 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) { + // mail only sent to added assignees and not self-assignee + if !removed && doer.ID != assignee.ID { + ct := fmt.Sprintf("Assigned #%d.", issue.Index) + mailer.SendIssueAssignedMail(issue, doer, ct, []string{assignee.Email}) + } +} diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 06220ecb04019..8974ff49d3bdb 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -138,9 +138,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) } } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index dee2c6e698dbc..6c4a2773f71d8 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1130,10 +1130,18 @@ 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 + } + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) } } ctx.JSON(200, map[string]interface{}{ diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 3f0a789dc4cd4..94fe1583a977a 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, tos []string) { + SendAsync(composeIssueCommentMessage(issue, doer, content, nil, mailIssueAssigned, tos, "issue assigned")) +} diff --git a/templates/mail/issue/assigned.tmpl b/templates/mail/issue/assigned.tmpl new file mode 100644 index 0000000000000..324f05cb8659b --- /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.ID}}.

+

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

+ + + From 385c14d39546571343f63af67d96f5b6e988e2ea Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 16 Oct 2019 18:15:34 +0200 Subject: [PATCH 02/18] Only send mail if enabled --- routers/repo/issue.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 6c4a2773f71d8..355b1c7ce7eb2 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1141,7 +1141,9 @@ func UpdateIssueAssignee(ctx *context.Context) { ctx.ServerError("GetUserByID", err) return } - notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) + if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) + } } } ctx.JSON(200, map[string]interface{}{ From 27bbc9d195fc4a99606a11117d75ad6e99cb3f9d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 16 Oct 2019 19:18:43 +0200 Subject: [PATCH 03/18] Mail also when assigned through API --- models/issue_assignees.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 28ebc7ee75966..c2062dcb9316c 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -8,6 +8,8 @@ import ( "fmt" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/notification" + "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "github.com/go-xorm/xorm" @@ -119,9 +121,20 @@ func AddAssigneeIfNotAssigned(issue *Issue, doer *User, assigneeID int64) (err e } if !isAssigned { - _, err = issue.ChangeAssignee(doer, assigneeID) + if _, err = issue.ChangeAssignee(doer, assigneeID); err != nil { + return err + } + + assignee, err := models.GetUserByID(assigneeID) + if err != nil { + return err + } + + if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) + } } - return err + return nil } // UpdateAssignee deletes or adds an assignee to an issue From b063676cd77a4bcf8f72e7171aeaafdf606449f1 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 16 Oct 2019 19:33:22 +0200 Subject: [PATCH 04/18] Need to refactor functions from models to issue service --- models/issue_assignees.go | 82 ----------------------------------- routers/api/v1/repo/issue.go | 2 +- routers/api/v1/repo/pull.go | 3 +- services/issue/issue.go | 83 ++++++++++++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 84 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index c2062dcb9316c..0efc86adcaa1d 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -8,8 +8,6 @@ import ( "fmt" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" "github.com/go-xorm/xorm" @@ -112,31 +110,6 @@ 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 { - if _, err = issue.ChangeAssignee(doer, assigneeID); err != nil { - return err - } - - assignee, err := models.GetUserByID(assigneeID) - if err != nil { - return err - } - - if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { - notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) - } - } - return nil -} - // UpdateAssignee deletes or adds an assignee to an issue func UpdateAssignee(issue *Issue, doer *User, assigneeID int64) (err error) { _, err = issue.ChangeAssignee(doer, assigneeID) @@ -232,61 +205,6 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in return removed, 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 - } - - 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 -} - // MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string) (assigneeIDs []int64, err error) { diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index aab167bc6816c..a28dd493f98dd 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -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 diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 978c8a3f1f491..3c153191def3d 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" ) @@ -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)) diff --git a/services/issue/issue.go b/services/issue/issue.go index a28916a7f9b5e..6fcb6c78fb131 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -9,6 +9,8 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/notification" + "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" ) @@ -96,3 +98,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 { + removed, err := issue.ChangeAssignee(doer, assigneeID) + if err != nil { + return err + } + + assignee, err := models.GetUserByID(assigneeID) + if err != nil { + return err + } + + if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed) + } + } + return nil +} From 1374db57d5bdc49b9f2ef64d59744c306f8e6998 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 16 Oct 2019 20:37:45 +0200 Subject: [PATCH 05/18] Refer to issue index rather than ID --- templates/mail/issue/assigned.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/mail/issue/assigned.tmpl b/templates/mail/issue/assigned.tmpl index 324f05cb8659b..23cad65dd0922 100644 --- a/templates/mail/issue/assigned.tmpl +++ b/templates/mail/issue/assigned.tmpl @@ -6,7 +6,7 @@ -

@{{.Doer.Name}} assigned you to the {{if eq .Issue.IsPull true}}pull request{{else}}issue{{end}} #{{.Issue.ID}}.

+

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

---
From 630dcebed0cf6f7b402a3a3ad485f2ecec3e7421 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 17 Oct 2019 06:37:34 +0000 Subject: [PATCH 06/18] Disable email notifications completly at initalization if global disable --- modules/notification/notification.go | 5 ++++- routers/repo/issue.go | 2 +- services/issue/issue.go | 3 +-- services/mailer/mail_issue.go | 4 ---- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 8974ff49d3bdb..eabe4db1ead50 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 ( @@ -26,7 +27,9 @@ func RegisterNotifier(notifier base.Notifier) { func init() { RegisterNotifier(ui.NewNotifier()) - RegisterNotifier(mail.NewNotifier()) + if setting.Service.EnableNotifyMail { + RegisterNotifier(mail.NewNotifier()) + } RegisterNotifier(indexer.NewNotifier()) RegisterNotifier(webhook.NewNotifier()) } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 355b1c7ce7eb2..d28bff590e325 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1141,7 +1141,7 @@ func UpdateIssueAssignee(ctx *context.Context) { ctx.ServerError("GetUserByID", err) return } - if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + if !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) } } diff --git a/services/issue/issue.go b/services/issue/issue.go index 6fcb6c78fb131..fa5b224fcda12 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -10,7 +10,6 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" ) @@ -173,7 +172,7 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID return err } - if setting.Service.EnableNotifyMail && !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + if !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed) } } diff --git a/services/mailer/mail_issue.go b/services/mailer/mail_issue.go index b16323909ca5e..a5f3251807aae 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 { From 18187fae0ae826056bbdcbf9c79c23e764546fce Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Thu, 17 Oct 2019 06:42:43 +0000 Subject: [PATCH 07/18] Check of user enbled mail shall be in mail notification function only --- modules/notification/mail/mail.go | 2 +- routers/repo/issue.go | 2 +- services/issue/issue.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/notification/mail/mail.go b/modules/notification/mail/mail.go index 52e2ee1b361c3..f1b40bb7ed2dd 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -93,7 +93,7 @@ func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models 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 { + 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}) } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index d28bff590e325..9668c60c5a060 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1141,7 +1141,7 @@ func UpdateIssueAssignee(ctx *context.Context) { ctx.ServerError("GetUserByID", err) return } - if !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + if !assignee.IsOrganization() { notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) } } diff --git a/services/issue/issue.go b/services/issue/issue.go index fa5b224fcda12..7c7cf2db8a5e1 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -172,7 +172,7 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID return err } - if !assignee.IsOrganization() && assignee.EmailNotifications() == models.EmailNotificationsEnabled { + if !assignee.IsOrganization() { notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed) } } From 99f8db424fc01cc52035f4d22f692fe0903fc36d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 08:21:40 +0200 Subject: [PATCH 08/18] Initialize notifications from routers init function. --- modules/notification/notification.go | 3 ++- routers/init.go | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/notification/notification.go b/modules/notification/notification.go index eabe4db1ead50..53e2689bdd9e7 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -25,7 +25,8 @@ func RegisterNotifier(notifier base.Notifier) { notifiers = append(notifiers, notifier) } -func init() { +// NewContext registers notification handlers +func NewContext() { RegisterNotifier(ui.NewNotifier()) if setting.Service.EnableNotifyMail { RegisterNotifier(mail.NewNotifier()) diff --git a/routers/init.go b/routers/init.go index e4e880dbbb256..bdd978d717abc 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 From ba31fedc1b2112ae6278a4f147f87251261325b5 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 08:38:08 +0200 Subject: [PATCH 09/18] Use the assigned comment when sending assigned mail --- models/issue.go | 2 +- models/issue_assignees.go | 35 ++++++++++++++------------- modules/notification/base/notifier.go | 2 +- modules/notification/base/null.go | 2 +- modules/notification/mail/mail.go | 4 +-- modules/notification/notification.go | 4 +-- routers/repo/issue.go | 4 +-- services/issue/issue.go | 4 +-- services/mailer/mail.go | 4 +-- 9 files changed, 31 insertions(+), 30 deletions(-) diff --git a/models/issue.go b/models/issue.go index bc5440e05a29c..3552c7f8637a2 100644 --- a/models/issue.go +++ b/models/issue.go @@ -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 } diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 0efc86adcaa1d..70f4367ee9596 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -112,60 +112,61 @@ func clearAssigneeByUserID(sess *xorm.Session, userID int64) (err error) { // UpdateAssignee deletes or adds an assignee to an issue func UpdateAssignee(issue *Issue, doer *User, assigneeID int64) (err error) { - _, err = 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) (removed bool, err error) { +func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (removed bool, comment *Comment, err error) { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { - return false, err + return false, nil, err } - removed, err = issue.changeAssignee(sess, doer, assigneeID, false) + removed, comment, err = issue.changeAssignee(sess, doer, assigneeID, false) if err != nil { - return false, err + return false, nil, err } if err := sess.Commit(); err != nil { - return false, err + return false, nil, err } go HookQueue.Add(issue.RepoID) - return removed, nil + return removed, comment, nil } -func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (removed bool, err error) { +func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (removed bool, comment *Comment, err error) { // Update the assignee removed, err = updateIssueAssignee(sess, issue, assigneeID) if err != nil { - return false, fmt.Errorf("UpdateIssueUserByAssignee: %v", err) + return false, nil, fmt.Errorf("UpdateIssueUserByAssignee: %v", err) } // Repo infos if err = issue.loadRepo(sess); err != nil { - return false, 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 false, 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 removed, err + return removed, comment, err } if issue.IsPull { mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypePullRequests) if err = issue.loadPullRequest(sess); err != nil { - return false, fmt.Errorf("loadPullRequest: %v", err) + return false, nil, fmt.Errorf("loadPullRequest: %v", err) } issue.PullRequest.Issue = issue apiPullRequest := &api.PullRequestPayload{ @@ -181,7 +182,7 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in } 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 false, err + return false, nil, err } } else { mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues) @@ -199,10 +200,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 false, err + return false, nil, err } } - return removed, nil + return removed, comment, nil } // MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs diff --git a/modules/notification/base/notifier.go b/modules/notification/base/notifier.go index 54c24754ac01b..c74bb52014dfa 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, assignee *models.User, 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 9801944a6bf8f..9fb08884a1981 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, assignee *models.User, 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 f1b40bb7ed2dd..0900c6dcdf124 100644 --- a/modules/notification/mail/mail.go +++ b/modules/notification/mail/mail.go @@ -91,10 +91,10 @@ func (m *mailNotifier) NotifyPullRequestReview(pr *models.PullRequest, r *models } } -func (m *mailNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool) { +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, []string{assignee.Email}) + mailer.SendIssueAssignedMail(issue, doer, ct, comment, []string{assignee.Email}) } } diff --git a/modules/notification/notification.go b/modules/notification/notification.go index 53e2689bdd9e7..0f1b63cf672bc 100644 --- a/modules/notification/notification.go +++ b/modules/notification/notification.go @@ -142,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, assignee *models.User, 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, assignee, removed) + notifier.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) } } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 9668c60c5a060..0e3494e632ede 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1130,7 +1130,7 @@ func UpdateIssueAssignee(ctx *context.Context) { return } default: - removed, err := issue.ChangeAssignee(ctx.User, assigneeID) + removed, comment, err := issue.ChangeAssignee(ctx.User, assigneeID) if err != nil { ctx.ServerError("ChangeAssignee", err) return @@ -1142,7 +1142,7 @@ func UpdateIssueAssignee(ctx *context.Context) { return } if !assignee.IsOrganization() { - notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed) + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed, comment) } } } diff --git a/services/issue/issue.go b/services/issue/issue.go index 7c7cf2db8a5e1..715e1a5b67025 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -162,7 +162,7 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID } if !isAssigned { - removed, err := issue.ChangeAssignee(doer, assigneeID) + removed, comment, err := issue.ChangeAssignee(doer, assigneeID) if err != nil { return err } @@ -173,7 +173,7 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID } if !assignee.IsOrganization() { - notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed) + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) } } return nil diff --git a/services/mailer/mail.go b/services/mailer/mail.go index 94fe1583a977a..bc2aff7314f88 100644 --- a/services/mailer/mail.go +++ b/services/mailer/mail.go @@ -224,6 +224,6 @@ func SendIssueMentionMail(issue *models.Issue, doer *models.User, content string } // SendIssueAssignedMail composes and sends issue assigned email -func SendIssueAssignedMail(issue *models.Issue, doer *models.User, content string, tos []string) { - SendAsync(composeIssueCommentMessage(issue, doer, content, nil, mailIssueAssigned, tos, "issue assigned")) +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")) } From 04450f9d9144e4fc23ff83d9c3adf7ad0f6edb37 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 17:01:23 +0200 Subject: [PATCH 10/18] Refactor so that assignees always added as separate step when new issue/pr. --- models/issue.go | 49 +++--------------------------------- models/issue_test.go | 2 +- models/pull.go | 7 +++--- routers/api/v1/repo/issue.go | 8 +++--- routers/api/v1/repo/pull.go | 8 +++--- routers/repo/issue.go | 4 ++- routers/repo/pull.go | 5 +++- services/issue/issue.go | 18 ++++++++++--- services/pull/pull.go | 4 +-- 9 files changed, 40 insertions(+), 65 deletions(-) diff --git a/models/issue.go b/models/issue.go index 3552c7f8637a2..97470fd0a7eb4 100644 --- a/models/issue.go +++ b/models/issue.go @@ -984,7 +984,6 @@ type NewIssueOptions struct { Repo *Repository Issue *Issue LabelIDs []int64 - AssigneeIDs []int64 Attachments []string // In UUID format. IsPull bool } @@ -1006,39 +1005,6 @@ 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. if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1"). Where("repo_id=?", opts.Issue.RepoID). @@ -1060,14 +1026,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 { @@ -1125,11 +1083,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) { @@ -1143,7 +1101,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 { @@ -1155,7 +1113,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_test.go b/models/issue_test.go index 0be3f68808254..eaa77bcfe1eb0 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -344,7 +344,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/pull.go b/models/pull.go index 962e433fb0759..2b71eba34c102 100644 --- a/models/pull.go +++ b/models/pull.go @@ -656,11 +656,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) { @@ -674,7 +674,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 { @@ -687,7 +687,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/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index a28dd493f98dd..86701ff91adbb 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -218,7 +218,7 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { 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 +227,8 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { return } + issue_service.AddAssignees(issue, ctx.User, assigneeIDs) + notification.NotifyNewIssue(issue) if form.Closed { @@ -336,9 +338,9 @@ func EditIssue(ctx *context.APIContext, form api.EditIssueOption) { oneAssignee = *form.Assignee } - err = issue_service.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 3c153191def3d..c48ff12339df3 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -288,7 +288,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption return } - if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, patch, assigneeIDs); err != nil { + if err := pull_service.NewPullRequest(repo, prIssue, labelIDs, []string{}, pr, patch); err != nil { if models.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(400, "UserDoesNotHaveAccessToRepo", err) return @@ -300,6 +300,8 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption return } + issue_service.AddAssignees(prIssue, ctx.User, assigneeIDs) + notification.NotifyNewPullRequest(pr) log.Trace("Pull request created: %d/%d", repo.ID, prIssue.ID) @@ -389,12 +391,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 = issue_service.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/repo/issue.go b/routers/repo/issue.go index 0e3494e632ede..8986ac49b5061 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -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,8 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { return } + issue_service.AddAssignees(issue, ctx.User, assigneeIDs) + notification.NotifyNewIssue(issue) log.Trace("Issue created: %d/%d", repo.ID, issue.ID) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 8b97e55670987..4ee74db31bb8f 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" @@ -767,7 +768,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 @@ -779,6 +780,8 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } + issue_service.AddAssignees(pullIssue, ctx.User, assigneeIDs) + 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 715e1a5b67025..59e22bf6d5751 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -14,8 +14,8 @@ import ( ) // 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 } @@ -98,13 +98,13 @@ 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) +// 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 UpdateAPIAssignee(issue *models.Issue, oneAssignee string, multipleAssignees []string, doer *models.User) (err error) { +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 @@ -178,3 +178,13 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID } 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/pull/pull.go b/services/pull/pull.go index 3c584fce74844..959da6740528d 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 } From fea106d525b6c279acad3610d2e708313fb82e79 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 19:06:04 +0200 Subject: [PATCH 11/18] Check error from AddAssignees --- routers/api/v1/repo/issue.go | 5 ++++- routers/api/v1/repo/pull.go | 5 ++++- routers/repo/issue.go | 5 ++++- routers/repo/pull.go | 5 ++++- 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 86701ff91adbb..97b54cb16dec0 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -227,7 +227,10 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { return } - issue_service.AddAssignees(issue, ctx.User, assigneeIDs) + if err := issue_service.AddAssignees(issue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } notification.NotifyNewIssue(issue) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index c48ff12339df3..623110986343b 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -300,7 +300,10 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption return } - issue_service.AddAssignees(prIssue, ctx.User, assigneeIDs) + if err := issue_service.AddAssignees(prIssue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } notification.NotifyNewPullRequest(pr) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 8986ac49b5061..9824c15eb8b42 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -583,7 +583,10 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { return } - issue_service.AddAssignees(issue, ctx.User, assigneeIDs) + if err := issue_service.AddAssignees(issue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } notification.NotifyNewIssue(issue) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 4ee74db31bb8f..ebf8f1724f65d 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -780,7 +780,10 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) return } - issue_service.AddAssignees(pullIssue, ctx.User, assigneeIDs) + if err := issue_service.AddAssignees(pullIssue, ctx.User, assigneeIDs); err != nil { + ctx.ServerError("AddAssignees", err) + return + } notification.NotifyNewPullRequest(pullRequest) From 5f6f77f0cac2d8bd7f67cd6cd250c8b90f4a708d Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 19:46:11 +0200 Subject: [PATCH 12/18] Check if user can be assiged to issue or pull request --- models/repo_permission.go | 19 ++++++++++++++++--- routers/repo/issue.go | 31 ++++++++++++++++++++++++------- services/issue/issue.go | 25 ++++++++++++++++++++----- 3 files changed, 60 insertions(+), 15 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index 916678d16859e..c6bb946213a54 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -329,10 +329,23 @@ 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 +func canBeAssignedIssues(e Engine, user *User, repo *Repository) (bool, error) { + return hasAccessUnit(e, user, repo, UnitTypeIssues, AccessModeWrite) +} + +// CanBeAssignedIssues return true if user can be assigned to issues in repo +func CanBeAssignedIssues(user *User, repo *Repository) (bool, error) { + return canBeAssignedIssues(x, user, repo) +} + // 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 canBeAssignedPullRequests(e Engine, user *User, repo *Repository) (bool, error) { + return hasAccessUnit(e, user, repo, UnitTypePullRequests, AccessModeWrite) +} + +// CanBeAssignedPullRequests return true if user can be assigned to pull requests in repo +func CanBeAssignedPullRequests(user *User, repo *Repository) (bool, error) { + return canBeAssignedPullRequests(x, user, repo) } func hasAccess(e Engine, userID int64, repo *Repository) (bool, error) { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 9824c15eb8b42..e1cadb577a605 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1117,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() { @@ -1135,20 +1135,37 @@ func UpdateIssueAssignee(ctx *context.Context) { return } default: - removed, comment, err := issue.ChangeAssignee(ctx.User, assigneeID) + assignee, err := models.GetUserByID(assigneeID) if err != nil { - ctx.ServerError("ChangeAssignee", err) + ctx.ServerError("GetUserByID", err) return } + if assignee.IsOrganization() { + ctx.ServerError("assignee.IsOrganization", fmt.Errorf("Organization can't be added as assignee [user_id: %d, repo_id: %d]", assigneeID, issue.RepoID)) + return + } + var valid bool + if issue.IsPull { + valid, err = models.CanBeAssignedPullRequests(assignee, issue.Repo) + } else { + valid, err = models.CanBeAssignedIssues(assignee, issue.Repo) + } - assignee, err := models.GetUserByID(assigneeID) if err != nil { - ctx.ServerError("GetUserByID", err) + ctx.ServerError("canBeAssigned", err) return } - if !assignee.IsOrganization() { - notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed, comment) + if !valid { + ctx.ServerError("caanBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}) + } + + removed, comment, err := issue.ChangeAssignee(ctx.User, assigneeID) + if err != nil { + ctx.ServerError("ChangeAssignee", err) + return } + + notification.NotifyIssueChangeAssignee(ctx.User, issue, assignee, removed, comment) } } ctx.JSON(200, map[string]interface{}{ diff --git a/services/issue/issue.go b/services/issue/issue.go index 59e22bf6d5751..cf3ef67b19ee5 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -153,7 +153,8 @@ func UpdateAssignees(issue *models.Issue, oneAssignee string, multipleAssignees return } -// AddAssigneeIfNotAssigned adds an assignee only if he isn't aleady assigned to the issue +// 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) { // Check if the user is already assigned isAssigned, err := models.IsUserAssignedToIssue(issue, &models.User{ID: assigneeID}) @@ -162,19 +163,33 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID } if !isAssigned { - removed, comment, err := issue.ChangeAssignee(doer, assigneeID) + assignee, err := models.GetUserByID(assigneeID) if err != nil { return err } + if assignee.IsOrganization() { + return fmt.Errorf("Organization can't be added as assignee [user_id: %d, repo_id: %d]", assigneeID, issue.RepoID) + } + var valid bool + if issue.IsPull { + valid, err = models.CanBeAssignedPullRequests(assignee, issue.Repo) + } else { + valid, err = models.CanBeAssignedIssues(assignee, issue.Repo) + } - assignee, err := models.GetUserByID(assigneeID) if err != nil { return err } + if !valid { + return models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} + } - if !assignee.IsOrganization() { - notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) + removed, comment, err := issue.ChangeAssignee(doer, assigneeID) + if err != nil { + return err } + + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) } return nil } From 15b84ed746578eedfb911836f23ad240e7768b83 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Fri, 18 Oct 2019 23:14:05 +0200 Subject: [PATCH 13/18] Missing return --- routers/repo/issue.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index e1cadb577a605..64a0cc0d642d2 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1156,7 +1156,8 @@ func UpdateIssueAssignee(ctx *context.Context) { return } if !valid { - ctx.ServerError("caanBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}) + ctx.ServerError("canBeAssigned", models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}) + return } removed, comment, err := issue.ChangeAssignee(ctx.User, assigneeID) From a7562d12bd0b9d64e20560653a7106b318ccd521 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 19 Oct 2019 22:34:16 +0200 Subject: [PATCH 14/18] Refactor of CanBeAssigned check. CanBeAssigned shall have same check as UI. --- models/repo_permission.go | 27 +++++++++++---------------- routers/api/v1/repo/issue.go | 19 +++++++++++++++++++ routers/api/v1/repo/pull.go | 18 ++++++++++++++++++ routers/repo/issue.go | 23 +++++++---------------- services/issue/issue.go | 10 +--------- 5 files changed, 56 insertions(+), 41 deletions(-) diff --git a/models/repo_permission.go b/models/repo_permission.go index c6bb946213a54..fad29bd169598 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -329,23 +329,18 @@ func HasAccessUnit(user *User, repo *Repository, unitType UnitType, testMode Acc return hasAccessUnit(x, user, repo, unitType, testMode) } -func canBeAssignedIssues(e Engine, user *User, repo *Repository) (bool, error) { - return hasAccessUnit(e, user, repo, UnitTypeIssues, AccessModeWrite) -} - -// CanBeAssignedIssues return true if user can be assigned to issues in repo -func CanBeAssignedIssues(user *User, repo *Repository) (bool, error) { - return canBeAssignedIssues(x, user, 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 canBeAssignedPullRequests(e Engine, user *User, repo *Repository) (bool, error) { - return hasAccessUnit(e, user, repo, UnitTypePullRequests, AccessModeWrite) -} - -// CanBeAssignedPullRequests return true if user can be assigned to pull requests in repo -func CanBeAssignedPullRequests(user *User, repo *Repository) (bool, error) { - return canBeAssignedPullRequests(x, user, repo) +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/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 97b54cb16dec0..9529e09b29002 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -213,6 +213,25 @@ 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) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 623110986343b..ea1d602006fef 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -287,6 +287,24 @@ 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 + } + + 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) { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 64a0cc0d642d2..981efdfa94fe3 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 } } @@ -1140,17 +1140,8 @@ func UpdateIssueAssignee(ctx *context.Context) { ctx.ServerError("GetUserByID", err) return } - if assignee.IsOrganization() { - ctx.ServerError("assignee.IsOrganization", fmt.Errorf("Organization can't be added as assignee [user_id: %d, repo_id: %d]", assigneeID, issue.RepoID)) - return - } - var valid bool - if issue.IsPull { - valid, err = models.CanBeAssignedPullRequests(assignee, issue.Repo) - } else { - valid, err = models.CanBeAssignedIssues(assignee, issue.Repo) - } + valid, err := models.CanBeAssigned(assignee, issue.Repo, issue.IsPull) if err != nil { ctx.ServerError("canBeAssigned", err) return diff --git a/services/issue/issue.go b/services/issue/issue.go index cf3ef67b19ee5..970607f529cb7 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -167,16 +167,8 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID if err != nil { return err } - if assignee.IsOrganization() { - return fmt.Errorf("Organization can't be added as assignee [user_id: %d, repo_id: %d]", assigneeID, issue.RepoID) - } - var valid bool - if issue.IsPull { - valid, err = models.CanBeAssignedPullRequests(assignee, issue.Repo) - } else { - valid, err = models.CanBeAssignedIssues(assignee, issue.Repo) - } + valid, err := models.CanBeAssigned(assignee, issue.Repo, issue.IsPull) if err != nil { return err } From 075c7c6a0600e8844101ad738a24819c2130213a Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sat, 19 Oct 2019 23:33:21 +0200 Subject: [PATCH 15/18] Clarify function names (toggle rather than update/change), and clean up. --- models/issue.go | 2 +- models/issue_assignees.go | 64 ++++++++++++++++++++++++++-------- models/issue_assignees_test.go | 6 ++-- models/issue_user.go | 38 -------------------- routers/repo/issue.go | 4 +-- services/issue/issue.go | 41 ++++++++++++---------- 6 files changed, 78 insertions(+), 77 deletions(-) diff --git a/models/issue.go b/models/issue.go index 97470fd0a7eb4..391a504854775 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1005,7 +1005,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { } } - // 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 { diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 70f4367ee9596..2108c936d96c6 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -58,7 +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 isUserAssignedToIssue(x, issue, user) +} + +func isUserAssignedToIssue(e Engine, issue *Issue, user *User) (isAssigned bool, err error) { + isAssigned, err = e.Get(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID}) return } @@ -78,7 +82,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,14 +114,8 @@ func clearAssigneeByUserID(sess *xorm.Session, userID int64) (err error) { return } -// UpdateAssignee deletes or adds an assignee to an issue -func UpdateAssignee(issue *Issue, doer *User, assigneeID int64) (err error) { - _, _, err = issue.ChangeAssignee(doer, assigneeID) - return err -} - -// ChangeAssignee changes the Assignee of this issue. -func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (removed bool, comment *Comment, 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() @@ -125,7 +123,7 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (removed bool, return false, nil, err } - removed, comment, err = issue.changeAssignee(sess, doer, assigneeID, false) + removed, comment, err = issue.toggleAssignee(sess, doer, assigneeID, false) if err != nil { return false, nil, err } @@ -139,9 +137,8 @@ func (issue *Issue) ChangeAssignee(doer *User, assigneeID int64) (removed bool, return removed, comment, nil } -func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID int64, isCreate bool) (removed bool, comment *Comment, 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 false, nil, fmt.Errorf("UpdateIssueUserByAssignee: %v", err) } @@ -180,6 +177,7 @@ 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 false, nil, err @@ -198,6 +196,7 @@ 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 false, nil, err @@ -206,6 +205,43 @@ func (issue *Issue) changeAssignee(sess *xorm.Session, doer *User, assigneeID in return removed, comment, nil } +// toggles user assignee state in database +func toggleUserAssignee(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 +} + // MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string) (assigneeIDs []int64, err error) { diff --git a/models/issue_assignees_test.go b/models/issue_assignees_test.go index d32f41737a3e0..1c5b5e7a22f40 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_user.go b/models/issue_user.go index d55a0dc2fb85d..67a118fe57929 100644 --- a/models/issue_user.go +++ b/models/issue_user.go @@ -6,8 +6,6 @@ package models import ( "fmt" - - "github.com/go-xorm/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/routers/repo/issue.go b/routers/repo/issue.go index 981efdfa94fe3..379d080e65427 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1151,9 +1151,9 @@ func UpdateIssueAssignee(ctx *context.Context) { return } - removed, comment, err := issue.ChangeAssignee(ctx.User, assigneeID) + removed, comment, err := issue.ToggleAssignee(ctx.User, assigneeID) if err != nil { - ctx.ServerError("ChangeAssignee", err) + ctx.ServerError("ToggleAssignee", err) return } diff --git a/services/issue/issue.go b/services/issue/issue.go index 970607f529cb7..a5f725ab70258 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -156,33 +156,36 @@ func UpdateAssignees(issue *models.Issue, oneAssignee string, multipleAssignees // 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, &models.User{ID: assigneeID}) + isAssigned, err := models.IsUserAssignedToIssue(issue, assignee) if err != nil { return err } + if isAssigned { + // nothing to to + return nil + } - if !isAssigned { - assignee, err := models.GetUserByID(assigneeID) - if err != nil { - return err - } + 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} + } - 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 + } - removed, comment, err := issue.ChangeAssignee(doer, assigneeID) - if err != nil { - return err - } + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) - notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) - } return nil } From 0d5202c4c7f29531a73bcc3dfe6fb599b68838a6 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Sun, 20 Oct 2019 08:43:33 +0200 Subject: [PATCH 16/18] Fix review comments. --- models/issue_assignees.go | 5 ++--- templates/mail/issue/assigned.tmpl | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index b317b345a9ca6..cbdb368cbe855 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -62,8 +62,7 @@ func IsUserAssignedToIssue(issue *Issue, user *User) (isAssigned bool, err error } func isUserAssignedToIssue(e Engine, issue *Issue, user *User) (isAssigned bool, err error) { - isAssigned, err = e.Get(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID}) - return + return e.Get(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID}) } // DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array @@ -214,7 +213,7 @@ func toggleUserAssignee(e *xorm.Session, issue *Issue, assigneeID int64) (remove return false, err } - // Check if the submitted user is already assigne, if yes delete him otherwise add him + // 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 { diff --git a/templates/mail/issue/assigned.tmpl b/templates/mail/issue/assigned.tmpl index 23cad65dd0922..ab06ade1f4cd5 100644 --- a/templates/mail/issue/assigned.tmpl +++ b/templates/mail/issue/assigned.tmpl @@ -6,7 +6,7 @@ -

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

+

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

---
From abc21a528b4056a7c9f165404def099b8aae6a33 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Mon, 21 Oct 2019 06:20:19 +0000 Subject: [PATCH 17/18] Flash error if assignees was not added when creating issue/pr --- options/locale/locale_en-US.ini | 1 + routers/repo/issue.go | 4 ++-- routers/repo/pull.go | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7b65de6addea4..2e85976bf6622 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -976,6 +976,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/repo/issue.go b/routers/repo/issue.go index 379d080e65427..94c39ae2240ec 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -584,8 +584,8 @@ func NewIssuePost(ctx *context.Context, form auth.CreateIssueForm) { } if err := issue_service.AddAssignees(issue, ctx.User, assigneeIDs); err != nil { - ctx.ServerError("AddAssignees", err) - return + log.Error("AddAssignees: %v", err) + ctx.Flash.Error(ctx.Tr("issues.assignee.error")) } notification.NotifyNewIssue(issue) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 56c671e01786d..66bfa43ee2ffd 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -780,8 +780,8 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) } if err := issue_service.AddAssignees(pullIssue, ctx.User, assigneeIDs); err != nil { - ctx.ServerError("AddAssignees", err) - return + log.Error("AddAssignees: %v", err) + ctx.Flash.Error(ctx.Tr("issues.assignee.error")) } notification.NotifyNewPullRequest(pullRequest) From 430a3c870f702c78fd0f171c4bfa29771fb53fc6 Mon Sep 17 00:00:00 2001 From: David Svantesson Date: Wed, 23 Oct 2019 08:41:19 +0200 Subject: [PATCH 18/18] Generate error if assignee users doesn't exist --- models/issue_assignees.go | 2 +- models/user.go | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index cbdb368cbe855..ed0576b38b071 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -262,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/user.go b/models/user.go index a3679c9a5b01f..7aa1e143e835b 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.