Fix issue overview for teams (#19652)

- Don't use hacky solution to limit to the correct RepoID's, instead use
current code to handle these limits. The existing code is more correct
than the hacky solution.
- Resolves #19636
- Add test-case
This commit is contained in:
Gusted 2022-05-16 09:49:17 +00:00 committed by GitHub
parent d494cc3356
commit 71ca131582
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 87 additions and 36 deletions

View file

@ -209,7 +209,7 @@ func TestAPISearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = MakeRequest(t, req, http.StatusOK) resp = MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.EqualValues(t, "15", resp.Header().Get("X-Total-Count")) assert.EqualValues(t, "17", resp.Header().Get("X-Total-Count"))
assert.Len(t, apiIssues, 10) // there are more but 10 is page item limit assert.Len(t, apiIssues, 10) // there are more but 10 is page item limit
query.Add("limit", "20") query.Add("limit", "20")
@ -217,14 +217,14 @@ func TestAPISearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = MakeRequest(t, req, http.StatusOK) resp = MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 15) assert.Len(t, apiIssues, 17)
query = url.Values{"assigned": {"true"}, "state": {"all"}, "token": {token}} query = url.Values{"assigned": {"true"}, "state": {"all"}, "token": {token}}
link.RawQuery = query.Encode() link.RawQuery = query.Encode()
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = MakeRequest(t, req, http.StatusOK) resp = MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 1) assert.Len(t, apiIssues, 2)
query = url.Values{"milestones": {"milestone1"}, "state": {"all"}, "token": {token}} query = url.Values{"milestones": {"milestone1"}, "state": {"all"}, "token": {token}}
link.RawQuery = query.Encode() link.RawQuery = query.Encode()
@ -252,7 +252,7 @@ func TestAPISearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = MakeRequest(t, req, http.StatusOK) resp = MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 3) assert.Len(t, apiIssues, 5)
query = url.Values{"owner": {"user3"}, "team": {"team1"}, "token": {token}} // organization + team query = url.Values{"owner": {"user3"}, "team": {"team1"}, "token": {token}} // organization + team
link.RawQuery = query.Encode() link.RawQuery = query.Encode()

View file

@ -29,7 +29,7 @@ func TestNodeinfo(t *testing.T) {
assert.True(t, nodeinfo.OpenRegistrations) assert.True(t, nodeinfo.OpenRegistrations)
assert.Equal(t, "gitea", nodeinfo.Software.Name) assert.Equal(t, "gitea", nodeinfo.Software.Name)
assert.Equal(t, 23, nodeinfo.Usage.Users.Total) assert.Equal(t, 23, nodeinfo.Usage.Users.Total)
assert.Equal(t, 15, nodeinfo.Usage.LocalPosts) assert.Equal(t, 17, nodeinfo.Usage.LocalPosts)
assert.Equal(t, 2, nodeinfo.Usage.LocalComments) assert.Equal(t, 2, nodeinfo.Usage.LocalComments)
}) })
} }

View file

