From 495d5e4329326b27158a25b44c37986923d0bb6b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 28 Oct 2019 10:11:50 +0800 Subject: [PATCH] Move more issue assignee code from models to issue service (#8690) * Move more issue assignee code from models to issue service * fix test --- models/issue_assignees.go | 73 -------------------- models/issue_assignees_test.go | 9 --- models/repo_permission.go | 6 ++ modules/notification/webhook/webhook.go | 92 +++++++++++++++++++++++++ routers/repo/issue.go | 7 +- services/issue/assignee.go | 53 ++++++++++++++ services/issue/assignee_test.go | 37 ++++++++++ services/issue/issue.go | 45 +----------- 8 files changed, 193 insertions(+), 129 deletions(-) create mode 100644 services/issue/assignee.go create mode 100644 services/issue/assignee_test.go diff --git a/models/issue_assignees.go b/models/issue_assignees.go index ed0576b38..e15b718eb 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -7,9 +7,6 @@ package models import ( "fmt" - "code.gitea.io/gitea/modules/log" - api "code.gitea.io/gitea/modules/structs" - "xorm.io/xorm" ) @@ -65,31 +62,6 @@ func isUserAssignedToIssue(e Engine, issue *Issue, user *User) (isAssigned bool, return e.Get(&IssueAssignees{IssueID: issue.ID, AssigneeID: user.ID}) } -// DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array -func DeleteNotPassedAssignee(issue *Issue, doer *User, assignees []*User) (err error) { - var found bool - - for _, assignee := range issue.Assignees { - - found = false - for _, alreadyAssignee := range assignees { - if assignee.ID == alreadyAssignee.ID { - found = true - break - } - } - - 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 := issue.ToggleAssignee(doer, assignee.ID); err != nil { - return err - } - } - } - - return nil -} - // MakeAssigneeList concats a string with all names of the assignees. Useful for logs. func MakeAssigneeList(issue *Issue) (assigneeList string, err error) { err = issue.loadAssignees(x) @@ -131,8 +103,6 @@ func (issue *Issue) ToggleAssignee(doer *User, assigneeID int64) (removed bool, return false, nil, err } - go HookQueue.Add(issue.RepoID) - return removed, comment, nil } @@ -158,49 +128,6 @@ func (issue *Issue) toggleAssignee(sess *xorm.Session, doer *User, assigneeID in return removed, comment, err } - if issue.IsPull { - mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypePullRequests) - - if err = issue.loadPullRequest(sess); err != nil { - return false, nil, fmt.Errorf("loadPullRequest: %v", err) - } - issue.PullRequest.Issue = issue - apiPullRequest := &api.PullRequestPayload{ - Index: issue.Index, - PullRequest: issue.PullRequest.apiFormat(sess), - Repository: issue.Repo.innerAPIFormat(sess, mode, false), - Sender: doer.APIFormat(), - } - if removed { - apiPullRequest.Action = api.HookIssueUnassigned - } 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 - } - } else { - mode, _ := accessLevelUnit(sess, doer, issue.Repo, UnitTypeIssues) - - apiIssue := &api.IssuePayload{ - Index: issue.Index, - Issue: issue.apiFormat(sess), - Repository: issue.Repo.innerAPIFormat(sess, mode, false), - Sender: doer.APIFormat(), - } - if removed { - apiIssue.Action = api.HookIssueUnassigned - } 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 - } - } return removed, comment, nil } diff --git a/models/issue_assignees_test.go b/models/issue_assignees_test.go index 1c5b5e7a2..163234b16 100644 --- a/models/issue_assignees_test.go +++ b/models/issue_assignees_test.go @@ -58,13 +58,4 @@ func TestUpdateAssignee(t *testing.T) { isAssigned, err = IsUserAssignedToIssue(issue, &User{ID: 4}) assert.NoError(t, err) assert.False(t, isAssigned) - - // Clean everyone - err = DeleteNotPassedAssignee(issue, user1, []*User{}) - assert.NoError(t, err) - - // Check they're gone - assignees, err = GetAssigneesByIssue(issue) - assert.NoError(t, err) - assert.Equal(t, 0, len(assignees)) } diff --git a/models/repo_permission.go b/models/repo_permission.go index fad29bd16..782b19562 100644 --- a/models/repo_permission.go +++ b/models/repo_permission.go @@ -311,6 +311,12 @@ func AccessLevel(user *User, repo *Repository) (AccessMode, error) { return accessLevelUnit(x, user, repo, UnitTypeCode) } +// AccessLevelUnit returns the Access a user has to a repository's. Will return NoneAccess if the +// user does not have access. +func AccessLevelUnit(user *User, repo *Repository, unitType UnitType) (AccessMode, error) { + return accessLevelUnit(x, user, repo, unitType) +} + func accessLevelUnit(e Engine, user *User, repo *Repository, unitType UnitType) (AccessMode, error) { perm, err := getUserRepoPermission(e, repo, user) if err != nil { diff --git a/modules/notification/webhook/webhook.go b/modules/notification/webhook/webhook.go index 13f2f4486..704b42ec8 100644 --- a/modules/notification/webhook/webhook.go +++ b/modules/notification/webhook/webhook.go @@ -129,3 +129,95 @@ func (m *webhookNotifier) NotifyDeleteRepository(doer *models.User, repo *models go models.HookQueue.Add(repo.ID) } } + +func (m *webhookNotifier) NotifyIssueChangeAssignee(doer *models.User, issue *models.Issue, assignee *models.User, removed bool, comment *models.Comment) { + if issue.IsPull { + mode, _ := models.AccessLevelUnit(doer, issue.Repo, models.UnitTypePullRequests) + + if err := issue.LoadPullRequest(); err != nil { + log.Error("LoadPullRequest failed: %v", err) + return + } + issue.PullRequest.Issue = issue + apiPullRequest := &api.PullRequestPayload{ + Index: issue.Index, + PullRequest: issue.PullRequest.APIFormat(), + Repository: issue.Repo.APIFormat(mode), + Sender: doer.APIFormat(), + } + if removed { + apiPullRequest.Action = api.HookIssueUnassigned + } else { + apiPullRequest.Action = api.HookIssueAssigned + } + // Assignee comment triggers a webhook + if err := models.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, apiPullRequest); err != nil { + log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err) + return + } + } else { + mode, _ := models.AccessLevelUnit(doer, issue.Repo, models.UnitTypeIssues) + + apiIssue := &api.IssuePayload{ + Index: issue.Index, + Issue: issue.APIFormat(), + Repository: issue.Repo.APIFormat(mode), + Sender: doer.APIFormat(), + } + if removed { + apiIssue.Action = api.HookIssueUnassigned + } else { + apiIssue.Action = api.HookIssueAssigned + } + // Assignee comment triggers a webhook + if err := models.PrepareWebhooks(issue.Repo, models.HookEventIssues, apiIssue); err != nil { + log.Error("PrepareWebhooks [is_pull: %v, remove_assignee: %v]: %v", issue.IsPull, removed, err) + return + } + } + + go models.HookQueue.Add(issue.RepoID) +} + +func (m *webhookNotifier) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) { + mode, _ := models.AccessLevel(issue.Poster, issue.Repo) + var err error + if issue.IsPull { + if err = issue.LoadPullRequest(); err != nil { + log.Error("LoadPullRequest failed: %v", err) + return + } + issue.PullRequest.Issue = issue + err = models.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ + Action: api.HookIssueEdited, + Index: issue.Index, + Changes: &api.ChangesPayload{ + Title: &api.ChangesFromPayload{ + From: oldTitle, + }, + }, + PullRequest: issue.PullRequest.APIFormat(), + Repository: issue.Repo.APIFormat(mode), + Sender: doer.APIFormat(), + }) + } else { + err = models.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ + Action: api.HookIssueEdited, + Index: issue.Index, + Changes: &api.ChangesPayload{ + Title: &api.ChangesFromPayload{ + From: oldTitle, + }, + }, + Issue: issue.APIFormat(), + Repository: issue.Repo.APIFormat(mode), + Sender: issue.Poster.APIFormat(), + }) + } + + if err != nil { + log.Error("PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err) + } else { + go models.HookQueue.Add(issue.RepoID) + } +} diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 94c39ae22..4e755b719 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -1050,14 +1050,11 @@ func UpdateIssueTitle(ctx *context.Context) { return } - oldTitle := issue.Title if err := issue_service.ChangeTitle(issue, ctx.User, title); err != nil { ctx.ServerError("ChangeTitle", err) return } - notification.NotifyIssueChangeTitle(ctx.User, issue, oldTitle) - ctx.JSON(200, map[string]interface{}{ "title": issue.Title, }) @@ -1130,7 +1127,7 @@ func UpdateIssueAssignee(ctx *context.Context) { for _, issue := range issues { switch action { case "clear": - if err := models.DeleteNotPassedAssignee(issue, ctx.User, []*models.User{}); err != nil { + if err := issue_service.DeleteNotPassedAssignee(issue, ctx.User, []*models.User{}); err != nil { ctx.ServerError("ClearAssignees", err) return } @@ -1151,7 +1148,7 @@ func UpdateIssueAssignee(ctx *context.Context) { return } - removed, comment, err := issue.ToggleAssignee(ctx.User, assigneeID) + removed, comment, err := issue_service.ToggleAssignee(issue, ctx.User, assigneeID) if err != nil { ctx.ServerError("ToggleAssignee", err) return diff --git a/services/issue/assignee.go b/services/issue/assignee.go new file mode 100644 index 000000000..281f824da --- /dev/null +++ b/services/issue/assignee.go @@ -0,0 +1,53 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package issue + +import ( + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/notification" +) + +// DeleteNotPassedAssignee deletes all assignees who aren't passed via the "assignees" array +func DeleteNotPassedAssignee(issue *models.Issue, doer *models.User, assignees []*models.User) (err error) { + var found bool + + for _, assignee := range issue.Assignees { + + found = false + for _, alreadyAssignee := range assignees { + if assignee.ID == alreadyAssignee.ID { + found = true + break + } + } + + 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 := ToggleAssignee(issue, doer, assignee.ID); err != nil { + return err + } + } + } + + return nil +} + +// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it. +func ToggleAssignee(issue *models.Issue, doer *models.User, assigneeID int64) (removed bool, comment *models.Comment, err error) { + removed, comment, err = issue.ToggleAssignee(doer, assigneeID) + if err != nil { + return + } + + assignee, err1 := models.GetUserByID(assigneeID) + if err1 != nil { + err = err1 + return + } + + notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) + + return +} diff --git a/services/issue/assignee_test.go b/services/issue/assignee_test.go new file mode 100644 index 000000000..bdd2009bf --- /dev/null +++ b/services/issue/assignee_test.go @@ -0,0 +1,37 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package issue + +import ( + "testing" + + "code.gitea.io/gitea/models" + "github.com/stretchr/testify/assert" +) + +func TestDeleteNotPassedAssignee(t *testing.T) { + assert.NoError(t, models.PrepareTestDatabase()) + + // Fake issue with assignees + issue, err := models.GetIssueWithAttrsByID(1) + assert.NoError(t, err) + + user1, err := models.GetUserByID(1) // This user is already assigned (see the definition in fixtures), so running UpdateAssignee should unassign him + assert.NoError(t, err) + + // Check if he got removed + isAssigned, err := models.IsUserAssignedToIssue(issue, user1) + assert.NoError(t, err) + assert.True(t, isAssigned) + + // Clean everyone + err = DeleteNotPassedAssignee(issue, user1, []*models.User{}) + assert.NoError(t, err) + + // Check they're gone + assignees, err := models.GetAssigneesByIssue(issue) + assert.NoError(t, err) + assert.Equal(t, 0, len(assignees)) +} diff --git a/services/issue/issue.go b/services/issue/issue.go index a5f725ab7..06472d865 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -56,44 +56,7 @@ func ChangeTitle(issue *models.Issue, doer *models.User, title string) (err erro return } - mode, _ := models.AccessLevel(issue.Poster, issue.Repo) - if issue.IsPull { - if err = issue.LoadPullRequest(); err != nil { - return fmt.Errorf("loadPullRequest: %v", err) - } - issue.PullRequest.Issue = issue - err = models.PrepareWebhooks(issue.Repo, models.HookEventPullRequest, &api.PullRequestPayload{ - Action: api.HookIssueEdited, - Index: issue.Index, - Changes: &api.ChangesPayload{ - Title: &api.ChangesFromPayload{ - From: oldTitle, - }, - }, - PullRequest: issue.PullRequest.APIFormat(), - Repository: issue.Repo.APIFormat(mode), - Sender: doer.APIFormat(), - }) - } else { - err = models.PrepareWebhooks(issue.Repo, models.HookEventIssues, &api.IssuePayload{ - Action: api.HookIssueEdited, - Index: issue.Index, - Changes: &api.ChangesPayload{ - Title: &api.ChangesFromPayload{ - From: oldTitle, - }, - }, - Issue: issue.APIFormat(), - Repository: issue.Repo.APIFormat(mode), - Sender: issue.Poster.APIFormat(), - }) - } - - if err != nil { - log.Error("PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err) - } else { - go models.HookQueue.Add(issue.RepoID) - } + notification.NotifyIssueChangeTitle(doer, issue, oldTitle) return nil } @@ -134,7 +97,7 @@ func UpdateAssignees(issue *models.Issue, oneAssignee string, multipleAssignees } // Delete all old assignees not passed - if err = models.DeleteNotPassedAssignee(issue, doer, allNewAssignees); err != nil { + if err = DeleteNotPassedAssignee(issue, doer, allNewAssignees); err != nil { return err } @@ -179,13 +142,11 @@ func AddAssigneeIfNotAssigned(issue *models.Issue, doer *models.User, assigneeID return models.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} } - removed, comment, err := issue.ToggleAssignee(doer, assigneeID) + _, _, err = ToggleAssignee(issue, doer, assigneeID) if err != nil { return err } - notification.NotifyIssueChangeAssignee(doer, issue, assignee, removed, comment) - return nil }