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.

Sign up for GitHub

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

Jump to bottom

Issue 3692: Updated teams channel member add, teams channel member re… #3698

Closed
wants to merge 3 commits into from
Closed

Issue 3692: Updated teams channel member add, teams channel member re… #3698

wants to merge 3 commits into from

Conversation

AaronS16
Copy link
Contributor

@AaronS16 AaronS16 commented Sep 24, 2022 •
edited

'Closes #3692'

@AaronS16
Issue 3692: Updated teams channel member add, teams channel member re…
b35e121 
…move, and teams channel member set to include shared channels as having the same capability.
@milanholemans
Copy link
Contributor

Thank you @AaronS16 for this PR! We'll review it asap.
One more thing, could you reference the issue linked with this PR?
You can do this by writing "Closes #3692" in your PR description. Also could you remove the sample text from the PR description? It says "delete this section after reading" so you can safely delete it.

But your contribution is looking good on first sight!

@milanholemans milanholemans self-assigned this Sep 24, 2022
milanholemans
milanholemans requested changes Sep 24, 2022
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment •
edited

Choose a reason for hiding this comment

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

Well done so far. There are still some places we need to update.
Check the command description for example and also some options refer to a private channel.
For example the --channelId option says "The private channel's ID". This should be "The channel ID" let's just leave the spefic type away.

Also something you should consider: you should update the description in the command code itself.

cli-microsoft365/src/m365/teams/commands/channel/channel-member-add.ts

Lines 34 to 36 in e60254a

public get description(): string {
return 'Adds a conversation member in a private channel.';
}

I'm marking this PR as draft to show that some changes are still needed. If you made the changes, please hit the 'ready for review' button at the bottom of the page. Then the maintainers know this PR is ready for another review.

milanholemans
milanholemans requested changes Sep 24, 2022
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

We also missed this one.

docs/docs/cmd/teams/channel/channel-member-add.md Outdated
@@ -43,7 +43,7 @@ m365 teams conversationmember add [options]

At least one owner must be assigned to a private channel.
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
At least one owner must be assigned to a private channel.
At least one owner must be assigned to a private or shared channel.

@milanholemans milanholemans marked this pull request as draft September 24, 2022 19:02
@AaronS16 AaronS16 marked this pull request as ready for review September 24, 2022 19:05
milanholemans
milanholemans requested changes Sep 24, 2022
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Almost there, seems like you've removed too many private words.

Also don't forget to update the description of the commands in the command code itself.

cli-microsoft365/src/m365/teams/commands/channel/channel-member-add.ts

Lines 34 to 36 in e60254a

public get description(): string {
return 'Adds a conversation member in a private channel.';
}

Also for the 2 other commands.

docs/docs/cmd/teams/channel/channel-member-add.md
@@ -56,5 +56,5 @@ m365 teams channel member add --teamId 47d6625d-a540-4b59-a4ab-19b787e40593 --ch
Add owners to a channel based on their display names

```sh
m365 teams channel member add --teamName "Human Resources" --channelName "Private Channel" --userDisplayName "Anne Matthews,John Doe" --owner
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't delete this private.

@AaronS16
Copy link
Contributor Author

I have made the requested changes. Thank you for the assistance in this and I appreciate the assistance and opportunity so far. Please let me know if anything else has been missed.

milanholemans
milanholemans approved these changes Sep 24, 2022
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Added the command descriptions.
Everything is looking fine now. Thank you for this contribution! 👏

Is it correct that your name is Aaron Sotelo? We need your name so we can add you to the list of contributors.

@milanholemans
Copy link
Contributor

milanholemans commented Sep 24, 2022 •
edited

Merged manually, thank you!

Welcome to the team by the way! https://pnp.github.io/cli-microsoft365/about/team

@AaronS16
Copy link
Contributor Author

Added the command descriptions. Everything is looking fine now. Thank you for this contribution! 👏

Is it correct that your name is Aaron Sotelo? We need your name so we can add you to the list of contributors.

Yes, that is my full name and thank you for allowing me to contribute to this project. Please let me know if there is anything else I can help with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milanholemans milanholemans milanholemans approved these changes

Assignees

@milanholemans milanholemans

Labels
docs pr-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update documentation teams channel member commands
2 participants
@AaronS16 @milanholemans