feat: Add overview section to the Json report #1531

jcpitre · 2023-07-10T01:43:19Z

Summary:
Closes #1503
Added an overview section to the JSON report according to https://docs.google.com/document/d/157X9lMTkZHH7oYxK1fYPiY-rMh_VWyPMW_s9wImOP-I/edit

Expected behavior:
There should be this summary section in the JSON report:

  "summary": {
    "validatorVersion": "4.1.1-SNAPSHOT",
    "validatedAt": "2023-07-17 at 22:10:15 EDT",
    "gtfsInput": "file:///Users/jcpitre/datasets/test.zip",
    "threads": 1,
    "outputDirectory": "qaz",
    "systemErrorsReportName": "system_errors.json",
    "validationReportName": "report.json",
    "htmlReportName": "report.html",
    "countryCode": "ZZ",
    "feedInfo": {
      "publisherName": "Otwarte Dane GTFS przyjazdy.pl",
      "publisherUrl": "https://przyjazdy.pl/gtfs",
      "feedLanguage": "Polish"
    },
    "agencies": [
      {
        "name": "MKS Krosno",
        "url": "http://www.mks-krosno.pl",
        "phone": "N/A",
        "email": "N/A"
      }
    ],
    "files": [
      "agency.txt",
      "calendar.txt",
      "calendar_dates.txt",
      "feed_info.txt",
      "routes.txt",
      "stop_times.txt",
      "stops.txt",
      "trips.txt"
    ],
    "counts": {
      "Shapes": 0,
      "Stops": 514,
      "Routes": 18,
      "Trips": 659,
      "Agencies": 1,
      "Blocks": 1
    },
    "gtfsComponents": [
      "Feed Information",
      "Route Names",
      "Headsigns",
      "Wheelchair Accessibility",
      "Bikes Allowance"
    ]

Note that the summary section is not used for validation result comparison.

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 · 2023-07-12T16:41:41Z

✅ Rule acceptance tests passed.
New Errors: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1436 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1436 sources (~0 %) are corrupted.
Commit: ef7713f
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

bdferris-v2 · 2023-07-13T03:15:47Z

main/src/main/java/org/mobilitydata/gtfsvalidator/report/JsonReportSummary.java

+ * Fields names used here will be the same as in the generated json file.
+ */
+@SuppressWarnings("unused") // The fields of this class are only read by Gson when serializing.
+public class JsonReportSummary {


Hey, I had a macro comment on this class.

This class defines the exported API for the JSON report summary. As such, identifiers used here should be stable, since folks external users will be depending on this API. However, this summary depends on a number of identifiers export from FeedMetadata and AgencyMetadata, two classes that were written primarily as UI presentation model classes. As such, I think there is a risk someone might make a change to one of those UI presentation classes, not realizing it would impact the exported JSON api.

For example, the key names in feedInfo below, any of the data in AgencyMetadata, the keys in counts, and the identifiers in gtfsComponents. They'd all ideally be defined fields or enums, where it's very clear that changing a value would change the exported API.

Does that feedback make sense?

It does thanks.
For the class AgencyMetadata, would you suggest I use @SerializedName annotation with a comment explaining why it's there? Or extract the data in a Map and use defined fields for the key.

Created classes devoted to the json generation with controlled fields and copied values from the existing classes.
Also added some string constants used in the Html and Json generation code so it's obvious changing the string could have an effect in the Json report code.

Continued using gtfsComponents directly. We will have to be avoid changing the component names or document it properly.

These changes look good. Thanks!

Doesn't have to be in this PR, but long-term, I think it would be more valuable to have GTFS Components modeled as an enum internally, not a string. It might ultimately be rendered as a string, but I think a enum would be more practical for long-term programatic access.

main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/JsonReportGeneratorTest.java

main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/JsonReportSummaryTest.java

qcdyx

I used ae-dubayy-road-and-transport-authority-rta-gtfs-904.zip to generate the report. I saw that the value of agency email is N/A. Question: shall we keep it in this way? Or, remove email from Summary section since it's N/A?

ae-dubayy-road-and-transport-authority-rta-gtfs-904.zip

qcdyx · 2023-07-13T16:00:18Z

main/src/main/java/org/mobilitydata/gtfsvalidator/runner/ValidationRunner.java

@@ -229,18 +233,31 @@ public static void exportReport(
            "Error creating output directory: %s", config.outputDirectory());
      }
    }
+    SimpleDateFormat formatter = new SimpleDateFormat("yyyy-MM-dd 'at' HH:mm:ss z");


I see here you are using SimpleDateFormat. I recommend using DateTimeFormatter. DateTimeFormatter is a replacement for the old SimpleDateFormat that is thread-safe and provides additional functionality. I recommend using the new Date Time API introduced in Java 8.

This piece of code already existed and was simply moved from main/src/main/java/org/mobilitydata/gtfsvalidator/report/HtmlReportGenerator.java.
I would prefer at this point to keep it as is.

I see that the GtfsDate is already using LocalDate which is introduced with DateTimeFormatter in the new java 8 date time API. I'm ok with keeping it for now for this issue, but shall we open an issue about updating to the java 8 data time API since it's more immutable and threadsafe than the old ones? @davidgamez

I was willing to give it a go, but DateTimeFormatter had trouble interpreting the part of the pattern related to time zone.
Since like I said the date related code in this PR is actually some code that was moved and not modified, I prefer to keep it the way it is.

I see. I agree to keep it the way it is. I can open a new issue about this, maybe use ZonedDateTime for GtfsDate.

