Skip to content
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

feat(api): implement crud operations for team members #614

Merged
merged 6 commits into from
Nov 30, 2021

Conversation

vatasha
Copy link
Contributor

@vatasha vatasha commented Nov 16, 2021

Summary

Implement team members.

How did you test this change?

There are some new tests.

Issue

https://lacework.atlassian.net/browse/ALLY-736

Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good!

tenor-5332604

Comment on lines 59 to 61
if userEnabledEnum > 1 || userEnabledEnum < 0 {
return TeamMember{}, errors.New("userEnabled field must be 0 for disabled or 1 for enabled")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about this over in standup, lets stick with numbers and set it to 1 (enabled) by default.

err = errors.New("please specify a guid")
return
}
err = svc.client.RequestEncoderDecoder("PATCH", fmt.Sprintf(apiV2TeamMembersFromGUID, tm.UserGuid), tm, &res)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For updates, verify that you are able to send the UserGuid as part of the request payload, some endpoints complain about it since you are already injecting it via URL parameter.

// Delete deletes a single team member with the corresponding guid
func (svc *TeamMembersService) Delete(guid string) error {
if guid == "" {
errors.New("please specify a guid")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
errors.New("please specify a guid")
return errors.New("please specify a guid")

@vatasha vatasha marked this pull request as ready for review November 18, 2021 15:51
Company string `json:"company"`
CreatedTime string `json:"createdTime,omitempty"`
FirstName string `json:"firstName"`
JitCreated bool `json:"jitCreated,omitempty"`
Copy link
Contributor Author

@vatasha vatasha Nov 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afiune @dmurray-lacework I'm worried about a couple fields here that are strings but the API returns integers. For example here is the API response for getting a single team member. { "data": { "custGuid": "<redacted>", "props": { "accountAdmin": true, "company": "CUSTOMERDEMO", "createdTime": "2020-04-09T17:39:51.572Z", "firstName": "<redacted>", "jitCreated": false, "lastName": "<redacted>", "lastSessionCreatedTime": 0, "orgAdmin": true, "orgUser": false, "updatedTime": 0 }, "userEnabled": 1, "userGuid": "<redacted>", "userName": "<redacted>" } }

How should we handle the API returning ints for lastSessionCreatedTime and updatedTime? I suspect that we're going to have issues deserializing.

Copy link
Collaborator

@dmurray-lacework dmurray-lacework Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems when something hasn't been updated updatedTime/lastSessionCreatedTime returns as 0(int) but when populated it returns as string time stamp. "updatedTime": "2021-02-25T15:25:01.691Z".

This may not actually cause any issues. int value will fail to deserialize, but updatedTime/lastSessionCreatedTime would be empty strings anyway. But would be good to test this to confirm.

LastSessionCreatedTime: "0",
OrgAdmin: false,
OrgUser: false,
UpdatedTime: "0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the comment above would this not be an int of 0. And for scenarios when the member has been updated it would be a string timestamp like CreatedTime "2021-11-16T16:33:17.573Z"

"lastSessionCreatedTime": "0",
"orgAdmin": false,
"orgUser": false,
"updatedTime": "0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what @dmurray-lacework already mentioned, what happens if this field is 0?

      		"updatedTime": 0

Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenor-172213508

@vatasha vatasha merged commit 4aa40a2 into main Nov 30, 2021
@vatasha vatasha deleted the vatasha/ally-736/implement-team-members branch November 30, 2021 23:38
@lacework-releng lacework-releng mentioned this pull request Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants