New vectortile layer for Digitransit realtime stops #5743

sharhio · 2024-03-12T11:01:38Z

Summary

A new Digitransit vectortile layer for stops which also includes realtime alert information.

Issue

The new layer allows showing information related to alerts affecting stops on the map. Alert information is only fetched when the map is zoomed in, hence the requirement for a new layer. The implementation follows the same structure as VehicleRental layers. The realtime stop mapper uses the code from the previously existing stop layer for the parts that are identical between the two layers.

Unit tests

Unit test for alerts added to StopsLayerTest.
Manual testing was also performed.

codecov · 2024-03-12T11:08:27Z

Codecov Report

Attention: Patch coverage is 80.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 67.78%. Comparing base (51a381e) to head (e6c31e6).
Report is 114 commits behind head on dev-2.x.

Files	Patch %	Lines
...xt/vectortiles/layers/stops/StopsLayerBuilder.java	0.00%	5 Missing ⚠️
...entripplanner/routing/alertpatch/TransitAlert.java	25.00%	0 Missing and 3 partials ⚠️
...es/layers/stops/DigitransitStopPropertyMapper.java	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             dev-2.x    #5743    +/-   ##
===========================================
  Coverage      67.78%   67.78%            
- Complexity     16478    16527    +49     
===========================================
  Files           1901     1905     +4     
  Lines          72132    72293   +161     
  Branches        7430     7441    +11     
===========================================
+ Hits           48892    49007   +115     
- Misses         20727    20765    +38     
- Partials        2513     2521     +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

optionsome · 2024-03-12T12:05:35Z

src/ext-test/java/org/opentripplanner/ext/vectortiles/layers/stops/StopsLayerTest.java


  @BeforeEach
-  public void setUp() {
+  public final void setUp() {


It's not really necessary to mark methods as final especially if you don't plan to extend the class.

optionsome · 2024-03-12T12:16:33Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+  protected static DigitransitRealtimeStopPropertyMapper create(
+    TransitService transitService,
+    Locale locale
+  ) {
+    return new DigitransitRealtimeStopPropertyMapper(transitService, locale);
+  }


Do we need this or could you just use the normal constructor in tests as well?

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

optionsome · 2024-03-12T12:21:57Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+      new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())),
+      new KeyValue(
+        "parentStation",
+        stop.getParentStation() != null ? stop.getParentStation().getId() : "null"


I think this "null" is a mistake we made in the old layer, you can just return null

optionsome · 2024-03-12T12:28:21Z

src/ext/java/org/opentripplanner/ext/vectortiles/layers/stops/StopsBaseLayerBuilder.java

+import org.opentripplanner.inspector.vector.LayerParameters;
+import org.opentripplanner.transit.service.TransitService;
+
+public class StopsBaseLayerBuilder extends StopsLayerBuilder {


Is this class necessary? Could we just edit the existing StopsLayerBuilder since this is not really overriding anything.

leonardehrenfried · 2024-03-13T10:12:01Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+        alert.effect().equals(AlertEffect.NO_SERVICE) &&
+        (
+          alert.getEffectiveStartDate() != null &&
+          alert.getEffectiveStartDate().isBefore(currentTime)
+        ) &&
+        (alert.getEffectiveEndDate() != null && alert.getEffectiveEndDate().isAfter(currentTime))


Can you move this into a method on TransitAlert? Maybe call it noServiceOn(instant).

I also fixed the logic a little, as I realised that the end time doesn't need to be defined for the alert to be effective, it could be open ended.

leonardehrenfried · 2024-03-13T10:21:29Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+
+  @Override
+  protected Collection<KeyValue> map(RegularStop stop) {
+    Instant currentTime = ZonedDateTime.now(transitService.getTimeZone()).toInstant();


Suggested change

Instant currentTime = ZonedDateTime.now(transitService.getTimeZone()).toInstant();

Instant currentTime = Instant.now();

No need for the detour via the ZonedDateTime.

Really? I thought the time zone might be important in this context to ensure the alert is actually valid according to local time.

If you call toInstant all timezone information is lost again. An instant is just a number of seconds since 1970-01-01 UTC.

This instant can be converted to many time zones but it remains the same number of seconds.

And since we only want to give an answer at the time of someone asking and not the future/past the time zone doesn't matter.

Haha, you're right. Somehow tricked myself into thinking the instant is different in different places. 😂 Thanks!

leonardehrenfried · 2024-03-13T10:30:31Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+      new KeyValue("gtfsId", stop.getId().toString()),
+      new KeyValue("name", i18NStringMapper.mapNonnullToApi(stop.getName())),
+      new KeyValue("code", stop.getCode()),
+      new KeyValue("platform", stop.getPlatformCode()),
+      new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())),


Can you extract a method for these KeyValue instances that are shared between the two mappers?

You can define two lists and use ListUtils.combine to combine the two lists.

leonardehrenfried · 2024-03-13T10:31:12Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+      ),
+      new KeyValue("type", getType(transitService, stop)),
+      new KeyValue("routes", getRoutes(transitService, stop)),
+      new KeyValue("noServiceAlert", noServiceAlert)


