FE: CFP reopen notification, recipient selection and Notify button - #1050
FE: CFP reopen notification, recipient selection and Notify button#1050caseylocker wants to merge 17 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds the front-end UI and client-side wiring for sending CFP “submission reopened” notification emails to selected recipients (submitter/speakers/moderator) from the event edit form, including recipient-row modeling and the thunk that calls the (future) API endpoint.
Changes:
- Wires a new
notifySubmissionReopenedthunk from the edit page down intoEventForm. - Implements recipient row modeling (dedupe by identity, merge by normalized email) and payload building for the notify endpoint.
- Adds UI (checkbox recipient selection + confirm dialog + “Notify selected” button), i18n strings, and comprehensive unit tests for model/UI/action behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/pages/events/edit-summit-event-page.js | Wires the new notify thunk through page props, JSX props, and connect. |
| src/models/reopen-notification-recipients.js | Adds recipient-row construction and conversion to the notify payload shape. |
| src/i18n/en.json | Adds new strings for notify UI labels, confirm copy, roles, and success message. |
| src/components/forms/event-form.js | Adds notify selection UI, confirm flow, selection intersection logic, and invokes the notify thunk. |
| src/components/forms/tests/event-form-notify.test.js | Adds tests for recipient model behavior and notify UI behavior in EventForm. |
| src/actions/event-actions.js | Adds notifySubmissionReopened thunk and supporting action type constant. |
| src/actions/tests/event-actions-notify.test.js | Adds action tests verifying request shape, loading flags, success count handling, and stop-loading on failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@caseylocker there are many review comments that I keep asking over and over again, like the unnecessary comments, or the meaningless tests. Please make sure to review those yourself before sending the PR to review.
Also please clean up the models directory, its purpose is not to hold util methods for specific pages
| expect(snackbar.payload.type).toBe("success"); | ||
| }); | ||
|
|
||
| it("stops loading even when the request rejects", async () => { |
There was a problem hiding this comment.
please lets only have meaningful tests
There was a problem hiding this comment.
Cut from 37 to 19. Gone: the ordering assertion on startLoading, tests for inputs the API cannot produce (speaker ids are serialised as ints), render tests that re-asserted row-model behaviour already covered at the unit level, and four that only reached their state by calling rerender directly, which bypasses the page's if (loading) return null. That gate also meant the post-confirm intersection could never narrow anything in the running app, so that code is gone too, along with its tests.
Kept the one you flagged here. The thunk's .finally is the only thing that clears loading on rejection, and the page renders null while loading is set, so a stuck flag blanks the page rather than leaving a spinner.
| 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 |
There was a problem hiding this comment.
please only add meaningful comments
There was a problem hiding this comment.
Removed. Went through the whole PR and trimmed comments.
| [ROLE.SPEAKER]: "edit_event.notify_role_speaker" | ||
| }; | ||
|
|
||
| const NOTIFY_ROW_GAP = 7; |
There was a problem hiding this comment.
No good reason. I assumed no-magic-numbers would flag an inline gap, but the rule resolves with detectObjects: false, so a numeric object value is never flagged. Constants removed and the values inlined as 10, matching the other inline gaps in this file.
| 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. |
There was a problem hiding this comment.
no need for this comment. When you write a comment think if it really needs a comment, if we have to explain in a comment why a variable has an empty array as default every time, then the code will be 90% comments
| if (confirmed) onCloseSubmission(entity.id)?.catch(() => {}); | ||
| } | ||
|
|
||
| // Persisted entity, not the editable one: `include_submitter` carries no identity, |
There was a problem hiding this comment.
this can be one line: "state entity doesn't include id"
There was a problem hiding this comment.
Removed it rather than shortened it, since the suggested wording would not have been accurate: state.entity is initialised as { ...props.entity }, so it does carry the id. The actual reason for reading props.entity is that state.entity is the editable copy, and include_submitter is a bare boolean the server resolves against the saved creator, so an unsaved submitter swap would relabel the row while the send still reached the persisted one. That is in the PR description now. Happy to put a one-line version back here if you would rather it live in the code.
| 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. |
There was a problem hiding this comment.
make it shorter, comments should be hints, not a thorough explanation
| {speakerDeepLink} | ||
| </span> | ||
| )} | ||
| <div style={{ flexBasis: "100%" }}> |
There was a problem hiding this comment.
Try not to keep growing an already extremely large file, this whole section could be an auxiliary component and only add one line to this render.
There was a problem hiding this comment.
Done. The block is now ReopenNotifyPanel, and render gets one line. While moving it I also turned event-form.js into event-form/index.js with the panel, its utils and the tests beside it, matching promocode-form/. Every external importer resolves unchanged.
| @@ -0,0 +1,136 @@ | |||
| /** | |||
There was a problem hiding this comment.
@caseylocker this directory models used to hold only classes with behavior that were reused accross pages and components. Now you started adding util files with methods that duplicate code and are only used on a single page or component. Please refactor this directory, remove all files that are used by a single owner, move the specific methods used on a single source to a local utils file under the same directory, define which methods are global and are reused across clients and put them in utils
There was a problem hiding this comment.
Fair. My file is out of src/models/ and now lives next to its only consumer as event-form/utils.js, so this PR no longer adds to the problem.
The wider cleanup I have left alone here, since it is other features' code: app-config.js and speakers-report.js each have a single importer, and materializer-allowlist.js is used only by the allowlist panel. Only member.js and lead-report-settings.js are genuinely shared.
|
@caseylocker please review @santipalenque comments |
…ests Responds to the review on #1050. - Drop the narrative comments added by this PR. The Apache header stays. - Extract the recipient block into ReopenNotifyPanel so render gains one line instead of ~57. toggleNotifyRecipient now needs a constructor bind: it was only ever called inside an arrow, and is passed as a bare prop. - Move event-form.js to event-form/index.js and colocate ReopenNotifyPanel, utils and the tests beside it, matching promocode-form. External importers resolve unchanged. src/models/reopen-notification-recipients.js is gone; src/models/ is back to its previous contents. - Inline the two hoisted spacing constants. no-magic-numbers resolves with detectObjects:false, so a numeric object value was never flagged. - Drop the post-confirm intersection and the submitter pin. The page returns null while loading is set, and every thunk that replaces the entity starts loading before its request, so EventForm is unmounted whenever props.entity changes. The intersection could never narrow anything in the running app. - Cut tests to 19. Removed the four that reached their state by calling rerender directly, bypassing that loading gate, plus duplicates of the row model already covered in utils.test.js. Co-Authored-By: Claude <noreply@anthropic.com>
|
All addressed. Comments: cleaned. Tests: 37 down to 19, detail in the thread on the action test. On the repeats, that is a fair hit. The comment density and the test volume were both things I should have caught before sending this out, not after. The PR description is updated. |
Co-Authored-By: Claude <noreply@anthropic.com>
…est can fail Co-Authored-By: Claude <noreply@anthropic.com>
…tion 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.
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.
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.
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 <noreply@anthropic.com>
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.
Round 4 removed the post-intersection empty-payload guard because it conflicted with a required test that asserted the send thunk was called with a fully-empty payload. That test was mis-specified: relying on the not-yet-built endpoint's 412 to explain an empty send couples this change to an untested contract, and if the eventual implementation instead returns 200 with a zero count, the admin sees a false success snackbar for a send that never happened. The client already knows locally when there is nothing left to send. Restores the guard and fixes the test to assert the thunk is NOT called once the submitter pin (round 4) intersects the payload down to empty, rather than asserting a call with an empty body.
- destructure props in handleNotifySpeakers (react/destructuring-assignment) - drop the duplicated spacing rationale; the reopen row above is a row-direction flex with gap 10 and abc-checkbox rows are 17px, not 22px - drop a test comment justifying getAllByLabelText by a MemberInput singleValue that the file's own member-input mock removed - drop an unused onNotifySubmissionReopened mock from a test that never sends
…ests Responds to the review on #1050. - Drop the narrative comments added by this PR. The Apache header stays. - Extract the recipient block into ReopenNotifyPanel so render gains one line instead of ~57. toggleNotifyRecipient now needs a constructor bind: it was only ever called inside an arrow, and is passed as a bare prop. - Move event-form.js to event-form/index.js and colocate ReopenNotifyPanel, utils and the tests beside it, matching promocode-form. External importers resolve unchanged. src/models/reopen-notification-recipients.js is gone; src/models/ is back to its previous contents. - Inline the two hoisted spacing constants. no-magic-numbers resolves with detectObjects:false, so a numeric object value was never flagged. - Drop the post-confirm intersection and the submitter pin. The page returns null while loading is set, and every thunk that replaces the entity starts loading before its request, so EventForm is unmounted whenever props.entity changes. The intersection could never narrow anything in the running app. - Cut tests to 19. Removed the four that reached their state by calling rerender directly, bypassing that loading gate, plus duplicates of the row model already covered in utils.test.js. Co-Authored-By: Claude <noreply@anthropic.com>
6847ef1 to
1b7ace9
Compare
|
@santipalenque your comments have been addressed. I also repointed to master in prep for the merge. |
ref: https://app.clickup.com/t/86bbkbrct
Front-end half of the CFP reopen notification. Spec: SDS §7 + §10.
Stacked on #1047
Base is
refactor/reopen-into-materials-panel, notmaster. The control lives inside the Materials panel that #1047 introduces. #1047 has since merged, so this can be unstacked withgit rebase --onto origin/master 7692c57whenever it is convenient.Backend
PUT .../submission-period/reopen/notify(86bbkbrue) is now deployed to dev, so the control works end to end there. Note the response field is a queue count, not a delivery count: the API returns the number of recipients whose email job was enqueued, and the success message says "queued" for that reason.What it does
An admin picks which of the submitter, the speakers and the moderator to email about a live reopen window. Rows come from the event already in state, so there is no extra request.
The part that is easy to get wrong
The UI shows rows; the endpoint takes two channels (
speaker_idsplusinclude_submitter). Rows are keyed by identity, never by email, and two identities merge into one row when their emails match case-insensitively, so a shared mailbox gets one message while the payload still names both people.include_submitteris a bare boolean carrying no identity, which the server resolves from the persistedgetCreatedBy(). So rows derive fromprops.entity(persisted), notstate.entity(editable) — otherwise an unsaved submitter swap would relabel the row while the send still reached the saved creator.Note for reviewers: the wiring is five touchpoints, not four
SDS §7 and the ClickUp ticket both say four. They omit the page-level
propsdestructure inedit-summit-event-page.js. Routing an action toEventFormtakes: the named import, theconst { … } = propsdestructure in the component body, the JSX prop, themapDispatchToPropsentry, and thethis.propsdestructure in the form's handler.Missing the page-level one does not produce
undefined. The module-scope import shadows through, so the JSX resolves to an un-dispatched action creator: no request, no error, suite green.Layout
event-form.jsis nowevent-form/index.js, withReopenNotifyPanel.jsx,utils.jsand the tests beside it, matchingpromocode-form/. Every external importer resolves unchanged. Nothing was added tosrc/models/.Testing
19 tests: the row model, the notify thunk, and the panel's behaviour through the real form. Full suite green at 172 suites / 1529 tests. Verified end to end on dev against a real reopened activity.