From 942b203b0fb6f957bc9e5c2404554a11aba6077e Mon Sep 17 00:00:00 2001 From: lenhanphung Date: Fri, 10 Oct 2025 11:45:10 +0700 Subject: [PATCH] fix: correct recurring to non-recurring event update logic - Fix bug where updating recurring event to non-recurring created duplicate events - Deduplicate URLs before deletion to avoid multiple delete calls on same file - Add onCloseAll prop to close both preview and update modals after save - Adjust smooth UI Resolves issue where repeat->no-repeat conversion left old recurring events visible --- .../features/Events/EventUpdateModal.test.tsx | 165 +++++++++++------- src/features/Events/EventDisplayPreview.tsx | 4 + src/features/Events/EventUpdateModal.tsx | 165 ++++++++++++------ 3 files changed, 209 insertions(+), 125 deletions(-) diff --git a/__test__/features/Events/EventUpdateModal.test.tsx b/__test__/features/Events/EventUpdateModal.test.tsx index 10add7e..4b46355 100644 --- a/__test__/features/Events/EventUpdateModal.test.tsx +++ b/__test__/features/Events/EventUpdateModal.test.tsx @@ -2,9 +2,11 @@ import { screen, fireEvent, waitFor, act } from "@testing-library/react"; import { renderWithProviders } from "../../utils/Renderwithproviders"; import EventUpdateModal from "../../../src/features/Events/EventUpdateModal"; import * as EventApi from "../../../src/features/Events/EventApi"; +import * as CalendarApi from "../../../src/features/Calendars/CalendarApi"; import * as eventUtils from "../../../src/components/Event/utils/eventUtils"; jest.mock("../../../src/features/Events/EventApi"); +jest.mock("../../../src/features/Calendars/CalendarApi"); describe("EventUpdateModal Timezone Handling", () => { const mockOnClose = jest.fn(); @@ -193,7 +195,7 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { it("converts recurring event to non-recurring when repeat is disabled in edit all mode", async () => { // Create recurring event with multiple instances const eventDate = new Date("2025-01-15T10:00:00.000Z"); - + const masterEvent = { uid: baseUID, title: "Recurring Meeting", @@ -247,12 +249,18 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { }; // Mock API calls - const mockDeleteEvent = jest.spyOn(EventApi, 'deleteEvent').mockResolvedValue({} as any); - const mockPutEvent = jest.spyOn(EventApi, 'putEvent').mockResolvedValue({ status: 201, url: `/calendars/${calId}/new-event.ics` } as any); - const consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(); - - // Mock refreshCalendars - const mockRefreshCalendars = jest.spyOn(eventUtils, 'refreshCalendars').mockResolvedValue(undefined); + const mockDeleteEvent = jest + .spyOn(EventApi, "deleteEvent") + .mockResolvedValue({} as any); + const mockPutEvent = jest.spyOn(EventApi, "putEvent").mockResolvedValue({ + status: 201, + url: `/calendars/${calId}/new-event.ics`, + } as any); + jest.spyOn(CalendarApi, "getCalendar").mockResolvedValue({ + _embedded: { + "dav:item": [], + }, + } as any); const { store } = renderWithProviders( { // Uncheck repeat checkbox const repeatCheckbox = screen.getByLabelText("Repeat"); expect(repeatCheckbox).toBeChecked(); - + await act(async () => { fireEvent.click(repeatCheckbox); }); @@ -282,30 +290,31 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { // Click Save button const saveButton = screen.getByText("Save"); - + await act(async () => { fireEvent.click(saveButton); // Wait for the 500ms delay in the code - await new Promise(resolve => setTimeout(resolve, 600)); + await new Promise((resolve) => setTimeout(resolve, 600)); }); - // Verify API calls - await waitFor(() => { - expect(mockDeleteEvent).toHaveBeenCalledWith(`/calendars/${calId}/${baseUID}.ics`); - }, { timeout: 3000 }); + // Verify API calls - should delete all instances + await waitFor( + () => { + // Should have called deleteEvent for each instance (4 total: base + 3 recurrences) + expect(mockDeleteEvent).toHaveBeenCalled(); + // At least one instance should be deleted + expect(mockDeleteEvent.mock.calls.length).toBeGreaterThan(0); + }, + { timeout: 3000 } + ); + // Verify new event was created via putEvent expect(mockPutEvent).toHaveBeenCalled(); const putEventCall = mockPutEvent.mock.calls[0][0]; expect(putEventCall.title).toBe("Recurring Meeting"); expect(putEventCall.repetition?.freq).toBeFalsy(); expect(putEventCall.uid).not.toContain(baseUID); - // Verify console.log for successful deletion - expect(consoleLogSpy).toHaveBeenCalledWith( - "Deleted master event via direct API call:", - expect.stringContaining(baseUID) - ); - // Verify Redux store state changes const finalState = store.getState(); const calendar = finalState.calendars.list[calId]; @@ -316,15 +325,15 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { expect(calendar.events[`${baseUID}/20250116`]).toBeUndefined(); expect(calendar.events[`${baseUID}/20250117`]).toBeUndefined(); - // Verify refreshCalendars was called - expect(mockRefreshCalendars).toHaveBeenCalled(); - - consoleLogSpy.mockRestore(); + // Verify modal was closed after completion + await waitFor(() => { + expect(mockOnClose).toHaveBeenCalled(); + }); }); - it("continues creating new event even if deletion of old series fails", async () => { + it("gracefully handles deletion errors and continues to create new event", async () => { const eventDate = new Date("2025-01-15T10:00:00.000Z"); - + const masterEvent = { uid: baseUID, title: "Recurring Meeting", @@ -360,14 +369,16 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { }; // Mock deleteEvent to fail - const mockDeleteEvent = jest.spyOn(EventApi, 'deleteEvent') + jest + .spyOn(EventApi, "deleteEvent") .mockRejectedValue(new Error("Network error")); - const mockPutEvent = jest.spyOn(EventApi, 'putEvent') + const mockPutEvent = jest + .spyOn(EventApi, "putEvent") .mockResolvedValue({ status: 201 } as any); - const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); - + const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(); + // Mock refreshCalendars - jest.spyOn(eventUtils, 'refreshCalendars').mockResolvedValue(undefined); + jest.spyOn(eventUtils, "refreshCalendars").mockResolvedValue(undefined); const { store } = renderWithProviders( { // Uncheck repeat checkbox and save const repeatCheckbox = screen.getByLabelText("Repeat"); - + await act(async () => { fireEvent.click(repeatCheckbox); }); const saveButton = screen.getByText("Save"); - + await act(async () => { fireEvent.click(saveButton); - await new Promise(resolve => setTimeout(resolve, 600)); + await new Promise((resolve) => setTimeout(resolve, 600)); }); - // Verify error was logged - await waitFor(() => { - expect(consoleErrorSpy).toHaveBeenCalledWith( - "Failed to delete recurring event:", - expect.any(Error) - ); - }, { timeout: 3000 }); + // Verify error was logged for failed deletion + await waitFor( + () => { + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining("Failed to delete event file") + ); + }, + { timeout: 3000 } + ); + + // New implementation gracefully handles deletion errors: + // Even if some instances fail to delete, we still create the new event + // This ensures user gets their non-recurring event + await waitFor( + () => { + expect(mockPutEvent).toHaveBeenCalled(); + }, + { timeout: 3000 } + ); - // Verify new event was still created despite deletion failure - expect(mockPutEvent).toHaveBeenCalled(); const putEventCall = mockPutEvent.mock.calls[0][0]; expect(putEventCall.title).toBe("Recurring Meeting"); expect(putEventCall.repetition?.freq).toBeFalsy(); - expect(putEventCall.uid).not.toContain(baseUID); - // When API deletion fails, the error is caught and logged - // The code continues to create the new event (graceful degradation) - // Old events remain in store since API deletion failed - // They will be cleaned up when calendar refreshes from server - + // Modal should close after operation completes + await waitFor( + () => { + expect(mockOnClose).toHaveBeenCalled(); + }, + { timeout: 3000 } + ); + consoleErrorSpy.mockRestore(); }); - it("refreshes calendar after converting recurring to non-recurring", async () => { + it("closes all modals after converting recurring to non-recurring", async () => { const eventDate = new Date("2025-01-15T10:00:00.000Z"); - + const masterEvent = { uid: baseUID, title: "Recurring Meeting", @@ -460,16 +483,22 @@ describe("EventUpdateModal Recurring to Non-Recurring Conversion", () => { }; // Mock API calls - jest.spyOn(EventApi, 'deleteEvent').mockResolvedValue({} as any); - jest.spyOn(EventApi, 'putEvent').mockResolvedValue({ status: 201 } as any); - - // Mock refreshCalendars to track calls - const mockRefreshCalendars = jest.spyOn(eventUtils, 'refreshCalendars').mockResolvedValue(undefined); + jest.spyOn(EventApi, "deleteEvent").mockResolvedValue({} as any); + jest.spyOn(EventApi, "putEvent").mockResolvedValue({ status: 201 } as any); + jest.spyOn(CalendarApi, "getCalendar").mockResolvedValue({ + _embedded: { + "dav:item": [], + }, + } as any); + + // Mock onCloseAll to test closing both modals + const mockOnCloseAll = jest.fn(); renderWithProviders( { // Uncheck repeat and save const repeatCheckbox = screen.getByLabelText("Repeat"); - + await act(async () => { fireEvent.click(repeatCheckbox); }); const saveButton = screen.getByText("Save"); - + await act(async () => { fireEvent.click(saveButton); - await new Promise(resolve => setTimeout(resolve, 600)); + await new Promise((resolve) => setTimeout(resolve, 600)); }); - // Verify refreshCalendars was called - await waitFor(() => { - expect(mockRefreshCalendars).toHaveBeenCalledWith( - expect.any(Function), // dispatch - expect.any(Array), // calendars list - expect.any(Object) // calendar range - ); - }, { timeout: 3000 }); + // Verify onCloseAll was called to close both preview and update modals + await waitFor( + () => { + expect(mockOnCloseAll).toHaveBeenCalled(); + }, + { timeout: 3000 } + ); + + // Verify onClose was NOT called (onCloseAll is used instead) + expect(mockOnClose).not.toHaveBeenCalled(); }); }); diff --git a/src/features/Events/EventDisplayPreview.tsx b/src/features/Events/EventDisplayPreview.tsx index fdbe5bf..537b776 100644 --- a/src/features/Events/EventDisplayPreview.tsx +++ b/src/features/Events/EventDisplayPreview.tsx @@ -510,6 +510,10 @@ export default function EventPreviewModal({ setOpenUpdateModal(false)} + onCloseAll={() => { + setOpenUpdateModal(false); + onClose({}, "backdropClick"); + }} eventId={eventId} calId={calId} typeOfAction={typeOfAction} diff --git a/src/features/Events/EventUpdateModal.tsx b/src/features/Events/EventUpdateModal.tsx index c86b2f6..daf8474 100644 --- a/src/features/Events/EventUpdateModal.tsx +++ b/src/features/Events/EventUpdateModal.tsx @@ -32,6 +32,7 @@ function EventUpdateModal({ calId, open, onClose, + onCloseAll, eventData, typeOfAction, }: { @@ -39,6 +40,7 @@ function EventUpdateModal({ calId: string; open: boolean; onClose: (event: {}, reason: "backdropClick" | "escapeKeyDown") => void; + onCloseAll?: () => void; eventData?: CalendarEvent | null; typeOfAction?: "solo" | "all"; }) { @@ -283,8 +285,17 @@ function EventUpdateModal({ } }, [open, event, calId, userPersonnalCalendars, calendarsList]); + // Helper to close modal(s) - use onCloseAll if available to close preview modal too + const closeModal = () => { + if (onCloseAll) { + onCloseAll(); + } else { + onClose({}, "backdropClick"); + } + }; + const handleClose = () => { - onClose({}, "backdropClick"); + closeModal(); resetAllStateToDefault(); initializedKeyRef.current = null; }; @@ -337,8 +348,94 @@ function EventUpdateModal({ x_openpass_videoconference: meetingLink || undefined, }; - // Close popup immediately for better UX - onClose({}, "backdropClick"); + // Handle recurrence instances + const [, recurrenceId] = event.uid.split("/"); + + // Special case: When converting recurring event to non-recurring + // Keep modal open until all async operations complete + if ( + recurrenceId && + typeOfAction === "all" && + event.repetition?.freq && + !repetition.freq + ) { + const baseUID = event.uid.split("/")[0]; + + try { + // STEP 1: Delete ALL instances of recurring event + // Note: This system stores instances only, no master event file + + // Collect all instances that need to be deleted + const instancesToDelete = Object.keys(targetCalendar.events) + .filter((eventId) => eventId.split("/")[0] === baseUID) + .map((eventId) => targetCalendar.events[eventId]); + + // Get unique URLs to avoid deleting same file multiple times + const uniqueURLs = new Set(); + const instancesByURL = new Map(); + + instancesToDelete.forEach((instance) => { + if (!instancesByURL.has(instance.URL)) { + instancesByURL.set(instance.URL, []); + } + instancesByURL.get(instance.URL)!.push(instance); + uniqueURLs.add(instance.URL); + }); + + // Delete each unique URL once + const deletePromises = Array.from(uniqueURLs).map(async (url) => { + try { + await deleteEvent(url); + } catch (deleteError: any) { + // Silently ignore 404 - file might already be deleted + const is404 = + deleteError.response?.status === 404 || + deleteError.message?.includes("404") || + deleteError.message?.includes("Not Found"); + + if (!is404) { + console.error( + `Failed to delete event file: ${deleteError.message}` + ); + } + } + }); + + await Promise.all(deletePromises); + + // STEP 2: Clean up all instances from Redux store + Object.keys(targetCalendar.events).forEach((eventId) => { + if (eventId.split("/")[0] === baseUID) { + dispatch(removeEvent({ calendarUid: calId, eventUid: eventId })); + } + }); + + // STEP 3: Create new non-recurring event AFTER deletion + // Note: putEventAsync automatically fetches calendar events after creation, + // so we don't need to call refreshCalendars separately + await dispatch( + putEventAsync({ + cal: targetCalendar, + newEvent: { + ...newEvent, + uid: crypto.randomUUID(), + URL: `/calendars/${newCalId || calId}/${crypto.randomUUID()}.ics`, + }, + }) + ).unwrap(); + + // STEP 4: Close modal after everything completes + closeModal(); + } catch (err) { + console.error("Failed to convert recurring to non-recurring:", err); + // Keep modal open on error, user can retry or cancel + } + + return; + } + + // Close popup immediately for better UX (for non-special cases) + closeModal(); // If converting from a non-repeating event to a repeating one, // remove the original single instance to avoid duplicates on the grid @@ -346,9 +443,6 @@ function EventUpdateModal({ dispatch(removeEvent({ calendarUid: calId, eventUid: event.uid })); } - // Handle recurrence instances - const [, recurrenceId] = event.uid.split("/"); - // Execute API calls in background based on typeOfAction if (recurrenceId) { if (typeOfAction === "solo") { @@ -360,58 +454,13 @@ function EventUpdateModal({ }) ); } else if (typeOfAction === "all") { - // Update all instances - - // Special case: When converting recurring event to non-recurring - if (event.repetition?.freq && !repetition.freq) { - // For repeat -> no-repeat, create a new non-repeating event - dispatch( - putEventAsync({ - cal: targetCalendar, - newEvent: { - ...newEvent, - uid: crypto.randomUUID(), // Generate new ID for the single event - URL: `/calendars/${newCalId || calId}/${crypto.randomUUID()}.ics`, - }, - }) - ); - - // Delete the old repeating series - we need to be more direct and forceful - const baseUID = event.uid.split("/")[0]; - - try { - // Directly use the deleteEvent API without going through the store - const eventBaseURL = `/calendars/${calId}/${baseUID}.ics`; - await deleteEvent(eventBaseURL); - console.log( - "Deleted master event via direct API call:", - eventBaseURL - ); - - // Force a small delay to ensure deletion completes - await new Promise((resolve) => setTimeout(resolve, 500)); - - // Make sure to clean up any instances in the store - Object.keys(targetCalendar.events).forEach((eventId) => { - if (eventId.split("/")[0] === baseUID) { - dispatch( - removeEvent({ calendarUid: calId, eventUid: eventId }) - ); - } - }); - } catch (err) { - console.error("Failed to delete recurring event:", err); - // Even if deletion fails, continue with creating the new event - } - } else { - // Normal update for recurring events - dispatch( - updateSeriesAsync({ - cal: targetCalendar, - event: { ...newEvent, recurrenceId }, - }) - ); - } + // Normal update for recurring events + dispatch( + updateSeriesAsync({ + cal: targetCalendar, + event: { ...newEvent, recurrenceId }, + }) + ); // Refresh calendars to ensure all instances are updated const calendarRange = getCalendarRange(new Date(start));