Created ListUsers database helper #134
Conversation
|
This pull request has been linked to Shortcut Story #13268: Create ListUsers database helper. |
There was a problem hiding this comment.
This is looking good @pdamodaran! I had a handful of minor suggestions if you could take a quick look.
| listUserIdSQL = "SELECT user_id FROM organization_users where organization_id = $1" | ||
| getUserSQL = "SELECT id, name, email, terms_agreement, privacy_agreement, last_login, created, modified from users where id = $1" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Let's add a docstring here similar to the one we have for ListAPIKeys to make sure the godoc has a good description of what the function does with different combinations of input params. In particular, I think we should call out:
- what types we expect
orgIDto be (looks like string or slice of bytes or maybe aulid.ULID?) - the different behaviors that can be achieved by passing in different values for
prevPage(if it's nil, if a specific value is set for thePageSize)
| //test passing null orgID results in error | ||
| users, cursor, err := models.ListUsers(ctx, ulids.Null, nil) | ||
| require.ErrorIs(err, models.ErrMissingOrgID, "orgID is required for list queries") | ||
| require.NotNil(err) | ||
| require.Nil(cursor) | ||
| require.Nil(users) |
There was a problem hiding this comment.
Nice! Should we also test to make sure we get the error we expect if the input orgID is non-nil but not parseable by the Parse method (e.g. possibly an integer?)
There was a problem hiding this comment.
good point - I added the check
| require.Nil(users) | ||
|
|
||
| _, _, err = models.ListUsers(ctx, orgID, &pagination.Cursor{}) | ||
| require.ErrorIs(err, models.ErrMissingPageSize, "pagination is required for list queries") |
There was a problem hiding this comment.
Would you mind if we made this error message slightly different so that it's easier to differentiate from the one that the API keys list test will throw?
There was a problem hiding this comment.
updated the error message
|
|
||
| // Should return all checkers users (page cursor not required) | ||
| users, cursor, err = models.ListUsers(ctx, orgID, nil) | ||
| require.NoError(err, "could not fetch all users for checkers org") |
There was a problem hiding this comment.
Guessing that the orgID 01GQFQ14HXF2VC7C1HJECS60XX corresponds to a test fixture for an org called Checkers?
There was a problem hiding this comment.
yes - I modified the comments to make this more clear
| require.Len(users, 2, "expected 2 users from checkers org") | ||
| user := users[0] | ||
| //verify password is not returned | ||
| require.Empty(user.Password) |
| require.Equal("Owner", role) | ||
| permissions, err := user.Permissions(ctx, false) | ||
| require.Nil(err) | ||
| require.Len(permissions, 18, "expected 10 permissions for user") |
There was a problem hiding this comment.
Should this error message say "expected 18 permissions for user" ?
There was a problem hiding this comment.
good catch! I have made the change.
| require.Equal(1, pages, "expected 2 results in 1 pages") | ||
| require.Equal(2, nRows, "expected 2 results in 1 pages") |
There was a problem hiding this comment.
Should 591 and 592 have different error messages?
There was a problem hiding this comment.
yes - I have updated the messages
pkg/quarterdeck/db/models/users.go
Outdated
| //create user object to append to the users list and add the orgID to it | ||
| user := &User{orgID: userOrg} | ||
| var userID ulid.ULID | ||
| if err = rows.Scan(&userID); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if err = tx.QueryRow(getUserSQL, userID).Scan(&user.ID, &user.Name, &user.Email, &user.AgreeToS, &user.AgreePrivacy, &user.LastLogin, &user.Created, &user.Modified); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, nil, nil | ||
| } | ||
| return nil, nil, err |
There was a problem hiding this comment.
I'm not super familiar with the underlying database schema, but I'm curious if you think this could be done within a single query (e.g. by using a join?). I don't think that it's necessary to change anything in this PR, so just wondering! If you think it might be possible to refactor this in the future to reduce queries (which could speed up the Raft consensus process, especially if we have orgs with lots of users), maybe you could add a little inline comment here with a TODO so we don't forget? Otherwise feel free to ignore this 😁
There was a problem hiding this comment.
That's a very good and you are correct, there is a more efficient way to do this. I decided to refactor the code because it's not a big lift.
pkg/quarterdeck/db/models/users.go
Outdated
| ) | ||
|
|
||
| // ListUsers returns a paginated collection of users filtered by the orgID. | ||
| // The orgID must be a valid non-zero value of type ulid.ULID. |
There was a problem hiding this comment.
I think it could also be a string or slice of bytes, so long as it can be parsed by ulids.Parse
There was a problem hiding this comment.
Ok, just checked the tests for Parse -- here's what it has for some examples of vals that are valid vs invalid:
… organization that is passed in
|
Ok looks good @pdamodaran -- guess we could add a test that covers the possibility of a user that belongs to multiple orgs and has different roles for different orgs. The test would prove that we only get back the correct roles for that user in that org. Want to add a backlog story to your next sprint to take that on? Also, just wanted to make sure you saw my note above about the the possible values for |
|
@rebeccabilbro - I have updated the docstring to reflect all the valid values for orgID and I will create a follow on story to add the test you mentioned. |
Scope of changes
Created a ListUsers database helper similar to ListAPIKeys in
quarterdeck\db\models\apikeys.goFixes SC-13268
Type of change
Acceptance criteria
Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible.
Author checklist
Reviewer(s) checklist