Skip to content

Conversation

Saurabh7019
Copy link
Contributor

Closes #6863

@Saurabh7019 Saurabh7019 marked this pull request as ready for review October 11, 2025 19:19
@milanholemans
Copy link
Contributor

Thanks @Saurabh7019! We'll try to review it ASAP.

@milanholemans milanholemans self-assigned this Oct 12, 2025
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.

The command is nearly perfect! When the comments below are processed, it looks good to go.

Comment on lines +14 to +17
webUrl: zod.alias('u', z.string())
.refine(url => validation.isValidSharePointUrl(url) === true, url => ({
message: `'${url}' is not a valid SharePoint URL.`
})),
Copy link
Contributor

Choose a reason for hiding this comment

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

The zod.alias should include the refine as well. Otherwise, the alias is not registered correctly and cannot be used.

Suggested change
webUrl: zod.alias('u', z.string())
.refine(url => validation.isValidSharePointUrl(url) === true, url => ({
message: `'${url}' is not a valid SharePoint URL.`
})),
webUrl: zod.alias('u', z.string()
.refine(url => validation.isValidSharePointUrl(url) === true, url => ({
message: `'${url}' is not a valid SharePoint URL.`
}))),

}

public get description(): string {
return 'Removes an alert from a SharePoint site';
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
return 'Removes an alert from a SharePoint site';
return 'Removes an alert from a SharePoint list';


# spo site alert remove

Removes an alert from a SharePoint site
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
Removes an alert from a SharePoint site
Removes an alert from a SharePoint list

await request.delete(requestOptions);
}
catch (err: any) {
this.handleRejectedPromise(err);
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
this.handleRejectedPromise(err);
this.handleRejectedODataJsonPromise(err);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the delay :)

handleRejectedPromise is showing cleaner error instead of raw xml.

with handleRejectedODataJsonPromise, I am getting below:

Error: <?xml version="1.0" encoding="utf-8"?><m:error xmlns:m="http://schemas.microsoft.com/ado/2007/08/dataservices/metadata"><m:code>-2146232832, Microsoft.SharePoint.SPException</m:code><m:message xml:lang="en-US">The alert you are trying to access does not exist or has just been deleted. </m:message></m:error>

should I keep the current one?

});

it('prompts before removing the alert', async () => {
await command.action(logger, { options: commandOptionsSchema.parse({ webUrl: webUrl, id: alertId }) });
Copy link
Contributor

Choose a reason for hiding this comment

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

The parsing of the options is not needed. Let's fix this for other tests as well.

Suggested change
await command.action(logger, { options: commandOptionsSchema.parse({ webUrl: webUrl, id: alertId }) });
await command.action(logger, { options: { webUrl: webUrl, id: alertId } });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will fix it. I was asked to add it on another PR.

});

it('aborts removing the alert when prompt is not confirmed', async () => {
const deleteStub = sinon.stub(request, 'delete').resolves({});
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
const deleteStub = sinon.stub(request, 'delete').resolves({});
const deleteStub = sinon.stub(request, 'delete').resolves();


it('correctly handles random API error', async () => {
sinon.stub(request, 'delete').rejects(
new Error('The alert you are trying to access does not exist or has just been deleted.')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not how a failing API call to SharePoint would look like. Instead, let's make it as realistic as possible with a read failing request body.

: The ID of the alert.

`-f, --force`
: Don't prompt for confirmation
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
: Don't prompt for confirmation
: Don't prompt for confirmation.

@milanholemans milanholemans marked this pull request as draft October 12, 2025 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New command: spo site alert remove

2 participants