Make graph time zone configurable by hannesj · Pull Request #4281 · opentripplanner/OpenTripPlanner

hannesj · 2022-07-19T08:15:43Z

Summary

Make it possible to configure the time zone when building the graph. Also throw an error, if multiple time zones are detected.

The scheduled times are adjusted to be in the configured time zone. No changes needed for real-time updaters are needed, as they operate on absolute times already.

Unit tests

New unit test for behaviour with multiple timezones is tested. TODO: Add tests for TimeZoneAdjusterModule

Documentation

Added configuration parameter

OSMModule doesn't query the time zone so clearing cache is not necessary

codecov-commenter · 2022-07-19T08:21:46Z

Codecov Report

Merging #4281 (887f806) into dev-2.x (8c9aa6c) will increase coverage by 0.00%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##             dev-2.x    #4281   +/-   ##
==========================================
  Coverage      57.21%   57.22%           
- Complexity     10767    10774    +7     
==========================================
  Files           1426     1427    +1     
  Lines          57938    57955   +17     
  Branches        6714     6715    +1     
==========================================
+ Hits           33151    33162   +11     
- Misses         22762    22766    +4     
- Partials        2025     2027    +2

Impacted Files	Coverage Δ
.../org/opentripplanner/api/mapping/AgencyMapper.java	`0.00% <0.00%> (ø)`
...rg/opentripplanner/graph_builder/GraphBuilder.java	`0.00% <0.00%> (ø)`
.../graph_builder/module/osm/OpenStreetMapModule.java	`86.74% <ø> (-0.02%)`	⬇️
...ripplanner/model/calendar/CalendarServiceData.java	`69.69% <ø> (-5.92%)`	⬇️
...anner/model/calendar/impl/CalendarServiceImpl.java	`81.81% <ø> (-1.52%)`	⬇️
...ipplanner/model/impl/OtpTransitServiceBuilder.java	`91.27% <ø> (-0.06%)`	⬇️
...opentripplanner/standalone/config/NodeAdapter.java	`87.09% <0.00%> (-0.41%)`	⬇️
.../opentripplanner/transit/service/TransitModel.java	`73.93% <80.95%> (+1.29%)`	⬆️
...r/graph_builder/module/TimeZoneAdjusterModule.java	`85.71% <85.71%> (ø)`
...aphqlapi/datafetchers/LegacyGraphQLAgencyImpl.java	`23.91% <100.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c9aa6c...887f806. Read the comment docs.

…time_zone

optionsome

Seems reasonable changes. However, I don't understand the wording in the GTFS specification "Timezone where the transit agency is located. If multiple agencies are specified in the dataset, each must have the same agency_timezone.". I have hard time interpreting that in any other way than that the time zone should be the same for each agency in one data set.

t2gran

This looks good, a few requested changes. The logic for setting and validating the timezone is probably not the transit models responsibility, but there is not a clear candidate for it, so this is fine for now.

…into otp2_configurable_time_zone

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

t2gran · 2022-07-25T12:54:56Z

+  public ZoneId asZoneId(String paramName, ZoneId defaultValue) {
+    return exist(paramName) ? ZoneId.of(param(paramName).asText()) : defaultValue;
+  }
+


This is missing a UnitTest, it also throws something else then a OTPApplication exception if the timezone is not found - that might be ok, but most other method do not. If the OTPApplication exception is thrown the application logs the error WITHOUT a stacktrace.

hannesj · 2022-07-25T13:30:22Z

I have hard time interpreting that in any other way than that the time zone should be the same for each agency in one data set.

Currently this is not enforced, and there are no changes related to it. This is to support multiple bundles with different timezones

optionsome · 2022-07-25T13:33:52Z

Currently this is not enforced, and there are no changes related to it. This is to support multiple bundles with different timezones

I wonder what is the origin of that restriction in the specification. Seems like a really arbitrary limitation.

… otp2_configurable_time_zone

hannesj · 2022-07-25T13:52:16Z

I wonder what is the origin of that restriction in the specification. Seems like a really arbitrary limitation.

https://groups.google.com/g/gtfs-changes/c/Sh0e4o9o2Gw/m/vd8t58WhW0oJ Here is some context for the decision

hannesj added 4 commits July 18, 2022 15:42

Throw error when building graph with multiple timezones

7d7d0a3

OSMModule doesn't query the time zone so clearing cache is not necessary

Add possibility to explicitly set time zone for graph

56a5f7f

Adjust scheduled trips' time zones

168e9cc

Move agency time zone from CalendarServiceData to transit model

c1caa8f

hannesj added !Improvement A functional improvement or micro feature !Technical Debt Improve code quality, no functional changes. +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Jul 19, 2022

hannesj added this to the 2.2 milestone Jul 19, 2022

hannesj requested review from optionsome and t2gran July 19, 2022 09:53

hannesj added 2 commits July 21, 2022 15:12

Merge remote-tracking branch 'origin/dev-2.x' into otp2_configurable_…

1d12f51

…time_zone

Add test for modifying time zones

6213110

hannesj marked this pull request as ready for review July 21, 2022 12:29

hannesj requested a review from a team as a code owner July 21, 2022 12:29

optionsome reviewed Jul 21, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/transit/service/TransitModel.java Outdated

t2gran reviewed Jul 22, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/standalone/config/BuildConfig.java Outdated

t2gran reviewed Jul 22, 2022

View reviewed changes

Comment thread docs/BuildConfiguration.md Outdated

t2gran reviewed Jul 22, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/transit/service/TransitModel.java Outdated

t2gran reviewed Jul 22, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/transit/service/TransitModel.java Outdated

t2gran requested changes Jul 22, 2022

View reviewed changes

Comment thread src/main/java/org/opentripplanner/transit/service/TransitModel.java Outdated

Comment thread src/test/java/org/opentripplanner/transit/service/TransitModelTest.java Outdated

Comment thread src/main/java/org/opentripplanner/graph_builder/GraphBuilder.java Outdated

hannesj added 3 commits July 25, 2022 10:10

Apply review feedback

f0f2317

Merge branch 'dev-2.x' of github.com:opentripplanner/OpenTripPlanner …

e9c13c3

…into otp2_configurable_time_zone

Fix test

887f806

optionsome reviewed Jul 25, 2022

View reviewed changes

Comment thread docs/BuildConfiguration.md Outdated

Update docs/BuildConfiguration.md

d36ef93

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

t2gran requested changes Jul 25, 2022

View reviewed changes

Add unit test, and correct exception type

9853a17

Merge remote-tracking branch 'entur/otp2_configurable_time_zone' into…

617e8b8

… otp2_configurable_time_zone

optionsome approved these changes Jul 25, 2022

View reviewed changes

t2gran approved these changes Jul 25, 2022

View reviewed changes

hannesj merged commit f84dc1a into opentripplanner:dev-2.x Jul 25, 2022

t2gran pushed a commit that referenced this pull request Jul 25, 2022

Add changelog entry for #4281 [ci skip]

58e8cf3

hannesj deleted the otp2_configurable_time_zone branch July 25, 2022 14:35

t2gran pushed a commit that referenced this pull request Jul 25, 2022

Bump serialization version id for #4281

c22b73e

leonardehrenfried mentioned this pull request Mar 24, 2023

How To Handle Time Zones? (General Discussion) #2602

Closed

habrahamsson-skanetrafiken mentioned this pull request Oct 27, 2025

TimeZone adjustment doesn't work #7010

Closed

Navigation Menu

Repository navigation

Make graph time zone configurable - #4281

Make graph time zone configurable#4281
hannesj merged 12 commits into
opentripplanner:dev-2.xfrom
entur:otp2_configurable_time_zone

hannesj commented Jul 19, 2022

Uh oh!

codecov-commenter commented Jul 19, 2022 •

edited

Loading

Uh oh!

optionsome left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran Jul 25, 2022

Uh oh!

hannesj commented Jul 25, 2022

Uh oh!

optionsome commented Jul 25, 2022

Uh oh!

hannesj commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

hannesj commented Jul 19, 2022

Summary

Unit tests

Documentation

Uh oh!

codecov-commenter commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t2gran Jul 25, 2022

Choose a reason for hiding this comment

Uh oh!

hannesj commented Jul 25, 2022

Uh oh!

optionsome commented Jul 25, 2022

Uh oh!

hannesj commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 19, 2022 •

edited

Loading