From e72d94129c4666d5151f6131cb2f8c1de127d9d0 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 7 Nov 2019 18:56:50 +0100 Subject: [PATCH 01/15] keep sure if assigneeIDs == nil -> do nothing --- routers/api/v1/repo/issue.go | 34 ++++++++++++++++++---------------- routers/api/v1/repo/pull.go | 33 ++++++++++++++++++--------------- 2 files changed, 36 insertions(+), 31 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 186e66cb8f8c6..2df9811802577 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -344,22 +344,24 @@ 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 + if assigneeIDs != nil { + for _, aID := range assigneeIDs { + assignee, err := models.GetUserByID(aID) + if err != nil { + ctx.Error(500, "GetUserByID", err) + return + } + + // Check if the passed assignees is assignable + 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 { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 6d86105a15d13..1e1eb2ceaf973 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -286,22 +286,25 @@ 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 assigneeIDs != nil { + for _, aID := range assigneeIDs { + assignee, err := models.GetUserByID(aID) + if err != nil { + ctx.Error(500, "GetUserByID", err) + return + } + + // Check if the passed assignees is assignable + 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 + } } } From 3139ccf22af573d33f1f9bb4e36e2cf2b2fba170 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 7 Nov 2019 18:57:12 +0100 Subject: [PATCH 02/15] fix #8872 --- models/issue_assignees.go | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index e15b718eb2afc..f4aa250ddcb51 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -171,25 +171,34 @@ func toggleUserAssignee(e *xorm.Session, issue *Issue, assigneeID int64) (remove // MakeIDsFromAPIAssigneesToAdd returns an array with all assignee IDs func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string) (assigneeIDs []int64, err error) { - // Keeping the old assigning method for compatibility reasons - if oneAssignee != "" { - - // Prevent double adding assignees - var isDouble bool - for _, assignee := range multipleAssignees { - if assignee == oneAssignee { - isDouble = true - break - } + var requestAssignees []string + + // Prevent double adding assignees and empty assignees + var isDouble bool + for _, assignee := range multipleAssignees { + // Keeping the old assigning method for compatibility reasons + if assignee == oneAssignee { + isDouble = true + continue } - if !isDouble { - multipleAssignees = append(multipleAssignees, oneAssignee) + //if assignee is empty skip + if len(assignee) == 0 { + continue } + requestAssignees = append(requestAssignees, assignee) } - // Get the IDs of all assignees - assigneeIDs, err = GetUserIDsByNames(multipleAssignees, false) + if !isDouble { + requestAssignees = append(requestAssignees, oneAssignee) + } + + if len(requestAssignees) > 0 { + // Get the IDs of all assignees + assigneeIDs, err = GetUserIDsByNames(requestAssignees, false) + } else { + return nil, nil + } return } From b67543421515d3790a2c4f01da640f6f7e0d0b95 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 7 Nov 2019 19:03:04 +0100 Subject: [PATCH 03/15] Revert "keep sure if assigneeIDs == nil -> do nothing" -> go handle it itself preaty well This reverts commit e72d94129c4666d5151f6131cb2f8c1de127d9d0. --- routers/api/v1/repo/issue.go | 34 ++++++++++++++++------------------ routers/api/v1/repo/pull.go | 33 +++++++++++++++------------------ 2 files changed, 31 insertions(+), 36 deletions(-) diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 2df9811802577..186e66cb8f8c6 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -344,24 +344,22 @@ func CreateIssue(ctx *context.APIContext, form api.CreateIssueOption) { return } - if assigneeIDs != nil { - for _, aID := range assigneeIDs { - assignee, err := models.GetUserByID(aID) - if err != nil { - ctx.Error(500, "GetUserByID", err) - return - } - - // Check if the passed assignees is assignable - 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 - } + // 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 { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1e1eb2ceaf973..6d86105a15d13 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -286,25 +286,22 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption } return } + // Check if the passed assignees is assignable + for _, aID := range assigneeIDs { + assignee, err := models.GetUserByID(aID) + if err != nil { + ctx.Error(500, "GetUserByID", err) + return + } - if assigneeIDs != nil { - for _, aID := range assigneeIDs { - assignee, err := models.GetUserByID(aID) - if err != nil { - ctx.Error(500, "GetUserByID", err) - return - } - - // Check if the passed assignees is assignable - 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 - } + 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 } } From 2d8a320837633919f1aa949057225cff5ec13757 Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Thu, 7 Nov 2019 20:07:52 +0100 Subject: [PATCH 04/15] clarity comparson Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> --- models/issue_assignees.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index f4aa250ddcb51..150740df590dc 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -183,7 +183,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string } //if assignee is empty skip - if len(assignee) == 0 { + if assignee == "" { continue } requestAssignees = append(requestAssignees, assignee) From 84203b05ffba3285ab7aaec866dcd2e696fe29b1 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Thu, 7 Nov 2019 20:13:43 +0100 Subject: [PATCH 05/15] simplify --- models/issue_assignees.go | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 150740df590dc..ec6aea5566eac 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -5,6 +5,7 @@ package models import ( + "code.gitea.io/gitea/modules/util" "fmt" "xorm.io/xorm" @@ -173,24 +174,21 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string var requestAssignees []string - // Prevent double adding assignees and empty assignees - var isDouble bool - for _, assignee := range multipleAssignees { - // Keeping the old assigning method for compatibility reasons - if assignee == oneAssignee { - isDouble = true - continue - } - - //if assignee is empty skip - if assignee == "" { - continue - } - requestAssignees = append(requestAssignees, assignee) + // Keeping the old assigning method for compatibility reasons + if !util.ExistsInSlice(oneAssignee, multipleAssignees) { + requestAssignees = append(requestAssignees, oneAssignee) } - if !isDouble { - requestAssignees = append(requestAssignees, oneAssignee) + //Prevent empty assignerees + if util.ExistsInSlice("", multipleAssignees) { + for _, assignee := range multipleAssignees { + if assignee == "" { + continue + } + requestAssignees = append(requestAssignees, assignee) + } + } else { + requestAssignees = append(requestAssignees, multipleAssignees...) } if len(requestAssignees) > 0 { From e4d738d5cc91872ec8a5572099d539ea977ca89d Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Thu, 7 Nov 2019 21:01:48 +0100 Subject: [PATCH 06/15] Update models/issue_assignees.go Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> --- models/issue_assignees.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index ec6aea5566eac..a99ce726899e3 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -180,7 +180,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string } //Prevent empty assignerees - if util.ExistsInSlice("", multipleAssignees) { + if util.IsStringInSlice("", multipleAssignees) { for _, assignee := range multipleAssignees { if assignee == "" { continue From 001fa9d7fe2df31c1f992fd4b3ad37209e419f37 Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Thu, 7 Nov 2019 21:03:20 +0100 Subject: [PATCH 07/15] Update issue_assignees.go --- models/issue_assignees.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index a99ce726899e3..830e269d4539a 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -175,7 +175,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string var requestAssignees []string // Keeping the old assigning method for compatibility reasons - if !util.ExistsInSlice(oneAssignee, multipleAssignees) { + if !util.IsStringInSlice(oneAssignee, multipleAssignees) { requestAssignees = append(requestAssignees, oneAssignee) } From 3fd71104dd44de54a44f8830a1e8f873238022b3 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 8 Nov 2019 03:53:19 +0100 Subject: [PATCH 08/15] simplify more --- models/issue_assignees.go | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 830e269d4539a..ac1485c3ffed7 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -179,24 +179,13 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string requestAssignees = append(requestAssignees, oneAssignee) } - //Prevent empty assignerees - if util.IsStringInSlice("", multipleAssignees) { - for _, assignee := range multipleAssignees { - if assignee == "" { - continue - } - requestAssignees = append(requestAssignees, assignee) - } - } else { + //Prevent empty assignees + if multipleAssignees[0] != "" { requestAssignees = append(requestAssignees, multipleAssignees...) } - if len(requestAssignees) > 0 { - // Get the IDs of all assignees - assigneeIDs, err = GetUserIDsByNames(requestAssignees, false) - } else { - return nil, nil - } + // Get the IDs of all assignees + assigneeIDs, err = GetUserIDsByNames(requestAssignees, false) return } From 51f0e17aafab4f49e7bbb3cd3891cde2b73dbda6 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 8 Nov 2019 03:57:39 +0100 Subject: [PATCH 09/15] add --if oneAssignee != ""-- again --- models/issue_assignees.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index ac1485c3ffed7..c8becf9606268 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -175,7 +175,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string var requestAssignees []string // Keeping the old assigning method for compatibility reasons - if !util.IsStringInSlice(oneAssignee, multipleAssignees) { + if oneAssignee != "" && !util.IsStringInSlice(oneAssignee, multipleAssignees) { requestAssignees = append(requestAssignees, oneAssignee) } From 4a5f52f9d55b173668296ca8770286668c751ddc Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Fri, 8 Nov 2019 04:41:53 +0100 Subject: [PATCH 10/15] Update models/issue_assignees.go Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> --- models/issue_assignees.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index c8becf9606268..40da639ad884c 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -180,7 +180,7 @@ func MakeIDsFromAPIAssigneesToAdd(oneAssignee string, multipleAssignees []string } //Prevent empty assignees - if multipleAssignees[0] != "" { + if len(multipleAssignees) > 0 && multipleAssignees[0] != "" { requestAssignees = append(requestAssignees, multipleAssignees...) } From 1ded1ae13a200af8159cbd6e93ab04cfe4f41623 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Fri, 8 Nov 2019 05:23:36 +0100 Subject: [PATCH 11/15] CI.restart() From 6a7d15e1bc59397f6f9917cae8e8bca657bf249c Mon Sep 17 00:00:00 2001 From: 6543 <24977596+6543@users.noreply.github.com> Date: Fri, 8 Nov 2019 13:27:24 +0100 Subject: [PATCH 12/15] Update issue_assignees.go --- models/issue_assignees.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/issue_assignees.go b/models/issue_assignees.go index 40da639ad884c..70bed039c2e7d 100644 --- a/models/issue_assignees.go +++ b/models/issue_assignees.go @@ -5,9 +5,10 @@ package models import ( - "code.gitea.io/gitea/modules/util" "fmt" + "code.gitea.io/gitea/modules/util" + "xorm.io/xorm" ) From 763ba495539209d9454076464d985f63bd3c8165 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 9 Nov 2019 20:53:04 +0100 Subject: [PATCH 13/15] add Test for GetUserIDsByNames --- models/user_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/models/user_test.go b/models/user_test.go index bcb955817c330..8be59c5d725d3 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -373,3 +373,16 @@ func TestCreateUser_Issue5882(t *testing.T) { assert.NoError(t, DeleteUser(v.user)) } } + +func TestGetUserIDsByNames(t *testing.T) { + + //ignore non existing + IDs, err := GetUserIDsByNames([]string{"user1", "user2", "do_not_exist"}, true) + assert.NoError(t, err) + assert.Equal(t, []int64{1, 2}, IDs) + + //ignore non existing + IDs, err = GetUserIDsByNames([]string{"user1", "do_not_exist"}, false) + assert.Error(t, err) + assert.Equal(t, []int64{}, IDs) +} From 3ebb8f3e5c7398218ccffd58e5155fd417367951 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 9 Nov 2019 23:02:09 +0100 Subject: [PATCH 14/15] add Test for MakeIDsFromAPIAssigneesToAdd --- models/issue_assignees_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/models/issue_assignees_test.go b/models/issue_assignees_test.go index 163234b167793..f2eb74a2016b1 100644 --- a/models/issue_assignees_test.go +++ b/models/issue_assignees_test.go @@ -59,3 +59,24 @@ func TestUpdateAssignee(t *testing.T) { assert.NoError(t, err) assert.False(t, isAssigned) } + +func TestMakeIDsFromAPIAssigneesToAdd(t *testing.T) { + IDs, err := MakeIDsFromAPIAssigneesToAdd("", []string{""}) + assert.NoError(t, err) + assert.Equal(t, []int64{}, IDs) + + IDs, err = MakeIDsFromAPIAssigneesToAdd("", []string{"non_existing_user"}) + assert.Error(t, err) + + IDs, err = MakeIDsFromAPIAssigneesToAdd("user1", []string{"user1"}) + assert.NoError(t, err) + assert.Equal(t, []int64{1}, IDs) + + IDs, err = MakeIDsFromAPIAssigneesToAdd("user2", []string{""}) + assert.NoError(t, err) + assert.Equal(t, []int64{2}, IDs) + + IDs, err = MakeIDsFromAPIAssigneesToAdd("", []string{"user1", "user2"}) + assert.NoError(t, err) + assert.Equal(t, []int64{1, 2}, IDs) +} From 59f3a9d6e74183b257d05cb63044d27ffd238a8a Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sat, 9 Nov 2019 23:39:53 +0100 Subject: [PATCH 15/15] fix test --- models/issue_assignees_test.go | 2 +- models/user_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/models/issue_assignees_test.go b/models/issue_assignees_test.go index f2eb74a2016b1..79257013f8431 100644 --- a/models/issue_assignees_test.go +++ b/models/issue_assignees_test.go @@ -65,7 +65,7 @@ func TestMakeIDsFromAPIAssigneesToAdd(t *testing.T) { assert.NoError(t, err) assert.Equal(t, []int64{}, IDs) - IDs, err = MakeIDsFromAPIAssigneesToAdd("", []string{"non_existing_user"}) + IDs, err = MakeIDsFromAPIAssigneesToAdd("", []string{"none_existing_user"}) assert.Error(t, err) IDs, err = MakeIDsFromAPIAssigneesToAdd("user1", []string{"user1"}) diff --git a/models/user_test.go b/models/user_test.go index 8be59c5d725d3..f3952422afd33 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -377,12 +377,12 @@ func TestCreateUser_Issue5882(t *testing.T) { func TestGetUserIDsByNames(t *testing.T) { //ignore non existing - IDs, err := GetUserIDsByNames([]string{"user1", "user2", "do_not_exist"}, true) + IDs, err := GetUserIDsByNames([]string{"user1", "user2", "none_existing_user"}, true) assert.NoError(t, err) assert.Equal(t, []int64{1, 2}, IDs) //ignore non existing IDs, err = GetUserIDsByNames([]string{"user1", "do_not_exist"}, false) assert.Error(t, err) - assert.Equal(t, []int64{}, IDs) + assert.Equal(t, []int64(nil), IDs) }