Skip to content

Commit 11829e2

Browse files
galuszkakgaluszkak
authored
refactor(calendar): moving validation logic outside of service - implements #313 (#316)
* refactor(calendar): moving validation logic outside of service - implements #313 * fix: address really great @allenhutchison code review feedback * fix: improve error support based on Google API documentation
1 parent 3ab1b79 commit 11829e2

7 files changed

Lines changed: 692 additions & 229 deletions

File tree

‎CONTRIBUTING.md‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,13 @@ ready for a formal review but is open for discussion and initial feedback.
8383
#### 4. Ensure All Checks Pass
8484

8585
Before submitting your PR, ensure that all automated checks are passing by
86-
running `npm run test && npm run lint`. This command runs all tests, linting,
87-
and other style checks.
86+
running:
87+
88+
```bash
89+
npm run test && npm run lint && npm run format:check && npx tsc --noEmit --project workspace-server
90+
```
91+
92+
This command runs all tests, linting, formatting, and type checks.
8893

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

‎skills/google-calendar/SKILL.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,8 @@ calendar.createEvent({
234234
```
235235

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

273275
> **Important:** The `attendees` field is a full replacement, not an append. To
274276
> add a new attendee, include all existing attendees plus the new one. The same

‎workspace-server/src/__tests__/services/CalendarService.test.ts‎

Lines changed: 152 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ describe('CalendarService', () => {
6363

6464
afterEach(() => {
6565
jest.restoreAllMocks();
66+
expect(mockCalendarAPI.events.update).not.toHaveBeenCalled();
6667
});
6768

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

172+
it('creates a single-day all-day workingLocation event', async () => {
173+
mockCalendarAPI.events.insert.mockResolvedValue({ data: { id: 'wl1' } });
174+
await calendarService.createEvent({
175+
start: { date: '2024-01-15' },
176+
end: { date: '2024-01-16' },
177+
eventType: 'workingLocation',
178+
workingLocationProperties: { type: 'homeOffice' },
179+
});
180+
const body = mockCalendarAPI.events.insert.mock.calls[0][0].requestBody;
181+
expect(body.start).toEqual({ date: '2024-01-15' });
182+
expect(body.end).toEqual({ date: '2024-01-16' });
183+
expect(body.eventType).toBe('workingLocation');
184+
});
185+
171186
it('should create a calendar event', async () => {
172187
const eventInput = {
173188
calendarId: 'primary',
@@ -857,7 +872,7 @@ describe('CalendarService', () => {
857872
attendees: [{ email: 'new@example.com' }],
858873
};
859874

860-
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
875+
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });
861876

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

870-
expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
885+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
871886
calendarId: 'primary',
872887
eventId: 'event123',
873888
requestBody: {
@@ -889,14 +904,14 @@ describe('CalendarService', () => {
889904
description: 'New updated description',
890905
};
891906

892-
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
907+
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });
893908

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

899-
expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
914+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
900915
calendarId: 'primary',
901916
eventId: 'event123',
902917
requestBody: {
@@ -910,7 +925,7 @@ describe('CalendarService', () => {
910925

911926
it('should handle update errors', async () => {
912927
const apiError = new Error('Update failed');
913-
mockCalendarAPI.events.update.mockRejectedValue(apiError);
928+
mockCalendarAPI.events.patch.mockRejectedValue(apiError);
914929

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

939+
it('should surface structured Google API field errors on update', async () => {
940+
mockCalendarAPI.events.patch.mockRejectedValue({
941+
response: {
942+
data: {
943+
error: {
944+
message: 'Invalid Value',
945+
code: 400,
946+
errors: [
947+
{
948+
location: 'start.dateTime',
949+
reason: 'invalid',
950+
message: 'Invalid start time',
951+
},
952+
{
953+
location: 'attendees',
954+
reason: 'invalid',
955+
message: 'Attendee email is invalid',
956+
},
957+
],
958+
},
959+
},
960+
},
961+
});
962+
963+
const result = await calendarService.updateEvent({
964+
eventId: 'event123',
965+
start: { dateTime: '2024-01-15T10:00:00Z' },
966+
});
967+
968+
const parsedResult = JSON.parse(result.content[0].text);
969+
expect(parsedResult.error).toBe(
970+
'Invalid Value (code 400): start.dateTime invalid: Invalid start time; attendees invalid: Attendee email is invalid',
971+
);
972+
});
973+
924974
it('should only send fields that are provided', async () => {
925975
const updatedEvent = {
926976
id: 'event123',
927977
summary: 'Updated Meeting Only',
928978
};
929979

930-
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
980+
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });
931981

932982
await calendarService.updateEvent({
933983
eventId: 'event123',
934984
summary: 'Updated Meeting Only',
935985
});
936986

937-
expect(mockCalendarAPI.events.update).toHaveBeenCalledWith({
987+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
938988
calendarId: 'primary',
939989
eventId: 'event123',
940990
requestBody: {
941991
summary: 'Updated Meeting Only',
942992
},
943993
});
944994
});
995+
996+
it.each([
997+
['summary', { summary: 'X' }],
998+
['description', { description: 'X' }],
999+
['start', { start: { dateTime: '2024-01-15T10:00:00Z' } }],
1000+
['end', { end: { dateTime: '2024-01-15T11:00:00Z' } }],
1001+
['attendees', { attendees: ['a@b.com'] }],
1002+
])(
1003+
'#313: patch body for %s update contains only that field',
1004+
async (field, patch) => {
1005+
mockCalendarAPI.events.patch.mockResolvedValue({ data: { id: 'e' } });
1006+
await calendarService.updateEvent({ eventId: 'e', ...patch });
1007+
const body = mockCalendarAPI.events.patch.mock.calls[0][0].requestBody;
1008+
expect(Object.keys(body)).toEqual([field]);
1009+
},
1010+
);
1011+
1012+
it('should clear description when passed an empty string', async () => {
1013+
mockCalendarAPI.events.patch.mockResolvedValue({
1014+
data: { id: 'event123', description: '' },
1015+
});
1016+
1017+
await calendarService.updateEvent({
1018+
eventId: 'event123',
1019+
description: '',
1020+
});
1021+
1022+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
1023+
calendarId: 'primary',
1024+
eventId: 'event123',
1025+
requestBody: {
1026+
description: '',
1027+
},
1028+
});
1029+
});
1030+
1031+
it('should clear attendees when passed an empty array', async () => {
1032+
mockCalendarAPI.events.patch.mockResolvedValue({
1033+
data: { id: 'event123', attendees: [] },
1034+
});
1035+
1036+
await calendarService.updateEvent({
1037+
eventId: 'event123',
1038+
attendees: [],
1039+
});
1040+
1041+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
1042+
calendarId: 'primary',
1043+
eventId: 'event123',
1044+
requestBody: {
1045+
attendees: [],
1046+
},
1047+
});
1048+
});
9451049
});
9461050

9471051
describe('respondToEvent', () => {
@@ -1495,14 +1599,14 @@ describe('CalendarService', () => {
14951599
},
14961600
};
14971601

1498-
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
1602+
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });
14991603

15001604
const result = await calendarService.updateEvent({
15011605
eventId: 'event123',
15021606
addGoogleMeet: true,
15031607
});
15041608

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

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

15201624
await calendarService.updateEvent({
15211625
eventId: 'event123',
15221626
summary: 'No Meet',
15231627
addGoogleMeet: false,
15241628
});
15251629

1526-
const callArgs = mockCalendarAPI.events.update.mock.calls[0][0];
1630+
const callArgs = mockCalendarAPI.events.patch.mock.calls[0][0];
15271631
expect(callArgs.conferenceDataVersion).toBeUndefined();
15281632
expect(callArgs.requestBody.conferenceData).toBeUndefined();
15291633
});
@@ -1541,7 +1645,7 @@ describe('CalendarService', () => {
15411645
],
15421646
};
15431647

1544-
mockCalendarAPI.events.update.mockResolvedValue({ data: updatedEvent });
1648+
mockCalendarAPI.events.patch.mockResolvedValue({ data: updatedEvent });
15451649

15461650
const result = await calendarService.updateEvent({
15471651
eventId: 'event123',
@@ -1553,7 +1657,7 @@ describe('CalendarService', () => {
15531657
],
15541658
});
15551659

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

15651669
expect(JSON.parse(result.content[0].text)).toEqual(updatedEvent);
15661670
});
1671+
1672+
it('should clear attachments when passed an empty array', async () => {
1673+
mockCalendarAPI.events.patch.mockResolvedValue({
1674+
data: { id: 'event123', attachments: [] },
1675+
});
1676+
1677+
await calendarService.updateEvent({
1678+
eventId: 'event123',
1679+
attachments: [],
1680+
});
1681+
1682+
expect(mockCalendarAPI.events.patch).toHaveBeenCalledWith({
1683+
calendarId: 'primary',
1684+
eventId: 'event123',
1685+
supportsAttachments: true,
1686+
requestBody: {
1687+
attachments: [],
1688+
},
1689+
});
1690+
});
15671691
});
15681692
});
15691693

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

2248+
it('should reject all-day workingLocation events that span multiple days', async () => {
2249+
const result = await calendarService.createEvent({
2250+
start: { date: '2024-01-15' },
2251+
end: { date: '2024-01-17' },
2252+
eventType: 'workingLocation',
2253+
workingLocationProperties: { type: 'homeOffice' },
2254+
});
2255+
2256+
const parsedResult = JSON.parse(result.content[0].text);
2257+
expect(parsedResult.error).toBe('Invalid input format');
2258+
expect(parsedResult.details).toContain(
2259+
'all-day workingLocation events must span exactly one day',
2260+
);
2261+
});
2262+
21242263
it('should reject start with both dateTime and date', async () => {
21252264
const result = await calendarService.createEvent({
21262265
summary: 'Ambiguous Event',

0 commit comments

Comments
 (0)