-
Notifications
You must be signed in to change notification settings - Fork 2
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orgID
to 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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added docstring
//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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing that the orgID 01GQFQ14HXF2VC7C1HJECS60XX
corresponds to a test fixture for an org called Checkers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good check!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this error message say "expected 18 permissions for user" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should 591 and 592 have different error messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.go
Fixes 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