Skip to content

Commit a3dcc1c

Browse files
jhodappjhodapp
authored
Merge pull request #282 from refactor-group/fix/prevent-duplicate-agreement-action-creation
Disable Save button and show spinner during Agreement/Action creation/update
2 parents 57b98db + e6c0541 commit a3dcc1c

File tree

8 files changed

+245
-14
lines changed

8 files changed

+245
-14
lines changed

‎__tests__/components/actions-list-responsive.test.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ describe('ActionsList Responsive Design', () => {
6060
coachName: 'Coach Jane',
6161
coacheeId: MOCK_COACHEE_ID,
6262
coacheeName: 'Coachee John',
63+
isSaving: false,
6364
onActionAdded: vi.fn(),
6465
onActionEdited: vi.fn(),
6566
onActionDeleted: vi.fn(),

‎__tests__/components/actions-list.test.tsx‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ import { ActionsList } from '@/components/ui/coaching-sessions/actions-list'
55
import { TestProviders } from '@/test-utils/providers'
66
import { ItemStatus } from '@/types/general'
77
import { DateTime } from 'ts-luxon'
8+
import { toast } from 'sonner'
9+
10+
vi.mock('sonner')
811

912
// Mock user IDs for coach and coachee
1013
const MOCK_COACH_ID = 'coach-123'
@@ -64,6 +67,7 @@ describe('ActionsList', () => {
6467
coachName: 'Coach Jane',
6568
coacheeId: MOCK_COACHEE_ID,
6669
coacheeName: 'Coachee John',
70+
isSaving: false,
6771
onActionAdded: vi.fn(),
6872
onActionEdited: vi.fn(),
6973
onActionDeleted: vi.fn(),
@@ -366,4 +370,118 @@ describe('ActionsList', () => {
366370
const assigneeHeader = screen.getByRole('columnheader', { name: /assignee/i })
367371
expect(assigneeHeader).toBeInTheDocument()
368372
})
373+
374+
/**
375+
* Error Toast Tests
376+
* Validates that user-facing error messages appear when operations fail
377+
*/
378+
describe('Error Toasts', () => {
379+
const mockToast = vi.mocked(toast)
380+
381+
/**
382+
* Asserts toast.error is called when saving a new action fails
383+
* This ensures visible feedback on create failure
384+
*/
385+
it('should show error toast when saving a new action fails', async () => {
386+
mockProps.onActionAdded.mockRejectedValue(new Error('Network error'))
387+
388+
render(
389+
<TestProviders>
390+
<ActionsList {...mockProps} />
391+
</TestProviders>
392+
)
393+
394+
const input = screen.getByPlaceholderText('Enter new action')
395+
const saveButton = screen.getByText('Save')
396+
397+
fireEvent.change(input, { target: { value: 'New action' } })
398+
fireEvent.click(saveButton)
399+
400+
await waitFor(() => {
401+
expect(mockToast.error).toHaveBeenCalledWith('Failed to save action.')
402+
})
403+
})
404+
405+
/**
406+
* Asserts toast.error is called when updating an action fails
407+
* This ensures visible feedback on update failure
408+
*/
409+
it('should show error toast when updating an action fails', async () => {
410+
const user = userEvent.setup()
411+
mockProps.onActionEdited.mockRejectedValue(new Error('Network error'))
412+
413+
render(
414+
<TestProviders>
415+
<ActionsList {...mockProps} />
416+
</TestProviders>
417+
)
418+
419+
// Enter edit mode via dropdown on first action (action-3 due to sort)
420+
const dropdownTriggers = screen.getAllByRole('button', { name: /open menu/i })
421+
await user.click(dropdownTriggers[0])
422+
423+
await waitFor(() => {
424+
expect(screen.getByText('Edit')).toBeInTheDocument()
425+
})
426+
427+
await user.click(screen.getByText('Edit'))
428+
429+
const updateButton = screen.getByText('Update')
430+
await user.click(updateButton)
431+
432+
await waitFor(() => {
433+
expect(mockToast.error).toHaveBeenCalledWith('Failed to update action.')
434+
})
435+
})
436+
437+
/**
438+
* Asserts toast.error is called when deleting an action fails
439+
* This ensures visible feedback on delete failure
440+
*/
441+
it('should show error toast when deleting an action fails', async () => {
442+
const user = userEvent.setup()
443+
mockProps.onActionDeleted.mockRejectedValue(new Error('Network error'))
444+
445+
render(
446+
<TestProviders>
447+
<ActionsList {...mockProps} />
448+
</TestProviders>
449+
)
450+
451+
const dropdownTriggers = screen.getAllByRole('button', { name: /open menu/i })
452+
await user.click(dropdownTriggers[0])
453+
454+
await waitFor(() => {
455+
expect(screen.getByText('Delete')).toBeInTheDocument()
456+
})
457+
458+
await user.click(screen.getByText('Delete'))
459+
460+
await waitFor(() => {
461+
expect(mockToast.error).toHaveBeenCalledWith('Failed to delete action.')
462+
})
463+
})
464+
465+
/**
466+
* Asserts toast.error is called when toggling action completion fails
467+
* This ensures visible feedback on checkbox toggle failure
468+
*/
469+
it('should show error toast when completion toggle fails', async () => {
470+
mockProps.onActionEdited.mockRejectedValue(new Error('Network error'))
471+
472+
render(
473+
<TestProviders>
474+
<ActionsList {...mockProps} />
475+
</TestProviders>
476+
)
477+
478+
// Click the first checkbox (action-3 due to sort, NotStarted -> Completed)
479+
const checkboxes = screen.getAllByRole('checkbox')
480+
fireEvent.click(checkboxes[0])
481+
482+
await waitFor(() => {
483+
expect(mockToast.error).toHaveBeenCalledWith('Failed to update action status.')
484+
})
485+
})
486+
})
369487
})

‎__tests__/components/agreements-list.test.tsx‎

Lines changed: 101 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'
44
import { AgreementsList } from '@/components/ui/coaching-sessions/agreements-list'
55
import { TestProviders } from '@/test-utils/providers'
66
import { DateTime } from 'ts-luxon'
7+
import { toast } from 'sonner'
8+
9+
vi.mock('sonner')
710

811
// Mock the agreements API hook
912
const mockAgreements = [
@@ -43,6 +46,7 @@ describe('AgreementsList', () => {
4346
coachingSessionId: 'session-123',
4447
userId: 'user-123',
4548
locale: 'us',
49+
isSaving: false,
4650
onAgreementAdded: vi.fn(),
4751
onAgreementEdited: vi.fn(),
4852
onAgreementDeleted: vi.fn(),
@@ -296,23 +300,115 @@ describe('AgreementsList', () => {
296300
const user = userEvent.setup()
297301
const dropdownTriggers = screen.getAllByRole('button', { name: /open menu/i })
298302
await user.click(dropdownTriggers[0])
299-
303+
300304
await waitFor(() => {
301305
expect(screen.getByText('Edit')).toBeInTheDocument()
302306
})
303-
307+
304308
await user.click(screen.getByText('Edit'))
305309

306310
const input = screen.getByDisplayValue('Monthly goal review sessions')
307-
311+
308312
// Change the text
309313
fireEvent.change(input, { target: { value: 'Modified text' } })
310-
314+
311315
// Press Escape to cancel
312316
fireEvent.keyDown(input.closest('div')!, { key: 'Escape' })
313-
317+
314318
// Should return to add mode with empty form
315319
expect(screen.getByPlaceholderText('Enter new agreement')).toHaveValue('')
316320
expect(screen.getByText('Save')).toBeInTheDocument()
317321
})
322+
323+
/**
324+
* Error Toast Tests
325+
* Validates that user-facing error messages appear when operations fail
326+
*/
327+
describe('Error Toasts', () => {
328+
const mockToast = vi.mocked(toast)
329+
330+
/**
331+
* Asserts toast.error is called when saving a new agreement fails
332+
* This ensures visible feedback on create failure
333+
*/
334+
it('should show error toast when saving a new agreement fails', async () => {
335+
mockProps.onAgreementAdded.mockRejectedValue(new Error('Network error'))
336+
337+
render(
338+
<TestProviders>
339+
<AgreementsList {...mockProps} />
340+
</TestProviders>
341+
)
342+
343+
const input = screen.getByPlaceholderText('Enter new agreement')
344+
const saveButton = screen.getByText('Save')
345+
346+
fireEvent.change(input, { target: { value: 'New agreement' } })
347+
fireEvent.click(saveButton)
348+
349+
await waitFor(() => {
350+
expect(mockToast.error).toHaveBeenCalledWith('Failed to save agreement.')
351+
})
352+
})
353+
354+
/**
355+
* Asserts toast.error is called when updating an agreement fails
356+
* This ensures visible feedback on update failure
357+
*/
358+
it('should show error toast when updating an agreement fails', async () => {
359+
const user = userEvent.setup()
360+
mockProps.onAgreementEdited.mockRejectedValue(new Error('Network error'))
361+
362+
render(
363+
<TestProviders>
364+
<AgreementsList {...mockProps} />
365+
</TestProviders>
366+
)
367+
368+
// Enter edit mode
369+
const dropdownTriggers = screen.getAllByRole('button', { name: /open menu/i })
370+
await user.click(dropdownTriggers[0])
371+
372+
await waitFor(() => {
373+
expect(screen.getByText('Edit')).toBeInTheDocument()
374+
})
375+
376+
await user.click(screen.getByText('Edit'))
377+
378+
const updateButton = screen.getByText('Update')
379+
await user.click(updateButton)
380+
381+
await waitFor(() => {
382+
expect(mockToast.error).toHaveBeenCalledWith('Failed to update agreement.')
383+
})
384+
})
385+
386+
/**
387+
* Asserts toast.error is called when deleting an agreement fails
388+
* This ensures visible feedback on delete failure
389+
*/
390+
it('should show error toast when deleting an agreement fails', async () => {
391+
const user = userEvent.setup()
392+
mockProps.onAgreementDeleted.mockRejectedValue(new Error('Network error'))
393+
394+
render(
395+
<TestProviders>
396+
<AgreementsList {...mockProps} />
397+
</TestProviders>
398+
)
399+
400+
const dropdownTriggers = screen.getAllByRole('button', { name: /open menu/i })
401+
await user.click(dropdownTriggers[0])
402+
403+
await waitFor(() => {
404+
expect(screen.getByText('Delete')).toBeInTheDocument()
405+
})
406+
407+
await user.click(screen.getByText('Delete'))
408+
409+
await waitFor(() => {
410+
expect(mockToast.error).toHaveBeenCalledWith('Failed to delete agreement.')
411+
})
412+
})
413+
})
318414
})

‎__tests__/components/ui/coaching-sessions/actions-list-responsive.test.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ describe('ActionsList Responsive Design', () => {
6060
coachName: 'Coach Jane',
6161
coacheeId: MOCK_COACHEE_ID,
6262
coacheeName: 'Coachee John',
63+
isSaving: false,
6364
onActionAdded: vi.fn(),
6465
onActionEdited: vi.fn(),
6566
onActionDeleted: vi.fn(),

‎__tests__/components/ui/coaching-sessions/agreements-list-responsive.test.tsx‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ describe('AgreementsList Responsive Design', () => {
4949
coachingSessionId: 'session-123',
5050
userId: 'user-123',
5151
locale: 'us',
52+
isSaving: false,
5253
onAgreementAdded: vi.fn(),
5354
onAgreementEdited: vi.fn(),
5455
onAgreementDeleted: vi.fn(),

‎src/components/ui/coaching-sessions/actions-list.tsx‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ import {
3636
ArrowUp,
3737
ArrowDown,
3838
CalendarClock,
39+
Loader2,
3940
} from "lucide-react";
41+
import { toast } from "sonner";
4042
import {
4143
ItemStatus,
4244
actionStatusToString,
@@ -91,6 +93,7 @@ interface ActionsListProps {
9193
assigneeIds?: Id[]
9294
) => Promise<Action>;
9395
onActionDeleted: (id: Id) => Promise<Action>;
96+
isSaving: boolean;
9497
}
9598

9699
const ActionsList: React.FC<ActionsListProps> = ({
@@ -100,6 +103,7 @@ const ActionsList: React.FC<ActionsListProps> = ({
100103
coachName,
101104
coacheeId,
102105
coacheeName,
106+
isSaving,
103107
onActionAdded,
104108
onActionEdited,
105109
onActionDeleted,
@@ -214,6 +218,7 @@ const ActionsList: React.FC<ActionsListProps> = ({
214218
refresh();
215219
} catch (err) {
216220
console.error("Failed to update action completion status: " + err);
221+
toast.error("Failed to update action status.");
217222
}
218223
};
219224

@@ -263,7 +268,7 @@ const ActionsList: React.FC<ActionsListProps> = ({
263268
clearNewActionForm();
264269
} catch (err) {
265270
console.error("Failed to save Action: " + err);
266-
throw err;
271+
toast.error(editingActionId ? "Failed to update action." : "Failed to save action.");
267272
}
268273
};
269274

@@ -278,7 +283,7 @@ const ActionsList: React.FC<ActionsListProps> = ({
278283
refresh();
279284
} catch (err) {
280285
console.error("Failed to delete Action (id: " + id + "): " + err);
281-
throw err;
286+
toast.error("Failed to delete action.");
282287
}
283288
};
284289

@@ -451,7 +456,7 @@ const ActionsList: React.FC<ActionsListProps> = ({
451456
onKeyDown={(e) => {
452457
if (e.key === "Escape") {
453458
editingActionId ? cancelEditAction() : clearNewActionForm();
454-
} else if (e.key === "Enter") {
459+
} else if (e.key === "Enter" && !isSaving) {
455460
addAction();
456461
}
457462
}}
@@ -515,7 +520,8 @@ const ActionsList: React.FC<ActionsListProps> = ({
515520
/>
516521
</PopoverContent>
517522
</Popover>
518-
<Button onClick={addAction} className="w-full sm:w-auto">
523+
<Button onClick={addAction} disabled={isSaving} className="w-full sm:w-auto">
524+
{isSaving && <Loader2 className="mr-2 h-4 w-4 animate-spin" />}
519525
{editingActionId ? "Update" : "Save"}
520526
</Button>
521527
</div>

0 commit comments

Comments
 (0)