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

Clean up Organization.(viewerTeams, allTeams, publicTeams) #9917

Open
3 tasks
mattkrick opened this issue Jul 1, 2024 · 5 comments
Open
3 tasks

Clean up Organization.(viewerTeams, allTeams, publicTeams) #9917

mattkrick opened this issue Jul 1, 2024 · 5 comments

Comments

@mattkrick
Copy link
Member

mattkrick commented Jul 1, 2024

I noticed about 8 months ago we got rid of the resolver for Organization.teams & more recently we we added some new fields that can get queried. I hit a bug because Organization.teams is valid GQL, but teams doesn't have a resolver!

My tension here with having viewerTeams, allTeams, publicTeams is that these 3 lists can get out of sync with each other. E.g. if a team gets added, the client has to be sure to fetch that team under all 3 lists or it will be missing from 1 list but not the others! Less importantly, it triples the amount of data sent because if I have 100 teams, then I'll have to send data for300 teams, 100 for each list.

Instead, since all 3 return a list of teams that the viewer can access, I propose we re-instate the Organization.teams resolver & get rid of these 3. Then, to determine if it's a public team that the user isn't on, we just check Team.isViewerOnTeam.

@nickoferrall since you worked on this recently I wanted to get your thoughts on this direction

AC

  • viewerTeams, allTeams, publicTeams are removed from the GraphQL API
  • teams is re-added to the GraphQL API
  • filtering is performed client-side based on if the viewer in on that team
@mattkrick mattkrick added this to Product Jul 1, 2024
@github-project-automation github-project-automation bot moved this to To triage in Product Jul 1, 2024
@nickoferrall
Copy link
Contributor

nickoferrall commented Jul 3, 2024

If we fetch all teams with Organization.teams and filter on the client, would it be a problem that users would have access to teams they don't belong to?

So a user that doesn't have the public teams flag and isn't a super user, org admin, or billing lead could see info about "Execs Top Secret Team".

That was the scenario I was trying to avoid with the different endpoints, but I do agree that it's now a bit messy with 3 lists.

Maybe we could re-instate Organization.teams, but add args that filter the lists? So teams could take an allTeams and viewerTeams arg, check on the server whether the user has the correct permission, then return the list?

We could also release public teams to everyone which would make the codebase cleaner. We've tested it for a while now without any push back.

@Dschoordsch
Copy link
Contributor

Dschoordsch commented Jul 3, 2024

Organization.teams should return all teams the viewer is allowed to see. That is:

  • their member teams if they don't have any additional rights
  • all public teams if the feature flag is set
  • all teams if they are org admin

If we add an argument to the teams query we run into the same update issue I think Matt wants to avoid. If the query has different arguments, it needs to be updated separately in Relay as far as I know.

@nickoferrall
Copy link
Contributor

OK cool, yeah, I think that'll work!

Copy link
Contributor

Stale issue

@jordanh
Copy link
Contributor

jordanh commented Jan 8, 2025

Going to keep this one open as we've all engaged in forming a solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready
Development

No branches or pull requests

4 participants