@ -392,7 +392,7 @@ func TestSearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = session.MakeRequest(t, req, http.StatusOK) resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.EqualValues(t, "15", resp.Header().Get("X-Total-Count")) assert.EqualValues(t, "17", resp.Header().Get("X-Total-Count"))
assert.Len(t, apiIssues, 10) // there are more but 10 is page item limit assert.Len(t, apiIssues, 10) // there are more but 10 is page item limit
query.Add("limit", "20") query.Add("limit", "20")
@ -400,14 +400,14 @@ func TestSearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = session.MakeRequest(t, req, http.StatusOK) resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 15) assert.Len(t, apiIssues, 17)
query = url.Values{"assigned": {"true"}, "state": {"all"}} query = url.Values{"assigned": {"true"}, "state": {"all"}}
link.RawQuery = query.Encode() link.RawQuery = query.Encode()
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = session.MakeRequest(t, req, http.StatusOK) resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 1) assert.Len(t, apiIssues, 2)
query = url.Values{"milestones": {"milestone1"}, "state": {"all"}} query = url.Values{"milestones": {"milestone1"}, "state": {"all"}}
link.RawQuery = query.Encode() link.RawQuery = query.Encode()
@ -435,7 +435,7 @@ func TestSearchIssues(t *testing.T) {
req = NewRequest(t, "GET", link.String()) req = NewRequest(t, "GET", link.String())
resp = session.MakeRequest(t, req, http.StatusOK) resp = session.MakeRequest(t, req, http.StatusOK)
DecodeJSON(t, resp, &apiIssues) DecodeJSON(t, resp, &apiIssues)
assert.Len(t, apiIssues, 3) assert.Len(t, apiIssues, 5)
query = url.Values{"owner": {"user3"}, "team": {"team1"}} // organization + team query = url.Values{"owner": {"user3"}, "team": {"team1"}} // organization + team
link.RawQuery = query.Encode() link.RawQuery = query.Encode()

View file

@ -184,3 +184,27 @@
is_pull: false is_pull: false
created_unix: 1602935696 created_unix: 1602935696
updated_unix: 1602935696 updated_unix: 1602935696
-
id: 16
repo_id: 32
index: 1
poster_id: 2
name: just a normal issue
content: content
is_closed: false
is_pull: false
created_unix: 1602935696
updated_unix: 1602935696
-
id: 17
repo_id: 32
index: 2
poster_id: 15
name: a issue with a assignment
content: content
is_closed: false
is_pull: false
created_unix: 1602935696
updated_unix: 1602935696

View file

@ -10,3 +10,7 @@
id: 3 id: 3
assignee_id: 2 assignee_id: 2
issue_id: 6 issue_id: 6
-
id: 4
assignee_id: 2
issue_id: 17

View file

@ -10,6 +10,9 @@
- -
group_id: 10 group_id: 10
max_index: 1 max_index: 1
-
group_id: 32
max_index: 2
- -
group_id: 48 group_id: 48
max_index: 1 max_index: 1

View file

@ -483,7 +483,7 @@
is_private: false is_private: false
num_stars: 0 num_stars: 0
num_forks: 0 num_forks: 0
num_issues: 0 num_issues: 2
is_mirror: false is_mirror: false
status: 0 status: 0

View file

@ -252,6 +252,7 @@
- -
id: 43 id: 43
org_id: 3
team_id: 7 team_id: 7
type: 2 # issues type: 2 # issues
access_mode: 2 access_mode: 2

View file

@ -1349,9 +1349,7 @@ func (opts *IssuesOptions) setupSessionNoLimit(sess *xorm.Session) {
} }
if opts.User != nil { if opts.User != nil {
sess.And( sess.And(issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.IsTrue()))
issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.IsTrue()),
)
} }
} }
@ -1463,6 +1461,7 @@ func Issues(opts *IssuesOptions) ([]*Issue, error) {
sess := e.Join("INNER", "repository", "`issue`.repo_id = `repository`.id") sess := e.Join("INNER", "repository", "`issue`.repo_id = `repository`.id")
opts.setupSessionWithLimit(sess) opts.setupSessionWithLimit(sess)
sortIssuesSession(sess, opts.SortType, opts.PriorityRepoID) sortIssuesSession(sess, opts.SortType, opts.PriorityRepoID)
issues := make([]*Issue, 0, opts.ListOptions.PageSize) issues := make([]*Issue, 0, opts.ListOptions.PageSize)
@ -1484,6 +1483,7 @@ func CountIssues(opts *IssuesOptions) (int64, error) {
sess := e.Select("COUNT(issue.id) AS count").Table("issue") sess := e.Select("COUNT(issue.id) AS count").Table("issue")
sess.Join("INNER", "repository", "`issue`.repo_id = `repository`.id") sess.Join("INNER", "repository", "`issue`.repo_id = `repository`.id")
opts.setupSessionNoLimit(sess) opts.setupSessionNoLimit(sess)
return sess.Count() return sess.Count()
} }

View file

