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

Adds "teams meeting create" command. Closes #1345 #5550

Closed
wants to merge 4 commits into from

Conversation

mkm17
Copy link
Contributor

@mkm17 mkm17 commented Oct 7, 2023

Adds "teams meeting create" command. Closes #1345

Closes #1345

This is my first pull request that introduces a completely new command. I hope that all necessary information is included.

In comparison to the initial concept described in issue #1345, I've made the following changes:

  • Removed the "-o" shortcut for "-organizerEmail" as "-o" is already used as the output shortcut.
  • Changed "participantEmails" to "participants" since I've used User Principal Names (UPNs) instead of emails to add participants in the code.
  • Note that "organizerEmail" can only be used with App-only permissions. Additionally, I've added a description of the required policy at the end of the command's remarks section.

To use this command as the current user, we will need to extend the "PnP Office 365 Management Shell" app with the "OnlineMeetings.ReadWrite" Microsoft Graph delegated permission.

All comments would be more than welcome. :)

I noticed that there is an option to label the PR as Hacktoberfest-related. Since I've joined the event this year, I would appreciate it if you could include it in the initiative.

@martinlingstuyl
Copy link
Contributor

Great work @mkm17! We'll review ASAP!

@martinlingstuyl martinlingstuyl self-assigned this Nov 6, 2023
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @mkm17, for your first command this is really well done! I do have some comments, like I always do 😀 could you look at them and get back to me?

By the way: I totally overlooked your remark on hacktoberfest, which is unfortunate. Sorry man! Hope you made it to the required amount of PR's anyway!

docs/docs/cmd/teams/meeting/meeting-create.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/teams/meeting/meeting-create.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/teams/meeting/meeting-create.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/teams/meeting/meeting-create.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/teams/meeting/meeting-create.mdx Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-create.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-create.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-create.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-create.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-create.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft November 6, 2023 22:58
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 7, 2023

Hello @martinlingstuyl, thank you for the review. I will make updates later this week.

@mkm17 mkm17 force-pushed the issues/1345_Create_New_Meeting branch from 65fd80a to 9cde362 Compare November 16, 2023 21:11
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 16, 2023

Hi @martinlingstuyl, thank you once again for all the checks. I have made changes to the code and rebased it, so I hope that everything is okay. In case of any issues, let me know, and I will try to correct them promptly.

@mkm17 mkm17 marked this pull request as ready for review November 16, 2023 21:58
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 16, 2023

@martinlingstuyl And just a reminder, before deploying this command, we will need to extend the PnP Office 365 Management Shell app with the 'OnlineMeetings.ReadWrite' permission. https://learn.microsoft.com/en-us/graph/api/application-post-onlinemeetings?view=graph-rest-1.0&tabs=http

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @mkm17,

One last round of comments. Do check out your use of white lines. We're almost there!

src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.spec.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.spec.ts Outdated Show resolved Hide resolved
docs/docs/cmd/teams/meeting/meeting-add.mdx Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
src/m365/teams/commands/meeting/meeting-add.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft November 21, 2023 21:33
@mkm17 mkm17 marked this pull request as ready for review November 23, 2023 09:04
@mkm17
Copy link
Contributor Author

mkm17 commented Nov 23, 2023

Hi @martinlingstuyl thanks again for the review. All requested changes should be included :)

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Looks fine, a couple of last small comments. If you like you may update them, otherwise I'll do it when merging. It might be that I'll merge a week from now, as I'm prepping for a conference currently.

@mkm17
Copy link
Contributor Author

mkm17 commented Nov 23, 2023

@martinlingstuyl ok I will make the change later today, so you will just have it the merge in on the list. Fingers crossed for your conference presentation!

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

fantastic!

@mkm17
Copy link
Contributor Author

mkm17 commented Nov 23, 2023

@martinlingstuyl Ok I have applied the changes. Thanks for the prompt approval!

@martinlingstuyl
Copy link
Contributor

Merged manually, thank you! 🥳

@mkm17 mkm17 deleted the issues/1345_Create_New_Meeting branch January 15, 2024 15:52
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.

New command: Start a meeting in a team
2 participants