Repository navigation
fix: preserve stop alerts when location arrivals are empty - #1574
SurajSingh563 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary fix: preserving stop alerts when location arrivals are empty. Description check The description explains the problem, root cause, implementation, testing, response behavior, and scope. It uses alternate section headings and does not reproduce every repository checklist item, but … Linked Issues check The description references issue #1565, and the implementation directly addresses its stated requirement to preserve stop-level alerts when no arrivals exist.Out of Scope Changes check The changes are limited to the batched arrivals logic and its regression test. They match the stated objective and do not include unrelated behavior changes.
- Fix all pre-merge checks with AI
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Looks good. Checked against Java Two optional notes: the manual loop at the end of the new test could be Separately (not for this PR): Java also adds route alerts for scheduled routes at the end of that method even with no arrivals; worth checking whether Maglev's batched path does the same. |
|
Thanks for the review @Ahmedhossamdev and for checking it against the Java implementation.
|
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Thanks @SurajSingh563, the fix itself looks right to me. Collecting alerts for every stop in in.Stops matches Java, which adds each found stop's alerts inside its per-stop loop before anything is trimmed and whether or not the stop has arrivals (StopWithArrivalsAndDeparturesBeanServiceImpl.java#L112-L136). I also confirmed the new test fails against main's arrivals_core.go and passes here.
There's one thing to fix before this can merge. Commit 546cb42 has a Co-authored-by: Copilot trailer, and CONTRIBUTING asks that commits not be attributed to coding agents. That commit's body is also the first sentence split across a blank line ("…contains no" / "stop times, …"). Please reword that commit to remove the trailer and give it a proper body, then force-push. Rewriting history is fine here because removing the trailer requires it.
- #1557 also edits this loop, but it scopes stop alerts to the stops left after trimming, and that doesn't match Java. Your version is the one that does.
arrivalsForStops returned early when no stop had a stop time in the window, before stop-level alerts were collected, so quiet location searches lost their situation references. Collect stop alerts for every found stop before returning, matching Java. Fixes OneBusAway#1565.
45b2deb to
308f031
Compare
|
|
Thanks, @aaronbrethorst . Done: Reworded the commits to remove the Co-authored-by trailer, with a proper body on the first commit. Force-pushed. hope you will re-review thanks a lot |
3 participants
Collect stop alerts on the batched path even when its window contains no
stop times, so quiet location searches keep their situation references.
Fixes #1565
Summary
arrivalsForStopsreturned early when no stop in the area had a stop time in the window, before stop-level alerts were collected. Late-night or no-service queries returned no stop alerts.Changes
internal/restapi/arrivals_core.go: extracted the stop-alert loop intoaddStopAlertsForStopsand call it on both thelen(allActive) == 0early return and the normal return. Each path calls it exactly once.internal/restapi/arrivals_and_departures_for_location_handler_test.go: addedTestArrivalsAndDeparturesForLocationIncludesStopAlertsWithoutArrivals.Size: ~56 lines changed (52 additions, 4 deletions)
Understanding
Root cause: The
len(allActive) == 0early return sat above the loop that callsGetAlertsForStop. Alerts were only collected when at least one stop had an arrival, so a quiet area lost its alerts.Alternatives considered: Moving the loop above the early return would also work. I extracted a helper and called it from both paths so the normal path keeps its existing order (alerts are still collected last) and the early return stays a small change.
Riskiest part of this change: The two call sites in
arrivalsForStops. If both ever ran for the same request, alerts would be added twice, but the early return exits before the second call. Look there first if duplicate situations appear.Testing
situationIdsand resolves inreferences.situations.go test -tags "sqlite_fts5 sqlite_math_functions" ./internal/restapi -run 'TestArrivalsAndDeparturesForLocation(IncludesStopAlertsWithoutArrivals|EndToEnd|RouteTypeFiltersArrivalsNotStops)$' -count=1arrivalsForStopshas a single production caller (the location arrivals handler), so no other call sites are affected.Response comparison
Region / production server: None. Maglev only; I have no Java OBA server to compare against.
GTFS static feed(s): Maglev test fixture (testdata)
GTFS-RT feed(s): Injected test stop alert on stop 4062
Endpoint(s) exercised: arrivals-and-departures-for-location, quiet window (03:00, minutesBefore=0, minutesAfter=0)
Trimmed to the relevant fields.
Maglev before this PR (main):
Response
{ "data.entry.arrivalsAndDepartures": [], "data.entry.situationIds": [], "data.references.situations": [] }Maglev after this PR:
Response
{ "data.entry.arrivalsAndDepartures": [], "data.entry.situationIds": [ "25_location-no-arrivals-stop-alert" ], "data.references.situations": [ { "id": "25_location-no-arrivals-stop-alert", "creationTime": 0, "activeWindows": [], "allAffects": [ { "agencyId": "", "applicationId": "", "directionId": "UNSPECIFIED", "routeId": "", "stopId": "4062", "tripId": "" } ], "consequenceMessage": "", "consequences": [], "publicationWindows": [], "reason": "UNKNOWN_CAUSE", "severity": "noImpact", "summary": { "value": "Stop alert without arrivals", "lang": "en" } } ] }Author checklist
go vet -tags "sqlite_fts5 sqlite_math_functions" ./...passesgo vet -tags "purego" ./...passesmake testpassesgo fmt ./...was run and its changes committedgtfsdb/(generated by sqlc)r.Context()to DB/service calls, notcontext.Background()Summary by CodeRabbit