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

Add 'graph teams user remove' command capability solving #757 #761

Closed
wants to merge 4 commits into from

Conversation

appieschot
Copy link
Member

Add 'graph teams user remove' command capability solving #757

@coveralls
Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 3c0a13e on appieschot:feature/teams-user-remove into 2b4f005 on pnp:dev.

@waldekmastykarz
Copy link
Member

Awesome! I'll have a look shortly!

@waldekmastykarz waldekmastykarz self-assigned this Jan 17, 2019
@waldekmastykarz
Copy link
Member

Is it by design that you can't remove owners? Using the command I can remove members just fine, but when I try to remove someone who is an owner, I'm getting the following error:

Error: Resource 'c2618083-08b9-471c-bab2-b4a3222fc04c' does not exist or one of its queried reference-property objects are not present.

@appieschot
Copy link
Member Author

Found the issue; what happens is that you cannot delete a owner using the same call as a member. There are two ways we can solve this:

  • We can retrieve all owners and members and check what role the user has before deleting
  • We can add a -Role <Member|Owner> option that you can specify, and thus explicitly set the type of user you want to delete.

The last option is the way it is implemented in PowerShell and has my preference. Let me know what implementation you think would fit best and I'll update the PR.

@waldekmastykarz
Copy link
Member

Thanks for looking into it. It seems odd to me to have to specify the user role after you've already specified the user name. The user name by itself, followed by a confirmation, should be enough. I'd actually vote for option 1 where we detect the type of user and issue the correct call.

@appieschot
Copy link
Member Author

@waldekmastykarz As requested; the call now retrieves all owners. If the passed user is an owner we execute the appropriate remove call, if the user is not an owner we assume its a member and execute that call. Let me know what you think

@waldekmastykarz
Copy link
Member

Will have a look. Thanks! 👏

Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Very nicely done! 👏

let vorpal: Vorpal;
let log: string[];
let cmdInstance: any;
//let cmdInstanceLogSpy: sinon.SinonSpy;
Copy link
Member

Choose a reason for hiding this comment

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

We should not ship commented out code

cb({ continue: false });
}
};
//cmdInstanceLogSpy = sinon.spy(cmdInstance, 'log');
Copy link
Member

Choose a reason for hiding this comment

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

We should not ship commented out code

}

public get description(): string {
return 'Removes a specified user from the specified Microsoft Teams team';
Copy link
Member

Choose a reason for hiding this comment

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

...the specified user...

@@ -0,0 +1,43 @@
# graph teams user remove

Removes a specified user from the specified Microsoft Teams team
Copy link
Member

Choose a reason for hiding this comment

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

...the specified user...

graph teams user remove --teamId '00000000-0000-0000-0000-000000000000' --userName 'anne.matthews@contoso.onmicrosoft.com'
```

Removes user from the specified team without confirmation
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary indenting

@waldekmastykarz
Copy link
Member

Merged manually. Thank you! 👏

@appieschot appieschot deleted the feature/teams-user-remove branch April 16, 2019 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants