Skip to content

feat: add organization_ids in the user(s) response #1184

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 28, 2022
Merged

feat: add organization_ids in the user(s) response #1184

merged 8 commits into from
Apr 28, 2022

Conversation

BrunoQuaresma
Copy link
Collaborator

Reason:
FE needs to access the user org id to perform some actions like creating a new user in the same org. You can see the full conversation here https://codercom.slack.com/archives/C01A9SEKFEE/p1650907824531999.

Possible solutions

  • Make it on the endpoint side doing extra database requests
  • Update the user queries to return the organization_ids directly

@BrunoQuaresma BrunoQuaresma requested a review from a team April 26, 2022 17:11
@BrunoQuaresma BrunoQuaresma self-assigned this Apr 26, 2022
@BrunoQuaresma BrunoQuaresma requested a review from a team as a code owner April 26, 2022 17:13
@codecov
Copy link

codecov bot commented Apr 26, 2022 •
edited
Loading

Codecov Report

Merging #1184 (e088a0e) into main (8661f92) will decrease coverage by 0.25%.
The diff coverage is 51.16%.

@@            Coverage Diff             @@
##             main    #1184      +/-   ##
==========================================
- Coverage   66.27%   66.01%   -0.26%     
==========================================
  Files         265      265              
  Lines       16681    16751      +70     
  Branches      157      157              
==========================================
+ Hits        11055    11059       +4     
- Misses       4484     4532      +48     
- Partials     1142     1160      +18
Flag Coverage Δ
unittest-go-macos-latest 53.54% <41.86%> (-0.01%) ⬇️
unittest-go-postgres- 65.32% <47.67%> (-0.18%) ⬇️
unittest-go-ubuntu-latest 55.96% <41.86%> (-0.18%) ⬇️
unittest-go-windows-2022 52.97% <41.86%> (-0.16%) ⬇️
unittest-js 66.50% <ø> (ø)
Impacted Files Coverage Δ
codersdk/users.go 64.20% <ø> (ø)
coderd/database/queries.sql.go 81.12% <40.00%> (-0.54%) ⬇️
coderd/users.go 61.27% <54.54%> (-2.01%) ⬇️
cli/cliui/agent.go 77.19% <0.00%> (-5.27%) ⬇️
provisioner/echo/serve.go 54.40% <0.00%> (-4.81%) ⬇️
peerbroker/proxy.go 58.13% <0.00%> (-4.07%) ⬇️
provisionerd/provisionerd.go 76.84% <0.00%> (-1.34%) ⬇️
coderd/provisionerdaemons.go 62.79% <0.00%> (-0.51%) ⬇️
provisioner/terraform/provision.go 70.96% <0.00%> (-0.44%) ⬇️
... and 1 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 8661f92...e088a0e. Read the comment docs.

BrunoQuaresma
BrunoQuaresma commented Apr 26, 2022
View reviewed changes
kylecarbs
kylecarbs requested changes Apr 26, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

We should probably cover the case of fetching multiple users before merging this. Otherwise we'll have a struct that our API only serves on some routes.

@BrunoQuaresma
Copy link
Collaborator Author

@kylecarbs this PR is covering the scenario of fetching multiple users. https://github.com/coder/coder/pull/1184/files#diff-b4e10e23e473674139719f634b3ad7e760467574e342916c4bbb42df7a613722R151 this is the GET /users endpoint. Or are you talking about something different?

Emyrk
Emyrk reviewed Apr 26, 2022
View reviewed changes
@presleyp presleyp mentioned this pull request Apr 28, 2022
Merged
kylecarbs
kylecarbs approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

LGTM. I'm happy this just adds a single database call!

coderd/database/queries/organizationmembers.sql

-- name: GetOrganizationIDsByMemberIDs :many
SELECT
user_id, array_agg(organization_id) :: uuid [ ] AS "organization_IDs"
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove AS "organization_IDs"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately not because sqlc is generating the field name as "organization_ids" not forcing the "ID" uppercase stuff 😕

@BrunoQuaresma BrunoQuaresma merged commit 816441e into main Apr 28, 2022
@BrunoQuaresma BrunoQuaresma deleted the bq/add-org-id-to-the-user-response branch April 28, 2022 14:10
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
@BrunoQuaresma @kylecarbs
feat: add organization_ids in the user(s) response (#1184)
5581324
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@presleyp presleyp presleyp left review comments

@kylecarbs kylecarbs kylecarbs approved these changes

@Emyrk Emyrk Awaiting requested review from Emyrk

Assignees

@BrunoQuaresma BrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
V2 Beta
Development

Successfully merging this pull request may close these issues.
5 participants
@BrunoQuaresma @presleyp @Emyrk @kylecarbs @misskniss