Skip to content

refactor(calendar): moving validation logic outside of service - implements #313 #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
galuszkak wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor(calendar): moving validation logic outside of service - implements #313 #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3743cf7
refactor(calendar): moving validation logic outside of service - impl…
galuszkak Apr 1, 2026
a92c505
fix: address really great @allenhutchison code review feedback
galuszkak Apr 7, 2026
463f92e
fix: improve error support based on Google API documentation
galuszkak Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,13 @@ ready for a formal review but is open for discussion and initial feedback.
#### 4. Ensure All Checks Pass

Before submitting your PR, ensure that all automated checks are passing by
running `npm run test && npm run lint`. This command runs all tests, linting,
and other style checks.
running:

```bash
npm run test && npm run lint && npm run format:check && npx tsc --noEmit --project workspace-server
```

This command runs all tests, linting, formatting, and type checks.

#### 5. Write Clear Commit Messages and a Good PR Description

Expand Down
4 changes: 3 additions & 1 deletion skills/google-calendar/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ calendar.createEvent({
```

- **`summary`** defaults to `"Working Location"` if omitted
- **All-day working location events** must span **exactly one day**. Use the
next day as the exclusive `end` date.
- **`workingLocationProperties`** is **required** when `eventType` is
`"workingLocation"`
- **`workingLocationProperties.type`** — `"homeOffice"`, `"officeLocation"`, or
Expand Down Expand Up @@ -268,7 +270,7 @@ be changed — everything else is preserved.
- **Changing title/description**: Update `summary` or `description`
- **Adding Google Meet**: Set `addGoogleMeet: true` to generate a Meet link
- **Managing attachments**: Provide the full attachment list (replaces all
existing)
existing). Pass `attachments: []` to clear all attachments.

> **Important:** The `attendees` field is a full replacement, not an append. To
> add a new attendee, include all existing attendees plus the new one. The same
Expand Down
165 changes: 152 additions & 13 deletions workspace-server/src/__tests__/services/CalendarService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ describe('CalendarService', () => {

afterEach(() => {
jest.restoreAllMocks();
expect(mockCalendarAPI.events.update).not.toHaveBeenCalled();
});

describe('listCalendars', () => {
Expand Down Expand Up @@ -168,6 +169,20 @@ describe('CalendarService', () => {
expect(JSON.parse(result.content[0].text)).toEqual(mockCreatedEvent);
});

it('creates a single-day all-day workingLocation event', async () => {
mockCalendarAPI.events.insert.mockResolvedValue({ data: { id: 'wl1' } });
await calendarService.createEvent({
start: { date: '2024-01-15' },
end: { date: '2024-01-16' },
eventType: 'workingLocation',
workingLocationProperties: { type: 'homeOffice' },
});
const body = mockCalendarAPI.events.insert.mock.calls[0][0].requestBody;
expect(body.start).toEqual({ date: '2024-01-15' });
expect(body.end).toEqual({ date: '2024-01-16' });
expect(body.eventType).toBe('workingLocation');
});

it('should create a calendar event', async () => {
const eventInput = {
calendarId: 'primary',
Expand Down Expand Up @@ -857,7 +872,7 @@ describe('CalendarService', () => {
attendees: [{ email: 'new@example.com' }],
};

mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

const result = await calendarService.updateEvent({
eventId: 'event123',
Expand All @@ -867,7 +882,7 @@ describe('CalendarService', () => {
attendees: ['new@example.com'],
});

expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
requestBody: {
Expand All @@ -889,14 +904,14 @@ describe('CalendarService', () => {
description: 'New updated description',
};

mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

const result = await calendarService.updateEvent({
eventId: 'event123',
description: 'New updated description',
});

expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
requestBody: {
Expand All @@ -910,7 +925,7 @@ describe('CalendarService', () => {

it('should handle update errors', async () => {
const apiError = new Error('Update failed');
mockCalendarAPI.events.update.mockRejectedValue(apiError);
mockCalendarAPI.events.patch.mockRejectedValue(apiError);

const result = await calendarService.updateEvent({
eventId: 'event123',
Expand All @@ -921,27 +936,116 @@ describe('CalendarService', () => {
expect(parsedResult.error).toBe('Update failed');
});

it('should surface structured Google API field errors on update', async () => {
mockCalendarAPI.events.patch.mockRejectedValue({
response: {
data: {
error: {
message: 'Invalid Value',
code: 400,
errors: [
{
location: 'start.dateTime',
reason: 'invalid',
message: 'Invalid start time',
},
{
location: 'attendees',
reason: 'invalid',
message: 'Attendee email is invalid',
},
],
},
},
},
});

const result = await calendarService.updateEvent({
eventId: 'event123',
start: { dateTime: '2024-01-15T10:00:00Z' },
});

const parsedResult = JSON.parse(result.content[0].text);
expect(parsedResult.error).toBe(
'Invalid Value (code 400): start.dateTime invalid: Invalid start time; attendees invalid: Attendee email is invalid',
);
});

it('should only send fields that are provided', async () => {
const updatedEvent = {
id: 'event123',
summary: 'Updated Meeting Only',
};

mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

await calendarService.updateEvent({
eventId: 'event123',
summary: 'Updated Meeting Only',
});

expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
requestBody: {
summary: 'Updated Meeting Only',
},
});
});

it.each([
['summary', { summary: 'X' }],
['description', { description: 'X' }],
['start', { start: { dateTime: '2024-01-15T10:00:00Z' } }],
['end', { end: { dateTime: '2024-01-15T11:00:00Z' } }],
['attendees', { attendees: ['a@b.com'] }],
])(
'#313: patch body for %s update contains only that field',
async (field, patch) => {
mockCalendarAPI.events.patch.mockResolvedValue({ data: { id: 'e' } });
await calendarService.updateEvent({ eventId: 'e', ...patch });
const body = mockCalendarAPI.events.patch.mock.calls[0][0].requestBody;
expect(Object.keys(body)).toEqual([field]);
},
);

it('should clear description when passed an empty string', async () => {
mockCalendarAPI.events.patch.mockResolvedValue({
data: { id: 'event123', description: '' },
});

await calendarService.updateEvent({
eventId: 'event123',
description: '',
});

expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
requestBody: {
description: '',
},
});
});

it('should clear attendees when passed an empty array', async () => {
mockCalendarAPI.events.patch.mockResolvedValue({
data: { id: 'event123', attendees: [] },
});

await calendarService.updateEvent({
eventId: 'event123',
attendees: [],
});

expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
requestBody: {
attendees: [],
},
});
});
});

describe('respondToEvent', () => {
Expand Down Expand Up @@ -1495,14 +1599,14 @@ describe('CalendarService', () => {
},
};

mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

const result = await calendarService.updateEvent({
eventId: 'event123',
addGoogleMeet: true,
});

const callArgs = mockCalendarAPI.events.update.mock.calls[0][0];
const callArgs = mockCalendarAPI.events.patch.mock.calls[0][0];
expect(callArgs.conferenceDataVersion).toBe(1);
expect(callArgs.requestBody.conferenceData).toBeDefined();
expect(
Expand All @@ -1515,15 +1619,15 @@ describe('CalendarService', () => {

it('should not include conferenceData when addGoogleMeet is false', async () => {
const updatedEvent = { id: 'event123', summary: 'No Meet' };
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

await calendarService.updateEvent({
eventId: 'event123',
summary: 'No Meet',
addGoogleMeet: false,
});

const callArgs = mockCalendarAPI.events.update.mock.calls[0][0];
const callArgs = mockCalendarAPI.events.patch.mock.calls[0][0];
expect(callArgs.conferenceDataVersion).toBeUndefined();
expect(callArgs.requestBody.conferenceData).toBeUndefined();
});
Expand All @@ -1541,7 +1645,7 @@ describe('CalendarService', () => {
],
};

mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });

const result = await calendarService.updateEvent({
eventId: 'event123',
Expand All @@ -1553,7 +1657,7 @@ describe('CalendarService', () => {
],
});

const callArgs = mockCalendarAPI.events.update.mock.calls[0][0];
const callArgs = mockCalendarAPI.events.patch.mock.calls[0][0];
expect(callArgs.supportsAttachments).toBe(true);
expect(callArgs.requestBody.attachments).toEqual([
expect.objectContaining({
Expand All @@ -1564,6 +1668,26 @@ describe('CalendarService', () => {

expect(JSON.parse(result.content[0].text)).toEqual(updatedEvent);
});

it('should clear attachments when passed an empty array', async () => {
mockCalendarAPI.events.patch.mockResolvedValue({
data: { id: 'event123', attachments: [] },
});

await calendarService.updateEvent({
eventId: 'event123',
attachments: [],
});

expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
calendarId: 'primary',
eventId: 'event123',
supportsAttachments: true,
requestBody: {
attachments: [],
},
});
});
Copy link
Copy Markdown
Contributor

@allenhutchison allenhutchison Apr 5, 2026

Choose a reason for hiding this comment

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

Optional follow-up: since this PR introduces explicit attachments: [] clearing semantics, it'd be nice to pin the parallel behaviors for the other fields too:

  • description: '' — current code sends description: '' (clears it), which seems intentional but is untested.
  • attendees: [] — current code sends attendees: [] (which Google interprets as "clear all attendees"), also untested and undocumented.

Just one test each to lock in whichever semantics you intend. Happy for this to be a separate PR.

Copy link
Copy Markdown
Contributor Author

@galuszkak galuszkak Apr 7, 2026

Choose a reason for hiding this comment

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

added - actually this was really good point.

});
});

Expand Down Expand Up @@ -2121,6 +2245,21 @@ describe('CalendarService', () => {
expect(parsedResult.error).toBe('Invalid input format');
});

it('should reject all-day workingLocation events that span multiple days', async () => {
const result = await calendarService.createEvent({
start: { date: '2024-01-15' },
end: { date: '2024-01-17' },
eventType: 'workingLocation',
workingLocationProperties: { type: 'homeOffice' },
});

const parsedResult = JSON.parse(result.content[0].text);
expect(parsedResult.error).toBe('Invalid input format');
expect(parsedResult.details).toContain(
'all-day workingLocation events must span exactly one day',
);
});
Copy link
Copy Markdown
Contributor

@allenhutchison allenhutchison Apr 5, 2026

Choose a reason for hiding this comment

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

The rejection path for multi-day workingLocation is tested here, and the single-day happy path is covered at the unit level in CalendarValidation.test.ts — but I don't see an integration-style test on CalendarService that proves a valid single-day all-day workingLocation event actually survives validation and reaches events.insert with the right body. Something like:

it('creates a single-day all-day workingLocation event', async () => {
  mockCalendarAPI.events.insert.mockResolvedValue({ data: { id: 'wl1' } });
  await calendarService.createEvent({
    start: { date: '2024-01-15' },
    end: { date: '2024-01-16' },
    eventType: 'workingLocation',
    workingLocationProperties: { type: 'homeOffice' },
  });
  const body = mockCalendarAPI.events.insert.mock.calls[0][0].requestBody;
  expect(body.start).toEqual({ date: '2024-01-15' });
  expect(body.end).toEqual({ date: '2024-01-16' });
  expect(body.eventType).toBe('workingLocation');
});

Without this, a future tightening of the validator that accidentally rejects single-day input would only fail unit tests, not the integration surface.


it('should reject start with both dateTime and date', async () => {
const result = await calendarService.createEvent({
summary: 'Ambiguous Event',
Expand Down
Loading
Loading