New command: spo site archive. Closes #6018 #6031
Conversation
|
Thank you @Saurabh7019! We'll try to review it ASAP! |
There was a problem hiding this comment.
Cool stuff @Saurabh7019! I made a few comments while reviewing your code. Could you take a look at them, please?
|
|
||
| it('correctly handles API error', async () => { | ||
| sinon.stub(request, 'post').callsFake(async (opts) => { | ||
| if ((opts.url as string).indexOf(`/_vti_bin/client.svc/ProcessQuery`) > -1) { |
There was a problem hiding this comment.
In this case, it's not really needed to check for a URL. Since we just want to test if the message was logged correctfully.
So let's use sinon.stub(request, 'post').rejects({errorObject}).
There was a problem hiding this comment.
I have removed the URL check from the test. I am using callsFake to resolve with a response that contains an ErrorInfo object to test this part:
if (response.ErrorInfo) {
throw response.ErrorInfo.ErrorMessage;
}
There was a problem hiding this comment.
Yes, it's entirely fine to add a test for this. But in this test we don't really care if we passed the right URL (that's already tested in other tests). In this test we only care that if we catch an error, that it's properly displayed.
Therefore I think we can remove the URL check.
| @@ -247,6 +247,7 @@ export default { | |||
| SITE_APPPERMISSION_LIST: `${prefix} site apppermission list`, | |||
| SITE_APPPERMISSION_REMOVE: `${prefix} site apppermission remove`, | |||
| SITE_APPPERMISSION_SET: `${prefix} site apppermission set`, | |||
| SITE_ARCHIVE: `${prefix} site archive`, | |||
There was a problem hiding this comment.
Thinking of it some more, I think this command should be called spo tenant site archive. Let's discuss this on the issue.
There was a problem hiding this comment.
I agree. I will wait for the discussion to conclude before making this change and marking the PR as ready.
Closes #6018