Issues overview should not show issues from archived repos #13220

eneuschild · 2020-10-20T17:00:35Z

Context

I'd like to fix #13171.

As discussed there, I feel some refactoring is in order in the function user.Issues().
Following @zeripath's advice, I'm starting with some comments throughout the function.

Stylistic question

There are 4 routes that call the functionuser.Issues():

/issues
/pulls
/org/{orgID}/issues
/org/{orgID}/pulls

Since the function then painstakingly distinguishes between those four cases, wouldn't it be nicer to have four separate functions as entrypoints, that first do their respective things and then call a (new) function containing the code they have in common?

codecov-io · 2020-10-20T18:26:00Z

Codecov Report

Merging #13220 (2992f54) into master (74a0481) will decrease coverage by 0.00%.
The diff coverage is 53.53%.

@@            Coverage Diff             @@
##           master   #13220      +/-   ##
==========================================
- Coverage   41.79%   41.79%   -0.01%     
==========================================
  Files         744      744              
  Lines       79538    79620      +82     
==========================================
+ Hits        33246    33278      +32     
- Misses      40818    40860      +42     
- Partials     5474     5482       +8

Impacted Files	Coverage Δ
models/org.go	`70.58% <0.00%> (-1.93%)`	⬇️
routers/user/home.go	`59.70% <50.69%> (-0.30%)`	⬇️
models/user.go	`53.85% <75.86%> (-0.68%)`	⬇️
models/issue.go	`57.19% <100.00%> (+0.18%)`	⬆️
routers/routes/macaron.go	`93.22% <100.00%> (+0.02%)`	⬆️
modules/util/timer.go	`42.85% <0.00%> (-42.86%)`	⬇️
modules/secret/secret.go	`66.66% <0.00%> (-7.41%)`	⬇️
modules/indexer/stats/db.go	`56.00% <0.00%> (-4.00%)`	⬇️
models/repo_permission.go	`74.31% <0.00%> (-2.76%)`	⬇️
modules/git/repo_commit_nogogit.go	`63.33% <0.00%> (-1.67%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9659808...2992f54. Read the comment docs.

routers/user/home.go

.github/issue_template.md

models/migrations/v156.go

eneuschild · 2020-10-22T13:38:40Z

It seemed like a good idea to me to pull (--rebase) master into this branch, which I did.
Obviously, this is making things messy and was probably wrong.

If it was wrong, I'd rather throw this PR away and start a new one.

Sorry about this, I'm still rather new to github. :(

6543 · 2020-10-22T13:58:05Z

@eneuschild the best way to update a pull with changes from master is:

fetch/pull upstream-master to your local repo
merge master into your feature branch git merge --no-ff master

this way you dont have to force push <- witch make reviews inconvinient & you dont see all the commits from master here in this branch :)

6543 · 2020-10-22T13:59:17Z

@eneuschild if you already messed up ... the best thing is a rebase and force push then

fetch upstream master
git rebase master issue_13171
git push -f own issue_13171

6543 · 2020-10-22T14:01:13Z

@eneuschild the best way to update a pull with changes from master is:
1. fetch/pull upstream-master to your local repo
2. merge master into your feature branch git merge --no-ff master
...

the "Update Branch" button on github is doing the same thing - but dont forget to pull to your local repo afterwards before commiting new stuff :)

eneuschild

@eneuschild the best way to update a pull with changes from master is:

fetch/pull upstream-master to your local repo

merge master into your feature branch git merge --no-ff master
...

the "Update Branch" button on github is doing the same thing - but dont forget to pull to your local repo afterwards before commiting new stuff :)

Alrighty, thank you!

.github/issue_template.md

…rations closer to their usages

6543 · 2020-11-05T19:18:59Z

@eneuschild what's the state of this pull?

eneuschild · 2021-01-05T15:27:35Z

Meh, I just realized that I forgot to set my git user name and email when I moved to a new PC a few days ago and now my last few commits are under the name "Gitea". :(

Once again, sorry for the confusion.

models/issue.go

lunny · 2021-01-06T02:08:29Z

I think it's better to use join but not in and ids for the conditions if possible.

6543 · 2021-01-06T13:50:59Z

oh NO it got a conflict ...

eneuschild · 2021-01-06T14:08:21Z

oh NO it got a conflict ...

Neither the first nor the biggest one. :) I'll resolve it.

models/user.go

lunny · 2021-01-11T11:05:04Z

The IDs on old codes should be replaced by conditions because ids maybe very hug for a public site. And I would like the new code could avoid to do that again.

A condition with join is better than ids.

models/org.go

eneuschild · 2021-01-11T12:42:23Z

The IDs on old codes should be replaced by conditions because ids maybe very hug for a public site. And I would like the new code could avoid to do that again.

A condition with join is better than ids.

Do you mean the IDs in User.GetActiveRepositoryIDs()?
The resulting repo IDs are used for 3 different query Options for issue filtering and statistics, so I don't see how we can avoid using IDs here.

Or are you referring to AccessibleReposEnvironment.ActiveRepoIDs(), which I have now removed?

Can you please clarify what and where?

lunny · 2021-01-11T13:55:43Z

The IDs on old codes should be replaced by conditions because ids maybe very hug for a public site. And I would like the new code could avoid to do that again.
A condition with join is better than ids.

Do you mean the IDs in User.GetActiveRepositoryIDs()?
The resulting repo IDs are used for 3 different query Options for issue filtering and statistics, so I don't see how we can avoid using IDs here.

Or are you referring to AccessibleReposEnvironment.ActiveRepoIDs(), which I have now removed?

Can you please clarify what and where?

Yeah, I mean User.GetActiveRepositoryIDs. Assume one user have over 1000 repositories, the IN SQL will fail because the SQL is too big on some databases. AccessibleReposEnvironment.ActiveRepoIDs() has a pagination, so it will not be a problem.

eneuschild · 2021-01-11T14:45:41Z

The IDs on old codes should be replaced by conditions because ids maybe very hug for a public site. And I would like the new code could avoid to do that again.
A condition with join is better than ids.

Do you mean the IDs in User.GetActiveRepositoryIDs()?
The resulting repo IDs are used for 3 different query Options for issue filtering and statistics, so I don't see how we can avoid using IDs here.

Yeah, I mean User.GetActiveRepositoryIDs. Assume one user have over 1000 repositories, the IN SQL will fail because the SQL is too big on some databases.

This IN?

It contains UnitTypes, not repo IDs, so at worst this becomes

WHERE repo_unit.type IN (1,2,3,4,5,6,7,8)

techknowlogick · 2021-01-13T03:11:43Z

🚀

* master: (252 commits) Issues overview should not show issues from archived repos (go-gitea#13220) Display SVG files as images instead of text (go-gitea#14101) [skip ci] Updated translations via Crowdin Update docs to clarify issues raised in go-gitea#14272 (go-gitea#14318) [skip ci] Updated translations via Crowdin [Refactor] Passwort Hash/Set (go-gitea#14282) Add option to change username to the admin panel (go-gitea#14229) fix mailIssueCommentBatch for pull request (go-gitea#14252) Remove self from MAINTAINERS (go-gitea#14286) Do not reload page after adding comments in Pull Request reviews (go-gitea#13877) Fix session bug when introduce chi (go-gitea#14287) [skip ci] Updated translations via Crowdin Add secure/httpOnly attributes to the lang cookie (go-gitea#9690) (go-gitea#14279) Some code improvements (go-gitea#14266) [skip ci] Updated translations via Crowdin Fix wrong type on hooktask to convert typ from char(16) to varchar(16) (go-gitea#14148) Upgrade XORM links in documentation. (go-gitea#14265) Check permission for the appropriate unit type (go-gitea#14261) Add compliance check for windows to ensure cross platform build (go-gitea#14260) [skip ci] Updated translations via Crowdin ...

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2020

zeripath reviewed Oct 21, 2020

View reviewed changes

routers/user/home.go Outdated Show resolved Hide resolved

6543 reviewed Oct 22, 2020

View reviewed changes

.github/issue_template.md Outdated Show resolved Hide resolved

6543 reviewed Oct 22, 2020

View reviewed changes

models/migrations/v156.go Outdated Show resolved Hide resolved

eneuschild commented Oct 22, 2020

View reviewed changes

.github/issue_template.md Outdated Show resolved Hide resolved

eneuschild added 5 commits October 22, 2020 17:45

Add lots of comments to user.Issues()

91f207d

Answered some questions from comments

89bb53d

fix typo in comment

c2f7acc

Refac user.Issues(): add func repoIDs

19221f0

Refac user.Issues(): add func userRepoIDs

355a90f

eneuschild force-pushed the issue_13171 branch from b9f1d44 to 355a90f Compare October 22, 2020 15:48

eneuschild added 13 commits October 22, 2020 19:23

Refac user.Issues(): add func issueIDsFromSearch

b2e6d26

Merge branch 'master' into issue_13171

628c8b1

Refac user.Issues(): improve error handling

9673406

Refac user.Issues(): add inline documentation and move variable decla…

dbc22f7

…rations closer to their usages

Refac user.Issues(): add func repoIDMap

fdb2237

Refac user.Issues(): cleanup

5e8d323

Merge branch 'master' into issue_13171

2317cac

Refac: Separate Issues from Pulls during routing

0294079

fix typo in comment

afb9af7

Merge branch 'master' into issue_13171

4ff651f

Merge branch 'master' into issue_13171

4cd5f17

Merge branch 'master' into issue_13171

8359624

Merge branch 'master' into issue_13171

7a0d75d

lunny requested changes Jan 6, 2021

View reviewed changes

models/issue.go Outdated Show resolved Hide resolved

Improved IsArchived filtering in issue.GetUserIssueStats

9de7cb4

eneuschild added 4 commits January 6, 2021 15:37

Merge branch 'master' into issue_13171

ec6412c

Merge branch 'master' into issue_13171

7019096

Merge branch 'master' into issue_13171

4b1d4a8

Merge branch 'master' into issue_13171

2992f54

eneuschild requested a review from lunny January 11, 2021 10:38

lunny reviewed Jan 11, 2021

View reviewed changes

models/user.go Show resolved Hide resolved

lunny reviewed Jan 11, 2021

View reviewed changes

models/org.go Outdated Show resolved Hide resolved

Removed unused func

29df59a

eneuschild requested a review from lunny January 11, 2021 12:42

Added grouping to avoid returning duplicate repo IDs

3c6db2d

lunny approved these changes Jan 12, 2021

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 12, 2021

Merge branch 'master' into issue_13171

a24e2c7

Merge branch 'master' into issue_13171

4201024

techknowlogick merged commit 5640303 into go-gitea:master Jan 13, 2021

go-gitea locked and limited conversation to collaborators Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issues overview should not show issues from archived repos #13220

Issues overview should not show issues from archived repos #13220

eneuschild commented Oct 20, 2020

codecov-io commented Oct 20, 2020 •

edited

Loading

eneuschild commented Oct 22, 2020 •

edited

Loading

6543 commented Oct 22, 2020

6543 commented Oct 22, 2020

6543 commented Oct 22, 2020

eneuschild left a comment

6543 commented Nov 5, 2020

eneuschild commented Jan 5, 2021

lunny commented Jan 6, 2021

6543 commented Jan 6, 2021

eneuschild commented Jan 6, 2021

lunny commented Jan 11, 2021 •

edited

Loading

eneuschild commented Jan 11, 2021

lunny commented Jan 11, 2021

eneuschild commented Jan 11, 2021

techknowlogick commented Jan 13, 2021

Issues overview should not show issues from archived repos #13220

Issues overview should not show issues from archived repos #13220

Conversation

eneuschild commented Oct 20, 2020

codecov-io commented Oct 20, 2020 • edited Loading

Codecov Report

eneuschild commented Oct 22, 2020 • edited Loading

6543 commented Oct 22, 2020

6543 commented Oct 22, 2020

6543 commented Oct 22, 2020

eneuschild left a comment

Choose a reason for hiding this comment

6543 commented Nov 5, 2020

eneuschild commented Jan 5, 2021

lunny commented Jan 6, 2021

6543 commented Jan 6, 2021

eneuschild commented Jan 6, 2021

lunny commented Jan 11, 2021 • edited Loading

eneuschild commented Jan 11, 2021

lunny commented Jan 11, 2021

eneuschild commented Jan 11, 2021

techknowlogick commented Jan 13, 2021

codecov-io commented Oct 20, 2020 •

edited

Loading

eneuschild commented Oct 22, 2020 •

edited

Loading

lunny commented Jan 11, 2021 •

edited

Loading