Cli-microsoft365: New command: get information about the specified list

[waldekmastykarz]

Get information about the specified list

• command: spo list get -u|--webUrl <webUrl> -i|--id [id] -t|--title [title]
• webUrl: URL of the site where the list to retrieve is located
• id: List id
• title: List title
• specify either id or title but not both

Retrieve list using GET <webUrl>/_api/web/lists(guid'<id>') or GET <webUrl>/_api/web/lists/GetByTitle('<title>'). Ensure you have access token for the specified web.

[waldekmastykarz]

As requested: all yours @rfjschouten 💪

[rfjschouten]

@waldekmastykarz I published my branch to my forked project. The command is implemented and works. I only have one test which is failing at the moment and I don't see what's the problem. Could you take a look?
https://github.com/rfjschouten/office365-cli/tree/getlistinformation

[VelinGeorgiev]

Hey @rfjschouten , I checked the issue and it is related to sinon.
TypeError: Attempted to wrap ensureAccessToken which is already wrapped.

Basically when you create sinon spy or sinon stub wrapper on a function/method, you have to restore it or remove that wrapper so the same function/method can be wrapped in other command tests suit.

In your case you are wrapping

 before(() => {
   ...
    sinon.stub(auth, 'ensureAccessToken').callsFake(() => { return Promise.resolve('ABC'); });
    ...
  });

but that ensureAccessToken has never been restored so the next tests suite fails.

Here is what you have to do to fix it. Change:

  after(() => {
    Utils.restore([
      appInsights.trackEvent,
      auth.getAccessToken,
      auth.restoreAuth,
      request.get
    ]);
  });

to

  after(() => {
    Utils.restore([
      appInsights.trackEvent,
      auth.ensureAccessToken,
      auth.restoreAuth,
      request.get
    ]);
  });

That should restore (release) the method and let other test suits to create wrapper around it.

[rfjschouten]

Hi @VelinGeorgiev thanks for taking a look at the code. Your change is resolving the problem indeed where the next test was failing due to sinon. I still have 1 test which is failing on an assert which I don't see why it is failing:

1) spo list get
retrieves and prints all details user custom actions if title option is passed:

  AssertionError [ERR_ASSERTION]: false == true
  + expected - actual

  -false
  +true

[waldekmastykarz]

On another note @rfjschouten, I had a quick look at your code and here are a few things that we should adjust:

• In https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L42 you're using ensureAccessToken. Because the command allows specifying the URL of the web on which the operation will be performed, you should use getAccessToken instead. More information about this is available at https://sharepoint.github.io/office365-cli/concepts/authorization-tokens/
• request options for retrieving the list by ID and title are the same except for the URL. It would be better to conditionally set the URL and have only one instance of the request options @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L63-L84
• there is no need to wrap request.get in a promise. Since we're in a promise chain, you can simply return request.get and the promise will be handled further down the chain @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L92-L100
• since you're printing out the complete listInstance object without any additional manipulation, you can pass the whole object directly to the log, like cmd.log(listInstance) rather than creating a new object and reassigning all properties @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L111-L161
• when resolving the promise, you're checking if the list has been returned @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L110. If the list isn't found, are you getting a 200 request back from SharePoint or will instead the promise be rejected with a 404 response which is already handled @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L165?
• since you're setting json:true in your request options, you should use handleRejectedODataJsonPromise which is specific for JSON requests rather than the generic handleRejectedODataPromise @ https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/list/list-get.ts#L165
• since your command has optional parameters, we want to be able to track which parameters people use when working with the CLI. For that you can override the getTelemetryProperties method. The important part is that we don't want to track the values that people use, just which parameters. See for example https://github.com/rfjschouten/office365-cli/blob/8d335aeec22677bd165625e837fcc15a8a729307/src/o365/spo/commands/app/app-deploy.ts#L38.

[waldekmastykarz]

@rfjschouten regarding your failing test: have you compared if the objects are identical? In your code, you can use console.log(log) to see which objects have been passed to the log and compare their values with the expected object in your test.

[rfjschouten]

@waldekmastykarz Thanks for your review. I have changed the code with your suggestions. My API call had an accept header with verbose in it, which caused a response which was put in a 'd' variable. In my browser I did the same call but with accept nometadata. This was causing a difference in the input/output in the test. I have changed this and have 100% code coverage and test results. I will do a PR of my code.

[rfjschouten]

I created PR 225 for this. I have 1 branche conflict on Commands.ts file. Not sure how this happened. I have gone through all documentation steps...

[waldekmastykarz]

No worries, I'll fix it when merging the PR 👍

[waldekmastykarz]

Meanwhile, care for taking another one @rfjschouten? 😄

[rfjschouten]

@waldekmastykarz Yes, off course! I will take a look in the issue list and let you know...

[waldekmastykarz]

Excellent @rfjschouten! Appreciate your help 👏