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

feat: Web Based Validator #1317

Merged
merged 60 commits into from
Mar 31, 2023
Merged

feat: Web Based Validator #1317

merged 60 commits into from
Mar 31, 2023

Conversation

KClough
Copy link
Collaborator

@KClough KClough commented Jan 24, 2023 •
edited

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!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

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

CLAassistant commented Feb 1, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

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
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.

web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Body.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/CreateJobBody.java Outdated Show resolved Hide resolved
web/service/src/main/java/org/mobilitydata/gtfsvalidator/web/service/Main.java Show resolved Hide resolved
web/service/src/test/java/com/mobilitydata/gtfsvalidator/web/service/MainTests.java Show resolved Hide resolved
bdferris-v2
bdferris-v2 reviewed Mar 13, 2023
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
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
Collaborator

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
Collaborator Author

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
Collaborator

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
Collaborator Author

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
Contributor

@emmambd emmambd left a comment •
edited

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.

web/client/src/routes/+layout.svelte Outdated Show resolved Hide resolved
web/client/src/routes/+page.svelte Outdated Show resolved Hide resolved
web/client/src/lib/countryCodes.js Show resolved Hide resolved
@briandonahue briandonahue self-assigned this Mar 20, 2023
@davidgamez davidgamez mentioned this pull request Mar 20, 2023
Closed
@bdferris-v2
Copy link
Collaborator

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
Member

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
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 <ryon55@gmail.com>
Co-authored-by: Brian Donahue <brian.donahue@vitreosolutions.com>
Co-authored-by: David Gamez Diaz <1192523+davidgamez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandonahue briandonahue briandonahue left review comments

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

@davidgamez davidgamez davidgamez approved these changes

@emmambd emmambd Awaiting requested review from emmambd

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