From 91f207d269dac2da594524478a448e57f994b9e3 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 20 Oct 2020 18:01:50 +0200 Subject: [PATCH 01/23] Add lots of comments to user.Issues() --- routers/user/home.go | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/routers/user/home.go b/routers/user/home.go index f7f1786b3302b..951bd70258b1a 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -322,6 +322,10 @@ var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) // Issues render the user issues page func Issues(ctx *context.Context) { + + // Distinguish Issues from Pulls. (Q: Why not make that distinction during routing and call different functions?) + // Return 404 if feature is disabled. + isPullList := ctx.Params(":type") == "pulls" unitType := models.UnitTypeIssues if isPullList { @@ -357,6 +361,13 @@ func Issues(ctx *context.Context) { filterMode = models.FilterModeAll ) + // Distinguish User from Organization. Q: Again, why not distinguish during routing? + // Org: + // - Remember pre-determined string for later. Will be posted to ctx.Data. + // User: + // - Use ctx.Query("type") to determine filterMode. Q: Can there even be different values in ctx.Query("type")? How? + // - Remember either this or a fallback. Will be posted to ctx.Data. + if ctxUser.IsOrganization() { viewType = "your_repositories" } else { @@ -374,11 +385,14 @@ func Issues(ctx *context.Context) { } } + // Make sure page number is at least 1. Will be posted to ctx.Data. page := ctx.QueryInt("page") if page <= 1 { page = 1 } + // Parse ctx.Query("repos") and remember matched repo IDs for later. + // Q: Where can ctx.Query("repos") be filled? With the given routing, where can a value come from? reposQuery := ctx.Query("repos") var repoIDs []int64 if len(reposQuery) != 0 { @@ -400,9 +414,11 @@ func Issues(ctx *context.Context) { } } + // No idea what this means. isShowClosed := ctx.Query("state") == "closed" - // Get repositories. + // Get repository IDs where User/Org has access. + // Again, the distinction between User and Org could perhaps be handled more elegantly. var err error var userRepoIDs []int64 if ctxUser.IsOrganization() { @@ -432,6 +448,8 @@ func Issues(ctx *context.Context) { userRepoIDs = []int64{-1} } + // Build IssuesOptions, which contains filter information. + opts := &models.IssuesOptions{ IsPull: util.OptionalBoolOf(isPullList), SortType: sortType, @@ -448,7 +466,11 @@ func Issues(ctx *context.Context) { opts.MentionedID = ctxUser.ID } + // Execute keyword search for issues. + + // Ensure issue list is emtpy if keyword search didn't produce any results. var forceEmpty bool + // Required for IssuesOptions. var issueIDsFromSearch []int64 var keyword = strings.Trim(ctx.Query("q"), " ") @@ -474,6 +496,7 @@ func Issues(ctx *context.Context) { opts.IsClosed = util.OptionalBoolOf(isShowClosed) + // Filter repos and count issues in them. Count will be used later. var counts map[int64]int64 if !forceEmpty { counts, err = models.CountIssuesByRepo(opts) @@ -485,6 +508,9 @@ func Issues(ctx *context.Context) { opts.Page = page opts.PageSize = setting.UI.IssuePagingNum + + // Get IDs for labels. Q: What are labels? Where do they come from? + // Required for IssuesOptions. var labelIDs []int64 selectLabels := ctx.Query("labels") if len(selectLabels) > 0 && selectLabels != "0" { @@ -500,6 +526,7 @@ func Issues(ctx *context.Context) { opts.RepoIDs = repoIDs } + // Get issues as defined by opts. var issues []*models.Issue if !forceEmpty { issues, err = models.Issues(opts) @@ -517,6 +544,7 @@ func Issues(ctx *context.Context) { return } + // showReposMap maps repository IDs to their Repository pointers. showReposMap := make(map[int64]*models.Repository, len(counts)) for repoID := range counts { if repoID > 0 { @@ -545,6 +573,7 @@ func Issues(ctx *context.Context) { } } + // a RepositoryList showRepos := models.RepositoryListOfMap(showReposMap) sort.Sort(showRepos) if err = showRepos.LoadAttributes(); err != nil { @@ -552,6 +581,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] @@ -571,12 +601,15 @@ func Issues(ctx *context.Context) { if len(repoIDs) > 0 { userIssueStatsOpts.UserRepoIDs = repoIDs } + + // Will be posted to ctx.Data. userIssueStats, err := models.GetUserIssueStats(userIssueStatsOpts) if err != nil { ctx.ServerError("GetUserIssueStats User", err) return } + // Will be posted to ctx.Data. var shownIssueStats *models.IssueStats if !forceEmpty { statsOpts := models.UserIssueStatsOptions{ @@ -599,6 +632,7 @@ func Issues(ctx *context.Context) { shownIssueStats = &models.IssueStats{} } + // Will be posted to ctx.Data. var allIssueStats *models.IssueStats if !forceEmpty { allIssueStats, err = models.GetUserIssueStats(models.UserIssueStatsOptions{ @@ -617,6 +651,7 @@ func Issues(ctx *context.Context) { allIssueStats = &models.IssueStats{} } + // Will be posted to ctx.Data. var shownIssues int var totalIssues int if !isShowClosed { From 89bb53daa9a85a472aaa29fa0b0e1e765700e998 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 20 Oct 2020 18:50:16 +0200 Subject: [PATCH 02/23] Answered some questions from comments --- routers/user/home.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 951bd70258b1a..ab05497ea7d39 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -365,7 +365,7 @@ func Issues(ctx *context.Context) { // Org: // - Remember pre-determined string for later. Will be posted to ctx.Data. // User: - // - Use ctx.Query("type") to determine filterMode. Q: Can there even be different values in ctx.Query("type")? How? + // - 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. if ctxUser.IsOrganization() { @@ -391,8 +391,7 @@ func Issues(ctx *context.Context) { page = 1 } - // Parse ctx.Query("repos") and remember matched repo IDs for later. - // Q: Where can ctx.Query("repos") be filled? With the given routing, where can a value come from? + // Parse ctx.Query("repos") -- gets set when clicking filters -- and remember matched repo IDs for later. reposQuery := ctx.Query("repos") var repoIDs []int64 if len(reposQuery) != 0 { @@ -509,7 +508,7 @@ func Issues(ctx *context.Context) { opts.Page = page opts.PageSize = setting.UI.IssuePagingNum - // Get IDs for labels. Q: What are labels? Where do they come from? + // Get IDs for labels (a filter option for issues/pulls). // Required for IssuesOptions. var labelIDs []int64 selectLabels := ctx.Query("labels") From c2f7accdabea069196926726e372325bb4837955 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 20 Oct 2020 19:53:05 +0200 Subject: [PATCH 03/23] fix typo in comment --- routers/user/home.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/user/home.go b/routers/user/home.go index ab05497ea7d39..c832c876b9149 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -467,7 +467,7 @@ func Issues(ctx *context.Context) { // Execute keyword search for issues. - // Ensure issue list is emtpy if keyword search didn't produce any results. + // Ensure issue list is empty if keyword search didn't produce any results. var forceEmpty bool // Required for IssuesOptions. var issueIDsFromSearch []int64 From 19221f0b9291468dcae5fa1489bed1675160643f Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Wed, 21 Oct 2020 18:21:48 +0200 Subject: [PATCH 04/23] Refac user.Issues(): add func repoIDs --- routers/user/home.go | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index c832c876b9149..279b66cf477cb 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -392,26 +392,7 @@ func Issues(ctx *context.Context) { } // Parse ctx.Query("repos") -- gets set when clicking filters -- and remember matched repo IDs for later. - 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") - } - } + repoIDs := repoIDs(ctx.Query("repos")) // No idea what this means. isShowClosed := ctx.Query("state") == "closed" @@ -719,6 +700,29 @@ func Issues(ctx *context.Context) { ctx.HTML(200, tplIssues) } +func repoIDs(reposQuery string) []int64 { + 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") + } + } + return repoIDs +} + // ShowSSHKeys output all the ssh keys of user by uid func ShowSSHKeys(ctx *context.Context, uid int64) { keys, err := models.ListPublicKeys(uid, models.ListOptions{}) From 355a90f87c86cce8ef587b413f1c890d449a7438 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Thu, 22 Oct 2020 15:14:47 +0200 Subject: [PATCH 05/23] Refac user.Issues(): add func userRepoIDs --- routers/user/home.go | 73 +++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 28 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 279b66cf477cb..aaf53c55d30a5 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -398,34 +398,10 @@ func Issues(ctx *context.Context) { isShowClosed := ctx.Query("state") == "closed" // Get repository IDs where User/Org has access. - // Again, the distinction between User and Org could perhaps be handled more elegantly. - var err error - var userRepoIDs []int64 - if ctxUser.IsOrganization() { - 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 - } - } - if len(userRepoIDs) == 0 { - userRepoIDs = []int64{-1} + userRepoIDs, errorTitle, err := userRepoIDs(ctxUser, ctx.User, unitType) + if err != nil { + ctx.ServerError(errorTitle, err) + return } // Build IssuesOptions, which contains filter information. @@ -723,6 +699,47 @@ func repoIDs(reposQuery string) []int64 { return repoIDs } +func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, string, error) { + var err error + var errorTitle string + var userRepoIDs []int64 + + if ctxUser.IsOrganization() { + userRepoIDs, errorTitle, err = orgRepoIds(ctxUser, user, unitType) + } else { + userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) + errorTitle = "ctxUser.GetAccessRepoIDs" + } + + if err != nil { + return nil, errorTitle, err + } + + if len(userRepoIDs) == 0 { + userRepoIDs = []int64{-1} + } + + return userRepoIDs, "", nil +} + +func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, string, error) { + var orgRepoIDs []int64 + var err error + env, err := org.AccessibleReposEnv(user.ID) + if err != nil { + return nil, "AccessibleReposEnv", err + } + orgRepoIDs, err = env.RepoIDs(1, org.NumRepos) + if err != nil { + return nil, "env.RepoIDs", err + } + orgRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(user, orgRepoIDs, unitType) + if err != nil { + return nil, "FilterOutRepoIdsWithoutUnitAccess", err + } + return orgRepoIDs, "", 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{}) From b2e6d26be67c32330acd723a2f12ae3a4d2a9d5b Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Thu, 22 Oct 2020 19:23:05 +0200 Subject: [PATCH 06/23] Refac user.Issues(): add func issueIDsFromSearch --- routers/user/home.go | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index aaf53c55d30a5..91ee33f7850ae 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -422,32 +422,19 @@ func Issues(ctx *context.Context) { opts.MentionedID = ctxUser.ID } + // Required for IssuesOptions. + var keyword = strings.Trim(ctx.Query("q"), " ") // Execute keyword search for issues. + issueIDsFromSearch, errorTitle, err := issueIDsFromSearch(ctxUser, keyword, opts) // Ensure issue list is empty if keyword search didn't produce any results. var forceEmpty bool - // Required for IssuesOptions. - var issueIDsFromSearch []int64 - var keyword = strings.Trim(ctx.Query("q"), " ") - 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 - } + if len(issueIDsFromSearch) > 0 { + opts.IssueIDs = issueIDsFromSearch + } else if len(keyword) > 0 { + forceEmpty = true } - ctx.Data["Keyword"] = keyword opts.IsClosed = util.OptionalBoolOf(isShowClosed) @@ -740,6 +727,21 @@ func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, stri return orgRepoIDs, "", nil } +func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.IssuesOptions) ([]int64, string, error) { + var issueIDsFromSearch []int64 + if len(keyword) > 0 { + searchRepoIDs, err := models.GetRepoIDsForIssuesOptions(opts, ctxUser) + if err != nil { + return nil, "GetRepoIDsForIssuesOptions", err + } + issueIDsFromSearch, err = issue_indexer.SearchIssuesByKeyword(searchRepoIDs, keyword) + if err != nil { + return nil, "SearchIssuesByKeyword", err + } + } + return issueIDsFromSearch, "", 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{}) From 9673406451696427813b0fa448751be532792fc8 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 26 Oct 2020 13:01:40 +0100 Subject: [PATCH 07/23] Refac user.Issues(): improve error handling --- routers/user/home.go | 45 ++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 91ee33f7850ae..5e95ac429e1a4 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -398,9 +398,9 @@ func Issues(ctx *context.Context) { isShowClosed := ctx.Query("state") == "closed" // Get repository IDs where User/Org has access. - userRepoIDs, errorTitle, err := userRepoIDs(ctxUser, ctx.User, unitType) + userRepoIDs, err := userRepoIDs(ctxUser, ctx.User, unitType) if err != nil { - ctx.ServerError(errorTitle, err) + ctx.ServerError("userRepoIDs", err) return } @@ -425,7 +425,11 @@ func Issues(ctx *context.Context) { // Required for IssuesOptions. var keyword = strings.Trim(ctx.Query("q"), " ") // Execute keyword search for issues. - issueIDsFromSearch, errorTitle, err := issueIDsFromSearch(ctxUser, keyword, opts) + issueIDsFromSearch, err := issueIDsFromSearch(ctxUser, keyword, opts) + if err != nil { + ctx.ServerError("issueIDsFromSearch", err) + return + } // Ensure issue list is empty if keyword search didn't produce any results. var forceEmpty bool @@ -686,60 +690,61 @@ func repoIDs(reposQuery string) []int64 { return repoIDs } -func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, string, error) { +func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, error) { var err error - var errorTitle string var userRepoIDs []int64 if ctxUser.IsOrganization() { - userRepoIDs, errorTitle, err = orgRepoIds(ctxUser, user, unitType) + userRepoIDs, err = orgRepoIds(ctxUser, user, unitType) + if err != nil { + return nil, err + } } else { userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) - errorTitle = "ctxUser.GetAccessRepoIDs" - } - if err != nil { - return nil, errorTitle, err + if err != nil { + return nil, fmt.Errorf("ctxUser.GetAccessRepoIDs: %v", err) + } } if len(userRepoIDs) == 0 { userRepoIDs = []int64{-1} } - return userRepoIDs, "", nil + return userRepoIDs, nil } -func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, string, error) { +func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, error) { var orgRepoIDs []int64 var err error env, err := org.AccessibleReposEnv(user.ID) if err != nil { - return nil, "AccessibleReposEnv", err + return nil, fmt.Errorf("AccessibleReposEnv: %v", err) } orgRepoIDs, err = env.RepoIDs(1, org.NumRepos) if err != nil { - return nil, "env.RepoIDs", err + return nil, fmt.Errorf("env.RepoIDs: %v", err) } orgRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(user, orgRepoIDs, unitType) if err != nil { - return nil, "FilterOutRepoIdsWithoutUnitAccess", err + return nil, fmt.Errorf("FilterOutRepoIdsWithoutUnitAccess: %v", err) } - return orgRepoIDs, "", nil + return orgRepoIDs, nil } -func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.IssuesOptions) ([]int64, string, error) { +func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.IssuesOptions) ([]int64, error) { var issueIDsFromSearch []int64 if len(keyword) > 0 { searchRepoIDs, err := models.GetRepoIDsForIssuesOptions(opts, ctxUser) if err != nil { - return nil, "GetRepoIDsForIssuesOptions", err + return nil, fmt.Errorf("GetRepoIDsForIssuesOptions: %v", err) } issueIDsFromSearch, err = issue_indexer.SearchIssuesByKeyword(searchRepoIDs, keyword) if err != nil { - return nil, "SearchIssuesByKeyword", err + return nil, fmt.Errorf("SearchIssuesByKeyword: %v", err) } } - return issueIDsFromSearch, "", nil + return issueIDsFromSearch, nil } // ShowSSHKeys output all the ssh keys of user by uid From dbc22f79e06877558b95aa9fd189a71c3ec6dcc9 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 26 Oct 2020 13:34:40 +0100 Subject: [PATCH 08/23] Refac user.Issues(): add inline documentation and move variable declarations closer to their usages --- routers/user/home.go | 97 ++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 31 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 5e95ac429e1a4..68643c3b62c0f 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -323,8 +323,12 @@ var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) // Issues render the user issues page func Issues(ctx *context.Context) { - // Distinguish Issues from Pulls. (Q: Why not make that distinction during routing and call different functions?) + // ---------------------------------- + // Distinguish Issues from Pulls. // Return 404 if feature is disabled. + // ---------------------------------- + + // TODO: make that distinction during routing and call different functions isPullList := ctx.Params(":type") == "pulls" unitType := models.UnitTypeIssues @@ -349,25 +353,36 @@ func Issues(ctx *context.Context) { ctx.Data["PageIsIssues"] = true } + // ---------------------------------------------------- + // 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. Q: Again, why not distinguish during routing? + // -------------------------------------------------------------------------------- + // Distinguish User from Organization. // Org: - // - Remember pre-determined string for later. Will be posted to ctx.Data. + // - 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. + // - 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 + // Organization does not have view type and filter mode. if ctxUser.IsOrganization() { viewType = "your_repositories" } else { @@ -385,17 +400,18 @@ func Issues(ctx *context.Context) { } } - // Make sure page number is at least 1. Will be posted to ctx.Data. - 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 inbetween, namely for: + // - Keyword search + // - Count Issues by repo + // -------------------------------------------------------------------------- - // Parse ctx.Query("repos") -- gets set when clicking filters -- and remember matched repo IDs for later. - repoIDs := repoIDs(ctx.Query("repos")) - - // No idea what this means. - isShowClosed := ctx.Query("state") == "closed" + opts := &models.IssuesOptions{ + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, + } // Get repository IDs where User/Org has access. userRepoIDs, err := userRepoIDs(ctxUser, ctx.User, unitType) @@ -404,13 +420,6 @@ func Issues(ctx *context.Context) { return } - // Build IssuesOptions, which contains filter information. - - opts := &models.IssuesOptions{ - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - } - switch filterMode { case models.FilterModeAll: opts.RepoIDs = userRepoIDs @@ -422,9 +431,10 @@ func Issues(ctx *context.Context) { opts.MentionedID = ctxUser.ID } - // Required for IssuesOptions. - var keyword = strings.Trim(ctx.Query("q"), " ") + // keyword holds the search term entered in to the search field. + keyword := strings.Trim(ctx.Query("q"), " ") // 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) @@ -441,9 +451,12 @@ func Issues(ctx *context.Context) { } ctx.Data["Keyword"] = keyword + // Educated guess: Do or don't show closed issues. + isShowClosed := ctx.Query("state") == "closed" opts.IsClosed = util.OptionalBoolOf(isShowClosed) // Filter repos and count issues in them. Count will be used later. + // USING NON-FINAL STATE OF opts FOR A QUERY. var counts map[int64]int64 if !forceEmpty { counts, err = models.CountIssuesByRepo(opts) @@ -453,6 +466,11 @@ func Issues(ctx *context.Context) { } } + // 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 @@ -469,11 +487,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 := repoIDs(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) @@ -485,11 +511,9 @@ func Issues(ctx *context.Context) { issues = []*models.Issue{} } - approvalCounts, err := models.IssueList(issues).GetApprovalCounts() - if err != nil { - ctx.ServerError("ApprovalCounts", err) - return - } + // ---------------------------------- + // Add repository pointers to Issues. + // ---------------------------------- // showReposMap maps repository IDs to their Repository pointers. showReposMap := make(map[int64]*models.Repository, len(counts)) @@ -538,6 +562,10 @@ func Issues(ctx *context.Context) { } } + // ----------- + // Fill stats. + // ----------- + userIssueStatsOpts := models.UserIssueStatsOptions{ UserID: ctxUser.ID, UserRepoIDs: userRepoIDs, @@ -609,10 +637,19 @@ func Issues(ctx *context.Context) { totalIssues = int(allIssueStats.ClosedCount) } + ctx.Data["IsShowClosed"] = isShowClosed + ctx.Data["TotalIssueCount"] = totalIssues + 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 { @@ -639,8 +676,6 @@ func Issues(ctx *context.Context) { ctx.Data["ViewType"] = viewType ctx.Data["SortType"] = sortType ctx.Data["RepoIDs"] = repoIDs - ctx.Data["IsShowClosed"] = isShowClosed - ctx.Data["TotalIssueCount"] = totalIssues if isShowClosed { ctx.Data["State"] = "closed" From fdb2237d62f71ed94e7ca8d4c9dd7b999b6b192e Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 26 Oct 2020 17:58:49 +0100 Subject: [PATCH 09/23] Refac user.Issues(): add func repoIDMap --- routers/user/home.go | 75 +++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 68643c3b62c0f..b00eac7a0b6bd 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -516,32 +516,14 @@ func Issues(ctx *context.Context) { // ---------------------------------- // showReposMap maps repository IDs to their Repository pointers. - 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] - - // 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(models.UnitTypeIssues) { - log.Error("User created Issues in Repository which they no longer have access to: [%d]", repoID) - } + showReposMap, err := repoIDMap(ctxUser, counts) + if err != nil { + if models.IsErrRepoNotExist(err) { + ctx.NotFound("GetRepositoryByID", err) + return } + ctx.ServerError("repoIDMap", err) + return } // a RepositoryList @@ -562,9 +544,9 @@ func Issues(ctx *context.Context) { } } - // ----------- - // Fill stats. - // ----------- + // ------------------------------- + // Fill stats to post to ctx.Data. + // ------------------------------- userIssueStatsOpts := models.UserIssueStatsOptions{ UserID: ctxUser.ID, @@ -577,14 +559,12 @@ func Issues(ctx *context.Context) { userIssueStatsOpts.UserRepoIDs = repoIDs } - // Will be posted to ctx.Data. userIssueStats, err := models.GetUserIssueStats(userIssueStatsOpts) if err != nil { ctx.ServerError("GetUserIssueStats User", err) return } - // Will be posted to ctx.Data. var shownIssueStats *models.IssueStats if !forceEmpty { statsOpts := models.UserIssueStatsOptions{ @@ -607,7 +587,6 @@ func Issues(ctx *context.Context) { shownIssueStats = &models.IssueStats{} } - // Will be posted to ctx.Data. var allIssueStats *models.IssueStats if !forceEmpty { allIssueStats, err = models.GetUserIssueStats(models.UserIssueStatsOptions{ @@ -628,17 +607,15 @@ func Issues(ctx *context.Context) { // 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["TotalIssueCount"] = totalIssues ctx.Data["IssueRefEndNames"], ctx.Data["IssueRefURLs"] = issue_service.GetRefEndNamesAndURLs(issues, ctx.Query("RepoLink")) @@ -782,6 +759,34 @@ func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.Issue return issueIDsFromSearch, nil } +func repoIDMap(ctxUser *models.User, counts map[int64]int64) (map[int64]*models.Repository, error) { + 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) { + return nil, err + } else if err != nil { + return nil, fmt.Errorf("GetRepositoryByID: [%d]%v", repoID, err) + } + showReposMap[repoID] = repo + } + repo := showReposMap[repoID] + + // Check if user has access to given repository. + perm, err := models.GetUserRepoPermission(repo, ctxUser) + if err != nil { + return nil, fmt.Errorf("GetUserRepoPermission: [%d]%v", repoID, err) + } + if !perm.CanRead(models.UnitTypeIssues) { + log.Error("User created Issues in Repository which they no longer have access to: [%d]", repoID) + } + } + } + return showReposMap, 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{}) From 5e8d323969d45fd9b8b6cc96b9b7ee9056431ea6 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 26 Oct 2020 21:00:58 +0100 Subject: [PATCH 10/23] Refac user.Issues(): cleanup --- routers/user/home.go | 128 ++++++++++++++++++++++--------------------- 1 file changed, 67 insertions(+), 61 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index b00eac7a0b6bd..9f42e49e57651 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -382,7 +382,6 @@ func Issues(ctx *context.Context) { // TODO: distinguish during routing - // Organization does not have view type and filter mode. if ctxUser.IsOrganization() { viewType = "your_repositories" } else { @@ -431,8 +430,10 @@ func Issues(ctx *context.Context) { opts.MentionedID = ctxUser.ID } - // keyword holds the search term entered in to the search field. + // keyword holds the search term entered into the search field. keyword := strings.Trim(ctx.Query("q"), " ") + ctx.Data["Keyword"] = keyword + // Execute keyword search for issues. // USING NON-FINAL STATE OF opts FOR A QUERY. issueIDsFromSearch, err := issueIDsFromSearch(ctxUser, keyword, opts) @@ -441,7 +442,7 @@ func Issues(ctx *context.Context) { return } - // Ensure issue list is empty if keyword search didn't produce any results. + // Ensure no issues are returned if a keyword was provided that didn't match any issues. var forceEmpty bool if len(issueIDsFromSearch) > 0 { @@ -449,7 +450,6 @@ func Issues(ctx *context.Context) { } else if len(keyword) > 0 { forceEmpty = true } - ctx.Data["Keyword"] = keyword // Educated guess: Do or don't show closed issues. isShowClosed := ctx.Query("state") == "closed" @@ -457,9 +457,9 @@ func Issues(ctx *context.Context) { // Filter repos and count issues in them. Count will be used later. // USING NON-FINAL STATE OF opts FOR A QUERY. - var counts map[int64]int64 + 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 @@ -477,9 +477,9 @@ func Issues(ctx *context.Context) { // 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 @@ -516,7 +516,7 @@ func Issues(ctx *context.Context) { // ---------------------------------- // showReposMap maps repository IDs to their Repository pointers. - showReposMap, err := repoIDMap(ctxUser, counts) + showReposMap, err := repoIDMap(ctxUser, issueCountByRepo) if err != nil { if models.IsErrRepoNotExist(err) { ctx.NotFound("GetRepositoryByID", err) @@ -647,7 +647,7 @@ 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 @@ -680,40 +680,42 @@ func Issues(ctx *context.Context) { } func repoIDs(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 - 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) - } - } + // 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") } } + return repoIDs } func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, error) { - var err error var userRepoIDs []int64 + var err error if ctxUser.IsOrganization() { userRepoIDs, err = orgRepoIds(ctxUser, user, unitType) if err != nil { - return nil, err + return nil, fmt.Errorf("orgRepoIds: %v", err) } } else { userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) - if err != nil { return nil, fmt.Errorf("ctxUser.GetAccessRepoIDs: %v", err) } @@ -729,6 +731,7 @@ func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, error) { var orgRepoIDs []int64 var err error + env, err := org.AccessibleReposEnv(user.ID) if err != nil { return nil, fmt.Errorf("AccessibleReposEnv: %v", err) @@ -745,46 +748,49 @@ func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, erro } func issueIDsFromSearch(ctxUser *models.User, keyword string, opts *models.IssuesOptions) ([]int64, error) { - var issueIDsFromSearch []int64 - if len(keyword) > 0 { - 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) - } + 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, counts map[int64]int64) (map[int64]*models.Repository, error) { - 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) { - return nil, err - } else if err != nil { - return nil, fmt.Errorf("GetRepositoryByID: [%d]%v", repoID, err) - } - showReposMap[repoID] = repo +func repoIDMap(ctxUser *models.User, issueCountByRepo map[int64]int64) (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) } - repo := showReposMap[repoID] + 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", repoID, err) - } - if !perm.CanRead(models.UnitTypeIssues) { - log.Error("User created Issues in Repository which they no longer have access to: [%d]", repoID) - } + // 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(models.UnitTypeIssues) { + log.Error("User created Issues in Repository which they no longer have access to: [%d]", id) } } - return showReposMap, nil + return repoByID, nil } // ShowSSHKeys output all the ssh keys of user by uid From 0294079f449c2956e2900a64576e8250c2a88f6f Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 27 Oct 2020 12:22:05 +0100 Subject: [PATCH 11/23] Refac: Separate Issues from Pulls during routing --- routers/routes/routes.go | 6 +++-- routers/user/home.go | 56 +++++++++++++++++++--------------------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/routers/routes/routes.go b/routers/routes/routes.go index f123613b1f87e..de63eda64ffc7 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -368,7 +368,8 @@ func RegisterRoutes(m *macaron.Macaron) { }, ignSignIn) m.Combo("/install", routers.InstallInit).Get(routers.Install). Post(bindIgnErr(auth.InstallForm{}), routers.InstallPost) - m.Get("/^:type(issues|pulls)$", reqSignIn, user.Issues) + m.Get("/issues", reqSignIn, user.Issues) + m.Get("/pulls", reqSignIn, user.Pulls) m.Get("/milestones", reqSignIn, reqMilestonesDashboardPageEnabled, user.Milestones) // ***** START: User ***** @@ -615,7 +616,8 @@ func RegisterRoutes(m *macaron.Macaron) { m.Group("/:org", func() { m.Get("/dashboard", user.Dashboard) - m.Get("/^:type(issues|pulls)$", user.Issues) + m.Get("/issues", user.Issues) + m.Get("/pulls", user.Pulls) m.Get("/milestones", reqMilestonesDashboardPageEnabled, user.Milestones) m.Get("/members", org.Members) m.Post("/members/action/:action", org.MembersAction) diff --git a/routers/user/home.go b/routers/user/home.go index 9f42e49e57651..674719292281f 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -317,41 +317,36 @@ func Milestones(ctx *context.Context) { ctx.HTML(200, tplMilestones) } -// Regexp for repos query -var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) - -// Issues render the user issues page -func Issues(ctx *context.Context) { +// 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 + } - // ---------------------------------- - // Distinguish Issues from Pulls. - // Return 404 if feature is disabled. - // ---------------------------------- + ctx.Data["Title"] = ctx.Tr("pull_requests") + ctx.Data["PageIsPulls"] = true + buildIssueOverview(ctx, models.UnitTypePullRequests) +} - // TODO: make that distinction during routing and call different functions +// Issues renders the user's issues overview page +func Issues(ctx *context.Context) { + if models.UnitTypeIssues.UnitGlobalDisabled() { + log.Debug("Issues overview page not available as it is globally disabled.") + ctx.Status(404) + return + } - 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 - } + ctx.Data["Title"] = ctx.Tr("issues") + ctx.Data["PageIsIssues"] = true + buildIssueOverview(ctx, models.UnitTypeIssues) +} - 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 - } +// Regexp for repos query +var issueReposQueryPattern = regexp.MustCompile(`^\[\d+(,\d+)*,?\]$`) - ctx.Data["Title"] = ctx.Tr("issues") - ctx.Data["PageIsIssues"] = true - } +func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { // ---------------------------------------------------- // Determine user; can be either user or organization. @@ -407,6 +402,7 @@ func Issues(ctx *context.Context) { // - Count Issues by repo // -------------------------------------------------------------------------- + isPullList := unitType == models.UnitTypePullRequests opts := &models.IssuesOptions{ IsPull: util.OptionalBoolOf(isPullList), SortType: sortType, From afb9af7a41479e12fdd627368c2e5304ba2e39f2 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 27 Oct 2020 13:55:04 +0100 Subject: [PATCH 12/23] fix typo in comment --- routers/user/home.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/user/home.go b/routers/user/home.go index 674719292281f..fa7e552a5a233 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -397,7 +397,7 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { // -------------------------------------------------------------------------- // 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 inbetween, namely for: + // Note: Non-final states of opts are used in-between, namely for: // - Keyword search // - Count Issues by repo // -------------------------------------------------------------------------- From faec896c9a51234d1ebfc5afd2f559df51fb82e7 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 10 Nov 2020 19:27:29 +0100 Subject: [PATCH 13/23] Adapt Unittests to Refactoring --- models/fixtures/repository.yml | 2 ++ routers/user/home_test.go | 14 +++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index c7f55a8f70211..5b8e9d0c94fef 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -4,6 +4,7 @@ owner_name: user2 lower_name: repo1 name: repo1 + is_archived: false is_empty: false is_private: false num_issues: 2 @@ -23,6 +24,7 @@ owner_name: user2 lower_name: repo2 name: repo2 + is_archived: false is_private: true num_issues: 2 num_closed_issues: 1 diff --git a/routers/user/home_test.go b/routers/user/home_test.go index ff48953d44023..082322c59f569 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -21,7 +21,6 @@ func TestIssues(t *testing.T) { 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 +31,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"], 4) +} + func TestMilestones(t *testing.T) { setting.UI.IssuePagingNum = 1 assert.NoError(t, models.LoadFixtures()) From 5e4ca72b54c38f8c387d005e534f94443b99109e Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Tue, 10 Nov 2020 19:30:23 +0100 Subject: [PATCH 14/23] Issue13171: Issue and PR Overviews now ignore archived Repositories --- models/issue.go | 30 ++++++++++++++------ models/org.go | 21 +++++++++++++- models/user.go | 53 +++++++++++++++++++++++++++++++++++ routers/user/home.go | 58 +++++++++++++++++++++------------------ routers/user/home_test.go | 2 +- 5 files changed, 127 insertions(+), 37 deletions(-) diff --git a/models/issue.go b/models/issue.go index ee75623f53025..c1f75a620c5fa 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1101,7 +1101,8 @@ type IssuesOptions struct { SortType string IssueIDs []int64 // prioritize issues from this repo - PriorityRepoID int64 + PriorityRepoID int64 + ExcludeArchivedRepos util.OptionalBool } // sortIssuesSession sort an issues-related session based on the provided @@ -1198,6 +1199,10 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) { sess.And("issue.is_pull=?", false) } + if opts.ExcludeArchivedRepos == util.OptionalBoolTrue { + sess.Join("INNER", "repository", "issue.repo_id = repository.id").And("repository.is_archived <> true") + } + if opts.LabelIDs != nil { for i, labelID := range opts.LabelIDs { if labelID > 0 { @@ -1484,13 +1489,14 @@ func getIssueStatsChunk(opts *IssueStatsOptions, issueIDs []int64) (*IssueStats, // UserIssueStatsOptions contains parameters accepted by GetUserIssueStats. type UserIssueStatsOptions struct { - UserID int64 - RepoIDs []int64 - UserRepoIDs []int64 - FilterMode int - IsPull bool - IsClosed bool - IssueIDs []int64 + UserID int64 + RepoIDs []int64 + UserRepoIDs []int64 + FilterMode int + IsPull bool + IsClosed bool + IssueIDs []int64 + ExcludeArchivedRepos bool } // GetUserIssueStats returns issue statistic information for dashboard by given conditions. @@ -1506,6 +1512,14 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { if len(opts.IssueIDs) > 0 { cond = cond.And(builder.In("issue.id", opts.IssueIDs)) } + if opts.ExcludeArchivedRepos { + activeRepoIDs := []int64{} + r := []*Repository{} + s := x.Table("repository").Where("is_archived <> true") + s.Select("id").Find(&activeRepoIDs) + s.Find(&r) + cond = cond.And(builder.In("issue.repo_id", activeRepoIDs)) + } switch opts.FilterMode { case FilterModeAll: diff --git a/models/org.go b/models/org.go index 84f2892e4a170..59985a2a757a2 100644 --- a/models/org.go +++ b/models/org.go @@ -737,6 +737,7 @@ func (org *User) GetUserTeams(userID int64) ([]*Team, error) { type AccessibleReposEnvironment interface { CountRepos() (int64, error) RepoIDs(page, pageSize int) ([]int64, error) + ActiveRepoIDs(page, pageSize int) ([]int64, error) Repos(page, pageSize int) ([]*Repository, error) MirrorRepos() ([]*Repository, error) AddKeyword(keyword string) @@ -752,7 +753,7 @@ type accessibleReposEnv struct { orderBy SearchOrderBy } -// AccessibleReposEnv an AccessibleReposEnvironment for the repositories in `org` +// AccessibleReposEnv builds an AccessibleReposEnvironment for the repositories in `org` // that are accessible to the specified user. func (org *User) AccessibleReposEnv(userID int64) (AccessibleReposEnvironment, error) { return org.accessibleReposEnv(x, userID) @@ -828,6 +829,24 @@ func (env *accessibleReposEnv) RepoIDs(page, pageSize int) ([]int64, error) { Find(&repoIDs) } +func (env *accessibleReposEnv) ActiveRepoIDs(page, pageSize int) ([]int64, error) { + if page <= 0 { + page = 1 + } + + repoIDs := make([]int64, 0, pageSize) + return repoIDs, env.e. + Table("repository"). + Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id"). + Where(env.cond()). + Where("is_archived <> true"). + GroupBy("`repository`.id,`repository`."+strings.Fields(string(env.orderBy))[0]). + OrderBy(string(env.orderBy)). + Limit(pageSize, (page-1)*pageSize). + Cols("`repository`.id"). + Find(&repoIDs) +} + func (env *accessibleReposEnv) Repos(page, pageSize int) ([]*Repository, error) { repoIDs, err := env.RepoIDs(page, pageSize) if err != nil { diff --git a/models/user.go b/models/user.go index 42f70b4666ffe..bc667ff9cd2c5 100644 --- a/models/user.go +++ b/models/user.go @@ -491,6 +491,23 @@ func (u *User) GetRepositoryIDs(units ...UnitType) ([]int64, error) { return ids, sess.Where("owner_id = ?", u.ID).Find(&ids) } +// GetActiveRepositoryIDs returns non-archived repositories IDs where user owned and has unittypes +// Caller shall check that units is not globally disabled +func (u *User) GetActiveRepositoryIDs(units ...UnitType) ([]int64, error) { + var ids []int64 + + sess := x.Table("repository").Cols("repository.id") + + if len(units) > 0 { + sess = sess.Join("INNER", "repo_unit", "repository.id = repo_unit.repo_id") + sess = sess.In("repo_unit.type", units) + } + + sess.Where("is_archived <> true") + + return ids, sess.Where("owner_id = ?", u.ID).Find(&ids) +} + // GetOrgRepositoryIDs returns repositories IDs where user's team owned and has unittypes // Caller shall check that units is not globally disabled func (u *User) GetOrgRepositoryIDs(units ...UnitType) ([]int64, error) { @@ -512,6 +529,28 @@ func (u *User) GetOrgRepositoryIDs(units ...UnitType) ([]int64, error) { return ids, nil } +// GetActiveOrgRepositoryIDs returns non-archived repositories IDs where user's team owned and has unittypes +// Caller shall check that units is not globally disabled +func (u *User) GetActiveOrgRepositoryIDs(units ...UnitType) ([]int64, error) { + var ids []int64 + + if err := x.Table("repository"). + Cols("repository.id"). + Join("INNER", "team_user", "repository.owner_id = team_user.org_id"). + Join("INNER", "team_repo", "(? != ? and repository.is_private != ?) OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)", true, u.IsRestricted, true). + Where("team_user.uid = ?", u.ID). + Where("is_archived <> true"). + GroupBy("repository.id").Find(&ids); err != nil { + return nil, err + } + + if len(units) > 0 { + return FilterOutRepoIdsWithoutUnitAccess(u, ids, units...) + } + + return ids, nil +} + // GetAccessRepoIDs returns all repositories IDs where user's or user is a team member organizations // Caller shall check that units is not globally disabled func (u *User) GetAccessRepoIDs(units ...UnitType) ([]int64, error) { @@ -526,6 +565,20 @@ func (u *User) GetAccessRepoIDs(units ...UnitType) ([]int64, error) { return append(ids, ids2...), nil } +// GetActiveAccessRepoIDs returns all non-archived repositories IDs where user's or user is a team member organizations +// Caller shall check that units is not globally disabled +func (u *User) GetActiveAccessRepoIDs(units ...UnitType) ([]int64, error) { + ids, err := u.GetActiveRepositoryIDs(units...) + if err != nil { + return nil, err + } + ids2, err := u.GetActiveOrgRepositoryIDs(units...) + if err != nil { + return nil, err + } + return append(ids, ids2...), nil +} + // GetMirrorRepositories returns mirror repositories that user owns, including private repositories. func (u *User) GetMirrorRepositories() ([]*Repository, error) { return GetUserMirrorRepositories(u.ID) diff --git a/routers/user/home.go b/routers/user/home.go index fa7e552a5a233..6b80fbb9a795d 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -404,12 +404,13 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { isPullList := unitType == models.UnitTypePullRequests opts := &models.IssuesOptions{ - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, + ExcludeArchivedRepos: util.OptionalBoolTrue, } // Get repository IDs where User/Org has access. - userRepoIDs, err := userRepoIDs(ctxUser, ctx.User, unitType) + userRepoIDs, err := getActiveUserRepoIDs(ctxUser, ctx.User, unitType) if err != nil { ctx.ServerError("userRepoIDs", err) return @@ -485,7 +486,7 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { // Parse ctx.Query("repos") and remember matched repo IDs for later. // Gets set when clicking filters on the issues overview page. - repoIDs := repoIDs(ctx.Query("repos")) + repoIDs := getRepoIDs(ctx.Query("repos")) if len(repoIDs) > 0 { opts.RepoIDs = repoIDs } @@ -545,11 +546,12 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { // ------------------------------- userIssueStatsOpts := models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + ExcludeArchivedRepos: true, } if len(repoIDs) > 0 { userIssueStatsOpts.UserRepoIDs = repoIDs @@ -564,12 +566,13 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { var shownIssueStats *models.IssueStats if !forceEmpty { statsOpts := models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, - IssueIDs: issueIDsFromSearch, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + IssueIDs: issueIDsFromSearch, + ExcludeArchivedRepos: true, } if len(repoIDs) > 0 { statsOpts.RepoIDs = repoIDs @@ -586,12 +589,13 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { var allIssueStats *models.IssueStats if !forceEmpty { allIssueStats, err = models.GetUserIssueStats(models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, - IssueIDs: issueIDsFromSearch, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + IssueIDs: issueIDsFromSearch, + ExcludeArchivedRepos: true, }) if err != nil { ctx.ServerError("GetUserIssueStats All", err) @@ -675,7 +679,7 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { ctx.HTML(200, tplIssues) } -func repoIDs(reposQuery string) []int64 { +func getRepoIDs(reposQuery string) []int64 { if len(reposQuery) == 0 { return []int64{} } @@ -701,17 +705,17 @@ func repoIDs(reposQuery string) []int64 { return repoIDs } -func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, error) { +func getActiveUserRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, error) { var userRepoIDs []int64 var err error if ctxUser.IsOrganization() { - userRepoIDs, err = orgRepoIds(ctxUser, user, unitType) + userRepoIDs, err = getActiveOrgRepoIds(ctxUser, user, unitType) if err != nil { return nil, fmt.Errorf("orgRepoIds: %v", err) } } else { - userRepoIDs, err = ctxUser.GetAccessRepoIDs(unitType) + userRepoIDs, err = ctxUser.GetActiveAccessRepoIDs(unitType) if err != nil { return nil, fmt.Errorf("ctxUser.GetAccessRepoIDs: %v", err) } @@ -724,7 +728,7 @@ func userRepoIDs(ctxUser, user *models.User, unitType models.UnitType) ([]int64, return userRepoIDs, nil } -func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, error) { +func getActiveOrgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, error) { var orgRepoIDs []int64 var err error @@ -732,7 +736,7 @@ func orgRepoIds(org, user *models.User, unitType models.UnitType) ([]int64, erro if err != nil { return nil, fmt.Errorf("AccessibleReposEnv: %v", err) } - orgRepoIDs, err = env.RepoIDs(1, org.NumRepos) + orgRepoIDs, err = env.ActiveRepoIDs(1, org.NumRepos) if err != nil { return nil, fmt.Errorf("env.RepoIDs: %v", err) } diff --git a/routers/user/home_test.go b/routers/user/home_test.go index 082322c59f569..8562d812eaa91 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -41,7 +41,7 @@ func TestPulls(t *testing.T) { Pulls(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) - assert.Len(t, ctx.Data["Issues"], 4) + assert.Len(t, ctx.Data["Issues"], 3) } func TestMilestones(t *testing.T) { From 1be19ed54e90fd64260cbf687fc710e0b63db913 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Thu, 26 Nov 2020 18:41:56 +0100 Subject: [PATCH 15/23] changed some verbatim SQL conditions to builder.Eq --- models/issue.go | 4 ++-- models/org.go | 2 +- models/user.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/models/issue.go b/models/issue.go index cb0e4fe2df577..c313cfeb42195 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1209,7 +1209,7 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) { } if opts.ExcludeArchivedRepos == util.OptionalBoolTrue { - sess.Join("INNER", "repository", "issue.repo_id = repository.id").And("repository.is_archived <> true") + sess.Join("INNER", "repository", "issue.repo_id = repository.id").And(builder.Eq{"repository.is_archived": false}) } if opts.LabelIDs != nil { @@ -1524,7 +1524,7 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { if opts.ExcludeArchivedRepos { activeRepoIDs := []int64{} r := []*Repository{} - s := x.Table("repository").Where("is_archived <> true") + s := x.Table("repository").Where(builder.Eq{"is_archived": false}) s.Select("id").Find(&activeRepoIDs) s.Find(&r) cond = cond.And(builder.In("issue.repo_id", activeRepoIDs)) diff --git a/models/org.go b/models/org.go index 59985a2a757a2..84b0085ec3999 100644 --- a/models/org.go +++ b/models/org.go @@ -839,7 +839,7 @@ func (env *accessibleReposEnv) ActiveRepoIDs(page, pageSize int) ([]int64, error Table("repository"). Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id"). Where(env.cond()). - Where("is_archived <> true"). + Where(builder.Eq{"is_archived": false}). GroupBy("`repository`.id,`repository`."+strings.Fields(string(env.orderBy))[0]). OrderBy(string(env.orderBy)). Limit(pageSize, (page-1)*pageSize). diff --git a/models/user.go b/models/user.go index 859542fca1667..ab76e67cc9f0d 100644 --- a/models/user.go +++ b/models/user.go @@ -503,7 +503,7 @@ func (u *User) GetActiveRepositoryIDs(units ...UnitType) ([]int64, error) { sess = sess.In("repo_unit.type", units) } - sess.Where("is_archived <> true") + sess.Where(builder.Eq{"is_archived": false}) return ids, sess.Where("owner_id = ?", u.ID).Find(&ids) } @@ -539,7 +539,7 @@ func (u *User) GetActiveOrgRepositoryIDs(units ...UnitType) ([]int64, error) { Join("INNER", "team_user", "repository.owner_id = team_user.org_id"). Join("INNER", "team_repo", "(? != ? and repository.is_private != ?) OR (team_user.team_id = team_repo.team_id AND repository.id = team_repo.repo_id)", true, u.IsRestricted, true). Where("team_user.uid = ?", u.ID). - Where("is_archived <> true"). + Where(builder.Eq{"is_archived": false}). GroupBy("repository.id").Find(&ids); err != nil { return nil, err } From d4021ad437985ca5809445400dd24cadd350afd2 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 30 Nov 2020 10:24:55 +0100 Subject: [PATCH 16/23] models/issue.go: use OptionalBool properly Co-authored-by: 6543 <6543@obermui.de> --- models/issue.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issue.go b/models/issue.go index c313cfeb42195..c66b376a8193c 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1208,8 +1208,8 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) { sess.And("issue.is_pull=?", false) } - if opts.ExcludeArchivedRepos == util.OptionalBoolTrue { - sess.Join("INNER", "repository", "issue.repo_id = repository.id").And(builder.Eq{"repository.is_archived": false}) + if opts.IsArchived != util.OptionalBoolNone { + sess.Join("INNER", "repository", "issue.repo_id = repository.id").And(builder.Eq{"repository.is_archived": opts.IsArchived.IsTrue()}) } if opts.LabelIDs != nil { From d23d8cd321f6bf788facfe6b32ff2e5c1e0c7c08 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 30 Nov 2020 11:12:37 +0100 Subject: [PATCH 17/23] Use IsArchived rather than ExcludeArchivedRepos --- models/issue.go | 30 ++++++++++++++--------------- routers/user/home.go | 46 ++++++++++++++++++++++---------------------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/models/issue.go b/models/issue.go index c66b376a8193c..518fbede45bcf 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1103,8 +1103,8 @@ type IssuesOptions struct { UpdatedAfterUnix int64 UpdatedBeforeUnix int64 // prioritize issues from this repo - PriorityRepoID int64 - ExcludeArchivedRepos util.OptionalBool + PriorityRepoID int64 + IsArchived util.OptionalBool } // sortIssuesSession sort an issues-related session based on the provided @@ -1498,14 +1498,14 @@ func getIssueStatsChunk(opts *IssueStatsOptions, issueIDs []int64) (*IssueStats, // UserIssueStatsOptions contains parameters accepted by GetUserIssueStats. type UserIssueStatsOptions struct { - UserID int64 - RepoIDs []int64 - UserRepoIDs []int64 - FilterMode int - IsPull bool - IsClosed bool - IssueIDs []int64 - ExcludeArchivedRepos bool + UserID int64 + RepoIDs []int64 + UserRepoIDs []int64 + FilterMode int + IsPull bool + IsClosed bool + IssueIDs []int64 + IsArchived util.OptionalBool } // GetUserIssueStats returns issue statistic information for dashboard by given conditions. @@ -1521,13 +1521,13 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { if len(opts.IssueIDs) > 0 { cond = cond.And(builder.In("issue.id", opts.IssueIDs)) } - if opts.ExcludeArchivedRepos { - activeRepoIDs := []int64{} + if opts.IsArchived != util.OptionalBoolNone { + relevantRepoIDs := []int64{} r := []*Repository{} - s := x.Table("repository").Where(builder.Eq{"is_archived": false}) - s.Select("id").Find(&activeRepoIDs) + s := x.Table("repository").Where(builder.Eq{"is_archived": opts.IsArchived.IsTrue()}) + s.Select("id").Find(&relevantRepoIDs) s.Find(&r) - cond = cond.And(builder.In("issue.repo_id", activeRepoIDs)) + cond = cond.And(builder.In("issue.repo_id", relevantRepoIDs)) } switch opts.FilterMode { diff --git a/routers/user/home.go b/routers/user/home.go index 07f0744736963..8d9dea417bb7a 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -410,9 +410,9 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { isPullList := unitType == models.UnitTypePullRequests opts := &models.IssuesOptions{ - IsPull: util.OptionalBoolOf(isPullList), - SortType: sortType, - ExcludeArchivedRepos: util.OptionalBoolTrue, + IsPull: util.OptionalBoolOf(isPullList), + SortType: sortType, + IsArchived: util.OptionalBoolFalse, } // Get repository IDs where User/Org has access. @@ -552,12 +552,12 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { // ------------------------------- userIssueStatsOpts := models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, - ExcludeArchivedRepos: true, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + IsArchived: util.OptionalBoolFalse, } if len(repoIDs) > 0 { userIssueStatsOpts.UserRepoIDs = repoIDs @@ -572,13 +572,13 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { var shownIssueStats *models.IssueStats if !forceEmpty { statsOpts := models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, - IssueIDs: issueIDsFromSearch, - ExcludeArchivedRepos: true, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + IssueIDs: issueIDsFromSearch, + IsArchived: util.OptionalBoolFalse, } if len(repoIDs) > 0 { statsOpts.RepoIDs = repoIDs @@ -595,13 +595,13 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { var allIssueStats *models.IssueStats if !forceEmpty { allIssueStats, err = models.GetUserIssueStats(models.UserIssueStatsOptions{ - UserID: ctxUser.ID, - UserRepoIDs: userRepoIDs, - FilterMode: filterMode, - IsPull: isPullList, - IsClosed: isShowClosed, - IssueIDs: issueIDsFromSearch, - ExcludeArchivedRepos: true, + UserID: ctxUser.ID, + UserRepoIDs: userRepoIDs, + FilterMode: filterMode, + IsPull: isPullList, + IsClosed: isShowClosed, + IssueIDs: issueIDsFromSearch, + IsArchived: util.OptionalBoolFalse, }) if err != nil { ctx.ServerError("GetUserIssueStats All", err) From 806ba056d627a4c39c7fbe6823bf6f1e1f41ee53 Mon Sep 17 00:00:00 2001 From: Gitea Date: Mon, 4 Jan 2021 18:23:02 +0100 Subject: [PATCH 18/23] fixed broken test after merge --- routers/user/home.go | 23 +++++++++++------------ routers/user/home_test.go | 3 +++ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index 994bbf6791a63..ad1e622219e52 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -417,7 +417,7 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { } // Get repository IDs where User/Org has access. - userRepoIDs, err := getActiveUserRepoIDs(ctx.Org, ctx.User, unitType) + userRepoIDs, err := getActiveUserRepoIDs(ctxUser, ctx.Org.Team, unitType) if err != nil { ctx.ServerError("userRepoIDs", err) return @@ -713,17 +713,17 @@ func getRepoIDs(reposQuery string) []int64 { return repoIDs } -func getActiveUserRepoIDs(ctxOrg *context.Organization, user *models.User, unitType models.UnitType) ([]int64, error) { +func getActiveUserRepoIDs(ctxUser *models.User, team *models.Team, unitType models.UnitType) ([]int64, error) { var userRepoIDs []int64 var err error - if ctxOrg != nil { - userRepoIDs, err = getActiveOrgRepoIds(ctxOrg, user, unitType) + if ctxUser.IsOrganization() { + userRepoIDs, err = getActiveTeamOrOrgRepoIds(ctxUser, team, unitType) if err != nil { return nil, fmt.Errorf("orgRepoIds: %v", err) } } else { - userRepoIDs, err = user.GetActiveAccessRepoIDs(unitType) + userRepoIDs, err = ctxUser.GetActiveAccessRepoIDs(unitType) if err != nil { return nil, fmt.Errorf("ctxUser.GetAccessRepoIDs: %v", err) } @@ -736,21 +736,20 @@ func getActiveUserRepoIDs(ctxOrg *context.Organization, user *models.User, unitT return userRepoIDs, nil } -// getActiveOrgRepoIds gets RepoIDs for ctx.Organisation. +// getActiveTeamOrOrgRepoIds gets RepoIDs for ctxUser as Organization. // Should be called if and only if ctxUser.IsOrganization == true. -func getActiveOrgRepoIds(ctxOrg *context.Organization, user *models.User, unitType models.UnitType) ([]int64, error) { +func getActiveTeamOrOrgRepoIds(ctxUser *models.User, team *models.Team, unitType models.UnitType) ([]int64, error) { var orgRepoIDs []int64 var err error var env models.AccessibleReposEnvironment - ctxUser := ctxOrg.Organization - if ctxOrg.Team != nil { - env = ctxUser.AccessibleTeamReposEnv(ctxOrg.Team) + if team != nil { + env = ctxUser.AccessibleTeamReposEnv(team) if err != nil { return nil, fmt.Errorf("AccessibleTeamReposEnv: %v", err) } } else { - env, err = ctxUser.AccessibleReposEnv(user.ID) + env, err = ctxUser.AccessibleReposEnv(ctxUser.ID) if err != nil { return nil, fmt.Errorf("AccessibleReposEnv: %v", err) } @@ -759,7 +758,7 @@ func getActiveOrgRepoIds(ctxOrg *context.Organization, user *models.User, unitTy if err != nil { return nil, fmt.Errorf("env.RepoIDs: %v", err) } - orgRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(user, orgRepoIDs, unitType) + orgRepoIDs, err = models.FilterOutRepoIdsWithoutUnitAccess(ctxUser, orgRepoIDs, unitType) if err != nil { return nil, fmt.Errorf("FilterOutRepoIdsWithoutUnitAccess: %v", err) } diff --git a/routers/user/home_test.go b/routers/user/home_test.go index 8562d812eaa91..1d0198796ff2b 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -9,6 +9,7 @@ import ( "testing" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" @@ -22,6 +23,7 @@ func TestIssues(t *testing.T) { ctx := test.MockContext(t, "issues") test.LoadUser(t, ctx, 2) ctx.Req.Form.Set("state", "closed") + ctx.Org = &context.Organization{} Issues(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) @@ -38,6 +40,7 @@ func TestPulls(t *testing.T) { ctx := test.MockContext(t, "pulls") test.LoadUser(t, ctx, 2) ctx.Req.Form.Set("state", "open") + ctx.Org = &context.Organization{} Pulls(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) From 93d4d0d284d2a89040f1ebbc9dd1bc87e9c421d5 Mon Sep 17 00:00:00 2001 From: Gitea Date: Tue, 5 Jan 2021 14:48:42 +0100 Subject: [PATCH 19/23] added nil check --- routers/user/home.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/routers/user/home.go b/routers/user/home.go index ec2e7025ea81d..9b65e0b8d61bb 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -412,8 +412,12 @@ func buildIssueOverview(ctx *context.Context, unitType models.UnitType) { IsArchived: util.OptionalBoolFalse, } - // Get repository IDs where User/Org has access. - userRepoIDs, err := getActiveUserRepoIDs(ctxUser, ctx.Org.Team, unitType) + // Get repository IDs where User/Org/Team has access. + var team *models.Team + if ctx.Org != nil { + team = ctx.Org.Team + } + userRepoIDs, err := getActiveUserRepoIDs(ctxUser, team, unitType) if err != nil { ctx.ServerError("userRepoIDs", err) return From cf255128ea41759a44b11b6915dcf3fbfabd6dc9 Mon Sep 17 00:00:00 2001 From: Gitea Date: Tue, 5 Jan 2021 14:49:56 +0100 Subject: [PATCH 20/23] Added Unit Test securing Issue 13171 fix --- integrations/api_issue_test.go | 4 ++-- integrations/api_repo_test.go | 6 ++--- models/fixtures/issue.yml | 25 +++++++++++++++++++++ models/fixtures/repo_unit.yml | 12 ++++++++++ models/fixtures/repository.yml | 40 ++++++++++++++++++++++++++++++++++ models/fixtures/user.yml | 18 +++++++++++++++ models/repo_list_test.go | 6 ++--- models/user_test.go | 4 ++-- routers/user/home_test.go | 37 ++++++++++++++++++++++++++++--- 9 files changed, 139 insertions(+), 13 deletions(-) diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go index 81e5c44873b1b..f932f79feea07 100644 --- a/integrations/api_issue_test.go +++ b/integrations/api_issue_test.go @@ -186,7 +186,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.EqualValues(t, "12", resp.Header().Get("X-Total-Count")) + assert.EqualValues(t, "14", resp.Header().Get("X-Total-Count")) assert.Len(t, apiIssues, 10) //there are more but 10 is page item limit query.Add("limit", "20") @@ -194,7 +194,7 @@ func TestAPISearchIssues(t *testing.T) { req = NewRequest(t, "GET", link.String()) resp = session.MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &apiIssues) - assert.Len(t, apiIssues, 12) + assert.Len(t, apiIssues, 14) query = url.Values{"assigned": {"true"}, "state": {"all"}} link.RawQuery = query.Encode() diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go index 8294a01773351..b6d41fe1e0ba6 100644 --- a/integrations/api_repo_test.go +++ b/integrations/api_repo_test.go @@ -77,9 +77,9 @@ func TestAPISearchRepo(t *testing.T) { expectedResults }{ {name: "RepositoriesMax50", requestURL: "/api/v1/repos/search?limit=50&private=false", expectedResults: expectedResults{ - nil: {count: 28}, - user: {count: 28}, - user2: {count: 28}}, + nil: {count: 30}, + user: {count: 30}, + user2: {count: 30}}, }, {name: "RepositoriesMax10", requestURL: "/api/v1/repos/search?limit=10&private=false", expectedResults: expectedResults{ nil: {count: 10}, diff --git a/models/fixtures/issue.yml b/models/fixtures/issue.yml index 3e836bf5d18fc..31df00d9e6999 100644 --- a/models/fixtures/issue.yml +++ b/models/fixtures/issue.yml @@ -147,3 +147,28 @@ is_pull: true created_unix: 1602935696 updated_unix: 1602935696 + + +- + id: 13 + repo_id: 50 + index: 0 + poster_id: 2 + name: issue in active repo + content: we'll be testing github issue 13171 with this. + is_closed: false + is_pull: false + created_unix: 1602935696 + updated_unix: 1602935696 + +- + id: 14 + repo_id: 51 + index: 0 + poster_id: 2 + name: issue in archived repo + content: we'll be testing github issue 13171 with this. + is_closed: false + is_pull: false + created_unix: 1602935696 + updated_unix: 1602935696 diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 726abf9af97e1..59ab61834070c 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -532,3 +532,15 @@ repo_id: 3 type: 8 created_unix: 946684810 + +- + id: 78 + repo_id: 50 + type: 2 + created_unix: 946684810 + +- + id: 79 + repo_id: 51 + type: 2 + created_unix: 946684810 diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index aebbe110b9473..952408b0c24ef 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -695,3 +695,43 @@ num_issues: 0 is_mirror: false status: 0 + +- + id: 50 + owner_id: 30 + owner_name: user30 + lower_name: repo50 + name: repo50 + is_archived: false + is_empty: false + is_private: false + num_issues: 1 + num_closed_issues: 0 + num_pulls: 0 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_watches: 0 + num_projects: 0 + num_closed_projects: 0 + status: 0 + +- + id: 51 + owner_id: 30 + owner_name: user30 + lower_name: repo51 + name: repo51 + is_archived: true + is_empty: false + is_private: false + num_issues: 1 + num_closed_issues: 0 + num_pulls: 0 + num_closed_pulls: 0 + num_milestones: 0 + num_closed_milestones: 0 + num_watches: 0 + num_projects: 0 + num_closed_projects: 0 + status: 0 \ No newline at end of file diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 655129580ceda..d903a7942f81b 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -507,3 +507,21 @@ avatar_email: user29@example.com num_repos: 0 is_active: true + + +- + id: 30 + lower_name: user30 + name: user30 + full_name: User Thirty + email: user30@example.com + passwd_hash_algo: argon2 + passwd: a3d5fcd92bae586c2e3dbe72daea7a0d27833a8d0227aa1704f4bbd775c1f3b03535b76dd93b0d4d8d22a519dca47df1547b # password + type: 0 # individual + salt: ZogKvWdyEx + is_admin: false + is_restricted: true + avatar: avatar29 + avatar_email: user30@example.com + num_repos: 2 + is_active: true diff --git a/models/repo_list_test.go b/models/repo_list_test.go index 97047b7ffa400..37af9d598d128 100644 --- a/models/repo_list_test.go +++ b/models/repo_list_test.go @@ -187,10 +187,10 @@ func TestSearchRepository(t *testing.T) { count: 14}, {name: "AllPublic/PublicRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, AllPublic: true, Template: util.OptionalBoolFalse}, - count: 26}, + count: 28}, {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborative", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true, AllLimited: true, Template: util.OptionalBoolFalse}, - count: 31}, + count: 33}, {name: "AllPublic/PublicAndPrivateRepositoriesOfUserIncludingCollaborativeByName", opts: &SearchRepoOptions{Keyword: "test", ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 15, Private: true, AllPublic: true}, count: 15}, @@ -199,7 +199,7 @@ func TestSearchRepository(t *testing.T) { count: 13}, {name: "AllPublic/PublicRepositoriesOfOrganization", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, OwnerID: 17, AllPublic: true, Collaborate: util.OptionalBoolFalse, Template: util.OptionalBoolFalse}, - count: 26}, + count: 28}, {name: "AllTemplates", opts: &SearchRepoOptions{ListOptions: ListOptions{Page: 1, PageSize: 10}, Template: util.OptionalBoolTrue}, count: 2}, diff --git a/models/user_test.go b/models/user_test.go index 224acd5f3c590..af654693b55b8 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -136,13 +136,13 @@ func TestSearchUsers(t *testing.T) { } testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", ListOptions: ListOptions{Page: 1}}, - []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29}) + []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 27, 28, 29, 30}) testUserSuccess(&SearchUserOptions{ListOptions: ListOptions{Page: 1}, IsActive: util.OptionalBoolFalse}, []int64{9}) testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", ListOptions: ListOptions{Page: 1}, IsActive: util.OptionalBoolTrue}, - []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 28, 29}) + []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24, 28, 29, 30}) testUserSuccess(&SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", ListOptions: ListOptions{Page: 1}, IsActive: util.OptionalBoolTrue}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) diff --git a/routers/user/home_test.go b/routers/user/home_test.go index 1d0198796ff2b..ecc02fd33a83c 100644 --- a/routers/user/home_test.go +++ b/routers/user/home_test.go @@ -9,13 +9,46 @@ import ( "testing" "code.gitea.io/gitea/models" - "code.gitea.io/gitea/modules/context" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/test" "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()) @@ -23,7 +56,6 @@ func TestIssues(t *testing.T) { ctx := test.MockContext(t, "issues") test.LoadUser(t, ctx, 2) ctx.Req.Form.Set("state", "closed") - ctx.Org = &context.Organization{} Issues(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) @@ -40,7 +72,6 @@ func TestPulls(t *testing.T) { ctx := test.MockContext(t, "pulls") test.LoadUser(t, ctx, 2) ctx.Req.Form.Set("state", "open") - ctx.Org = &context.Organization{} Pulls(ctx) assert.EqualValues(t, http.StatusOK, ctx.Resp.Status()) From 9de7cb44fb5578d2853c7453bbd90730b06277fa Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Wed, 6 Jan 2021 14:52:52 +0100 Subject: [PATCH 21/23] Improved IsArchived filtering in issue.GetUserIssueStats --- models/issue.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/models/issue.go b/models/issue.go index 1017d0f743e06..b517f334c450a 100644 --- a/models/issue.go +++ b/models/issue.go @@ -1523,14 +1523,6 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { if len(opts.IssueIDs) > 0 { cond = cond.And(builder.In("issue.id", opts.IssueIDs)) } - if opts.IsArchived != util.OptionalBoolNone { - relevantRepoIDs := []int64{} - r := []*Repository{} - s := x.Table("repository").Where(builder.Eq{"is_archived": opts.IsArchived.IsTrue()}) - s.Select("id").Find(&relevantRepoIDs) - s.Find(&r) - cond = cond.And(builder.In("issue.repo_id", relevantRepoIDs)) - } sess := func(cond builder.Cond) *xorm.Session { s := x.Where(cond) @@ -1538,6 +1530,10 @@ func GetUserIssueStats(opts UserIssueStatsOptions) (*IssueStats, error) { s.Join("INNER", "issue_label", "issue_label.issue_id = issue.id"). In("issue_label.label_id", opts.LabelIDs) } + if opts.IsArchived != util.OptionalBoolNone { + s.Join("INNER", "repository", "issue.repo_id = repository.id"). + And(builder.Eq{"repository.is_archived": opts.IsArchived.IsTrue()}) + } return s } From 29df59a2fd99ebab6964699919c3ed40d6a32020 Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 11 Jan 2021 12:57:37 +0100 Subject: [PATCH 22/23] Removed unused func --- models/org.go | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/models/org.go b/models/org.go index 1418751741fdd..f45c9af7a7338 100644 --- a/models/org.go +++ b/models/org.go @@ -737,7 +737,6 @@ func (org *User) GetUserTeams(userID int64) ([]*Team, error) { type AccessibleReposEnvironment interface { CountRepos() (int64, error) RepoIDs(page, pageSize int) ([]int64, error) - ActiveRepoIDs(page, pageSize int) ([]int64, error) Repos(page, pageSize int) ([]*Repository, error) MirrorRepos() ([]*Repository, error) AddKeyword(keyword string) @@ -845,24 +844,6 @@ func (env *accessibleReposEnv) RepoIDs(page, pageSize int) ([]int64, error) { Find(&repoIDs) } -func (env *accessibleReposEnv) ActiveRepoIDs(page, pageSize int) ([]int64, error) { - if page <= 0 { - page = 1 - } - - repoIDs := make([]int64, 0, pageSize) - return repoIDs, env.e. - Table("repository"). - Join("INNER", "team_repo", "`team_repo`.repo_id=`repository`.id"). - Where(env.cond()). - Where(builder.Eq{"is_archived": false}). - GroupBy("`repository`.id,`repository`."+strings.Fields(string(env.orderBy))[0]). - OrderBy(string(env.orderBy)). - Limit(pageSize, (page-1)*pageSize). - Cols("`repository`.id"). - Find(&repoIDs) -} - func (env *accessibleReposEnv) Repos(page, pageSize int) ([]*Repository, error) { repoIDs, err := env.RepoIDs(page, pageSize) if err != nil { From 3c6db2d0a3ebf0ea6e1937aada303bc9ec110fcf Mon Sep 17 00:00:00 2001 From: Elena Neuschild Date: Mon, 11 Jan 2021 15:49:28 +0100 Subject: [PATCH 23/23] Added grouping to avoid returning duplicate repo IDs --- models/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user.go b/models/user.go index cc9b6b8de3a99..de12b804fd3c9 100644 --- a/models/user.go +++ b/models/user.go @@ -517,7 +517,7 @@ func (u *User) GetActiveRepositoryIDs(units ...UnitType) ([]int64, error) { sess.Where(builder.Eq{"is_archived": false}) - return ids, sess.Where("owner_id = ?", u.ID).Find(&ids) + return ids, sess.Where("owner_id = ?", u.ID).GroupBy("repository.id").Find(&ids) } // GetOrgRepositoryIDs returns repositories IDs where user's team owned and has unittypes