Add CRUD operations for Org user #210
Conversation
dataclouder
commented
Jul 3, 2019
dataclouder
commented
- Update AdminOrg structure to include roles and users
- Define the new type User
- Define auxiliary types RoleReference, RightReference
- Add AdminOrg methods GetRole, GetUserByNameOrId
- Add AdminOrg methods CreateUser, SimpleCreateUser
- Add OrgUser methods GetRoleName, Update, Enable, Disable
- Add OrgUser methods UnconditionalDelete, SafeDelete
- Add OrgUser methods ChangePassword, Unlock, TakeOwnership
- Add tests for org user retrieval, creation, update, deletion
* Update AdminOrg structure to include roles and users * Define the new type User * Define auxiliary types RoleReference, RightReference * Add AdminOrg methods GetRole, GetUserByNameOrId * Add AdminOrg methods CreateUser, SimpleCreateUser * Add OrgUser methods GetRoleName, Update, Enable, Disable * Add OrgUser methods UnconditionalDelete, SafeDelete * Add OrgUser methods ChangePassword, Unlock, TakeOwnership * Add tests for org user retrieval, creation, update, deletion
dataclouder
requested review from
Didainius,
lvirbalas and
vbauzys
and removed request for
lvirbalas
Change return object for Role on failure Remove explicit timeout from CreateUser (use MaxRetryTimeout instead) Improve structure checking Add comment about field order in User type Change field AdminOrg in OrgUser structure to be public
govcd/user.go
Outdated
| // by either name or ID | ||
| // Returns a valid user if it exists. If it doesn't, returns nil and ErrorEntityNotFound | ||
| func (adminOrg *AdminOrg) GetUserByNameOrId(identifier string, willRefresh bool) (*OrgUser, error) { | ||
| if willRefresh { |
There was a problem hiding this comment.
I am kind'a lost here with context willRefresh - what it means
There was a problem hiding this comment.
Like other functions where we search the parent to find the entity, we need to refresh the entity to make sure we have a fresh image. However, since the refresh is costly, if we know that we want to get what is already there (such as when we want to get all the current users, we don't need to refresh every time) we set willRefhesh to false.
There was a problem hiding this comment.
Sorry, I meant 'willRefresh -> 'refresh' or something else
There was a problem hiding this comment.
"refresh" can be either a verb or a noun, while willRefresh shows that I want it to do something.
There was a problem hiding this comment.
@lvirbalas @Didainius what is your opinion? willRefresh, Refresh, toRefresh, withRefresh
There was a problem hiding this comment.
I can add one more withRefresh :) but it won't change too much so it can be even as it is for me.
There was a problem hiding this comment.
I have a pretty strong opinion on this one. It should be just refresh and we should use the same approach in other places if/when we need it.
govcd/user.go
Outdated
| // GetUserByNameOrId retrieves an user within an admin organization | ||
| // by either name or ID | ||
| // Returns a valid user if it exists. If it doesn't, returns nil and ErrorEntityNotFound | ||
| func (adminOrg *AdminOrg) GetUserByNameOrId(identifier string, willRefresh bool) (*OrgUser, error) { |
There was a problem hiding this comment.
Seems here we explicitly state that identifier can hold name of ID. Or maybe have seperata method GetUserByNameOrId(identifier string, identifier name, refresh bool).. O wrappers - GetUserByName, GetUserById
There was a problem hiding this comment.
As I mentioned above, it's unlikely that we can mistake an ID for a name, and for OrgUsers it is impossible, as names can't contain some of the characters we find in IDs.
I don't see the risk of this approach: even if it were possible to use a name that looks like urn:vcloud:user:440a94b2-414b-4923-aff7-439603c8dd48, what's the risk, since we check both fields and return a match if they are the same?
There was a problem hiding this comment.
I just personally don't like implicit behaviour - double meaning parameters. Especially in public API. If everyone else ok with this - I can live with it.
There was a problem hiding this comment.
It is not implicit. It does what it says. Finds "By name or by ID".
This is also on line with what I propose in #211
There was a problem hiding this comment.
I would like opinion @lvirbalas and @Didainius here
There was a problem hiding this comment.
IMHO probably separate fields would be clearer here.
Remove function GetUserByNameOrId Add function FetchUserByName Add function FetchUserById Add function FetchUserByNameOrId Remove SafeDelete and UnconditionalDelete Add function Delete with takeOwnership parameter
Introduce FetchByHref function
There was a problem hiding this comment.
LGTM now!
We now have the new kind of Fetch... function and all its variants. It's clear, intuitive and we could use that as a role model going forward:
The core:
- func (adminOrg *AdminOrg) FetchUserByHref(href string) (*OrgUser, error)
The two variants:
- func (adminOrg *AdminOrg) FetchUserByName(name string, refresh bool) (*OrgUser, error)
- func (adminOrg *AdminOrg) FetchUserById(id string, refresh bool) (*OrgUser, error)
And the helper:
- func (adminOrg *AdminOrg) FetchUserByNameOrId(identifier string, refresh bool) (*OrgUser, error)
