-
Notifications
You must be signed in to change notification settings - Fork 306
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Command - spo roledefinition list #3269
Conversation
Awesome! We'll review it shortly |
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.
Works like a charm! I've done a few small adjustments when merging the PR
## Options | ||
|
||
`-u, --webUrl <webUrl>` | ||
: URL of the site where the role definitions to retrieve are located |
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 can simplify this to URL of the site for which to retrieve role definition
}; | ||
|
||
request | ||
.get<any>(requestOptions) |
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.
The return type for the request shouldn't be any
but rather value: any[]
so that in the res property we get the known value
array.
request | ||
.get<any>(requestOptions) | ||
.then((response: { value: any[] }): void => { | ||
logger.log(response); |
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 should remove the value
array from the response and instead pass the array items directly to the output. This is consistent with other commands
const command: Command = require('./roledefinition-list'); | ||
|
||
describe(commands.ROLEDEFINITION_LIST, () => { | ||
|
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 remove this empty line
|
||
it('passes validation if the webUrl option is a valid SharePoint site URL', () => { | ||
const actual = command.validate({ options: { webUrl: 'https://contoso.sharepoint.com' } }); | ||
assert(actual); |
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.
This should be assert.strictEqual
. With just an assert
the test will pass even if validation failed.
sinon.stub(request, 'get').callsFake((opts) => { | ||
if ((opts.url as string).indexOf('/_api/web/roledefinitions') > -1) { | ||
return Promise.resolve( | ||
[ |
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.
This mock response is incorrect. The array of role definitions should be wrapped in a value
object
Merged manually. Thank you! 馃憦 |
thanks @waldekmastykarz for the fixes 馃ぉ |
New Command - spo roledefinition list
The aim of this PR is to add a new command which will allow to list role definitions from web
Linked Issue
Closes #3237
馃摳 Result
json output
text output