From d3d6096a3ade335ea2c46bd07c526fab00bc3d05 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sat, 28 Jul 2018 11:32:38 +0100 Subject: [PATCH 1/5] don't fail silently if trying to add a collaborator twice --- options/locale/locale_en-US.ini | 1 + routers/repo/setting.go | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 96d450b5ca8d1..68577a2882f8f 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1014,6 +1014,7 @@ settings.transfer_succeed = The repository has been transferred. settings.confirm_delete = Delete Repository settings.add_collaborator = Add Collaborator settings.add_collaborator_success = The collaborator has been added. +settings.add_collaborator_duplicate = The collaborator has been previously added. settings.delete_collaborator = Remove settings.collaborator_deletion = Remove Collaborator settings.collaborator_deletion_desc = Removing a collaborator will revoke their access to this repository. Continue? diff --git a/routers/repo/setting.go b/routers/repo/setting.go index fa3bd434d5598..835ba0a7512f1 100644 --- a/routers/repo/setting.go +++ b/routers/repo/setting.go @@ -401,6 +401,12 @@ func CollaborationPost(ctx *context.Context) { } } + if got, err := ctx.Repo.Repository.IsCollaborator(u.ID); err == nil && got { + ctx.Flash.Error(ctx.Tr("repo.settings.add_collaborator_duplicate")) + ctx.Redirect(ctx.Repo.RepoLink + "/settings/collaboration") + return + } + if err = ctx.Repo.Repository.AddCollaborator(u); err != nil { ctx.ServerError("AddCollaborator", err) return From b3448f9516464691fe23725e983c098e7623689b Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sun, 29 Jul 2018 10:21:56 +0100 Subject: [PATCH 2/5] fix translation text --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 68577a2882f8f..00c9ff04f7e45 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1014,7 +1014,7 @@ settings.transfer_succeed = The repository has been transferred. settings.confirm_delete = Delete Repository settings.add_collaborator = Add Collaborator settings.add_collaborator_success = The collaborator has been added. -settings.add_collaborator_duplicate = The collaborator has been previously added. +settings.add_collaborator_duplicate =The collaborator is already added to this repository. settings.delete_collaborator = Remove settings.collaborator_deletion = Remove Collaborator settings.collaborator_deletion_desc = Removing a collaborator will revoke their access to this repository. Continue? From 6d36ff70bfa470d80e7b5a3e36a6de20ecd59914 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sun, 29 Jul 2018 18:59:33 +0100 Subject: [PATCH 3/5] added collaborator test --- routers/repo/settings_test.go | 62 +++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/routers/repo/settings_test.go b/routers/repo/settings_test.go index 392c05f773a77..1a0bbccbc7f5e 100644 --- a/routers/repo/settings_test.go +++ b/routers/repo/settings_test.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/auth" + "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" @@ -59,3 +60,64 @@ func TestAddReadWriteOnlyDeployKey(t *testing.T) { Mode: models.AccessModeWrite, }) } + +func TestCollaborationPost(t *testing.T) { + + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "user2/repo1/issues/labels") + test.LoadUser(t, ctx, 2) + test.LoadUser(t, ctx, 4) + test.LoadRepo(t, ctx, 1) + + ctx.Req.Form.Set("collaborator", "user4") + + u := &models.User{ + LowerName: "user2", + Type: models.UserTypeIndividual, + } + + re := &models.Repository{ + ID: 2, + Owner: u, + } + + repo := &context.Repository{ + Owner: u, + Repository: re, + } + + ctx.Repo = repo + + CollaborationPost(ctx) + + assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) + + exists, err := re.IsCollaborator(4) + assert.NoError(t, err) + assert.True(t, exists) +} + +func TestCollaborationPost_InactiveUser(t *testing.T) { + + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "user2/repo1/issues/labels") + test.LoadUser(t, ctx, 2) + test.LoadUser(t, ctx, 9) + test.LoadRepo(t, ctx, 1) + + ctx.Req.Form.Set("collaborator", "user9") + + repo := &context.Repository{ + Owner: &models.User{ + LowerName: "user2", + }, + } + + ctx.Repo = repo + + CollaborationPost(ctx) + + assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) + assert.NotEmpty(t, ctx.Flash.ErrorMsg) + +} From 61cb9b90a74a8de828c1bd4c92c255bf3e6bb41f Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Sun, 29 Jul 2018 23:55:38 +0100 Subject: [PATCH 4/5] improvee testcases --- routers/repo/settings_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/routers/repo/settings_test.go b/routers/repo/settings_test.go index 1a0bbccbc7f5e..8df272898c381 100644 --- a/routers/repo/settings_test.go +++ b/routers/repo/settings_test.go @@ -119,5 +119,27 @@ func TestCollaborationPost_InactiveUser(t *testing.T) { assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) assert.NotEmpty(t, ctx.Flash.ErrorMsg) +} + +func TestCollaborationPost_NonExistentUser(t *testing.T) { + + models.PrepareTestEnv(t) + ctx := test.MockContext(t, "user2/repo1/issues/labels") + test.LoadUser(t, ctx, 2) + test.LoadRepo(t, ctx, 1) + + ctx.Req.Form.Set("collaborator", "user34") + + repo := &context.Repository{ + Owner: &models.User{ + LowerName: "user2", + }, + } + + ctx.Repo = repo + + CollaborationPost(ctx) + assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) + assert.NotEmpty(t, ctx.Flash.ErrorMsg) } From 618dabdd39ba5a3d306e56e2ae19f119af5dbc01 Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Mon, 6 Aug 2018 10:32:01 +0100 Subject: [PATCH 5/5] Added tests to make sure a collaborator cannot be added twice --- routers/repo/settings_test.go | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/routers/repo/settings_test.go b/routers/repo/settings_test.go index 8df272898c381..a8cc645075715 100644 --- a/routers/repo/settings_test.go +++ b/routers/repo/settings_test.go @@ -97,26 +97,44 @@ func TestCollaborationPost(t *testing.T) { assert.True(t, exists) } -func TestCollaborationPost_InactiveUser(t *testing.T) { +func TestCollaborationPost_AddCollaboratorTwice(t *testing.T) { models.PrepareTestEnv(t) ctx := test.MockContext(t, "user2/repo1/issues/labels") test.LoadUser(t, ctx, 2) - test.LoadUser(t, ctx, 9) + test.LoadUser(t, ctx, 4) test.LoadRepo(t, ctx, 1) - ctx.Req.Form.Set("collaborator", "user9") + ctx.Req.Form.Set("collaborator", "user4") + + u := &models.User{ + LowerName: "user2", + Type: models.UserTypeIndividual, + } + + re := &models.Repository{ + ID: 2, + Owner: u, + } repo := &context.Repository{ - Owner: &models.User{ - LowerName: "user2", - }, + Owner: u, + Repository: re, } ctx.Repo = repo CollaborationPost(ctx) + assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) + + exists, err := re.IsCollaborator(4) + assert.NoError(t, err) + assert.True(t, exists) + + // Try adding the same collaborator again + CollaborationPost(ctx) + assert.EqualValues(t, http.StatusFound, ctx.Resp.Status()) assert.NotEmpty(t, ctx.Flash.ErrorMsg) }