feat: support created_at filter for the GET /users endpoint #15633

joobisb · 2024-11-22T19:02:13Z

We support these filters currently: https://coder.com/docs/v2/latest/admin/users#user-filtering, adding created_at filter as well.

joobisb · 2024-11-22T19:03:47Z

@ericpaulsen @matifali could you please take a look at this?

ethanndickson · 2024-11-25T06:00:31Z

Thanks for the PR!

Whilst this technically addresses the linked issue, I'm not sure how useful it is if it doesn't support date ranges. I think it'd make more sense to implement it in terms of created_at_before and created_at_after, as we do for last_seen_*. You could obviously then use a combination of the two to retrieve users created on a specific day.

Although not mentioned in the issue, we'll also want to support this filter on the frontend (a separate PR for that is fine), on the deployment/users page.

joobisb · 2024-11-28T14:42:51Z

Thanks for the PR!

Whilst this technically addresses the linked issue, I'm not sure how useful it is if it doesn't support date ranges. I think it'd make more sense to implement it in terms of created_at_before and created_at_after, as we do for last_seen_*. You could obviously then use a combination of the two to retrieve users created on a specific day.

Although not mentioned in the issue, we'll also want to support this filter on the frontend (a separate PR for that is fine), on the deployment/users page.

@ethanndickson sure! will update the PR

joobisb · 2024-12-04T20:21:28Z

Thanks for the PR!

Whilst this technically addresses the linked issue, I'm not sure how useful it is if it doesn't support date ranges. I think it'd make more sense to implement it in terms of created_at_before and created_at_after, as we do for last_seen_*. You could obviously then use a combination of the two to retrieve users created on a specific day.

Although not mentioned in the issue, we'll also want to support this filter on the frontend (a separate PR for that is fine), on the deployment/users page.

@ethanndickson I've updated the PR to support date ranges. Also I don't think we need to make any changes in the frontend as the params are directly passed on to the backend and is working fine as shown below.

Please do have a look at the PR.

ethanndickson

LGTM, just one comment on naming, sorry for misleading you there!

ethanndickson · 2024-12-05T10:00:47Z

docs/admin/users/index.md

  platform (e.g. logging in, any API requests, connecting to workspaces). Uses
  the RFC3339Nano format.
+- `created_at_before` and `created_at_after` - The time a user was created. Uses


I don't think created_at_{after|before} reads very well. I didn't really think about the phrasing when I wrote it earlier. Let's just go with created_before and created_after.

@ethanndickson

updated the PR!

ethanndickson · 2024-12-05T14:08:38Z

coderd/users_test.go

+			Name: "CreatedAtRange",
+			Filter: codersdk.UsersRequest{
+				SearchQuery: `created_after:"2023-01-01T00:00:00Z" created_before:"2023-01-31T23:59:59Z"`,
+			},


All the users created in the setup of this test have the same creation date. We should add some users created within the range, and before the CreatedBefore testcase timestamp. Right now those testcases are just comparing empty slices.

@ethanndickson

I've updated the PR with the tests, please do have a look

joobisb · 2024-12-10T11:52:40Z

coderd/users_test.go

-				Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
-			},
+			Name:   "All",
+			Filter: codersdk.UsersRequest{},


updated this since "All" filter should include all the users

Given this is done twice, I assume this was intentional. Just a sanity check that all users are either suspended or active. I think we should just keep the original.

joobisb · 2024-12-10T11:53:34Z

coderd/users_test.go

@@ -1602,7 +1639,7 @@ func TestUsersFilter(t *testing.T) {
 				Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
 			},
 			FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
-				return true
+				return u.Status == codersdk.UserStatusSuspended || u.Status == codersdk.UserStatusActive


In the request, status is filtered with UserStatusSuspended and UserStatusActive, handling the same here

Same here, let's keep the original as it was almost certainly purposeful.

ethanndickson

Few comments then I'm happy to approve!

ethanndickson · 2024-12-10T12:55:46Z

coderd/users_test.go

-				Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
-			},
+			Name:   "All",
+			Filter: codersdk.UsersRequest{},


Given this is done twice, I assume this was intentional. Just a sanity check that all users are either suspended or active. I think we should just keep the original.

ethanndickson · 2024-12-10T12:57:36Z

coderd/users_test.go

