Skip to content
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

Jump to bottom

Handle error for values of invalid data types in DateField #9119

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Handle error for values of invalid data types in DateField #9119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/ra-ui-materialui/src/field/DateField.spec.tsx
View file
Open in desktop
Expand Up @@ -37,6 +37,13 @@ describe('<DateField />', () => {
expect(container.firstChild).toBeNull();
});

it('should return null when the record has an invalid type for the source', () => {
const { container } = render(
<DateField record={{ id: 123, foo: {} }} source="foo" />
);
expect(container.firstChild).toBeNull();
});

it('should render a date', () => {
const { queryByText } = render(
<DateField
Expand Down
20 changes: 12 additions & 8 deletions packages/ra-ui-materialui/src/field/DateField.tsx
View file
Open in desktop
Expand Up @@ -61,7 +61,18 @@ const DateFieldImpl = <
}

const value = get(record, source);
if (value == null || value === '') {
const parsedDate = (value: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

I doin't understand why you extracted to a function just to call it afterwards.

Copy link
Author

@qbantek qbantek Jul 24, 2023 •
edited

Choose a reason for hiding this comment

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

I did it to encapsulate the logic for validating whether the value can represent a Date or not. Probably a matter of style and personal preference, I can change it to inline code obviously.

Copy link
Member

Choose a reason for hiding this comment

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

yes, please inline this code

if (value == null || value === '') return undefined;

return value instanceof Date
? value
: typeof value === 'string' || typeof value === 'number'
? new Date(value)
: undefined;
};
const date = parsedDate(value);

if (typeof date === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the value == null test instead.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure if I understood this request properly: Are you asking to have BOTH value == null and typeof date === 'undefined' or to get rid if the undefined criteria?

The way I see it the null check is handled before (in parsedDate function) and the undefined check is needed to avoid the error, kinda the whole point of this PR. Please correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

No you're right, the check is already made in parsedDate, you can keep your code as is for this part.

return emptyText ? (
<Typography
component="span"
Expand All @@ -74,13 +85,6 @@ const DateFieldImpl = <
) : null;
}

const date =
value instanceof Date
? value
: typeof value === 'string' || typeof value === 'number'
? new Date(value)
: undefined;

let dateOptions = options;
if (
typeof value === 'string' &&
Expand Down