-
Notifications
You must be signed in to change notification settings - Fork 329
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
Adds new command site membership list
Closes #5980
#6087
Conversation
f5d3dd8
to
cdddadf
Compare
Thank you @mkm17, we'll try to review it ASAP! |
cdddadf
to
38f1b08
Compare
Here, the same change mentioned in this code review will be applied: using spo.getSiteId() to get the site ID in admin mode. |
Hey @mkm17, there seem to be some conflicts. Could you take a look at them? |
38f1b08
to
bc2e96e
Compare
5066cf0
to
f05b5a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool command @mkm17! Right now it's a bit clearer to me what the difference is between the spo web get
variant.
Anyway, could you have a look at the remarks I made?
src/m365/spo/commands/tenant/tenant-site-membership-list.spec.ts
Outdated
Show resolved
Hide resolved
f05b5a8
to
3757bba
Compare
Thank you @milanholemans for the review, I have made the requested changes, and added some comments :) |
3757bba
to
ae6887f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're making good progress. Let's change a few more things before we continue.
src/m365/spo/commands/tenant/tenant-site-membership-list.spec.ts
Outdated
Show resolved
Hide resolved
src/m365/spo/commands/tenant/tenant-site-membership-list.spec.ts
Outdated
Show resolved
Hide resolved
src/m365/spo/commands/tenant/tenant-site-membership-list.spec.ts
Outdated
Show resolved
Hide resolved
Hi @milanholemans, apologies for the delay in introducing the changes. I won't have access to a PC this weekend, but I’ll aim to send the updates at the beginning of next week. |
Thanks for the update @mkm17, take your time 😊 |
42e296c
to
d6694bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to do another review. Let's change a few more things.
src/m365/spo/commands/tenant/tenant-site-membership-list.spec.ts
Outdated
Show resolved
Hide resolved
d6694bc
to
23fb121
Compare
Hi @mkm17, Could you please resolve the conflicts and rebase them with the latest main? |
9938294
to
cedabf9
Compare
@milanholemans ok done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ready to be shipped!
throw 'Invalid request'; | ||
}); | ||
|
||
sinon.stub(spo, 'getSiteAdminPropertiesByUrl').resolves({ SiteId: siteId } as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to use this in almost every test, let's move it to the before
hook so we avoid repetition.
} | ||
|
||
public get description(): string { | ||
return `Retrieve information about default site groups' membership`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make the same edit in the docs.
return `Retrieve information about default site groups' membership`; | |
return `Retrieves information about default site groups' membership`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@milanholemans, Ok I have changed these last two things. Thanks once again!
cedabf9
to
0df6d49
Compare
Merged manually, thanks for the new command! |
Adds new command
site membership list
Closes #5980Closes #5980