Adds outlook event get command. Closes #7122#7189
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Microsoft Graph-based CLI command to retrieve a specific Outlook calendar event, plus supporting calendar lookup utilities, tests, and documentation.
Changes:
- Introduces
outlook event getcommand with options for user, calendar, and timezone. - Adds
src/utils/calendarhelper for resolving calendars by id/name (with interactive disambiguation). - Adds tests and Docusaurus docs/sidebar entry for the new command.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/calendar.ts | New utility to resolve a user calendar by id or by name (with multi-result prompting). |
| src/utils/calendar.spec.ts | Unit tests for the new calendar utility. |
| src/m365/outlook/commands/event/event-get.ts | Implements the new outlook event get command (builds Graph request, supports timezone preference). |
| src/m365/outlook/commands/event/event-get.spec.ts | Tests for command validation, happy path, and OData error handling. |
| src/m365/outlook/commands.ts | Registers the EVENT_GET command name. |
| docs/src/config/sidebars.ts | Adds the new command to the Outlook docs sidebar. |
| docs/docs/cmd/outlook/event/event-get.mdx | New documentation page for outlook event get. |
| .refine(options => !(options.calendarId && options.calendarName), { | ||
| error: 'Specify either calendarId or calendarName, but not both.' | ||
| }); |
There was a problem hiding this comment.
The schema only prevents specifying both calendarId and calendarName, but it allows specifying neither. In commandAction you always construct a URL that includes /calendars/${calendarId}/..., so when neither option is provided the request URL will contain calendars/undefined and fail. Either require one of calendarId/calendarName in the refined schema, or update commandAction to handle the no-calendar case (for example by calling the /users/{id}/events/{id} endpoint).
There was a problem hiding this comment.
This is a valid comment. We should elso ensure at least one is specified
| async getUserCalendarById(userId: string, calendarId: string, calendarGroupId?: string, properties?: string): Promise<Calendar> { | ||
| let url = `https://graph.microsoft.com/v1.0/users('${userId}')/${calendarGroupId ? `calendarGroups/${calendarGroupId}/` : ''}calendars/${calendarId}`; | ||
|
|
There was a problem hiding this comment.
The URL is built using users('${userId}') without encoding userId. UPNs for guest users commonly contain #EXT#, and an unencoded # will be treated as a URL fragment delimiter and won’t be sent to Microsoft Graph. Consider URL-encoding the userId value when composing the request URL (for example using formatting.encodeQueryParameter).
| async getUserCalendarByName(userId: string, name: string, calendarGroupId?: string, properties?: string): Promise<Calendar> { | ||
| let url = `https://graph.microsoft.com/v1.0/users('${userId}')/${calendarGroupId ? `calendarGroups/${calendarGroupId}/` : ''}calendars?$filter=name eq '${formatting.encodeQueryParameter(name)}'`; | ||
|
|
There was a problem hiding this comment.
The URL is built using users('${userId}') without encoding userId. UPNs for guest users commonly contain #EXT#, and an unencoded # will be treated as a URL fragment delimiter and won’t be sent to Microsoft Graph. Consider URL-encoding the userId value when composing the request URL (for example using formatting.encodeQueryParameter).
|
Thanks, we'll try to review it ASAP! |
Adam-it
left a comment
Adam-it
left a comment
There was a problem hiding this comment.
@nanddeepn the implementation looks really solid and I think we should only look at a few details before we merge 👍
| .refine(options => !(options.calendarId && options.calendarName), { | ||
| error: 'Specify either calendarId or calendarName, but not both.' | ||
| }); |
There was a problem hiding this comment.
This is a valid comment. We should elso ensure at least one is specified
Adam-it
left a comment
Adam-it
left a comment
There was a problem hiding this comment.
@nanddeepn I checked the code and tested it locally and I have not suggestion to the way it was implemented 👍
Only things I noticed is that I think we should allow to also run this command without the need to specify the calendar, the same as we do with the event list
I added a comment in the related issue for clarification before we proceed
#7122 (comment)
|
@nanddeepn we need to align the command to allow to execute it without the need of specifying calendarId or calendar name Similar handling is already done in |
Closes #7122