Skip to content

fix: adjust return order on normalizeSelectAllField function #948

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.

Sign up for GitHub

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

Merged
smarcet merged 2 commits into from
May 20, 2026
+24 −6
Merged

fix: adjust return order on normalizeSelectAllField function #948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1a17e4
fix: adjust return order on normalizeSelectAllField function
tomrndom May 18, 2026
06cf653
fix: add test cases for allSelected prop
tomrndom May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/utils/__tests__/methods.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,24 @@ describe("getMediaInputValue", () => {
});
});

it.each([[], null, undefined])(
"should return apply_to_all true when allSelected is true and items is %s",
(items) => {
expect(
normalizeSelectAllField(items, "apply_to_all", "items", true)
).toEqual({ apply_to_all: true, items: [] });
}
);

it.each([[], null, undefined])(
"should return apply_to_all false when allSelected is false and items is %s",
(items) => {
expect(
normalizeSelectAllField(items, "apply_to_all", "items", false)
).toEqual({ apply_to_all: false, items: [] });
}
);

it("should return array of ids when items are objects with id", () => {
expect(
normalizeSelectAllField([{ id: 1 }, { id: 2 }], "apply_to_all", "items")
Expand Down
12 changes: 6 additions & 6 deletions src/utils/methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,19 +601,19 @@ export const normalizeSelectAllField = (
listName,
allSelected
) => {
if (!items?.length)
const isAllSelected = allSelected ?? items?.includes("all");
Copy link
Copy Markdown

@smarcet smarcet May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom
No test coverage for the fixed edge case
The exact bug scenario (allSelected=true with empty/null/undefined items) has no test — the three changed paths can regress silently on any future refactor of this utility used across 7+ call sites .
Add tests for normalizeSelectAllField([], "apply_to_all", "items", true), same for null and undefined items — each should assert { apply_to_all: true, items: [] }

Copy link
Copy Markdown

@smarcet smarcet May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomrndom
Add a test for normalizeSelectAllField([], "flag", "list", false) asserting { flag: false, list: [] } to pin this behavior

if (isAllSelected) {
return {
[flagName]: false,
[flagName]: true,
[listName]: []
};
}

const isAllSelected = allSelected ?? items.includes("all");
if (isAllSelected) {
if (!items?.length)
return {
[flagName]: true,
[flagName]: false,
[listName]: []
};
}
return {
[flagName]: false,
[listName]: items.map((a) => a.id ?? a)
Expand Down
Loading