feat: move selection plans grid to mui#917
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModal-driven selection-plans list/editor using hooks; add per-page pagination through actions/reducer/components, update routing to redirect to the list, adjust imports, and add tests for save/delete flows. ChangesSelection Plans Modal-Driven UI Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/pages/selection-plans/selection-plan-list-page.js (2)
88-95: 💤 Low valueEdit flow goes through URL push — works but worth a brief inline comment.
Clicking edit pushes
/selection-plans/:idand relies on therouteSelectionPlanIdeffect to open the dialog. This is a nice property for deep-linking, but it's non-obvious and easy to break in future refactors. A one-line comment explaining the URL ↔ modal relationship would help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 88 - 95, Add a one-line inline comment in the handleEdit function explaining that calling history.push(`/app/summits/${currentSummit.id}/selection-plans/${selectionPlanId}`) intentionally updates the URL to trigger the routeSelectionPlanId effect which opens the edit modal/dialog (enables deep-linking), so future refactors know the URL change is relied upon to show the modal rather than directly invoking a modal open method.
60-73: ⚡ Quick winAdd a rejection handler to
openEditModal.If
getSelectionPlanorgetMarketingSettingsBySelectionPlanrejects (e.g., 404 on a deep-linked stale ID, network failure), the popup never opens and the user is left on the list with no signal. A.catchensures the route-driven open flow degrades gracefully.🛡️ Proposed fix
const openEditModal = (selectionPlanId) => { if (!selectionPlanId) return; getSelectionPlan(selectionPlanId) .then(() => getMarketingSettingsBySelectionPlan( selectionPlanId, null, DEFAULT_CURRENT_PAGE, MAX_PER_PAGE ) ) - .then(() => setOpenSelectionPlanPopup(true)); + .then(() => setOpenSelectionPlanPopup(true)) + .catch(() => { + if (routeSelectionPlanId) { + history.replace(`/app/summits/${currentSummit.id}/selection-plans`); + } + }); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 60 - 73, openEditModal's promise chain currently has no rejection handler so failures from getSelectionPlan or getMarketingSettingsBySelectionPlan leave the UI unchanged; add a .catch to the chain that handles errors (log/show a notification and/or update error state) and still calls setOpenSelectionPlanPopup(true) so the modal opens in a degraded state; keep references to openEditModal, getSelectionPlan, getMarketingSettingsBySelectionPlan and setOpenSelectionPlanPopup when implementing the catch behavior.src/layouts/selection-plan-layout.js (1)
41-52: 💤 Low valueLGTM, with a small readability note.
Routing is correct: the
exact strictroute on:selection_plan_id(\\d+)renders the list page (which auto-opens the edit dialog from the URL), while the non-exact one continues to delegate sub-paths (/extra-questions,/rating-types) toSelectionPlanIdLayout. Two<Route>entries with the samepathis unusual at first glance — a brief inline comment ("exact → list with auto-open dialog; non-exact → nested sub-pages") would save future readers a double-take.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/layouts/selection-plan-layout.js` around lines 41 - 52, Add a brief inline comment above the two Route entries explaining their intent: the exact/strict Route with path `${match.url}/:selection_plan_id(\\d+)` renders SelectionPlanListPage (which auto-opens the edit dialog from the URL), while the non-exact Route with the same path delegates nested sub-pages to SelectionPlanIdLayout (e.g., `/extra-questions`, `/rating-types`); place the comment near the two Route elements so future readers immediately understand why both routes share the same path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/forms/selection-plan-form.js`:
- Around line 191-205: handleSubmit currently chains onSubmit →
saveSelectionPlanSettings → onSaved with no error handling; update handleSubmit
to return the promise chain, add a .catch at the end, and guard the intermediate
result before using e.id. Concretely: in handleSubmit (method name) make the
call return this.props.onSubmit(this.state.entity).then((e) => { if (!e ||
!e.id) throw new Error("Missing id from onSubmit result"); return
this.props.saveSelectionPlanSettings(this.state.entity.marketing_settings,
e.id).then(() => { if (onSaved) onSaved(e); }); }).catch(err => { /* handle
error: set local state (e.g. this.setState({saveError: err, saving:false}))
and/or call this.props.onError(err) if provided, and rethrow or swallow as
appropriate */ }); ensure you reference onSubmit, saveSelectionPlanSettings,
onSaved and this.state.entity.marketing_settings in your changes.
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 174-242: Remove the redundant actions.edit entry from
tableOptions: locate the tableOptions constant and delete the actions: { edit: {
onClick: handleEdit } } block so that edit/delete behavior is provided only via
the top-level onEdit={handleEdit} and onDelete={handleDelete} props passed to
MuiTable; keep tableOptions containing sortCol and sortDir unchanged and ensure
no other references to actions.edit remain.
- Around line 246-254: Add an inline comment above the Dialog usage explaining
that focus management props (disableEnforceFocus, disableAutoFocus,
disableRestoreFocus) are intentionally disabled because EditSelectionPlanPage
triggers nested SweetAlert2 modals via Swal.fire() which conflict with MUI
Dialog's focus trap; mention that this is a deliberate tradeoff to allow those
modals to function and warn future maintainers not to re-enable those props
without addressing the SweetAlert2/MUI focus conflict.
- Around line 97-102: handleDelete currently performs a silent delete via
deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()) without
user confirmation or error handling; update this flow so the table shows a
confirmation dialog (add the deleteDialogBody prop to the MuiTable instance for
this page, matching the pattern used in sponsor users table and keep
confirmButtonColor="error"), and modify handleDelete to attach a .catch(...) to
deleteSelectionPlan to surface server errors to the user (e.g., via the existing
notification/snackbar mechanism) and only call refreshSelectionPlans() on
success; ensure you reference the handleDelete function and
deleteSelectionPlan/refreshSelectionPlans identifiers when making these changes.
- Around line 75-83: The first useEffect calling getSelectionPlans should
include all referenced variables (term, perPage, order, orderDir, and
getSelectionPlans) in its dependency array (or, if it truly must only run on
mount, replace the array with [] and add a comment explaining why it must not
re-run); the second effect that checks routeSelectionPlanId must include a
stable openEditModal in its deps—wrap openEditModal in useCallback to stabilize
its identity and then add openEditModal (and routeSelectionPlanId) to that
effect's dependency array so react-hooks/exhaustive-deps is satisfied.
---
Nitpick comments:
In `@src/layouts/selection-plan-layout.js`:
- Around line 41-52: Add a brief inline comment above the two Route entries
explaining their intent: the exact/strict Route with path
`${match.url}/:selection_plan_id(\\d+)` renders SelectionPlanListPage (which
auto-opens the edit dialog from the URL), while the non-exact Route with the
same path delegates nested sub-pages to SelectionPlanIdLayout (e.g.,
`/extra-questions`, `/rating-types`); place the comment near the two Route
elements so future readers immediately understand why both routes share the same
path.
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 88-95: Add a one-line inline comment in the handleEdit function
explaining that calling
history.push(`/app/summits/${currentSummit.id}/selection-plans/${selectionPlanId}`)
intentionally updates the URL to trigger the routeSelectionPlanId effect which
opens the edit modal/dialog (enables deep-linking), so future refactors know the
URL change is relied upon to show the modal rather than directly invoking a
modal open method.
- Around line 60-73: openEditModal's promise chain currently has no rejection
handler so failures from getSelectionPlan or getMarketingSettingsBySelectionPlan
leave the UI unchanged; add a .catch to the chain that handles errors (log/show
a notification and/or update error state) and still calls
setOpenSelectionPlanPopup(true) so the modal opens in a degraded state; keep
references to openEditModal, getSelectionPlan,
getMarketingSettingsBySelectionPlan and setOpenSelectionPlanPopup when
implementing the catch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e66a2e0b-b7c8-462d-8a25-e51770a30ccf
📒 Files selected for processing (8)
src/actions/selection-plan-actions.jssrc/components/forms/selection-plan-form.jssrc/components/inputs/import-modal/index.jsxsrc/layouts/selection-plan-id-layout.jssrc/layouts/selection-plan-layout.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/reducers/selection_plans/selection-plan-list-reducer.js
| const handleDelete = (row) => { | ||
| const selectionPlanId = row?.id || row; | ||
| if (!selectionPlanId) return; | ||
|
|
||
| deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether MuiTable wraps onDelete in a confirmation dialog and
# what `confirmButtonColor` actually controls.
fd -i 'mui' -t d
fd -i 'table' -t f --extension js --extension jsx --extension ts --extension tsx | xargs rg -nP -C3 'onDelete|confirmButtonColor' 2>/dev/null | head -200
rg -nP -C3 '"openstack-uicore-foundation/lib/components/mui/table"' --type=js --type=jsxRepository: fntechgit/summit-admin
Length of output: 10887
🏁 Script executed:
cat -n src/pages/selection-plans/selection-plan-list-page.js | sed -n '170,250p'Repository: fntechgit/summit-admin
Length of output: 2799
🏁 Script executed:
# Look for MuiTable component definition locally
find src/components -name "*table*" -o -name "*Table*" | head -20Repository: fntechgit/summit-admin
Length of output: 1142
🏁 Script executed:
# Search for MuiTable imports and their usage pattern
rg "from.*openstack-uicore-foundation.*table" -A 5 -B 2Repository: fntechgit/summit-admin
Length of output: 50378
🏁 Script executed:
sed -n '100,200p' src/pages/sponsors/sponsor-users-list-page/components/users-table.jsRepository: fntechgit/summit-admin
Length of output: 1332
🏁 Script executed:
sed -n '97,102p' src/pages/selection-plans/selection-plan-list-page.jsRepository: fntechgit/summit-admin
Length of output: 262
Add delete confirmation dialog and error handling to handleDelete.
handleDelete silently deletes selection plans without user confirmation. Unlike other MuiTable instances in the codebase (e.g., sponsor users table), this one lacks the deleteDialogBody prop required to display a confirmation dialog. confirmButtonColor="error" alone does not provide confirmation UX. Additionally, no .catch() is attached, so server-side failures leave the list unrefreshed and unsignaled to the user.
Suggested fix
const handleDelete = (row) => {
const selectionPlanId = row?.id || row;
if (!selectionPlanId) return;
- deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans());
+ deleteSelectionPlan(selectionPlanId)
+ .then(() => refreshSelectionPlans())
+ .catch(() => {
+ // surfaced via authErrorHandler; no-op
+ });
};Also add deleteDialogBody prop to MuiTable to match the pattern used elsewhere:
<MuiTable
// ...
onDelete={handleDelete}
+ deleteDialogBody={(plan) =>
+ T.translate("selection_plan_list.delete_confirmation_body", { plan })
+ }
confirmButtonColor="error"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDelete = (row) => { | |
| const selectionPlanId = row?.id || row; | |
| if (!selectionPlanId) return; | |
| deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); | |
| }; | |
| const handleDelete = (row) => { | |
| const selectionPlanId = row?.id || row; | |
| if (!selectionPlanId) return; | |
| deleteSelectionPlan(selectionPlanId) | |
| .then(() => refreshSelectionPlans()) | |
| .catch(() => { | |
| // surfaced via authErrorHandler; no-op | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 97 - 102,
handleDelete currently performs a silent delete via
deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()) without
user confirmation or error handling; update this flow so the table shows a
confirmation dialog (add the deleteDialogBody prop to the MuiTable instance for
this page, matching the pattern used in sponsor users table and keep
confirmButtonColor="error"), and modify handleDelete to attach a .catch(...) to
deleteSelectionPlan to surface server errors to the user (e.g., via the existing
notification/snackbar mechanism) and only call refreshSelectionPlans() on
success; ensure you reference the handleDelete function and
deleteSelectionPlan/refreshSelectionPlans identifiers when making these changes.
| } from "../../reducers/selection_plans/selection-plan-reducer"; | ||
| import history from "../../history"; | ||
|
|
||
| class SelectionPlanForm extends React.Component { |
There was a problem hiding this comment.
please refactor to functional component
There was a problem hiding this comment.
I thought this would be out of scope so I didn't do It initially but I'm adding this now.
| entity, | ||
| allowedMembers, | ||
| errors, | ||
| hideHeader = false, |
There was a problem hiding this comment.
do we really need a prop ? I don't see where this is used WITH the header
There was a problem hiding this comment.
Just did a refactor so this is not needed anymore! Thanks
| totalSelectionPlans: total, | ||
| currentPage: current_page, | ||
| lastPage: last_page | ||
| lastPage: last_page, |
There was a problem hiding this comment.
no need to do this if you are already doing it in the request action
| getSelectionPlans(term, currentPage, perPage, order, orderDir); | ||
|
|
||
| const handleEdit = (selectionPlan) => { | ||
| const selectionPlanId = selectionPlan?.id || selectionPlan; |
There was a problem hiding this comment.
why selectionPlan can be both an object or an id ? we should consolidate this on either one or the other
| if (!selectionPlanId) return; | ||
|
|
||
| const handleEdit = (selectionPlanId) => { | ||
| history.push( |
There was a problem hiding this comment.
@smarcet linking the user to /id kind of defeats the purpose of having the edit screen in a modal, shouldn't we just open the modal in this case ? or do we want the url to reflect the edit action ?
There was a problem hiding this comment.
The edit page for a selection plan uses sub-routes (e.g. /extra-questions, /rating-types) that depend on the selection_plan_id URL parameter being present. Removing the ID from the URL on edit would break navigation to those nested routes inside SelectionPlanIdLayout. Keeping the ID ensures the layout can resolve the correct selection plan context for all child routes.
There was a problem hiding this comment.
Not really, you can just open the popup on edit but keep the /id a valid route for the rest of the routes
| const handleNew = () => { | ||
| history.push(`/app/summits/${currentSummit.id}/selection-plans/new`); | ||
| resetSelectionPlanForm(); | ||
| setOpenSelectionPlanPopup(true); |
There was a problem hiding this comment.
on edit you do change the url, but on new not .. this is not consistent
There was a problem hiding this comment.
The /new path existed in the layout but was never functional — it just redirected back to the list. Creation is handled directly from the list page by opening the form with id=0. Once the plan is saved, the API returns the new ID and the user is redirected to the proper edit URL. There was no reason to keep a dead redirect route.
There was a problem hiding this comment.
Also, in task description it was requested to delete this route legacy route https://showadmin.dev.fnopen.com/app/summits/73/selection-plans/new should be removed
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
Assuming the form is out of scope. Good job @priscila-moneo
| }; | ||
|
|
||
| return ( | ||
| <form className="selection-plan-form"> |
There was a problem hiding this comment.
shouldn't we use MUI components ? or this file is out of scope ? cc @smarcet
There was a problem hiding this comment.
good observation, I had some time so I updated this file with MUI components
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
c8488e1 to
e66daff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/selection-plans/selection-plan-list-page.js (1)
96-100:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix
handleDeleteto accept the row id, not a row object.Line 97 returns early for the value
MuiTableactually sends toonDelete, so delete clicks become no-ops here.Suggested fix
- const handleDelete = (selectionPlan) => { - if (!selectionPlan?.id) return; - - deleteSelectionPlan(selectionPlan.id).then(() => refreshSelectionPlans()); + const handleDelete = (selectionPlanId) => { + if (!selectionPlanId) return; + + deleteSelectionPlan(selectionPlanId).then(() => refreshSelectionPlans()); };Based on learnings: When using
MuiTablefromopenstack-uicore-foundation/lib/components/mui/tablein this codebase,onDeleteis called with the primitive row identifier, not the full row object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 96 - 100, The handleDelete function currently expects a selectionPlan object and guards on selectionPlan?.id, but MuiTable's onDelete passes the primitive row id instead of the row object; update handleDelete to accept an id parameter (e.g., id) and return early if id is falsy, then call deleteSelectionPlan(id).then(() => refreshSelectionPlans()); adjust any references that call handleDelete to ensure they pass the id directly; keep function name handleDelete and preserve use of deleteSelectionPlan and refreshSelectionPlans for locating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 255-256: The IconButton rendering the CloseIcon (the element using
IconButton with onClick={handleClosePopup} and CloseIcon) lacks an accessible
name; add an aria-label (for example aria-label="Close" or a localized string)
to the IconButton so screen readers announce its purpose and optionally add a
tooltip/title if desired for sighted users.
---
Duplicate comments:
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 96-100: The handleDelete function currently expects a
selectionPlan object and guards on selectionPlan?.id, but MuiTable's onDelete
passes the primitive row id instead of the row object; update handleDelete to
accept an id parameter (e.g., id) and return early if id is falsy, then call
deleteSelectionPlan(id).then(() => refreshSelectionPlans()); adjust any
references that call handleDelete to ensure they pass the id directly; keep
function name handleDelete and preserve use of deleteSelectionPlan and
refreshSelectionPlans for locating the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e9228ae-e203-4da7-9b8d-a3a6c7c0c3f3
📒 Files selected for processing (9)
src/actions/selection-plan-actions.jssrc/components/forms/selection-plan-form.jssrc/components/inputs/import-modal/index.jsxsrc/layouts/selection-plan-id-layout.jssrc/layouts/selection-plan-layout.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/pages/sponsors/__tests__/edit-sponsor-page.test.jssrc/reducers/selection_plans/selection-plan-list-reducer.js
✅ Files skipped from review due to trivial changes (1)
- src/components/inputs/import-modal/index.jsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/pages/selection-plans/edit-selection-plan-page.js
- src/reducers/selection_plans/selection-plan-list-reducer.js
- src/layouts/selection-plan-layout.js
- src/actions/selection-plan-actions.js
- src/components/forms/selection-plan-form.js
| <IconButton size="small" onClick={handleClosePopup}> | ||
| <CloseIcon fontSize="small" /> |
There was a problem hiding this comment.
Add an accessible name to the close button.
The icon-only IconButton on Line 255 has no label, so assistive tech will announce a generic button. Add an aria-label.
Suggested fix
- <IconButton size="small" onClick={handleClosePopup}>
+ <IconButton
+ size="small"
+ onClick={handleClosePopup}
+ aria-label={T.translate("general.close")}
+ >
<CloseIcon fontSize="small" />
</IconButton>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 255 -
256, The IconButton rendering the CloseIcon (the element using IconButton with
onClick={handleClosePopup} and CloseIcon) lacks an accessible name; add an
aria-label (for example aria-label="Close" or a localized string) to the
IconButton so screen readers announce its purpose and optionally add a
tooltip/title if desired for sighted users.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/forms/selection-plan-form.js (1)
145-151: 💤 Low valueConsider guarding against undefined
marketing_settings.The code assumes
marketing_settingsexists when callinghasOwnProperty, but the render code uses optional chaining (?.) when reading these values. IfpropsEntity.marketing_settingsis ever undefined, this will throw a TypeError. The same pattern appears inhandleOnSwitchChange.♻️ Suggested defensive guard
if (id.startsWith("cfp_")) { + if (!newEntity.marketing_settings) { + newEntity.marketing_settings = {}; + } if ( !Object.prototype.hasOwnProperty.call(newEntity.marketing_settings, id) ) {Apply similar guard in
handleOnSwitchChangearound line 247.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/selection-plan-form.js` around lines 145 - 151, The code assumes newEntity.marketing_settings exists before calling Object.prototype.hasOwnProperty.call and before assigning keys; add a defensive guard to initialize marketing_settings when missing (e.g., if (!newEntity.marketing_settings) newEntity.marketing_settings = {}) before the block that checks id.startsWith("cfp_") and likewise in handleOnSwitchChange so both the hasOwnProperty call and subsequent assignment to newEntity.marketing_settings[id].value are safe when propsEntity.marketing_settings is undefined; update references to newEntity.marketing_settings in those functions (the id.startsWith("cfp_") branch and the handleOnSwitchChange handler) to rely on the initialized object.src/components/forms/__tests__/selection-plan-form.test.js (1)
150-172: ⚡ Quick winConsider asserting cleanup after successful save.
Test 3 verifies that
onSavingChange(false)is called after rejection, but this test does not verify the same cleanup happens after success. According to the implementation in context snippet 1,.finally()always callsonSavingChange(false), so asserting that behavior here would make the test more complete.✨ Suggested enhancement
await act(async () => { resolveSave({ id: 1 }); await Promise.resolve(); }); + + await waitFor(() => { + expect(onSavingChange).toHaveBeenCalledWith(false); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 150 - 172, Add an assertion that the component runs its cleanup after a successful save: after resolving the mocked onSubmit promise (resolveSave) in the test "disables submit and calls onSavingChange(true) while save is pending", assert that onSavingChange was later called with false and that the submit button is enabled again (verifying the .finally() cleanup path implemented around onSubmit/onSavingChange). Reference the mocked onSubmit, the onSavingChange spy, and resolveSave when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/forms/__tests__/selection-plan-form.test.js`:
- Around line 150-172: Add an assertion that the component runs its cleanup
after a successful save: after resolving the mocked onSubmit promise
(resolveSave) in the test "disables submit and calls onSavingChange(true) while
save is pending", assert that onSavingChange was later called with false and
that the submit button is enabled again (verifying the .finally() cleanup path
implemented around onSubmit/onSavingChange). Reference the mocked onSubmit, the
onSavingChange spy, and resolveSave when adding these assertions.
In `@src/components/forms/selection-plan-form.js`:
- Around line 145-151: The code assumes newEntity.marketing_settings exists
before calling Object.prototype.hasOwnProperty.call and before assigning keys;
add a defensive guard to initialize marketing_settings when missing (e.g., if
(!newEntity.marketing_settings) newEntity.marketing_settings = {}) before the
block that checks id.startsWith("cfp_") and likewise in handleOnSwitchChange so
both the hasOwnProperty call and subsequent assignment to
newEntity.marketing_settings[id].value are safe when
propsEntity.marketing_settings is undefined; update references to
newEntity.marketing_settings in those functions (the id.startsWith("cfp_")
branch and the handleOnSwitchChange handler) to rely on the initialized object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5256af02-8c39-4b53-b37b-4d45ea80ad1e
📒 Files selected for processing (7)
src/actions/__tests__/selection-plan-actions.test.jssrc/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/pages/selection-plans/__tests__/selection-plan-list-page.test.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.js
✅ Files skipped from review due to trivial changes (1)
- src/actions/tests/selection-plan-actions.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/selection-plans/edit-selection-plan-page.js
- src/pages/selection-plans/selection-plan-list-page.js
| }); | ||
| expect(onSavingChange).toHaveBeenCalledWith(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The three tests here only cover the save-guard (button disabled / double-submit prevention / re-enable on reject). The most critical new behaviour in this PR — the two-step save chain — has no coverage.
Missing: verify that after a successful onSubmit, saveSelectionPlanSettings is called with the correct args and onSaved is called with the saved entity.
it('calls saveSelectionPlanSettings then onSaved on successful submit', async () => {
const savedEntity = { id: 42 };
const onSubmitMock = jest.fn().mockResolvedValue(savedEntity);
const onSavedMock = jest.fn();
const saveSettingsMock = jest.fn().mockResolvedValue();
render(
<SelectionPlanForm
{...defaultProps}
onSubmit={onSubmitMock}
onSaved={onSavedMock}
saveSelectionPlanSettings={saveSettingsMock}
/>
);
await userEvent.click(screen.getByRole('button', { name: 'general.save' }));
await waitFor(() => {
expect(saveSettingsMock).toHaveBeenCalledWith(
defaultEntity.marketing_settings,
savedEntity.id
);
expect(onSavedMock).toHaveBeenCalledWith(savedEntity);
});
});
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
13befb9 to
14973bc
Compare
| const handleSort = (index, key, dir) => { | ||
| getSelectionPlans(term, currentPage, key, dir); | ||
| const handleSavingChange = (saving) => { | ||
| isSavingRef.current = saving; |
There was a problem hiding this comment.
Popup pattern deviation: duplicated isSaving + onSavingChange callback
The dominant pattern across this codebase (media-file-types, form-templates, tags, etc.) is:
isSavinglives exclusively in the popup/form component- the parent passes a single
onSaveprop that returns a Promise - the popup drives its own close via
.then(() => onClose())
Here the form already has its own isSaving (correct), but it also calls onSavingChange(true/false) to bubble the state up to the list page, which then maintains a parallel isSaving + isSavingRef to control the Dialog chrome (escape key, close icon). The isSavingRef workaround exists precisely because of this cross-layer sync.
The cleaner approach: extract the Dialog + DialogTitle into a dedicated SelectionPlanPopup component that receives isSaving as a prop from the form directly, or passes onSavingChange only down to that wrapper — eliminating the ref and the duplicated state in the list page.
Note: the complexity of selection plan (embedded full-page form, multiple sub-resources) makes this a harder refactor than the simpler popups. The deviation is understandable given the constraints, but worth tracking.
| strict | ||
| exact | ||
| path={`${match.url}/:selection_plan_id(\\d+)`} | ||
| component={SelectionPlanListPage} |
There was a problem hiding this comment.
This exact /:selection_plan_id route rendering SelectionPlanListPage introduces route-driven popup opening — navigating to /selection-plans/42 loads the list page and auto-opens the dialog for that plan via match.params.selection_plan_id.
This was not part of the requested migration. PR #897 (media-file-types), the reference for this migration wave, took the opposite approach: removed all /new and /:id routes entirely, leaving only list + catch-all redirect. No URL-based popup opening was added there.
The feature adds meaningful complexity: it requires a non-exact duplicate /:id → SelectionPlanIdLayout route to coexist in the same Switch (for sub-routes), plus the useEffect + openEditModal path in the list page, plus isSavingRef as a workaround for the cross-layer state sync this routing requires. None of that would be needed if the popup were opened only by UI interaction (clicking a row), consistent with the rest of the migration.
Remove this route and the corresponding useEffect([openEditModal, routeSelectionPlanId]) in selection-plan-list-page.js. Row edit should open the popup directly, same as media-file-types.
| Dropdown | ||
| } from "openstack-uicore-foundation/lib/components"; | ||
| import Input from "openstack-uicore-foundation/lib/components/inputs/text-input"; | ||
| import DateTimePicker from "openstack-uicore-foundation/lib/components/inputs/datetimepicker"; |
There was a problem hiding this comment.
The form still uses the legacy DateTimePicker from openstack-uicore-foundation/lib/components/inputs/datetimepicker (bootstrap-based, handled via ev.target.type === 'datetime' in handleChange).
Other recently migrated forms in this codebase (sponsor forms, page-template modules) use MuiFormikDatepicker from openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker, which renders a MUI DatePicker and is consistent with the rest of the MUI migration.
The blocker is that MuiFormikDatepicker is Formik-aware (useField internally), so it cannot be dropped in as a direct replacement without migrating the form to Formik. This form still uses its own useState + handleChange state management.
The selection plan form should be migrated to Formik + MuiFormikDatepicker for the date fields so the date UI is consistent with the rest of the forms shown in the same admin UI.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
@priscila-moneo please review comments
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-selection-plans-grid-mui
branch
from
14973bc to
b93c014
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/forms/__tests__/selection-plan-form.test.js (2)
94-98: 💤 Low valueAdd
time_zone_idtocurrentSummitfor test robustness.The form uses
currentSummit.time_zone_idfor date normalization inbuildInitialValuesandhandleFormikSubmit. Tests pass because all date fields arenull, but if any test later sets date values,moment.tz(value, undefined)will behave unexpectedly.const defaultProps = { entity: defaultEntity, errors: {}, - currentSummit: { id: 1 }, + currentSummit: { id: 1, time_zone_id: "UTC" },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 94 - 98, In the defaultProps object, add the missing time_zone_id property to the currentSummit object. Set it to a valid timezone string value (such as "UTC" or another appropriate timezone identifier) so that when the form's buildInitialValues and handleFormikSubmit methods use currentSummit.time_zone_id for date normalization with moment.tz(), the timezone will be properly defined rather than undefined.
18-68: 💤 Low valueIncomplete mocks for form's direct imports.
The form imports components from specific subpaths (e.g.,
openstack-uicore-foundation/lib/components/inputs/text-input,openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker) but the mock on line 18 targets the parent/lib/componentspath. Jest doesn't automatically mock subpaths.Missing mocks for:
MuiFormikDatepicker,Input(text-input),Panel,Dropdown,TextEditorV3,SortableTable(mui),SimpleLinkList.Tests may pass now because these components render without throwing, but this creates fragility—if any component adds required context or validation, tests will fail unexpectedly.
🧪 Add targeted mocks for imported components
+jest.mock( + "openstack-uicore-foundation/lib/components/inputs/text-input", + () => ({ + __esModule: true, + default: ({ id, value, onChange }) => ( + <input id={id} value={value ?? ""} onChange={onChange} /> + ) + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/sections/panel", + () => ({ + __esModule: true, + default: ({ children }) => <div>{children}</div> + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/dropdown", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/inputs/editor-input-v3", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/mui/sortable-table", + () => ({ + __esModule: true, + default: () => null + }) +); + +jest.mock( + "openstack-uicore-foundation/lib/components/simple-link-list", + () => ({ + __esModule: true, + default: () => null + }) +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/__tests__/selection-plan-form.test.js` around lines 18 - 68, The current mocks only target the parent path `openstack-uicore-foundation/lib/components` but the form imports components from specific subpaths like `openstack-uicore-foundation/lib/components/inputs/text-input` and `openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker`. Add individual jest.mock() calls for each specific subpath that the selection-plan-form component imports (including MuiFormikDatepicker, Input from text-input, Panel, Dropdown, TextEditorV3, SortableTable from mui, and SimpleLinkList), ensuring each mock properly exports the component as either a default export or named export matching what the component expects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/selection-plans/selection-plan-list-page.js`:
- Around line 83-89: The handleDelete function currently expects selectionPlan
to be an object and tries to access selectionPlan?.id, but MuiTable's onDelete
callback passes only the primitive ID value directly (not the full row object).
Change the parameter name from selectionPlan to id, update the null check from
if (!selectionPlan?.id) to if (!id), and pass id directly to deleteSelectionPlan
instead of selectionPlan.id.
---
Nitpick comments:
In `@src/components/forms/__tests__/selection-plan-form.test.js`:
- Around line 94-98: In the defaultProps object, add the missing time_zone_id
property to the currentSummit object. Set it to a valid timezone string value
(such as "UTC" or another appropriate timezone identifier) so that when the
form's buildInitialValues and handleFormikSubmit methods use
currentSummit.time_zone_id for date normalization with moment.tz(), the timezone
will be properly defined rather than undefined.
- Around line 18-68: The current mocks only target the parent path
`openstack-uicore-foundation/lib/components` but the form imports components
from specific subpaths like
`openstack-uicore-foundation/lib/components/inputs/text-input` and
`openstack-uicore-foundation/lib/components/mui/formik-inputs/datepicker`. Add
individual jest.mock() calls for each specific subpath that the
selection-plan-form component imports (including MuiFormikDatepicker, Input from
text-input, Panel, Dropdown, TextEditorV3, SortableTable from mui, and
SimpleLinkList), ensuring each mock properly exports the component as either a
default export or named export matching what the component expects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10a5bc10-1a24-4623-b74c-1416870b6991
📒 Files selected for processing (13)
src/actions/__tests__/selection-plan-actions.test.jssrc/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/components/inputs/import-modal/index.jsxsrc/layouts/selection-plan-id-layout.jssrc/layouts/selection-plan-layout.jssrc/pages/selection-plans/__tests__/selection-plan-list-page.test.jssrc/pages/selection-plans/edit-selection-plan-page.jssrc/pages/selection-plans/selection-plan-list-page.jssrc/pages/selection-plans/selection-plan-popup.jssrc/pages/sponsors/__tests__/edit-sponsor-page.test.jssrc/reducers/selection_plans/selection-plan-list-reducer.js
✅ Files skipped from review due to trivial changes (1)
- src/components/inputs/import-modal/index.jsx
🚧 Files skipped from review as they are similar to previous changes (7)
- src/layouts/selection-plan-id-layout.js
- src/pages/selection-plans/tests/selection-plan-list-page.test.js
- src/pages/sponsors/tests/edit-sponsor-page.test.js
- src/actions/tests/selection-plan-actions.test.js
- src/reducers/selection_plans/selection-plan-list-reducer.js
- src/actions/selection-plan-actions.js
- src/pages/selection-plans/edit-selection-plan-page.js
| const handleDelete = (selectionPlan) => { | ||
| if (!selectionPlan?.id) return; | ||
|
|
||
| deleteSelectionPlan(selectionPlan.id) | ||
| .finally(() => refreshSelectionPlans()) | ||
| .catch(() => {}); | ||
| }; |
There was a problem hiding this comment.
handleDelete receives a primitive ID, not an object — deletes will silently fail.
Based on learnings, MuiTable's onDelete callback receives the primitive row identifier (the id value), not the full row object. Since selectionPlan will be a number like 42, the expression selectionPlan?.id evaluates to undefined, causing the early return on Line 84 to trigger and skip the delete.
Proposed fix
- const handleDelete = (selectionPlan) => {
- if (!selectionPlan?.id) return;
-
- deleteSelectionPlan(selectionPlan.id)
+ const handleDelete = (selectionPlanId) => {
+ if (!selectionPlanId) return;
+
+ deleteSelectionPlan(selectionPlanId)
.finally(() => refreshSelectionPlans())
.catch(() => {});
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/selection-plans/selection-plan-list-page.js` around lines 83 - 89,
The handleDelete function currently expects selectionPlan to be an object and
tries to access selectionPlan?.id, but MuiTable's onDelete callback passes only
the primitive ID value directly (not the full row object). Change the parameter
name from selectionPlan to id, update the null check from if
(!selectionPlan?.id) to if (!id), and pass id directly to deleteSelectionPlan
instead of selectionPlan.id.
Source: Learnings
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/i18n/en.json`:
- Around line 571-572: The placeholder text for the link_question and
link_presentation_action_type keys in the English localization file contains the
misspelled word "Existent" which should be "Existing" for correct grammar and
user-facing copy. Update both occurrences by replacing "Existent" with
"Existing" in the respective message strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 063b12fd-914d-43d5-b0bb-eadbec7d7a1b
📒 Files selected for processing (5)
src/actions/selection-plan-actions.jssrc/components/forms/__tests__/selection-plan-form.test.jssrc/components/forms/selection-plan-form.jssrc/i18n/en.jsonsrc/pages/selection-plans/selection-plan-popup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/selection-plans/selection-plan-popup.js
| "link_question": "Link an Existent Question...", | ||
| "link_presentation_action_type": "Link an Existent Action Type...", |
There was a problem hiding this comment.
Fix placeholder wording typo in link actions.
Line 571 and Line 572 use “Existent,” which should be “Existing” for correct user-facing copy.
Suggested diff
- "link_question": "Link an Existent Question...",
- "link_presentation_action_type": "Link an Existent Action Type...",
+ "link_question": "Link an Existing Question...",
+ "link_presentation_action_type": "Link an Existing Action Type...",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/en.json` around lines 571 - 572, The placeholder text for the
link_question and link_presentation_action_type keys in the English localization
file contains the misspelled word "Existent" which should be "Existing" for
correct grammar and user-facing copy. Update both occurrences by replacing
"Existent" with "Existing" in the respective message strings.
3 participants
ref: https://app.clickup.com/t/86b9pbavj
Summary by CodeRabbit
onSaved).