Adds command spo site recyclebinitem clear, Closes #4724#4736
Adds command spo site recyclebinitem clear, Closes #4724#4736nicodecleyre wants to merge 3 commits intopnp:mainfrom
spo site recyclebinitem clear, Closes #4724#4736Conversation
|
Thank you @nicodecleyre, will review ASAP! |
nicodecleyre
force-pushed
the
spo-site-recyclebinitem-clear
branch
from
6822bf9 to
17a8cc9
Compare
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Great start, I made some comments while reviewing. Could you have a look at them please?
| throw error; | ||
| }); | ||
|
|
||
| await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com', confirm: true } } as any), error.error); |
There was a problem hiding this comment.
Let's wrap the error in a CommandError object.
There was a problem hiding this comment.
same here, can't wrap it in a CommandError object
There was a problem hiding this comment.
This is because your error message is not well structured. It should look to something like this:
const error = {
error: {
'odata.error': {
message: {
value: "The files cannot be removed from the second-stage recycle bin."
}
}
}
};Now the test actually fails when the error message doesn't match. Before this, the test always succeeded, no matter what you entered as a value. This is not really a useful test. Let's try to avoid this.
| : Don't prompt for confirmation. | ||
|
|
||
| --8<-- "docs/cmd/_global.md" | ||
|
|
There was a problem hiding this comment.
I feel like we should add a warning to inform the user again that it's a permanent deletion.
|
|
||
| public async commandAction(logger: Logger, args: CommandArgs): Promise<void> { | ||
| if (this.verbose) { | ||
| logger.logToStderr(`Permanently removing all items in recycle bin of site ${args.options.siteUrl}...`); |
There was a problem hiding this comment.
Let's move this log into the function when the prompt is confirmed.
| const error = { | ||
| 'odata.error': { | ||
| message: { | ||
| value: "The files cannot be moved to the second-stage recycle bin." |
There was a problem hiding this comment.
Error message looks odd for a command that permanently removes items.
| throw error; | ||
| }); | ||
|
|
||
| await assert.rejects(command.action(logger, { options: { siteUrl: 'https://contoso.sharepoint.com', confirm: true } } as any), error.error); |
There was a problem hiding this comment.
This is because your error message is not well structured. It should look to something like this:
const error = {
error: {
'odata.error': {
message: {
value: "The files cannot be removed from the second-stage recycle bin."
}
}
}
};Now the test actually fails when the error message doesn't match. Before this, the test always succeeded, no matter what you entered as a value. This is not really a useful test. Let's try to avoid this.
milanholemans
left a comment
milanholemans
left a comment
There was a problem hiding this comment.
Hi @nicodecleyre first and for all sorry for the previous single reviews. GitHub was doing quite weird.
Anyhow, code looks good! Made few small changes before merging.
| } | ||
|
|
||
| await request.post(requestOptions); | ||
| } |
There was a problem hiding this comment.
Seems like this is one of the stupid APIs that never return a failed status code. Even when the API fails, it returns a 200. We should check the response body for errors.
No problem, thank you for making these last changes ❤️ |
|
Merged manually, thank you for this PR! 👏 |
2 participants
Closes #4724