diff --git a/src/actions/__tests__/email-actions.test.js b/src/actions/__tests__/email-actions.test.js index 8431c6a0f..8bc4769b2 100644 --- a/src/actions/__tests__/email-actions.test.js +++ b/src/actions/__tests__/email-actions.test.js @@ -12,28 +12,34 @@ import { } from "openstack-uicore-foundation/lib/utils/actions"; import { saveEmailTemplate, - getEmailTemplates, + getEmailTemplate, + resetTemplateForm, deleteEmailTemplate, + queryTemplates, buildRenderPayload, normalizeRenderErrors } from "../email-actions"; import * as methods from "../../utils/methods"; -jest.mock("../../history", () => ({ push: jest.fn() })); - jest.mock("openstack-uicore-foundation/lib/utils/actions", () => ({ __esModule: true, ...jest.requireActual("openstack-uicore-foundation/lib/utils/actions"), postRequest: jest.fn(), putRequest: jest.fn(), getRequest: jest.fn(), - deleteRequest: jest.fn() + deleteRequest: jest.fn(), + fetchErrorHandler: jest.fn() })); jest.mock("../marketing-actions", () => ({ saveMarketingSetting: jest.fn() })); +jest.mock("../../history", () => ({ + __esModule: true, + default: { push: jest.fn() } +})); + const requestMock = (requestActionCreator, receiveActionCreator) => () => (dispatch) => { if (requestActionCreator && typeof requestActionCreator === "function") { @@ -49,23 +55,6 @@ const requestMock = }); }; -const getRequestMock = - (requestActionCreator, receiveActionCreator, _url, _handler, syncPayload) => - () => - (dispatch) => { - if (requestActionCreator && typeof requestActionCreator === "function") { - dispatch(requestActionCreator(syncPayload || {})); - } - return new Promise((resolve) => { - if (typeof receiveActionCreator === "function") { - dispatch(receiveActionCreator({ response: {} })); - } else { - dispatch(receiveActionCreator); - } - resolve({}); - }); - }; - const deleteRequestMock = (_requestActionCreator, receiveAction) => () => (dispatch) => new Promise((resolve) => { @@ -92,15 +81,6 @@ describe("saveEmailTemplate", () => { }); describe("create path (entity has no id)", () => { - it("returns a Promise", async () => { - const store = mockStore({}); - const result = store.dispatch( - saveEmailTemplate({ identifier: "test-template" }) - ); - expect(result).toBeInstanceOf(Promise); - await expect(result).resolves.toBeUndefined(); - }); - it("dispatches TEMPLATE_ADDED then STOP_LOADING on success", async () => { const store = mockStore({}); store.dispatch(saveEmailTemplate({ identifier: "test-template" })); @@ -116,15 +96,6 @@ describe("saveEmailTemplate", () => { }); describe("update path (entity has id)", () => { - it("returns a Promise", async () => { - const store = mockStore({}); - const result = store.dispatch( - saveEmailTemplate({ id: 1, identifier: "test-template" }) - ); - expect(result).toBeInstanceOf(Promise); - await expect(result).resolves.toBeUndefined(); - }); - it("dispatches TEMPLATE_UPDATED then STOP_LOADING on success", async () => { const store = mockStore({}); store.dispatch(saveEmailTemplate({ id: 1, identifier: "test-template" })); @@ -140,26 +111,88 @@ describe("saveEmailTemplate", () => { }); }); -describe("getEmailTemplates", () => { +describe("getEmailTemplate - stale response guard", () => { const middlewares = [thunk]; const mockStore = configureStore(middlewares); + const isTemplateFetchUrl = (url) => /\/mail-templates\/[^/]+$/.test(url); + + // each template fetch stays in flight until the test calls resolvers[id]() + const mockDeferredTemplateFetches = () => { + const resolvers = {}; + getRequest.mockImplementation( + (requestActionCreator, receiveActionCreator, url) => () => (dispatch) => { + if (isTemplateFetchUrl(url)) { + const id = Number(url.split("/").pop()); + return new Promise((resolve) => { + resolvers[id] = () => { + dispatch(receiveActionCreator({ response: { id } })); + resolve(); + }; + }); + } + if (requestActionCreator) dispatch(requestActionCreator({})); + dispatch(receiveActionCreator({ response: {} })); + return Promise.resolve(); + } + ); + return resolvers; + }; + beforeEach(() => { jest.spyOn(methods, "getAccessTokenSafely").mockResolvedValue("TOKEN"); - getRequest.mockImplementation(getRequestMock); }); afterEach(() => { jest.restoreAllMocks(); }); - it("dispatches REQUEST_TEMPLATES with page and perPage", async () => { + it("drops an older template's response after a newer template's response already landed", async () => { + const resolvers = mockDeferredTemplateFetches(); + const store = mockStore({}); - store.dispatch(getEmailTemplates("foo", 2, 25)); + + // User opens template 5, then quickly navigates to template 8 before + // template 5's fetch settles - both requests are genuinely in flight when + // template 8's response lands first. + store.dispatch(getEmailTemplate("5")); await flushPromises(); + store.dispatch(getEmailTemplate("8")); + await flushPromises(); + resolvers[8](); + await flushPromises(); + resolvers[5](); + await flushPromises(); + + const receivedIds = store + .getActions() + .filter((a) => a.type === "RECEIVE_TEMPLATE") + .map((a) => a.payload.response.id); + + expect(receivedIds).toEqual([8]); + }); + + it("drops a template's response that lands after resetTemplateForm supersedes it", async () => { + const resolvers = mockDeferredTemplateFetches(); + + const store = mockStore({}); + + // User opens template 5, then navigates back to the list and clicks + // "Add new" before template 5's fetch settles. + store.dispatch(getEmailTemplate("5")); + await flushPromises(); + store.dispatch(resetTemplateForm()); + await flushPromises(); + resolvers[5](); + await flushPromises(); + + const actionTypes = store.getActions().map((a) => a.type); - const req = store.getActions().find((a) => a.type === "REQUEST_TEMPLATES"); - expect(req.payload).toMatchObject({ page: 2, perPage: 25 }); + expect(actionTypes).not.toContain("RECEIVE_TEMPLATE"); + expect(actionTypes).toContain("RESET_TEMPLATE_FORM"); + expect(actionTypes.lastIndexOf("STOP_LOADING")).toBeGreaterThan( + actionTypes.indexOf("START_LOADING") + ); }); }); @@ -189,6 +222,32 @@ describe("deleteEmailTemplate", () => { }); }); +describe("queryTemplates", () => { + beforeEach(() => { + jest.spyOn(methods, "getAccessTokenSafely").mockResolvedValue("TOKEN"); + global.fetch = jest.fn(() => Promise.reject(new Error("network down"))); + }); + + afterEach(() => { + delete global.fetch; + jest.useRealTimers(); + jest.restoreAllMocks(); + }); + + it("settles the callback with no options when the search request fails", async () => { + jest.useFakeTimers(); + const callback = jest.fn(); + + queryTemplates("welcome", callback); + jest.runAllTimers(); + // flushPromises schedules on a timer, so it needs the real ones + jest.useRealTimers(); + await flushPromises(); + + expect(callback).toHaveBeenCalledWith([]); + }); +}); + describe("buildRenderPayload", () => { it("sends the mjml param when isMjml is true", () => { expect( diff --git a/src/actions/email-actions.js b/src/actions/email-actions.js index 62f54d116..9b4b5310b 100644 --- a/src/actions/email-actions.js +++ b/src/actions/email-actions.js @@ -11,9 +11,7 @@ * limitations under the License. * */ import T from "i18n-react/dist/i18n-react"; -import Swal from "sweetalert2"; import { - VALIDATE, getRequest, putRequest, postRequest, @@ -21,21 +19,24 @@ import { createAction, stopLoading, startLoading, - showSuccessMessage, authErrorHandler, + snackbarErrorHandler, + snackbarSuccessHandler, fetchResponseHandler, fetchErrorHandler, escapeFilterValue } from "openstack-uicore-foundation/lib/utils/actions"; import URI from "urijs"; import debounce from "lodash/debounce"; -import history from "../history"; -import { checkOrFilter, getAccessTokenSafely } from "../utils/methods"; +import { + checkOrFilter, + getAccessTokenSafely, + sequenced +} from "../utils/methods"; import { saveMarketingSetting } from "./marketing-actions"; import { DEBOUNCE_WAIT, DEFAULT_PER_PAGE, - ERROR_CODE_412, HUNDRED_PER_PAGE } from "../utils/constants"; @@ -97,17 +98,32 @@ export const getEmailTemplates = createAction(REQUEST_TEMPLATES), createAction(RECEIVE_TEMPLATES), `${window.EMAIL_API_BASE_URL}/api/v1/mail-templates`, - authErrorHandler, + snackbarErrorHandler, { order, orderDir, term, page, perPage } - )(params)(dispatch).then(() => { - dispatch(stopLoading()); - }); + )(params)(dispatch) + .finally(() => { + dispatch(stopLoading()); + }) + .catch(() => {}); }; +// Sequence-guard (see sequenced()): EditEmailTemplatePage dispatches a fresh +// getEmailTemplate(id) on every route param change, and concurrent calls for +// different template ids never abort each other - a stale response landing +// after a newer one (or after a reset) would overwrite the store's entity +// with the wrong template's data, and EmailTemplateForm's loadedEntityIdRef +// check would silently reseed the visible form with it. guardedDispatch +// drops the RECEIVE/loading dispatches from a superseded call. +const getEmailTemplateSeq = sequenced(); + export const getEmailTemplate = (templateId) => async (dispatch) => { + const { isCurrent, guardedDispatch } = getEmailTemplateSeq(dispatch); const accessToken = await getAccessTokenSafely(); - dispatch(startLoading()); + // Superseded while awaiting the token -> don't fire a request at all. + if (!isCurrent()) return Promise.resolve(); + + guardedDispatch(startLoading()); const params = { access_token: accessToken, expand: "parent,versions" }; @@ -115,13 +131,13 @@ export const getEmailTemplate = (templateId) => async (dispatch) => { null, createAction(RECEIVE_TEMPLATE), `${window.EMAIL_API_BASE_URL}/api/v1/mail-templates/${templateId}`, - authErrorHandler - )(params)(dispatch).then(() => { - dispatch(stopLoading()); - }); + snackbarErrorHandler + )(params)(guardedDispatch).finally(() => guardedDispatch(stopLoading())); }; export const resetTemplateForm = () => (dispatch) => { + getEmailTemplateSeq(dispatch); // invalidates any in-flight getEmailTemplate + dispatch(stopLoading()); dispatch(createAction(RESET_TEMPLATE_FORM)({})); }; @@ -141,12 +157,17 @@ export const saveEmailTemplate = createAction(TEMPLATE_UPDATED), `${window.EMAIL_API_BASE_URL}/api/v1/mail-templates/${entity.id}`, normalizedEntity, - customErrorHandler, + snackbarErrorHandler, entity )(params)(dispatch) .then(() => { if (!noAlert) - dispatch(showSuccessMessage(T.translate("emails.template_saved"))); + dispatch( + snackbarSuccessHandler({ + title: T.translate("general.success"), + html: T.translate("emails.template_saved") + }) + ); }) .finally(() => { dispatch(stopLoading()); @@ -157,12 +178,16 @@ export const saveEmailTemplate = createAction(TEMPLATE_ADDED), `${window.EMAIL_API_BASE_URL}/api/v1/mail-templates`, normalizedEntity, - customErrorHandler, + snackbarErrorHandler, entity )(params)(dispatch) - .then((payload) => { - dispatch(showSuccessMessage(T.translate("emails.template_created"))); - history.push(`/app/emails/templates/${payload.response.id}`); + .then(() => { + dispatch( + snackbarSuccessHandler({ + title: T.translate("general.success"), + html: T.translate("emails.template_created") + }) + ); }) .finally(() => { dispatch(stopLoading()); @@ -181,8 +206,8 @@ export const deleteEmailTemplate = (templateId) => async (dispatch) => { createAction(TEMPLATE_DELETED)({ templateId }), `${window.EMAIL_API_BASE_URL}/api/v1/mail-templates/${templateId}`, null, - authErrorHandler - )(params)(dispatch).then(() => { + snackbarErrorHandler + )(params)(dispatch).finally(() => { dispatch(stopLoading()); }); }; @@ -274,7 +299,11 @@ export const queryTemplates = debounce(async (input, callback) => { callback(options); }) - .catch(fetchErrorHandler); + .catch((err) => { + fetchErrorHandler(err); + // settle the callback so callers can clear their loading state + callback([]); + }); }, DEBOUNCE_WAIT); /** ********************************************************************************************************* */ @@ -351,11 +380,13 @@ export const getSentEmails = createAction(REQUEST_EMAILS), createAction(RECEIVE_EMAILS), `${window.EMAIL_API_BASE_URL}/api/v1/mails`, - authErrorHandler, + snackbarErrorHandler, { order, orderDir, term, page, perPage, filters } - )(params)(dispatch).then(() => { - dispatch(stopLoading()); - }); + )(params)(dispatch) + .finally(() => { + dispatch(stopLoading()); + }) + .catch(() => {}); }; export const updateTemplateJsonData = (data) => async (dispatch) => @@ -380,10 +411,12 @@ export const getAllClients = () => async (dispatch) => { createAction(REQUEST_EMAIL_CLIENTS), createAction(RECEIVE_EMAIL_CLIENTS), `${window.EMAIL_API_BASE_URL}/api/v1/clients`, - authErrorHandler - )(params)(dispatch).then(() => { - dispatch(stopLoading()); - }); + snackbarErrorHandler + )(params)(dispatch) + .finally(() => { + dispatch(stopLoading()); + }) + .catch(() => {}); }; /** ********************************************************************************************************* */ @@ -437,43 +470,6 @@ export const saveMarketingEmailSettings = }) ); -export const customErrorHandler = (err, res) => (dispatch) => { - const code = err.status; - let msg = ""; - - dispatch(stopLoading()); - - switch (code) { - case ERROR_CODE_412: - if (Array.isArray(err.response.body)) { - err.response.body.forEach((er) => { - msg += `${er}
`; - }); - } else { - for (const [key, value] of Object.entries(err.response.body)) { - if (isNaN(key)) { - msg += `${key}: `; - } - - msg += `${value}
`; - } - } - - Swal.fire("Validation error", msg, "warning"); - - if (err.response.body.errors) { - dispatch({ - type: VALIDATE, - payload: { errors: err.response.body } - }); - } - - break; - default: - dispatch(authErrorHandler(err, res)); - } -}; - const parseFilters = (filters) => { let filter = {}; diff --git a/src/actions/sponsor-reports-actions.js b/src/actions/sponsor-reports-actions.js index 2f7112fdf..c0e94197a 100644 --- a/src/actions/sponsor-reports-actions.js +++ b/src/actions/sponsor-reports-actions.js @@ -13,7 +13,8 @@ import { getAccessTokenSafely, isPositiveIntId, escapeFilterValue, - toCSV + toCSV, + sequenced } from "../utils/methods"; import { DEFAULT_CURRENT_PAGE, @@ -73,35 +74,6 @@ export const SET_PURCHASE_DETAILS_BY_ITEM_SORT = // line for one item together — the shape the warehouse pull sheet needs. export const LINES_ORDER_BY_ITEM = "item_code"; -// Per-thunk sequence-token factory guarding against stale-response commits. -// Two concurrent invocations of the same thunk (different filters/page/sponsor) -// carry different getRequest abort keys (only access_token is stripped from the -// key), so they never cancel each other and whichever response lands LAST would -// win — even the older one. Each begin(dispatch) bumps the thunk's counter and -// returns helpers scoped to that invocation: -// isCurrent() — false once a newer invocation has begun -// guardedDispatch(a) — forwards to dispatch only while current, so a -// superseded invocation cannot mutate state at all -// (REQUEST, RECEIVE, error handlers, start/stopLoading). -// The newest invocation is always current, so the overlay is always cleared by -// whichever call finishes last. A STALE call's 401 re-login is also dropped -// (uicore authErrorHandler reaches doLogin by dispatching a thunk through the -// dispatch it is given) — safe: the fresh call carries the same token, so it -// 401s too and drives the re-login, and uicore's isClearingSessionState guard -// dedupes concurrent attempts. -const sequenced = () => { - let seq = 0; - return (dispatch) => { - seq += 1; - const mySeq = seq; - return { - isCurrent: () => mySeq === seq, - guardedDispatch: (action) => { - if (mySeq === seq) dispatch(action); - } - }; - }; -}; // One sequence per thunk that commits fetched data to the store. const purchaseDetailsSeq = sequenced(); const purchaseLinesSeq = sequenced(); diff --git a/src/components/forms/__tests__/email-template-form.test.js b/src/components/forms/__tests__/email-template-form.test.js index a90b57e15..ee13a8d65 100644 --- a/src/components/forms/__tests__/email-template-form.test.js +++ b/src/components/forms/__tests__/email-template-form.test.js @@ -7,22 +7,30 @@ import { beforeEach, afterEach } from "@jest/globals"; -import { render, act, fireEvent } from "@testing-library/react"; +import { render, act, fireEvent, screen, within } from "@testing-library/react"; +import mjml2html from "mjml-browser"; +import showConfirmDialog from "../../mui/showConfirmDialog"; +import { EMAIL_TEMPLATE_TYPE_MJML } from "../../../utils/constants"; import EmailTemplateForm from "../email-template-form"; -// Mock heavy children that don't matter for the effect logic under test. jest.mock("@uiw/react-codemirror", () => ({ __esModule: true, - default: () => null + default: ({ id, value, onChange }) => ( +