feat: Implements the HTML output MVP #1127

maximearmstrong · 2022-04-28T00:49:12Z

Summary:

Fixes #1126: Implement HTML output for validation report (MVP)

This PR implements HtmlOutputUtil.java, script.js and style.css to generate a HTML version of the validation report.

Expected behavior:

The HTML report output, defaulted to report.html is located in the output folder. It is a standalone HTML file (all dependencies are included in it). It can be viewed in a local browser by opening the file.

Example:

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)

github-actions · 2022-04-28T00:49:57Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

github-actions · 2022-04-28T01:13:04Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 92a5887 here (report will disappear after 90 days).

github-actions · 2022-04-28T01:51:36Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 2ff6b65 here (report will disappear after 90 days).

github-actions · 2022-04-28T22:32:18Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit a6da3b7 here (report will disappear after 90 days).

github-actions · 2022-04-28T22:50:03Z

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

…va [acceptance test skip]

barbeau

Thanks for putting this together @maximearmstrong! HTML file is definitely an improvement over just the JSON. Some thoughts below.

main/build.gradle

docs/USAGE.md

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

barbeau · 2022-04-29T19:39:15Z

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

+      Scanner scanner = new Scanner(stream).useDelimiter("\\A");
+      content = scanner.hasNext() ? scanner.next() : "";
+    } catch (IOException e) {
+      System.out.println("The resource was not found at \"" + resourcePath + "\".");


All print statement in catch blocks that are errors should be output to System.err instead of System.out.

Also, I think the main result of this would be the HTML file would be malformed/broken? If so you should probably add that to the output message, otherwise the user might not tie these two things together.

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

barbeau · 2022-04-29T20:19:58Z

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

+  }
+
+  protected static String noticeColumnsBuilder(ArrayList<String> noticeFields) {
+    StringBuilder columns = new StringBuilder("                ");


The current HTML representation is definitely an improvement over digging through raw JSON, but I think the workflow for the HTML report could be improved further. Currently it's still very difficult to track down what an error actually means.

Right now, we show the user the list of errors/warnings:

The natural question is "what does this error mean?"

So I would click on NOTICES.md and RULES.md and search for the code like equal_shape_distance_diff_coordinates to find more info.

However, if I search RULES.md, I don't get any matches because the rules are listed by notice class name and not the error code.

If I search NOTICES.md, I do get a hit in a list of notice codes mapped to notice class names:

...but that's not very helpful by itself. If I click on the notice class name I'm taken to an anchor lower on the same NOTICES.md page that describes what the fields within that notice mean:

https://github.com/MobilityData/gtfs-validator/blob/master/docs/NOTICES.md#EqualShapeDistanceDiffCoordinatesNotice

But that still doesn't describe what the actual error means. If I click on the notice class name linked in this anchor THEN I get taken over to RULES.md for the description of the error that I was originally looking for:

https://github.com/MobilityData/gtfs-validator/blob/master/RULES.md#EqualShapeDistanceDiffCoordinatesNotice

IMHO in the HTML report itself we should link directly to these error descriptions so the user has a single click to find out what the error means. We did this in the GTFS Realtime Validator HTML so the user is directly linked to, for example:
https://github.com/MobilityData/gtfs-realtime-validator/blob/master/RULES.md#e050---timestamp-is-in-the-future

...when clicking on that error in the report.

Maybe a simple approach is just to add the error code to RULES.md for now, maybe as section anchors in the markdown if that's possible so you can link directly to them?

That's what we did with RT validator - see https://raw.githubusercontent.com/MobilityData/gtfs-realtime-validator/master/RULES.md and anchors like <a name="E003"/>.

Likely beyond the scope of this issue, but I think we should also move the field descriptions in NOTICES.md into RULES.md, possibly as collapsed text that you can expand if you want the field details.

@barbeau this is a great piece of feed-back.
We were thinking of having this PR merged as a first step and then implementing (at least) the two additional parts you flagged here (mentioned in the 3.1.0 release plan issue):

A description of the notice (what is this problem?)

Where exactly is the issue and how to fix it (How to fix my dataset?)

Before implementing these parts, we would like to (1) gather user feedback & do some user testing on different options for the next version of the HTML output, (2) discuss the best strategy to integrate the info from RULES.md and NOTICES.md into the results (knowing that we will want to translate).

Maybe a simple approach is just to add the error code to RULES.md for now, maybe as section anchors in the markdown if that's possible so you can link directly to them?

I really like this idea and this would be actionable for the next release, and I'm willing to PR this one. What about merging RULES.md and NOTICES.md in one document, using the collapsed text to make it more digestible?

What about merging RULES.md and NOTICES.md in one document, using the collapsed text to make it more digestible?

👍

✅ Done in PR #1132, and the anchor links have been added in the HTML output

barbeau · 2022-04-29T20:25:51Z

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

+    return columns.toString();
+  }
+
+  protected static String noticeRowsBuilder(


Maybe it's just me, but I find it hard to read some of the data that's dumped into tables now, like this:

Creating human-readable sentences would obviously be an improvement - is that outside of the scope of this PR?

I was thinking similar to what we have on the GTFS Realtime Validator:

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java

bdferris-v2 · 2022-05-02T16:08:12Z

I wanted to link to the comment I made on the feature request issue about the technical approach here, since I accidentally posted it there instead of here on the PR. Curious if @maximearmstrong or @barbeau have opinions?

barbeau · 2022-05-02T16:41:07Z

@bdferris-v2 ThymeLeaf (https://www.thymeleaf.org/) looks really nice - looks like there is an IntelliJ plugin too. I agree that having a framework that allows us to manage the HTML as HTML content (as opposed to strings within a Java class) would greatly reduce the amount of effort to maintain and enhance the HTML output. The internationalization feature also looks like it would be very useful.

maximearmstrong · 2022-05-02T17:07:54Z

@bdferris-v2 I also agree. I will implement it.

maximearmstrong · 2022-05-12T22:47:48Z

When I try to run this PR in IntelliJ with -i tma-tn-us.zip -o output -c us and tma-tn-us.zip (after merging master into this PR branch, to see what it will look like after merge), I'm getting this error at runtime and there is no output from the validator:

Oh, the problem is that we set the Validator-Version attribute in main/build.gradle to be in the manifest stored in the JAR file. We could use the Implementation-Version attribute instead, which solves the problem in IntelliJ, but the version displayed in the report would be v3.0-cli instead of v3.0. What do you think would be better?

After discussion with @bdferris-v2, we decided to use the Implementation-Version attribute for now, as it might be refactored and fixed along with issue #1139. Running the validator in IntelliJ now works, however the HTML output obtained this way does not display the Implementation-Version that we would get using the JAR file. Since the HTML output obtained with the JAR file is correct, I think we are ready to merge.

github-actions · 2022-05-12T23:15:26Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 2c6019d here (report will disappear after 90 days).

barbeau

Thanks @maximearmstrong, looks good! Two comments in-line. Note there are conflicts now that need to be resolved too.

barbeau · 2022-05-13T16:18:36Z

README.md

@@ -33,6 +34,12 @@ You can run this validator using a GTFS dataset on your computer, or from a URL.

 More detailed instructions with all the parameters that exists are available on our ["Usage"](/docs/USAGE.md) page.

+### <a name="visualize-results"></a>Visualize the results


Nit - You don't need the anchor here, you can just link to #visualize-the-results below

Interesting! I thought it wouldn't work because we have two "Visualize the results" titles, but the first one is properly referenced. We don't need it in this case, but do you know a way to reference the second one using the same method?

main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/ReportSummary.java

barbeau · 2022-05-13T16:38:31Z

@maximearmstrong Also, do you want to open a separate issue for human-readable sentences in the HTML report? That was discussed in this PR but my understanding is it's getting tabled for future work.

bdferris-v2 · 2022-05-13T18:04:38Z

(Apologies in advance for the merge conflicts, @maximearmstrong!)

maximearmstrong · 2022-05-16T17:46:54Z

No worries @bdferris-v2! I merged the master branch. I had to change the behavior around the argument table in report.html because we no longer have access to raw arguments - a ValidatorRunnerConfig object is passed to the ValidatorRunner, not an Arguments.

I removed the exportNoticeSchema and help arguments because we don't keep them in the config object - I don't think this is a problem since they weren't really relevant. The input and URL arguments are now both represented by config.gtfsSource, so I had to check whether the URI scheme was related to a file or a URL to display the correct one. The countryCode is not displayed when it is unknown because we don't have access to the raw data.

I think our original idea of displaying the raw arguments was probably better, but it looks like we would have to break the ValidatorRunner design to make this possible. Any thoughts? @barbeau @isabelle-dr Any concerns on your end?

bdferris-v2 · 2022-05-16T17:54:19Z

I understand the motivation to display the raw arguments, but I think given the ValidationRunner refactor, there is value in just displaying the contents of the config object, as you have done, as that ultimately controls the execution. As such, I wouldn't be opposed if you got rid of the idea of command-line arguments in the html report entirely and just described them as the validation configuration. That'll probably scale better to other execution environments for the validator in the future.

If you really wanted to record the raw cli arguments for debugging purposes, I would add an Optional<List> rawArguments() field to the config where you can optionally pass those in if it makes sense for the execution environment.

maximearmstrong · 2022-05-16T18:03:36Z

Thank you @bdferris-v2, that's helpful. I thought about changing the argument table to a configuration table and changing the "Argument Description" and "Argument Short Name" columns to "Related Argument" and "Related Argument Short Name". @isabelle-dr Do you think it would be relevant for the end user?

isabelle-dr · 2022-05-16T21:49:04Z

For the arguments, my only concern is consistency between (1) the table in USAGE.md and (2) the way the params are written in the "Advanced" section in PR #1146.
If a user ticks a box that says "number of threads for execution" on the GUI and then sees "threads" and "-t" in the HTML report, this is slightly confusing (although pretty straight forward to guess).

I think from a user perspective, the best would be to have the same short descriptions than in the GUI, and remove the argument short name. There is a link to the USAGE.md file if users want to map this with the exact params used in the cli. The short descriptions could be:

Input directory
Output directory
Number of threads used to run the validator
Country code (for phone validation)
Validation report name
System errors report name
HTML report name

maximearmstrong · 2022-05-16T22:30:16Z

@isabelle-dr I agree. I made the changes. I used "GTFS input (ZIP file, directory or URL)" instead of "Input directory". This would be the final version:

github-actions · 2022-05-16T23:03:23Z

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 7951a8a here (report will disappear after 90 days).

isabelle-dr · 2022-05-17T12:51:04Z

This looks great @maximearmstrong. Thanks for adding the sentence in the value for country code.
One last tiny change 😃: can you replace Argument description by Parameter description? We use the term Parameter above and in the documentation.
After this, ready to merge from my perspective, I'm approving now. great work!

maximearmstrong added 3 commits April 27, 2022 15:12

Adds jQuery dependency to gradle file

210d731

Implements HtmlOutputResources.java and HtmlOutputUtil.java

e22fe09

Implements argument for html report filename

863c795

maximearmstrong requested a review from isabelle-dr April 28, 2022 00:49

maximearmstrong added 2 commits April 27, 2022 21:15

Fixes imports for Google Java Format

e57b18e

Fixes blank line in HtmlOutputResources.java

8868ca3

isabelle-dr added this to the v3.1.0 - Result UI + Validator as Packaged Executable milestone Apr 28, 2022

maximearmstrong added 4 commits April 28, 2022 17:41

Adds resources and refactor HtmlOutputUtil.java

54eee5b

Adds --html_report_name documentation

a26e4f0

Modifies USAGE.md

7a58915

Adds tests for --html_report_name argument

ff0b0b4

Adds tests HtmlOutputUtil.java [acceptance test skip]

f639514

maximearmstrong added 2 commits April 28, 2022 18:51

Adds tests HtmlOutputUtil.java [acceptance test skip]

37bc076

Adds tests in HtmlOutputUtilTest.java and refactors HtmlOutputUtil.ja…

542751f

…va [acceptance test skip]

maximearmstrong marked this pull request as ready for review April 29, 2022 00:05

maximearmstrong requested a review from barbeau April 29, 2022 00:05

maximearmstrong added this to In Review in The Tech Dashboard (archived) via automation Apr 29, 2022

maximearmstrong requested a review from bdferris-v2 April 29, 2022 15:59

barbeau reviewed Apr 29, 2022

View reviewed changes

main/src/main/java/org/mobilitydata/gtfsvalidator/util/HtmlOutputUtil.java Outdated Show resolved Hide resolved

maximearmstrong added 2 commits May 2, 2022 19:09

Adds Thymeleaf [acceptance test skip]

8d3bf24

Resolve conflicts [acceptance test skip]

687c78d

maximearmstrong requested review from bdferris-v2 and isabelle-dr May 12, 2022 22:48

maximearmstrong mentioned this pull request May 12, 2022

Refactor handling of project version numbers and tags #1139

Closed

maximearmstrong self-assigned this May 13, 2022

barbeau reviewed May 13, 2022

View reviewed changes

maximearmstrong added 3 commits May 16, 2022 11:38

Modifies README.md [acceptance test skip]

a455d52

Modifies ReportSummary.java [acceptance test skip]

6991bbf

Merges master and fix report.html post review [acceptance test skip]

bb6f24e

maximearmstrong mentioned this pull request May 16, 2022

Implement human-readable sentences in the HTML report #1148

Open

maximearmstrong added 3 commits May 16, 2022 18:07

Removes notice names in HTML output [acceptance test skip]

7701744

Removes pretty print argument in HTML output [acceptance test skip]

06b533e

Modifies arguments in HTML output

808ee47

isabelle-dr approved these changes May 17, 2022

View reviewed changes

The Tech Dashboard (archived) automation moved this from Pending approval to Approved May 17, 2022

Modifies report.html [acceptance test skip]

d9f31c0

maximearmstrong merged commit 57813dd into master May 17, 2022

maximearmstrong deleted the issue/1126/html-output-mvp branch May 17, 2022 14:28

The Tech Dashboard (archived) automation moved this from Approved to Done May 17, 2022

maximearmstrong mentioned this pull request May 17, 2022

feat: Initial support for validation app GUI #1146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implements the HTML output MVP #1127

feat: Implements the HTML output MVP #1127

maximearmstrong commented Apr 28, 2022 •

edited

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

barbeau left a comment

barbeau Apr 29, 2022

barbeau Apr 29, 2022

barbeau Apr 29, 2022

isabelle-dr May 2, 2022 •

edited

barbeau May 2, 2022

isabelle-dr May 4, 2022

barbeau Apr 29, 2022

bdferris-v2 commented May 2, 2022

barbeau commented May 2, 2022

maximearmstrong commented May 2, 2022

maximearmstrong commented May 12, 2022

github-actions bot commented May 12, 2022

barbeau left a comment

barbeau May 13, 2022

maximearmstrong May 16, 2022

barbeau commented May 13, 2022

bdferris-v2 commented May 13, 2022

maximearmstrong commented May 16, 2022

bdferris-v2 commented May 16, 2022

maximearmstrong commented May 16, 2022 •

edited

isabelle-dr commented May 16, 2022

maximearmstrong commented May 16, 2022

github-actions bot commented May 16, 2022

isabelle-dr commented May 17, 2022

		@@ -33,6 +34,12 @@ You can run this validator using a GTFS dataset on your computer, or from a URL.

		More detailed instructions with all the parameters that exists are available on our ["Usage"](/docs/USAGE.md) page.

		### <a name="visualize-results"></a>Visualize the results

feat: Implements the HTML output MVP #1127

feat: Implements the HTML output MVP #1127

Conversation

maximearmstrong commented Apr 28, 2022 • edited

github-actions bot commented Apr 28, 2022

github-actions bot commented Apr 28, 2022

Information about source corruption

Acceptance test details

github-actions bot commented Apr 28, 2022

Information about source corruption

Acceptance test details

github-actions bot commented Apr 28, 2022

Information about source corruption

Acceptance test details

github-actions bot commented Apr 28, 2022

barbeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelle-dr May 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdferris-v2 commented May 2, 2022

barbeau commented May 2, 2022

maximearmstrong commented May 2, 2022

maximearmstrong commented May 12, 2022

github-actions bot commented May 12, 2022

Information about source corruption

Acceptance test details

barbeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbeau commented May 13, 2022

bdferris-v2 commented May 13, 2022

maximearmstrong commented May 16, 2022

bdferris-v2 commented May 16, 2022

maximearmstrong commented May 16, 2022 • edited

isabelle-dr commented May 16, 2022

maximearmstrong commented May 16, 2022

github-actions bot commented May 16, 2022

Information about source corruption

Acceptance test details

isabelle-dr commented May 17, 2022

maximearmstrong commented Apr 28, 2022 •

edited

isabelle-dr May 2, 2022 •

edited

maximearmstrong commented May 16, 2022 •

edited