Export option for teams report useractivityuserdetail [options] by sprider · Pull Request #1061 · pnp/cli-microsoft365

sprider · 2019-08-16T03:48:39Z

I have completed the export logic based on the comments provided by you at issue #1038.

Let us start the review and I can work on your feedbacks in parallel. Also I will work on code coverage items at teams-report-useractivityuserdetail.spec.ts file once we finish the review for teams-report-useractivityuserdetail.ts and report-useractivityuserdetail.md files.

Thank you.

VelinGeorgiev

@sprider , the tests are failing.
I have added some initial comments for you. Thanks for looking into this.

VelinGeorgiev · 2019-08-16T05:20:47Z

 `-p, --period <period>`|The length of time over which the report is aggregated. Supported values `D7, D30, D90, D180`. Specify the `period` or `date`, but not both.
 `-d, --date [date]`|The date for which you would like to view the users who performed any activity. Supported date format is `YYYY-MM-DD`. Specify the `date` or `period`, but not both.
+`-f, --outputFile [outputFile]`|Path to the file where the upgrade report should be stored in.
+`-o, --output [output]`|Output type. `csv|json|text`. Default `csv`


The output option is there for every command and by default is text. I would suggest you keep it the same as the rest of the commands where it defaults to text and the options are in the following order text,json,csv. See how Waldek did it here: https://github.com/pnp/office365-cli/pull/1037/files

I have fixed this in the latest commit.

VelinGeorgiev · 2019-08-16T05:23:26Z

  period?: string;
  date?: string;
+  outputFile?: string;
+  output?: string;


There is no need to introduce new output option since it is already introduced by the parent class, you just have to reuse it. See https://github.com/pnp/office365-cli/pull/1037/files

I have fixed this in the latest commit.

VelinGeorgiev · 2019-08-16T05:23:42Z

    telemetryProps.period = typeof args.options.period !== 'undefined';
    telemetryProps.date = typeof args.options.date !== 'undefined';
+    telemetryProps.outputFile = typeof args.options.outputFile !== 'undefined';
+    telemetryProps.output = typeof args.options.output !== 'undefined';


There is no need to log telemetry for output since it is already captured by the parent class. See https://github.com/pnp/office365-cli/pull/1037/files

I have fixed this in the latest commit.

VelinGeorgiev · 2019-08-16T05:26:09Z

+
+          let content: string = '';
+
+          if (args.options.outputFile.toLowerCase().endsWith('.json'))


You should check if the --output option is set to json instead of checking for extension.

I have fixed this in the latest commit. Just an FYI here user can export the report data to the specified path in csv,txt and json formats

