Skip to content

feat(itinerary): add ability to sort itineraries by fare #1411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 23, 2025
Merged
75 changes: 74 additions & 1 deletion __tests__/util/state.js
Original file line number Diff line number Diff line change
@@ -4,7 +4,8 @@ import '../test-utils/mock-window-url'
import {
addToSearches,
isValidSubsequence,
queryIsValid
queryIsValid,
sortItineraries
} from '../../lib/util/state'

describe('util > state', () => {
@@ -99,4 +100,76 @@ describe('util > state', () => {
expect(addToSearches(entries, spaceNeedle)).toEqual(tidiedEntries)
})
})
describe('sortItineraries', () => {
const makeItin = (transitFare) => ({ transitFare })

const itineraries = [
makeItin(100),
makeItin(200),
makeItin(undefined),
makeItin(50),
makeItin(null),
makeItin(0)
]

it('sorts by FARE ascending (undefined/null last)', () => {
const sorted = [...itineraries].sort((a, b) =>
sortItineraries('FARE', 'ASC', a, b)
)
// Fares: 0, 50, 100, 200, undefined, null
expect(sorted.map((i) => i.transitFare)).toEqual([
0,
50,
100,
200,
undefined,
null
])
})

it('sorts by FARE descending (undefined/null last)', () => {
const sorted = [...itineraries].sort((a, b) =>
sortItineraries('FARE', 'DESC', a, b)
)
// Fares: 200, 100, 50, 0, undefined, null
expect(sorted.map((i) => i.transitFare)).toEqual([
200,
100,
50,
0,
undefined,
null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These look good, but can you also add a $0 fare to make sure that it doesn't get accidentally lumped in with null/undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 52e6b1e

])
})

it('sorts undefined vs defined correctly', () => {
const a = makeItin(undefined)
const b = makeItin(100)
expect(sortItineraries('FARE', 'ASC', a, b)).toBe(1)
expect(sortItineraries('FARE', 'ASC', b, a)).toBe(-1)
expect(sortItineraries('FARE', 'DESC', a, b)).toBe(1)
expect(sortItineraries('FARE', 'DESC', b, a)).toBe(-1)
})

it('sorts null vs defined correctly', () => {
const a = makeItin(null)
const b = makeItin(100)
expect(sortItineraries('FARE', 'ASC', a, b)).toBe(1)
expect(sortItineraries('FARE', 'ASC', b, a)).toBe(-1)
})

it('sorts undefined vs null as equal', () => {
const a = makeItin(undefined)
const b = makeItin(null)
expect(sortItineraries('FARE', 'ASC', a, b)).toBe(0)
expect(sortItineraries('FARE', 'DESC', a, b)).toBe(0)
})

it('sorts 0 vs other fares correctly', () => {
const a = makeItin(0)
const b = makeItin(50)
expect(sortItineraries('FARE', 'ASC', a, b)).toBeLessThan(0)
expect(sortItineraries('FARE', 'DESC', a, b)).toBeGreaterThan(0)
})
})
})
1 change: 1 addition & 0 deletions i18n/en-US.yml
Original file line number Diff line number Diff line change
@@ -377,6 +377,7 @@ components:
selectCost: Cost
selectDepartureTime: Departure time
selectDuration: Duration
selectFare: Transit fare
selectWalkTime: Walk time
sortResults: Sort results
viewAll: View all options
1 change: 1 addition & 0 deletions i18n/fr.yml
Original file line number Diff line number Diff line change
@@ -386,6 +386,7 @@ components:
selectCost: Prix
selectDepartureTime: Heure de départ
selectDuration: Durée
selectFare: Prix du trajet
selectWalkTime: Temps de marche
sortResults: Trier les résultats
viewAll: Toutes les options
9 changes: 8 additions & 1 deletion lib/components/util/sortOptions.ts
Original file line number Diff line number Diff line change
@@ -12,7 +12,8 @@ export const sortOptions = (
'ARRIVALTIME',
'WALKTIME',
'COST',
'DEPARTURETIME'
'DEPARTURETIME',
'FARE'
]
): SortOptionEntry[] => {
const sortOptionsArray: SortOptionEntry[] = [
@@ -51,6 +52,12 @@ export const sortOptions = (
id: 'components.NarrativeItinerariesHeader.selectCost'
}),
value: 'COST'
},
{
text: intl.formatMessage({
id: 'components.NarrativeItinerariesHeader.selectFare'
}),
value: 'FARE'
}
]

1 change: 1 addition & 0 deletions lib/util/config-types.ts
Original file line number Diff line number Diff line change
@@ -264,6 +264,7 @@ export type ItinerarySortOption =
| 'WALKTIME'
| 'COST'
| 'DEPARTURETIME'
| 'FARE'