@@ -1602,7 +1639,7 @@ func TestUsersFilter(t *testing.T) {
 				Status: codersdk.UserStatusSuspended + "," + codersdk.UserStatusActive,
 			},
 			FilterF: func(_ codersdk.UsersRequest, u codersdk.User) bool {
-				return true
+				return u.Status == codersdk.UserStatusSuspended || u.Status == codersdk.UserStatusActive


Same here, let's keep the original as it was almost certainly purposeful.

ethanndickson · 2024-12-10T12:59:05Z

coderd/users_test.go

@@ -1513,6 +1513,45 @@ func TestUsersFilter(t *testing.T) {
 		users = append(users, user)
 	}

+	// Add users with different creation dates for testing date filters
+	for i := 0; i < 3; i++ {
+		// nolint:gocritic


Each of these nolint should mention that the system context is necessary here. (Also to make it clear which specific rule this is nolinting)

joobisb · 2024-12-10T18:58:36Z

Few comments then I'm happy to approve!

@ethanndickson addressed the comments!

ethanndickson

Last comment, thanks for your patience!

ethanndickson · 2024-12-11T01:48:59Z

coderd/users_test.go

@@ -2273,6 +2346,28 @@ func onlyUsernames[U codersdk.User | database.User](users []U) []string {
 	return out
 }

+// dbUserToSDKUser converts database.User to codersdk.User
+func dbUserToSDKUser(dbUser database.User) codersdk.User {


We fortunately already have this function, db2sdk.User. It takes a database.User and a slice of organization IDs. In this case that slice should just be []uuid.UUID{first.OrganizationID})

thanks for pointing this out!

joobisb · 2024-12-11T09:38:10Z

Last comment, thanks for your patience!

@ethanndickson Done!

ethanndickson · 2024-12-11T10:11:19Z

coderd/users_test.go

+			fmt.Printf("expexp: %+v\n", exp)
+			fmt.Printf("matched.Usersmatched.Users: %+v\n", matched.Users)


Suggested change

fmt.Printf("expexp: %+v\n", exp)

fmt.Printf("matched.Usersmatched.Users: %+v\n", matched.Users)

my bad, fixed it!

ethanndickson · 2024-12-12T09:34:06Z

Looks like the test is failing on Postgres.

The expected list of users have timestamps with time zones, while the actual list of users retrieved via the API has timestamps parsed from strings, which don’t have timezones. I’m not sure of the best way to fix that..

joobisb · 2024-12-14T07:45:27Z

Looks like the test is failing on Postgres.

The expected list of users have timestamps with time zones, while the actual list of users retrieved via the API has timestamps parsed from strings, which don’t have timezones. I’m not sure of the best way to fix that..

@ethanndickson put together a little hack to fix this. Instead of using the db object, call an API to update the status and used that user object to be added to the list of expected users. Please do have a look.

ethanndickson · 2024-12-16T10:05:02Z

coderd/users_test.go

+		})
+		require.NoError(t, err)
+
+		// hack: Call UpdateUserStatus to get API-formatted timestamps (without timezones)


Let's avoid all this extra work & extra DB queries if we can.

For each of these, we can round-trip just the timestamps.

sdkUser1 := db2sdk.User(user1, nil) sdkUser1.CreatedAt, err = time.Parse(time.RFC3339, sdkUser1.CreatedAt.Format(time.RFC3339)) require.NoError(t, err) sdkUser1.UpdatedAt, err = time.Parse(time.RFC3339, sdkUser1.UpdatedAt.Format(time.RFC3339)) require.NoError(t, err) sdkUser1.LastSeenAt, err = time.Parse(time.RFC3339, sdkUser1.LastSeenAt.Format(time.RFC3339)) require.NoError(t, err) users = append(users, sdkUser1)

I don't think we'll find a better way. I wish the API just let you manually enter this timezoneless state.

ethanndickson · 2024-12-16T10:09:33Z

Yep, definitely unrelated. Will fix.

ethanndickson · 2024-12-16T10:24:35Z

Should be fixed after a merge from main!

ethanndickson · 2024-12-17T03:29:48Z

coderd/users_test.go

@@ -1677,6 +1777,17 @@ func TestUsersFilter(t *testing.T) {
 					exp = append(exp, made)
 				}
 			}
