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

💡New command : tenant report settings set - closes #6247 #6548

Conversation

NishkalankBezawada
Copy link
Contributor

@NishkalankBezawada NishkalankBezawada commented Jan 5, 2025

New command : tenant report settings set - closes #6247

Explanation

This command can be used to update (adminReportSettings) tenant-level settings for Microsoft 365 reports.
Please refer to the Graph documentation page.

Usage

m365 tenant report settings set [options]

test screens

Below has set --hideUserInformation to false
image

below is the screen from the Admin center

image

Below has set --hideUserInformation to true

image

below is the screen from the Admin center

image

Below screen shows further validations

image

Thanks,
Nish

@milanholemans
Copy link
Contributor

Thanks @NishkalankBezawada, we'll try to review it ASAP.

@martinlingstuyl
Copy link
Contributor

Hi @NishkalankBezawada, I ran the tests that we usually do on PR's. There's a couple of issue though, or so it appears. Could you like at them and fix them in your code? You may want to run npm run test afterwards, to ensure the tests are running OK on your machine.

Let me know if you have questions.

@martinlingstuyl martinlingstuyl marked this pull request as draft January 10, 2025 14:29
@martinlingstuyl martinlingstuyl self-assigned this Jan 10, 2025
@NishkalankBezawada
Copy link
Contributor Author

Hi @NishkalankBezawada, I ran the tests that we usually do on PR's. There's a couple of issue though, or so it appears. Could you like at them and fix them in your code? You may want to run npm run test afterwards, to ensure the tests are running OK on your machine.

Let me know if you have questions.

Hey @martinlingstuyl

Thanks for reviewing. Since this is my first contribution to the repository, I might have missed a few things. I will surely check on this in the evening, and send in for your review.

Thanks,
Nish

@martinlingstuyl
Copy link
Contributor

Yes it's me 😄!

You can always check our contribution documentation, which is quite good. Also when it's about unit tests.
https://pnp.github.io/cli-microsoft365/contribute/contributing-guide

If you've got any questions, let me know 🚀

@NishkalankBezawada
Copy link
Contributor Author

Yes it's me 😄!

You can always check our contribution documentation, which is quite good. Also when it's about unit tests. https://pnp.github.io/cli-microsoft365/contribute/contributing-guide

If you've got any questions, let me know 🚀

Hey @martinlingstuyl, This pretty much helped me.

Will be sending a new review soon 🚀

Thanks,
Nish

@NishkalankBezawada NishkalankBezawada marked this pull request as ready for review January 11, 2025 18:19
@NishkalankBezawada
Copy link
Contributor Author

NishkalankBezawada commented Jan 11, 2025

Hey @martinlingstuyl,

ran the tests 😄 and also updated validations. now ready for your review 🚀

Thanks,
Nish

@NishkalankBezawada NishkalankBezawada marked this pull request as draft January 11, 2025 18:54
@NishkalankBezawada NishkalankBezawada marked this pull request as ready for review January 12, 2025 20:25
@NishkalankBezawada NishkalankBezawada changed the title New command : tenant report settings set - closes #6247 💡New command : tenant report settings set - closes #6247 Jan 13, 2025
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi nish, let's look at a couple of comments before we merge this. You've made a good start with this. A lot of my comments are actually just styling. :-)

docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
src/m365/tenant/commands/report/report-settings-set.ts Outdated Show resolved Hide resolved
src/m365/tenant/commands/report/report-settings-set.ts Outdated Show resolved Hide resolved
src/m365/tenant/commands/report/report-settings-set.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft January 13, 2025 21:26
@NishkalankBezawada NishkalankBezawada marked this pull request as ready for review January 15, 2025 17:57
@NishkalankBezawada
Copy link
Contributor Author

NishkalankBezawada commented Jan 15, 2025

Hi nish, let's look at a couple of comments before we merge this. You've made a good start with this. A lot of my comments are actually just styling. :-)

Hey @martinlingstuyl

I'm sorry, I couldnt check this yesterday, was a bit sick. Now I have resolved the suggestions and ran the test again,

image

Thanks,
Nish

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Almost done... some last comments nish! And something we need to discuss from the specs...

docs/docs/cmd/tenant/report/report-settings-set.mdx Outdated Show resolved Hide resolved
src/m365/tenant/commands/report/report-settings-set.ts Outdated Show resolved Hide resolved
src/m365/tenant/commands/report/report-settings-set.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft January 16, 2025 15:32
@NishkalankBezawada NishkalankBezawada marked this pull request as ready for review January 21, 2025 12:44
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Very nice Nish, There are a couple of minor comments open, which I'll fix while merging.

@NishkalankBezawada
Copy link
Contributor Author

Very nice Nish, There are a couple of minor comments open, which I'll fix while merging.

Hey @martinlingstuyl

Do you want me to do these changes? If so, I can do that. Its all about learning.

//Nish

@martinlingstuyl
Copy link
Contributor

Feel free! I'll merge it soon, if the changes aren't done I'll pick them up.

@NishkalankBezawada
Copy link
Contributor Author

Feel free! I'll merge it soon, if the changes aren't done I'll pick them up.

Hey @martinlingstuyl

Just pushed the final changes.

Thanks,
Nish

@martinlingstuyl
Copy link
Contributor

Hi Nish,

thanks, I'll give it a last look.
Just FYI: if you've resolved a comment, do mark the comment as resolved, that makes it easier for me to see whats been fixed.

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Very nice Nish, There are

@martinlingstuyl
Copy link
Contributor

Merged manually, thank you! 🚀

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.

New command: tenant report settings set
3 participants