Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue overview for teams #19652

Merged
merged 10 commits into from
May 16, 2022
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented May 8, 2022

- Don't use hacky solution to limit to the correct RepoID's, instead use
current code to handle these limits. The existing code is more correct
than the hacky solution.
- Resolves go-gitea#19636
@Gusted Gusted modified the milestones: 1.x.x, 1.17.0 May 8, 2022
@Gusted Gusted added issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels May 8, 2022
models/issue.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 8, 2022
Gusted added a commit to Gusted/gitea that referenced this pull request May 8, 2022
- Backport go-gitea#19652
  - Don't use hacky solution to limit to the correct RepoID's, instead use current code to handle these limits. The existing code is more correct than the hacky solution.
  - Resolves go-gitea#19636
@Gusted Gusted added the backport/done All backports for this PR have been created label May 8, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@5ca224a). Click here to learn what that means.
The diff coverage is 20.68%.

@@           Coverage Diff           @@
##             main   #19652   +/-   ##
=======================================
  Coverage        ?   47.35%           
=======================================
  Files           ?      957           
  Lines           ?   133318           
  Branches        ?        0           
=======================================
  Hits            ?    63132           
  Misses          ?    62544           
  Partials        ?     7642           
Impacted Files Coverage Δ
models/issue.go 52.73% <18.51%> (ø)
routers/web/user/home.go 65.10% <50.00%> (ø)

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 5ca224a...8a05ad7. Read the comment docs.

models/issue.go Outdated Show resolved Hide resolved
@zeripath
Copy link
Contributor

zeripath commented May 8, 2022

There's quite a few IN SELECTs in there and whilst the SQL is correct I'm wondering if we might want to use a few JOINS instead of chains of IN SELECTs - however perhaps we should consider that for another PR.

@Gusted
Copy link
Contributor Author

Gusted commented May 8, 2022

There's quite a few IN SELECTs in there and whilst the SQL is correct I'm wondering if we might want to use a few JOINS instead of chains of IN SELECTs - however perhaps we should consider that for another PR.

Feel free to do it. This is a plain copy-paste of the SQL that is otherwise used by the function(IsOwnedBy). I already found it a mystery that it worked first-try.

@lunny
Copy link
Member

lunny commented May 9, 2022

There's quite a few IN SELECTs in there and whilst the SQL is correct I'm wondering if we might want to use a few JOINS instead of chains of IN SELECTs - however perhaps we should consider that for another PR.

A subquery maybe faster than join in modern databases.

@lunny
Copy link
Member

lunny commented May 9, 2022

Could we have a test which could reproduce #19636 ? So that we can prevent to repeat the problem.

@Gusted
Copy link
Contributor Author

Gusted commented May 10, 2022

Added it: 50e61af

Quite annoying to debug the SQL statements to only realize that rows on the test database is set to NULL which conflicts with a real-world-data database.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good. If I understand correctly, the major change between this one and the code before the last PR is : Check if the user is in the owner team of the organisation., and fixes the bug caused by opts.RepoCond = models.SearchRepositoryCondition(repoOpts)

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 15, 2022
@Gusted
Copy link
Contributor Author

Gusted commented May 15, 2022

the major change between this one and the code before the last PR is : Check if the user is in the owner team of the organisation.,

Yes, this caused my previous PR to be incorrect, as I thought this was already implemented so I adjusted the code until it had that result(but was actually incorrect).

and fixes the bug caused by opts.RepoCond = models.SearchRepositoryCondition(repoOpts)

Yup, by using the already supported TeamID option.

@GiteaBot 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 May 15, 2022
wxiaoguang pushed a commit that referenced this pull request May 16, 2022
- Backport #19652
  - Don't use hacky solution to limit to the correct RepoID's, instead use current code to handle these limits. The existing code is more correct than the hacky solution.
  - Resolves #19636
@wxiaoguang wxiaoguang merged commit 71ca131 into go-gitea:main May 16, 2022
@Gusted Gusted deleted the fix-team-issue-overview branch May 16, 2022 09:50
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 16, 2022
* giteaofficial/main:
  Fix issue overview for teams (go-gitea#19652)
  Fix nodeinfo caching and prevent NPE if cache non-existent (go-gitea#19721)
  Update go-chi/cache to utilize Ping() (go-gitea#19719)
  Disable blank issues (go-gitea#19717)
  clarify what session provider 'db' does (go-gitea#19713)
  [skip ci] Updated translations via Crowdin
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
- Don't use hacky solution to limit to the correct RepoID's, instead use
current code to handle these limits. The existing code is more correct
than the hacky solution.
- Resolves go-gitea#19636
- Add test-case
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created issue/regression Issue needs no code to be fixed, only a description on how to fix it yourself lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue totals are wrong when filtering by team
7 participants