New command graph teams tab list solving #849 by thomyg · Pull Request #896 · pnp/cli-microsoft365

thomyg · 2019-03-28T20:53:08Z

Added new command for listing tabs in a Microsoft Teams channel.

Added:

command implementation
tests
doc file

Any feedback welcome! thx!

… for teams list

New command graph teams tab list

coveralls · 2019-03-28T20:56:59Z

Coverage remained the same at 100.0% when pulling 204c0ca on thomyg:master into b4bf46b on pnp:dev.

waldekmastykarz · 2019-03-29T06:35:07Z

Hero! We're going to review it asap. Thank you! 👏

VelinGeorgiev

Hey @thomyg, great first commit! Worked like charm! I did few minor changes before we can merge it. You can find my inline comments.
You can see the changes I made here: https://github.com/VelinGeorgiev/office365-cli/tree/thomy/master. Waldek will use that branch to merge it with the pnp one.

Two additional things:

When you do npm install, the npm-shrinkwrap.json file get changed. I usually do not include the changed file in the pull request and let Waldek make changes to the npm-shrinkwrap.json when needed.
I've added the teams-tab-list.md doc to the mkdocs.yaml file under the teams nodes. That is required as part of the pull request.

VelinGeorgiev · 2019-03-30T21:09:32Z

