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*

diff --git a/models/organization/org.go b/models/organization/org.go
index b33d15d29c12955132f29053b65fc3fe743d0b9a..28a46ec8f50da231a1809cbd6d545dffa98658e5 100644 (file)
--- 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))
 }
 
@@ -525,9 +532,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 23ef22e2fbaa4a31e9b30287a162db96ff582530..5442c37ccc94cf05027880ff3351fc2a4854b783 100644 (file)
--- 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"
@@ -103,7 +104,7 @@ func TestUser_GetTeams(t *testing.T) {
 func TestUser_GetMembers(t *testing.T) {
        assert.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})
        assert.NoError(t, err)
        if assert.Len(t, members, 3) {
                assert.Equal(t, int64(2), members[0].ID)
@@ -210,37 +211,42 @@ func TestFindOrgs(t *testing.T) {
 func TestGetOrgUsersByOrgID(t *testing.T) {
        assert.NoError(t, unittest.PrepareTestDatabase())
 
-       orgUsers, err := organization.GetOrgUsersByOrgID(db.DefaultContext, &organization.FindOrgMembersOpts{
-               ListOptions: db.ListOptions{},
-               OrgID:       3,
-               PublicOnly:  false,
+       opts := &organization.FindOrgMembersOpts{
+               Doer:  &user_model.User{IsAdmin: true},
+               OrgID: 3,
+       }
+       assert.False(t, opts.PublicOnly())
+       orgUsers, err := organization.GetOrgUsersByOrgID(db.DefaultContext, opts)
+       assert.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)
        assert.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])
-       }
+       assert.Len(t, orgUsers, 2)
 
        orgUsers, err = organization.GetOrgUsersByOrgID(db.DefaultContext, &organization.FindOrgMembersOpts{
                ListOptions: db.ListOptions{},
                OrgID:       unittest.NonexistentID,
-               PublicOnly:  false,
        })
        assert.NoError(t, err)
        assert.Len(t, orgUsers, 0)

diff --git a/models/organization/org_user_test.go b/models/organization/org_user_test.go
index cf7acdf83ba799aecaa7f9512ba2a060fb5ae059..55abb63203e86809464ccfc2392d70e4ea02d26d 100644 (file)
--- a/models/organization/org_user_test.go
+++ b/models/organization/org_user_test.go
@@ -94,7 +94,7 @@ func TestUserListIsPublicMember(t *testing.T) {
 func testUserListIsPublicMember(t *testing.T, orgID int64, expected map[int64]bool) {
        org, err := organization.GetOrgByID(db.DefaultContext, orgID)
        assert.NoError(t, err)
-       _, membersIsPublic, err := org.GetMembers(db.DefaultContext)
+       _, membersIsPublic, err := org.GetMembers(db.DefaultContext, &user_model.User{IsAdmin: true})
        assert.NoError(t, err)
        assert.Equal(t, expected, membersIsPublic)
 }
@@ -121,7 +121,7 @@ func TestUserListIsUserOrgOwner(t *testing.T) {
 func testUserListIsUserOrgOwner(t *testing.T, orgID int64, expected map[int64]bool) {
        org, err := organization.GetOrgByID(db.DefaultContext, orgID)
        assert.NoError(t, err)
-       members, _, err := org.GetMembers(db.DefaultContext)
+       members, _, err := org.GetMembers(db.DefaultContext, &user_model.User{IsAdmin: true})
        assert.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 9db9ad964b6bf7ad81fa55ac90dda554998b3c83..edcee1e207719f7c6c71c1daa018a370bf6f88bb 100644 (file)
--- 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 069bd549c1a2fd5f098f1fa8b6030f179a004730..544f5362b897d81370017f627ddf41be88756532 100644 (file)
--- a/routers/web/org/home.go
+++ b/routers/web/org/home.go
@@ -95,10 +95,12 @@ func home(ctx *context.Context, viewRepositories bool) {
        }
 
        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 8ff75b0651112ca0cd30a7a31a1a6f89bf6abf4f..97dfff3afe64de195357d79cb0bceb736953cf32 100644 (file)
--- a/routers/web/org/members.go
+++ b/routers/web/org/members.go
@@ -34,8 +34,8 @@ func Members(ctx *context.Context) {
        }
 
        opts := &organization.FindOrgMembersOpts{
-               OrgID:      org.ID,
-               PublicOnly: true,
+               Doer:  ctx.Doer,
+               OrgID: org.ID,
        }
 
        if ctx.Doer != nil {
@@ -44,9 +44,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 7eba80ff96b1fdd89f2b8a533499aae5f0cf51ec..e4206293724eb1dc7178401380164ac631f7c3ae 100644 (file)
--- 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 {