From d2acd4f14cd0023cde797365d000133d912ae8fc Mon Sep 17 00:00:00 2001 From: limiting-factor Date: Wed, 11 Mar 2026 03:40:32 +0100 Subject: [PATCH] chore: split `AddRepository` and `AddTeamMember` to return the inserted value (#11342) Split [AddRepository][0] and [AddTeamMember][1] to functions that return the inserted value. These can be used by the F3 driver to obtain the ID of the TeamRepo and TeamUser rows inserted in the database. Add test coverage for both functions and remove a duplicated test. Integration tests also already [make use of AddRepository and AddTeamMember][2] indirectly. [0]: https://codeberg.org/forgejo/forgejo/src/commit/f8bee35e7717e33a8dc122b038dd6335a198449c/models/org_team.go#L26 [1]: https://codeberg.org/forgejo/forgejo/src/commit/f8bee35e7717e33a8dc122b038dd6335a198449c/models/org_team.go#L359 [2]: https://codeberg.org/forgejo/forgejo/src/commit/f8bee35e7717e33a8dc122b038dd6335a198449c/tests/integration/api_helper_for_declarative_test.go#L461-L483 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11342 Reviewed-by: Gusted Co-authored-by: limiting-factor Co-committed-by: limiting-factor --- models/org_team.go | 53 ++++++++++------ models/org_team_test.go | 104 ++++++++++++++++++++++++++----- models/organization/team_repo.go | 10 --- 3 files changed, 125 insertions(+), 42 deletions(-) diff --git a/models/org_team.go b/models/org_team.go index de54c61745..187ea1560d 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -23,34 +23,44 @@ import ( "xorm.io/builder" ) -func AddRepository(ctx context.Context, t *organization.Team, repo *repo_model.Repository) (err error) { - if err = organization.AddTeamRepo(ctx, t.OrgID, t.ID, repo.ID); err != nil { - return err +func AddRepository(ctx context.Context, t *organization.Team, repo *repo_model.Repository) error { + _, err := InsertTeamRepository(ctx, t, repo) + return err +} + +func InsertTeamRepository(ctx context.Context, t *organization.Team, repo *repo_model.Repository) (teamRepo *organization.TeamRepo, err error) { + teamRepo = &organization.TeamRepo{ + OrgID: t.OrgID, + TeamID: t.ID, + RepoID: repo.ID, + } + if _, err = db.GetEngine(ctx).Insert(teamRepo); err != nil { + return nil, err } if err = organization.IncrTeamRepoNum(ctx, t.ID); err != nil { - return fmt.Errorf("update team: %w", err) + return nil, fmt.Errorf("update team: %w", err) } t.NumRepos++ if err = access_model.RecalculateTeamAccesses(ctx, repo, 0); err != nil { - return fmt.Errorf("recalculateAccesses: %w", err) + return nil, fmt.Errorf("recalculateAccesses: %w", err) } // Make all team members watch this repo if enabled in global settings if setting.Service.AutoWatchNewRepos { if err = t.LoadMembers(ctx); err != nil { - return fmt.Errorf("getMembers: %w", err) + return nil, fmt.Errorf("getMembers: %w", err) } for _, u := range t.Members { if err = repo_model.WatchRepo(ctx, u.ID, repo.ID, true); err != nil { - return fmt.Errorf("watchRepo: %w", err) + return nil, fmt.Errorf("watchRepo: %w", err) } } } - return nil + return teamRepo, nil } // addAllRepositories adds all repositories to the team. @@ -354,16 +364,27 @@ func DeleteTeam(ctx context.Context, t *organization.Team) error { return committer.Commit() } +func AddTeamMember(ctx context.Context, team *organization.Team, userID int64) error { + _, err := InsertTeamMember(ctx, team, userID) + return err +} + // AddTeamMember adds new membership of given team to given organization, // the user will have membership to given organization automatically when needed. -func AddTeamMember(ctx context.Context, team *organization.Team, userID int64) error { +func InsertTeamMember(ctx context.Context, team *organization.Team, userID int64) (*organization.TeamUser, error) { isAlreadyMember, err := organization.IsTeamMember(ctx, team.OrgID, team.ID, userID) if err != nil || isAlreadyMember { - return err + return nil, err } if err := organization.AddOrgUser(ctx, team.OrgID, userID); err != nil { - return err + return nil, err + } + + teamUser := &organization.TeamUser{ + UID: userID, + OrgID: team.OrgID, + TeamID: team.ID, } err = db.WithTx(ctx, func(ctx context.Context) error { @@ -375,11 +396,7 @@ func AddTeamMember(ctx context.Context, team *organization.Team, userID int64) e sess := db.GetEngine(ctx) - if err := db.Insert(ctx, &organization.TeamUser{ - UID: userID, - OrgID: team.OrgID, - TeamID: team.ID, - }); err != nil { + if err := db.Insert(ctx, teamUser); err != nil { return err } else if _, err := sess.Incr("num_members").ID(team.ID).Update(new(organization.Team)); err != nil { return err @@ -420,7 +437,7 @@ func AddTeamMember(ctx context.Context, team *organization.Team, userID int64) e return nil }) if err != nil { - return err + return nil, err } // this behaviour may spend much time so run it in a goroutine @@ -440,7 +457,7 @@ func AddTeamMember(ctx context.Context, team *organization.Team, userID int64) e }(team.Repos) } - return nil + return teamUser, nil } func removeTeamMember(ctx context.Context, team *organization.Team, userID int64) error { diff --git a/models/org_team_test.go b/models/org_team_test.go index 730bc65f7d..5f7544e9ea 100644 --- a/models/org_team_test.go +++ b/models/org_team_test.go @@ -19,20 +19,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestTeam_AddMember(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - test := func(teamID, userID int64) { - team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: teamID}) - require.NoError(t, AddTeamMember(db.DefaultContext, team, userID)) - unittest.AssertExistsAndLoadBean(t, &organization.TeamUser{UID: userID, TeamID: teamID}) - unittest.CheckConsistencyFor(t, &organization.Team{ID: teamID}, &user_model.User{ID: team.OrgID}) - } - test(1, 2) - test(1, 4) - test(3, 2) -} - func TestTeam_RemoveMember(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) @@ -132,6 +118,96 @@ func TestAddTeamMember(t *testing.T) { test(3, 2) } +func TestTeam_AddAndReturnTeamMember(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + for _, testCase := range []struct { + name string + alreadyMember bool + teamID int64 + userID int64 + }{ + { + name: "Already member of a team with repositories", + alreadyMember: true, + teamID: 1, + userID: 2, + }, + { + name: "New member of a team with repositories", + teamID: 1, + userID: 4, + }, + { + name: "New member of a team with no repositories", + teamID: 3, + userID: 2, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: testCase.teamID}) + teamUser, err := InsertTeamMember(db.DefaultContext, team, testCase.userID) + require.NoError(t, err) + if testCase.alreadyMember { + assert.Nil(t, teamUser) + } else { + require.NotNil(t, teamUser) + assert.Equal(t, testCase.teamID, teamUser.TeamID) + assert.Equal(t, testCase.userID, teamUser.UID) + } + unittest.AssertExistsAndLoadBean(t, &organization.TeamUser{UID: testCase.userID, TeamID: testCase.teamID}) + unittest.CheckConsistencyFor(t, &organization.Team{ID: testCase.teamID}, &user_model.User{ID: team.OrgID}) + }) + } +} + +func TestTeam_AddTeamRepository(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + for _, testCase := range []struct { + name string + teamID int64 + repoID int64 + }{ + { + name: "AddAndReturnTeamRepository", + teamID: 8, + repoID: 23, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: testCase.teamID}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: testCase.repoID}) + teamRepo, err := InsertTeamRepository(t.Context(), team, repo) + require.NoError(t, err) + require.NotNil(t, teamRepo) + assert.Equal(t, testCase.teamID, teamRepo.TeamID) + assert.Equal(t, testCase.repoID, teamRepo.RepoID) + unittest.AssertExistsAndLoadBean(t, &organization.TeamRepo{RepoID: testCase.repoID, TeamID: testCase.teamID}) + }) + } + + for _, testCase := range []struct { + name string + teamID int64 + repoID int64 + }{ + { + name: "AddTeamRepository", + teamID: 9, + repoID: 23, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: testCase.teamID}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: testCase.repoID}) + err := AddRepository(t.Context(), team, repo) + require.NoError(t, err) + unittest.AssertExistsAndLoadBean(t, &organization.TeamRepo{RepoID: testCase.repoID, TeamID: testCase.teamID}) + }) + } +} + func TestRemoveTeamMember(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go index 9331537e19..895d5a56d0 100644 --- a/models/organization/team_repo.go +++ b/models/organization/team_repo.go @@ -59,16 +59,6 @@ func GetTeamRepositories(ctx context.Context, opts *SearchTeamRepoOptions) (repo Find(&repos) } -// AddTeamRepo adds a repo for an organization's team -func AddTeamRepo(ctx context.Context, orgID, teamID, repoID int64) error { - _, err := db.GetEngine(ctx).Insert(&TeamRepo{ - OrgID: orgID, - TeamID: teamID, - RepoID: repoID, - }) - return err -} - // RemoveTeamRepo remove repository from team func RemoveTeamRepo(ctx context.Context, teamID, repoID int64) error { _, err := db.DeleteByBean(ctx, &TeamRepo{