diff options
author | Elena Neuschild <eneuschild@gmail.com> | 2021-01-13 05:19:17 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-01-12 23:19:17 -0500 |
commit | 564030336dbfe1227ec458ecdedc0cfabdd4c1cc (patch) | |
tree | 7311fc29a8ed0c112a7891a11c34fbf76d9ad0eb /routers/user | |
parent | 81467e6f35f343b911c09f746deca869a48da4c8 (diff) | |
download | gitea-564030336dbfe1227ec458ecdedc0cfabdd4c1cc.tar.gz gitea-564030336dbfe1227ec458ecdedc0cfabdd4c1cc.zip |
Issues overview should not show issues from archived repos (#13220)
* Add lots of comments to user.Issues()
* Answered some questions from comments
* fix typo in comment
* Refac user.Issues(): add func repoIDs
* Refac user.Issues(): add func userRepoIDs
* Refac user.Issues(): add func issueIDsFromSearch
* Refac user.Issues(): improve error handling
* Refac user.Issues(): add inline documentation and move variable declarations closer to their usages
* Refac user.Issues(): add func repoIDMap
* Refac user.Issues(): cleanup
* Refac: Separate Issues from Pulls during routing
* fix typo in comment
* Adapt Unittests to Refactoring
* Issue13171: Issue and PR Overviews now ignore archived Repositories
* changed some verbatim SQL conditions to builder.Eq
* models/issue.go: use OptionalBool properly
Co-authored-by: 6543 <6543@obermui.de>
* Use IsArchived rather than ExcludeArchivedRepos
* fixed broken test after merge
* added nil check
* Added Unit Test securing Issue 13171 fix
* Improved IsArchived filtering in issue.GetUserIssueStats
* Removed unused func
* Added grouping to avoid returning duplicate repo IDs
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Gitea <gitea@fake.local>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Diffstat (limited to 'routers/user')
-rw-r--r-- | routers/user/home.go | 414 | ||||
-rw-r--r-- | routers/user/home_test.go | 48 |
2 files changed, 313 insertions, 149 deletions
diff --git a/routers/user/home.go b/routers/user/home.go index 2b59b971aa..3c27bbe2a8 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -37,7 +37,7 @@ const ( tplProfile base.TplName = "user/profile" ) -// getDashboardContextUser finds out dashboard is viewing as which context user. +// getDashboardContextUser finds out which context user dashboard is being viewed as . func getDashboardContextUser(ctx *context.Context) *models.User { ctxUser := ctx.User orgName := ctx.Params(":org") @@ -324,46 +324,66 @@ func Milestones(ctx *context.Context) { ctx.HTML(200, tplMilestones) } -// Regexp for repos query -var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) +// Pulls renders the user's pull request overview page +func Pulls(ctx *context.Context) { + if models.UnitTypePullRequests.UnitGlobalDisabled() { + log.Debug("Pull request overview page not available as it is globally disabled.") + ctx.Status(404) + return + } + + ctx.Data["Title"] = ctx.Tr("pull_requests") + ctx.Data["PageIsPulls"] = true + buildIssueOverview(ctx, models.UnitTypePullRequests) +} -// Issues render the user issues page +// Issues renders the user's issues overview page func Issues(ctx *context.Context) { - isPullList := ctx.Params(":type") == "pulls" - unitType := models.UnitTypeIssues - if isPullList { - if models.UnitTypePullRequests.UnitGlobalDisabled() { - log.Debug("Pull request overview page not available as it is globally disabled.") - ctx.Status(404) - return - } + if models.UnitTypeIssues.UnitGlobalDisabled() { + log.Debug("Issues overview page not available as it is globally disabled.") + ctx.Status(404) + return + } - ctx.Data["Title"] = ctx.Tr("pull_requests") - ctx.Data["PageIsPulls"] = true - unitType = models.UnitTypePullRequests - } else { - if models.UnitTypeIssues.UnitGlobalDisabled() { - log.Debug("Issues overview page not available as it is globally disabled.") - ctx.Status(404) - return - } + ctx.Data["Title"] = ctx.Tr("issues") + ctx.Data["PageIsIssues"] = true + buildIssueOverview(ctx, models.UnitTypeIssues) +} - ctx.Data["Title"] = ctx.Tr("issues") - ctx.Data["PageIsIssues"] = true - } +// Regexp for repos query +var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) + +func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { + + // ---------------------------------------------------- + // Determine user; can be either user or organization. + // Return with NotFound or ServerError if unsuccessful. + // ---------------------------------------------------- ctxUser := getDashboardContextUser(ctx) if ctx.Written() { return } - // Organization does not have view type and filter mode. var ( viewType string sortType = ctx.Query("sort") filterMode = models.FilterModeAll ) + // -------------------------------------------------------------------------------- + // Distinguish User from Organization. + // Org: + // - Remember pre-determined viewType string for later. Will be posted to ctx.Data. + // Organization does not have view type and filter mode. + // User: + // - Use ctx.Query("type") to determine filterMode. + // The type is set when clicking for example "assigned to me" on the overview page. + // - Remember either this or a fallback. Will be posted to ctx.Data. + // -------------------------------------------------------------------------------- + + // TODO: distinguish during routing + viewType = ctx.Query("type") switch viewType { case "assigned": @@ -377,72 +397,30 @@ func Issues(ctx *context.Context) { viewType = "your_repositories" } - page := ctx.QueryInt("page") - if page <= 1 { - page = 1 - } + // -------------------------------------------------------------------------- + // Build opts (IssuesOptions), which contains filter information. + // Will eventually be used to retrieve issues relevant for the overview page. + // Note: Non-final states of opts are used in-between, namely for: + // - Keyword search + // - Count Issues by repo + // -------------------------------------------------------------------------- - reposQuery := ctx.Query("repos") - var repoIDs []int64 - if len(reposQuery) != 0 { - if issueReposQueryPattern.MatchString(reposQuery) { - // remove "[" and "]" from string - reposQuery = reposQuery[1 : len(reposQuery)-1] - //for each ID (delimiter ",") add to int to repoIDs - for _, rID := range strings.Split(reposQuery, ",") { - // Ensure nonempty string entries - if rID != "" && rID != "0" { - rIDint64, err := strconv.ParseInt(rID, 10, 64) - if err == nil { - repoIDs = append(repoIDs, rIDint64) - } - } - } - } else { - log.Warn("issueReposQueryPattern not match with query") - } + isPullList := unitType == models.UnitTypePullRequests + opts := &models.IssuesOptions{ + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, + IsArchived: util.OptionalBoolFalse, } - isShowClosed := ctx.Query("state") == "closed" - - // Get repositories. - var err error - var userRepoIDs []int64 - if ctxUser.IsOrganization() { - var env models.AccessibleReposEnvironment - if ctx.Org.Team != nil { - env = ctxUser.AccessibleTeamReposEnv(ctx.Org.Team) - } else { - env, err = ctxUser.AccessibleReposEnv(ctx.User.ID) - if err != nil { - ctx.ServerError("AccessibleReposEnv", err) - return - } - } - userRepoIDs, err = env.RepoIDs(1, ctxUser.NumRepos) - if err != nil { - ctx.ServerError("env.RepoIDs", err) - return - } - userRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctx.User, userRepoIDs, unitType) - if err != nil { - ctx.ServerError("FilterOutRepoIdsWithoutUnitAccess", err) - return - } - } else { - userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) - if err != nil { - ctx.ServerError("ctxUser.GetAccessRepoIDs", err) - return - } + // Get repository IDs where User/Org/Team has access. + var team *models.Team + if ctx.Org != nil { + team = ctx.Org.Team } - if len(userRepoIDs) == 0 { - userRepoIDs = []int64{-1} - } - - opts := &models.IssuesOptions{ - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, + userRepoIDs, err := getActiveUserRepoIDs(ctxUser, team, unitType) + if err != nil { + ctx.ServerError("userRepoIDs", err) + return } switch filterMode { @@ -460,47 +438,56 @@ func Issues(ctx *context.Context) { opts.RepoIDs = userRepoIDs } - var forceEmpty bool - var issueIDsFromSearch []int64 - var keyword = strings.Trim(ctx.Query("q"), " ") + // keyword holds the search term entered into the search field. + keyword := strings.Trim(ctx.Query("q"), " ") + ctx.Data["Keyword"] = keyword - if len(keyword) > 0 { - searchRepoIDs, err := models.GetRepoIDsForIssuesOptions(opts, ctxUser) - if err != nil { - ctx.ServerError("GetRepoIDsForIssuesOptions", err) - return - } - issueIDsFromSearch, err = issue_indexer.SearchIssuesByKeyword(searchRepoIDs, keyword) - if err != nil { - ctx.ServerError("SearchIssuesByKeyword", err) - return - } - if len(issueIDsFromSearch) > 0 { - opts.IssueIDs = issueIDsFromSearch - } else { - forceEmpty = true - } + // Execute keyword search for issues. + // USING NON-FINAL STATE OF opts FOR A QUERY. + issueIDsFromSearch, err := issueIDsFromSearch(ctxUser, keyword, opts) + if err != nil { + ctx.ServerError("issueIDsFromSearch", err) + return } - ctx.Data["Keyword"] = keyword + // Ensure no issues are returned if a keyword was provided that didn't match any issues. + var forceEmpty bool + + if len(issueIDsFromSearch) > 0 { + opts.IssueIDs = issueIDsFromSearch + } else if len(keyword) > 0 { + forceEmpty = true + } + // Educated guess: Do or don't show closed issues. + isShowClosed := ctx.Query("state") == "closed" opts.IsClosed = util.OptionalBoolOf(isShowClosed) - var counts map[int64]int64 + // Filter repos and count issues in them. Count will be used later. + // USING NON-FINAL STATE OF opts FOR A QUERY. + var issueCountByRepo map[int64]int64 if !forceEmpty { - counts, err = models.CountIssuesByRepo(opts) + issueCountByRepo, err = models.CountIssuesByRepo(opts) if err != nil { ctx.ServerError("CountIssuesByRepo", err) return } } + // Make sure page number is at least 1. Will be posted to ctx.Data. + page := ctx.QueryInt("page") + if page <= 1 { + page = 1 + } opts.Page = page opts.PageSize = setting.UI.IssuePagingNum + + // Get IDs for labels (a filter option for issues/pulls). + // Required for IssuesOptions. var labelIDs []int64 - selectLabels := ctx.Query("labels") - if len(selectLabels) > 0 && selectLabels != "0" { - labelIDs, err = base.StringsToInt64s(strings.Split(selectLabels, ",")) + selectedLabels := ctx.Query("labels") + if len(selectedLabels) > 0 && selectedLabels != "0" { + labelIDs, err = base.StringsToInt64s(strings.Split(selectedLabels, ",")) if err != nil { ctx.ServerError("StringsToInt64s", err) return @@ -508,10 +495,19 @@ func Issues(ctx *context.Context) { } opts.LabelIDs = labelIDs + // Parse ctx.Query("repos") and remember matched repo IDs for later. + // Gets set when clicking filters on the issues overview page. + repoIDs := getRepoIDs(ctx.Query("repos")) if len(repoIDs) > 0 { opts.RepoIDs = repoIDs } + // ------------------------------ + // Get issues as defined by opts. + // ------------------------------ + + // Slice of Issues that will be displayed on the overview page + // USING FINAL STATE OF opts FOR A QUERY. var issues []*models.Issue if !forceEmpty { issues, err = models.Issues(opts) @@ -523,40 +519,22 @@ func Issues(ctx *context.Context) { issues = []*models.Issue{} } - approvalCounts, err := models.IssueList(issues).GetApprovalCounts() - if err != nil { - ctx.ServerError("ApprovalCounts", err) - return - } - - showReposMap := make(map[int64]*models.Repository, len(counts)) - for repoID := range counts { - if repoID > 0 { - if _, ok := showReposMap[repoID]; !ok { - repo, err := models.GetRepositoryByID(repoID) - if models.IsErrRepoNotExist(err) { - ctx.NotFound("GetRepositoryByID", err) - return - } else if err != nil { - ctx.ServerError("GetRepositoryByID", fmt.Errorf("[%d]%v", repoID, err)) - return - } - showReposMap[repoID] = repo - } - repo := showReposMap[repoID] + // ---------------------------------- + // Add repository pointers to Issues. + // ---------------------------------- - // Check if user has access to given repository. - perm, err := models.GetUserRepoPermission(repo, ctxUser) - if err != nil { - ctx.ServerError("GetUserRepoPermission", fmt.Errorf("[%d]%v", repoID, err)) - return - } - if !perm.CanRead(unitType) { - log.Debug("User created Issues in Repository which they no longer have access to: [%d]", repoID) - } + // showReposMap maps repository IDs to their Repository pointers. + showReposMap, err := repoIDMap(ctxUser, issueCountByRepo, unitType) + if err != nil { + if models.IsErrRepoNotExist(err) { + ctx.NotFound("GetRepositoryByID", err) + return } + ctx.ServerError("repoIDMap", err) + return } + // a RepositoryList showRepos := models.RepositoryListOfMap(showReposMap) sort.Sort(showRepos) if err = showRepos.LoadAttributes(); err != nil { @@ -564,6 +542,7 @@ func Issues(ctx *context.Context) { return } + // maps pull request IDs to their CommitStatus. Will be posted to ctx.Data. var commitStatus = make(map[int64]*models.CommitStatus, len(issues)) for _, issue := range issues { issue.Repo = showReposMap[issue.RepoID] @@ -574,12 +553,17 @@ func Issues(ctx *context.Context) { } } + // ------------------------------- + // Fill stats to post to ctx.Data. + // ------------------------------- + userIssueStatsOpts := models.UserIssueStatsOptions{ UserID: ctx.User.ID, UserRepoIDs: userRepoIDs, FilterMode: filterMode, IsPull: isPullList, IsClosed: isShowClosed, + IsArchived: util.OptionalBoolFalse, LabelIDs: opts.LabelIDs, } if len(repoIDs) > 0 { @@ -603,6 +587,7 @@ func Issues(ctx *context.Context) { IsPull: isPullList, IsClosed: isShowClosed, IssueIDs: issueIDsFromSearch, + IsArchived: util.OptionalBoolFalse, LabelIDs: opts.LabelIDs, } if len(repoIDs) > 0 { @@ -628,6 +613,7 @@ func Issues(ctx *context.Context) { IsPull: isPullList, IsClosed: isShowClosed, IssueIDs: issueIDsFromSearch, + IsArchived: util.OptionalBoolFalse, LabelIDs: opts.LabelIDs, } if ctxUser.IsOrganization() { @@ -642,20 +628,28 @@ func Issues(ctx *context.Context) { allIssueStats = &models.IssueStats{} } + // Will be posted to ctx.Data. var shownIssues int - var totalIssues int if !isShowClosed { shownIssues = int(shownIssueStats.OpenCount) - totalIssues = int(allIssueStats.OpenCount) + ctx.Data["TotalIssueCount"] = int(allIssueStats.OpenCount) } else { shownIssues = int(shownIssueStats.ClosedCount) - totalIssues = int(allIssueStats.ClosedCount) + ctx.Data["TotalIssueCount"] = int(allIssueStats.ClosedCount) } + ctx.Data["IsShowClosed"] = isShowClosed + ctx.Data["IssueRefEndNames"], ctx.Data["IssueRefURLs"] = issue_service.GetRefEndNamesAndURLs(issues, ctx.Query("RepoLink")) ctx.Data["Issues"] = issues + + approvalCounts, err := models.IssueList(issues).GetApprovalCounts() + if err != nil { + ctx.ServerError("ApprovalCounts", err) + return + } ctx.Data["ApprovalCounts"] = func(issueID int64, typ string) int64 { counts, ok := approvalCounts[issueID] if !ok || len(counts) == 0 { @@ -676,15 +670,14 @@ func Issues(ctx *context.Context) { } ctx.Data["CommitStatus"] = commitStatus ctx.Data["Repos"] = showRepos - ctx.Data["Counts"] = counts + ctx.Data["Counts"] = issueCountByRepo ctx.Data["IssueStats"] = userIssueStats ctx.Data["ShownIssueStats"] = shownIssueStats ctx.Data["ViewType"] = viewType ctx.Data["SortType"] = sortType ctx.Data["RepoIDs"] = repoIDs ctx.Data["IsShowClosed"] = isShowClosed - ctx.Data["TotalIssueCount"] = totalIssues - ctx.Data["SelectLabels"] = selectLabels + ctx.Data["SelectLabels"] = selectedLabels if isShowClosed { ctx.Data["State"] = "closed" @@ -711,6 +704,131 @@ func Issues(ctx *context.Context) { ctx.HTML(200, tplIssues) } +func getRepoIDs(reposQuery string) []int64 { + if len(reposQuery) == 0 { + return []int64{} + } + if !issueReposQueryPattern.MatchString(reposQuery) { + log.Warn("issueReposQueryPattern does not match query") + return []int64{} + } + + var repoIDs []int64 + // remove "[" and "]" from string + reposQuery = reposQuery[1 : len(reposQuery)-1] + //for each ID (delimiter ",") add to int to repoIDs + for _, rID := range strings.Split(reposQuery, ",") { + // Ensure nonempty string entries + if rID != "" && rID != "0" { + rIDint64, err := strconv.ParseInt(rID, 10, 64) + if err == nil { + repoIDs = append(repoIDs, rIDint64) + } + } + } + + return repoIDs +} + +func getActiveUserRepoIDs(ctxUser *models.User, team *models.Team, unitType models.UnitType) ([]int64, error) { + var userRepoIDs []int64 + var err error + + if ctxUser.IsOrganization() { + userRepoIDs, err = getActiveTeamOrOrgRepoIds(ctxUser, team, unitType) + if err != nil { + return nil, fmt.Errorf("orgRepoIds: %v", err) + } + } else { + userRepoIDs, err = ctxUser.GetActiveAccessRepoIDs(unitType) + if err != nil { + return nil, fmt.Errorf("ctxUser.GetAccessRepoIDs: %v", err) + } + } + + if len(userRepoIDs) == 0 { + userRepoIDs = []int64{-1} + } + + return userRepoIDs, nil +} + +// getActiveTeamOrOrgRepoIds gets RepoIDs for ctxUser as Organization. +// Should be called if and only if ctxUser.IsOrganization == true. +func getActiveTeamOrOrgRepoIds(ctxUser *models.User, team *models.Team, unitType models.UnitType) ([]int64, error) { + var orgRepoIDs []int64 + var err error + var env models.AccessibleReposEnvironment + + if team != nil { + env = ctxUser.AccessibleTeamReposEnv(team) + if err != nil { + return nil, fmt.Errorf("AccessibleTeamReposEnv: %v", err) + } + } else { + env, err = ctxUser.AccessibleReposEnv(ctxUser.ID) + if err != nil { + return nil, fmt.Errorf("AccessibleReposEnv: %v", err) + } + } + orgRepoIDs, err = env.RepoIDs(1, ctxUser.NumRepos) + if err != nil { + return nil, fmt.Errorf("env.RepoIDs: %v", err) + } + orgRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctxUser, orgRepoIDs, unitType) + if err != nil { + return nil, fmt.Errorf("FilterOutRepoIdsWithoutUnitAccess: %v", err) + } + + return orgRepoIDs, nil +} + +func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.IssuesOptions) ([]int64, error) { + if len(keyword) == 0 { + return []int64{}, nil + } + + searchRepoIDs, err := models.GetRepoIDsForIssuesOptions(opts, ctxUser) + if err != nil { + return nil, fmt.Errorf("GetRepoIDsForIssuesOptions: %v", err) + } + issueIDsFromSearch, err := issue_indexer.SearchIssuesByKeyword(searchRepoIDs, keyword) + if err != nil { + return nil, fmt.Errorf("SearchIssuesByKeyword: %v", err) + } + + return issueIDsFromSearch, nil +} + +func repoIDMap(ctxUser *models.User, issueCountByRepo map[int64]int64, unitType models.UnitType) (map[int64]*models.Repository, error) { + repoByID := make(map[int64]*models.Repository, len(issueCountByRepo)) + for id := range issueCountByRepo { + if id <= 0 { + continue + } + if _, ok := repoByID[id]; !ok { + repo, err := models.GetRepositoryByID(id) + if models.IsErrRepoNotExist(err) { + return nil, err + } else if err != nil { + return nil, fmt.Errorf("GetRepositoryByID: [%d]%v", id, err) + } + repoByID[id] = repo + } + repo := repoByID[id] + + // Check if user has access to given repository. + perm, err := models.GetUserRepoPermission(repo, ctxUser) + if err != nil { + return nil, fmt.Errorf("GetUserRepoPermission: [%d]%v", id, err) + } + if !perm.CanRead(unitType) { + log.Debug("User created Issues in Repository which they no longer have access to: [%d]", id) + } + } + return repoByID, nil +} + // ShowSSHKeys output all the ssh keys of user by uid func ShowSSHKeys(ctx *context.Context, uid int64) { keys, err := models.ListPublicKeys(uid, models.ListOptions{}) diff --git a/routers/user/home_test.go b/routers/user/home_test.go index ff48953d44..ecc02fd33a 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -15,13 +15,46 @@ import ( "github.com/stretchr/testify/assert" ) +func TestArchivedIssues(t *testing.T) { + // Arrange + setting.UI.IssuePagingNum = 1 + assert.NoError(t, models.LoadFixtures()) + + ctx := test.MockContext(t, "issues") + test.LoadUser(t, ctx, 30) + ctx.Req.Form.Set("state", "open") + + // Assume: User 30 has access to two Repos with Issues, one of the Repos being archived. + repos, _, _ := models.GetUserRepositories(&models.SearchRepoOptions{Actor: ctx.User}) + assert.Len(t, repos, 2) + IsArchived := make(map[int64]bool) + NumIssues := make(map[int64]int) + for _, repo := range repos { + IsArchived[repo.ID] = repo.IsArchived + NumIssues[repo.ID] = repo.NumIssues + } + assert.EqualValues(t, false, IsArchived[50]) + assert.EqualValues(t, 1, NumIssues[50]) + assert.EqualValues(t, true, IsArchived[51]) + assert.EqualValues(t, 1, NumIssues[51]) + + // Act + Issues(ctx) + + // Assert: One Issue (ID 30) from one Repo (ID 50) is retrieved, while nothing from archived Repo 51 is retrieved + assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) + + assert.EqualValues(t, map[int64]int64{50: 1}, ctx.Data["Counts"]) + assert.Len(t, ctx.Data["Issues"], 1) + assert.Len(t, ctx.Data["Repos"], 1) +} + func TestIssues(t *testing.T) { setting.UI.IssuePagingNum = 1 assert.NoError(t, models.LoadFixtures()) ctx := test.MockContext(t, "issues") test.LoadUser(t, ctx, 2) - ctx.SetParams(":type", "issues") ctx.Req.Form.Set("state", "closed") Issues(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) @@ -32,6 +65,19 @@ func TestIssues(t *testing.T) { assert.Len(t, ctx.Data["Repos"], 2) } +func TestPulls(t *testing.T) { + setting.UI.IssuePagingNum = 20 + assert.NoError(t, models.LoadFixtures()) + + ctx := test.MockContext(t, "pulls") + test.LoadUser(t, ctx, 2) + ctx.Req.Form.Set("state", "open") + Pulls(ctx) + assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) + + assert.Len(t, ctx.Data["Issues"], 3) +} + func TestMilestones(t *testing.T) { setting.UI.IssuePagingNum = 1 assert.NoError(t, models.LoadFixtures()) |