Update GtfsRealtime and include severity, effect and cause from GTFS RT by optionsome · Pull Request #3747 · opentripplanner/OpenTripPlanner

optionsome · 2021-11-15T16:20:29Z

Summary

Updates the generated GtfsRealtime file (this had to be done so severity level information from GTFS RT datas could be included)
Includes severity, cause and effect from GTFS RT updater to transit alerts
- Refactored severity from a string to enum and updated its use in SIRI/transmodel
Implements alerts for the LegacyGraphQL API
Fixes an issue where agency selector was attempted to be added even though there was none
Changes missing start in active_period to be returned as null from effective start instead of 0

Issue

closes #3653

Unit tests

Updated tests related to SIRI and added tests for GTFS RT alerts.

Code style

Used the intellij formatting rules

Documentation

Not needed?

Changelog

From title and updated legacyGraphQL changelog

optionsome

I need clarification for the commented things before I'll finish this pr.

optionsome · 2021-11-16T08:58:14Z

-  // TODO
+  // This is deprecated
  @Override
  public DataFetcher<Iterable<TripPattern>> patterns() {


Not sure if this should be fixed or not

optionsome · 2021-11-16T08:59:15Z

+            return AlertSeverity.WARNING;
+        }
+        switch (severity) {
+            case PTI_26_255:


Is it ok to map these values into what seems to be their human readable versions?

As a general principle, I don't think OTP should change any of the data provided in any service.
However, all the PTI_-values have been removed in the upcoming version of SIRI, so I am not sure if these values are actually needed anymore.

Currently, the transmodel enum for the severity doesn't contain those so I'm not sure what would happen now if someone tried to use a SIRI updater that used the PTI_-values together with the transmodel API. We could alternatively just map these PTI_-values to the values known by the transmodel API in that side instead of when loading in the severities to the internal data model. But as you pointed out about the upcoming change, I'm not sure if that is worth the effort here.

optionsome · 2021-11-16T09:02:54Z

+/**
+ * An enum containing cause options for an alert.
+ */
+public enum AlertCause {


Is creating these duplicate enums for the internal model wanted?

We decided that it's better to use duplicate enums for the internal data models instead of using enum directly from the GTFS RT library.

optionsome · 2021-11-16T09:05:43Z

@@ -67,7 +67,7 @@ type Alert implements Node {
    stop: Stop


These stop, route, agency and trip fields need to be replaced with stops etc because alerts can have multiple entities but I'm not sure what is the ideal way. Should the stops etc field be at the root level of the alert of under some entities field in some way.

We discussed this in today's meeting. @hannesj suggested that we should implement the entities similarly as the places in the nearest query so you need to use fragments to handle the different entity types.

Implemented in 1ee1514

leonardehrenfried · 2021-11-16T10:15:12Z

-    return environment -> null;
+    return environment -> {
+      var text = getSource(environment).alertDescriptionText;
+      return text instanceof TranslatedString


This is duplicated code. Can you use a reusable method?

Indeed. I'll do that.

I changed that now in 973c9f3

optionsome · 2021-11-18T12:48:19Z

This is ready for review now. I'll also do some final checks with different kinds of alerts data in the meanwhile. One difference I noticed at least when compared against the hsldevcom fork of OTP1 is that the default values for the translated fields where in Finnish in OTP1 but in English in OTP2. I'm not sure what causes the difference but maybe it should be possible to add an option to configure the default language if that is not already possible somehow.

lassetyr

This looks good to me.

Based on tests created by evansiroky

optionsome · 2021-11-23T11:12:26Z

Found some issues while testing and fixed those. e7964d8 was an existing issue when only route_type entity was defined, it tried to add agency selector with empty string which lead to an error. 60706bc makes the start time behave like the end time so if they are not set, null is returned.

I plan to add support for the route_type selector (and probably also trip direction) in a separate pull request and also allow alerts with no known selectors to be included (now they are not added if they have no selectors known by OTP).

t2gran

@lassetyr has reviewed and approved this from Entur, and I quickly looked trough code - so I will approve this on behalf of Entur.

optionsome added 9 commits November 12, 2021 16:05

Generate GtfsRealtime.java based on updated gtfs-realtime.proto

77c500b

Refactor transit alert's severity

a227ff2

Initial implementation of legacyGraphQL alerts query

3d175f0

Set severity from GTFS RT updater to alerts

6319ca1

Add cause and effect from GTFS RT and use it in legacyGraphQL

82ac221

Implement alerts query param handling and add missing params

98b7165

Implement missing translated alert fields

6c344b1

Implement alertHash

2f4ccd4

Deprecate patterns field for alerts

bdd5d66

optionsome requested a review from a team as a code owner November 15, 2021 16:20

optionsome commented Nov 16, 2021

View reviewed changes

leonardehrenfried reviewed Nov 16, 2021

View reviewed changes

t2gran added this to the 2.1 milestone Nov 16, 2021

t2gran requested a review from lassetyr November 16, 2021 11:26

optionsome marked this pull request as draft November 16, 2021 11:48

optionsome changed the title ~~WIP: Update GtfsRealtime and include severity, effect and cause from GTFS RT~~ Update GtfsRealtime and include severity, effect and cause from GTFS RT Nov 16, 2021

optionsome added 3 commits November 18, 2021 14:06

Create new entities field to replace entity specific alert fields

1ee1514

Create method for getting translations

973c9f3

Add comments

199d1dc

optionsome marked this pull request as ready for review November 18, 2021 12:45

lassetyr previously approved these changes Nov 18, 2021

View reviewed changes

optionsome added 4 commits November 19, 2021 17:06

Fix NPE

76a03c3

Don't initialize empty agencyId

e7964d8

Return effective startDate for alerts as null if not set

60706bc

Add tests for alertsUpdateHandler

77babde

Based on tests created by evansiroky

optionsome dismissed lassetyr’s stale review via 77babde November 23, 2021 10:56

optionsome added 2 commits November 23, 2021 13:05

Update legacy graphql changelog

2cb50f0

Format code

f4ea9a5

Merge remote-tracking branch 'upstream/dev-2.x' into alert-updates

ff319bc

t2gran previously approved these changes Nov 23, 2021

View reviewed changes

leonardehrenfried previously approved these changes Nov 30, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/dev-2.x' into alert-updates

6b72182

optionsome dismissed stale reviews from leonardehrenfried and t2gran via 6b72182 November 30, 2021 12:34

leonardehrenfried approved these changes Nov 30, 2021

View reviewed changes

t2gran approved these changes Nov 30, 2021

View reviewed changes

optionsome merged commit 919f892 into opentripplanner:dev-2.x Nov 30, 2021

optionsome deleted the alert-updates branch November 30, 2021 14:05

t2gran pushed a commit that referenced this pull request Nov 30, 2021

Add changelog entry for #3747 [ci skip]

b96b0ab

Conversation

optionsome commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Code style

Documentation

Changelog

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Nov 18, 2021

Uh oh!

lassetyr left a comment

Choose a reason for hiding this comment

Uh oh!

optionsome commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

optionsome commented Nov 15, 2021 •

edited

Loading

optionsome commented Nov 23, 2021 •

edited

Loading