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.

Sign up for GitHub

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

Jump to bottom

make spo user get command return current user if no arguments are provided #5514

Closed
wants to merge 3 commits into from
Closed

make spo user get command return current user if no arguments are provided #5514

wants to merge 3 commits into from

Conversation

Vedu1996
Copy link
Contributor

@Vedu1996 Vedu1996 commented Sep 23, 2023 •
edited
Loading

Fixes #5513

@Vedu1996 Vedu1996 mentioned this pull request Sep 23, 2023
Closed
@Adam-it
Copy link
Member

Adam-it commented Sep 23, 2023

Awesome 👍
We will review it ASAP

@milanholemans milanholemans self-assigned this Sep 23, 2023
milanholemans
milanholemans requested changes Sep 24, 2023
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Hi @Vedu1996 great start so far.
I've made a few comments along the way, could you have a look at them, please?

Also, let's add an example to the docs so people know how they can use this enhancement.

src/m365/spo/commands/user/user-get.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/user/user-get.spec.ts Outdated Show resolved Hide resolved
src/m365/spo/commands/user/user-get.spec.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft September 24, 2023 14:45
@Vedu1996 Vedu1996 marked this pull request as ready for review September 25, 2023 09:48
@Vedu1996 Vedu1996 marked this pull request as draft September 25, 2023 09:48
@Vedu1996 Vedu1996 marked this pull request as ready for review September 25, 2023 10:00
@milanholemans
Copy link
Contributor

milanholemans commented Sep 25, 2023 •
edited
Loading

Hi, @Vedu1996 seems like this PR has some merge conflicts. Could you rebase it with the latest main please? When you are done, you can hit the 'Ready for review' button.
Thanks!

P.S. I also noticed you forgot to add the example to the docs page.

@Vedu1996 Vedu1996 marked this pull request as draft September 26, 2023 04:34
@Vedu1996 Vedu1996 marked this pull request as ready for review September 26, 2023 04:35
@Vedu1996
Copy link
Contributor Author

Hi, @Vedu1996 seems like this PR has some merge conflicts. Could you rebase it with the latest main please? When you are done, you can hit the 'Ready for review' button. Thanks!

P.S. I also noticed you forgot to add the example to the docs page.

Hey @milanholemans , I've rebased. Added an example in docs as well. Thanks!

@milanholemans
Copy link
Contributor

Hey @milanholemans , I've rebased. Added an example in docs as well. Thanks!

Thanks, will do another approval round soon!

@milanholemans
Copy link
Contributor

Hi @Vedu1996, seems like the code coverage of the file didn't succeed. Could you have a look at it?

@Vedu1996
Copy link
Contributor Author

Hi @Vedu1996, seems like the code coverage of the file didn't succeed. Could you have a look at it?

Screenshot 2023-09-26 at 1 22 35 PM I ran the coverage locally, and for some reason it is failing in fsUtil.ts, and then the other files are not tested for coverage. Could you let me know how can I fix it please?

@milanholemans
Copy link
Contributor

Hi @Vedu1996, seems like the code coverage of the file didn't succeed. Could you have a look at it?

Screenshot 2023-09-26 at 1 22 35 PM I ran the coverage locally, and for some reason it is failing in fsUtil.ts, and then the other files are not tested for coverage. Could you let me know how can I fix it please?

Yeah, you can totally ignore the fsutils. Just pay attention to the files you modified.
Are you sure the changes here in the cloud are in sync with the changes locally on your PC? It seems like we are missing a test where we mock the currentuser endpoint.
Did you build your code before running the tests locally?

@Vedu1996
Copy link
Contributor Author

Vedu1996 commented Sep 26, 2023 •
edited
Loading

Hi @Vedu1996, seems like the code coverage of the file didn't succeed. Could you have a look at it?

Screenshot 2023-09-26 at 1 22 35 PM I ran the coverage locally, and for some reason it is failing in fsUtil.ts, and then the other files are not tested for coverage. Could you let me know how can I fix it please?

Yeah, you can totally ignore the fsutils. Just pay attention to the files you modified. Are you sure the changes here in the cloud are in sync with the changes locally on your PC? It seems like we are missing a test where we mock the currentuser endpoint. Did you build your code before running the tests locally?

I'll build the app and have a look, thanks

@Vedu1996
Copy link
Contributor Author

Vedu1996 commented Sep 26, 2023 •
edited
Loading

It seems like we are missing a test where we mock the currentuser endpoint. Did you build your code before running the tests locally?

My bad, didnt run the build before testing. The missing test was actually because I messed up my rebase, I fixed it now.
Sorry, and thanks :)

@Vedu1996
Copy link
Contributor Author

Vedu1996 commented Oct 8, 2023

Hey, is there something I'm missing on my side? Just checking because I just noticed this PR hasnt been merged yet :)

@Jwaegebaert
Copy link
Contributor

Hey @Vedu1996, pardon for the wait. We currently have a heavy load of PRs to process so we'll try our best to process this one as soon as we can.

@Vedu1996
Copy link
Contributor Author

Vedu1996 commented Oct 8, 2023 via email

@milanholemans
Copy link
Contributor

Hi @Vedu1996, like Jasey mentioned, we had quite a hectic period because of our new major release. Additionally, I haven't had much time in recent weeks. I will try to take a look at it soon.

milanholemans
milanholemans approved these changes Oct 9, 2023
View reviewed changes
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Great work! Looks good to me! Thank you for your contribution!

I rephrased one small thing while merging.

docs/docs/cmd/spo/user/user-get.mdx
@@ -49,6 +49,11 @@ Get user by login name for a web
```sh
m365 spo user get --webUrl https://contoso.sharepoint.com/sites/project-x --loginName "i:0#.f|membership|[email protected]"
```
Get user logged-in currently
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Get user logged-in currently
Get the currently logged-in user

@milanholemans
Copy link
Contributor

Thank you very much for this enhancement @Vedu1996! It was a pleasure working with you and we hope to see you again on this project.
You're now officially part of the contributors' team, welcome! https://pnp.github.io/cli-microsoft365/about/team

@Vedu1996
Copy link
Contributor Author

Vedu1996 commented Oct 9, 2023

Hey @milanholemans , thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milanholemans milanholemans milanholemans approved these changes

Assignees

@milanholemans milanholemans

Labels
pr-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Enhancement]: Get current SP user
5 participants
@Vedu1996 @Adam-it @milanholemans @Jwaegebaert @vlakshminarayanan