feat: add support for timezones - 946 by qcdyx · Pull Request #952 · MobilityData/mobility-feed-api

qcdyx · 2025-03-06T16:46:16Z

Summary:

Closes #946 #947 #948

Expected behavior:

the schema in gtfsdataset supports timezone for service date range values

Also includes timezone_agency in the dataset / search endpoints (API)

Updated the functions to set data / compare service date range with timezones (UTC)

backfill_dataset_service_date_range: backfills timezone if available
update_feeds_status
process_validation_report

Testing tips:

For backfill function

delete the service date range for a dataset validated in 6.0 and assure it has a validation report with timezone, run the function and see the service date range with timezone saved

For update feed status

manually update a feed status to wrong, run the script and see the updated correct status. Assure the feeds latest dataset has service date range or it wont update

For process validation report

Find a feed, delete its corresponding validation report, and run the function. Should generate the validation report and fill the service date range + timezone if exists

Please make sure these boxes are checked before submitting your pull request - thanks!

Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
Add or update any needed documentation to the repo
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)

Example of the final result of backfill function or process_validation_report function

Example of the new endpoint return:

When the agency timezone exists it sets the utc offset accordingly

Alessandro100 · 2025-03-10T13:13:52Z

liquibase/changes/feat_946.sql

+ALTER TABLE gtfsdataset 
+ADD COLUMN service_date_range_timezone TIMESTAMP WITH TIME ZONE 


We can call the column agency_timezone since it applies to the entire feed and it should be of type VARCHAR(255) since the values will be ex: America/Montreal or Pacific/Fiji

We need to alter the columns service_date_range_start and service_date_range_end to be TIMESTAMP WITH TIME ZONE instead of DATE

Altering the service_date_range_start and service_date_range_end columns caused a liquibase.exception.DatabaseException due to a dependency on the feedsearch materialized view (_RETURN rule). Since modifying the column type directly isn’t possible, we discussed on Slack and decided to add a new column instead.

liquibase/changes/feat_946.sql

cka-y · 2025-03-11T12:03:11Z

Is this PR ready for review or are we planning to add code changes to it as discussed yesterday?

Alessandro100 · 2025-03-11T12:32:06Z

Is this PR ready for review or are we planning to add code changes to it as discussed yesterday?

Good call, it is not ready for review just yet, we are going to make the changes effected by the schema change here

cka-y · 2025-03-11T12:34:20Z

Is this PR ready for review or are we planning to add code changes to it as discussed yesterday?

Good call, it is not ready for review just yet, we are going to make the changes effected by the schema change here

Got it! Converted to draft to indicate to reviewers that it isn't ready

github-actions · 2025-03-12T14:08:40Z

Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-952-jvn7l2fz.web.app

cka-y · 2025-03-12T16:20:19Z

docs/DatabaseCatalogAPI.yaml

+          example: 2023-07-10T06:00:00Z
+          format: date-time
        service_date_range_end:
          description: The start date of the service date range for the dataset.


[question] should we specify in the descriptions of service_date_range_[start | end] that it's in UTC time?

yeah I think that would be good info

cka-y · 2025-03-12T16:31:54Z

functions-python/backfill_dataset_service_date_range/src/main.py

+            formatted_service_start_date = None
+            formatted_service_end_date = None


[nit] declared but unused variables. can be safely deleted

these variables are used!

sorry i misspoke -- since the variables are use but their initialization to None is not, it's redundant initialization and it can be removed from these lines (124-125). This is just a nit! Feel free to ignore it 😄

cka-y · 2025-03-12T16:35:30Z

functions-python/backfill_dataset_service_date_range/src/main.py

-            dataset.service_date_range_end = extracted_service_end_date
+            # If timezone is None (not found in the validation report), it will use UTC as base.
+            # It will not save the timezone in the database if it is None.
+            formatting_timezone = extracted_timezone


[suggestion] add a timezone validation to fallback on UTC if it's invalid. Based on the current logic, if the timezone is defined but invalid, it will throw an exception and no service dates will be added for this dataset.

cka-y · 2025-03-12T16:37:50Z

functions-python/backfill_dataset_service_date_range/src/main.py

+                    refresh_materialized_view(session, t_feedsearch.name)
                    session.commit()


You should commit before you refresh the materialized view otherwise the changes won't take effect until the next refresh

I referenced this from functions-python/extract_location/src/reverse_geolocation/location_extractor.py should it be changed there as well?

I removed the entire folder as part of #985

...n/backfill_dataset_service_date_range/tests/test_backfill_dataset_service_date_range_main.py

cka-y · 2025-03-12T16:39:25Z

functions-python/process_validation_report/src/main.py


+    summary = json_report.get("summary", {})
+    if isinstance(summary.get("agencies"), list) and summary["agencies"]:
+        extracted_timezone = summary["agencies"][0].get("timezone", None)


functions-python/process_validation_report/src/main.py

cka-y · 2025-03-12T16:42:31Z

functions-python/update_feed_status/src/main.py

+        refresh_materialized_view(session, t_feedsearch.name)
        session.commit()


same comment about commit before refresh

davidgamez · 2025-03-12T13:56:08Z

functions-python/backfill_dataset_service_date_range/src/main.py

+                formatted_service_start_date = datetime.strptime(
+                    extracted_service_start_date, "%Y-%m-%d"
+                )


Could you create a function for this code to avoid repetition?

davidgamez · 2025-03-12T14:00:21Z