@ -16,6 +16,7 @@ import (
"code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/foreignreference" "code.gitea.io/gitea/models/foreignreference"
issues_model "code.gitea.io/gitea/models/issues" issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/organization"
repo_model "code.gitea.io/gitea/models/repo" repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
@ -287,6 +288,20 @@ func TestGetUserIssueStats(t *testing.T) {
ClosedCount: 0, ClosedCount: 0,
}, },
}, },
{
UserIssueStatsOptions{
UserID: 2,
Org: unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}).(*organization.Organization),
Team: unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 7}).(*organization.Team),
FilterMode: FilterModeAll,
},
IssueStats{
YourRepositoriesCount: 2,
AssignCount: 1,
CreateCount: 1,
OpenCount: 2,
},
},
} { } {
t.Run(fmt.Sprintf("%#v", test.Opts), func(t *testing.T) { t.Run(fmt.Sprintf("%#v", test.Opts), func(t *testing.T) {
stats, err := GetUserIssueStats(test.Opts) stats, err := GetUserIssueStats(test.Opts)
@ -341,7 +356,7 @@ func TestGetRepoIDsForIssuesOptions(t *testing.T) {
IssuesOptions{ IssuesOptions{
AssigneeID: 2, AssigneeID: 2,
}, },
[]int64{3}, []int64{3, 32},
}, },
{ {
IssuesOptions{ IssuesOptions{
@ -595,5 +610,5 @@ func TestCountIssues(t *testing.T) {
assert.NoError(t, unittest.PrepareTestDatabase()) assert.NoError(t, unittest.PrepareTestDatabase())
count, err := CountIssues(&IssuesOptions{}) count, err := CountIssues(&IssuesOptions{})
assert.NoError(t, err) assert.NoError(t, err)
assert.EqualValues(t, 15, count) assert.EqualValues(t, 17, count)
} }

View file

@ -9,6 +9,7 @@ import (
"strings" "strings"
"code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization"
"code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/models/perm"
repo_model "code.gitea.io/gitea/models/repo" repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unit"
@ -246,12 +247,27 @@ func teamUnitsRepoCond(id string, userID, orgID, teamID int64, units ...unit.Typ
builder.Eq{ builder.Eq{
"team_id": teamID, "team_id": teamID,
}.And( }.And(
builder.Or(
// Check if the user is member of the team.
builder.In( builder.In(
"team_id", builder.Select("team_id").From("team_user").Where( "team_id", builder.Select("team_id").From("team_user").Where(
builder.Eq{ builder.Eq{
"uid": userID, "uid": userID,
}, },
), ),
),
// Check if the user is in the owner team of the organisation.
builder.Exists(builder.Select("team_id").From("team_user").
Where(builder.Eq{
"org_id": orgID,
"team_id": builder.Select("id").From("team").Where(
builder.Eq{
"org_id": orgID,
"lower_name": strings.ToLower(organization.OwnerTeamName),
}),
"uid": userID,
}),
),
)).And( )).And(
builder.In( builder.In(
"team_id", builder.Select("team_id").From("team_unit").Where( "team_id", builder.Select("team_id").From("team_unit").Where(

View file

@ -443,12 +443,13 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
AllLimited: false, AllLimited: false,
} }
if ctxUser.IsOrganization() && ctx.Org.Team != nil { if team != nil {
repoOpts.TeamID = ctx.Org.Team.ID repoOpts.TeamID = team.ID
} }
switch filterMode { switch filterMode {
case models.FilterModeAll: case models.FilterModeAll:
case models.FilterModeYourRepositories:
case models.FilterModeAssign: case models.FilterModeAssign:
opts.AssigneeID = ctx.Doer.ID opts.AssigneeID = ctx.Doer.ID
case models.FilterModeCreate: case models.FilterModeCreate:
@ -457,13 +458,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
opts.MentionedID = ctx.Doer.ID opts.MentionedID = ctx.Doer.ID
case models.FilterModeReviewRequested: case models.FilterModeReviewRequested:
opts.ReviewRequestedID = ctx.Doer.ID opts.ReviewRequestedID = ctx.Doer.ID
case models.FilterModeYourRepositories:
if ctxUser.IsOrganization() && ctx.Org.Team != nil {
// Fixes a issue whereby the user's ID would be used
// to check if it's in the team(which possible isn't the case).
opts.User = nil
}
opts.RepoCond = models.SearchRepositoryCondition(repoOpts)
} }
// keyword holds the search term entered into the search field. // keyword holds the search term entered into the search field.
@ -595,13 +589,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) {
Org: org, Org: org,
Team: team, Team: team,
} }
if filterMode == models.FilterModeYourRepositories {
statsOpts.RepoCond = models.SearchRepositoryCondition(repoOpts)
}
// Detect when we only should search by team.
if opts.User == nil {
statsOpts.UserID = 0
}
issueStats, err = models.GetUserIssueStats(statsOpts) issueStats, err = models.GetUserIssueStats(statsOpts)
if err != nil { if err != nil {
ctx.ServerError("GetUserIssueStats Shown", err) ctx.ServerError("GetUserIssueStats Shown", err)