Skip to content

Conversation

MathijsVerbeeck
Copy link
Contributor

Closes #6862

@milanholemans milanholemans self-assigned this Oct 14, 2025
@milanholemans milanholemans requested a review from Copilot October 17, 2025 16:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new command spo site alert get to retrieve details of a specific alert from a SharePoint site list, addressing issue #6862.

Key changes:

  • Implements a new command to get SharePoint site alert details by ID
  • Adds comprehensive test coverage with validation scenarios
  • Includes complete documentation with examples and response formats

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/m365/spo/commands/site/site-alert-get.ts Main command implementation with API call to SharePoint alerts endpoint
src/m365/spo/commands/site/site-alert-get.spec.ts Comprehensive test suite covering success, error, and validation scenarios
src/m365/spo/commands.ts Adds command constant for registration
docs/src/config/sidebars.ts Updates documentation navigation
docs/docs/cmd/spo/site/site-alert-get.mdx Complete command documentation with examples
.devproxy/api-specs/sharepoint.yaml API specification for the SharePoint alerts endpoint

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.

Solid work, let's change a few things before we merge it.

Comment on lines +12 to +17
webUrl: zod.alias('u', z.string().refine(url => validation.isValidSharePointUrl(url) === true, {
message: 'Specify a valid SharePoint site URL'
})),
id: z.string().refine(id => validation.isValidGuid(id) === true, {
message: 'Specify a valid GUID'
})
Copy link
Contributor

Choose a reason for hiding this comment

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

For options validation, let's make sure that:

  • Error messages end on a period ..
  • Print the passed value in the error message (like ' is not a valid GUID.'). This makes it easier for folks to debug their scripts.


public async commandAction(logger: Logger, args: CommandArgs): Promise<void> {
const requestOptions: CliRequestOptions = {
url: `${args.options.webUrl}/_api/Web/Alerts/GetById('${args.options.id}')?$expand=List,User,List/Rootfolder&$select=*,List/Id,List/Title,List/Rootfolder/ServerRelativeUrl`,
Copy link
Contributor

Choose a reason for hiding this comment

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

We forgot to expand the Item property. Let's do that as well.
Check the API URL of the list command.

};

try {
const res = await request.get(requestOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the list command, we remove a duplicate ID property at the Item object, let's do this as well here since PowerShell has a hard time parsing it.

});

it('has correct name', () => {
assert.strictEqual(command.name.startsWith(commands.SITE_ALERT_GET), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use an exact equal.

Comment on lines +177 to +183
sinon.stub(request, 'get').callsFake(async (opts) => {
if (opts.url === `${webUrl}/_api/Web/Alerts/GetById('${alertId}')?$expand=List,User,List/Rootfolder&$select=*,List/Id,List/Title,List/Rootfolder/ServerRelativeUrl`) {
throw error;
}

throw 'Invalid request';
});
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, we don't really care if the API URL is correct. We want to keep the test as simple as possible, only to validate if the command correctly shows a decent error message.
So in this case we can just use .rejects(error).

Comment on lines +220 to +226
it('passes validation if the id option is a valid GUID', async () => {
const actual = commandOptionsSchema.safeParse({
webUrl: webUrl,
id: alertId
});
assert.strictEqual(actual.success, true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is the same as the one 2 tests above, isn't it?

Comment on lines +18 to +22
`-u, --webUrl <webUrl>`
: The URL of the SharePoint site

`--id <id>`
: The ID of the alert
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
`-u, --webUrl <webUrl>`
: The URL of the SharePoint site
`--id <id>`
: The ID of the alert
`-u, --webUrl <webUrl>`
: The URL of the SharePoint site.
`--id <id>`
: The ID of the alert.

Comment on lines +76 to +97
"Properties": [
{
"Key": "dispformurl",
"Value": "Documents/Forms/DispForm.aspx",
"ValueType": "Edm.String"
},
{
"Key": "filterindex",
"Value": "0",
"ValueType": "Edm.String"
},
{
"Key": "defaultitemopen",
"Value": "Browser",
"ValueType": "Edm.String"
},
{
"Key": "sendurlinsms",
"Value": "False",
"ValueType": "Edm.String"
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

For a list, we require to only show 1 item of the list to reduce cluttering the docs. Let's update this for other response types as well.


</TabItem>
</Tabs>

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove one empty line at the end of the file.

@milanholemans milanholemans marked this pull request as draft October 17, 2025 16:37
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 get

2 participants