Adding teams user app list by sebastienlevert · Pull Request #1874 · pnp/cli-microsoft365

sebastienlevert · 2020-10-21T05:59:59Z

Adding teams user app list.

Coverage 100%

waldekmastykarz · 2020-10-21T12:42:28Z

Hey @sebastienlevert, seems like we're missing some coverage. Care to have another look?

sebastienlevert · 2020-10-21T12:49:05Z

It actually seems that something happened on the build side. Running them locally is successful. Trying another thing. Just did a new push and it seems that it's at 100% now. Maybe some instability? CircleCI is all good now!

sebastienlevert · 2020-10-21T12:52:07Z

Actually... It might have been a PR / push issue by myself (the squash part, probably).

garrytrinder · 2020-10-21T15:13:37Z

Thanks @sebastienlevert tests all looks good now 👍

I assume that this PR is good for review? If so, let us know clicking the 'Ready for review' button to take the PR out of draft status and it will show up on our radar 😊

waldekmastykarz · 2020-10-22T14:17:30Z

Hey @sebastienlevert, have you checked if this command is working? When I'm running it, I'm getting a 403 Forbidden error. Looking at the docs, it seems like users/<user>/teamwork/installedApps is available only in beta while currently the code points to v1.0. Am I missing something?

sebastienlevert · 2020-10-22T14:35:47Z

This is so weird... It works on my tenant on both endpoints... But the documentation says /beta.

I'll revert to beta for the code, for now.

sebastienlevert · 2020-10-22T14:41:56Z

Also, these CircleCI tests are breaking on and off... Am I breaking all of this or I'm crazy?

It seems like there are issues with App Insights!

sebastienlevert · 2020-10-22T15:09:28Z

Also @waldekmastykarz, did you reconsent to the app permissions? As this command requires the new TeamsAppInstallation.ReadWriteForUser.All scope!

waldekmastykarz · 2020-10-22T15:15:21Z

@sebastienlevert I've seen intermittent issues with tests. Not sure where it's coming from, but it's something that popped up recently. Seems like we should investigate it.

As for the permissions, I did reconsent the app, but looking at the token, I'm not seeing this scope despite it being in the app reg. Let me have another look at it. Thanks for confirming and sorry for the trouble.

waldekmastykarz · 2020-10-24T12:26:32Z

Hey @sebastienlevert, seems like the last change you've pushed, is breaking a couple of your tests:

Would you mind having a look?

sebastienlevert · 2020-10-24T17:54:39Z

Just added the /beta to the Unit Tests!

waldekmastykarz

Nicely done with a few minor changes I've done when merging the PR 👍

waldekmastykarz · 2020-10-27T11:43:10Z

+  public get description(): string {
+    return 'Lists the apps deployed in the personal scope of the specified user';
+  }
+


We're missing telemetry to track the usage of optional command options

waldekmastykarz · 2020-10-27T11:44:18Z

+  }
+
+  public commandAction(logger: Logger, args: CommandArgs, cb: () => void): void {
+    var userIdPromise: Promise<{ value: string; }>;


We should use let or const instead of the generic var

waldekmastykarz · 2020-10-27T11:46:47Z

+  public commandAction(logger: Logger, args: CommandArgs, cb: () => void): void {
+    var userIdPromise: Promise<{ value: string; }>;
+
+    if(args.options.userName) {


We could make the code easier to read and maintain by putting userId resolution in a separate method.

waldekmastykarz · 2020-10-27T11:47:08Z

+    userIdPromise.then((userId) => {
+      const endpoint: string = `${this.resource}/beta/users/${encodeURIComponent(userId.value)}/teamwork/installedApps`
+
+      this.getAllItems(endpoint, logger, true)


We should avoid nesting promises and instead return this operation to the main chain.

waldekmastykarz · 2020-10-27T11:50:15Z

+      this.getAllItems(endpoint, logger, true)
+        .then((): void => {
+          this.items.map(i => {
+            var userAppId = Buffer.from(i.id, 'base64').toString('ascii');


We should use const instead of var as this const is not assigned another value

waldekmastykarz · 2020-10-27T11:50:54Z

+        .then((): void => {
+          this.items.map(i => {
+            var userAppId = Buffer.from(i.id, 'base64').toString('ascii');
+            var appId = userAppId.substr(userAppId.indexOf("##") + 2, userAppId.length - userId.value.length - 2)


We should use const instead of var as this const is not assigned another value

waldekmastykarz · 2020-10-27T11:51:41Z

+    const options: CommandOption[] = [
+      {
+        option: '--userId',
+        description: 'The ID of user to get the apps from'


Doesn't match description from the docs

waldekmastykarz · 2020-10-27T11:51:45Z

+      },
+      {
+        option: '--userName',
+        description: 'The UPN of user to get the apps from'


Doesn't match description from the docs

waldekmastykarz · 2020-10-27T11:54:16Z

+  }
+
+  public get description(): string {
+    return 'Lists the apps deployed in the personal scope of the specified user';


Doesn't match description from the docs (installed vs. deployed)

waldekmastykarz · 2020-10-27T13:00:34Z

Merged manually. Thank you! 👏

sebastienlevert added 4 commits October 21, 2020 01:58

Adding teams user app list

8808eea

Coverage 100%

ee1a319

Adding teams user app list

4598814

Coverage 100%

Merge branch '1866' of https://github.com/sebastienlevert/cli-microso…

c0a8368

…ft365 into 1866

waldekmastykarz marked this pull request as draft October 21, 2020 12:42

Fixes

c4e4c72

sebastienlevert marked this pull request as ready for review October 21, 2020 16:00

waldekmastykarz added the waiting on response label Oct 22, 2020

Reverting to /beta endpoint

19a2999

waldekmastykarz removed the waiting on response label Oct 22, 2020

waldekmastykarz self-assigned this Oct 24, 2020

waldekmastykarz removed their assignment Oct 24, 2020

waldekmastykarz marked this pull request as draft October 24, 2020 12:27

Adding /beta to Unit Tests

60e2b98

sebastienlevert marked this pull request as ready for review October 24, 2020 18:56

sebastienlevert mentioned this pull request Oct 24, 2020

Adding remove + Updating install personal apps #1883

Closed

waldekmastykarz self-assigned this Oct 27, 2020

waldekmastykarz approved these changes Oct 27, 2020

View reviewed changes

waldekmastykarz added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 27, 2020

waldekmastykarz added pr-merged review-complete labels Oct 27, 2020

waldekmastykarz closed this Oct 27, 2020

Conversation

sebastienlevert commented Oct 21, 2020

Uh oh!

waldekmastykarz commented Oct 21, 2020

Uh oh!

sebastienlevert commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastienlevert commented Oct 21, 2020

Uh oh!

garrytrinder commented Oct 21, 2020

Uh oh!

waldekmastykarz commented Oct 22, 2020

Uh oh!

sebastienlevert commented Oct 22, 2020

Uh oh!

sebastienlevert commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastienlevert commented Oct 22, 2020

Uh oh!

waldekmastykarz commented Oct 22, 2020

Uh oh!

waldekmastykarz commented Oct 24, 2020

Uh oh!

sebastienlevert commented Oct 24, 2020

Uh oh!

waldekmastykarz 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!

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!

waldekmastykarz commented Oct 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sebastienlevert commented Oct 21, 2020 •

edited

Loading

sebastienlevert commented Oct 22, 2020 •

edited

Loading