export interface ItineraryCostWeights {
driveReluctance: number
5 changes: 4 additions & 1 deletion lib/util/itinerary.tsx
Original file line number Diff line number Diff line change
@@ -48,6 +48,7 @@ export interface ItineraryWithCO2Info extends Itinerary {
export interface ItineraryWithSortingCosts extends Itinerary {
rank: number
totalFare: number
transitFare?: number
}

export interface ItineraryFareSummary {
@@ -453,10 +454,12 @@ export function addSortingCosts<T extends Itinerary>(
totalFareResult === null ? Number.MAX_VALUE : totalFareResult

const rank = calculateItineraryCost(itinerary, config)
const transitFare = getFare(itinerary).transitFare
return {
...itinerary,
rank,
totalFare
totalFare,
transitFare
}
}

13 changes: 13 additions & 0 deletions lib/util/state.js
Original file line number Diff line number Diff line change
@@ -33,6 +33,17 @@ export function getActiveSearch(state) {
return state.otp.searches[state.otp.activeSearchId]
}

function compareItineraryFare(a, b, direction = 'ASC') {
const aMissing = a === undefined || a === null
const bMissing = b === undefined || b === null
// Always sort missing values (null/undefined) to the end of the array.
if (aMissing && bMissing) return 0
if (aMissing) return 1
if (bMissing) return -1
// If both fares are defined, compare them according to direction
return direction === 'ASC' ? a - b : b - a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahah so the old version didn't work? Good thing you wrote the test!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old version didn't handle the direction of undefined/null values. Good thinking on a unit test!

}

/**
* Array sort function for itineraries (in batch routing context) that attempts
* to sort based on the type/direction specified.
@@ -50,6 +61,8 @@ export function sortItineraries(type, direction, a, b, config) {
return dirFactor * (a.duration - b.duration)
case 'COST':
return dirFactor * (a.totalFare - b.totalFare)
case 'FARE':
return compareItineraryFare(a.transitFare, b.transitFare, direction)
default:
if (type !== 'BEST')
console.warn(`Sort (${type}) not supported. Defaulting to BEST.`)

Unchanged files with check annotations Beta

import { Field, Form, Formik } from 'formik'
import React, {Component} from 'react'

Check failure on line 2 in lib/components/admin/editable-section.js

GitHub Actions / test-build-release

Replace `Component` with `·Component·`
import {
Button,
import { createAction } from 'redux-actions'
if (typeof (fetch) === 'undefined') require('isomorphic-fetch')

Check failure on line 2 in lib/actions/zipcar.js

GitHub Actions / test-build-release

Replace `(fetch)` with `fetch`
export const receivedZipcarLocationsError = createAction('ZIPCAR_LOCATIONS_ERROR')

Check failure on line 4 in lib/actions/zipcar.js

GitHub Actions / test-build-release

Replace `'ZIPCAR_LOCATIONS_ERROR'` with `⏎··'ZIPCAR_LOCATIONS_ERROR'⏎`
export const receivedZipcarLocationsResponse = createAction('ZIPCAR_LOCATIONS_RESPONSE')

Check failure on line 5 in lib/actions/zipcar.js

GitHub Actions / test-build-release

Replace `'ZIPCAR_LOCATIONS_RESPONSE'` with `⏎··'ZIPCAR_LOCATIONS_RESPONSE'⏎`
export const requestZipcarLocationsResponse = createAction('ZIPCAR_LOCATIONS_REQUEST')

Check failure on line 6 in lib/actions/zipcar.js

GitHub Actions / test-build-release

Replace `'ZIPCAR_LOCATIONS_REQUEST'` with `⏎··'ZIPCAR_LOCATIONS_REQUEST'⏎`
export function zipcarLocationsQuery (url) {

Check failure on line 8 in lib/actions/zipcar.js

GitHub Actions / test-build-release

Delete `·`
return async function (dispatch, getState) {
dispatch(requestZipcarLocationsResponse())
let json
import { replace, push } from 'connected-react-router'

Check failure on line 1 in lib/actions/auth.js

GitHub Actions / test-build-release

Member 'push' of the import declaration should be sorted alphabetically
import { setPathBeforeSignIn } from '../actions/user'
* @param {Error} err
* @param {AccessTokenRequestOptions} options
*/
export function showAccessTokenError (err, options) {

Check failure on line 11 in lib/actions/auth.js

GitHub Actions / test-build-release

Delete `·`
return function (dispatch, getState) {
// TODO: improve this.
console.error('Failed to retrieve access token: ', err)
* when signing-in fails for some reason.
* @param {Error} err
*/
export function showLoginError (err) {

Check failure on line 23 in lib/actions/auth.js

GitHub Actions / test-build-release

Delete `·`
return function (dispatch, getState) {
// TODO: improve this.
if (err) dispatch(push('/oops'))
* @param {Object} appState The state stored when calling useAuth0().loginWithRedirect
* or when instantiating a component that uses withAuhenticationRequired.
*/
export function processSignIn (appState) {

Check failure on line 36 in lib/actions/auth.js

GitHub Actions / test-build-release

Delete `·`
return function (dispatch, getState) {
if (appState && appState.returnTo) {
// Remove URL parameters that were added by auth0-react