Issues overview should not show issues from archived repos (#13220)

eneuschild · 6543 · Gitea · web-flow · commit 564030336dbf · 2021-01-12T23:19:17.000-05:00
* 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 &lt;6543@obermui.de&gt;

* 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 &lt;6543@obermui.de&gt;
Co-authored-by: Gitea &lt;gitea@fake.local&gt;
Co-authored-by: techknowlogick &lt;techknowlogick@gitea.io&gt;
diff --git a/integrations/api_issue_test.go b/integrations/api_issue_test.go
@@ -186,15 +186,15 @@ 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")
 	link.RawQuery = query.Encode()
 	req = NewRequest(t, "GET", link.String())
 	resp = session.MakeRequest(t, req, http.StatusOK)
 	DecodeJSON(t, resp, &apiIssues)
-	assert.Len(t, apiIssues, 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -693,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
diff --git 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/issue.go b/models/issue.go
@@ -1104,6 +1104,7 @@ type IssuesOptions struct {
 	UpdatedBeforeUnix  int64
 	// prioritize issues from this repo
 	PriorityRepoID int64
+	IsArchived     util.OptionalBool
 }
 
 // sortIssuesSession sort an issues-related session based on the provided
@@ -1207,6 +1208,10 @@ func (opts *IssuesOptions) setupSession(sess *xorm.Session) {
 		sess.And("issue.is_pull=?", 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 {
 		for i, labelID := range opts.LabelIDs {
 			if labelID > 0 {
@@ -1501,6 +1506,7 @@ type UserIssueStatsOptions struct {
 	IsPull      bool
 	IsClosed    bool
 	IssueIDs    []int64
+	IsArchived  util.OptionalBool
 	LabelIDs    []int64
 }
 
@@ -1524,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
 	}
 
diff --git a/models/org.go b/models/org.go
@@ -753,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)
diff --git 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.go b/models/user.go
@@ -503,6 +503,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(builder.Eq{"is_archived": false})
+
+	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
 // Caller shall check that units is not globally disabled
 func (u *User) GetOrgRepositoryIDs(units ...UnitType) ([]int64, error) {
@@ -524,6 +541,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(builder.Eq{"is_archived": false}).
+		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) {
@@ -538,6 +577,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/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/routes/macaron.go b/routers/routes/macaron.go
@@ -199,7 +199,8 @@ func RegisterMacaronRoutes(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 *****
@@ -447,8 +448,10 @@ func RegisterMacaronRoutes(m *macaron.Macaron) {
 		m.Group("/:org", func() {
 			m.Get("/dashboard", user.Dashboard)
 			m.Get("/dashboard/:team", user.Dashboard)
-			m.Get("/^:type(issues|pulls)$", user.Issues)
-			m.Get("/^:type(issues|pulls)$/:team", user.Issues)
+			m.Get("/issues", user.Issues)
+			m.Get("/issues/:team", user.Issues)
+			m.Get("/pulls", user.Pulls)
+			m.Get("/pulls/:team", user.Pulls)
 			m.Get("/milestones", reqMilestonesDashboardPageEnabled, user.Milestones)
 			m.Get("/milestones/:team", reqMilestonesDashboardPageEnabled, user.Milestones)
 			m.Get("/members", org.Members)
diff --git a/routers/user/home.go b/routers/user/home.go
diff --git a/routers/user/home_test.go b/routers/user/home_test.go

Original file line number	Diff line number	Diff line change
`@@ -753,7 +753,7 @@ type accessibleReposEnv struct {`
`753`	`753`	`orderBy SearchOrderBy`
`754`	`754`	`}`
`755`	`755`
`756`		-// AccessibleReposEnv an AccessibleReposEnvironment for the repositories in `org`
	`756`	+// AccessibleReposEnv builds an AccessibleReposEnvironment for the repositories in `org`
`757`	`757`	`// that are accessible to the specified user.`
`758`	`758`	`func (org *User) AccessibleReposEnv(userID int64) (AccessibleReposEnvironment, error) {`
`759`	`759`	`return org.accessibleReposEnv(x, userID)`