Fix: Implement Date Range Validation for Activity Dates #4515
Conversation
📝 WalkthroughWalkthroughThe pull request updates the date validation logic in the device registry activities validators. A new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Validator as Validator
participant Moment as moment Library
Client->>Validator: Submit request with date
Validator->>Moment: Retrieve current date and date one month prior
Moment-->>Validator: Return date boundaries
Validator->>Validator: Check if date is within allowed range
alt Date is valid
Validator-->>Client: Continue processing activity
else Date is invalid
Validator-->>Client: Respond with updated error message
end
Suggested Reviewers
Poem
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4515 +/- ##
===========================================
- Coverage 11.45% 11.24% -0.22%
===========================================
Files 121 156 +35
Lines 15666 18030 +2364
Branches 0 388 +388
===========================================
+ Hits 1795 2027 +232
- Misses 13871 16001 +2130
- Partials 0 2 +2
|
|
Device registry changes in this PR available for preview here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/device-registry/validators/activities.validators.js (1)
418-418:⚠️ Potential issuePotential redundant date validation for deployActivity
This appears to be a duplicate application of the date validation that's already applied at line 393. The validation is already being spread from
commonValidations.date, so this additional spread is redundant.- ...commonValidations.date,
🧹 Nitpick comments (2)
src/device-registry/validators/activities.validators.js (2)
179-193: Well-implemented date range validationThe custom validation logic for date fields is implemented well, ensuring dates aren't in the future and aren't more than one month in the past. This matches the PR objectives perfectly.
One suggestion: Consider adding a custom error message that includes both constraints in a single message for better user experience.
- if (inputDate.isAfter(now)) { - throw new Error("date cannot be in the future"); - } - if (inputDate.isBefore(oneMonthAgo)) { - throw new Error("date cannot be more than one month in the past"); - } + if (inputDate.isAfter(now)) { + throw new Error("Date must be in the past and not more than one month old"); + } + if (inputDate.isBefore(oneMonthAgo)) { + throw new Error("Date must be in the past and not more than one month old"); + }
179-193: Consider adding moment timezone handlingThe current implementation uses local time zone for validation. If your application handles dates from different time zones, consider using
moment.utc()or explicitly specify the expected timezone to ensure consistent validation regardless of server location.- const now = moment(); - const oneMonthAgo = moment().subtract(1, "month"); - const inputDate = moment(date); + const now = moment.utc(); + const oneMonthAgo = moment.utc().subtract(1, "month"); + const inputDate = moment.utc(date);Also applies to: 513-527
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device-registry/validators/activities.validators.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/device-registry/validators/activities.validators.js (5)
6-6: Good addition of the moment libraryAdding moment as a dependency makes sense for the date validation requirements. It provides robust tools for date manipulation and comparison.
349-349: Appropriate application of validation to recallActivityCorrectly using the spread operator to apply the common date validations to this activity type. This ensures consistent validation behavior across the application.
393-393: Appropriate application of validation to deployActivityCorrectly implementing the date validation for deployment activities. The spread operator ensures consistent validation behavior.
432-432: Date validation correctly applied to maintainActivityThe date validation has been correctly applied to the maintenance activity type, ensuring consistent validation across all activity types.
6-6:❓ Verification inconclusive
Verify moment dependency version
Make sure the moment library is already in the package.json dependencies or add it if it's not. Also, consider using a more modern date library like date-fns or luxon as moment.js is now considered legacy.
Also applies to: 179-193, 513-527
🏁 Script executed:
#!/bin/bash # Check if moment is already in package.json dependencies echo "Checking if moment is in package.json..." if grep -q '"moment":' package.json; then echo "moment is already in package.json" grep -A 1 '"moment":' package.json else echo "moment is NOT in package.json and needs to be added" fi # Check for more modern alternatives echo "Checking for modern date libraries..." if grep -q '"date-fns":' package.json || grep -q '"luxon":' package.json; then echo "Modern date library already exists in package.json" grep -A 1 '"date-fns":' package.json || grep -A 1 '"luxon":' package.json else echo "Consider using modern alternatives like date-fns or luxon instead of moment.js" fiLength of output: 699
Action Required: Verify Dependency Declaration for Moment.js
It appears that our automated check couldn’t locate a
package.jsonfile at the repository root, which is where dependencies like moment.js are typically managed. Please ensure that:
- The moment dependency is declared in the appropriate
package.jsonfile. If you are using a monorepo or a sub-package structure, verify that the dependency is added in the correct location.- Given that moment.js is considered legacy, consider evaluating modern alternatives (e.g., date-fns or luxon) for new implementations.
This verification is applicable not only to the import in
src/device-registry/validators/activities.validators.js(line 6) but also to similar patterns noted in ranges 179-193 and 513-527.
| .withMessage("Invalid date format") | ||
| .bail() | ||
| .custom((date) => { | ||
| const now = moment(); | ||
| const oneMonthAgo = moment().subtract(1, "month"); | ||
| const inputDate = moment(date); | ||
|
|
||
| if (inputDate.isAfter(now)) { | ||
| throw new Error("date cannot be in the future"); | ||
| } | ||
| if (inputDate.isBefore(oneMonthAgo)) { | ||
| throw new Error("date cannot be more than one month in the past"); | ||
| } | ||
| return true; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication in batchDeployActivity date validation
The date validation logic here duplicates what's already in commonValidations.date. Consider using a reusable function for this validation to avoid future maintenance issues.
You could potentially refactor this to reuse the validation logic:
- body("*.date")
- .exists()
- .withMessage("date is required")
- .trim()
- .toDate()
- .isISO8601({ strict: true, strictSeparator: true })
- .withMessage("Invalid date format")
- .bail()
- .custom((date) => {
- const now = moment();
- const oneMonthAgo = moment().subtract(1, "month");
- const inputDate = moment(date);
-
- if (inputDate.isAfter(now)) {
- throw new Error("date cannot be in the future");
- }
- if (inputDate.isBefore(oneMonthAgo)) {
- throw new Error("date cannot be more than one month in the past");
- }
- return true;
- }),
+ body("*.date")
+ .exists()
+ .withMessage("date is required")
+ .trim()
+ .toDate()
+ .isISO8601({ strict: true, strictSeparator: true })
+ .withMessage("Invalid date format")
+ .bail()
+ .custom((date) => {
+ return commonValidations.date[0].builder.custom.func(date);
+ }),Alternatively, consider creating a reusable function outside the validation definitions that both can use:
+ // Add this near the top of the file, after the imports
+ const validateDateRange = (date) => {
+ const now = moment();
+ const oneMonthAgo = moment().subtract(1, "month");
+ const inputDate = moment(date);
+
+ if (inputDate.isAfter(now)) {
+ throw new Error("date cannot be in the future");
+ }
+ if (inputDate.isBefore(oneMonthAgo)) {
+ throw new Error("date cannot be more than one month in the past");
+ }
+ return true;
+ };And then use it in both places:
- .custom((date) => {
- const now = moment();
- const oneMonthAgo = moment().subtract(1, "month");
- const inputDate = moment(date);
-
- if (inputDate.isAfter(now)) {
- throw new Error("date cannot be in the future");
- }
- if (inputDate.isBefore(oneMonthAgo)) {
- throw new Error("date cannot be more than one month in the past");
- }
- return true;
- }),
+ .custom(validateDateRange),📝 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.
| .withMessage("Invalid date format") | |
| .bail() | |
| .custom((date) => { | |
| const now = moment(); | |
| const oneMonthAgo = moment().subtract(1, "month"); | |
| const inputDate = moment(date); | |
| if (inputDate.isAfter(now)) { | |
| throw new Error("date cannot be in the future"); | |
| } | |
| if (inputDate.isBefore(oneMonthAgo)) { | |
| throw new Error("date cannot be more than one month in the past"); | |
| } | |
| return true; | |
| }), | |
| .withMessage("Invalid date format") | |
| .bail() | |
| .custom((date) => { | |
| return commonValidations.date[0].builder.custom.func(date); | |
| }), |
|
Device registry changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/device-registry/validators/activities.validators.js (2)
197-208: Good implementation of array date validation.The newly added
eachDatevalidation array effectively applies the same validation rules to each date in an array. This is a clean implementation that reuses thevalidateDateRangefunction, promoting code reusability and consistency.One minor enhancement could be to add a comment explaining that this validator is specifically for handling arrays of objects containing date fields. This would make the code even more self-documenting:
eachDate: [ + // Validates date fields in each object of an array body("*.date") .exists() .withMessage("date is missing") .bail() .trim() .toDate() .isISO8601({ strict: true, strictSeparator: true }) .withMessage("date must be a valid datetime.") .bail() .custom(validateDateRange), ],
9-21: Consider adding timezone handling for date validation.The current implementation uses the default moment behavior for timezone handling. In a system where users might be in different timezones, this could lead to edge cases where a date that appears valid in one timezone might be rejected in another.
Consider explicitly handling timezones or documenting the expected timezone for date inputs to ensure consistent validation behavior across different environments and user locations.
const validateDateRange = (date) => { - const now = moment(); - const oneMonthAgo = moment().subtract(1, "month"); - const inputDate = moment(date); + // Use UTC to ensure consistent timezone handling + const now = moment().utc(); + const oneMonthAgo = moment().utc().subtract(1, "month"); + const inputDate = moment(date).utc(); if (inputDate.isAfter(now)) { throw new Error("date cannot be in the future"); } if (inputDate.isBefore(oneMonthAgo)) { throw new Error("date cannot be more than one month in the past"); } return true; };Also applies to: 193-195, 197-208
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-12: src/device-registry/validators/activities.validators.js#L10-L12
Added lines #L10 - L12 were not covered by tests
[warning] 14-15: src/device-registry/validators/activities.validators.js#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 17-18: src/device-registry/validators/activities.validators.js#L17-L18
Added lines #L17 - L18 were not covered by tests
[warning] 20-20: src/device-registry/validators/activities.validators.js#L20
Added line #L20 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device-registry/validators/activities.validators.js(5 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/device-registry/validators/activities.validators.js
[warning] 10-12: src/device-registry/validators/activities.validators.js#L10-L12
Added lines #L10 - L12 were not covered by tests
[warning] 14-15: src/device-registry/validators/activities.validators.js#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 17-18: src/device-registry/validators/activities.validators.js#L17-L18
Added lines #L17 - L18 were not covered by tests
[warning] 20-20: src/device-registry/validators/activities.validators.js#L20
Added line #L20 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-device-registry
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/device-registry/validators/activities.validators.js (5)
6-6: Good addition of the moment library.The moment library is an excellent choice for handling date operations in JavaScript. It provides robust functionality for parsing, validating, manipulating, and formatting dates.
193-195: Enhanced date validation logic.The updated date validation now properly validates both the format (ISO8601) and applies business rules through the custom validation function. This is a significant improvement over just validating the format.
363-363: Good use of spread operator for date validation.Using the spread operator to include the
commonValidations.datearray is a clean approach. This ensures consistency in date validation across different activities and simplifies maintenance.
407-407: Consistent validation pattern for deploy activities.Applying the same date validation pattern to
deployActivityensures consistency across different activity types. This is a good practice for maintaining a uniform validation approach throughout the codebase.
521-521: Addressed previous code duplication feedback.Excellent work on addressing the previous review comment about code duplication. By using
commonValidations.eachDate, you've eliminated the duplication while maintaining the required validation logic.
| const validateDateRange = (date) => { | ||
| const now = moment(); | ||
| const oneMonthAgo = moment().subtract(1, "month"); | ||
| const inputDate = moment(date); | ||
|
|
||
| if (inputDate.isAfter(now)) { | ||
| throw new Error("date cannot be in the future"); | ||
| } | ||
| if (inputDate.isBefore(oneMonthAgo)) { | ||
| throw new Error("date cannot be more than one month in the past"); | ||
| } | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-structured date validation function.
The validateDateRange function effectively implements the business requirement to validate that dates are not in the future and not more than one month in the past. The function is clean, focused, and reusable across different validators.
However, the static analysis indicates this function isn't covered by tests yet. Consider adding specific test cases for this function to ensure it correctly handles edge cases.
🏁 Script executed:
#!/bin/bash
# Check if there are any test files for the validators
# Look for test files related to activities validators
echo "Searching for test files related to activities validators..."
fd "activities.*test" -t f
# Check if there are any date validation tests in the test files
echo "Checking for date validation tests in test files..."
rg -i "test.*date.*validation|validate.*date" --type jsLength of output: 15618
Action: Enhance Test Coverage for validateDateRange Function
The implementation of the validateDateRange function in src/device-registry/validators/activities.validators.js (lines 9–21) is clean and effectively enforces the intended business rules. However, our search confirms that there are currently no dedicated tests covering this function. Please add unit tests that verify:
- A date in the future correctly triggers an error.
- A date more than one month in the past correctly triggers an error.
- A valid date within the acceptable range returns
true.
This will help ensure robust edge-case handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 10-12: src/device-registry/validators/activities.validators.js#L10-L12
Added lines #L10 - L12 were not covered by tests
[warning] 14-15: src/device-registry/validators/activities.validators.js#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 17-18: src/device-registry/validators/activities.validators.js#L17-L18
Added lines #L17 - L18 were not covered by tests
[warning] 20-20: src/device-registry/validators/activities.validators.js#L20
Added line #L20 was not covered by tests
There was a problem hiding this comment.
Create new Github issue for this.
There was a problem hiding this comment.
🧩 Analysis chain
@Baalmart I've created a new GitHub issue titled "Add test coverage for validateDateRange function" and assigned it to you. The issue includes:
- A detailed description of the problem
- Specific test requirements for the
validateDateRangefunction - Context linking back to this PR
- Importance of adding these tests
- Clear acceptance criteria
This will help track the need for test coverage separately while allowing you to proceed with the current PR implementation. The issue captures all the necessary details to implement proper test coverage for the date validation logic in a future update.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for adding test coverage for validateDateRange function
# Create the issue
gh issue create \
--title "Add test coverage for validateDateRange function" \
--body "## Description
The newly implemented \`validateDateRange\` function in \`src/device-registry/validators/activities.validators.js\` lacks test coverage. This function is critical as it validates that activity dates are not in the future and not more than one month in the past.
## Requirements
Please add unit tests that verify:
- A date in the future correctly triggers an error.
- A date more than one month in the past correctly triggers an error.
- A valid date within the acceptable range returns \`true\`.
## Context
- Referenced in PR: #4515
- Comment URL: https://github.com/airqo-platform/AirQo-api/pull/4515#discussion_r1977414281
## Importance
Adding these tests will help ensure robust edge-case handling and maintain code quality as the application evolves.
## Acceptance Criteria
- [ ] Unit tests created for the \`validateDateRange\` function
- [ ] All test cases passing
- [ ] Code coverage for the function reaches 100%
" \
--assignee "Baalmart"
Length of output: 1096
@Baalmart Great! I've successfully created GitHub issue #4516 for tracking the test coverage requirements for the validateDateRange function. You can access it here: #4516
The issue includes:
- Detailed description of the missing test coverage
- Specific test cases that should be implemented
- Clear acceptance criteria for completion
- Reference back to this PR for context
This will help ensure the validation logic receives proper test coverage while allowing this PR to proceed as planned.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Description
This pull request addresses a validation issue with the date fields used in activity-related requests. The following changes have been implemented:
These changes improve data quality and prevent inconsistencies caused by invalid date inputs.
Changes Made
activities.validators.js:commonValidations.datearray.commonValidations.dateto the following routes:recallActivitydeployActivitymaintainActivitybatchDeployActivityTesting
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit