feat: add count endpoint for users, enabling better pagination #4848
Conversation
presleyp
requested review from
Kira-Pilot,
f0ssel and
coadler
and removed request for
a team
|
The active users test is failing on CI but passing locally (I remembered |
| @@ -240,27 +240,35 @@ describe("UsersPage", () => { | |||
|
|
|||
| describe("pagination", () => { | |||
| it("goes to next and previous page", async () => { | |||
| renderPage() | |||
| const { container } = renderPage() | |||
There was a problem hiding this comment.
I love that we have tests and I'm happy to keep these in there. I'm curious if we should just add tests for the pagination widget itself, rather than tests for each time it's implemented.
There was a problem hiding this comment.
Yeah, I added a bit to the widget in a previous PR but I think there's also value in making sure the page is using the widget properly
| @@ -60,7 +61,7 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { | |||
| // - users are loading or | |||
| // - the user can edit the users but the roles are loading | |||
| const isLoading = | |||
| usersState.matches("gettingUsers") || | |||
| usersState.matches("users.gettingUsers") || | |||
There was a problem hiding this comment.
Was this just not working before? How come we have to add users now?
There was a problem hiding this comment.
I put all the existing states inside a "users" state node (submachine) so that the count-related states can run in parallel to them
| @@ -57,6 +57,8 @@ export interface UsersContext { | |||
| updateUserRolesError?: Error | unknown | |||
| paginationContext: PaginationContext | |||
| paginationRef: PaginationMachineRef | |||
| count: number | |||
| getCountError: Error | unknown | |||
There was a problem hiding this comment.
I see we aren't using this anywhere. To reduce complexity, shall we leave it out?
If we don't want to leave it out, should we at least console.error the count error so we can fix it if we come across it?
There was a problem hiding this comment.
thank you, I meant to make sure all the errors were handled! though it might be okay for this to fail silently...maybe a console error is the best option, what do you think?
There was a problem hiding this comment.
Yeah, I completely agree! No need to warn the user but a console.error for us devs might be nice.
| @@ -245,6 +245,7 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) { | |||
| // Endpoints that use the SQLQuery filter. | |||
| "GET:/api/v2/workspaces/": {StatusCode: http.StatusOK, NoAuthorize: true}, | |||
| "GET:/api/v2/workspaces/count": {StatusCode: http.StatusOK, NoAuthorize: true}, | |||
| "GET:/api/v2/users/count": {StatusCode: http.StatusOK, NoAuthorize: true}, | |||
There was a problem hiding this comment.
not doing an auth check here, combined with the fact that you can query for username or email substrings means that you can easily dump all usernames and emails by repeatedly hitting this endpoint and doing a tree search.
There was a problem hiding this comment.
To follow up, you can follow how we do this for workspaces.
coder/coderd/database/modelqueries.go
Lines 113 to 117 in 6f8ec23
If you add api.Authorize(r, rbac.ActionRead, rbac.ResourceUser) it is technically ok as it checks if you can read all users. We should really do it proper though so we can mess with perms later and everything still works.
Add a new function called GetAuthorizedUsersCount
Here's the last page, showing that we know how many pages there are and that the "next page" button is disabled:
