Fixed : Automatic Correction for Invalid End Date Input #9583
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)src/components/Form/FormFields/DateRangeFormField.tsx (2)
This logic correctly adjusts the end date if it's earlier than the start date, aligning with the PR’s objective. However, be sure to cover edge cases, such as when the user enters only one date or toggles back-and-forth between invalid dates. Automated tests will help confirm this works in all scenarios. Do you want me to create sample test cases for these date adjustments or open a new GitHub issue to track it?
Using the new 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
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Patient/PatientFilter.tsx (2)
272-273: Consider using a unified date library for consistency.You're using
dayjsin other parts of the file but defaulting to nativeDatehere. It would be more consistent (and potentially safer) to rely on the same library (dayjs) for parsing and comparing dates if possible.- const startDate = event.value.start; - let endDate = event.value.end; + const startDate = event.value.start ? dayjs(event.value.start) : null; + let endDate = event.value.end ? dayjs(event.value.end) : null;
274-276: Automatic end date fix is correct; consider notifying the user.Automatically adjusting the end date to match the start date when it is earlier than the start date is logical. However, consider providing visual feedback or a small notification to the user for transparency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientFilter.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Patient/PatientFilter.tsx (1)
278-279: Confirm date string format for downstream usage.
Storing the dates as string (startDate?.toString(), endDate?.toString()) might affect other components or queries. Verify if the next stages of processing expect stringified ISO dates or need a specific format.
✅ Verification successful
Date string format is properly handled through dateQueryString utility
The codebase shows that date strings in filterData are processed through the dateQueryString utility before being used in API calls. This is evident from the code in PatientFilter.tsx where all date fields are processed:
{
date_declared_positive_before: dateQueryString(date_declared_positive_before),
date_declared_positive_after: dateQueryString(date_declared_positive_after),
created_date_before: dateQueryString(created_date_before),
created_date_after: dateQueryString(created_date_after),
// ... other date fields
}The toString() conversion is safe as the dates are properly formatted before being used in queries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Identify references to <event.name>_after / <event.name>_before fields to confirm any format expectations.
rg -A 5 "\_after\"\]|\_before\"\]"
Length of output: 123
Script:
#!/bin/bash
# Search for date-related fields with _after or _before suffixes
rg -A 5 "_after|_before"
Length of output: 37287
Script:
#!/bin/bash
# Search for filterData usage to understand how these dates are processed
rg -A 5 "filterData\["
Length of output: 3081
Script:
#!/bin/bash
# Look for API calls or query builders that might use these date filters
ast-grep --pattern 'axios.$_($$$, { params: filterData })'
Length of output: 60
Script:
#!/bin/bash
# Search for any date formatting or parsing utilities
rg -A 5 "toISOString|toLocaleDateString|format.*Date"
Length of output: 65880
|
@rithviknishad @Jacobjeevan Any changes required ? |
Jacobjeevan
added
needs testing
needs review
and removed
question
|
@rithviknishad @Jacobjeevan Applied Same logic in all Filters related to calendar |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Assets/AssetFilter.tsx (2)
84-86: Use dayjs-based comparisons for consistency.
Currently, you're creatingDateobjects for comparison (new Date(endDate) < new Date(startDate)), whereas the rest of the file usesdayjs. To avoid potential edge cases or time zone discrepancies, consider using dayjs consistently across the codebase.- if (startDate && endDate && new Date(endDate) < new Date(startDate)) { + if (startDate && endDate && dayjs(endDate).isBefore(dayjs(startDate))) {
92-93: Ensure correct naming forbefore/afterto prevent confusion.
It might be more intuitive to storewarrantyExpiryStartandwarrantyExpiryEndor some naming that aligns with “start” and “end.” The current naming ofbeforeandafteris somewhat reversed, potentially leading to confusion when reading the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Assets/AssetFilter.tsx(1 hunks)src/components/Resource/ResourceFilter.tsx(1 hunks)src/components/Shifting/ShiftingFilters.tsx(1 hunks)
🔇 Additional comments (7)
src/components/Assets/AssetFilter.tsx (1)
87-89: Auto-correction logic looks good!
This condition enforces that the end date cannot precede the start date, thus aligning perfectly with the PR objective of automatic correction.
src/components/Resource/ResourceFilter.tsx (3)
131-133: Consistent naming of temporary variables.
Using startDate and endDate aligns well with your naming convention in the other files. Good job on unifying the approach.
134-136: Solid date validation.
Preventing endDate from being before startDate addresses the invalid range scenario efficiently. No issues spotted.
137-138: Consider a single source of truth for date-friendly strings.
After setting the new date range, you’re converting them directly to strings. If these values are used downstream, ensure they’re consistently parsed into valid Date objects or dayjs. Sudden type mismatches could lead to future debugging complexities.
src/components/Shifting/ShiftingFilters.tsx (3)
197-199: Helpful variable extraction for clarity.
Defining startDate and endDate before validation makes the code easier to read and maintain.
200-202: Matches PR objective of automatically correcting invalid end date inputs.
Swapping the end date to the start date if it’s earlier effectively prevents invalid ranges. This aligns with the user story in PR #9583.
204-205: String conversion might impact date/time offsets.
When storing dates as strings, confirm that no unexpected timezone offset is introduced. Consider maintaining a standardized format or using dayjs for safe reconversion downstream.
|
@rithviknishad Updated in Common DateRangeForm |
rithviknishad
added
needs testing
needs review
and removed
invalid
|
Closing as we no longer have these filters in |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
date_time.mp4
Summary by CodeRabbit
New Features
Bug Fixes