Conversation
… utils, validations and tests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughThis PR adds rate-specific utilities and tests, introduces an ItemPriceTiers Formik UI component, replaces prior money helpers with rate helpers across actions/reducers/pages, updates validations to support nullable decimals and a renamed i18n key, and makes rate column editability conditional. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 10
🧹 Nitpick comments (4)
src/utils/__tests__/rate-helpers.test.js (1)
12-66: Add edge-case assertions forundefinedand empty string.Given helper behavior, add explicit tests for
isRateEnabled(undefined)andisRateEnabled("")(and optionallyrateToCents(undefined)) to prevent regressions on nullable inputs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/rate-helpers.test.js` around lines 12 - 66, Add edge-case unit tests for undefined and empty-string inputs: call isRateEnabled(undefined) and isRateEnabled("") and assert they return false (matching existing null behavior), and add an optional test calling rateToCents(undefined) asserting it returns null (matching rateToCents(null)). Place these new assertions alongside the existing describe blocks for isRateEnabled and rateToCents so they run with the current test suite.src/reducers/sponsors_inventory/inventory-item-reducer.js (1)
64-75: Extract duplicated entity normalization/conversion logic.Line 64–75 and Line 87–98 repeat the same transformation. A shared helper will reduce drift risk between receive/add/update flows.
♻️ Suggested refactor
+const normalizeInventoryItemEntity = (source) => { + const entity = { ...source }; + const rateFieldValues = Object.values(RATE_FIELDS); + + for (const key in entity) { + if ( + Object.prototype.hasOwnProperty.call(entity, key) && + !rateFieldValues.includes(key) + ) { + entity[key] = entity[key] == null ? "" : entity[key]; + } + } + + entity.early_bird_rate = rateFromCents(entity.early_bird_rate); + entity.standard_rate = rateFromCents(entity.standard_rate); + entity.onsite_rate = rateFromCents(entity.onsite_rate); + return entity; +}; + case RECEIVE_INVENTORY_ITEM: { - const entity = { ...payload.response }; - const rateFieldValues = Object.values(RATE_FIELDS); - ... - entity.early_bird_rate = rateFromCents(entity.early_bird_rate); - entity.standard_rate = rateFromCents(entity.standard_rate); - entity.onsite_rate = rateFromCents(entity.onsite_rate); + const entity = normalizeInventoryItemEntity(payload.response); return { ...state, entity: { ...DEFAULT_ENTITY, ...entity } }; } case INVENTORY_ITEM_ADDED: case INVENTORY_ITEM_UPDATED: { - const entity = { ...payload.response }; - const rateFieldValues = Object.values(RATE_FIELDS); - ... - entity.early_bird_rate = rateFromCents(entity.early_bird_rate); - entity.standard_rate = rateFromCents(entity.standard_rate); - entity.onsite_rate = rateFromCents(entity.onsite_rate); + const entity = normalizeInventoryItemEntity(payload.response);Also applies to: 87-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors_inventory/inventory-item-reducer.js` around lines 64 - 75, Extract the repeated normalization/conversion into a single helper function (e.g., normalizeInventoryEntity) that takes an entity, iterates over keys and replaces null/undefined with "" except keys in RATE_FIELDS, and then applies rateFromCents to early_bird_rate, standard_rate, and onsite_rate; replace the duplicated blocks in the reducer (both the receive/add/update flows that reference RATE_FIELDS and call rateFromCents) with a call to this new helper, and export/import it as needed so all reducer cases share the same implementation.src/reducers/sponsors_inventory/form-template-item-reducer.js (1)
65-75: Same duplicated normalization block—consider one helper here too.Line 65–75 and Line 88–98 duplicate the same logic. Centralizing this transformation would keep both action paths consistent long-term.
Also applies to: 88-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors_inventory/form-template-item-reducer.js` around lines 65 - 75, Extract the duplicated normalization logic into a single helper (e.g., normalizeFormTemplateItem(entity)) that: 1) iterates over entity keys and replaces null/undefined with "" for keys not in RATE_FIELDS, and 2) applies rateFromCents to entity.early_bird_rate, entity.standard_rate, and entity.onsite_rate; then replace the duplicated blocks in the reducer with a call to normalizeFormTemplateItem(entity) so both code paths use the same function (keeping references to RATE_FIELDS and rateFromCents unchanged).src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js (1)
95-97: Add null-rate coverage for the new nullable tier behavior.These updated fixtures validate numeric cents, but this suite still misses explicit assertions for
nulltier inputs in both list and current-item receive paths.Also applies to: 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js` around lines 95 - 97, Update the tests to cover nullable tier rates by adding fixtures where early_bird_rate, standard_rate, and onsite_rate are null and asserting the reducer handles them correctly: for the list receive path (the test in sponsor-form-items-list-reducer.test that feeds a list of items) assert the resulting state stores null for those rate fields, and for the current-item receive path (the test that populates the current item) assert the reducer sets the current item rates to null; locate the relevant tests in sponsor-form-items-list-reducer.test.js and modify the fixture objects and expectations around the existing numeric-rate cases to include and assert null-rate behavior for both list and current-item handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/__tests__/item-price-tiers.test.js`:
- Around line 8-11: The jest.mock path is wrong so the test doesn’t override the
real import; update the jest.mock call to mock "i18n-react" (not
"i18n-react/dist/i18n-react") and export __esModule: true with a default object
that provides translate: (key) => key (matching how the component uses the
imported module), i.e., modify the existing jest.mock invocation in the test to
target "i18n-react" and return the same default shape.
In `@src/components/mui/formik-inputs/item-price-tiers.js`:
- Around line 24-33: The local enabled state (enabled / setEnabled) in the
component is redundant and can desync from Formik after reinitialization; remove
the enabled useState and any references to setEnabled, derive the toggle state
directly from Formik values by calling isRateEnabled(values[RATE_FIELDS.*])
where needed, and simplify handleToggle to only call setFieldValue(field,
checked ? 0 : null); ensure any UI that read enabled now computes it from values
(or useMemo(values) for perf) and keep references to isRateEnabled, RATE_FIELDS,
handleToggle, setFieldValue, and values to locate the changes.
In `@src/i18n/en.json`:
- Line 92: Update the validation message for the key "validation.non_negative":
it currently reads "Must be a positive number." which is wrong for non-negative
checks; change the string to explicitly state non-negative/zero-allowed (for
example "Must be non-negative" or "Must be 0 or greater") so users understand
the allowed value range and the "validation.non_negative" localization key is
accurate.
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js`:
- Line 67: The validation call uses .min(BYTES_PER_MB,
T.translate("validation.non_negative")) which mislabels the constraint; update
the message passed to .min so it reflects the actual minimum (BYTES_PER_MB)
instead of "validation.non_negative" — either use the correct translation key
(e.g., "validation.min_bytes") or interpolate BYTES_PER_MB into the translated
string via T.translate, and keep this change where .min(BYTES_PER_MB, ...) is
used so users see a correct message for values like 0 or 500000.
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js`:
- Line 95: ItemPriceTiers' local enabled state is initialized only on mount and
becomes stale when Formik reinitializes; add a useEffect inside the
ItemPriceTiers component that subscribes to the Formik value(s) controlling tier
availability (via useFormikContext().values or the prop currently passed from
sponsor-form-item-form) and updates the local enabled state whenever those
Formik values change so the toggle reflects reinitialized form values; ensure
the effect's dependency array contains the exact Formik field(s) (e.g.,
values.priceTiersEnabled or the field name used in your form) to avoid
unnecessary updates.
In
`@src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js`:
- Line 215: The current editable callbacks (e.g., editable: (row) =>
row.early_bird_rate !== "N/A") rely on a hard-coded locale string; replace this
with a locale-independent check by adding a small helper (e.g.,
isRateEditable(value)) that returns false for null/undefined or when the value
cannot be parsed into a finite number (strip currency/spacing and use
Number.isFinite(parsedNumber)), and true otherwise; then update the editable
arrow functions for early_bird_rate and the other rate columns to call
isRateEditable(row.early_bird_rate) (and the analogous fields) instead of
comparing to "N/A".
In `@src/utils/__tests__/yup.test.js`:
- Around line 3-5: The jest mock currently targets "i18n-react/dist/i18n-react"
but the validator imports "i18n-react", so update the jest.mock call to mock
"i18n-react" instead (keeping the same translate: (key) => key export) so the
module name matches the actual import used by the yup validator and avoids
resolver-dependent behavior; locate the jest.mock invocation in the test and
change its module specifier to "i18n-react".
In `@src/utils/rate-helpers.js`:
- Around line 14-15: rateToCents currently passes empty strings into
amountToCents while isRateEnabled treats "" as disabled; modify rateToCents to
mirror isRateEnabled by returning undefined (or null, matching surrounding
conventions) when value is null, undefined, or an empty string (use the same
check: value !== null && value !== undefined && value !== "") so you never call
amountToCents with "" and invalid rates cannot be normalized/persisted; update
the function that calls amountToCents (rateToCents) to early-return when the
value fails that check.
In `@src/utils/yup.js`:
- Around line 163-171: The nullableDecimalValidation schema fails when
Formik/MUI inputs return an empty string because number().nullable() doesn't
accept ""—add a transform to nullableDecimalValidation that converts empty
string (and whitespace-only) original values to null before other validations
run; update the function nullableDecimalValidation to call .transform((value,
originalValue) => (typeof originalValue === "string" && originalValue.trim() ===
"" ? null : value)) so cleared fields validate as null and your existing
.nullable(), .typeError(), .min(), and .test("max-decimals", ...) continue to
work.
- Line 50: Replace the use of .positive(T.translate("validation.non_negative"))
with .min(0, T.translate("validation.non_negative")) in the validators inside
src/utils/yup.js so the rule allows zero (non-negative) instead of requiring >0;
update both occurrences (the one at the shown diff and the other around line 82)
so they match the existing nullableDecimalValidation behavior (which uses
.min(0)) and keep the same translation key.
---
Nitpick comments:
In `@src/reducers/sponsors_inventory/form-template-item-reducer.js`:
- Around line 65-75: Extract the duplicated normalization logic into a single
helper (e.g., normalizeFormTemplateItem(entity)) that: 1) iterates over entity
keys and replaces null/undefined with "" for keys not in RATE_FIELDS, and 2)
applies rateFromCents to entity.early_bird_rate, entity.standard_rate, and
entity.onsite_rate; then replace the duplicated blocks in the reducer with a
call to normalizeFormTemplateItem(entity) so both code paths use the same
function (keeping references to RATE_FIELDS and rateFromCents unchanged).
In `@src/reducers/sponsors_inventory/inventory-item-reducer.js`:
- Around line 64-75: Extract the repeated normalization/conversion into a single
helper function (e.g., normalizeInventoryEntity) that takes an entity, iterates
over keys and replaces null/undefined with "" except keys in RATE_FIELDS, and
then applies rateFromCents to early_bird_rate, standard_rate, and onsite_rate;
replace the duplicated blocks in the reducer (both the receive/add/update flows
that reference RATE_FIELDS and call rateFromCents) with a call to this new
helper, and export/import it as needed so all reducer cases share the same
implementation.
In `@src/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.js`:
- Around line 95-97: Update the tests to cover nullable tier rates by adding
fixtures where early_bird_rate, standard_rate, and onsite_rate are null and
asserting the reducer handles them correctly: for the list receive path (the
test in sponsor-form-items-list-reducer.test that feeds a list of items) assert
the resulting state stores null for those rate fields, and for the current-item
receive path (the test that populates the current item) assert the reducer sets
the current item rates to null; locate the relevant tests in
sponsor-form-items-list-reducer.test.js and modify the fixture objects and
expectations around the existing numeric-rate cases to include and assert
null-rate behavior for both list and current-item handlers.
In `@src/utils/__tests__/rate-helpers.test.js`:
- Around line 12-66: Add edge-case unit tests for undefined and empty-string
inputs: call isRateEnabled(undefined) and isRateEnabled("") and assert they
return false (matching existing null behavior), and add an optional test calling
rateToCents(undefined) asserting it returns null (matching rateToCents(null)).
Place these new assertions alongside the existing describe blocks for
isRateEnabled and rateToCents so they run with the current test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b233ec2-0b58-4f57-a414-5edc4f714930
📒 Files selected for processing (24)
src/actions/form-template-item-actions.jssrc/actions/inventory-item-actions.jssrc/actions/sponsor-forms-actions.jssrc/components/mui/__tests__/item-price-tiers.test.jssrc/components/mui/editable-table/mui-table-editable.jssrc/components/mui/formik-inputs/item-price-tiers.jssrc/i18n/en.jsonsrc/pages/sponsors-global/form-templates/add-form-template-item-popup.jssrc/pages/sponsors-global/form-templates/sponsor-inventory-popup.jssrc/pages/sponsors-global/page-templates/page-template-popup/index.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-add-item-from-inventory-popup.jssrc/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-form-item-from-inventory.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/reducers/sponsors/__tests__/sponsor-form-items-list-reducer.test.jssrc/reducers/sponsors/sponsor-customized-form-items-list-reducer.jssrc/reducers/sponsors/sponsor-form-items-list-reducer.jssrc/reducers/sponsors_inventory/form-template-item-reducer.jssrc/reducers/sponsors_inventory/inventory-item-reducer.jssrc/utils/__tests__/rate-helpers.test.jssrc/utils/__tests__/yup.test.jssrc/utils/rate-helpers.jssrc/utils/yup.js
| const [enabled, setEnabled] = useState({ | ||
| [RATE_FIELDS.EARLY_BIRD]: isRateEnabled(values[RATE_FIELDS.EARLY_BIRD]), | ||
| [RATE_FIELDS.STANDARD]: isRateEnabled(values[RATE_FIELDS.STANDARD]), | ||
| [RATE_FIELDS.ONSITE]: isRateEnabled(values[RATE_FIELDS.ONSITE]) | ||
| }); | ||
|
|
||
| const handleToggle = (field, checked) => { | ||
| setEnabled((prev) => ({ ...prev, [field]: checked })); | ||
| setFieldValue(field, checked ? 0 : null); | ||
| }; |
There was a problem hiding this comment.
enabled state can desync from Formik values after reinitialization.
The local state is only seeded once. If Formik values change later, toggle UI can be stale.
💡 Suggested fix (derive from Formik values, remove duplicate state)
-import React, { useState } from "react";
+import React from "react";
@@
- const [enabled, setEnabled] = useState({
+ const enabled = {
[RATE_FIELDS.EARLY_BIRD]: isRateEnabled(values[RATE_FIELDS.EARLY_BIRD]),
[RATE_FIELDS.STANDARD]: isRateEnabled(values[RATE_FIELDS.STANDARD]),
[RATE_FIELDS.ONSITE]: isRateEnabled(values[RATE_FIELDS.ONSITE])
- });
+ };
const handleToggle = (field, checked) => {
- setEnabled((prev) => ({ ...prev, [field]: checked }));
setFieldValue(field, checked ? 0 : null);
};📝 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 [enabled, setEnabled] = useState({ | |
| [RATE_FIELDS.EARLY_BIRD]: isRateEnabled(values[RATE_FIELDS.EARLY_BIRD]), | |
| [RATE_FIELDS.STANDARD]: isRateEnabled(values[RATE_FIELDS.STANDARD]), | |
| [RATE_FIELDS.ONSITE]: isRateEnabled(values[RATE_FIELDS.ONSITE]) | |
| }); | |
| const handleToggle = (field, checked) => { | |
| setEnabled((prev) => ({ ...prev, [field]: checked })); | |
| setFieldValue(field, checked ? 0 : null); | |
| }; | |
| const enabled = { | |
| [RATE_FIELDS.EARLY_BIRD]: isRateEnabled(values[RATE_FIELDS.EARLY_BIRD]), | |
| [RATE_FIELDS.STANDARD]: isRateEnabled(values[RATE_FIELDS.STANDARD]), | |
| [RATE_FIELDS.ONSITE]: isRateEnabled(values[RATE_FIELDS.ONSITE]) | |
| }; | |
| const handleToggle = (field, checked) => { | |
| setFieldValue(field, checked ? 0 : null); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/formik-inputs/item-price-tiers.js` around lines 24 - 33,
The local enabled state (enabled / setEnabled) in the component is redundant and
can desync from Formik after reinitialization; remove the enabled useState and
any references to setEnabled, derive the toggle state directly from Formik
values by calling isRateEnabled(values[RATE_FIELDS.*]) where needed, and
simplify handleToggle to only call setFieldValue(field, checked ? 0 : null);
ensure any UI that read enabled now computes it from values (or useMemo(values)
for perf) and keep references to isRateEnabled, RATE_FIELDS, handleToggle,
setFieldValue, and values to locate the changes.
| "string": "Must be a string.", | ||
| "number": "Must be a number.", | ||
| "number_positive": "Must be a positive number.", | ||
| "non_negative": "Must be a positive number.", |
There was a problem hiding this comment.
validation.non_negative message text is semantically incorrect
Line 92 currently says “Must be a positive number.” For non-negative validation (>= 0), this should mention non-negative (or at least “0 or greater”) to avoid misleading users.
💡 Proposed fix
- "non_negative": "Must be a positive number.",
+ "non_negative": "Must be a non-negative number.",📝 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.
| "non_negative": "Must be a positive number.", | |
| "non_negative": "Must be a non-negative number.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/i18n/en.json` at line 92, Update the validation message for the key
"validation.non_negative": it currently reads "Must be a positive number." which
is wrong for non-negative checks; change the string to explicitly state
non-negative/zero-allowed (for example "Must be non-negative" or "Must be 0 or
greater") so users understand the allowed value range and the
"validation.non_negative" localization key is accurate.
| then: (schema) => | ||
| schema | ||
| .min(BYTES_PER_MB, T.translate("validation.number_positive")) | ||
| .min(BYTES_PER_MB, T.translate("validation.non_negative")) |
There was a problem hiding this comment.
Validation message does not match the actual minimum constraint
Line 67 enforces a minimum of BYTES_PER_MB (1 MiB), but the message key is validation.non_negative. Users get misleading feedback for values like 0 or 500000.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/sponsors-global/page-templates/page-template-popup/index.js` at
line 67, The validation call uses .min(BYTES_PER_MB,
T.translate("validation.non_negative")) which mislabels the constraint; update
the message passed to .min so it reflects the actual minimum (BYTES_PER_MB)
instead of "validation.non_negative" — either use the correct translation key
(e.g., "validation.min_bytes") or interpolate BYTES_PER_MB into the translated
string via T.translate, and keep this change where .min(BYTES_PER_MB, ...) is
used so users see a correct message for values like 0 or 500000.
| required | ||
| /> | ||
| </Grid2> | ||
| <ItemPriceTiers /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify reinitialize + local enabled state initialization pattern
rg -n -C3 'enableReinitialize|ItemPriceTiers' src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js
rg -n -C5 'const \[enabled, setEnabled\] = useState|useFormikContext|handleToggle|useEffect' src/components/mui/formik-inputs/item-price-tiers.jsRepository: fntechgit/summit-admin
Length of output: 2428
Add useEffect to re-sync ItemPriceTiers local state when form reinitializes.
The enabled state initializes once on mount but doesn't track subsequent Formik value changes. When enableReinitialize: true causes the form to reinitialize (e.g., when switching items), the local state becomes stale while Formik values update, leading to toggles displaying incorrect availability and potentially overwriting rates with outdated state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js`
at line 95, ItemPriceTiers' local enabled state is initialized only on mount and
becomes stale when Formik reinitializes; add a useEffect inside the
ItemPriceTiers component that subscribes to the Formik value(s) controlling tier
availability (via useFormikContext().values or the prop currently passed from
sponsor-form-item-form) and updates the local enabled state whenever those
Formik values change so the toggle reflects reinitialized form values; ensure
the effect's dependency array contains the exact Formik field(s) (e.g.,
values.priceTiersEnabled or the field name used in your form) to avoid
unnecessary updates.
| ), | ||
| sortable: false, | ||
| editable: true, | ||
| editable: (row) => row.early_bird_rate !== "N/A", |
There was a problem hiding this comment.
Avoid hard-coding "N/A" for rate editability checks.
These checks are tied to one locale string. In non-English locales, cells can become incorrectly editable/non-editable.
💡 Suggested fix
+ const notAvailableLabel = T.translate("price_tiers.not_available");
+ const isRateCellEditable = (value) => value !== notAvailableLabel;
+
const sponsorItemColumns = [
@@
- editable: (row) => row.early_bird_rate !== "N/A",
+ editable: (row) => isRateCellEditable(row.early_bird_rate),
@@
- editable: (row) => row.standard_rate !== "N/A",
+ editable: (row) => isRateCellEditable(row.standard_rate),
@@
- editable: (row) => row.onsite_rate !== "N/A",
+ editable: (row) => isRateCellEditable(row.onsite_rate),Also applies to: 226-226, 237-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js`
at line 215, The current editable callbacks (e.g., editable: (row) =>
row.early_bird_rate !== "N/A") rely on a hard-coded locale string; replace this
with a locale-independent check by adding a small helper (e.g.,
isRateEditable(value)) that returns false for null/undefined or when the value
cannot be parsed into a finite number (strip currency/spacing and use
Number.isFinite(parsedNumber)), and true otherwise; then update the editable
arrow functions for early_bird_rate and the other rate columns to call
isRateEditable(row.early_bird_rate) (and the analogous fields) instead of
comparing to "N/A".
| export const isRateEnabled = (value) => | ||
| value !== null && value !== undefined && value !== ""; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In openstack-uicore-foundation/lib/utils/money, what does amountToCents("") return?
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🏁 Script executed:
cat -n src/utils/rate-helpers.jsRepository: fntechgit/summit-admin
Length of output: 1113
🏁 Script executed:
rg "rateToCents" --type js --type ts -B 2 -A 2Repository: fntechgit/summit-admin
Length of output: 4724
🏁 Script executed:
fd -e test.js -e spec.js -e test.ts -e spec.ts | head -20Repository: fntechgit/summit-admin
Length of output: 1209
🏁 Script executed:
fd "rate.*helpers" --type fRepository: fntechgit/summit-admin
Length of output: 132
🏁 Script executed:
cat -n src/utils/__tests__/rate-helpers.test.jsRepository: fntechgit/summit-admin
Length of output: 2132
🏁 Script executed:
rg "rateToCents" -B 5 -A 5 src/actions/sponsor-forms-actions.jsRepository: fntechgit/summit-admin
Length of output: 908
🏁 Script executed:
rg "isRateEnabled" -B 3 -A 3Repository: fntechgit/summit-admin
Length of output: 3250
🏁 Script executed:
rg "amountToCents" src/ --type js --type ts -B 2 -A 2 | head -40Repository: fntechgit/summit-admin
Length of output: 586
Empty-string handling is inconsistent between isRateEnabled and rateToCents
isRateEnabled("") returns false (line 15 checks value !== ""), but rateToCents("") does not guard against empty strings and passes them to amountToCents. This inconsistency can allow empty strings to flow into rate normalization during save operations, potentially persisting invalid values.
Add empty-string check to rateToCents to align with isRateEnabled:
Proposed fix
export const rateToCents = (value) => {
- if (value === null || value === undefined) return null;
+ if (value === null || value === undefined || value === "") return null;
return amountToCents(value);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/rate-helpers.js` around lines 14 - 15, rateToCents currently passes
empty strings into amountToCents while isRateEnabled treats "" as disabled;
modify rateToCents to mirror isRateEnabled by returning undefined (or null,
matching surrounding conventions) when value is null, undefined, or an empty
string (use the same check: value !== null && value !== undefined && value !==
"") so you never call amountToCents with "" and invalid rates cannot be
normalized/persisted; update the function that calls amountToCents (rateToCents)
to early-return when the value fails that check.
| .number() | ||
| .typeError(T.translate("validation.number")) | ||
| .positive(T.translate("validation.number_positive")) | ||
| .positive(T.translate("validation.non_negative")) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Yup, what is the behavioral difference between number().positive()andnumber().min(0)regarding whether0 is valid?
💡 Result:
In Yup:
number().positive()is strictly greater than 0, so0is invalid (it’s implemented asmoreThan(0), i.e.value > 0). [1]number().min(0)is greater than or equal to 0, so0is valid (the check isvalue >= min). [1]
So: use .positive() to reject zero; use .min(0) to allow zero. [1]
Sources:
[1] Yup NumberSchema implementation (min uses >=, positive uses moreThan(0)): (app.unpkg.com)
Citations:
🏁 Script executed:
cat -n src/utils/yup.js | head -180Repository: fntechgit/summit-admin
Length of output: 7246
positive() contradicts the non-negative rule here
Lines 50 and 82 use positive() (> 0), but these validators advertise validation.non_negative and should accept 0. This conflicts with the semantics of "non-negative" (which means >= 0) and is inconsistent with nullableDecimalValidation at line 168, which correctly uses .min(0) for the same message.
Replace positive() with min(0) to allow zero and maintain consistency:
Fix
export const decimalValidation = () =>
yup
.number()
.typeError(T.translate("validation.number"))
- .positive(T.translate("validation.non_negative"))
+ .min(0, T.translate("validation.non_negative"))
.required(T.translate("validation.required"))
export const rateCellValidation = () =>
yup
.number()
- .positive(T.translate("validation.non_negative"))
+ .min(0, T.translate("validation.non_negative"))
.test("max-decimals", T.translate("validation.two_decimals"), (value) => {📝 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.
| .positive(T.translate("validation.non_negative")) | |
| .min(0, T.translate("validation.non_negative")) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/yup.js` at line 50, Replace the use of
.positive(T.translate("validation.non_negative")) with .min(0,
T.translate("validation.non_negative")) in the validators inside
src/utils/yup.js so the rule allows zero (non-negative) instead of requiring >0;
update both occurrences (the one at the shown diff and the other around line 82)
so they match the existing nullableDecimalValidation behavior (which uses
.min(0)) and keep the same translation key.
| export const nullableDecimalValidation = () => | ||
| yup | ||
| .number() | ||
| .nullable() | ||
| .typeError(T.translate("validation.number")) | ||
| .min(0, T.translate("validation.non_negative")) | ||
| .test("max-decimals", T.translate("validation.two_decimals"), (value) => { | ||
| if (value === undefined || value === null) return true; | ||
| return /^\d+(\.\d{1,2})?$/.test(value.toString()); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Yup, does number().nullable()accept an empty string""without a.transform()?
💡 Result:
No. number().nullable() only makes null a valid value; it does not treat the empty string "" as null. In Yup, number() uses parseFloat for casting and “failed casts return NaN” [1]. Since parseFloat("") results in NaN (i.e., not a valid number) [2], validating "" against yup.number().nullable() will still fail unless you add a .transform() (or otherwise convert "" to null/undefined)—a common workaround discussed by Yup users [3].
Sources: [1] Yup docs (GitHub) number() casting / failed casts -> NaN; nullable() applies to null only. [2] MDN parseFloat returns NaN when it can’t parse a number. [3] Yup issue “Number but allow empty string” (#298).
🏁 Script executed:
# Check the context around the validation function
cd src/utils && head -20 yup.js && tail -20 yup.jsRepository: fntechgit/summit-admin
Length of output: 1395
🏁 Script executed:
# Search for usages of nullableDecimalValidation
rg "nullableDecimalValidation" --type jsRepository: fntechgit/summit-admin
Length of output: 1335
🏁 Script executed:
# Check if Formik and MUI are in the project
rg "formik|@mui|material-ui" --type js --type json -l | head -20Repository: fntechgit/summit-admin
Length of output: 1425
🏁 Script executed:
# Look for other validation functions with similar patterns
ast-grep --pattern $'export const $_ = () => yup.number()'Repository: fntechgit/summit-admin
Length of output: 48
🏁 Script executed:
# Check the test file for nullableDecimalValidation
cat src/utils/__tests__/yup.test.js | grep -A 30 "nullableDecimalValidation"Repository: fntechgit/summit-admin
Length of output: 1081
🏁 Script executed:
# Check how the form uses Formik with these fields
cat src/pages/sponsors/sponsor-form-item-list-page/components/sponsor-form-item-form.js | head -100Repository: fntechgit/summit-admin
Length of output: 3518
Add .transform() to normalize empty strings to null in nullableDecimalValidation
With Formik/MUI text inputs, clearing a field yields "". Since number().nullable() only accepts null (not empty string), validation fails on cleared fields. parseFloat("") returns NaN, causing the field to fail the number validation regardless of .nullable().
Proposed fix
export const nullableDecimalValidation = () =>
yup
.number()
+ .transform((value, originalValue) =>
+ originalValue === "" ? null : value
+ )
.nullable()
.typeError(T.translate("validation.number"))
.min(0, T.translate("validation.non_negative"))
.test("max-decimals", T.translate("validation.two_decimals"), (value) => {
if (value === undefined || value === null) return true;
return /^\d+(\.\d{1,2})?$/.test(value.toString());
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/yup.js` around lines 163 - 171, The nullableDecimalValidation
schema fails when Formik/MUI inputs return an empty string because
number().nullable() doesn't accept ""—add a transform to
nullableDecimalValidation that converts empty string (and whitespace-only)
original values to null before other validations run; update the function
nullableDecimalValidation to call .transform((value, originalValue) => (typeof
originalValue === "string" && originalValue.trim() === "" ? null : value)) so
cleared fields validate as null and your existing .nullable(), .typeError(),
.min(), and .test("max-decimals", ...) continue to work.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/__tests__/yup.test.js (1)
7-34: Good test coverage for the main validation scenarios.The test suite appropriately covers null, zero, positive integers, decimal precision limits, and negative values.
Consider adding a test for
undefinedsince the implementation explicitly handles it in the custom test function:if (value === undefined || value === null) return true;🧪 Proposed addition for completeness
it("should pass for null", async () => { await expect(schema.isValid(null)).resolves.toBe(true); }); + + it("should pass for undefined", async () => { + await expect(schema.isValid(undefined)).resolves.toBe(true); + }); it("should pass for 0", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/yup.test.js` around lines 7 - 34, Add a test case to the nullableDecimalValidation suite that asserts undefined is considered valid: inside the existing describe("nullableDecimalValidation", ...) block add an it("should pass for undefined", async () => { await expect(schema.isValid(undefined)).resolves.toBe(true); }); to exercise the explicit guard in the custom test (value === undefined || value === null) and ensure nullableDecimalValidation handles undefined as intended.src/pages/sponsors/sponsor-form-item-list-page/index.js (1)
140-141: Consider using raw rate values or a dedicated availability flag instead of comparing translated display strings.The current logic compares
row.early_bird_rate !== T.translate("price_tiers.not_available")to determine editability. While this works, it couples business logic to display formatting and i18n, making it fragile if the translation changes or formatting logic is updated independently.The codebase already has an
isRateEnabled()helper that could be used if raw values were available. Store raw rate values (or boolean flags) alongside the formatted display values in the reducer, then compare against the underlying data instead of the translated string. This decouples business logic from presentation.This applies to all three rate columns (lines 140–141, 150–151, 160–161).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/sponsors/sponsor-form-item-list-page/index.js` around lines 140 - 141, The editable predicate currently compares the translated display string (row.early_bird_rate !== T.translate("price_tiers.not_available")), which couples logic to i18n; change the data model and predicate to use raw rate values or a boolean availability flag produced by the reducer and the existing helper isRateEnabled(), e.g., populate row.early_bird_rate_raw or row.early_bird_enabled in the reducer alongside the formatted value and update the editable function (and the other two rate column predicates) to call isRateEnabled(row.early_bird_rate_raw) or check row.early_bird_enabled instead of comparing against T.translate; this keeps business logic independent of presentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/sponsors/sponsor-form-item-list-page/index.js`:
- Around line 140-141: The editable predicate currently compares the translated
display string (row.early_bird_rate !==
T.translate("price_tiers.not_available")), which couples logic to i18n; change
the data model and predicate to use raw rate values or a boolean availability
flag produced by the reducer and the existing helper isRateEnabled(), e.g.,
populate row.early_bird_rate_raw or row.early_bird_enabled in the reducer
alongside the formatted value and update the editable function (and the other
two rate column predicates) to call isRateEnabled(row.early_bird_rate_raw) or
check row.early_bird_enabled instead of comparing against T.translate; this
keeps business logic independent of presentation.
In `@src/utils/__tests__/yup.test.js`:
- Around line 7-34: Add a test case to the nullableDecimalValidation suite that
asserts undefined is considered valid: inside the existing
describe("nullableDecimalValidation", ...) block add an it("should pass for
undefined", async () => { await
expect(schema.isValid(undefined)).resolves.toBe(true); }); to exercise the
explicit guard in the custom test (value === undefined || value === null) and
ensure nullableDecimalValidation handles undefined as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9070f818-3948-4fb3-9db8-5dcc0cc7bd6e
📒 Files selected for processing (5)
src/actions/sponsor-forms-actions.jssrc/components/mui/__tests__/item-price-tiers.test.jssrc/pages/sponsors/sponsor-form-item-list-page/index.jssrc/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.jssrc/utils/__tests__/yup.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/actions/sponsor-forms-actions.js
- src/pages/sponsors/sponsor-forms-tab/components/manage-items/sponsor-forms-manage-items.js
- src/components/mui/tests/item-price-tiers.test.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/rate-helpers.js (1)
22-26:⚠️ Potential issue | 🟠 MajorSemantic mismatch: empty string returns
0butisRateEnabled("")returnsfalse.The function treats empty string (cleared input field) as a valid $0.00 rate, but
isRateEnabled("")treats it as disabled. This inconsistency means a user clearing a rate field persists it as zero instead of null, contradicting the semantic meaning of an empty rate value.Change
rateToCents("")to returnnullinstead of0to align withisRateEnabledbehavior.Proposed fix
export const rateToCents = (value) => { if (value === null || value === undefined) return null; - if (value === "") return 0; + if (value === "") return null; return amountToCents(value); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/rate-helpers.js` around lines 22 - 26, rateToCents currently maps an empty string to 0 while isRateEnabled("") treats empty string as disabled; update rateToCents to return null for value === "" so semantics match isRateEnabled, i.e., change the empty-string branch in rateToCents (which calls amountToCents) to return null instead of 0, and run/update any related tests that expect the previous behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/utils/rate-helpers.js`:
- Around line 22-26: rateToCents currently maps an empty string to 0 while
isRateEnabled("") treats empty string as disabled; update rateToCents to return
null for value === "" so semantics match isRateEnabled, i.e., change the
empty-string branch in rateToCents (which calls amountToCents) to return null
instead of 0, and run/update any related tests that expect the previous
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60399247-3a87-4f77-a4a4-f7218bd98361
📒 Files selected for processing (4)
src/utils/__tests__/rate-helpers.test.jssrc/utils/__tests__/yup.test.jssrc/utils/rate-helpers.jssrc/utils/yup.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/utils/tests/rate-helpers.test.js
- src/utils/yup.js
- src/utils/tests/yup.test.js
1 participant
ref: https://app.clickup.com/t/86b8ve10b
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation
Tests