feat: Enhance date validation logic and add tests for timePrecision in DatePickerWidget2 #37218
Conversation
…granularity checks (#37210)
… ensure correct date handling across boundaries
WalkthroughThe changes in this pull request primarily involve the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Bug
github-actions
bot
added
Enhancement
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11679324581. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.test.js (1)
105-118: Consider adding boundary test cases for minute precision.While the current test verifies that seconds don't affect validation, consider adding edge cases where the minute is exactly at the boundary.
+ it("should pass when selectedDate minute exactly matches maxDate", () => { + const { isValidDate } = derivedProperty; + const input = { + isRequired: true, + maxDate: "2021-12-01T05:49:00.000Z", + minDate: "2021-12-01T05:48:00.000Z", + selectedDate: "2021-12-01T05:49:00.000Z", + timePrecision: "minute", + }; + expect(isValidDate(input, moment, _)).toStrictEqual(true); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/widgets/DatePickerWidget2/widget/derived.js(1 hunks)app/client/src/widgets/DatePickerWidget2/widget/derived.test.js(1 hunks)
🔇 Additional comments (2)
app/client/src/widgets/DatePickerWidget2/widget/derived.test.js (2)
89-103: LGTM! Good test coverage for minute precision.
The test effectively validates that dates with incorrect minute values are rejected.
150-207: LGTM! Comprehensive test coverage for 'None' precision.
The test suite thoroughly covers all scenarios for date validation without time precision:
- Exact matches with min/max dates
- Valid dates within range
- Invalid dates outside range
|
Deploy-Preview-URL: https://ce-37218.dp.appsmith.com |
…dability and early returns on optional selections
…emoving unused vars and consolidating comparison settings
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11681629187. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.js(1 hunks)
🔇 Additional comments (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.js (1)
2-18: LGTM! Clean implementation of early returns
The early validation checks and function signature cleanup improve code clarity.
|
Deploy-Preview-URL: https://ce-37218.dp.appsmith.com |
…idDate logic and adjusting timePrecision comparisons
…o rahulbarwal/issue37083
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11702159642. |
…g unnecessary timePrecision checks for min/max dates
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11702215554. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11702241917. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.js (1)
18-33: Document default case behavior.While the default case is handled during initialization as mentioned in past discussions, it would be helpful to add a comment explaining this behavior for future maintainers.
Add a comment above the switch statement:
+ // Note: Default values for granularity and inclusivity are handled during component initialization switch (props.timePrecision) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
app/client/src/widgets/DatePickerWidget2/widget/derived.js (1)
Learnt from: rahulbarwal
PR: appsmithorg/appsmith#37218
File: app/client/src/widgets/DatePickerWidget2/widget/derived.js:20-34
Timestamp: 2024-11-05T11:23:44.713Z
Learning: In the `DatePickerWidget2`'s `isValidDate` function, the default case for `timePrecision` is handled during initialization, so no additional default case is necessary in the switch statement.
🔇 Additional comments (3)
app/client/src/widgets/DatePickerWidget2/widget/derived.js (3)
4-5: LGTM! Good use of early return pattern.
The early validation check simplifies the control flow and improves readability.
14-16: LGTM! Proper handling of required field validation.
The logic correctly handles the case when no date is selected based on the isRequired flag.
35-45: LGTM! Proper date range validation with inclusive comparisons.
The use of isSameOrAfter and isSameOrBefore correctly addresses the validation issues mentioned in #37083, ensuring consistent behavior when selecting today's date.
|
Deploy-Preview-URL: https://ce-37218.dp.appsmith.com |
…n DatePickerWidget2 (appsmithorg#37218) ## Description <ins>Problem</ins> The DatePickerWidget2 component had incomplete date validation logic, allowing incorrect dates to be selected, and lacked comprehensive testing for time precision. <ins>Root cause</ins> The date validation logic did not accurately account for time precision, and the testing was limited, making it difficult to ensure the component's correctness. <ins>Solution</ins> This PR enhances the date validation logic in DatePickerWidget2 to provide better granularity checks based on the timePrecision property, and adds comprehensive tests to ensure correct date handling across boundaries for different timePrecision settings. This PR handles... - Enhanced date validation logic to accurately account for time precision. - Comprehensive testing to ensure correct date handling for all possible input combinations. Fixes appsmithorg#37083 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Datepicker" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11702221741> > Commit: 306373a > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11702221741&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Datepicker` > Spec: > <hr>Wed, 06 Nov 2024 11:32:28 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced date validation logic in the DatePickerWidget2 for improved accuracy based on time precision. - **Tests** - Introduced comprehensive test cases for the `isValidDate` function, covering various scenarios related to time precision, ensuring robust validation against defined date ranges. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Problem
The DatePickerWidget2 component had incomplete date validation logic, allowing incorrect dates to be selected, and lacked comprehensive testing for time precision.
Root cause
The date validation logic did not accurately account for time precision, and the testing was limited, making it difficult to ensure the component's correctness.
Solution
This PR enhances the date validation logic in DatePickerWidget2 to provide better granularity checks based on the timePrecision property, and adds comprehensive tests to ensure correct date handling across boundaries for different timePrecision settings. This PR handles...
Fixes #37083
or
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Datepicker"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11702221741
Commit: 306373a
Cypress dashboard.
Tags:
@tag.DatepickerSpec:
Wed, 06 Nov 2024 11:32:28 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests
isValidDatefunction, covering various scenarios related to time precision, ensuring robust validation against defined date ranges.