Conversation
|
Thank you @nanddeepn we will review shortly 👍🏻 |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Let's update the changes a bit before we merge
| request | ||
| .get(requestOptions) | ||
| .then((res: any): void => { | ||
| .then((res: any): Promise<any> => { |
There was a problem hiding this comment.
We should avoid using any to avoid refactoring issues in the future. Since we know what data we expect from the API, we should use specific types instead.
| @@ -0,0 +1,13 @@ | |||
| export interface User { | |||
There was a problem hiding this comment.
As we're getting User from MS Graph, let's use the type from the @microsoft/microsoft-graph-types package instead of defining our own.
There was a problem hiding this comment.
Oops! I will correct. Thank you.
|
The failing test is |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Hey @nanddeepn, we're almost there. Sorry I didn't catch the issues earlier and I appreciate you sticking with us ❤️
| request | ||
| .get(requestOptions) | ||
| .then((res: any): void => { | ||
| .then((res: any): Promise<User> => { |
There was a problem hiding this comment.
Since we know the shape of the data that we expect from the API, let's avoid using any and use the expected types instead to avoid runtime errors.
| .then((res: any): Promise<User> => { | ||
| if (args.options.email) { | ||
| if (res.value.length > 0) { | ||
| return Promise.resolve(res.value[0]); |
There was a problem hiding this comment.
Rather than returning the first user silently, it would be better to throw an error and prompt the user to disambiguate using the user name or ID. Otherwise you might end up working with the wrong user object.
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Very nicely done with just one small change I've done when merging the PR
|
|
||
| return Promise.reject(`Multiple users with ${identifier} found. Please disambiguate (user names): ${res.value.map(a => a.userPrincipalName).join(', ')} or (ids): ${res.value.map(a => a.id).join(', ')}`); | ||
| }) | ||
| .then((res: any): any => { |
There was a problem hiding this comment.
This line should be:
.then((res: User): void => {or for short:
.then(res => {Let's avoid using any when possible
|
Merged manually. Thank you! 👏 |
3 participants
Extends
aad user getwithemailoption. Closes #2855