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

@@ -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
 }
 

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))
 }

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 {

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)
+	}
+}

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

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
+}

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))
+}

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)
+	notification.NotifyIssueChangeTitle(doer, issue, oldTitle)
-	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)
-	}
 
 	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
 }