From e239bf391f377cfff87ddd85b7f08aaa9930c90c Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 13:51:01 -0500 Subject: [PATCH 01/17] feat(event-form): derive CFP reopen notification recipient rows --- .../forms/__tests__/event-form-notify.test.js | 230 ++++++++++++++++++ src/models/reopen-notification-recipients.js | 126 ++++++++++ 2 files changed, 356 insertions(+) create mode 100644 src/components/forms/__tests__/event-form-notify.test.js create mode 100644 src/models/reopen-notification-recipients.js diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js new file mode 100644 index 000000000..8b8b4a596 --- /dev/null +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -0,0 +1,230 @@ +import { + normalizeEmail, + buildRecipientRows, + toNotifyPayload, + ROLE +} from "../../../models/reopen-notification-recipients"; + +const speaker = (id, first, last, email) => ({ + id, + first_name: first, + last_name: last, + email +}); + +describe("normalizeEmail", () => { + it("trims and lowercases", () => { + expect(normalizeEmail(" Ada@Example.COM ")).toBe("ada@example.com"); + }); + + it("returns an empty string for a non-string", () => { + expect(normalizeEmail(undefined)).toBe(""); + expect(normalizeEmail(null)).toBe(""); + expect(normalizeEmail(42)).toBe(""); + }); +}); + +describe("buildRecipientRows", () => { + it("returns no rows for an entity with no people", () => { + // normalizeEventResponse coerces server nulls to "", which is why these are + // empty strings rather than null. + expect( + buildRecipientRows({ created_by: "", speakers: [], moderator: "" }) + ).toEqual([]); + }); + + it("builds a submitter row carrying includeSubmitter and no speaker id", () => { + const rows = buildRecipientRows({ + created_by: speaker(3, "Ada", "Lovelace", "ada@example.com"), + speakers: [], + moderator: "" + }); + + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ + key: "submitter", + name: "Ada Lovelace", + roles: [ROLE.SUBMITTER], + speakerIds: [], + includeSubmitter: true, + email: "ada@example.com", + disabled: false + }); + }); + + it("builds one row per speaker, keyed by speaker id", () => { + const rows = buildRecipientRows({ + created_by: "", + speakers: [ + speaker(7, "Grace", "Hopper", "grace@example.com"), + speaker(12, "Katherine", "Johnson", "katherine@example.com") + ], + moderator: "" + }); + + expect(rows.map((r) => r.key)).toEqual(["speaker:7", "speaker:12"]); + expect(rows[0].speakerIds).toEqual([7]); + expect(rows[0].includeSubmitter).toBe(false); + }); + + it("merges a moderator who is also a speaker into one row with both roles", () => { + const alan = speaker(9, "Alan", "Turing", "alan@example.com"); + const rows = buildRecipientRows({ + created_by: "", + speakers: [alan], + moderator: alan + }); + + expect(rows).toHaveLength(1); + expect(rows[0].key).toBe("speaker:9"); + expect(rows[0].roles).toEqual([ROLE.SPEAKER, ROLE.MODERATOR]); + expect(rows[0].speakerIds).toEqual([9]); + }); + + it("merges the moderator by id even when the two records disagree on email", () => { + // Identity dedupe runs before the email merge precisely so a stale email on + // one of the two records cannot split one person into two rows. + const rows = buildRecipientRows({ + created_by: "", + speakers: [speaker(9, "Alan", "Turing", "alan@example.com")], + moderator: speaker(9, "Alan", "Turing", "alan.turing@example.com") + }); + + expect(rows).toHaveLength(1); + expect(rows[0].speakerIds).toEqual([9]); + expect(rows[0].roles).toEqual([ROLE.SPEAKER, ROLE.MODERATOR]); + }); + + it("adds a moderator who is not in the speakers array as its own row", () => { + const rows = buildRecipientRows({ + created_by: "", + speakers: [speaker(7, "Grace", "Hopper", "grace@example.com")], + moderator: speaker(9, "Alan", "Turing", "alan@example.com") + }); + + expect(rows.map((r) => r.key)).toEqual(["speaker:7", "speaker:9"]); + expect(rows[1].roles).toEqual([ROLE.MODERATOR]); + }); + + it("merges a submitter who is also a speaker into one row spanning both channels", () => { + const rows = buildRecipientRows({ + created_by: speaker(3, "Ada", "Lovelace", "Ada@Example.com"), + speakers: [speaker(7, "Ada", "Lovelace", "ada@example.com")], + moderator: "" + }); + + expect(rows).toHaveLength(1); + // The submitter is built first, so it keeps the key. Key stability across + // renders is what lets the checked set be a list of keys. + expect(rows[0].key).toBe("submitter"); + expect(rows[0].roles).toEqual([ROLE.SUBMITTER, ROLE.SPEAKER]); + expect(rows[0].speakerIds).toEqual([7]); + expect(rows[0].includeSubmitter).toBe(true); + }); + + it("merges two distinct speakers whose emails differ only by case", () => { + const rows = buildRecipientRows({ + created_by: "", + speakers: [ + speaker(7, "Grace", "Hopper", "shared@example.com"), + speaker(12, "Katherine", "Johnson", "SHARED@example.com") + ], + moderator: "" + }); + + expect(rows).toHaveLength(1); + expect(rows[0].key).toBe("speaker:7"); + expect(rows[0].speakerIds).toEqual([7, 12]); + }); + + it("marks a row with no email disabled and never merges on the empty email", () => { + const rows = buildRecipientRows({ + created_by: "", + speakers: [ + speaker(7, "Grace", "Hopper", ""), + speaker(12, "Katherine", "Johnson", "") + ], + moderator: "" + }); + + expect(rows).toHaveLength(2); + expect(rows[0].disabled).toBe(true); + expect(rows[1].disabled).toBe(true); + }); + + it("falls back to the email when both name fields are blank", () => { + const rows = buildRecipientRows({ + created_by: "", + speakers: [speaker(7, "", "", " Grace@Example.com ")], + moderator: "" + }); + + expect(rows[0].name).toBe("Grace@Example.com"); + }); + + it("tolerates a missing speakers array", () => { + expect(buildRecipientRows({})).toEqual([]); + expect(buildRecipientRows(undefined)).toEqual([]); + }); +}); + +describe("toNotifyPayload", () => { + const rows = [ + { + key: "submitter", + speakerIds: [7], + includeSubmitter: true, + disabled: false + }, + { + key: "speaker:12", + speakerIds: [12], + includeSubmitter: false, + disabled: false + }, + { + key: "speaker:20", + speakerIds: [20], + includeSubmitter: false, + disabled: true + } + ]; + + it("is empty when nothing is checked", () => { + expect(toNotifyPayload(rows, [])).toEqual({ + speakerIds: [], + includeSubmitter: false + }); + }); + + it("unions the channels of every checked row", () => { + expect(toNotifyPayload(rows, ["submitter", "speaker:12"])).toEqual({ + speakerIds: [7, 12], + includeSubmitter: true + }); + }); + + it("drops both channels of a merged row when it is unchecked", () => { + // The regression this guards: clearing includeSubmitter but leaving speaker 7 + // in the payload still mails a person the admin unchecked. + expect(toNotifyPayload(rows, ["speaker:12"])).toEqual({ + speakerIds: [12], + includeSubmitter: false + }); + }); + + it("excludes a disabled row even if its key is somehow checked", () => { + expect(toNotifyPayload(rows, ["speaker:20"])).toEqual({ + speakerIds: [], + includeSubmitter: false + }); + }); + + it("de-duplicates speaker ids across checked rows", () => { + const overlapping = [ + { key: "a", speakerIds: [7], includeSubmitter: false, disabled: false }, + { key: "b", speakerIds: [7, 9], includeSubmitter: false, disabled: false } + ]; + expect(toNotifyPayload(overlapping, ["a", "b"]).speakerIds).toEqual([7, 9]); + }); +}); diff --git a/src/models/reopen-notification-recipients.js b/src/models/reopen-notification-recipients.js new file mode 100644 index 000000000..99b19aed0 --- /dev/null +++ b/src/models/reopen-notification-recipients.js @@ -0,0 +1,126 @@ +/** + * Copyright 2026 OpenStack Foundation + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * */ + +/** + * Recipient rows for the CFP reopen notification (SDS section 7). + * + * The UI shows ROWS; the endpoint takes TWO CHANNELS (speaker_ids plus + * include_submitter). A row is keyed by IDENTITY, never by email: "submitter" + * for the creator, "speaker:" for each speaker and the moderator. Email is + * only the MERGE PREDICATE, because two people can legitimately share a mailbox + * and the payload still has to name both of them. + * + * Two passes, in a fixed order (submitter, speakers in array order, moderator): + * 1. dedupe by speaker id. The moderator is usually also in speakers, and one + * id must never yield two rows. + * 2. merge by normalized email. An empty email is never a merge key, so two + * people with no address on file stay two rows. + * + * The first identity in that order keeps the row key, which is what makes keys + * stable across renders and lets the checked set be a plain list of keys. This + * is also why toggling a merged row clears every channel it spans: the key names + * the whole row, so there is no way to clear one channel and leave the other live. + */ + +export const ROLE = { + SUBMITTER: "submitter", + SPEAKER: "speaker", + MODERATOR: "moderator" +}; + +export const normalizeEmail = (email) => + typeof email === "string" ? email.trim().toLowerCase() : ""; + +const displayName = (person) => { + const name = `${person?.first_name || ""} ${person?.last_name || ""}`.trim(); + if (name) return name; + return typeof person?.email === "string" ? person.email.trim() : ""; +}; + +export const buildRecipientRows = (entity) => { + const identities = []; + const bySpeakerId = new Map(); + + // normalizeEventResponse coerces server nulls to "", so an absent submitter or + // moderator is the empty string. Guard on the id, not on null. + const submitter = entity?.created_by; + if (submitter?.id) { + identities.push({ + key: "submitter", + name: displayName(submitter), + roles: [ROLE.SUBMITTER], + speakerIds: [], + includeSubmitter: true, + email: normalizeEmail(submitter.email) + }); + } + + const addSpeaker = (person, role) => { + if (!person?.id) return; + const seen = bySpeakerId.get(person.id); + if (seen) { + if (!seen.roles.includes(role)) seen.roles.push(role); + return; + } + const identity = { + key: `speaker:${person.id}`, + name: displayName(person), + roles: [role], + speakerIds: [person.id], + includeSubmitter: false, + email: normalizeEmail(person.email) + }; + bySpeakerId.set(person.id, identity); + identities.push(identity); + }; + + const speakers = Array.isArray(entity?.speakers) ? entity.speakers : []; + speakers.forEach((s) => addSpeaker(s, ROLE.SPEAKER)); + addSpeaker(entity?.moderator, ROLE.MODERATOR); + + const rows = []; + const byEmail = new Map(); + + identities.forEach((identity) => { + const target = identity.email ? byEmail.get(identity.email) : null; + if (!target) { + const row = { ...identity, disabled: !identity.email }; + if (identity.email) byEmail.set(identity.email, row); + rows.push(row); + return; + } + identity.roles.forEach((role) => { + if (!target.roles.includes(role)) target.roles.push(role); + }); + target.speakerIds = [...target.speakerIds, ...identity.speakerIds]; + target.includeSubmitter = + target.includeSubmitter || identity.includeSubmitter; + }); + + return rows; +}; + +/** + * Union of the channels of every checked, enabled row. Disabled rows are filtered + * again here and not only at toggle time: a row can go disabled between mount and + * send if the entity is refetched. + */ +export const toNotifyPayload = (rows, checkedKeys) => { + const checked = rows.filter( + (row) => !row.disabled && checkedKeys.includes(row.key) + ); + return { + speakerIds: [...new Set(checked.flatMap((row) => row.speakerIds))], + includeSubmitter: checked.some((row) => row.includeSubmitter) + }; +}; From 5f48334fef1058132ed9d7abaeac68e3e9e36355 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 14:00:53 -0500 Subject: [PATCH 02/17] fix(event-form): key the speaker dedupe map by the string row key --- .../forms/__tests__/event-form-notify.test.js | 15 +++++++++++++++ src/models/reopen-notification-recipients.js | 10 +++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index 8b8b4a596..14c859d9c 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -95,6 +95,21 @@ describe("buildRecipientRows", () => { expect(rows[0].roles).toEqual([ROLE.SPEAKER, ROLE.MODERATOR]); }); + it("merges the moderator into the speaker row when the ids differ only by type", () => { + // A Map keys strictly but the row key string-coerces, so 7 and "7" would + // otherwise become two rows sharing the key "speaker:7". + const rows = buildRecipientRows({ + created_by: "", + speakers: [speaker(7, "Grace", "Hopper", "grace@example.com")], + moderator: speaker("7", "Grace", "Hopper", "grace.new@example.com") + }); + + expect(rows).toHaveLength(1); + expect(rows[0].key).toBe("speaker:7"); + expect(rows[0].roles).toEqual([ROLE.SPEAKER, ROLE.MODERATOR]); + expect(rows[0].speakerIds).toEqual([7]); + }); + it("adds a moderator who is not in the speakers array as its own row", () => { const rows = buildRecipientRows({ created_by: "", diff --git a/src/models/reopen-notification-recipients.js b/src/models/reopen-notification-recipients.js index 99b19aed0..8cd133db1 100644 --- a/src/models/reopen-notification-recipients.js +++ b/src/models/reopen-notification-recipients.js @@ -67,20 +67,24 @@ export const buildRecipientRows = (entity) => { const addSpeaker = (person, role) => { if (!person?.id) return; - const seen = bySpeakerId.get(person.id); + // One expression for both the map key and the row key: a Map compares keys + // strictly, so keying it on the raw id would let 7 and "7" miss each other + // and produce two rows sharing one key. + const key = `speaker:${person.id}`; + const seen = bySpeakerId.get(key); if (seen) { if (!seen.roles.includes(role)) seen.roles.push(role); return; } const identity = { - key: `speaker:${person.id}`, + key, name: displayName(person), roles: [role], speakerIds: [person.id], includeSubmitter: false, email: normalizeEmail(person.email) }; - bySpeakerId.set(person.id, identity); + bySpeakerId.set(key, identity); identities.push(identity); }; From 667c601292a0d671816f5d6db02d69ef55cb583d Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 14:03:57 -0500 Subject: [PATCH 03/17] feat(event-actions): add notifySubmissionReopened thunk Co-Authored-By: Claude --- .../__tests__/event-actions-notify.test.js | 107 ++++++++++++++++++ src/actions/event-actions.js | 41 +++++++ src/i18n/en.json | 1 + 3 files changed, 149 insertions(+) create mode 100644 src/actions/__tests__/event-actions-notify.test.js diff --git a/src/actions/__tests__/event-actions-notify.test.js b/src/actions/__tests__/event-actions-notify.test.js new file mode 100644 index 000000000..c5b130292 --- /dev/null +++ b/src/actions/__tests__/event-actions-notify.test.js @@ -0,0 +1,107 @@ +import configureStore from "redux-mock-store"; +import thunk from "redux-thunk"; +import { putRequest } from "openstack-uicore-foundation/lib/utils/actions"; +import * as methods from "../../utils/methods"; +import { notifySubmissionReopened } from "../event-actions"; + +jest.mock("i18n-react/dist/i18n-react", () => ({ + __esModule: true, + default: { translate: (key) => key } +})); + +jest.mock("openstack-uicore-foundation/lib/utils/actions", () => ({ + __esModule: true, + ...jest.requireActual("openstack-uicore-foundation/lib/utils/actions"), + putRequest: jest.fn() +})); + +const mockStore = configureStore([thunk]); + +describe("notifySubmissionReopened", () => { + const summitId = 5; + let store; + + beforeEach(() => { + jest.clearAllMocks(); + window.API_BASE_URL = "https://api.test"; + store = mockStore({ + currentSummitState: { currentSummit: { id: summitId } } + }); + jest.spyOn(methods, "getAccessTokenSafely").mockResolvedValue("TOKEN"); + }); + + const arrangeRequest = ( + result = Promise.resolve({ response: { recipients: 3 } }) + ) => { + putRequest.mockReturnValue(() => () => result); + }; + + it("PUTs the selection to the notify endpoint in the API's snake_case shape", async () => { + arrangeRequest(); + + await store.dispatch( + notifySubmissionReopened(42, { + speakerIds: [7, 12], + includeSubmitter: true + }) + ); + + expect(putRequest).toHaveBeenCalledTimes(1); + const [requestAction, , url, body] = putRequest.mock.calls[0]; + expect(requestAction).toBeNull(); + expect(url).toBe( + `https://api.test/api/v1/summits/${summitId}/presentations/42/submission-period/reopen/notify` + ); + expect(body).toEqual({ speaker_ids: [7, 12], include_submitter: true }); + }); + + it("dispatches startLoading before awaiting the access token", async () => { + arrangeRequest(); + let resolveToken; + methods.getAccessTokenSafely.mockReturnValue( + new Promise((resolve) => { + resolveToken = resolve; + }) + ); + + const pending = store.dispatch( + notifySubmissionReopened(42, { speakerIds: [7], includeSubmitter: false }) + ); + + // Synchronous assertion: the flag must already be set while the token + // promise is still unresolved, or a slow refresh leaves an unblocked window. + expect(store.getActions().map((a) => a.type)).toContain("START_LOADING"); + + resolveToken("TOKEN"); + await pending; + }); + + it("reports the recipient count from the response, not a client tally", async () => { + arrangeRequest(Promise.resolve({ response: { recipients: 3 } })); + + await store.dispatch( + notifySubmissionReopened(42, { speakerIds: [7], includeSubmitter: false }) + ); + + const snackbar = store + .getActions() + .find((a) => a.type === "SET_SNACKBAR_MESSAGE"); + expect(snackbar).toBeDefined(); + expect(snackbar.payload.type).toBe("success"); + }); + + it("stops loading even when the request rejects", async () => { + arrangeRequest(Promise.reject(new Error("412"))); + + await expect( + store.dispatch( + notifySubmissionReopened(42, { + speakerIds: [7], + includeSubmitter: false + }) + ) + ).rejects.toThrow(); + + expect(store.getActions().map((a) => a.type)).toContain("STOP_LOADING"); + }); +}); diff --git a/src/actions/event-actions.js b/src/actions/event-actions.js index 338c6f333..983759165 100644 --- a/src/actions/event-actions.js +++ b/src/actions/event-actions.js @@ -83,6 +83,10 @@ export const RECEIVE_EVENT_COMMENTS = "RECEIVE_EVENT_COMMENTS"; export const CHANGE_SEARCH_TERM = "CHANGE_SEARCH_TERM"; export const SUBMISSION_PERIOD_REOPENED = "SUBMISSION_PERIOD_REOPENED"; export const SUBMISSION_PERIOD_CLOSED = "SUBMISSION_PERIOD_CLOSED"; +// Dispatched so uicore's putRequest has a receive action to fire (it dispatches the +// second argument directly, and dispatch(null) throws). No reducer handles it: the +// response changes no entity state. +export const SUBMISSION_REOPEN_NOTIFIED = "SUBMISSION_REOPEN_NOTIFIED"; export const ATTENDEES_EXPECTED_LEARNT = "attendees_expected_learnt"; export const ATTENDING_MEDIA = "attending_media"; @@ -815,6 +819,43 @@ export const closeSubmissionPeriod = }); }; +export const notifySubmissionReopened = + (eventId, { speakerIds, includeSubmitter }) => + async (dispatch, getState) => { + const { currentSummitState } = getState(); + + // See reopenSubmissionPeriod: in-flight flag before the token await. + dispatch(startLoading()); + + const accessToken = await getAccessTokenSafely(); + const { currentSummit } = currentSummitState; + + const params = { access_token: accessToken }; + + return putRequest( + null, + createAction(SUBMISSION_REOPEN_NOTIFIED), + `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/presentations/${eventId}/submission-period/reopen/notify`, + { speaker_ids: speakerIds, include_submitter: includeSubmitter }, + snackbarErrorHandler + )(params)(dispatch) + .then((payload) => { + dispatch( + snackbarSuccessHandler({ + title: T.translate("general.success"), + html: T.translate("edit_event.notify_speakers_success", { + // The server's number, not the client's tally: a record can change + // between page load and send. + count: payload?.response?.recipients ?? 0 + }) + }) + ); + }) + .finally(() => { + dispatch(stopLoading()); + }); + }; + export const cloneEvent = (entity) => async (dispatch, getState) => { const { currentSummitState } = getState(); const accessToken = await getAccessTokenSafely(); diff --git a/src/i18n/en.json b/src/i18n/en.json index abf852acd..84fddd144 100644 --- a/src/i18n/en.json +++ b/src/i18n/en.json @@ -725,6 +725,7 @@ "reopen_confirm_title": "Reopen submission for this activity?", "reopen_confirm_text": "This lets the speaker edit this talk until {deadline}.", "reopen_submission_success": "Submission reopened.", + "notify_speakers_success": "Notification queued for {count} recipient(s).", "close_submission": "Close now", "close_submission_confirm_title": "Close the submission window now?", "close_submission_confirm_text": "The speaker will immediately lose the ability to edit this talk.", From f08874883aa23e7dc3e473972d5e4a975479f65c Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 14:13:25 -0500 Subject: [PATCH 04/17] test(event-actions): assert translate params so the recipient-count test can fail Co-Authored-By: Claude --- src/actions/__tests__/event-actions-notify.test.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/actions/__tests__/event-actions-notify.test.js b/src/actions/__tests__/event-actions-notify.test.js index c5b130292..41983bf45 100644 --- a/src/actions/__tests__/event-actions-notify.test.js +++ b/src/actions/__tests__/event-actions-notify.test.js @@ -1,12 +1,13 @@ import configureStore from "redux-mock-store"; import thunk from "redux-thunk"; import { putRequest } from "openstack-uicore-foundation/lib/utils/actions"; +import T from "i18n-react/dist/i18n-react"; import * as methods from "../../utils/methods"; import { notifySubmissionReopened } from "../event-actions"; jest.mock("i18n-react/dist/i18n-react", () => ({ __esModule: true, - default: { translate: (key) => key } + default: { translate: jest.fn((key) => key) } })); jest.mock("openstack-uicore-foundation/lib/utils/actions", () => ({ @@ -77,12 +78,19 @@ describe("notifySubmissionReopened", () => { }); it("reports the recipient count from the response, not a client tally", async () => { + // One speaker id goes in, the server says three recipients. Asserting the 3 is + // what makes this fail if the count is ever derived from the input instead. arrangeRequest(Promise.resolve({ response: { recipients: 3 } })); await store.dispatch( notifySubmissionReopened(42, { speakerIds: [7], includeSubmitter: false }) ); + expect(T.translate).toHaveBeenCalledWith( + "edit_event.notify_speakers_success", + { count: 3 } + ); + const snackbar = store .getActions() .find((a) => a.type === "SET_SNACKBAR_MESSAGE"); From ca305f5c6e124f494f543302ce100dec171f3d3a Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 14:21:21 -0500 Subject: [PATCH 05/17] feat(event-form): notify selected recipients about a CFP reopen window --- .../forms/__tests__/event-form-notify.test.js | 323 ++++++++++++++++++ src/components/forms/event-form.js | 118 ++++++- src/i18n/en.json | 10 +- src/pages/events/edit-summit-event-page.js | 10 +- 4 files changed, 455 insertions(+), 6 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index 14c859d9c..88327ecaa 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -1,3 +1,10 @@ +import React from "react"; +import { render, screen, waitFor, fireEvent } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import moment from "moment-timezone"; +import EventForm from "../event-form"; +import currentSummitMock from "../../../__mocks__/currentSummitMock"; +import showConfirmDialog from "../../mui/showConfirmDialog"; import { normalizeEmail, buildRecipientRows, @@ -5,6 +12,16 @@ import { ROLE } from "../../../models/reopen-notification-recipients"; +jest.mock("i18n-react/dist/i18n-react", () => ({ + __esModule: true, + default: { translate: (key) => key } +})); + +jest.mock("../../mui/showConfirmDialog", () => ({ + __esModule: true, + default: jest.fn() +})); + const speaker = (id, first, last, email) => ({ id, first_name: first, @@ -243,3 +260,309 @@ describe("toNotifyPayload", () => { expect(toNotifyPayload(overlapping, ["a", "b"]).speakerIds).toEqual([7, 9]); }); }); + +describe("EventForm reopen notification control", () => { + const marketplaceHoursType = currentSummitMock.event_types.find( + (t) => t.id === 935 + ); + + const baseProps = { + history: { push: jest.fn() }, + currentSummit: currentSummitMock, + levelOpts: [], + trackOpts: currentSummitMock.tracks, + typeOpts: currentSummitMock.event_types, + locationOpts: currentSummitMock.locations, + // is_enabled + a submission_end_date in the past are what make the reopen block + // applicable at all: the API only grants a reopen once the window has ended. + selectionPlansOpts: [ + { + id: 99, + is_enabled: true, + submission_end_date: moment().subtract(7, "days").unix(), + allowed_presentation_questions: [], + track_groups: [] + } + ], + rsvpTemplateOpts: [], + actionTypes: [], + entity: { + id: 0, + title: "Test Event", + type_id: marketplaceHoursType.id, + track_id: 0, + location_id: 0, + start_date: currentSummitMock.start_date + 3600, + end_date: currentSummitMock.start_date + 7200, + duration: 3600, + is_published: false, + description: "", + speakers: [], + moderator: null, + sponsors: [], + tags: [], + extra_questions: [], + materials: [] + }, + errors: {}, + onSubmit: jest.fn(), + onSaveIncomplete: jest.fn(), + onUpdate: jest.fn(), + onEventUpgrade: jest.fn(), + onAttach: jest.fn(), + onUnpublish: jest.fn(), + onMaterialDelete: jest.fn(), + onRemoveImage: jest.fn(), + onAddQAMember: jest.fn(), + onDeleteQAMember: jest.fn(), + feedbackState: { term: "", page: 1, comments: [] }, + getEventFeedback: jest.fn(), + fetchExtraQuestions: jest.fn(), + fetchExtraQuestionsAnswers: jest.fn(), + commentState: { filters: {}, comments: [] }, + getEventComments: jest.fn(), + onCommentDelete: jest.fn(), + deleteEventFeedback: jest.fn(), + getEventFeedbackCSV: jest.fn(), + onFlagChange: jest.fn(), + onClone: jest.fn() + }; + + const baseEntity = { + ...baseProps.entity, + id: 42, + title: "A TALK", + class_name: "Presentation", + type_id: 930, + track_id: 1, + selection_plan_id: 99, + // Note: the incumbent sets submission_reopened_until: "" (no grant); this + // copy carries a live one so the reopen-notification block renders. + submission_reopened_until: moment().add(24, "hours").unix(), + submission_reopened_by_id: 0, + submission_reopened_by: null + }; + + const withPeople = { + ...baseEntity, + created_by: { + id: 3, + first_name: "Ada", + last_name: "Lovelace", + email: "ada@example.com" + }, + speakers: [ + { + id: 7, + first_name: "Grace", + last_name: "Hopper", + email: "grace@example.com" + }, + { id: 12, first_name: "Katherine", last_name: "Johnson", email: "" } + ], + moderator: "" + }; + + // Panel mounts children only while expanded. + const renderEventForm = (overrides = {}) => { + const result = render(); + const materialsHeading = screen.queryByText(/^edit_event\.materials/, { + selector: ".panel-title" + }); + if (materialsHeading) fireEvent.click(materialsHeading); + return result; + }; + + beforeEach(() => { + jest.clearAllMocks(); + showConfirmDialog.mockResolvedValue(true); + }); + + it("does not offer the notify control without a live grant", () => { + renderEventForm({ + entity: { ...withPeople, submission_reopened_until: "" } + }); + expect( + screen.queryByRole("button", { name: "edit_event.notify_speakers" }) + ).not.toBeInTheDocument(); + }); + + it("offers the notify control with a live grant", () => { + renderEventForm({ entity: withPeople }); + expect( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ).toBeInTheDocument(); + }); + + it("checks nothing on mount and opens with the send button disabled", () => { + renderEventForm({ entity: withPeople }); + + screen + .getAllByRole("checkbox") + .filter((box) => box.id.startsWith("notify_recipient_")) + .forEach((box) => expect(box).not.toBeChecked()); + + expect( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ).toBeDisabled(); + }); + + it("renders a submitter who is also a speaker as one row", () => { + renderEventForm({ + entity: { + ...withPeople, + speakers: [ + { + id: 7, + first_name: "Ada", + last_name: "Lovelace", + email: "ada@example.com" + } + ] + } + }); + + // getAllByText, scoped by getAllByLabelText: the entity's created_by also + // renders as a MemberInput singleValue elsewhere in the form (unrelated to + // this control), so a plain text query would double-count the same name. + expect(screen.getAllByLabelText(/Ada Lovelace/)).toHaveLength(1); + }); + + it("renders a row with no email as disabled with the reason inline", () => { + renderEventForm({ entity: withPeople }); + + expect(screen.getByLabelText(/Katherine Johnson/)).toBeDisabled(); + expect(screen.getByText(/edit_event\.notify_no_email/)).toBeInTheDocument(); + }); + + it("sends the checked selection after confirmation", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + renderEventForm({ entity: withPeople, onNotifySubmissionReopened }); + + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + await waitFor(() => + expect(onNotifySubmissionReopened).toHaveBeenCalledWith(42, { + speakerIds: [7], + includeSubmitter: false + }) + ); + }); + + it("does not send when the admin cancels", async () => { + const onNotifySubmissionReopened = jest.fn(); + showConfirmDialog.mockResolvedValue(false); + renderEventForm({ entity: withPeople, onNotifySubmissionReopened }); + + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + expect(onNotifySubmissionReopened).not.toHaveBeenCalled(); + }); + + it("unchecking a merged submitter+speaker row clears BOTH channels", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + renderEventForm({ + entity: { + ...withPeople, + speakers: [ + { + id: 7, + first_name: "Ada", + last_name: "Lovelace", + email: "ada@example.com" + }, + { + id: 12, + first_name: "Grace", + last_name: "Hopper", + email: "grace@example.com" + } + ] + }, + onNotifySubmissionReopened + }); + + const merged = screen.getByLabelText(/Ada Lovelace/); + await userEvent.click(merged); + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + await userEvent.click(merged); // uncheck the merged row + + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + // The regression: clearing includeSubmitter but leaving 7 in speakerIds + // still mails Ada, who the admin just unchecked. + await waitFor(() => + expect(onNotifySubmissionReopened).toHaveBeenCalledWith(42, { + speakerIds: [12], + includeSubmitter: false + }) + ); + }); + + it("renders two speakers sharing an email as one row and sends both ids", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + renderEventForm({ + entity: { + ...withPeople, + created_by: "", + speakers: [ + { + id: 7, + first_name: "Grace", + last_name: "Hopper", + email: "shared@example.com" + }, + { + id: 12, + first_name: "Katherine", + last_name: "Johnson", + email: "SHARED@example.com" + } + ] + }, + onNotifySubmissionReopened + }); + + // One row, not two: they share a mailbox. The payload still has to name both. + expect( + screen.queryByLabelText(/Katherine Johnson/) + ).not.toBeInTheDocument(); + + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + await waitFor(() => + expect(onNotifySubmissionReopened).toHaveBeenCalledWith(42, { + speakerIds: [7, 12], + includeSubmitter: false + }) + ); + }); + + it("clears the selection after a successful send", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + renderEventForm({ entity: withPeople, onNotifySubmissionReopened }); + + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + await waitFor(() => + expect( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ).toBeDisabled() + ); + expect(screen.getByLabelText(/Grace Hopper/)).not.toBeChecked(); + }); +}); diff --git a/src/components/forms/event-form.js b/src/components/forms/event-form.js index 08ff6f8d1..f7909fe96 100644 --- a/src/components/forms/event-form.js +++ b/src/components/forms/event-form.js @@ -72,9 +72,20 @@ import CopyClipboard from "../buttons/copy-clipboard"; import EventRsvpList from "../rsvp/event-rsvp-list"; import EventRsvpInvitationList from "../rsvp/event-rsvp-invitation-list"; import showConfirmDialog from "../mui/showConfirmDialog"; +import { + buildRecipientRows, + toNotifyPayload, + ROLE +} from "../../models/reopen-notification-recipients"; const REOPEN_DEADLINE_FORMAT = "MMMM DD, YYYY h:mm a"; +const ROLE_LABEL = { + [ROLE.SUBMITTER]: "edit_event.notify_role_submitter", + [ROLE.MODERATOR]: "edit_event.notify_role_moderator", + [ROLE.SPEAKER]: "edit_event.notify_role_speaker" +}; + class EventForm extends React.Component { constructor(props) { super(props); @@ -87,7 +98,11 @@ class EventForm extends React.Component { publish: false, commentFilters: { ...props.commentState.filters }, reopenHours: DEFAULT_REOPEN_HOURS, - reopenCustomHours: "" + reopenCustomHours: "", + // Transient and per-form by design: there is no server-side record of who was + // notified last time, so remembering a selection would assert more than the + // backend can back up. Empty on mount, emptied again after a successful send. + notifyChecked: [] }; this.formRef = React.createRef(); @@ -137,6 +152,7 @@ class EventForm extends React.Component { this.handleSaveIncomplete = this.handleSaveIncomplete.bind(this); this.handleReopenSubmission = this.handleReopenSubmission.bind(this); this.handleCloseSubmission = this.handleCloseSubmission.bind(this); + this.handleNotifySpeakers = this.handleNotifySpeakers.bind(this); } componentDidMount() { @@ -868,6 +884,57 @@ class EventForm extends React.Component { if (confirmed) onCloseSubmission(entity.id)?.catch(() => {}); } + getRecipientRows() { + const { entity } = this.state; + return buildRecipientRows(entity); + } + + toggleNotifyRecipient(key) { + this.setState((prev) => ({ + notifyChecked: prev.notifyChecked.includes(key) + ? prev.notifyChecked.filter((k) => k !== key) + : [...prev.notifyChecked, key] + })); + } + + async handleNotifySpeakers() { + // Edit 3 of 4. Omitting this destructure leaves the button inert with the + // whole suite green. + const { onNotifySubmissionReopened } = this.props; + const { entity, notifyChecked } = this.state; + + const rows = this.getRecipientRows(); + const checked = rows.filter( + (row) => !row.disabled && notifyChecked.includes(row.key) + ); + if (checked.length === 0) return; + + const confirmed = await showConfirmDialog({ + title: T.translate("edit_event.notify_speakers_confirm_title", { + count: checked.length + }), + text: T.translate("edit_event.notify_speakers_confirm_text", { + deadline: this.getReopenDeadline().format(REOPEN_DEADLINE_FORMAT), + names: checked.map((row) => row.name).join(", ") + }), + iconType: "warning", + confirmButtonText: T.translate("edit_event.notify_speakers") + }); + + if (!confirmed) return; + + // See handleReopenSubmission: snackbarErrorHandler has already surfaced the + // API's message and an expired-window 412 is expected, so don't let the + // rejection escape as an unhandled one. + onNotifySubmissionReopened(entity.id, toNotifyPayload(rows, notifyChecked)) + ?.then(() => { + // Cleared rather than retained so a second press is a deliberate + // re-selection, not a repeat of whatever was ticked a moment ago. + this.setState({ notifyChecked: [] }); + }) + ?.catch(() => {}); + } + isNew() { const { entity } = this.state; return !entity.id; @@ -1051,7 +1118,8 @@ class EventForm extends React.Component { errors, speakerToAdd, reopenHours, - reopenCustomHours + reopenCustomHours, + notifyChecked } = this.state; const maxReopenHours = this.getMaxReopenHours(); @@ -2092,6 +2160,52 @@ class EventForm extends React.Component { {speakerDeepLink} )} +
+ + {this.getRecipientRows().map((row) => ( +
+ + this.toggleNotifyRecipient(row.key) + } + /> + + {row.disabled && ( + +   + {T.translate("edit_event.notify_no_email")} + + )} +
+ ))} + +
)} diff --git a/src/i18n/en.json b/src/i18n/en.json index 84fddd144..5babfefb4 100644 --- a/src/i18n/en.json +++ b/src/i18n/en.json @@ -732,7 +732,15 @@ "close_submission_success": "Submission window closed.", "reopened_until": "Reopened until {deadline}", "reopened_by": "by {admin}", - "reopen_deep_link_label": "Speaker link" + "reopen_deep_link_label": "Speaker link", + "notify_recipients_label": "Notify about this window", + "notify_speakers": "Notify selected", + "notify_role_submitter": "submitter", + "notify_role_speaker": "speaker", + "notify_role_moderator": "moderator", + "notify_no_email": "(no email address on file)", + "notify_speakers_confirm_title": "Notify {count} people?", + "notify_speakers_confirm_text": "An email with the submission link and the deadline ({deadline}) will be queued for: {names}. This can be sent again while the window is open." }, "edit_event_material": { "material": "Material", diff --git a/src/pages/events/edit-summit-event-page.js b/src/pages/events/edit-summit-event-page.js index 716fdbdd6..2a38f8fa5 100644 --- a/src/pages/events/edit-summit-event-page.js +++ b/src/pages/events/edit-summit-event-page.js @@ -33,7 +33,8 @@ import { cloneEvent, upgradeEvent, reopenSubmissionPeriod, - closeSubmissionPeriod + closeSubmissionPeriod, + notifySubmissionReopened } from "../../actions/event-actions"; import { unPublishEvent } from "../../actions/summit-builder-actions"; import { deleteEventMaterial } from "../../actions/event-material-actions"; @@ -245,7 +246,8 @@ function EditSummitEventPage(props) { cloneEvent, upgradeEvent, reopenSubmissionPeriod, - closeSubmissionPeriod + closeSubmissionPeriod, + notifySubmissionReopened } = props; if (loading) return null; @@ -317,6 +319,7 @@ function EditSummitEventPage(props) { onClone={cloneEvent} onReopenSubmission={reopenSubmissionPeriod} onCloseSubmission={closeSubmissionPeriod} + onNotifySubmissionReopened={notifySubmissionReopened} /> )} @@ -364,5 +367,6 @@ export default connect(mapStateToProps, { cloneEvent, upgradeEvent, reopenSubmissionPeriod, - closeSubmissionPeriod + closeSubmissionPeriod, + notifySubmissionReopened })(EditSummitEventPage); From f5520a08b1d35312ef673e7f6848bb9a3f66e075 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 14:41:40 -0500 Subject: [PATCH 06/17] fix(event-form): gate notify-send button on the actual sendable selection Derives the button's disabled state from toNotifyPayload instead of the raw checked-key count, so a checked recipient whose row disappears or goes disabled (e.g. removed from the talk, email cleared) can no longer leave an enabled button that silently does nothing on click. --- .../forms/__tests__/event-form-notify.test.js | 21 ++++++++++++++++++- src/components/forms/event-form.js | 15 +++++++++---- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index 88327ecaa..d9023f16b 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -422,7 +422,7 @@ describe("EventForm reopen notification control", () => { } }); - // getAllByText, scoped by getAllByLabelText: the entity's created_by also + // Switched from getAllByText to getAllByLabelText: the entity's created_by also // renders as a MemberInput singleValue elsewhere in the form (unrelated to // this control), so a plain text query would double-count the same name. expect(screen.getAllByLabelText(/Ada Lovelace/)).toHaveLength(1); @@ -565,4 +565,23 @@ describe("EventForm reopen notification control", () => { ); expect(screen.getByLabelText(/Grace Hopper/)).not.toBeChecked(); }); + + it("disables send when the only checked recipient leaves the talk", async () => { + // componentDidUpdate refreshes the entity but not the checked keys, so a key + // can outlive its row. The button must follow what is actually sendable. + const { rerender } = renderEventForm({ entity: withPeople }); + + await userEvent.click(screen.getByLabelText(/Grace Hopper/)); + expect( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ).toBeEnabled(); + + rerender( + + ); + + expect( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ).toBeDisabled(); + }); }); diff --git a/src/components/forms/event-form.js b/src/components/forms/event-form.js index f7909fe96..4b3801bac 100644 --- a/src/components/forms/event-form.js +++ b/src/components/forms/event-form.js @@ -898,8 +898,7 @@ class EventForm extends React.Component { } async handleNotifySpeakers() { - // Edit 3 of 4. Omitting this destructure leaves the button inert with the - // whole suite green. + // Omitting this destructure leaves the button inert with the whole suite green. const { onNotifySubmissionReopened } = this.props; const { entity, notifyChecked } = this.state; @@ -1124,6 +1123,14 @@ class EventForm extends React.Component { const maxReopenHours = this.getMaxReopenHours(); + const recipientRows = this.getRecipientRows(); + const notifySelection = toNotifyPayload(recipientRows, notifyChecked); + // The button asks the same function that builds the payload whether there is + // anything to send, so a checked key whose row has since disappeared or gone + // disabled cannot leave an enabled button that does nothing. + const canNotify = + notifySelection.speakerIds.length > 0 || notifySelection.includeSubmitter; + const { currentSummit, levelOpts, @@ -2164,7 +2171,7 @@ class EventForm extends React.Component { - {this.getRecipientRows().map((row) => ( + {recipientRows.map((row) => (
{T.translate("edit_event.notify_speakers")} From d27bb42ae96892fcd072aefec696acad70fe2141 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 15:05:58 -0500 Subject: [PATCH 07/17] fix(event-form): fix merged recipient names and stale notify payload A row merged across a shared mailbox kept only the first identity's name, hiding the second person from the checkbox label and the confirm dialog even though the payload still carried their id. buildRecipientRows now carries a names array and joins it into the display name on merge. handleNotifySpeakers also built the send payload from the rows captured before the confirm-dialog await. If the entity refreshes while the dialog is open and a merged row splits, the stale rows could still ship an id whose checkbox now reads unticked. The payload is now re-derived from fresh rows after the await; the dialog copy still describes what the admin saw when they pressed the button. --- .../forms/__tests__/event-form-notify.test.js | 111 +++++++++++++++++- src/components/forms/event-form.js | 9 +- src/models/reopen-notification-recipients.js | 6 + 3 files changed, 121 insertions(+), 5 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index d9023f16b..a017602ce 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -198,6 +198,40 @@ describe("buildRecipientRows", () => { expect(buildRecipientRows({})).toEqual([]); expect(buildRecipientRows(undefined)).toEqual([]); }); + + it("names both people on a row merged across a shared mailbox", () => { + const rows = buildRecipientRows({ + created_by: { + id: 3, + first_name: "Ada", + last_name: "Lovelace", + email: "shared@example.com" + }, + speakers: [speaker(7, "Grace", "Hopper", "SHARED@example.com")], + moderator: "" + }); + + expect(rows).toHaveLength(1); + // Hiding the second identity would be a lie about who the send reaches. + expect(rows[0].name).toBe("Ada Lovelace, Grace Hopper"); + expect(rows[0].speakerIds).toEqual([7]); + expect(rows[0].includeSubmitter).toBe(true); + }); + + it("does not repeat a name when the merged identities share one", () => { + const rows = buildRecipientRows({ + created_by: { + id: 3, + first_name: "Ada", + last_name: "Lovelace", + email: "shared@example.com" + }, + speakers: [speaker(7, "Ada", "Lovelace", "shared@example.com")], + moderator: "" + }); + + expect(rows[0].name).toBe("Ada Lovelace"); + }); }); describe("toNotifyPayload", () => { @@ -531,10 +565,12 @@ describe("EventForm reopen notification control", () => { onNotifySubmissionReopened }); - // One row, not two: they share a mailbox. The payload still has to name both. - expect( - screen.queryByLabelText(/Katherine Johnson/) - ).not.toBeInTheDocument(); + // One row, not two: they share a mailbox. The payload still has to name both, + // and (per the merged-name fix) so does the row's label: both names resolve + // to the same single checkbox rather than Katherine's being hidden. + expect(screen.getByLabelText(/Katherine Johnson/)).toBe( + screen.getByLabelText(/Grace Hopper/) + ); await userEvent.click(screen.getByLabelText(/Grace Hopper/)); await userEvent.click( @@ -584,4 +620,71 @@ describe("EventForm reopen notification control", () => { screen.getByRole("button", { name: "edit_event.notify_speakers" }) ).toBeDisabled(); }); + + it("does not send an identity whose row split away while the dialog was open", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + let resolveConfirm; + showConfirmDialog.mockReturnValue( + new Promise((resolve) => { + resolveConfirm = resolve; + }) + ); + + const merged = { + ...withPeople, + created_by: { + id: 3, + first_name: "Ada", + last_name: "Lovelace", + email: "shared@example.com" + }, + speakers: [ + { + id: 7, + first_name: "Grace", + last_name: "Hopper", + email: "shared@example.com" + } + ] + }; + + const { rerender } = renderEventForm({ + entity: merged, + onNotifySubmissionReopened + }); + + await userEvent.click(screen.getByLabelText(/Ada Lovelace/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + // The dialog is open. The entity refreshes and the merged row splits: Grace + // becomes her own, unticked row. + rerender( + + ); + + resolveConfirm(true); + + await waitFor(() => + expect(onNotifySubmissionReopened).toHaveBeenCalledWith(42, { + speakerIds: [], + includeSubmitter: true + }) + ); + }); }); diff --git a/src/components/forms/event-form.js b/src/components/forms/event-form.js index 4b3801bac..a72605216 100644 --- a/src/components/forms/event-form.js +++ b/src/components/forms/event-form.js @@ -922,10 +922,17 @@ class EventForm extends React.Component { if (!confirmed) return; + // Re-derived after the await: componentDidUpdate can refresh the entity while + // the dialog is open, and a merged row that has since split must not send an + // identity whose checkbox now reads unticked. + const currentRows = this.getRecipientRows(); + const payload = toNotifyPayload(currentRows, notifyChecked); + if (payload.speakerIds.length === 0 && !payload.includeSubmitter) return; + // See handleReopenSubmission: snackbarErrorHandler has already surfaced the // API's message and an expired-window 412 is expected, so don't let the // rejection escape as an unhandled one. - onNotifySubmissionReopened(entity.id, toNotifyPayload(rows, notifyChecked)) + onNotifySubmissionReopened(entity.id, payload) ?.then(() => { // Cleared rather than retained so a second press is a deliberate // re-selection, not a repeat of whatever was ticked a moment ago. diff --git a/src/models/reopen-notification-recipients.js b/src/models/reopen-notification-recipients.js index 8cd133db1..8f79f3cf1 100644 --- a/src/models/reopen-notification-recipients.js +++ b/src/models/reopen-notification-recipients.js @@ -58,6 +58,7 @@ export const buildRecipientRows = (entity) => { identities.push({ key: "submitter", name: displayName(submitter), + names: [displayName(submitter)], roles: [ROLE.SUBMITTER], speakerIds: [], includeSubmitter: true, @@ -79,6 +80,7 @@ export const buildRecipientRows = (entity) => { const identity = { key, name: displayName(person), + names: [displayName(person)], roles: [role], speakerIds: [person.id], includeSubmitter: false, @@ -109,6 +111,10 @@ export const buildRecipientRows = (entity) => { target.speakerIds = [...target.speakerIds, ...identity.speakerIds]; target.includeSubmitter = target.includeSubmitter || identity.includeSubmitter; + identity.names.forEach((n) => { + if (n && !target.names.includes(n)) target.names.push(n); + }); + target.name = target.names.join(", "); }); return rows; From 8de08a3f8c4ef637f9b76b13e5b7336ec2b6ad97 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 15:12:55 -0500 Subject: [PATCH 08/17] fix(event-form): intersect pre/post-dialog notify selections Round 2's re-derivation fixed the shrink case (a merged row splitting away a checked person) but opened the grow case: a refresh while the dialog is open could merge an unchecked identity onto an already-checked row, and the plain re-derivation would then ship that identity's id even though they were never ticked and never named in the dialog. The send now uses the intersection of the pre-await selection (what the admin saw and ticked) and the post-await selection (what is still valid). Intersecting can only ever shrink the set, never add anyone. --- .../forms/__tests__/event-form-notify.test.js | 69 +++++++++++++++++++ src/components/forms/event-form.js | 18 +++-- 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index a017602ce..3861ce0ef 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -687,4 +687,73 @@ describe("EventForm reopen notification control", () => { }) ); }); + + it("does not send an identity that merged onto a checked row while the dialog was open", async () => { + const onNotifySubmissionReopened = jest.fn().mockResolvedValue({}); + let resolveConfirm; + showConfirmDialog.mockReturnValue( + new Promise((resolve) => { + resolveConfirm = resolve; + }) + ); + + const separate = { + ...withPeople, + created_by: { + id: 3, + first_name: "Ada", + last_name: "Lovelace", + email: "ada@example.com" + }, + speakers: [ + { + id: 7, + first_name: "Grace", + last_name: "Hopper", + email: "grace@example.com" + } + ] + }; + + const { rerender } = renderEventForm({ + entity: separate, + onNotifySubmissionReopened + }); + + // Only Ada is ticked. Grace is deliberately left alone. + await userEvent.click(screen.getByLabelText(/Ada Lovelace/)); + await userEvent.click( + screen.getByRole("button", { name: "edit_event.notify_speakers" }) + ); + + // While the dialog is open, Grace's record changes to Ada's address, so the + // two identities now merge onto the row Ada's key points at. + rerender( + + ); + + resolveConfirm(true); + + // Grace was never ticked and the dialog never named her. + await waitFor(() => + expect(onNotifySubmissionReopened).toHaveBeenCalledWith(42, { + speakerIds: [], + includeSubmitter: true + }) + ); + }); }); diff --git a/src/components/forms/event-form.js b/src/components/forms/event-form.js index a72605216..d68c76e9b 100644 --- a/src/components/forms/event-form.js +++ b/src/components/forms/event-form.js @@ -922,11 +922,19 @@ class EventForm extends React.Component { if (!confirmed) return; - // Re-derived after the await: componentDidUpdate can refresh the entity while - // the dialog is open, and a merged row that has since split must not send an - // identity whose checkbox now reads unticked. - const currentRows = this.getRecipientRows(); - const payload = toNotifyPayload(currentRows, notifyChecked); + // Intersection, not replacement. `intended` is what the admin saw and ticked; + // `current` is what is still valid after any refresh that landed while the + // dialog was open. Sending the intersection can only ever shrink the set: + // a row that split away drops out via `current`, and a row that newly merged + // in cannot add anyone via `intended`. + const intended = toNotifyPayload(rows, notifyChecked); + const current = toNotifyPayload(this.getRecipientRows(), notifyChecked); + const payload = { + speakerIds: intended.speakerIds.filter((id) => + current.speakerIds.includes(id) + ), + includeSubmitter: intended.includeSubmitter && current.includeSubmitter + }; if (payload.speakerIds.length === 0 && !payload.includeSubmitter) return; // See handleReopenSubmission: snackbarErrorHandler has already surfaced the From 2c99229422cb11a6b49b3d2996dff25d11a72b20 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 15:32:29 -0500 Subject: [PATCH 09/17] chore(event-form): convention pass on notify recipients Un-export normalizeEmail: nothing in src imported it, only the test. Cover its trimming through buildRecipientRows instead and drop the trivial-helper describe block. Drop a comment that claimed the suite would stay green without the props destructure; the render tests fail without it. Co-Authored-By: Claude --- .../forms/__tests__/event-form-notify.test.js | 15 ++------------- src/components/forms/event-form.js | 1 - src/models/reopen-notification-recipients.js | 2 +- 3 files changed, 3 insertions(+), 15 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index 3861ce0ef..6a6d37362 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -6,7 +6,6 @@ import EventForm from "../event-form"; import currentSummitMock from "../../../__mocks__/currentSummitMock"; import showConfirmDialog from "../../mui/showConfirmDialog"; import { - normalizeEmail, buildRecipientRows, toNotifyPayload, ROLE @@ -29,18 +28,6 @@ const speaker = (id, first, last, email) => ({ email }); -describe("normalizeEmail", () => { - it("trims and lowercases", () => { - expect(normalizeEmail(" Ada@Example.COM ")).toBe("ada@example.com"); - }); - - it("returns an empty string for a non-string", () => { - expect(normalizeEmail(undefined)).toBe(""); - expect(normalizeEmail(null)).toBe(""); - expect(normalizeEmail(42)).toBe(""); - }); -}); - describe("buildRecipientRows", () => { it("returns no rows for an entity with no people", () => { // normalizeEventResponse coerces server nulls to "", which is why these are @@ -192,6 +179,8 @@ describe("buildRecipientRows", () => { }); expect(rows[0].name).toBe("Grace@Example.com"); + // The merge predicate is the trimmed, lowercased address. + expect(rows[0].email).toBe("grace@example.com"); }); it("tolerates a missing speakers array", () => { diff --git a/src/components/forms/event-form.js b/src/components/forms/event-form.js index d68c76e9b..1871f0cf3 100644 --- a/src/components/forms/event-form.js +++ b/src/components/forms/event-form.js @@ -898,7 +898,6 @@ class EventForm extends React.Component { } async handleNotifySpeakers() { - // Omitting this destructure leaves the button inert with the whole suite green. const { onNotifySubmissionReopened } = this.props; const { entity, notifyChecked } = this.state; diff --git a/src/models/reopen-notification-recipients.js b/src/models/reopen-notification-recipients.js index 8f79f3cf1..18d489927 100644 --- a/src/models/reopen-notification-recipients.js +++ b/src/models/reopen-notification-recipients.js @@ -38,7 +38,7 @@ export const ROLE = { MODERATOR: "moderator" }; -export const normalizeEmail = (email) => +const normalizeEmail = (email) => typeof email === "string" ? email.trim().toLowerCase() : ""; const displayName = (person) => { From f1a73862159a6b33cdf40af53777fe2273fbce81 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 15:47:13 -0500 Subject: [PATCH 10/17] fix(i18n): use recipient(s) in the notify confirm title --- src/i18n/en.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/i18n/en.json b/src/i18n/en.json index 5babfefb4..9f787b323 100644 --- a/src/i18n/en.json +++ b/src/i18n/en.json @@ -739,7 +739,7 @@ "notify_role_speaker": "speaker", "notify_role_moderator": "moderator", "notify_no_email": "(no email address on file)", - "notify_speakers_confirm_title": "Notify {count} people?", + "notify_speakers_confirm_title": "Notify {count} recipient(s)?", "notify_speakers_confirm_text": "An email with the submission link and the deadline ({deadline}) will be queued for: {names}. This can be sent again while the window is open." }, "edit_event_material": { From 08e8fdc3d510b66b09f6da27b7802f73270905a4 Mon Sep 17 00:00:00 2001 From: Casey Locker Date: Mon, 24 Aug 2026 16:27:19 -0500 Subject: [PATCH 11/17] fix(event-form): derive notify rows from persisted entity getRecipientRows built rows from state.entity, which is editable, unsaved form state. include_submitter is a bare boolean with no identity in it; the server resolves it from the persisted creator. An unsaved submitter swap could show and let the admin tick the wrong person while the payload still mailed whoever is actually saved, and a mid-dialog change to the persisted creator could make the boolean silently denote someone the dialog never named even after the round-3 intersection. getRecipientRows now reads props.entity (last server truth) instead of state.entity. The submitter identity is also pinned before the confirm dialog and compared after it, since a boolean channel cannot be intersected on identity the way speakerIds can. Also drops the post-intersection empty-payload guard: with the submitter now pinned, a mid-dialog identity change can legitimately intersect the payload down to nothing after the admin already confirmed a send. Letting the request go through and surface the server's own empty-selection 412 is consistent with treating a silent local skip as a lie about what was sent. --- .../forms/__tests__/event-form-notify.test.js | 92 +++++++++++++++++++ src/components/forms/event-form.js | 33 +++++-- 2 files changed, 119 insertions(+), 6 deletions(-) diff --git a/src/components/forms/__tests__/event-form-notify.test.js b/src/components/forms/__tests__/event-form-notify.test.js index 6a6d37362..5ce287896 100644 --- a/src/components/forms/__tests__/event-form-notify.test.js +++ b/src/components/forms/__tests__/event-form-notify.test.js @@ -21,6 +21,36 @@ jest.mock("../../mui/showConfirmDialog", () => ({ default: jest.fn() })); +// Lets a test drive the real onChange path to diverge state.entity from +// props.entity (an unsaved form edit), without pulling in the real react-select +// widget. +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/member-input", + () => ({ + __esModule: true, + default: ({ id, onChange }) => ( +
+ +); + +ReopenNotifyPanel.propTypes = { + rows: PropTypes.array.isRequired, + checked: PropTypes.array.isRequired, + onToggle: PropTypes.func.isRequired, + canNotify: PropTypes.bool.isRequired, + onNotify: PropTypes.func.isRequired +}; + +export default ReopenNotifyPanel; diff --git a/src/components/forms/event-form/__tests__/event-form-notify.test.js b/src/components/forms/event-form/__tests__/event-form-notify.test.js new file mode 100644 index 000000000..e11676541 --- /dev/null +++ b/src/components/forms/event-form/__tests__/event-form-notify.test.js @@ -0,0 +1,269 @@ +import React from "react"; +import { render, screen, waitFor, fireEvent } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import moment from "moment-timezone"; +import EventForm from "../index"; +import currentSummitMock from "../../../../__mocks__/currentSummitMock"; +import showConfirmDialog from "../../../mui/showConfirmDialog"; + +jest.mock("i18n-react/dist/i18n-react", () => ({ + __esModule: true, + default: { translate: (key) => key } +})); + +jest.mock("../../../mui/showConfirmDialog", () => ({ + __esModule: true, + default: jest.fn() +})); + +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/member-input", + () => ({ + __esModule: true, + default: ({ id, onChange }) => ( + - + )} diff --git a/src/models/reopen-notification-recipients.js b/src/components/forms/event-form/utils.js similarity index 66% rename from src/models/reopen-notification-recipients.js rename to src/components/forms/event-form/utils.js index 18d489927..5b682c8fe 100644 --- a/src/models/reopen-notification-recipients.js +++ b/src/components/forms/event-form/utils.js @@ -11,27 +11,6 @@ * limitations under the License. * */ -/** - * Recipient rows for the CFP reopen notification (SDS section 7). - * - * The UI shows ROWS; the endpoint takes TWO CHANNELS (speaker_ids plus - * include_submitter). A row is keyed by IDENTITY, never by email: "submitter" - * for the creator, "speaker:" for each speaker and the moderator. Email is - * only the MERGE PREDICATE, because two people can legitimately share a mailbox - * and the payload still has to name both of them. - * - * Two passes, in a fixed order (submitter, speakers in array order, moderator): - * 1. dedupe by speaker id. The moderator is usually also in speakers, and one - * id must never yield two rows. - * 2. merge by normalized email. An empty email is never a merge key, so two - * people with no address on file stay two rows. - * - * The first identity in that order keeps the row key, which is what makes keys - * stable across renders and lets the checked set be a plain list of keys. This - * is also why toggling a merged row clears every channel it spans: the key names - * the whole row, so there is no way to clear one channel and leave the other live. - */ - export const ROLE = { SUBMITTER: "submitter", SPEAKER: "speaker", @@ -51,8 +30,6 @@ export const buildRecipientRows = (entity) => { const identities = []; const bySpeakerId = new Map(); - // normalizeEventResponse coerces server nulls to "", so an absent submitter or - // moderator is the empty string. Guard on the id, not on null. const submitter = entity?.created_by; if (submitter?.id) { identities.push({ @@ -68,9 +45,6 @@ export const buildRecipientRows = (entity) => { const addSpeaker = (person, role) => { if (!person?.id) return; - // One expression for both the map key and the row key: a Map compares keys - // strictly, so keying it on the raw id would let 7 and "7" miss each other - // and produce two rows sharing one key. const key = `speaker:${person.id}`; const seen = bySpeakerId.get(key); if (seen) { @@ -120,11 +94,6 @@ export const buildRecipientRows = (entity) => { return rows; }; -/** - * Union of the channels of every checked, enabled row. Disabled rows are filtered - * again here and not only at toggle time: a row can go disabled between mount and - * send if the entity is refetched. - */ export const toNotifyPayload = (rows, checkedKeys) => { const checked = rows.filter( (row) => !row.disabled && checkedKeys.includes(row.key)