VelinGeorgiev · 2019-08-16T05:29:13Z

      .then((res: any): void => {
-        cmd.log(res);
-
+        if (args.options.outputFile) {


The --outputFile check should come after checking the --output option. We should be able to print json on the screen in case of json being specified in the --output and not --outputFile specified. We should be also able to save that json to a file in case --outputFile and --outputFile is specified as well. Something similar is done by Waldek here: https://github.com/pnp/office365-cli/pull/1037/files

I have fixed this in the latest commit. Just an FYI here user can export the report data to the specified path in csv,txt and json formats

VelinGeorgiev · 2019-08-16T05:31:28Z

+        return 'Specified outputFile path where to save the file does not exist';
+      }
+
+      if (args.options.outputFile && args.options.output && args.options.output.toLowerCase() === 'csv' && !args.options.outputFile.toLowerCase().endsWith('.csv')) {


I do not think the extension matters as long as the have --output option specified.

I have fixed this in the latest commit but do we need to validate the below scenario where output is csv and path ends with json

teams report useractivityuserdetail --period D7 --output csv --outputFile '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json'

My opinion is that we do not have to validate the extension nor the name for the specified file. The user can name it whatever, he/she will get content in json format when the --output option is set to json.

Sure, Thank you for the clarification.

VelinGeorgiev · 2019-08-16T05:31:38Z

+        return 'Specified outputFile path should be csv file extension';
+      }
+
+      if (args.options.outputFile && args.options.output && args.options.output.toLowerCase() === 'json' && !args.options.outputFile.toLowerCase().endsWith('.json')) {


I do not think the extension matters as long as the have --output option specified.

I have fixed this in the latest commit but do we need to validate the below scenario where output is csv and path ends with json

teams report useractivityuserdetail --period D7 --output csv --outputFile '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json'

My opinion is that we do not have to validate the extension nor the name for the specified file. The user can name it whatever, he/she will get content in json format when the --output option is set to json.

Sure, Thank you for the clarification.

VelinGeorgiev · 2019-08-16T05:31:44Z

+        return 'Specified outputFile path should be json file extension';
+      }
+
+      if (args.options.outputFile && args.options.output && args.options.output.toLowerCase() === 'text' && !args.options.outputFile.toLowerCase().endsWith('.txt')) {


I do not think the extension matters as long as the have --output option specified.

I have fixed this in the latest commit but do we need to validate the below scenario where output is csv and path ends with json

teams report useractivityuserdetail --period D7 --output csv --outputFile '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json'

VelinGeorgiev · 2019-08-16T05:32:34Z

      for July 13, 2019
      ${commands.TEAMS_REPORT_USERACTIVITYUSERDETAIL} --date 2019-07-13
+
+      Gets details about Microsoft Teams user activity by user for the last week and exports the report data in the specified path in csv format


Seems like you've build functionality for json output/export, but your example is with csv. Could you please align?

I have fixed this in the latest commit.

VelinGeorgiev · 2019-08-16T05:32:53Z

+Gets details about Microsoft Teams user activity by user for the last week and exports the report data in the specified path in csv format
+
+```sh
+teams report useractivityuserdetail --period D7 --outputFile '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.csv'


Seems like you've build functionality for json output/export, but your example is with csv. Could you please align?

I have fixed this in the latest commit.

sprider · 2019-08-16T12:58:31Z

@VelinGeorgiev
Please guide me on the unit test case for this method as well.

VelinGeorgiev · 2019-08-16T17:51:58Z

Joseph you will have to do a test with at least two rows of data to cover those lines

sprider · 2019-08-17T15:39:20Z

Joseph you will have to do a test with at least two rows of data to cover those lines

@VelinGeorgiev Sure, this is done.

sprider · 2019-08-17T15:47:14Z

@VelinGeorgiev
This is complete and ready for your review but ci/circleci: test shows failed.

My local npm test shows 100%. Do I need to modify anything?

VelinGeorgiev · 2019-08-17T16:02:40Z

Seems line 74 fails on the build machine. https://circleci.com/gh/pnp/office365-cli/2409?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

You can do npm run clean + npm run build + npm t on all the tests to see if fresh build might not fail on your pc.

sprider · 2019-08-17T17:10:52Z

Seems line 74 fails on the build machine. https://circleci.com/gh/pnp/office365-cli/2409?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

You can do npm run clean + npm run build + npm t on all the tests to see if fresh build might not fail on your pc.

@VelinGeorgiev
I did a fresh build and I am getting 100% and all looks good.

If I am not wrong, circleci is trying to find the path "/Users/josephvelliah/Desktop" to save the file

Please let me know how to proceed or mock this.

VelinGeorgiev · 2019-08-18T16:28:32Z

Joseph @sprider , to fix that you should fake the fs.writeFileSync function to return something instead of actually try to read from the hard drive. There is an example how to fake it here in these tests https://github.com/pnp/office365-cli/blob/c13ba0388ebc1579a6ae727783350bb4466e3cd1/src/o365/azmgmt/commands/flow/flow-export.spec.ts#L211

Have a look how Waldek did it and you can try to do it the same way.

coveralls · 2019-08-21T12:35:37Z

Coverage remained the same at 100.0% when pulling d177723 on sprider:exportteamsuseractivity into 6ce20cb on pnp:dev.

sprider · 2019-08-21T12:36:01Z

Joseph @sprider , to fix that you should fake the fs.writeFileSync function to return something instead of actually try to read from the hard drive. There is an example how to fake it here in these tests

https://github.com/pnp/office365-cli/blob/c13ba0388ebc1579a6ae727783350bb4466e3cd1/src/o365/azmgmt/commands/flow/flow-export.spec.ts#L211

Have a look how Waldek did it and you can try to do it the same way.

@VelinGeorgiev I have fixed the issues as per your suggestion. Could you please review and merge this? Thank you.

VelinGeorgiev

@sprider very well done! I tested the command and everything worked. This is the branch from where we will merge it.

I had to make some cosmetic changes. You can find my comments above.

VelinGeorgiev · 2019-08-23T11:10:11Z

+        if(args.options.outputFile)
+        {
+          fs.writeFileSync(args.options.outputFile, content, 'binary');
+          cmd.log(`File saved to path '${args.options.outputFile}'`);


I added extra condition so that message will show in verbose mode only so the code looks like:

if (this.verbose) { cmd.log(`File saved to path '${args.options.outputFile}'`); }

This follows the same pattern we have in other commands like file-get

VelinGeorgiev · 2019-08-23T11:19:00Z

+        {
+          let reportdata = this.getJsonReport(res);
+          content = JSON.stringify(reportdata);
+          cmd.log(reportdata);


I added extra condition so that log should show in case user has not specified outputFile so the code looks like:

if (!args.options.outputFile) { cmd.log(reportdata); }

This follows the same pattern we have in other commands like file-get where no output on the screen is shown, but just file is generated.

VelinGeorgiev · 2019-08-23T11:33:42Z

      ${commands.TEAMS_REPORT_USERACTIVITYUSERDETAIL} --date 2019-07-13
+
+      Gets details about Microsoft Teams user activity by user for the last week and exports the report data in the specified path in json format
+      ${commands.TEAMS_REPORT_USERACTIVITYUSERDETAIL} --period D7 --output json --outputFile '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json'


/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json changed to C:/report.json to shorten the message for smaller screens.

VelinGeorgiev · 2019-08-23T12:29:07Z

+
+        if(args.options.outputFile)
+        {
+          fs.writeFileSync(args.options.outputFile, content, 'binary');


binary changed to utf8 since when exported to CSV the report had weird char in the first column

VelinGeorgiev · 2019-08-23T13:02:55Z

+        else
+        {
+          content = res;
+          cmd.log(content);


I added extra condition so that log should show in case user has not specified outputFile so the code looks like:

if (!args.options.outputFile) { cmd.log(content); }

This follows the same pattern we have in other commands like file-get where no output on the screen is shown, but just file is generated.

VelinGeorgiev · 2019-08-23T13:09:38Z

      Gets details about Microsoft Teams user activity by user 
      for July 13, 2019
      ${commands.TEAMS_REPORT_USERACTIVITYUSERDETAIL} --date 2019-07-13
+


I added another example showing how to export in csv as well

Gets details about Microsoft Teams user activity by user for the last week and exports the report data in the specified path in csv format ${commands.TEAMS_REPORT_USERACTIVITYUSERDETAIL} --period D7 --output csv --outputFile 'C:/report.csv'

VelinGeorgiev · 2019-08-23T13:12:32Z

+
+      return Promise.reject('Invalid request');
+    });
+    sinon.stub(fs, 'writeFileSync').callsFake(writeFileSyncFake);


I assigned that stub to a var so I can tested later like

const fileStub: sinon.SinonStub = sinon.stub(fs, 'writeFileSync').callsFake(writeFileSyncFake);

VelinGeorgiev · 2019-08-23T13:13:24Z

+        assert.equal(requestStub.lastCall.args[0].headers["accept"], 'application/json;odata.metadata=none');
+        assert.equal(requestStub.lastCall.args[0].json, true);
+        assert(cmdInstanceLogSpy.calledWith(`File saved to path '/Users/josephvelliah/Desktop/teams-report-useractivityuserdetail.json'`));
+        done();


I added extra assets to check if the writeFileSync is actually been called. This will verify that file will be created.

assert.equal(fileStub.called, true);

VelinGeorgiev · 2019-08-25T20:13:03Z

Thank you Jospeh @sprider ! Merged manually 🚀

sprider · 2019-08-25T22:25:24Z

Thank you Jospeh @sprider ! Merged manually 🚀

Thank you for your guidance @VelinGeorgiev. I will implement the same in other teams report related commands.

initial commit

2c6f779

sprider changed the title ~~initial commit~~ Export option for teams report useractivityuserdetail [options] Aug 16, 2019

VelinGeorgiev suggested changes Aug 16, 2019

View reviewed changes

VelinGeorgiev added the waiting on response label Aug 16, 2019

sprider added 2 commits August 16, 2019 07:44

Fixed partial feedback points

ca750be

Removed output validation

755f1a0

fixed test cases

a332c7a

Fixed the test case for local path validation

d177723

VelinGeorgiev self-assigned this Aug 23, 2019

VelinGeorgiev added work in progress and removed waiting on response labels Aug 23, 2019

VelinGeorgiev approved these changes Aug 23, 2019

View reviewed changes

VelinGeorgiev added review-complete and removed work in progress labels Aug 23, 2019

VelinGeorgiev closed this Aug 25, 2019

sprider mentioned this pull request Aug 25, 2019

Enhancement: Export report data of teams reports for the remaining commands #1075

Closed


		let content: string = '';

		if (args.options.outputFile.toLowerCase().endsWith('.json'))

Conversation

sprider commented Aug 16, 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!

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 Aug 16, 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!

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!

VelinGeorgiev Aug 16, 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!

VelinGeorgiev Aug 16, 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!

sprider commented Aug 16, 2019

Uh oh!

VelinGeorgiev commented Aug 16, 2019

Uh oh!

sprider commented Aug 17, 2019

Uh oh!

sprider commented Aug 17, 2019

Uh oh!

VelinGeorgiev commented Aug 17, 2019

Uh oh!

sprider commented Aug 17, 2019

Uh oh!

VelinGeorgiev commented Aug 18, 2019

Uh oh!

coveralls commented Aug 21, 2019

Uh oh!

sprider commented Aug 21, 2019

Uh oh!

VelinGeorgiev left a comment

Choose a reason for hiding this comment

VelinGeorgiev Aug 16, 2019 •

edited

Loading

VelinGeorgiev Aug 16, 2019 •

edited

Loading

VelinGeorgiev Aug 16, 2019 •

edited

Loading