Skip to content

chore: add /groups endpoint to filter by organization and/or member #14260

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

Merged
merged 13 commits into from
Aug 15, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Aug 13, 2024

Closes #14249

@Emyrk Emyrk changed the title chore: merge get groups sql queries into 1 chore: add /groups endpoint to filter by organization and/or member Aug 13, 2024
@Emyrk Emyrk force-pushed the stevenmasley/remove_extra_groups_query branch from 813a4da to 190bd90 Compare August 13, 2024 16:29
@Emyrk Emyrk marked this pull request as ready for review August 13, 2024 17:34
Copy link

alwaysmeticulous bot commented Aug 13, 2024

✅ Meticulous spotted zero visual differences across 1412 screens tested: view results.

Expected differences? Click here. Last updated for commit ee3c8e4. This comment will update as new commits are pushed.

@Emyrk Emyrk requested a review from f0ssel August 13, 2024 19:21
Comment on lines +73 to +81
source
)
SELECT
gen_random_uuid(),
group_name,
@organization_id,
@source
gen_random_uuid(),
group_name,
@organization_id,
@source
FROM
UNNEST(@group_names :: text[]) AS group_name
UNNEST(@group_names :: text[]) AS group_name
Copy link
Contributor

Choose a reason for hiding this comment

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

weird spacing here

Copy link
Member Author

Choose a reason for hiding this comment

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

SQL spacing is so annoying. I wish there was some nice formatter that just worked.

// UUIDorName will parse a string as a UUID, if it fails, it uses the "fetchByName"
// function to return a UUID based on the value as a string.
// This is useful when fetching something like an organization by ID or by name.
func (p *QueryParamParser) UUIDorName(vals url.Values, def uuid.UUID, queryParam string, fetchByName func(name string) (uuid.UUID, error)) uuid.UUID {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: having the query param 3rd here feels off to me personally, I'd expect the order values, query param, uuid, fetchByName.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see that, unfortunately the other functions all do default, queryParam so it felt odd to switch the order of those two.

I agree with you though, I think all the functions could be reordered, but don't want to do that refactor here.

@Emyrk Emyrk merged commit 7b09d98 into main Aug 15, 2024
33 checks passed
@Emyrk Emyrk deleted the stevenmasley/remove_extra_groups_query branch August 15, 2024 18:40
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement endpoint to list all organizations memberships and group memberships
2 participants