Add includeTrip parameter to trips-for-route - #1217
Conversation
The trips-for-route handler previously conflated the includeTrip parameter with includeSchedule. It passed includeSchedule to the buildTripReferences function as the trip-beans flag. This caused includeTrip=false to have no effect, and includeSchedule=false to wrongly drop trip beans from the references block. This commit parses includeTrip from the query parameters with a default of true, and passes it independently to buildTripReferences. Both the empty-list and main response paths are updated. Table-driven tests are added to verify all inclusion combinations and guard against future regressions.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe trips-for-route handler now reads ChangesTrip reference inclusion
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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/trips_for_route_handler.go`:
- Line 32: Replace the direct includeTrip string comparison in the route handler
with the shared parseIncludeTrip logic used by the location handler, preserving
its handling of false, zero, case variants, and malformed values. Add regression
coverage for invalid and case-variant includeTrip query values.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a531cfc-3a82-4e00-815c-312c106ae16f
📒 Files selected for processing (2)
internal/restapi/trips_for_route_handler.gointernal/restapi/trips_for_route_handler_test.go
Addresses automated review feedback to use the shared parseIncludeTrip function, which correctly handles case-variants and numeric boolean values (e.g., 'FALSE', '0') unlike the naive string comparison. Added corresponding test cases.
|
@3rabiii this has merge conflicts |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Approved on the merits — this is a good catch and a clean fix.
The bug is a genuinely sneaky one: buildTripReferences's third parameter is already named includeTrip and gates references.trips, but the handler was passing includeSchedule into it. So includeSchedule=false was silently also emptying the trip references, and includeTrip did nothing at all. Two conflated flags in one argument position is exactly the kind of thing that survives review by looking correct at the call site.
I checked the behavior against the spec and it lines up:
includeTripdefaults totruewhen the parameter is omitted, which is what the spec requires.- With
includeTrip=false,references.tripsis empty butdata.list[].tripIdis still populated — that's spec extension 5a verbatim ("the trip ID is still present in the list entry but no detail is available in references"). Your test asserts both halves, which is the part that matters. - Reusing the existing
parseIncludeTriphelper instead of hand-rolling a string comparison was the right call.
The table-driven test covering omitted / true / false / FALSE / 0 plus an includeSchedule=false regression guard is more thorough than I'd have asked for.
One thing before this can land: it has merge conflicts. Since you opened it, main wrapped both buildTripReferences call sites in an if includeReferences { ... } block. The conflict is purely textual — the same one-argument swap re-applies cleanly on the other side of that guard — so no re-review needed once it's rebased. Please merge main in and resolve, and I'll merge it.
One non-blocking observation for whenever you or someone else is next in this area: parseIncludeTrip returns false for a present-but-malformed value (includeTrip=yes, or an empty includeTrip=), whereas the analogous ShouldIncludeReferences falls back to the documented default on a parse failure. Since the documented default here is true, malformed input currently drops all trip references silently. That's pre-existing behavior in the shared helper and reusing it was correct, so I don't want it in this PR — just worth knowing it's there.
# Conflicts: # internal/restapi/trips_for_route_handler.go
|
3 participants
Summary
Implements the
includeTripquery parameter for thetrips-for-routeendpoint. Previously, the handler conflatedincludeTripwithincludeSchedule, passing the wrong flag tobuildTripReferences. This causedincludeTrip=falseto be silently ignored andincludeSchedule=falseto incorrectly drop trip beans fromdata.references.trips.Changes
internal/restapi/trips_for_route_handler.go:includeTripparsing alongside the existingincludeScheduleandincludeStatusparameters (defaults totrue).buildTripReferencescalls in both the early empty-list return path and the main response path to useincludeTripinstead ofincludeSchedule.internal/restapi/trips_for_route_handler_test.go:TestTripsForRouteHandler_TripInclusionwith four cases: default inclusion, explicit inclusion, explicit exclusion, and a regression guard (excluding schedule but keeping trips).Closes: #1208
Summary by CodeRabbit
New Features
includeTripoption for controlling trip references in trips-for-route responses.Bug Fixes