Skip to content

Conversation

@philip-cline
Copy link
Contributor

Closes #716, #717. New fields added with the changes to the GTFS spec (#668) are missing from old feeds when a GTFS entity is saved, producing a field missing from JSON object error. Fixed by providing default values which are overridden if the feed provides its own values for the new fields.

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Getting in the right direction. The remaining gap is to initialize the feed email and URL fields in UI. See also other comments.

Comment on lines +22 to +24
import {saveTripPattern} from './tripPattern'
import {newGtfsEntity, fetchBaseGtfs} from './editor'

Copy link
Contributor

Choose a reason for hiding this comment

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

Can these imports be before the import type or is this something prettier automatically format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an automatic format

Comment on lines +341 to +342
defaults.feed_contact_url = ''
defaults.feed_contact_email = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

These fields make it to the database, but on page reload are not populated in the UI. Can you fix this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a problem with the fetchBaseGTFS method, which did not fetch the new fields. Fixed in c48bb18

trips = trips.map(snakeCaseKeys)
return Promise.all(trips.filter(t => t).map((trip, index) => {
const tripExists = !entityIsNew(trip) && trip.id !== null
const tripCopy: any = (trip: any)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would clone it (otherwise you are improperly overwrite data).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloned in c48bb18

return Promise.all(trips.filter(t => t).map((trip, index) => {
const tripExists = !entityIsNew(trip) && trip.id !== null
const tripCopy: any = (trip: any)
// Add default value to continuous pickup if not provided
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment saying editing continuous pickup/drop off is not currently supported in the schedule editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in c48bb18

Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working on this.

@binh-dam-ibigroup
Copy link
Contributor

Passing this on to @miles-grant-ibigroup for review

Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Great patch! Well done finding that error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants