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

Calculate PublicOnly for org membership only once #32234

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

6543
Copy link
Member

@6543 6543 commented Oct 10, 2024

Refactoring of #32211

this move the PublicOnly() filter calcuation next to the DB querys and let it be decided by the Doer


Sponsored by Kithara Software GmbH

@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Oct 10, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 10, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 10, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Oct 10, 2024
@delvh delvh changed the title Make calculation of PublicOnly filter for OrgMembership in single place Calculate PublicOnly for org membership only once Oct 10, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 11, 2024
@6543 6543 marked this pull request as ready for review October 11, 2024 17:31
@6543 6543 requested a review from delvh October 11, 2024 17:31
@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 Oct 12, 2024
Where("`team_user`.org_id = ?", org.ID).
Join("INNER", "team_user", "`team_user`.team_id = team.id").
And("`team_user`.uid = ?", userID).
Where(builder.In("team.id", userTeamIDbuilder(org.ID, userID))).
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's unnecessary select id from team where id in (select id where ....

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but DBMS should optimize it out ... @lunny if you know how to have the function creating the builder query, to be used, while remove one select ... just tell me :)

@lunny
Copy link
Member

lunny commented Oct 13, 2024

As a habit, we almost check permission in service layer but not in models layer. What's the problem with the previous code?

@6543
Copy link
Member Author

6543 commented Oct 13, 2024

I can create a new func in the service witch checks permissions ... but we would have to alter the existing code a lot and i want to only touch it here for a explizite context ... : the refactoring that makes the #32211 diff smal

@6543
Copy link
Member Author

6543 commented Oct 13, 2024

What's the problem with the previous code?

the determination if a user can see public or private org membership is determined in different places ... I want to have that check in a single place ...

6543 and others added 3 commits October 14, 2024 17:53
@6543
Copy link
Member Author

6543 commented Oct 14, 2024

@lunny to #32234 (comment) i tryed: 3af5ae7

see https://github.com/go-gitea/gitea/actions/runs/11331009974/job/31510076461?pr=32234

it does not work, it errors with: Unsupported condition type

@6543
Copy link
Member Author

6543 commented Oct 14, 2024

(I tried to have a single source of truth for the query ... but if preferred i will just use duplicated code by have a builder version and classic xorm version)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants