fix(trip-details): omit status key when tracking is absent#1100
fix(trip-details): omit status key when tracking is absent#11003rabiii wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTrip-details now omits ChangesStatus omission fix and coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/restapi/trip_details_handler_test.go`:
- Around line 199-201: The current assertion using
`assert.Nil(model.Data.Entry.Status)` only checks if the unmarshaled Status
field is nil, but this will incorrectly pass when the JSON response contains
`"status": null`. For Extension 4e compliance, verify that the "status" key is
completely absent from the raw JSON response at the map level (the decoded
payload before unmarshaling into the model struct), not just that the field is
nil after unmarshaling. Modify the assertion to check the raw map representation
to confirm the "status" key does not exist in data.entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a58cec5-8ea9-4202-9503-5ae752d85c26
📒 Files selected for processing (3)
internal/restapi/servicedate_timezone_regression_test.gointernal/restapi/trip_details_handler.gointernal/restapi/trip_details_handler_test.go
Extracts the inline condition for untracked vehicles into a dedicated TripStatus method per PR review. This improves handler readability and includes tests to verify the logic.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/models/trip_details.go (1)
69-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the
"default"sentinel into a shared constant.
IsUntracked()couples to the placeholder marker via a magic string literal that must also be set verbatim inBuildTripStatus(per PR description). If either side's literal drifts (typo, refactor), the omission logic silently breaks andstatusreappears in responses without any compile-time signal.♻️ Proposed refactor
+const defaultTripStatusValue = "default" + +func (ts *TripStatus) IsUntracked() bool { + return ts.Status == defaultTripStatusValue && !ts.Predicted +} -func (ts *TripStatus) IsUntracked() bool { - return ts.Status == "default" && !ts.Predicted -}And use
defaultTripStatusValuewhereverBuildTripStatussets the placeholderStatus.🤖 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/models/trip_details.go` around lines 69 - 74, Extract the "default" placeholder value into a shared constant and use it in both TripStatus.IsUntracked and BuildTripStatus so the sentinel stays consistent. Update IsUntracked() to compare against that constant instead of a magic string, and make BuildTripStatus assign the same shared value when creating the placeholder status.
🤖 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/models/trip_details.go`:
- Around line 69-74: Extract the "default" placeholder value into a shared
constant and use it in both TripStatus.IsUntracked and BuildTripStatus so the
sentinel stays consistent. Update IsUntracked() to compare against that constant
instead of a magic string, and make BuildTripStatus assign the same shared value
when creating the placeholder status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c135262b-6061-4647-949f-f61f25f98089
📒 Files selected for processing (4)
internal/models/trip_details.gointernal/models/trip_details_test.gointernal/restapi/trip_details_handler.gointernal/restapi/trip_details_handler_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/restapi/trip_details_handler.go
- internal/restapi/trip_details_handler_test.go
Replaces the magic string 'default' with a shared constant to prevent silent breakage from typos across IsUntracked and BuildTripStatus.
|
2 participants
Description
This PR addresses Extension 4e of the
trip-detailsspecification, which requires thestatuskey to be entirely absent from the response if the block has no current tracking record.Previously,
BuildTripStatusreturned a default non-nil placeholder (Status="default",Predicted=false) when no vehicle was tracked, which prevented theomitemptyJSON tag from dropping the field.Changes made:
statuspointer tonilintripDetailsHandlerwhen the returned status is just the default placeholder.TestTripDetailsHandlerStatusOmittedWhenNoTrackingto explicitly verify compliance with Extension 4e.TestServiceDateTimezoneRegression_BlockTripSequenceto inject a mock vehicle, ensuring the timezone calculation still has valid tracking data to test against.Closes: #1086
Summary by CodeRabbit
statusfield entirely when no vehicle tracking data is available (instead of returning an untracked placeholder).statusin the raw JSON response when tracking is missing, and added unit tests for the untracked status detection logic.