+  }
+
+  public get description(): string {
+    return 'Lists Tabs in a Microsoft Teams team.';


Changed to Lists tabs in the specified Microsoft Teams channel

VelinGeorgiev · 2019-03-30T21:11:42Z

+    const options: CommandOption[] = [
+      {
+        option: '-i, --teamId <teamId>',
+        description: 'The ID of the team to list the tab of'


Changed to The ID of the team of the specific channel

VelinGeorgiev · 2019-03-30T21:12:17Z

+      },
+      {
+        option: '-c, --channelId <channelId>',
+        description: 'The ID of the channel to list the tab of'


Changed to The ID of the channel for which to list tabs

VelinGeorgiev · 2019-03-30T21:15:35Z

+
+To list the tabs in a Microsoft Teams channel, you have to first log in to the Microsoft Graph using the [graph login](../login.md) command, eg. `graph login`.
+
+## Examples


Example removed since not part of this command

VelinGeorgiev · 2019-03-30T21:16:08Z

+!!! important
+    Before using this command, log in to the Microsoft Graph, using the [graph login](../login.md) command
+
+## Remarks


Remarks removed since it duplicates with another one bellow

VelinGeorgiev · 2019-03-30T21:32:51Z

+}
+
+interface Options extends GlobalOptions {
+  joined?: boolean;


joined removed since it is not part of the command specification.

VelinGeorgiev · 2019-03-30T21:34:18Z

+    return 'Lists Tabs in a Microsoft Teams team.';
+  }
+
+  public getTelemetryProperties(args: CommandArgs): any {


getTelemetryProperties is removed since the command has two mandatory options teamId and channelId and they are tracked by default. We use the getTelemetryProperties method in a case of optional options.

VelinGeorgiev · 2019-03-30T21:36:05Z

+
+  public commandAction(cmd: CommandInstance, args: CommandArgs, cb: () => void): void {
+
+    let endpoint: string = `${auth.service.resource}/V1.0/teams/${args.options.teamId}/channels/${args.options.channelId}/tabs?$expand=teamsApp`;


encodeURIComponent(args.options.channelId) instead of just args.options.channelId just to ensure no weird characters in the id can break the request.

Also V1.0 changed to v1.0.

VelinGeorgiev · 2019-03-30T21:41:22Z

+    List all tabs in a Microsoft Teams team
+      ${chalk.grey(config.delimiter)} ${this.name} --teamId <teamId> --channelId <channelId>
+
+    Include the all the values from the tab configuration and associated teams app


Changed to Include all the values from the tab configuration and associated teams app to remove one unnecessary the and match the description in the MD file.

VelinGeorgiev · 2019-03-30T21:42:56Z

+
+    You can only see the tab list of a team you are a member of.
+
+    The tabs Conversations and Files are present in every team and therefor not provided as a


Typo. therefor changed to therefore.

thomyg · 2019-03-31T09:38:01Z

thx @VelinGeorgiev for your feedback! appreciated! Sorry for not adding the documentation in the yaml file, that is stated in the help an I missed it, sorry. All the other remarks will be taken into account for future commits, THX!

One more question, maybe not 100% only related to this PR, but I reached out to the Teams PG and the one responsible for the graph APIs and asked about how one could validate the channel ID. His answer basically was that he would only check for a "19:" on the begin of the string and the "@thread.skype.com" at the end. Everything else in between, that looks like a GUID without hyphens might change in the future. Details can be found in an NDA Teams team, so maybe ping me in private if you are interested.
Nevertheless, my point is, should there be a central utility that offers channel validation in the teams command section? Thinking of something like teams-utilities.ts for that. My argument is, that this validation is needed quite a few times as the --channelId is a parameter at some teams calls. If we would centralize the validation it could be easily adjusted in the future. What I have seen so far is, that most of the commands only check the existence of the parameter but not the actual content of the string. Opinions? Or do you already have that on the roadmap somewhere and I missed it?

waldekmastykarz · 2019-03-31T13:08:33Z

@thomyg good suggestion! We shouldn't create a separate utils class for Teams but instead add it to the existing Utils class. Feel free to create a new issue for this improvement so that we can track it separately. We can then decide if you want to pick it up yourself or let others do it 👏

waldekmastykarz · 2019-03-31T18:52:07Z

Merged manually. Thank you! 👏

VelinGeorgiev · 2019-04-02T11:59:49Z

Good stuff! I am not very concerned about strict validation on the channel id, because the request will fail when the one using the CLI has entered wrong input. However, I also agree that will be better experience if we provide more meaningful message if the garph apis do not.

thomyg added 5 commits March 24, 2019 23:41

initial commit, started playing with the cli, tried the v1.0 endpoint…

a2f1851

… for teams list

teams tabs list first version

d621419

teams tab list ready for PR

d0dea3b

deleted local vscode files

2bc6dda

Merge pull request #1 from thomyg/dev

204c0ca

New command graph teams tab list

thomyg changed the title ~~New command graph teams tab list~~ New command graph teams tab list solvion #849 Mar 28, 2019

thomyg changed the title ~~New command graph teams tab list solvion #849~~ New command graph teams tab list solving #849 Mar 28, 2019

VelinGeorgiev self-assigned this Mar 30, 2019

VelinGeorgiev added the work in progress label Mar 30, 2019

VelinGeorgiev approved these changes Mar 30, 2019

View reviewed changes

waldekmastykarz removed the work in progress label Mar 31, 2019

waldekmastykarz closed this Mar 31, 2019

waldekmastykarz added the pr-merged label Mar 31, 2019

thomyg mentioned this pull request Apr 4, 2019

Create central validation for Teams channel IDs #909

Closed


		To list the tabs in a Microsoft Teams channel, you have to first log in to the Microsoft Graph using the [graph login](../login.md) command, eg. `graph login`.

		## Examples


		public commandAction(cmd: CommandInstance, args: CommandArgs, cb: () => void): void {

		let endpoint: string = `${auth.service.resource}/V1.0/teams/${args.options.teamId}/channels/${args.options.channelId}/tabs?$expand=teamsApp`;


		You can only see the tab list of a team you are a member of.

		The tabs Conversations and Files are present in every team and therefor not provided as a

Conversation

thomyg commented Mar 28, 2019

Uh oh!

coveralls commented Mar 28, 2019

Uh oh!

waldekmastykarz commented Mar 29, 2019

Uh oh!

VelinGeorgiev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VelinGeorgiev Mar 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomyg commented Mar 31, 2019

Uh oh!

waldekmastykarz commented Mar 31, 2019

Uh oh!

waldekmastykarz commented Mar 31, 2019

Uh oh!

VelinGeorgiev commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VelinGeorgiev Mar 30, 2019 •

edited

Loading

VelinGeorgiev commented Apr 2, 2019 •

edited

Loading