In reality I don't think you should open an issue about this.
The fact that it's not thread safe is irrelevant here because we are creating and using the formatter in the same thread. As for extra functionalities, we don't need them here.

…report

…used code.

github-actions · 2023-07-18T00:48:19Z

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 · 2023-07-18T01:35:56Z

✅ Rule acceptance tests passed.
New Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1439 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1439 sources (~0 %) are corrupted.
Commit: ada5285
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

qcdyx

LGTM

jcpitre · 2023-07-19T01:45:06Z

@isabelle-dr If you have time could you take a look at the exemple summary in the first comment of this PR: #1531 (comment)
And maybe tell us if it looks like what you expected.

Also @qcdyx mentioned this in her review #1531 (review):

I used ae-dubayy-road-and-transport-authority-rta-gtfs-904.zip to generate the report. I saw that the value of agency email is N/A. Question: shall we keep it in this way? Or, remove email from Summary section since it's N/A?

Do you have an opinion on this? The N/A is also present in the json report summary.

emmambd · 2023-07-19T13:36:12Z

@jcpitre Hello! Isabelle's off for the rest of the week so I can share comments here to unblock this. I downloaded the JAR file from this branch's Test Package Document action to test this on a few reports.

The JSON report adheres to the original design that you linked so this looks good to me!
Re: omitting or including N/As, I think this is outside the scope of this issue, since as you mentioned, it would need to be included or omitted in both report versions. We can discuss this next week with @isabelle-dr for design history/context.

Draft commit

a9e8d24

jcpitre linked an issue Jul 10, 2023 that may be closed by this pull request

Add the Overview section to the JSON Report #1503

Closed

jcpitre marked this pull request as draft July 10, 2023 02:17

jcpitre changed the title ~~Draft commit~~ Add overview section Jul 10, 2023

jcpitre added 5 commits July 10, 2023 11:38

Prevented some NPEs

b3861d0

Corrected formatting

337be38

Added JsonReportGenerator and associated classes.

1dba765

Made sure the html and json reports have the same date.

3920b3d

Protected against some nulls

3befdf9

jcpitre changed the title ~~Add overview section~~ feat: Add overview section to the Json report Jul 11, 2023

jcpitre added 4 commits July 11, 2023 23:05

Added unit tests

08d77d6

Corrected unit tests

18f2130

Corrected unit tests again

b1db1ae

Reformatted

ae77df5

MobilityData deleted a comment from github-actions bot Jul 12, 2023

jcpitre marked this pull request as ready for review July 12, 2023 16:37

jcpitre requested a review from qcdyx July 12, 2023 16:37

bdferris-v2 reviewed Jul 13, 2023

View reviewed changes

qcdyx reviewed Jul 13, 2023

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/JsonReportGeneratorTest.java Outdated Show resolved Hide resolved

qcdyx reviewed Jul 13, 2023

View reviewed changes

main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/JsonReportSummaryTest.java Outdated Show resolved Hide resolved

qcdyx reviewed Jul 13, 2023

View reviewed changes

jcpitre requested a review from emmambd July 14, 2023 13:43

jcpitre added 2 commits July 17, 2023 20:38

Merge branch 'master' into 1503-add-the-overview-section-to-the-json-…

901f442

…report

Made the json file less vulnerable to unwanted changes and removed un…

24c44d0

…used code.

jcpitre added 2 commits July 17, 2023 20:51

Reformatted code.

1c88b54

Removed unused file

9302eb7

jcpitre requested review from bdferris-v2 and qcdyx July 18, 2023 02:00

qcdyx approved these changes Jul 18, 2023

View reviewed changes

jcpitre requested review from isabelle-dr and removed request for emmambd July 19, 2023 01:40

bdferris-v2 approved these changes Jul 19, 2023

View reviewed changes

jcpitre merged commit fd6e43c into master Jul 19, 2023
333 checks passed

jcpitre deleted the 1503-add-the-overview-section-to-the-json-report branch July 19, 2023 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add overview section to the Json report #1531

feat: Add overview section to the Json report #1531

jcpitre commented Jul 10, 2023 •

edited

Loading

github-actions bot commented Jul 12, 2023

bdferris-v2 Jul 13, 2023

jcpitre Jul 13, 2023

jcpitre Jul 18, 2023

bdferris-v2 Jul 19, 2023

qcdyx left a comment •

edited

Loading

qcdyx Jul 13, 2023 •

edited

Loading

jcpitre Jul 18, 2023

qcdyx Jul 18, 2023

jcpitre Jul 25, 2023

qcdyx Jul 25, 2023

jcpitre Jul 25, 2023 •

edited

Loading

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

qcdyx left a comment

jcpitre commented Jul 19, 2023

emmambd commented Jul 19, 2023

feat: Add overview section to the Json report #1531

feat: Add overview section to the Json report #1531

Conversation

jcpitre commented Jul 10, 2023 • edited Loading

github-actions bot commented Jul 12, 2023

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

qcdyx left a comment • edited Loading

Choose a reason for hiding this comment

qcdyx Jul 13, 2023 • edited Loading

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

Choose a reason for hiding this comment

jcpitre Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2023

github-actions bot commented Jul 18, 2023

qcdyx left a comment

Choose a reason for hiding this comment

jcpitre commented Jul 19, 2023

emmambd commented Jul 19, 2023

jcpitre commented Jul 10, 2023 •

edited

Loading

qcdyx left a comment •

edited

Loading

qcdyx Jul 13, 2023 •

edited

Loading

jcpitre Jul 25, 2023 •

edited

Loading