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

Microsoft Graph v1.0: ‘teams team list‘ #2628

Closed
wants to merge 2 commits into from

Conversation

dips365
Copy link
Contributor

@dips365 dips365 commented Aug 2, 2021

@garrytrinder garrytrinder self-assigned this Aug 2, 2021
@garrytrinder garrytrinder changed the title Commit Microsoft Graph v1.0: ‘teams team list‘ Aug 2, 2021
@garrytrinder garrytrinder marked this pull request as draft August 2, 2021 05:24
@arjunumenon
Copy link
Member

Hey @dips365 - I will have a look into the failing test cases and will update you on the same.

@garrytrinder - Assigning it to myself. Hope you are fine with that.

const id: string = (<string>opts.url).substring((<string>opts.url).lastIndexOf(`/`) + 1);
return Promise.resolve({
"@odata.context": "https://graph.microsoft.com/beta/$metadata#teams/$entity",
return Promise.resolve([{
Copy link
Member

Choose a reason for hiding this comment

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

This test case is failing because, the function in actual resolves JSON object in the original method. In your case, you are returning JSON array and hence it is causing the subsequent method to fail. Rather than you returning JSON array, you can return JSON object and this will work without any issue. So your code may look something like below.

else if ((opts.url as string).startsWith(`https://graph.microsoft.com/v1.0/teams/`)) {
        const id: string = (<string>opts.url).substring((<string>opts.url).lastIndexOf(`/`) + 1);
        return Promise.resolve({
          "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#teams/$entity",
          "id": id,
          "webUrl": "https://teams.microsoft.com/l/team/19:a5c6eccad3fb401997756a1501d561aa%40thread.skype/conversations?groupId=8090c93e-ba7c-433e-9f39-08c7ba07c0b3&tenantId=dcd219dd-bc68-4b9b-bf0b-4a33a796be35",
          "isArchived": false,
          "memberSettings": {
            "allowCreateUpdateChannels": true,
            "allowDeleteChannels": true,
            "allowAddRemoveApps": true,
            "allowCreateUpdateRemoveTabs": true,
            "allowCreateUpdateRemoveConnectors": true
          },
          "guestSettings": {
            "allowCreateUpdateChannels": false,
            "allowDeleteChannels": false
          },
          "messagingSettings": {
            "allowUserEditMessages": false,
            "allowUserDeleteMessages": false,
            "allowOwnerDeleteMessages": false,
            "allowTeamMentions": true,
            "allowChannelMentions": true
          },
          "funSettings": {
            "allowGiphy": true,
            "giphyContentRating": "moderate",
            "allowStickersAndMemes": true,
            "allowCustomMemes": false
          }
        });
      }

Totally understand the confusion for you in this case since you are having multiple promises in the original method. Eventhough promises are multiple, all it returns is a single object and all we need to do is mock it as a Promise.resolve of single JSON object.

Hope it is clear for you now

@@ -421,7 +438,7 @@ describe(commands.TEAM_LIST, () => {

it('lists all properties for output json', (done) => {
sinon.stub(request, 'get').callsFake((opts) => {
if (opts.url === `https://graph.microsoft.com/beta/groups?$filter=resourceProvisioningOptions/Any(x:x eq 'Team')&$select=id,displayName,description`) {
if (opts.url === `https://graph.microsoft.com/v1.0/groups?$select=id,displayName,description,resourceProvisioningOptions`) {
return Promise.resolve({
"value": [
Copy link
Member

Choose a reason for hiding this comment

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

As per the updated API, the response should have the key resourceProvisioningOptions along with existing properties.

Hence you may have to update the Fake Promise with the update which would be something like below.

        return Promise.resolve({
          "value": [
            {
              "id": "02bd9fd6-8f93-4758-87c3-1fb73740a315",
              "description": "Team 1 description",
              "displayName": "Team 1",
              "resourceProvisioningOptions": ["Team"]
            },
            {
              "id": "13be6971-79db-4f33-9d41-b25589ca25af",
              "description": "Team 2 description",
              "displayName": "Team 2",
              "resourceProvisioningOptions": ["Team"]
            },

Guess you had already done that in the other methods. Might have missed in this particular method. Nothing major.. That happens. 👍

@arjunumenon
Copy link
Member

Hello @dips365 - You have already completed most of the test cases. Good job on that👏

For the 2 failing tests, I have updated my comments. Once you update that and initiate PR, test cases along with coverage should work expected. Let me know if you have any questions.

@arjunumenon arjunumenon removed their assignment Aug 5, 2021
@dips365
Copy link
Contributor Author

dips365 commented Aug 5, 2021

@arjunumenon Thanks for review !! I will update it and let you know if needs any more clarification

@dips365
Copy link
Contributor Author

dips365 commented Aug 9, 2021

@arjunumenon Thanks much !!

@dips365 dips365 marked this pull request as ready for review August 9, 2021 12:25
@arjunumenon
Copy link
Member

It is good that the checks work as expected. Good job @dips365 .

@dips365 dips365 closed this Aug 18, 2021
@dips365 dips365 reopened this Aug 18, 2021
@dips365 dips365 mentioned this pull request Aug 19, 2021
@garrytrinder garrytrinder self-assigned this Aug 27, 2021
@garrytrinder
Copy link
Member

Thank you @dips365 great work! 👏

Manually merged 🚀

@garrytrinder garrytrinder added this to the v3.13 milestone Aug 27, 2021
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.

Microsoft Graph v1.0: teams team list
3 participants