functions-python/backfill_dataset_service_date_range/src/main.py

+            if isinstance(summary.get("agencies"), list) and summary["agencies"]:
+                extracted_timezone = summary["agencies"][0].get("timezone", None)
+
+            formatted_service_start_date = None


Should we just continue the loop if extracted_service_start_date or extracted_service_end_date are None? This will avoid a ValueErrror in case there is no service date in the feed.

davidgamez · 2025-03-12T14:01:05Z

functions-python/backfill_dataset_service_date_range/src/main.py

-                json_data.get("summary", {})
-                .get("feedInfo", {})
-                .get("feedServiceWindowEnd", None)
+            extracted_service_end_date = summary.get("feedInfo", {}).get(


[suggestion]: We can use here the newly added get_nested_value function to account for empty strings(trimmed). The same for the extracted time zone value.

davidgamez · 2025-03-12T14:06:47Z

functions-python/backfill_dataset_service_date_range/src/main.py

+            local_service_start_date = formatted_service_start_date.replace(
+                hour=0, minute=0, tzinfo=ZoneInfo(formatting_timezone)
+            )
+            utc_service_start_date = local_service_start_date.astimezone(
+                ZoneInfo("UTC")


Could you create a function to avoid repetition?

liquibase/changes/feat_946.sql

…ge_start and service_date_range_end

cka-y

LGTM

qcdyx linked an issue Mar 6, 2025 that may be closed by this pull request

Add support for timezones #937

Closed

qcdyx marked this pull request as draft March 6, 2025 16:54

qcdyx marked this pull request as ready for review March 6, 2025 19:59

qcdyx requested a review from Alessandro100 March 10, 2025 12:43

Alessandro100 reviewed Mar 10, 2025

View reviewed changes

qcdyx requested a review from Alessandro100 March 10, 2025 14:34

cka-y reviewed Mar 11, 2025

View reviewed changes

liquibase/changes/feat_946.sql Show resolved Hide resolved

cka-y marked this pull request as draft March 11, 2025 12:33

Alessandro100 force-pushed the 937-add-support-for-timezones branch 3 times, most recently from d8be5be to 1ef7f0e Compare March 12, 2025 13:23

Alessandro100 marked this pull request as ready for review March 12, 2025 13:31

Alessandro100 force-pushed the 937-add-support-for-timezones branch from 1ef7f0e to faf0ee8 Compare March 12, 2025 13:32

emmambd linked an issue Mar 12, 2025 that may be closed by this pull request

Modify Backfill Function to Handle Time Zones #947

Closed

emmambd linked an issue Mar 12, 2025 that may be closed by this pull request

Modify dataset endpoint to support timezone #948

Closed

cka-y reviewed Mar 12, 2025

View reviewed changes

davidgamez approved these changes Mar 12, 2025

View reviewed changes

davidgamez reviewed Mar 12, 2025

View reviewed changes

liquibase/changes/feat_946.sql Show resolved Hide resolved

Alessandro100 force-pushed the 937-add-support-for-timezones branch from 700156c to 78709f7 Compare March 12, 2025 20:36

qcdyx added 7 commits March 17, 2025 11:37

Updated gtfsdataset Schema to Support Time Zones for service_date_ran…

802f1af

…ge_start and service_date_range_end

added service_date_range_timezone column to gtfsdataset table

321d935

renamed files

825f23b

fixed SQL error

a3eb76c

fixed SQL error

701e50e

removed DEFAULT 'UTC';

da63e5a

updated db schema

6b4494b

Alessandro100 added 15 commits March 17, 2025 11:38

timezone changes

ab9be1d

lint

82b2596

added logging

10e82f7

lint and test

f97abec

timezone fix

8ab2bd6

unit test fix

9110b00

update open api schema

f9e77f1

unit test

a07bf7e

lint

03898bb

unit test fix

975779a

agency added to endpoints + test values updated

4badfec

lint

034f596

model fix

39e8f45

changelog

ad1ad87

updated view

12ca886

Alessandro100 force-pushed the 937-add-support-for-timezones branch from 0ae8191 to 12ca886 Compare March 17, 2025 15:39

Alessandro100 added 8 commits March 21, 2025 10:16

fix ENV issue

66cdd67

update documentation

e6fe98f

refactor sdr logic + tests

56db337

lint

7e49b13

frontend type update

6052ebd

timezone.py independence

d639dae

process validation report test fix

1a3f39d

documentation

5b82008

Alessandro100 requested review from cka-y and davidgamez March 24, 2025 13:11

cka-y approved these changes Mar 25, 2025

View reviewed changes

Alessandro100 merged commit 904cfeb into main Mar 25, 2025
15 of 17 checks passed

Alessandro100 deleted the 937-add-support-for-timezones branch March 25, 2025 17:44

		ALTER TABLE gtfsdataset
		ADD COLUMN service_date_range_timezone TIMESTAMP WITH TIME ZONE No newline at end of file

		formatted_service_start_date = None
		formatted_service_end_date = None

		refresh_materialized_view(session, t_feedsearch.name)
		session.commit()

Conversation

qcdyx commented Mar 6, 2025 • edited by Alessandro100 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cka-y commented Mar 11, 2025

Uh oh!

Alessandro100 commented Mar 11, 2025

Uh oh!

cka-y commented Mar 11, 2025

Uh oh!

github-actions bot commented Mar 12, 2025

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

cka-y left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

qcdyx commented Mar 6, 2025 •

edited by Alessandro100

Loading