Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add MobilityData Validator #916

Merged
merged 30 commits into from
Mar 29, 2023
Merged

Add MobilityData Validator #916

merged 30 commits into from
Mar 29, 2023

Conversation

miles-grant-ibigroup
Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup commented Feb 24, 2023 •
edited
Loading

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
  • The description lists all relevant PRs included in this release (remove this if not merging to master)
  • e2e tests are all passing (remove this if not merging to master)

Description

Adds support for viewing MobilityData validation results. Also adds a new script for importing the rules.md file from the repo into the codebase.

miles-grant-ibigroup
miles-grant-ibigroup commented Mar 9, 2023
View reviewed changes
lib/manager/components/validation/MobilityDataValidationResult.js
if (!rule) return null

const errorClass = `gtfs-error-${mobilityDataValidationErrorMapping[notice.severity]}`
const [expanded, setExpanded] = useState(notice.totalNotices < 2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this...

Copy link
Contributor

Choose a reason for hiding this comment

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

What are you not sure about? If 2 is a good number for when to show the notices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and if we should expand by default at all?

@miles-grant-ibigroup miles-grant-ibigroup marked this pull request as ready for review March 9, 2023 16:32
@philip-cline
Copy link
Contributor

Is this ready for review? I'm getting white screening on selecting a MobilityDataValidation item

@miles-grant-ibigroup
Copy link
Contributor Author

Yes it is! Could you please share which feed this is happening with? Could be a certain feed/rule that's causing the issue.

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Mar 10, 2023
@miles-grant-ibigroup
Copy link
Contributor Author

I've implemented one of a few possible solutions to your problem. Let me know what you think!

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Mar 22, 2023
@miles-grant-ibigroup
Copy link
Contributor Author

Thanks Phil all changes are addressed

philip-cline
philip-cline approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@philip-cline philip-cline left a comment

Choose a reason for hiding this comment

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

That's looking good to me. I'll leave it to the second reviewer to comment on adjusting the width of the table columns.

@philip-cline
Copy link
Contributor

philip-cline commented Mar 23, 2023 •
edited
Loading

Jumping back in here to say that if there's a parsing error the table will expand because of the content:
image

(This is the MBTA's GTFS btw)
Also, when I run the validator locally I don't get this CSV parsing error so seems like this might be something happening on the backend.

@miles-grant-ibigroup
Copy link
Contributor Author

Yes this is a timing issue on the sever. Could you possibly open a ticket there @philip-cline

@miles-grant-ibigroup
Copy link
Contributor Author

The speed issue is resolved by ibi-group/datatools-server#525

daniel-heppner-ibigroup
daniel-heppner-ibigroup approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

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

Only a few nits, other than that this is some of the best code I've ever seen. Thanks!

lib/manager/components/validation/MobilityDataValidationResult.js
if (!rule) return null

const errorClass = `gtfs-error-${mobilityDataValidationErrorMapping[notice.severity]}`
const [expanded, setExpanded] = useState(notice.totalNotices < 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

What are you not sure about? If 2 is a good number for when to show the notices?

lib/manager/components/validation/MobilityDataValidationResult.js

const MobilityDataValidationResult = (props) => {
const { notice } = props
const rule = rules.find((rd) => rd.rule === notice.code)
Copy link
Contributor

Choose a reason for hiding this comment

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

would indexOf be more efficient here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be but I think it would be dirtier code wouldn't it

lib/manager/components/validation/MobilityDataValidationResult.js Outdated Show resolved Hide resolved
lib/manager/components/validation/MobilityDataValidationResult.js Outdated Show resolved Hide resolved
lib/manager/components/version/FeedVersionViewer.js Show resolved Hide resolved
scripts/import-mobility-data-rules.mjs Show resolved Hide resolved
@philip-cline
Copy link
Contributor

Only a few nits, other than that this is some of the best code I've ever seen. Thanks!

Somebody show this guy the exception date range editor 😮‍💨

@miles-grant-ibigroup
Copy link
Contributor Author

Thanks @daniel-heppner-ibigroup! All changes are addressed

@miles-grant-ibigroup miles-grant-ibigroup merged commit 2156be9 into dev Mar 29, 2023
@miles-grant-ibigroup miles-grant-ibigroup deleted the mobility-data-validator branch March 29, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philip-cline philip-cline philip-cline approved these changes

@daniel-heppner-ibigroup daniel-heppner-ibigroup daniel-heppner-ibigroup approved these changes

Assignees

@miles-grant-ibigroup miles-grant-ibigroup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miles-grant-ibigroup @philip-cline @daniel-heppner-ibigroup