Skip to content

feat: Web Based Validator#1317

Merged
KClough merged 60 commits intoMobilityData:masterfrom
JarvusInnovations:feat/web
Mar 31, 2023
Merged

feat: Web Based Validator#1317
KClough merged 60 commits intoMobilityData:masterfrom
JarvusInnovations:feat/web

Conversation

@KClough
Copy link
Copy Markdown
Contributor

@KClough KClough commented Jan 24, 2023 •
edited
Loading

Summary:

Resolves #1184

Expected behavior:

Explain and/or show screenshots for how you expect the pull request to work in your testing (in case other devices exhibit different behavior).

Please make sure these boxes are checked before submitting your pull request - thanks!

TODO:

  • Link to documentation hosted on Google Cloud Storage rather than Github pages
  • Support for user supplied country code
  • Review + implement Mobility Data's launch requirements

@KClough KClough marked this pull request as draft January 24, 2023 01:42
@KClough KClough mentioned this pull request Jan 24, 2023
Closed
4 tasks
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 1, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 1, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ KClough
❌ ryon
You have signed the CLA already but the status is still pending? Let us recheck it.

@KClough KClough force-pushed the feat/web branch 3 times, most recently from 732a4ba to 6b97a51 Compare March 1, 2023 22:22
@KClough KClough marked this pull request as ready for review March 9, 2023 20:09
@KClough KClough requested a review from bdferris-v2 March 9, 2023 20:59
davidgamez
davidgamez reviewed Mar 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

praise: Great work! I added few questions and comments in-line before approval.

bdferris-v2
bdferris-v2 reviewed Mar 13, 2023
View reviewed changes
bdferris-v2
bdferris-v2 reviewed Mar 13, 2023
View reviewed changes
web/client/static/fontawesome6/css/regular.css
@@ -0,0 +1,19 @@
/*!
* Font Awesome Free 6.2.1 by @fontawesome - https://fontawesome.com
Copy link
Copy Markdown
Collaborator

@bdferris-v2 bdferris-v2 Mar 13, 2023

Choose a reason for hiding this comment

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

General question: is it typical to have so much fontawesome checked directly into our own repo? I'm surprised this isn't made available as a library package of some kind.

Copy link
Copy Markdown
Contributor Author

@KClough KClough Mar 20, 2023

Choose a reason for hiding this comment

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

We looked into this, and the current deployment is what fontawesome recommends.

This has the additional potential performance benefit that the fontawesome library may already be cached by the browser.

bdferris-v2
bdferris-v2 reviewed Mar 13, 2023
View reviewed changes
web/client/src/routes/+page.svelte
data.url = url
}

const xhr = new XMLHttpRequest();
Copy link
Copy Markdown
Collaborator

@bdferris-v2 bdferris-v2 Mar 13, 2023

Choose a reason for hiding this comment

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

Are you targeting a specific browser version set? Wondering if you can get away with fetch here.

Copy link
Copy Markdown
Contributor Author

@KClough KClough Mar 14, 2023

Choose a reason for hiding this comment

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

We ran into some issues with fetch during testing. The upload request to GCS has to be configured in a very specific way to be accepted. I'd prefer to leave this as is.

emmambd
emmambd previously requested changes Mar 14, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@emmambd emmambd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Very excited about this @KClough! I've shared some comments from a QA perspective on the UI. I also tested 8 feeds from a mix of our acceptance tests and Cal-ITP's original validator review, for both ZIP and URL feeds:

  1. When I generate 1 URL feed report, it works. When I try to generate another URL feed report in the same browser session, it fails and I get an error when I click "Open Report". Tested in Chrome and Firefox. failed-report-generation-nz-wellington Here are 2 links I tested with: https://static.opendata.metlink.org.nz/v1/gtfs/full.zip and http://data.trilliumtransit.com/gtfs/victorville-ca-us/victorville-ca-us.zip

  2. stop_without_stop_time seems to be missing a details accordion on the rules page. stops_without_stop_times_missing_details_route_long_name I assume this is an issue with the web UI report, since it does appear in the RULES.md

  3. When trying to validate https://download.gtfs.de/germany/nv_free/latest.zip, the web UI stayed running for 20 minutes in the validator web UI without generating a report, whereas it loaded in 5 minutes with the desktop app. Would be curious on @davidgamez and @bdferris's perspective on handling very large aggregate feeds here too, and if there's any quick wins to improve performance.

davidgamez
davidgamez reviewed Mar 16, 2023
View reviewed changes
@briandonahue briandonahue self-assigned this Mar 20, 2023
@davidgamez davidgamez mentioned this pull request Mar 20, 2023
Closed
@bdferris-v2
Copy link
Copy Markdown
Collaborator

bdferris-v2 commented Mar 30, 2023

I think this is generally LGTM. I'm going to let @davidgamez give the ultimate review approval, since this will eventually be his free puppy to own and maintain.

@davidgamez
Copy link
Copy Markdown
Member

davidgamez commented Mar 30, 2023

I think this is generally LGTM. I'm going to let @davidgamez give the ultimate review approval, since this will eventually be his free puppy to own and maintain.

I connected with @KClough and the pending items will be addressed in follow-up PRs. We will be creating separate issues to keep the scope reduced per PR.

davidgamez
davidgamez approved these changes Mar 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

LGTM!

@davidgamez davidgamez requested a review from emmambd March 31, 2023 16:53
@KClough KClough mentioned this pull request Mar 31, 2023
Closed
@KClough KClough dismissed emmambd’s stale review March 31, 2023 20:39

Confirmed with Emma in slack that these changes have been made.

@KClough KClough merged commit 34a0de2 into MobilityData:master Mar 31, 2023
@KClough KClough deleted the feat/web branch March 31, 2023 20:49
This was referenced Mar 31, 2023
Closed
Closed
Closed
bradyhunsaker pushed a commit to bradyhunsaker/gtfs-validator that referenced this pull request Apr 25, 2023
@KClough @ryon
@briandonahue @davidgamez @bradyhunsaker
feat: Web Based Validator (MobilityData#1317)
69d4493 
* feat: add web api service

* chore: move web service to subfolder

* feat: add web client

* feat: clear form when report is ready

* feat: add footer with credits

* feat: improve button/form styling

* feat: add data retention info to rooter

* fix: don't auto-upload when using form input

* feat: add SelectField component

* feat: add language selector

* chore: reorganize imports

* fix: actually upload file when using form

* wip: add country code + url support

* chore: add start script alias

* feat: add markdown and some add'l postcss features

* feat: reorganize css

* feat: improve component event handling and data flow

* feat: update form events and country code handling

* feat: mock up sourceUrl and improve errors

* feat: add local rules page

* fix: remove incorrect danging comma from css

* fix: add |local to in-page transitions to prevent awkward nav

* fix: inject host to URLs in rules.md

* Fix country code loading/saving

* Update docs to include deploy command

* Fix Dockerfile jar reference

* Update web client to properly pass country code + url

* Update copy + add github footer

* Add rules page and update links

* Fix markdown errors

* Add feedback button to validator web client

* Add GCS lifecycle documentation

* Tweak scroll on hash change behavior

* Add build step for copying RULES.md to web client

* fix: first pass on pr feedback

* Update README's for local dev

* fix: reset jobId on each run

* fix: Move detected region to top of dropdown, make region optional

* fix: attempting to make region optional on service

* fix: spring boot tests running due to missing credential files

* feat: tweak jvm params

* feat: tweak jvm params - try 12gb for jvm

* refactor: extract StorageHelper.java

* refactor: cleanup and use JSON serialization.

* refactor: further refactoring to increase separation of concerns.

* add class & method comments

* formatting

* Use request/response object and JSON serialization

* further class documentation

* comments enjoy a good formatting, as well

* PR feedback

* PR feedback

* rename method, inline variable

* Autowire StorageHelper

* move method back onto controller

* skip directories

* refactoring

---------

Co-authored-by: Ryon Coleman <[email protected]>
Co-authored-by: Brian Donahue <[email protected]>
Co-authored-by: David Gamez Diaz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdferris-v2 bdferris-v2 bdferris-v2 left review comments

@davidgamez davidgamez davidgamez approved these changes

@emmambd emmambd Awaiting requested review from emmambd

+1 more reviewer

@briandonahue briandonahue briandonahue left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@briandonahue briandonahue

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Proposal: enable direct deployment of validator as a web service

8 participants

@KClough @CLAassistant @bdferris-v2 @davidgamez @briandonahue @emmambd @isabelle-dr @ryon