feat: add add-on types pages, dialog, actions, reducers, tests#999
feat: add add-on types pages, dialog, actions, reducers, tests#999tomrndom wants to merge 2 commits into
Conversation
…yout Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an Add-On Types CRUD feature to the sponsors inventory section: Redux actions and reducers, a dialog and list page, app routing and menu wiring, translation strings, and Jest coverage for the new flow. ChangesAdd-On Types CRUD Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 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/actions/add-on-types-actions.js`:
- Around line 176-182: The create payload normalization is leaving the
client-side id in place, so new records can be sent with an id of 0. Update
normalizeEntity to also strip id alongside created and last_edited, and ensure
the POST flow that uses normalizeEntity in add-on-types-actions builds create
payloads without any client-generated identifier.
In `@src/components/menu/menu-definition.js`:
- Around line 56-58: The Add-on Types submenu item is missing the accessRoute
restriction, so it can appear for users who only have access to
sponsors_inventory. Update the menuDefinition entry in the add-on types submenu
to include accessRoute set to add-on-types, matching the page’s permission and
the SubMenuItem filtering behavior.
In `@src/pages/sponsors-global/add-on-types/add-on-types-dialog.js`:
- Around line 35-54: AddOnTypesDialog is receiving reducer-backed save errors
from AddOnTypesListPage but never declares or uses an errors prop, so failed
saves are not surfaced in Formik. Update AddOnTypesDialog to accept the errors
prop alongside entity/onSave/onClose, and wire those server-side field errors
into the form state in the formik setup so they display on the matching fields
after a rejected save. Make sure the handling is applied in the AddOnTypesDialog
component and any related submit/error flow that currently swallows onSave
rejections.
In `@src/pages/sponsors-global/add-on-types/add-on-types-list-page.js`:
- Around line 91-94: The add-on type save flow currently waits for the follow-up
reload before resolving, which can keep the dialog open even after a successful
save. Update handleAddOnTypeSave in addOnTypesListPage so it resolves
immediately after saveAddOnType succeeds, and trigger getAddOnTypes(term,
DEFAULT_CURRENT_PAGE, perPage, order, orderDir) separately without blocking the
dialog close; keep the refresh logic associated with the existing save flow so
the list still updates, but do not chain it into the save promise.
- Around line 170-186: The delete confirmation text in MuiTable is missing the
add-on type name because deleteDialogBody receives the resolved display name
from getName, not from the onDelete payload. Update the MuiTable usage in
add-on-types-list-page by passing the appropriate getName prop alongside
deleteDialogBody so the table can resolve the item name before showing the
delete warning.
In `@src/reducers/sponsors_inventory/add-on-types-list-reducer.js`:
- Around line 41-65: The reducer in add-on-types-list-reducer.js is keeping
addOnTypes when currentPage changes, which lets stale rows remain visible after
handleSearch resets the page back to 1. Update the branch in the addOnTypes list
reducer so it also clears addOnTypes when the action is driven by a search term
change, using the existing state update logic around currentPage, order, and
orderDir to distinguish page-only navigation from search/filter resets.
🪄 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: 8ea29459-b89b-4c12-86c8-1f346bbf4ded
📒 Files selected for processing (14)
src/actions/__tests__/add-on-types-actions.test.jssrc/actions/add-on-types-actions.jssrc/components/menu/menu-definition.jssrc/i18n/en.jsonsrc/layouts/add-on-types-layout.jssrc/layouts/primary-layout.jssrc/pages/sponsors-global/add-on-types/__tests__/add-on-types-dialog.test.jssrc/pages/sponsors-global/add-on-types/__tests__/add-on-types-list-page.test.jssrc/pages/sponsors-global/add-on-types/add-on-types-dialog.jssrc/pages/sponsors-global/add-on-types/add-on-types-list-page.jssrc/reducers/__tests__/add-on-types-reducers.test.jssrc/reducers/sponsors_inventory/add-on-type-reducer.jssrc/reducers/sponsors_inventory/add-on-types-list-reducer.jssrc/store.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new “Add-On Types” feature to the sponsors inventory area, including Redux state management, routing/navigation, a list page with CRUD dialog, and accompanying Jest tests.
Changes:
- Added Add-On Types Redux actions + reducers and registered them in the global store.
- Introduced Add-On Types list page + modal dialog, and wired routing/menu navigation with permission restriction.
- Added Jest tests covering actions, reducers, and the new page/dialog behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/store.js | Registers the new Add-On Types reducers in the persisted Redux store. |
| src/reducers/sponsors_inventory/add-on-types-list-reducer.js | Adds list reducer for pagination/sorting/search + delete behavior. |
| src/reducers/sponsors_inventory/add-on-type-reducer.js | Adds single-entity reducer for the add/edit dialog form state. |
| src/reducers/tests/add-on-types-reducers.test.js | Unit tests for the two new reducers. |
| src/actions/add-on-types-actions.js | Thunk actions for fetching/listing/saving/deleting Add-On Types with snackbar feedback. |
| src/actions/tests/add-on-types-actions.test.js | Unit tests for the new actions and loading lifecycle. |
| src/pages/sponsors-global/add-on-types/add-on-types-list-page.js | New list page wiring table actions to Redux thunks and opening the dialog. |
| src/pages/sponsors-global/add-on-types/add-on-types-dialog.js | New validated Formik/MUI modal dialog for add/edit. |
| src/pages/sponsors-global/add-on-types/tests/add-on-types-list-page.test.js | Component tests for list page behavior (mount fetch, open/edit/delete/save flows). |
| src/pages/sponsors-global/add-on-types/tests/add-on-types-dialog.test.js | Component tests for dialog title, submit/disable behavior, and close behavior. |
| src/layouts/primary-layout.js | Adds a route entry for /app/add-on-types. |
| src/layouts/add-on-types-layout.js | New layout with breadcrumb + Restrict wrapper for the feature route. |
| src/components/menu/menu-definition.js | Adds the new navigation item under the global sponsors inventory menu. |
| src/i18n/en.json | Adds menu + feature-specific i18n strings for list/dialog labels and messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| term = null, | ||
| page = 1, | ||
| perPage = DEFAULT_PER_PAGE, | ||
| order = "order", | ||
| orderDir = 1 |
There was a problem hiding this comment.
Confirmed - checked every real call site of getAddOnTypes( in the app (grep across src, excluding tests): all 7 pass order explicitly, so this default is currently unreachable in production code (only exercised by an isolated action-level unit test calling getAddOnTypes() with no args). Not an active bug today, but worth fixing the default to "name" to match the reducer's DEFAULT_STATE.order and avoid a footgun for future callers.
| currentAddonTypesListState: addOnTypesListReducer, | ||
| currentAddOnTypeState: addOnTypeReducer, |
There was a problem hiding this comment.
Confirmed, and purely cosmetic - verified it's self-consistent within this PR (mapStateToProps in the list page destructures the matching currentAddonTypesListState key), so no functional bug today. Still worth the rename for consistency with currentAddOnTypeState and the rest of the codebase's AddOn casing.
| const handleAddOnTypeSave = (item) => | ||
| saveAddOnType(item).then(() => | ||
| getAddOnTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); |
There was a problem hiding this comment.
@tomrndom
Confirmed - and this is the same root cause flagged independently in two other places:
- CodeRabbit already raised this exact issue on this same line back on 2026-06-30 (feat: add add-on types pages, dialog, actions, reducers, tests #999 (comment)). That thread is marked resolved, but I checked the current code and it's unchanged - looks like it was dismissed rather than actually fixed.
- I flagged the same pattern independently at line 90 (
handleAddOnTypeSave) in this same file, plus a related issue there (page reset on edit).
Three independent reviews converging on the same bug (and one prior instance of it being waved off without a fix) is a strong signal this should get fixed before merge.
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Deep review findings — see inline comments.
| } | ||
| }; | ||
|
|
||
| const handleDelete = (addOnTypeId) => |
There was a problem hiding this comment.
@tomrndom handleDelete returns the chained promise from getAddOnTypes(...) without a .catch(). If the delete succeeds but the follow-up list refresh fails (transient network/server error), the rejection propagates back through this promise, and the failure looks identical to the delete itself failing - even though the delete already went through.
Suggested fix - don't return the refresh call, and let it fail independently:
const handleDelete = (addOnTypeId) =>
deleteAddOnType(addOnTypeId).then(() => {
getAddOnTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir).catch(() => {});
});This matches the fire-and-forget refresh convention used elsewhere in the codebase (e.g. page-template-list-page.js's handleDelete).
| getAddOnTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir) | ||
| ); | ||
|
|
||
| const handleAddOnTypeSave = (item) => |
There was a problem hiding this comment.
@tomrndom Two things here:
- Same issue as
handleDeleteabove - this returns thegetAddOnTypes(...)promise without a.catch(). If the save succeeds but the refresh fails, the rejection reachesAddOnTypesDialog's.catch(() => {}), the dialog stays open, and the user sees a generic error even though the save already succeeded - likely prompting a retry that creates a duplicate record (the dialog still holds the unsaved values, and for a new entityidis still0). - This handler is shared by create and edit and always reloads at
DEFAULT_CURRENT_PAGE. For an edit, that bounces the admin back to page 1 away from the row they were just editing on a later page.
Suggested fix:
const handleAddOnTypeSave = (item) =>
saveAddOnType(item).then(() => {
const page = item.id ? currentPage : DEFAULT_CURRENT_PAGE;
getAddOnTypes(term, page, perPage, order, orderDir).catch(() => {});
});| </div> | ||
| ); | ||
|
|
||
| export default Restrict(withRouter(AddOnTypesLayout), "add-on-types"); |
There was a problem hiding this comment.
@tomrndom Restrict(withRouter(AddOnTypesLayout), "add-on-types") guards this route with the "add-on-types" key, but that key doesn't exist in src/access-routes.yml. Member.hasAccess() returns true immediately when the key is missing - before it even checks the member - so any authenticated user can reach this page and create/edit/delete Add-On Types regardless of group.
(page-template-layout.js, form-template-layout.js, and inventory-item-layout.js have the same gap already, so this isn't new to this PR — but worth closing here too, e.g. by adding an add-on-types entry to access-routes.yml or reusing the existing inventory key.)
| @@ -0,0 +1,170 @@ | |||
| /** | |||
There was a problem hiding this comment.
@tomrndom this file is just testing standard redux procedures, what is the point of this ? Please remove this file entirely and next time ask AI for specific user cases that you want to test against, not just "make tests" because it will test everything, even nonsense
| strict | ||
| exact | ||
| path={`${match.url}`} | ||
| component={AddOnTypesListPage} |
There was a problem hiding this comment.
why do we need a layout file here if there is only 1 view ?
| <Route path="/app/inventory" component={InventoryItemLayout} /> | ||
| <Route path="/app/form-templates" component={FormTemplateLayout} /> | ||
| <Route path="/app/page-templates" component={PageTemplateLayout} /> | ||
| <Route path="/app/add-on-types" component={AddOnTypesLayout} /> |
There was a problem hiding this comment.
looks like this should be strict and just go to AddOnTypesListPage
ref: https://app.clickup.com/t/9014802374/86bamaxc8
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit