Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions admin_live_handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,22 @@ func handleListTrips(store TripLister) http.HandlerFunc {
return
}

// Absent user_id means "all drivers". A present one must be a real
// users.id, so 0 and negatives are rejected rather than silently
// collapsing into the no-filter sentinel.
var userID int64
if raw := q.Get("user_id"); raw != "" {
userID, err = strconv.ParseInt(raw, 10, 64)
if err != nil || userID < 1 {
writeJSON(w, http.StatusBadRequest, map[string]string{"error": "user_id must be a positive integer"})
return
}
}

filter := TripFilter{
Status: status,
VehicleID: q.Get("vehicle_id"),
UserID: userID,
Q: q.Get("q"),
// Fetch one extra row to detect whether results were truncated at limit.
Limit: limit + 1,
Expand Down Expand Up @@ -195,6 +208,40 @@ func handleListTrips(store TripLister) http.HandlerFunc {
}
}

// handleGetTrip returns a single trip's summary for the admin trip detail
// view. It is the trail-free counterpart to handleTripLocations, so callers
// that only need trip metadata do not pay for up to 10k location points. A
// non-numeric or unknown {id} both produce 404, since neither identifies a
// real trip.
func handleGetTrip(store TripSummaryGetter) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
id, err := strconv.ParseInt(r.PathValue("id"), 10, 64)
if err != nil {
writeJSON(w, http.StatusNotFound, map[string]string{"error": "trip not found"})
return
}

trip, err := store.GetTripSummary(r.Context(), id)
if err != nil {
if errors.Is(err, ErrTripNotFound) {
writeJSON(w, http.StatusNotFound, map[string]string{"error": "trip not found"})
return
}
slog.Error("failed to get trip summary", "trip_id", id, "error", err)
writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "internal server error"})
return
}
if trip == nil {
// Defensive: a well-behaved store returns ErrTripNotFound rather
// than (nil, nil), but guard against it to avoid a nil dereference.
writeJSON(w, http.StatusNotFound, map[string]string{"error": "trip not found"})
return
}

writeJSON(w, http.StatusOK, trip)
}
}

// handleTripLocations returns a single trip's summary joined with its
// location trail, for the admin trip detail/map view. A non-numeric or
// unknown {id} both produce 404, since neither identifies a real trip.
Expand Down
136 changes: 136 additions & 0 deletions admin_live_handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -462,3 +462,139 @@ func TestHandleTripLocations_LocationsStoreError(t *testing.T) {
handleTripLocations(fake).ServeHTTP(w, req)
assert.Equal(t, http.StatusInternalServerError, w.Code)
}

// TestHandleListTrips_UserIDFilterPassthrough verifies user_id is parsed and
// forwarded to the store as TripFilter.UserID.
func TestHandleListTrips_UserIDFilterPassthrough(t *testing.T) {
fake := &fakeTripLister{}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips?user_id=7", nil)
w := httptest.NewRecorder()
handleListTrips(fake).ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)

assert.Equal(t, int64(7), fake.captured.UserID)
}

// TestHandleListTrips_NoUserIDMeansAllDrivers verifies an absent user_id
// leaves the filter at 0, which ListTrips treats as "no driver filter".
func TestHandleListTrips_NoUserIDMeansAllDrivers(t *testing.T) {
fake := &fakeTripLister{}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips", nil)
w := httptest.NewRecorder()
handleListTrips(fake).ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)

assert.Zero(t, fake.captured.UserID)
}

// TestHandleListTrips_InvalidUserID verifies a present but unusable user_id is
// rejected rather than silently collapsing into the 0 "all drivers" sentinel,
// which would quietly return every trip instead of the caller's filter.
func TestHandleListTrips_InvalidUserID(t *testing.T) {
for _, userID := range []string{"0", "-1", "abc", "1.5", ""} {
t.Run("user_id="+userID, func(t *testing.T) {
fake := &fakeTripLister{}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips?user_id="+userID, nil)
w := httptest.NewRecorder()
handleListTrips(fake).ServeHTTP(w, req)

if userID == "" {
// An empty value is indistinguishable from an absent param.
assert.Equal(t, http.StatusOK, w.Code)
return
}
assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Zero(t, fake.captured.UserID, "store must not be filtered on a rejected user_id")
})
}
}

// TestHandleGetTrip_HappyPath verifies the trip summary is returned unwrapped,
// including the vehicle label and driver name joins.
func TestHandleGetTrip_HappyPath(t *testing.T) {
end := time.Unix(1752570000, 0).UTC()
trip := &TripSummary{
ID: 5, VehicleID: "bus-1", VehicleLabel: "Bus 1",
UserID: 7, DriverName: "Asha", RouteID: "route-9",
GtfsTripID: "gtfs-3", StartTime: time.Unix(1752566400, 0).UTC(),
EndTime: &end, Status: "completed",
}
fake := &fakeTripTrailStore{trip: trip}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/5", nil)
req.SetPathValue("id", "5")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)

var got TripSummary
require.NoError(t, json.NewDecoder(w.Body).Decode(&got))
assert.Equal(t, int64(5), got.ID)
assert.Equal(t, "Bus 1", got.VehicleLabel)
assert.Equal(t, "Asha", got.DriverName)
assert.Equal(t, int64(7), got.UserID)
require.NotNil(t, got.EndTime)
assert.Equal(t, end, got.EndTime.UTC())
}

// TestHandleGetTrip_ActiveTripOmitsEndTime verifies an active trip's null
// end_time round-trips as an absent field rather than a zero timestamp.
func TestHandleGetTrip_ActiveTripOmitsEndTime(t *testing.T) {
fake := &fakeTripTrailStore{trip: &TripSummary{ID: 5, Status: "active"}}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/5", nil)
req.SetPathValue("id", "5")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
require.Equal(t, http.StatusOK, w.Code)

var raw map[string]any
require.NoError(t, json.NewDecoder(w.Body).Decode(&raw))
assert.NotContains(t, raw, "end_time")
}

// TestHandleGetTrip_NotFound verifies an unknown id produces a 404.
func TestHandleGetTrip_NotFound(t *testing.T) {
fake := &fakeTripTrailStore{tripErr: ErrTripNotFound}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/999", nil)
req.SetPathValue("id", "999")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
assert.Equal(t, http.StatusNotFound, w.Code)
}

// TestHandleGetTrip_NonNumericID verifies a non-numeric {id} produces a 404,
// matching handleTripLocations rather than leaking a parse error.
func TestHandleGetTrip_NonNumericID(t *testing.T) {
fake := &fakeTripTrailStore{}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/not-a-number", nil)
req.SetPathValue("id", "not-a-number")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
assert.Equal(t, http.StatusNotFound, w.Code)
}

// TestHandleGetTrip_NilTripWithoutError verifies the defensive nil guard
// returns 404 instead of panicking on a (nil, nil) store result.
func TestHandleGetTrip_NilTripWithoutError(t *testing.T) {
fake := &fakeTripTrailStore{}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/5", nil)
req.SetPathValue("id", "5")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
assert.Equal(t, http.StatusNotFound, w.Code)
}

// TestHandleGetTrip_StoreError verifies a store failure produces a 500 and
// does not leak the underlying error to the client.
func TestHandleGetTrip_StoreError(t *testing.T) {
fake := &fakeTripTrailStore{tripErr: assert.AnError}
req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/trips/5", nil)
req.SetPathValue("id", "5")
w := httptest.NewRecorder()
handleGetTrip(fake).ServeHTTP(w, req)
require.Equal(t, http.StatusInternalServerError, w.Code)

var resp map[string]string
require.NoError(t, json.NewDecoder(w.Body).Decode(&resp))
assert.Equal(t, "internal server error", resp["error"])
assert.NotContains(t, resp["error"], assert.AnError.Error())
}
1 change: 1 addition & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ func newMux(store appStore, tracker *Tracker, rateLimiter *VehicleRateLimiter, j
mux.Handle("DELETE /api/v1/admin/vehicles/{id}", authMiddleware(adminMiddleware(handleDeactivateVehicle(store))))
mux.Handle("GET /api/v1/admin/vehicles/{vehicleID}/locations", authMiddleware(adminMiddleware(handleGetLocationHistory(store, store))))
mux.Handle("GET /api/v1/admin/trips", authMiddleware(adminMiddleware(handleListTrips(store))))
mux.Handle("GET /api/v1/admin/trips/{id}", authMiddleware(adminMiddleware(handleGetTrip(store))))
mux.Handle("GET /api/v1/admin/trips/{id}/locations", authMiddleware(adminMiddleware(handleTripLocations(store))))
mux.HandleFunc("GET /health", func(w http.ResponseWriter, r *http.Request) {
writeJSON(w, http.StatusOK, map[string]string{"status": "ok"})
Expand Down
2 changes: 2 additions & 0 deletions route_wiring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ func TestAdminRoutes_DriverTokenRejected(t *testing.T) {
{"DELETE", "/api/v1/admin/vehicles/bus-1"},
{"GET", "/api/v1/admin/vehicles/bus-1/locations"},
{"GET", "/api/v1/admin/trips"},
{"GET", "/api/v1/admin/trips/1"},
{"GET", "/api/v1/admin/trips/1/locations"},
{"GET", "/api/v1/admin/users"},
{"GET", "/api/v1/admin/users/1"},
Expand Down Expand Up @@ -200,6 +201,7 @@ func TestAdminRoutes_AdminTokenAllowed(t *testing.T) {
{"DELETE", "/api/v1/admin/vehicles/bus-1"},
{"GET", "/api/v1/admin/vehicles/bus-1/locations"},
{"GET", "/api/v1/admin/trips"},
{"GET", "/api/v1/admin/trips/1"},
{"GET", "/api/v1/admin/trips/1/locations"},
{"GET", "/api/v1/admin/users"},
{"GET", "/api/v1/admin/users/1"},
Expand Down
4 changes: 4 additions & 0 deletions store_trips.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ type TripSummary struct {
type TripFilter struct {
Status string // "", "active", "completed"
VehicleID string // "" = all
UserID int64 // 0 = all; users.id is a bigserial, so 0 is never a real driver
Q string // ILIKE substring on driver name, route_id, gtfs_trip_id
Limit int // callers pass limit+1 to detect hasMore
Offset int
Expand Down Expand Up @@ -205,6 +206,9 @@ func (s *Store) ListTrips(ctx context.Context, f TripFilter) ([]TripSummary, err
if f.VehicleID != "" {
conds = append(conds, "t.vehicle_id = "+arg(f.VehicleID))
}
if f.UserID != 0 {
conds = append(conds, "t.user_id = "+arg(f.UserID))
}
if f.Q != "" {
// Escape LIKE metacharacters so a search for a literal % or _
// (common in GTFS ids) matches the literal text instead of acting
Expand Down
48 changes: 48 additions & 0 deletions store_trips_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,54 @@ func TestListTripsFiltersAndOrder(t *testing.T) {
assert.NotEqual(t, page1[0].ID, page2[0].ID)
}

// TestListTripsUserIDFilter covers the user_id filter: it must narrow results
// to one driver, compose with the other filters, and treat 0 as "all drivers"
// so the zero value cannot silently hide rows.
func TestListTripsUserIDFilter(t *testing.T) {
store := newTestStore(t)
clearTripTestData(t, store)
t.Cleanup(func() { clearTripTestData(t, store) })
ctx := context.Background()

driver1 := setupTripUser(t, store, "Cara Driver", "cara-trips@test.com", "bus-user-1", "Bus User 1")
trip1, err := store.StartTrip(ctx, driver1, "bus-user-1", "route-1", "gtfs-1")
require.NoError(t, err)
require.NoError(t, store.EndTrip(ctx, trip1.ID, driver1))

driver2 := setupTripUser(t, store, "Dan Driver", "dan-trips@test.com", "bus-user-2", "Bus User 2")
_, err = store.StartTrip(ctx, driver2, "bus-user-2", "route-2", "gtfs-2")
require.NoError(t, err)

byUser, err := store.ListTrips(ctx, TripFilter{UserID: driver1, Limit: 200})
require.NoError(t, err)
require.NotEmpty(t, byUser)
for _, tr := range byUser {
assert.Equal(t, driver1, tr.UserID)
}

// Composes with status: driver1's only trip is completed, so filtering
// for their active trips must come back empty rather than falling back
// to an unfiltered list.
activeForUser1, err := store.ListTrips(ctx, TripFilter{UserID: driver1, Status: "active", Limit: 200})
require.NoError(t, err)
assert.Empty(t, activeForUser1)

activeForUser2, err := store.ListTrips(ctx, TripFilter{UserID: driver2, Status: "active", Limit: 200})
require.NoError(t, err)
require.Len(t, activeForUser2, 1)
assert.Equal(t, driver2, activeForUser2[0].UserID)

// A user with no trips at all yields an empty result, not everyone's.
absent, err := store.ListTrips(ctx, TripFilter{UserID: driver2 + 100_000, Limit: 200})
require.NoError(t, err)
assert.Empty(t, absent)

// Zero means "no driver filter": both drivers' trips come back.
all, err := store.ListTrips(ctx, TripFilter{UserID: 0, Limit: 200})
require.NoError(t, err)
assert.GreaterOrEqual(t, len(all), 2)
}

func TestGetTripSummary(t *testing.T) {
store := newTestStore(t)
userID := setupTripTestData(t, store)
Expand Down
Loading