If you want to keep the boolean, you could use a property name like "closedByServiceAlert".

leonardehrenfried · 2024-03-13T13:26:44Z

src/main/java/org/opentripplanner/routing/alertpatch/TransitAlert.java

+   * @param instant
+   * @return
+   */
+  public boolean noServiceOn(Instant instant) {


Sorry, I think in English "no service at instant" is more correct. Can you rename the method?

leonardehrenfried · 2024-03-13T13:28:04Z

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

+      i18NStringMapper.mapNonnullToApi(stop.getName()),
+      i18NStringMapper.mapToApi(stop.getDescription()),


These two calls can move to the method by passing the i18nstring mapper.

Done, passed the transitService as well.

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java

...ext/java/org/opentripplanner/ext/vectortiles/layers/stops/DigitransitStopPropertyMapper.java

leonardehrenfried

Only tiny requests left.

…DigitransitRealtimeStopPropertyMapper.java Co-authored-by: Leonard Ehrenfried <[email protected]>

leonardehrenfried · 2024-03-18T10:54:30Z

One thing I forgot: you need to add documentation.

leonardehrenfried · 2024-03-18T14:00:18Z

You need to modify the md file in doc-templates and then run mvn test so produce the documentation.

sharhio · 2024-03-19T07:23:18Z

You need to modify the md file in doc-templates and then run mvn test so produce the documentation.

Sorry about the confusion with the files, can't believe I forgot about the templates. It should be ok now.

optionsome · 2024-03-21T11:33:41Z

doc-templates/sandbox/MapboxVectorTilesApi.md

+        "mapper": "DigitransitRealtime",
+        "maxZoom": 20,
+        "minZoom": 14,
+        "cacheMaxSeconds": 600


For the other realtime layers, we have suggested 60 second cache time. Not sure what is actually optimal here but we could use the same.

optionsome · 2024-03-21T11:35:16Z

...ext/java/org/opentripplanner/ext/vectortiles/layers/stops/DigitransitStopPropertyMapper.java

+      new KeyValue("desc", i18NStringMapper.mapToApi(stop.getDescription())),
+      new KeyValue(
+        "parentStation",
+        stop.getParentStation() != null ? stop.getParentStation().getId() : null


This is a breaking change. This shouldn't be shared between the mappers as the old one always returned a string.

optionsome · 2024-03-21T11:39:18Z

src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLDataFetchers.java

This has been regenerated (although there wasn't really a need since the graphql schema has not been updated. There are just some duplicate imports here.

optionsome · 2024-03-21T11:39:33Z

src/main/java/org/opentripplanner/apis/gtfs/generated/GraphQLTypes.java

Same as above.

This reverts commit 89e82e9.

optionsome · 2024-03-25T08:39:08Z

@leonardehrenfried seemingly our UI at least doesn't rely on the fact that the parentStation id would always be a string. Should we just return null instead of "null" for the old layer?

leonardehrenfried · 2024-03-25T09:43:26Z

Yes, you can. I'm not aware of anyone using this field. I think the null as string came from a time when we used Map.of or something similar which didn't allow nulls.

sharhio added 2 commits March 12, 2024 12:46

Stop layer with alert information

dbc00ee

Repeated code cleaned up

c977425

sharhio requested a review from a team as a code owner March 12, 2024 11:01

sharhio mentioned this pull request Mar 12, 2024

DT-5452 Show closed stops on map using badge icons HSLdevcom/digitransit-ui#4970

Open

optionsome requested changes Mar 12, 2024

View reviewed changes

t2gran added this to the 2.5 (next release) milestone Mar 12, 2024

sharhio added 3 commits March 12, 2024 16:08

added check for validityperiod, cleaned up unnecessary parts

2e201f7

Show stop status instead of alerts

c38e648

Show if there is a no service alert

f80544b

leonardehrenfried reviewed Mar 13, 2024

View reviewed changes

refactoring, validity logic fixed

dd4db60

t2gran modified the milestones: 2.5, 2.6 (next release) Mar 13, 2024

sharhio requested review from leonardehrenfried and optionsome March 13, 2024 13:23

leonardehrenfried reviewed Mar 13, 2024

View reviewed changes

refactoring

816fba9

sharhio requested a review from leonardehrenfried March 14, 2024 06:26

leonardehrenfried reviewed Mar 18, 2024

View reviewed changes

.../org/opentripplanner/ext/vectortiles/layers/stops/DigitransitRealtimeStopPropertyMapper.java Outdated Show resolved Hide resolved

leonardehrenfried reviewed Mar 18, 2024

View reviewed changes

...ext/java/org/opentripplanner/ext/vectortiles/layers/stops/DigitransitStopPropertyMapper.java Outdated Show resolved Hide resolved

leonardehrenfried requested changes Mar 18, 2024

View reviewed changes

Update src/ext/java/org/opentripplanner/ext/vectortiles/layers/stops/…

c148daf

…DigitransitRealtimeStopPropertyMapper.java Co-authored-by: Leonard Ehrenfried <[email protected]>

sharhio added 2 commits March 18, 2024 13:11

naming

796d542

documentation

5120e12

sharhio requested a review from leonardehrenfried March 18, 2024 11:21

docs generation

89e82e9

documentation

d75df7f

leonardehrenfried previously approved these changes Mar 20, 2024

View reviewed changes

optionsome requested changes Mar 21, 2024

View reviewed changes

config doc fix, parentstation not shared

5132bf2

sharhio dismissed leonardehrenfried’s stale review via 5132bf2 March 21, 2024 12:10

sharhio added 2 commits March 21, 2024 14:10

Revert "docs generation"

a408f01

This reverts commit 89e82e9.

docs generation

e8f9488

optionsome previously approved these changes Mar 21, 2024

View reviewed changes

leonardehrenfried previously approved these changes Mar 21, 2024

View reviewed changes

optionsome added Sandbox Skip Changelog labels Mar 21, 2024

changelog entry

b4a0f8e

sharhio dismissed stale reviews from leonardehrenfried and optionsome via b4a0f8e March 25, 2024 08:02

removed old string return value

e6c31e6

optionsome approved these changes Mar 25, 2024

View reviewed changes

leonardehrenfried approved these changes Mar 25, 2024

View reviewed changes

optionsome merged commit 4d7f4dd into opentripplanner:dev-2.x Mar 25, 2024
5 checks passed

optionsome deleted the new-vectortile-realtime-stop-layer branch March 25, 2024 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New vectortile layer for Digitransit realtime stops #5743

New vectortile layer for Digitransit realtime stops #5743

sharhio commented Mar 12, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

optionsome Mar 12, 2024

optionsome Mar 12, 2024

optionsome Mar 12, 2024

optionsome Mar 12, 2024

leonardehrenfried Mar 13, 2024

sharhio Mar 13, 2024

sharhio Mar 13, 2024

leonardehrenfried Mar 13, 2024 •

edited

Loading

sharhio Mar 13, 2024

leonardehrenfried Mar 13, 2024

leonardehrenfried Mar 13, 2024

leonardehrenfried Mar 13, 2024

sharhio Mar 13, 2024

leonardehrenfried Mar 13, 2024

leonardehrenfried Mar 13, 2024

leonardehrenfried Mar 13, 2024

leonardehrenfried Mar 13, 2024

sharhio Mar 14, 2024

leonardehrenfried left a comment

leonardehrenfried commented Mar 18, 2024

leonardehrenfried commented Mar 18, 2024

sharhio commented Mar 19, 2024

optionsome Mar 21, 2024 •

edited

Loading

optionsome Mar 21, 2024

optionsome Mar 21, 2024

optionsome Mar 21, 2024

optionsome commented Mar 25, 2024

leonardehrenfried commented Mar 25, 2024

	Instant currentTime = ZonedDateTime.now(transitService.getTimeZone()).toInstant();
	Instant currentTime = Instant.now();

		i18NStringMapper.mapNonnullToApi(stop.getName()),
		i18NStringMapper.mapToApi(stop.getDescription()),

New vectortile layer for Digitransit realtime stops #5743

New vectortile layer for Digitransit realtime stops #5743

Conversation

sharhio commented Mar 12, 2024

Summary

Issue

Unit tests

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried Mar 13, 2024 • 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

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

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

leonardehrenfried commented Mar 18, 2024

leonardehrenfried commented Mar 18, 2024

sharhio commented Mar 19, 2024

optionsome Mar 21, 2024 • 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

optionsome commented Mar 25, 2024

leonardehrenfried commented Mar 25, 2024

codecov bot commented Mar 12, 2024 •

edited

Loading

leonardehrenfried Mar 13, 2024 •

edited

Loading

optionsome Mar 21, 2024 •

edited

Loading