From 1446b6181f807393ca692fd75cb38d3686df3a0f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 11 Oct 2023 18:26:06 +0800 Subject: [PATCH 1/2] Refactor the function RemoveOrgUser --- models/org.go | 22 ++++++++-------------- models/org_team.go | 2 +- models/org_test.go | 9 +++++---- routers/api/v1/org/member.go | 2 +- routers/web/org/members.go | 4 ++-- services/user/user.go | 2 +- services/user/user_test.go | 2 +- tests/integration/auth_ldap_test.go | 2 +- 8 files changed, 20 insertions(+), 25 deletions(-) diff --git a/models/org.go b/models/org.go index 5f0e678ab45b6..f6a1a89692213 100644 --- a/models/org.go +++ b/models/org.go @@ -14,7 +14,14 @@ import ( repo_model "code.gitea.io/gitea/models/repo" ) -func removeOrgUser(ctx context.Context, orgID, userID int64) error { +// RemoveOrgUser removes user from given organization. +func RemoveOrgUser(ctx context.Context, orgID, userID int64) error { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + ou := new(organization.OrgUser) sess := db.GetEngine(ctx) @@ -93,18 +100,5 @@ func removeOrgUser(ctx context.Context, orgID, userID int64) error { } } - return nil -} - -// RemoveOrgUser removes user from given organization. -func RemoveOrgUser(orgID, userID int64) error { - ctx, committer, err := db.TxContext(db.DefaultContext) - if err != nil { - return err - } - defer committer.Close() - if err := removeOrgUser(ctx, orgID, userID); err != nil { - return err - } return committer.Commit() } diff --git a/models/org_team.go b/models/org_team.go index acbab560892df..03a4f6e98d8fa 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -502,7 +502,7 @@ func removeInvalidOrgUser(ctx context.Context, userID, orgID int64) error { }); err != nil { return err } else if count == 0 { - return removeOrgUser(ctx, orgID, userID) + return RemoveOrgUser(ctx, orgID, userID) } return nil } diff --git a/models/org_test.go b/models/org_test.go index 54e8f08465979..d10a1dc218daf 100644 --- a/models/org_test.go +++ b/models/org_test.go @@ -6,6 +6,7 @@ package models import ( "testing" + "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -20,7 +21,7 @@ func TestUser_RemoveMember(t *testing.T) { // remove a user that is a member unittest.AssertExistsAndLoadBean(t, &organization.OrgUser{UID: 4, OrgID: 3}) prevNumMembers := org.NumMembers - assert.NoError(t, RemoveOrgUser(org.ID, 4)) + assert.NoError(t, RemoveOrgUser(db.DefaultContext, org.ID, 4)) unittest.AssertNotExistsBean(t, &organization.OrgUser{UID: 4, OrgID: 3}) org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) assert.Equal(t, prevNumMembers-1, org.NumMembers) @@ -28,7 +29,7 @@ func TestUser_RemoveMember(t *testing.T) { // remove a user that is not a member unittest.AssertNotExistsBean(t, &organization.OrgUser{UID: 5, OrgID: 3}) prevNumMembers = org.NumMembers - assert.NoError(t, RemoveOrgUser(org.ID, 5)) + assert.NoError(t, RemoveOrgUser(db.DefaultContext, org.ID, 5)) unittest.AssertNotExistsBean(t, &organization.OrgUser{UID: 5, OrgID: 3}) org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) assert.Equal(t, prevNumMembers, org.NumMembers) @@ -44,7 +45,7 @@ func TestRemoveOrgUser(t *testing.T) { if unittest.BeanExists(t, &organization.OrgUser{OrgID: orgID, UID: userID}) { expectedNumMembers-- } - assert.NoError(t, RemoveOrgUser(orgID, userID)) + assert.NoError(t, RemoveOrgUser(db.DefaultContext, orgID, userID)) unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: orgID, UID: userID}) org = unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: orgID}) assert.EqualValues(t, expectedNumMembers, org.NumMembers) @@ -52,7 +53,7 @@ func TestRemoveOrgUser(t *testing.T) { testSuccess(3, 4) testSuccess(3, 4) - err := RemoveOrgUser(7, 5) + err := RemoveOrgUser(db.DefaultContext, 7, 5) assert.Error(t, err) assert.True(t, organization.IsErrLastOrgOwner(err)) unittest.AssertExistsAndLoadBean(t, &organization.OrgUser{OrgID: 7, UID: 5}) diff --git a/routers/api/v1/org/member.go b/routers/api/v1/org/member.go index 139e01e1f9899..422b7cecfee14 100644 --- a/routers/api/v1/org/member.go +++ b/routers/api/v1/org/member.go @@ -318,7 +318,7 @@ func DeleteMember(ctx *context.APIContext) { if ctx.Written() { return } - if err := models.RemoveOrgUser(ctx.Org.Organization.ID, member.ID); err != nil { + if err := models.RemoveOrgUser(ctx, ctx.Org.Organization.ID, member.ID); err != nil { ctx.Error(http.StatusInternalServerError, "RemoveOrgUser", err) } ctx.Status(http.StatusNoContent) diff --git a/routers/web/org/members.go b/routers/web/org/members.go index 247025a7cb7f1..15a615c706fe6 100644 --- a/routers/web/org/members.go +++ b/routers/web/org/members.go @@ -104,14 +104,14 @@ func MembersAction(ctx *context.Context) { ctx.Error(http.StatusNotFound) return } - err = models.RemoveOrgUser(org.ID, uid) + err = models.RemoveOrgUser(ctx, org.ID, uid) if organization.IsErrLastOrgOwner(err) { ctx.Flash.Error(ctx.Tr("form.last_org_owner")) ctx.JSONRedirect(ctx.Org.OrgLink + "/members") return } case "leave": - err = models.RemoveOrgUser(org.ID, ctx.Doer.ID) + err = models.RemoveOrgUser(ctx, org.ID, ctx.Doer.ID) if err == nil { ctx.Flash.Success(ctx.Tr("form.organization_leave_success", org.DisplayName())) ctx.JSON(http.StatusOK, map[string]any{ diff --git a/services/user/user.go b/services/user/user.go index b95a7e0639d07..b958c3f47d4e9 100644 --- a/services/user/user.go +++ b/services/user/user.go @@ -204,7 +204,7 @@ func DeleteUser(ctx context.Context, u *user_model.User, purge bool) error { break } for _, org := range orgs { - if err := models.RemoveOrgUser(org.ID, u.ID); err != nil { + if err := models.RemoveOrgUser(ctx, org.ID, u.ID); err != nil { if organization.IsErrLastOrgOwner(err) { err = organization.DeleteOrganization(ctx, org) } diff --git a/services/user/user_test.go b/services/user/user_test.go index 550dafd9c56c0..73f1491c1224c 100644 --- a/services/user/user_test.go +++ b/services/user/user_test.go @@ -41,7 +41,7 @@ func TestDeleteUser(t *testing.T) { orgUsers := make([]*organization.OrgUser, 0, 10) assert.NoError(t, db.GetEngine(db.DefaultContext).Find(&orgUsers, &organization.OrgUser{UID: userID})) for _, orgUser := range orgUsers { - if err := models.RemoveOrgUser(orgUser.OrgID, orgUser.UID); err != nil { + if err := models.RemoveOrgUser(db.DefaultContext, orgUser.OrgID, orgUser.UID); err != nil { assert.True(t, organization.IsErrLastOrgOwner(err)) return } diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index dbd3bc93465f4..9bb9e7b3c7ebf 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -430,7 +430,7 @@ func TestLDAPGroupTeamSyncAddMember(t *testing.T) { assert.True(t, isMember, "Membership should be added to the right team") err = models.RemoveTeamMember(db.DefaultContext, team, user.ID) assert.NoError(t, err) - err = models.RemoveOrgUser(usersOrgs[0].ID, user.ID) + err = models.RemoveOrgUser(db.DefaultContext, usersOrgs[0].ID, user.ID) assert.NoError(t, err) } else { // assert members of LDAP group "cn=admin_staff" keep initial team membership since mapped team does not exist From 3ed88acd849fed59be6c5c50de881ff8945ff52d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 Nov 2023 21:21:47 +0800 Subject: [PATCH 2/2] Fix bug --- models/org.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/models/org.go b/models/org.go index f6a1a89692213..5e0deeb8de7a2 100644 --- a/models/org.go +++ b/models/org.go @@ -16,17 +16,9 @@ import ( // RemoveOrgUser removes user from given organization. func RemoveOrgUser(ctx context.Context, orgID, userID int64) error { - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return err - } - defer committer.Close() - ou := new(organization.OrgUser) - sess := db.GetEngine(ctx) - - has, err := sess. + has, err := db.GetEngine(ctx). Where("uid=?", userID). And("org_id=?", orgID). Get(ou) @@ -59,7 +51,13 @@ func RemoveOrgUser(ctx context.Context, orgID, userID int64) error { } } - if _, err := sess.ID(ou.ID).Delete(ou); err != nil { + ctx, committer, err := db.TxContext(ctx) + if err != nil { + return err + } + defer committer.Close() + + if _, err := db.GetEngine(ctx).ID(ou.ID).Delete(ou); err != nil { return err } else if _, err = db.Exec(ctx, "UPDATE `user` SET num_members=num_members-1 WHERE id=?", orgID); err != nil { return err @@ -81,7 +79,7 @@ func RemoveOrgUser(ctx context.Context, orgID, userID int64) error { } if len(repoIDs) > 0 { - if _, err = sess. + if _, err = db.GetEngine(ctx). Where("user_id = ?", userID). In("repo_id", repoIDs). Delete(new(access_model.Access)); err != nil {