+
+			// TODO: This can be removed with dbmem
+			if !dbtestutil.WillUsePostgres() {


The plan is to eventually get rid of dbmem entirely, so I'm not too worried about cluttering up this test. I've used dbtestutil.WillUsePostgres as it's more likely to found and removed when that happens.

This is to handle a disparity between organization-less members in Postgres and dbmem. For dbmem, the API returns an empty slice. For postgres it returns nil. (FWIW it's not possible to have organization-less members on a real Coder deployment.)

ethanndickson

Thanks for your contribution and your patience through the review cycle! Greatly appreciated!

feat: support created_at filter for the GET /users endpoint

c0ff163

cdr-bot bot added the community Pull Requests and issues created by the community. label Nov 22, 2024

github-actions bot assigned joobisb Nov 22, 2024

update the query to created_at_before and created_at_after

e29a208

matifali requested review from ethanndickson and ericpaulsen December 5, 2024 05:11

ethanndickson reviewed Dec 5, 2024

View reviewed changes

rename the created fields

8f246c5

ethanndickson reviewed Dec 5, 2024

View reviewed changes

matifali and others added 3 commits December 9, 2024 17:36

Merge branch 'main' into issue#12747

caee9d9

Merge remote-tracking branch 'origin/main' into issue#12747

7cb8eaf

chore: update tests to handle user created filters

70895a4

joobisb commented Dec 10, 2024

View reviewed changes

ethanndickson reviewed Dec 10, 2024

View reviewed changes

addressed comments

daf481e

ethanndickson reviewed Dec 11, 2024

View reviewed changes

use existing db2sdk method

8193234

ethanndickson reviewed Dec 11, 2024

View reviewed changes

joobisb added 2 commits December 11, 2024 16:27

remove unnecessary print statements

67a7113

fix tests

20ed862

fix: test failing on postgres

f2d1a02

fix lint

0f2104d

ethanndickson reviewed Dec 16, 2024

View reviewed changes

joobisb and others added 4 commits December 16, 2024 23:26

Merge remote-tracking branch 'origin/main' into issue#12747

5771a32

fix timezone

f00b0d5

use nil org ID slice to compare equal

049aafa

use nil org ID slice on dbmem API to compare equal

981656f

ethanndickson reviewed Dec 17, 2024

View reviewed changes

fmt

de63832

ethanndickson approved these changes Dec 17, 2024

View reviewed changes

ethanndickson merged commit e191d96 into coder:main Dec 17, 2024
30 checks passed

github-actions bot locked and limited conversation to collaborators Dec 17, 2024

joobisb deleted the issue#12747 branch December 17, 2024 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support created_at filter for the GET /users endpoint #15633

feat: support created_at filter for the GET /users endpoint #15633

joobisb commented Nov 22, 2024

joobisb commented Nov 22, 2024

ethanndickson commented Nov 25, 2024 •

edited

Loading

joobisb commented Nov 28, 2024 •

edited

Loading

joobisb commented Dec 4, 2024

ethanndickson left a comment

ethanndickson Dec 5, 2024

joobisb Dec 5, 2024

ethanndickson Dec 5, 2024

joobisb Dec 10, 2024

joobisb Dec 10, 2024

ethanndickson Dec 10, 2024

joobisb Dec 10, 2024

ethanndickson Dec 10, 2024

ethanndickson left a comment

ethanndickson Dec 10, 2024

ethanndickson Dec 10, 2024

ethanndickson Dec 10, 2024

joobisb commented Dec 10, 2024

ethanndickson left a comment

ethanndickson Dec 11, 2024

joobisb Dec 11, 2024

joobisb commented Dec 11, 2024

ethanndickson Dec 11, 2024

joobisb Dec 11, 2024

ethanndickson commented Dec 12, 2024

joobisb commented Dec 14, 2024

ethanndickson Dec 16, 2024 •

edited

Loading

ethanndickson commented Dec 16, 2024

ethanndickson commented Dec 16, 2024

ethanndickson Dec 17, 2024 •

edited

Loading

ethanndickson left a comment

		fmt.Printf("expexp: %+v\n", exp)
		fmt.Printf("matched.Usersmatched.Users: %+v\n", matched.Users)

feat: support created_at filter for the GET /users endpoint #15633

feat: support created_at filter for the GET /users endpoint #15633

Conversation

joobisb commented Nov 22, 2024

joobisb commented Nov 22, 2024

ethanndickson commented Nov 25, 2024 • edited Loading

joobisb commented Nov 28, 2024 • edited Loading

joobisb commented Dec 4, 2024

ethanndickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanndickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joobisb commented Dec 10, 2024

ethanndickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joobisb commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanndickson commented Dec 12, 2024

joobisb commented Dec 14, 2024

ethanndickson Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

ethanndickson commented Dec 16, 2024

ethanndickson commented Dec 16, 2024

ethanndickson Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

ethanndickson left a comment

Choose a reason for hiding this comment

ethanndickson commented Nov 25, 2024 •

edited

Loading

joobisb commented Nov 28, 2024 •

edited

Loading

ethanndickson Dec 16, 2024 •

edited

Loading

ethanndickson Dec 17, 2024 •

edited

Loading