NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

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.

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

Already on GitHub?Sign in to your account

Jump to bottom

Merged

BrunoQuaresma merged 8 commits intomainfrombq/add-org-id-to-the-user-response

Apr 28, 2022

Merged

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

BrunoQuaresma merged 8 commits intomainfrombq/add-org-id-to-the-user-response

Apr 28, 2022

Conversation

Copy link

Collaborator

BrunoQuaresma commentedApr 26, 2022

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 herehttps://codercom.slack.com/archives/C01A9SEKFEE/p1650907824531999.

Possible solutions

Make it on the endpoint side doing extra database requests
Update the user queries to return theorganization_ids directly
- To make that happen we would have to use subqueries or joins which unfortunately are not well supported bysqlc at this moment. You can read more details here:feat: Add embedded structs based on result match sqlc-dev/sqlc#984

feat: add organization_ids in the user(s) response

494aca2

BrunoQuaresma requested a review froma team

April 26, 2022 17:11

BrunoQuaresma self-assigned this

Apr 26, 2022

fix: update user endpoint types

0cfa1cb

BrunoQuaresma requested a review froma team as acode owner

April 26, 2022 17:13

Copy link

codecovbot commentedApr 26, 2022•
edited
Loading

Codecov Report

Merging#1184 (e088a0e) intomain (8661f92) willdecrease coverage by0.25%.
The diff coverage is51.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%)`	⬇️
... and1 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update8661f92...e088a0e. Read thecomment docs.

BrunoQuaresma commented

Apr 26, 2022

View reviewed changes

codersdk/users.go OutdatedShow resolvedHide resolved

kylecarbs requested changes

Apr 26, 2022

View reviewed changes

Copy link

Member

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 onsome routes.