feat: support yyyy-MM-dd format for serviceDate in trip parameters#1070
Open
3rabiii wants to merge 2 commits into
Open
feat: support yyyy-MM-dd format for serviceDate in trip parameters#10703rabiii wants to merge 2 commits into
3rabiii wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesserviceDate dual-format parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/restapi/trip_details_handler.go (1)
95-109: 💤 Low valueOptional: Update comment to reflect yyyy-MM-dd behavior.
The comment explains why localization is needed for Unix millisecond parsing (preventing UTC date-extraction bugs), but doesn't mention that for yyyy-MM-dd dates, this block is a no-op since
time.ParseInLocationat line 49 already parses in the target timezone. Consider adding a note that yyyy-MM-dd dates are already localized at parse time.📝 Proposed comment update
// If a timezone location was provided, localize serviceDate and time so that // callers receive times in the agency's timezone by default. This prevents the // bug where time.Unix(ms/1000, 0) creates a UTC time and downstream // Year()/Month()/Day()/Format() calls extract the wrong calendar date for -// agencies in positive UTC offsets. +// agencies in positive UTC offsets. For yyyy-MM-dd dates, this is a no-op since +// they are already parsed in the correct timezone at line 49. if len(loc) > 0 && loc[0] != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/restapi/trip_details_handler.go` around lines 95 - 109, Update the comment block that precedes the timezone localization logic (starting around the "If a timezone location was provided" text) to clarify that while the comment explains why localization is necessary for Unix millisecond parsing to prevent UTC date-extraction bugs, yyyy-MM-dd formatted dates are already localized at parse time via the time.ParseInLocation call at line 49, making this localization block a no-op for those dates. Add a note explaining that this block primarily handles the Unix millisecond case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/restapi/trip_details_handler.go`:
- Around line 95-109: Update the comment block that precedes the timezone
localization logic (starting around the "If a timezone location was provided"
text) to clarify that while the comment explains why localization is necessary
for Unix millisecond parsing to prevent UTC date-extraction bugs, yyyy-MM-dd
formatted dates are already localized at parse time via the time.ParseInLocation
call at line 49, making this localization block a no-op for those dates. Add a
note explaining that this block primarily handles the Unix millisecond case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 510b6872-fb77-4504-935d-c5d02c813ae9
📒 Files selected for processing (3)
internal/restapi/trip_details_handler.gointernal/restapi/trip_details_handler_test.gointernal/restapi/trip_for_vehicle_handler_test.go
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses by enhancing the shared
parseTripParamsfunction to natively support calendar dates (yyyy-MM-dd) for theserviceDateparameter, conforming to the wiki spec.Changes Included:
parseTripParamsto first attempt parsing theserviceDateas Unix milliseconds. If that fails, it falls back to parsing the string as ayyyy-MM-ddcalendar date.time.ParseInLocationwith the provided agency timezone (loc). This ensures "midnight" is calculated correctly relative to the agency's local time, preventing UTC-offset date-shift bugs. Defaults to UTC safely if no location is provided."must be a valid Unix timestamp in milliseconds or a date in yyyy-MM-dd format".TestParseTripIdDetailsParams_Unit/serviceDate yyyy-MM-dd formatto verify accurate year/month/day extraction.TestTripDetailsHandlerWithServiceDateStringto integration test a full handler call with the calendar string format.Fixes: #1054
Summary by CodeRabbit
yyyy-MM-ddformat, in addition to Unix timestamps in milliseconds. Calendar dates are interpreted using the agency’s local timezone.TripDetailsHandlercorrectly acceptsserviceDate=YYYY-MM-DDand returns the expected service date value.serviceDateformats.