fix: adjusting validations and payload on sponsor forms template #823
fix: adjusting validations and payload on sponsor forms template #823
Conversation
…oints Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
📝 WalkthroughWalkthroughAdds a new utility Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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.
🧹 Nitpick comments (1)
src/actions/sponsor-forms-actions.js (1)
568-575: Consider restructuring to avoid intermediate undefined mapping.When
add_onscontains the string"all", line 569 maps it toundefined(since"all".idis undefined) before lines 572-573 overwrite the array. The code works correctly, but could be clearer:♻️ Suggested refactor for clarity
if (entity.add_ons?.length > 0) { - normalizedEntity.allowed_add_ons = entity.add_ons.map((a) => a.id); - normalizedEntity.apply_to_all_add_ons = false; if (entity.add_ons.includes("all")) { normalizedEntity.apply_to_all_add_ons = true; normalizedEntity.allowed_add_ons = []; + } else { + normalizedEntity.allowed_add_ons = entity.add_ons.map((a) => a.id); + normalizedEntity.apply_to_all_add_ons = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/sponsor-forms-actions.js` around lines 568 - 575, The current block maps entity.add_ons to normalizedEntity.allowed_add_ons before checking for the special "all" token, which yields an undefined entry for the string "all"; update the logic in the sponsor-forms normalization so you first detect if entity.add_ons includes the string "all" and set normalizedEntity.apply_to_all_add_ons = true and normalizedEntity.allowed_add_ons = [] in that case, otherwise set apply_to_all_add_ons = false and populate normalizedEntity.allowed_add_ons by mapping only addon objects (e.g., filter out non-objects or use a guard when mapping) to avoid producing undefined values from ("all").
🤖 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/actions/sponsor-forms-actions.js`:
- Around line 568-575: The current block maps entity.add_ons to
normalizedEntity.allowed_add_ons before checking for the special "all" token,
which yields an undefined entry for the string "all"; update the logic in the
sponsor-forms normalization so you first detect if entity.add_ons includes the
string "all" and set normalizedEntity.apply_to_all_add_ons = true and
normalizedEntity.allowed_add_ons = [] in that case, otherwise set
apply_to_all_add_ons = false and populate normalizedEntity.allowed_add_ons by
mapping only addon objects (e.g., filter out non-objects or use a guard when
mapping) to avoid producing undefined values from ("all").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2d8dc02-cd64-496a-85d9-d8132f348799
📒 Files selected for processing (4)
src/actions/sponsor-forms-actions.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/global-template-popup.jssrc/pages/sponsors/sponsor-forms-list-page/components/global-template/select-sponsorships-dialog.jssrc/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js
💤 Files with no reviewable changes (1)
- src/pages/sponsors/sponsor-forms-tab/components/add-sponsor-form-template-popup/index.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| // eslint-disable-next-line no-magic-numbers | ||
| export const bytesToMb = (bytes) => (bytes / BYTES_IN_MEGABYTE).toFixed(2); | ||
|
|
||
| export const normalizeSelectAllField = ( |
There was a problem hiding this comment.
@tomrndom
returns {} when items is empty or null:
if (!items?.length) return {};
The old code in normalizeSponsorManagedForm always sent both fields:
allowed_add_ons: entity.add_ons.map((a) => a.id), // [] when empty
apply_to_all_add_ons: false
After this change, an empty add_ons array produces no allowed_add_ons or apply_to_all_add_ons in the payload at all. Same issue applies to cloneGlobalTemplate — empty sponsorIds now omits both sponsorship_types and
apply_to_all_types. If the API requires or expects these fields, this is a breaking change.
Suggestion: Return both fields with defaults instead of {}:
if (!items?.length) return { [flagName]: false, [listName]: [] };
create a unit test for this
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/__tests__/methods.test.js (1)
59-104: Add one regression case forallSelected = falsewith"all"present.Nice coverage overall; one extra case would protect the precedence edge case in
normalizeSelectAllField.Suggested test addition
describe("normalizeSelectAllField", () => { @@ it("should return all selected when allSelected flag is true", () => { expect( normalizeSelectAllField([{ id: 1 }], "apply_to_all", "items", true) ).toEqual({ apply_to_all: true, items: [] }); }); + + it("should still return all selected when items contains 'all' and allSelected is false", () => { + expect( + normalizeSelectAllField(["all", { id: 1 }], "apply_to_all", "items", false) + ).toEqual({ + apply_to_all: true, + items: [] + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/methods.test.js` around lines 59 - 104, Add a regression test for normalizeSelectAllField to ensure an explicit allSelected = false overrides a literal "all" in the items array: call normalizeSelectAllField(["all", { id: 1 }], "apply_to_all", "items", false) and assert the result is { apply_to_all: false, items: [1] } so the function ignores the "all" marker when the allSelected flag is explicitly false.
🤖 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/utils/methods.js`:
- Around line 571-580: The current isAllSelected calculation (isAllSelected =
allSelected ?? items.includes("all")) lets an explicit false with items
containing "all" leak into the payload; change the logic so
items.includes("all") takes precedence (e.g. compute isAllSelected =
items.includes("all") || Boolean(allSelected)) and also ensure when building the
fallback list you filter out the literal "all" before mapping (use
items.filter(a => a !== "all") before .map) so flagName/listName behavior is
consistent.
---
Nitpick comments:
In `@src/utils/__tests__/methods.test.js`:
- Around line 59-104: Add a regression test for normalizeSelectAllField to
ensure an explicit allSelected = false overrides a literal "all" in the items
array: call normalizeSelectAllField(["all", { id: 1 }], "apply_to_all", "items",
false) and assert the result is { apply_to_all: false, items: [1] } so the
function ignores the "all" marker when the allSelected flag is explicitly false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0733dd8b-6b5d-4a47-ab6d-475d50d37cba
📒 Files selected for processing (2)
src/utils/__tests__/methods.test.jssrc/utils/methods.js
| const isAllSelected = allSelected ?? items.includes("all"); | ||
| if (isAllSelected) { | ||
| return { | ||
| [flagName]: true, | ||
| [listName]: [] | ||
| }; | ||
| } | ||
| return { | ||
| [flagName]: false, | ||
| [listName]: items.map((a) => a.id ?? a) |
There was a problem hiding this comment.
allSelected precedence can leak "all" into payload.
On Line 571, allSelected ?? items.includes("all") treats explicit false as final. If items still contains "all", the function can return listName: ["all", ...] instead of toggling flagName: true, which can break payload expectations.
Suggested fix
- const isAllSelected = allSelected ?? items.includes("all");
+ const isAllSelected = allSelected === true || items.includes("all");📝 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 isAllSelected = allSelected ?? items.includes("all"); | |
| if (isAllSelected) { | |
| return { | |
| [flagName]: true, | |
| [listName]: [] | |
| }; | |
| } | |
| return { | |
| [flagName]: false, | |
| [listName]: items.map((a) => a.id ?? a) | |
| const isAllSelected = allSelected === true || items.includes("all"); | |
| if (isAllSelected) { | |
| return { | |
| [flagName]: true, | |
| [listName]: [] | |
| }; | |
| } | |
| return { | |
| [flagName]: false, | |
| [listName]: items.map((a) => a.id ?? a) | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/methods.js` around lines 571 - 580, The current isAllSelected
calculation (isAllSelected = allSelected ?? items.includes("all")) lets an
explicit false with items containing "all" leak into the payload; change the
logic so items.includes("all") takes precedence (e.g. compute isAllSelected =
items.includes("all") || Boolean(allSelected)) and also ensure when building the
fallback list you filter out the literal "all" before mapping (use
items.filter(a => a !== "all") before .map) so flagName/listName behavior is
consistent.
2 participants
ref:
https://app.clickup.com/t/86b8u5uqb
https://app.clickup.com/t/86b8u65jm
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Bug Fixes
Improvements
New Features