-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add PrimaryEmail method for Directory User #169
Add PrimaryEmail method for Directory User #169
Conversation
expected: "firstprimaryemail@foo-corp.com", | ||
}, | ||
{ | ||
scenario: "No primary emails returns null", |
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.
It looks like we're returning an error rather than null
here. Can we rename these tests to indicate an error is returned as well as an empty string?
@@ -72,3 +73,13 @@ func DeleteDirectory( | |||
) error { | |||
return DefaultClient.DeleteDirectory(ctx, opts) | |||
} | |||
|
|||
// PrimaryEmail is a method for finding a user's primary email (when applicable) | |||
func (r User) PrimaryEmail() (string, error) { |
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 logic be in the client.go file, like the other methods? And then called via DefaultClient
?
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 could be wrong here, but my gut said no. Client performs Directory Sync requests to the WorkOS API, but primary email doesn't actually make a request to the API, it just takes information that already exists from the User type and distills it.
Someone more familiar than me may need to confirm my understanding.
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.
If I understand this correctly, I do think that the PrimaryEmail
should be a method on the User
object rather than being tied with the methods on the client that actually call the API.
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.
Ahh ok, that makes sense. It's just we only have methods that use the client that connects to the API, so that's why we have that logic in the client now.
Adds a method to get the primary email of a directory user.