-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add new page and route for sponsor customized form managed items #698
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
2105784 to
fc25ec6
Compare
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new page and routing system for managing sponsor customized form items. It introduces functionality to view, add, edit, archive, and delete items associated with sponsor forms, including the ability to add items from an existing inventory.
- Added new route
/app/summits/:summit_id/sponsors/:sponsor_id/sponsor-forms/:form_id/itemsfor managing form items - Implemented editable table with inline validation for price fields
- Added ability to clone items from inventory into sponsor forms
- Enhanced UI with Material-UI components for better user experience
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/constants.js | Added FOUR constant for tab index reference |
| src/store.js | Registered new reducer for sponsor customized form items list state |
| src/reducers/sponsors/sponsor-customized-form-items-list-reducer.js | New reducer to manage form items list state, pagination, and CRUD operations |
| src/pages/sponsors_inventory/popup/sponsor-inventory-popup.js | Updated popup to use price field component, fixed quantity field defaults, added dialog title translation |
| src/pages/sponsors/sponsor-forms-tab/index.js | Added navigation handler to route to manage items page |
| src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js | New component implementing items management with search, pagination, inline editing, and archiving |
| src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.js | New popup component for selecting and adding items from inventory |
| src/pages/sponsors/edit-sponsor-page.js | Added conditional rendering for items route and tab navigation handling |
| src/layouts/sponsor-id-layout.js | Added new route path for form items management, improved code style |
| src/i18n/en.json | Added translations for form items management UI and fixed typo in "Standard rate" |
| src/components/mui/editable-table/mui-table-editable.js | Enhanced table with validation support for editable cells |
| src/actions/sponsor-forms-actions.js | Added actions for fetching, saving, deleting, archiving form items and cloning from inventory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| if (fieldType.id) { | ||
| if (fieldType.id && onMetaFieldTypeDeleted) { |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition checking for onMetaFieldTypeDeleted should be more specific. Adding a check for onMetaFieldTypeDeleted prevents calling an undefined function, but there's no feedback to the user or developer when this callback is missing. Consider logging a warning or ensuring this prop is always provided if fieldType.id exists.
| <FormControlLabel | ||
| control={ | ||
| <Checkbox | ||
| value={hideArchived} |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name 'hideArchived' is misleading in this context. The checkbox's 'value' prop should be 'checked' instead. When using controlled checkboxes with FormControlLabel in Material-UI, the Checkbox component should use 'checked' prop, not 'value'.
| value={hideArchived} | |
| checked={hideArchived} |
|
|
||
| export const TWO = 2; | ||
|
|
||
| export const FOUR = 4; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FOUR constant is exported but its purpose is unclear from its name alone. Consider using a more descriptive name like TAB_INDEX_FORMS or FORMS_TAB_INDEX to make the code more self-documenting.
| export const FOUR = 4; | |
| export const TAB_INDEX_FORMS = 4; |
| onClose: PropTypes.func.isRequired, | ||
| onSave: PropTypes.func.isRequired |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PropTypes definition is incomplete. It only validates onClose and onSave, but the component receives many more props from Redux (open, inventoryItems, term, order, perPage, orderDir, currentPage, totalInventoryItems, getInventoryItems) that should also have PropTypes defined for better type safety and documentation.
| onClose: PropTypes.func.isRequired, | |
| onSave: PropTypes.func.isRequired | |
| open: PropTypes.bool.isRequired, | |
| inventoryItems: PropTypes.array.isRequired, | |
| term: PropTypes.string, | |
| order: PropTypes.string, | |
| perPage: PropTypes.number, | |
| orderDir: PropTypes.string, | |
| currentPage: PropTypes.number, | |
| totalInventoryItems: PropTypes.number, | |
| onClose: PropTypes.func.isRequired, | |
| onSave: PropTypes.func.isRequired, | |
| getInventoryItems: PropTypes.func.isRequired |
src/i18n/en.json
Outdated
| "default_quantity": "Default Quantity", | ||
| "add_selected": "Add Selected Items", | ||
| "item_updated": "Form item updated successfully", | ||
| "item_created": "Form item {item} updated successfully", |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translation key has a mismatch. The message says "updated successfully" but the key is "item_created". This should either be "created" instead of "updated" or the key should be changed to reflect that it's about updating.
| "item_created": "Form item {item} updated successfully", | |
| "item_created": "Form item {item} created successfully", |
| ); | ||
| return { ...state, items }; | ||
| } | ||
| case SPONSOR_CUSTOMIZED_FORM_ITEMS_ADDED: { |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SPONSOR_CUSTOMIZED_FORM_ITEMS_ADDED case handler returns state unchanged without any explanation. If this is intentional (perhaps because the items are fetched separately), a comment explaining this would improve code clarity.
| case SPONSOR_CUSTOMIZED_FORM_ITEMS_ADDED: { | |
| case SPONSOR_CUSTOMIZED_FORM_ITEMS_ADDED: { | |
| // No state update is needed here; items are fetched separately after this action. |
| .test("max-decimals", T.translate("validation.two_decimals"), (value) => { | ||
| if (value === undefined || value === null) return true; | ||
| return /^\d+(\.\d{1,2})?$/.test(value.toString()); |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation schema test uses a regex that doesn't properly validate negative numbers with decimals. The pattern /^\d+(.\d{1,2})?$/ only matches positive numbers, but the previous validation allows negative numbers with the '-?' in the pattern. This inconsistency could cause valid negative decimal numbers to fail validation.
| import SponsorFormItemFromInventoryPopup from "./sponsor-form-item-from-inventory"; | ||
| import { parsePrice } from "../../../../../utils/currency"; | ||
| import { DEFAULT_CURRENT_PAGE } from "../../../../../utils/constants"; | ||
| // import FormTemplateDialog from "../../../../sponsors_inventory/popup/form-template-popup"; |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out line suggests there's a placeholder or incomplete template feature. Either this should be implemented and uncommented, or the comment should be removed if it's not part of the current feature scope.
| // import FormTemplateDialog from "../../../../sponsors_inventory/popup/form-template-popup"; |
| const hasQuantities = | ||
| type === "Quantity" && (minQty != null || maxQty != null); |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation condition has a logic issue. The expression uses != instead of !== for null checks, which is inconsistent with JavaScript best practices and could lead to unexpected behavior since != performs type coercion. For example, 0 != null is false, which might not be the intended behavior when checking if minQty is set to 0.
src/actions/sponsor-forms-actions.js
Outdated
| }) | ||
| ); | ||
| }) | ||
| .catch(console.log) // need to catch promise reject |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handler is inconsistent. Line 1484 uses console.log to catch promise rejection, which silently swallows errors and makes debugging difficult. This should use a proper error handler like the snackbarErrorHandler used in other parts of the codebase.
| .catch(console.log) // need to catch promise reject | |
| .catch(snackbarErrorHandler) |
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tomrndom please review coments left on tickets
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
ref:
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com