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.

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

Rewrite GET /users to use GROUP_CONCAT for depots and roles #4136

Closed
jniles opened this issue Jan 27, 2020 · 1 comment · Fixed by #4165
Closed

Rewrite GET /users to use GROUP_CONCAT for depots and roles #4136

jniles opened this issue Jan 27, 2020 · 1 comment · Fixed by #4165
Assignees
@jniles
Labels
Best Practice in progress performance

Comments

@jniles
Copy link
Collaborator

jniles commented Jan 27, 2020

The /users route currently does and SQL JOIN in javascript, and involves rather complicated code to list the depots and roles for each user. It would be good to simply JOIN, then do a GROUP_CONCAT() to assemble a list of depots/roles assigned to each user.

Whoever tackles this should look to the feeCenter SQL for inspiration about how to accomplish this task.
@jniles
Copy link
Collaborator Author

jniles commented Feb 13, 2020

@kwilu claim

@jniles jniles mentioned this issue Feb 13, 2020
Merged
bors bot added a commit that referenced this issue Feb 13, 2020
@bors @jniles
Merge #4165
84b22fe 
4165: perf(users): use GROUP_CONCAT for roles/depots r=jniles a=jniles

Uses GROUP_CONCAT() to ensure that roles and depots are assigned to users instead of using JavaScript Objects.  This is also helpful because we can order the values in alphabetical order.

Closes #4136

Co-authored-by: Jonathan Niles <jonathanwniles@gmail.com>
@bors bors bot closed this as completed in 24f3406 Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jniles jniles

Labels
Best Practice in progress performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

perf(users): use GROUP_CONCAT for roles/depots jniles/bhima
2 participants
@jniles @kwilu