From 7751bb64cb20ca65c62466771f393f623eb37a0d Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 11 Nov 2024 01:38:30 +0100 Subject: [PATCH] Calculate `PublicOnly` for org membership only once (#32234) Refactoring of #32211 this move the PublicOnly() filter calcuation next to the DB querys and let it be decided by the Doer --- *Sponsored by Kithara Software GmbH* (cherry picked from commit 43c252dfeaf9ab03c4db3e7ac5169bc0d69901ac) Conflicts: models/organization/org_test.go models/organization/org_user_test.go routers/web/org/home.go rather simple conflict resolution but not trivial tests/integration/user_count_test.go had to be adapted (simple) because it does not exist in Gitea and uses the modified model --- models/organization/org.go | 18 ++++++--- models/organization/org_test.go | 60 +++++++++++++++------------- models/organization/org_user_test.go | 4 +- routers/api/v1/org/member.go | 22 +++++----- routers/web/org/home.go | 8 ++-- routers/web/org/members.go | 8 ++-- services/context/org.go | 7 ++-- tests/integration/user_count_test.go | 5 ++- 8 files changed, 76 insertions(+), 56 deletions(-) diff --git a/models/organization/org.go b/models/organization/org.go index 45f19c7696..e686b6f304 100644 --- a/models/organization/org.go +++ b/models/organization/org.go @@ -141,8 +141,9 @@ func (org *Organization) LoadTeams(ctx context.Context) ([]*Team, error) { } // GetMembers returns all members of organization. -func (org *Organization) GetMembers(ctx context.Context) (user_model.UserList, map[int64]bool, error) { +func (org *Organization) GetMembers(ctx context.Context, doer *user_model.User) (user_model.UserList, map[int64]bool, error) { return FindOrgMembers(ctx, &FindOrgMembersOpts{ + Doer: doer, OrgID: org.ID, }) } @@ -195,16 +196,22 @@ func (org *Organization) CanCreateRepo() bool { // FindOrgMembersOpts represensts find org members conditions type FindOrgMembersOpts struct { db.ListOptions - OrgID int64 - PublicOnly bool + Doer *user_model.User + IsDoerMember bool + OrgID int64 +} + +func (opts FindOrgMembersOpts) PublicOnly() bool { + return opts.Doer == nil || !(opts.IsDoerMember || opts.Doer.IsAdmin) } // CountOrgMembers counts the organization's members func CountOrgMembers(ctx context.Context, opts *FindOrgMembersOpts) (int64, error) { sess := db.GetEngine(ctx).Where("org_id=?", opts.OrgID) - if opts.PublicOnly { + if opts.PublicOnly() { sess.And("is_public = ?", true) } + return sess.Count(new(OrgUser)) } @@ -524,9 +531,10 @@ func GetOrgsCanCreateRepoByUserID(ctx context.Context, userID int64) ([]*Organiz // GetOrgUsersByOrgID returns all organization-user relations by organization ID. func GetOrgUsersByOrgID(ctx context.Context, opts *FindOrgMembersOpts) ([]*OrgUser, error) { sess := db.GetEngine(ctx).Where("org_id=?", opts.OrgID) - if opts.PublicOnly { + if opts.PublicOnly() { sess.And("is_public = ?", true) } + if opts.ListOptions.PageSize > 0 { sess = db.SetSessionPagination(sess, opts) diff --git a/models/organization/org_test.go b/models/organization/org_test.go index 39ccc2d466..a81da28808 100644 --- a/models/organization/org_test.go +++ b/models/organization/org_test.go @@ -4,6 +4,7 @@ package organization_test import ( + "sort" "testing" "code.gitea.io/gitea/models/db" @@ -104,7 +105,7 @@ func TestUser_GetTeams(t *testing.T) { func TestUser_GetMembers(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) - members, _, err := org.GetMembers(db.DefaultContext) + members, _, err := org.GetMembers(db.DefaultContext, &user_model.User{IsAdmin: true}) require.NoError(t, err) if assert.Len(t, members, 3) { assert.Equal(t, int64(2), members[0].ID) @@ -211,37 +212,42 @@ func TestFindOrgs(t *testing.T) { func TestGetOrgUsersByOrgID(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) - orgUsers, err := organization.GetOrgUsersByOrgID(db.DefaultContext, &organization.FindOrgMembersOpts{ - ListOptions: db.ListOptions{}, - OrgID: 3, - PublicOnly: false, - }) - require.NoError(t, err) - if assert.Len(t, orgUsers, 3) { - assert.Equal(t, organization.OrgUser{ - ID: orgUsers[0].ID, - OrgID: 3, - UID: 2, - IsPublic: true, - }, *orgUsers[0]) - assert.Equal(t, organization.OrgUser{ - ID: orgUsers[1].ID, - OrgID: 3, - UID: 4, - IsPublic: false, - }, *orgUsers[1]) - assert.Equal(t, organization.OrgUser{ - ID: orgUsers[2].ID, - OrgID: 3, - UID: 28, - IsPublic: true, - }, *orgUsers[2]) + opts := &organization.FindOrgMembersOpts{ + Doer: &user_model.User{IsAdmin: true}, + OrgID: 3, } + assert.False(t, opts.PublicOnly()) + orgUsers, err := organization.GetOrgUsersByOrgID(db.DefaultContext, opts) + require.NoError(t, err) + sort.Slice(orgUsers, func(i, j int) bool { + return orgUsers[i].ID < orgUsers[j].ID + }) + assert.EqualValues(t, []*organization.OrgUser{{ + ID: 1, + OrgID: 3, + UID: 2, + IsPublic: true, + }, { + ID: 2, + OrgID: 3, + UID: 4, + IsPublic: false, + }, { + ID: 9, + OrgID: 3, + UID: 28, + IsPublic: true, + }}, orgUsers) + + opts = &organization.FindOrgMembersOpts{OrgID: 3} + assert.True(t, opts.PublicOnly()) + orgUsers, err = organization.GetOrgUsersByOrgID(db.DefaultContext, opts) + require.NoError(t, err) + assert.Len(t, orgUsers, 2) orgUsers, err = organization.GetOrgUsersByOrgID(db.DefaultContext, &organization.FindOrgMembersOpts{ ListOptions: db.ListOptions{}, OrgID: unittest.NonexistentID, - PublicOnly: false, }) require.NoError(t, err) assert.Empty(t, orgUsers) diff --git a/models/organization/org_user_test.go b/models/organization/org_user_test.go index 07d07ce3b8..336bf1a5a4 100644 --- a/models/organization/org_user_test.go +++ b/models/organization/org_user_test.go @@ -95,7 +95,7 @@ func TestUserListIsPublicMember(t *testing.T) { func testUserListIsPublicMember(t *testing.T, orgID int64, expected map[int64]bool) { org, err := organization.GetOrgByID(db.DefaultContext, orgID) require.NoError(t, err) - _, membersIsPublic, err := org.GetMembers(db.DefaultContext) + _, membersIsPublic, err := org.GetMembers(db.DefaultContext, &user_model.User{IsAdmin: true}) require.NoError(t, err) assert.Equal(t, expected, membersIsPublic) } @@ -122,7 +122,7 @@ func TestUserListIsUserOrgOwner(t *testing.T) { func testUserListIsUserOrgOwner(t *testing.T, orgID int64, expected map[int64]bool) { org, err := organization.GetOrgByID(db.DefaultContext, orgID) require.NoError(t, err) - members, _, err := org.GetMembers(db.DefaultContext) + members, _, err := org.GetMembers(db.DefaultContext, &user_model.User{IsAdmin: true}) require.NoError(t, err) assert.Equal(t, expected, organization.IsUserOrgOwner(db.DefaultContext, members, orgID)) } diff --git a/routers/api/v1/org/member.go b/routers/api/v1/org/member.go index fb66d4c3f5..0895c53328 100644 --- a/routers/api/v1/org/member.go +++ b/routers/api/v1/org/member.go @@ -18,11 +18,12 @@ import ( ) // listMembers list an organization's members -func listMembers(ctx *context.APIContext, publicOnly bool) { +func listMembers(ctx *context.APIContext, isMember bool) { opts := &organization.FindOrgMembersOpts{ - OrgID: ctx.Org.Organization.ID, - PublicOnly: publicOnly, - ListOptions: utils.GetListOptions(ctx), + Doer: ctx.Doer, + IsDoerMember: isMember, + OrgID: ctx.Org.Organization.ID, + ListOptions: utils.GetListOptions(ctx), } count, err := organization.CountOrgMembers(ctx, opts) @@ -73,16 +74,19 @@ func ListMembers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - publicOnly := true + var ( + isMember bool + err error + ) + if ctx.Doer != nil { - isMember, err := ctx.Org.Organization.IsOrgMember(ctx, ctx.Doer.ID) + isMember, err = ctx.Org.Organization.IsOrgMember(ctx, ctx.Doer.ID) if err != nil { ctx.Error(http.StatusInternalServerError, "IsOrgMember", err) return } - publicOnly = !isMember && !ctx.Doer.IsAdmin } - listMembers(ctx, publicOnly) + listMembers(ctx, isMember) } // ListPublicMembers list an organization's public members @@ -112,7 +116,7 @@ func ListPublicMembers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - listMembers(ctx, true) + listMembers(ctx, false) } // IsMember check if a user is a member of an organization diff --git a/routers/web/org/home.go b/routers/web/org/home.go index 92793d95a4..9ebefa334c 100644 --- a/routers/web/org/home.go +++ b/routers/web/org/home.go @@ -110,10 +110,12 @@ func Home(ctx *context.Context) { } opts := &organization.FindOrgMembersOpts{ - OrgID: org.ID, - PublicOnly: ctx.Org.PublicMemberOnly, - ListOptions: db.ListOptions{Page: 1, PageSize: 25}, + Doer: ctx.Doer, + OrgID: org.ID, + IsDoerMember: ctx.Org.IsMember, + ListOptions: db.ListOptions{Page: 1, PageSize: 25}, } + members, _, err := organization.FindOrgMembers(ctx, opts) if err != nil { ctx.ServerError("FindOrgMembers", err) diff --git a/routers/web/org/members.go b/routers/web/org/members.go index 9a3d60e122..3a5509f911 100644 --- a/routers/web/org/members.go +++ b/routers/web/org/members.go @@ -33,8 +33,8 @@ func Members(ctx *context.Context) { } opts := &organization.FindOrgMembersOpts{ - OrgID: org.ID, - PublicOnly: true, + Doer: ctx.Doer, + OrgID: org.ID, } if ctx.Doer != nil { @@ -43,9 +43,9 @@ func Members(ctx *context.Context) { ctx.Error(http.StatusInternalServerError, "IsOrgMember") return } - opts.PublicOnly = !isMember && !ctx.Doer.IsAdmin + opts.IsDoerMember = isMember } - ctx.Data["PublicOnly"] = opts.PublicOnly + ctx.Data["PublicOnly"] = opts.PublicOnly() total, err := organization.CountOrgMembers(ctx, opts) if err != nil { diff --git a/services/context/org.go b/services/context/org.go index 018b76de43..9673f2f5a9 100644 --- a/services/context/org.go +++ b/services/context/org.go @@ -26,7 +26,6 @@ type Organization struct { Organization *organization.Organization OrgLink string CanCreateOrgRepo bool - PublicMemberOnly bool // Only display public members Team *organization.Team Teams []*organization.Team @@ -176,10 +175,10 @@ func HandleOrgAssignment(ctx *Context, args ...bool) { ctx.Data["OrgLink"] = ctx.Org.OrgLink // Member - ctx.Org.PublicMemberOnly = ctx.Doer == nil || !ctx.Org.IsMember && !ctx.Doer.IsAdmin opts := &organization.FindOrgMembersOpts{ - OrgID: org.ID, - PublicOnly: ctx.Org.PublicMemberOnly, + Doer: ctx.Doer, + OrgID: org.ID, + IsDoerMember: ctx.Org.IsMember, } ctx.Data["NumMembers"], err = organization.CountOrgMembers(ctx, opts) if err != nil { diff --git a/tests/integration/user_count_test.go b/tests/integration/user_count_test.go index db3a9b11a7..8fb78a61ee 100644 --- a/tests/integration/user_count_test.go +++ b/tests/integration/user_count_test.go @@ -75,8 +75,9 @@ func (countTest *userCountTest) Init(t *testing.T, doerID, userID int64) { require.NoError(t, err) countTest.memberCount, err = organization.CountOrgMembers(db.DefaultContext, &organization.FindOrgMembersOpts{ - OrgID: org.ID, - PublicOnly: !isMember, + Doer: countTest.doer, + OrgID: org.ID, + IsDoerMember: isMember, }) require.NoError(t, err)