From 71ca131582fff51da614291732ed43b2bf48d8a1 Mon Sep 17 00:00:00 2001 From: Gusted Date: Mon, 16 May 2022 09:49:17 +0000 Subject: [PATCH] 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 --- integrations/api_issue_test.go | 8 ++++---- integrations/api_nodeinfo_test.go | 2 +- integrations/issue_test.go | 8 ++++---- models/fixtures/issue.yml | 24 ++++++++++++++++++++++++ models/fixtures/issue_assignees.yml | 4 ++++ models/fixtures/issue_index.yml | 5 ++++- models/fixtures/repository.yml | 2 +- models/fixtures/team_unit.yml | 1 + models/issue.go | 6 +++--- models/issue_test.go | 19 +++++++++++++++++-- models/repo_list.go | 24 ++++++++++++++++++++---- routers/web/user/home.go | 20 ++++---------------- 12 files changed, 87 insertions(+), 36 deletions(-) diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go index 5ed5a0ad99..cc7d8d6bd5 100644 --- a/integrations/api_issue_test.go +++ b/integrations/api_issue_test.go @@ -209,7 +209,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = MakeRequest(t, req, http.StatusOK) 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 query.Add("limit", "20") @@ -217,14 +217,14 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 15) + assert.Len(t, apiIssues, 17) query = url.Values{"assigned": {"true"}, "state": {"all"}, "token": {token}} link.RawQuery = query.Encode() req = NewRequest(t, "GET", link.String()) resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 1) + assert.Len(t, apiIssues, 2) query = url.Values{"milestones": {"milestone1"}, "state": {"all"}, "token": {token}} link.RawQuery = query.Encode() @@ -252,7 +252,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = MakeRequest(t, req, http.StatusOK) 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 link.RawQuery = query.Encode() diff --git a/integrations/api_nodeinfo_test.go b/integrations/api_nodeinfo_test.go index 822dbf3f0e..c2fcd2fea5 100644 --- a/integrations/api_nodeinfo_test.go +++ b/integrations/api_nodeinfo_test.go @@ -29,7 +29,7 @@ func TestNodeinfo(t *testing.T) { assert.True(t, nodeinfo.OpenRegistrations) assert.Equal(t, "gitea", nodeinfo.Software.Name) 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) }) } diff --git a/integrations/issue_test.go b/integrations/issue_test.go index c6b801c9a6..8e04b99d5e 100644 --- a/integrations/issue_test.go +++ b/integrations/issue_test.go @@ -392,7 +392,7 @@ func TestSearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) 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 query.Add("limit", "20") @@ -400,14 +400,14 @@ func TestSearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 15) + assert.Len(t, apiIssues, 17) query = url.Values{"assigned": {"true"}, "state": {"all"}} link.RawQuery = query.Encode() req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 1) + assert.Len(t, apiIssues, 2) query = url.Values{"milestones": {"milestone1"}, "state": {"all"}} link.RawQuery = query.Encode() @@ -435,7 +435,7 @@ func TestSearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 3) + assert.Len(t, apiIssues, 5) query = url.Values{"owner": {"user3"}, "team": {"team1"}} // organization + team link.RawQuery = query.Encode() diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index d5738d5db4..39dacc92ff 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -184,3 +184,27 @@ is_pull: false created_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 diff --git a/models/fixtures/issue_assignees.yml b/models/fixtures/issue_assignees.yml index 2e89b2b0b3..e5d36f921a 100644 --- a/models/fixtures/issue_assignees.yml +++ b/models/fixtures/issue_assignees.yml @@ -10,3 +10,7 @@ id: 3 assignee_id: 2 issue_id: 6 +- + id: 4 + assignee_id: 2 + issue_id: 17 diff --git a/models/fixtures/issue_index.yml b/models/fixtures/issue_index.yml index 49d95c57ab..de6e955804 100644 --- a/models/fixtures/issue_index.yml +++ b/models/fixtures/issue_index.yml @@ -10,6 +10,9 @@ - group_id: 10 max_index: 1 +- + group_id: 32 + max_index: 2 - group_id: 48 max_index: 1 @@ -21,4 +24,4 @@ max_index: 1 - group_id: 51 - max_index: 1 \ No newline at end of file + max_index: 1 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 475cda3b55..450c2f26af 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -483,7 +483,7 @@ is_private: false num_stars: 0 num_forks: 0 - num_issues: 0 + num_issues: 2 is_mirror: false status: 0 diff --git a/models/fixtures/team_unit.yml b/models/fixtures/team_unit.yml index 66f0d22efd..2e23a63129 100644 --- a/models/fixtures/team_unit.yml +++ b/models/fixtures/team_unit.yml @@ -252,6 +252,7 @@ - id: 43 + org_id: 3 team_id: 7 type: 2 # issues access_mode: 2 diff --git a/models/issue.go b/models/issue.go index d9540f25dd..c344998b90 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1349,9 +1349,7 @@ func (opts *IssuesOptions) setupSessionNoLimit(sess *xorm.Session) { } if opts.User != nil { - sess.And( - issuePullAccessibleRepoCond("issue.repo_id", opts.User.ID, opts.Org, opts.Team, opts.IsPull.IsTrue()), - ) + sess.And(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") opts.setupSessionWithLimit(sess) + sortIssuesSession(sess, opts.SortType, opts.PriorityRepoID) 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.Join("INNER", "repository", "`issue`.repo_id = `repository`.id") opts.setupSessionNoLimit(sess) + return sess.Count() } diff --git a/models/issue_test.go b/models/issue_test.go index 9b82fc80fb..9a8b7bd533 100644 --- a/models/issue_test.go +++ b/models/issue_test.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/foreignreference" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/organization" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" @@ -287,6 +288,20 @@ func TestGetUserIssueStats(t *testing.T) { 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) { stats, err := GetUserIssueStats(test.Opts) @@ -341,7 +356,7 @@ func TestGetRepoIDsForIssuesOptions(t *testing.T) { IssuesOptions{ AssigneeID: 2, }, - []int64{3}, + []int64{3, 32}, }, { IssuesOptions{ @@ -595,5 +610,5 @@ func TestCountIssues(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) count, err := CountIssues(&IssuesOptions{}) assert.NoError(t, err) - assert.EqualValues(t, 15, count) + assert.EqualValues(t, 17, count) } diff --git a/models/repo_list.go b/models/repo_list.go index 35b2ab5bf8..4b76cbc08b 100644 --- a/models/repo_list.go +++ b/models/repo_list.go @@ -9,6 +9,7 @@ import ( "strings" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" @@ -246,11 +247,26 @@ func teamUnitsRepoCond(id string, userID, orgID, teamID int64, units ...unit.Typ builder.Eq{ "team_id": teamID, }.And( - builder.In( - "team_id", builder.Select("team_id").From("team_user").Where( - builder.Eq{ + builder.Or( + // Check if the user is member of the team. + builder.In( + "team_id", builder.Select("team_id").From("team_user").Where( + builder.Eq{ + "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( builder.In( diff --git a/routers/web/user/home.go b/routers/web/user/home.go index 2e7b382de6..37f6b88352 100644 --- a/routers/web/user/home.go +++ b/routers/web/user/home.go @@ -443,12 +443,13 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { AllLimited: false, } - if ctxUser.IsOrganization() && ctx.Org.Team != nil { - repoOpts.TeamID = ctx.Org.Team.ID + if team != nil { + repoOpts.TeamID = team.ID } switch filterMode { case models.FilterModeAll: + case models.FilterModeYourRepositories: case models.FilterModeAssign: opts.AssigneeID = ctx.Doer.ID case models.FilterModeCreate: @@ -457,13 +458,6 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { opts.MentionedID = ctx.Doer.ID case models.FilterModeReviewRequested: 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. @@ -595,13 +589,7 @@ func buildIssueOverview(ctx *context.Context, unitType unit.Type) { Org: org, 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) if err != nil { ctx.ServerError("GetUserIssueStats Shown", err)