Support timezones for Puerto Rico #8225

emyl3 · 2024-10-23T22:05:40Z

BACKEND PULL REQUEST

Related Issue

Resolves Support Request: User not receiving CSV upload notification (successful or unsuccessful) #8203

Changes Proposed

Fixes a bug in our CSV bulk uploader where we were generating a timezone for date values in the following columns:
order_test_date, specimen_collection_date, testing_lab_specimen_received_date, test_result_date, date_result_released
- this bug occurred for any valid ordering provider address in Puerto Rico AND when the above columns had only a date value OR date and time value without a timezone (e.g. 10/18/2024 or 10/18/2024 12:00 and NOT 10/18/2024 12:00 ET)
Added the timezone abbreviations for Puerto Rico (AST and ADT) so that these timezones can be used in dates for our bulk uploader and pass validation

Additional Information

N/A

Testing

example CSV with a VALID Puerto Rico ordering provider and no timestamps - pr_ordering_provider_ex.csv
- should fail silently on main
- should succeed on dev3
- editing the file and adding AST or ADT to any date timestamps should also pass the validations

emyl3 · 2024-10-24T16:35:07Z

backend/src/main/java/gov/cdc/usds/simplereport/service/AddressValidationService.java

@@ -144,6 +145,7 @@ public ZoneId getZoneIdByLookup(Lookup lookup) {
      return null;
    }

-    return ZoneId.of("US/" + timezoneInfo.timezoneCommonName);


⚠️ For a valid Puerto Rico address, we were getting back Atlantic for timezoneInfo.timezoneCommonName.
If you see the list here, US/Atlantic doesn't exist and was throwing an exception.

Decided to get the ZoneId using the ZoneOffset value instead.

I think using the ZoneOffset might run into issues with whether ZoneId.of() is returning the correct zone since it would change based on whether the zone obeys daylight savings time or not. The slight inaccuracy might not matter, but the original issue that forced us to start handling time zones in bulk upload was a STLT having issues with some of their data being marked as an incorrect day due to time wackiness. Wondering if it would be better in this case to just specifically handle the Atlantic time zone and still use the existing US/commonName to handle the other time zones since Smartystreets is still able to pinpoint what time zone the address is in compared to the possible ambiguity with utcOffset? I wish Smartystreets simply returned the official time zone rather than just the common name 😩

OOO thank you for this context, Mike! I will rethink my implementation!

DanielSass · 2024-10-24T16:50:45Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java

@@ -37,6 +37,7 @@ public class DateTimeUtils {
  private static final ZoneId hawaiiTimeZoneId = ZoneId.of("US/Hawaii");
  private static final ZoneId aleutianTimeZoneId = ZoneId.of("US/Aleutian");
  private static final ZoneId samoaTimeZoneId = ZoneId.of("US/Samoa");
+  private static final ZoneId puertoRicoTimeZoneId = ZoneId.of("America/Puerto_Rico");


In addition to specifically handling Puerto Rico, I think it makes sense to catch whatever exception was being thrown and default the timezone to something (eastern time?). If this is a big deal we can add alerting for when it happens, but I think the failure state should be that the file still gets uploaded successfully.

Do we know how the single entry flow was handling this case?

Re: handling the exception

Based on the fix implemented I don't think there would be an exception thrown anymore.

Because we were doing this before

return ZoneId.of("US/" + timezoneInfo.timezoneCommonName);

if timezoneInfo.timezoneCommonName returned something like Atlantic (like the valid Puerto Rico address did), we'd be trying to get the ZoneId.of("US/Atlantic") which would throw the exception.

Now, since we are using the timezoneInfo.utcOffset value instead of creating strings (that potentially don't exist) we should be good. If timezoneInfo == null we already return null. The method that calls getZoneIdByAddress sets the default timezone (eastern time) if the value is null.

Re: single entry flow
I believe this is how we are handling it. We aren't extracting any timezone data from the ordering provider so we would not run into the same issue in the single entry flow based on my read of this 👀

emyl3 · 2024-10-25T20:29:13Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java

@@ -62,9 +63,9 @@ public class DateTimeUtils {
          Map.entry("HI", hawaiiTimeZoneId),
          Map.entry("HST", hawaiiTimeZoneId),
          Map.entry("HDT", aleutianTimeZoneId),
-          Map.entry("AS", samoaTimeZoneId),


⚠️ removed "AS" and "ASM" because I don't think they are actually timezones for American Samoa based on this list Let me know if I should be using a different list 🤔

Generally we should use the IANA Time Zone Database. The wiki page here has the latest 2024 list.

For this particular map, IIRC we wanted to allow users to include these valid time zone abbreviations in their upload since some people still use AS or ASM. For example, this site still includes AS and ASM as abbreviations even though the ISO official time zone is SST.

OOO thank you for letting me know! I'll take a look!

Added "AS" and "ASM" back

mpbrown

Thanks for looking into this! Left some questions about whether we can rely on utc offset vs the geolocated time zone name from smarty

mpbrown · 2024-10-28T14:59:11Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java

@@ -62,9 +63,9 @@ public class DateTimeUtils {
          Map.entry("HI", hawaiiTimeZoneId),
          Map.entry("HST", hawaiiTimeZoneId),
          Map.entry("HDT", aleutianTimeZoneId),
-          Map.entry("AS", samoaTimeZoneId),


Generally we should use the IANA Time Zone Database. The wiki page here has the latest 2024 list.

For this particular map, IIRC we wanted to allow users to include these valid time zone abbreviations in their upload since some people still use AS or ASM. For example, this site still includes AS and ASM as abbreviations even though the ISO official time zone is SST.

mpbrown · 2024-10-28T15:18:52Z

backend/src/main/java/gov/cdc/usds/simplereport/service/AddressValidationService.java

@@ -144,6 +145,7 @@ public ZoneId getZoneIdByLookup(Lookup lookup) {
      return null;
    }

-    return ZoneId.of("US/" + timezoneInfo.timezoneCommonName);


I think using the ZoneOffset might run into issues with whether ZoneId.of() is returning the correct zone since it would change based on whether the zone obeys daylight savings time or not. The slight inaccuracy might not matter, but the original issue that forced us to start handling time zones in bulk upload was a STLT having issues with some of their data being marked as an incorrect day due to time wackiness. Wondering if it would be better in this case to just specifically handle the Atlantic time zone and still use the existing US/commonName to handle the other time zones since Smartystreets is still able to pinpoint what time zone the address is in compared to the possible ambiguity with utcOffset? I wish Smartystreets simply returned the official time zone rather than just the common name 😩

emyl3 · 2024-10-28T16:40:39Z

Back in draft while I address Mike's comments! - PLEASE DO NOT REVIEW!

sonarqubecloud · 2024-10-29T19:50:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

emyl3 · 2024-10-29T19:51:24Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java

+   * Map of common name `time_zone` values returned from SmartyStreets and their corresponding
+   * ZoneId https://www.smarty.com/docs/cloud/us-street-api
+   */
+  public static final Map<String, ZoneId> commonNameZoneIdMap =


⚠️ explicitly mapped all the possible SmartyStreets time_zone common names that could be returned to specific ZoneIds

strange that they use this list instead of something more standard

emyl3 · 2024-10-29T19:52:12Z

backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java

+   * Map of abbreviated timezones and their corresponding ZoneId Used for bulk upload timestamp
+   * validations and when converting user-supplied timezones to a ZoneId
+   */
+  public static final Map<String, ZoneId> timezoneAbbreviationZoneIdMap =


⚠️ renamed from validTimeZoneIdMap to hopefully be more descriptive 😅

emyl3 · 2024-10-29T20:40:44Z

Ready for review again! @mpbrown @mehansen @bobbywells52 @DanielSass

mpbrown · 2024-10-29T21:55:01Z

The example file worked for dev3, but when I edited it to include the time zones within the date time values, it gave me these errors 🤔

Here's the edited copy that I tried on dev3
pr_ordering_provider_ex_edit.csv

Also I noticed in the example file that we're using M/D/YYYY HH:mm:ss PM and wondering whether it currently tries to interpret PM as a time zone abbreviation. This could probably be a separate ticket, but maybe we should either explicitly handle if the user puts in AM/PM or update the guide to tell them not to put AM/PM in addition to a time zone (I could see some users not wanting to use 24H notation vs 12H notation though) 😮‍💨

emyl3 · 2024-10-30T14:52:11Z

The example file worked for dev3, but when I edited it to include the time zones within the date time values, it gave me these errors 🤔

Here's the edited copy that I tried on dev3 pr_ordering_provider_ex_edit.csv

Also I noticed in the example file that we're using M/D/YYYY HH:mm:ss PM and wondering whether it currently tries to interpret PM as a time zone abbreviation. This could probably be a separate ticket, but maybe we should either explicitly handle if the user puts in AM/PM or update the guide to tell them not to put AM/PM in addition to a time zone (I could see some users not wanting to use 24H notation vs 12H notation though) 😮‍💨

OO I checked that file and noticed that there's an extra whitespace in between the date and time so it was failing the DATE_TIME_REGEX check.

I could add that as part of the ticket you alluded to to make our DATE_TIME_REGEX a little bit more robust. Does that work, Mike? 🤔

mpbrown · 2024-10-30T16:10:53Z

The example file worked for dev3, but when I edited it to include the time zones within the date time values, it gave me these errors 🤔

Here's the edited copy that I tried on dev3 pr_ordering_provider_ex_edit.csv
Also I noticed in the example file that we're using M/D/YYYY HH:mm:ss PM and wondering whether it currently tries to interpret PM as a time zone abbreviation. This could probably be a separate ticket, but maybe we should either explicitly handle if the user puts in AM/PM or update the guide to tell them not to put AM/PM in addition to a time zone (I could see some users not wanting to use 24H notation vs 12H notation though) 😮‍💨

OO I checked that file and noticed that there's an extra whitespace in between the date and time so it was failing the DATE_TIME_REGEX check.

I could add that as part of the ticket you alluded to to make our DATE_TIME_REGEX a little bit more robust. Does that work, Mike? 🤔

Ooof my bad, I used excel to edit the csv and forgot that excel adds a bunch of autoformatting on save which ends up messing with the csv 😬 Those edited time zones work as expected on dev3 now!

I think it makes sense to create another ticket since we'll want to check with the rest of the team on what (if anything) we want to change about parsing date time inputs in the csv. It seems better to put it in the backlog for now and think about it more closely once we pivot our focus to revamping bulk upload

mpbrown

Great job on this! The time zone issues are a real nuisance so I appreciate you working on it!

emyl3 · 2024-10-30T18:17:10Z

@mpbrown created a ticket here: #8253

mehansen

lgtm! thoughts on a low-urgency alert of some kind that lets us know if we ever log Unsupported timezone common name? since we don't really have any guarantee that Smarty doesn't change the set of strings they use for that field

emyl3 · 2024-10-31T15:25:12Z

lgtm! thoughts on a low-urgency alert of some kind that lets us know if we ever log Unsupported timezone common name? since we don't really have any guarantee that Smarty doesn't change the set of strings they use for that field

I like that idea! Would it be enough for me to create a ticket for it? Or would you like it included in this PR, @mehansen? 🤔

mehansen · 2024-10-31T18:14:12Z

lgtm! thoughts on a low-urgency alert of some kind that lets us know if we ever log Unsupported timezone common name? since we don't really have any guarantee that Smarty doesn't change the set of strings they use for that field

I like that idea! Would it be enough for me to create a ticket for it? Or would you like it included in this PR, @mehansen? 🤔

new ticket seems good to me!

emyl3 · 2024-10-31T20:24:39Z

lgtm! thoughts on a low-urgency alert of some kind that lets us know if we ever log Unsupported timezone common name? since we don't really have any guarantee that Smarty doesn't change the set of strings they use for that field

I like that idea! Would it be enough for me to create a ticket for it? Or would you like it included in this PR, @mehansen? 🤔

new ticket seems good to me!

@mehansen ticket created here: #8257

emyl3 force-pushed the elisa/8203-PR-timezone-support branch from 10ead1e to 498ff68 Compare October 23, 2024 22:08

emyl3 temporarily deployed to dev3 October 24, 2024 14:48 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 October 24, 2024 14:56 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 October 24, 2024 14:58 — with GitHub Actions Inactive

emyl3 commented Oct 24, 2024

View reviewed changes

DanielSass reviewed Oct 24, 2024

View reviewed changes

emyl3 commented Oct 25, 2024

View reviewed changes

emyl3 requested review from mehansen, mpbrown and bobbywells52 October 25, 2024 20:47

emyl3 marked this pull request as ready for review October 25, 2024 20:47

emyl3 requested a review from DanielSass October 25, 2024 20:57

mpbrown reviewed Oct 28, 2024

View reviewed changes

emyl3 marked this pull request as draft October 28, 2024 15:43

emyl3 added 2 commits October 29, 2024 09:40

Support timezones for Puerto Rico

2d2beb4

Address feedback

1720d18

emyl3 force-pushed the elisa/8203-PR-timezone-support branch from 498ff68 to 1720d18 Compare October 29, 2024 19:39

emyl3 commented Oct 29, 2024

View reviewed changes

emyl3 temporarily deployed to dev3 October 29, 2024 19:53 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 October 29, 2024 20:03 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 October 29, 2024 20:06 — with GitHub Actions Inactive

emyl3 marked this pull request as ready for review October 29, 2024 20:40

emyl3 requested a review from mpbrown October 29, 2024 20:40

mpbrown approved these changes Oct 30, 2024

View reviewed changes

emyl3 mentioned this pull request Oct 30, 2024

[BUG] Update bulk uploader datetime validations #8253

Open

mehansen approved these changes Oct 30, 2024

View reviewed changes

emyl3 mentioned this pull request Oct 31, 2024

Create a low urgency PagerDuty alert when getting an unsupported timezone common name #8257

Open

emyl3 added this pull request to the merge queue Oct 31, 2024

Merged via the queue into main with commit 7d39218 Oct 31, 2024
37 of 38 checks passed

emyl3 deleted the elisa/8203-PR-timezone-support branch October 31, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support timezones for Puerto Rico #8225

Support timezones for Puerto Rico #8225

emyl3 commented Oct 23, 2024 •

edited

Loading

emyl3 Oct 24, 2024 •

edited

Loading

mpbrown Oct 28, 2024

emyl3 Oct 28, 2024

DanielSass Oct 24, 2024

emyl3 Oct 25, 2024

emyl3 Oct 25, 2024

emyl3 Oct 25, 2024

mpbrown Oct 28, 2024

emyl3 Oct 28, 2024

emyl3 Oct 29, 2024

mpbrown left a comment

mpbrown Oct 28, 2024

mpbrown Oct 28, 2024

emyl3 commented Oct 28, 2024

sonarqubecloud bot commented Oct 29, 2024

emyl3 Oct 29, 2024

mehansen Oct 30, 2024

emyl3 Oct 29, 2024

emyl3 commented Oct 29, 2024

mpbrown commented Oct 29, 2024

emyl3 commented Oct 30, 2024

mpbrown commented Oct 30, 2024

mpbrown left a comment

emyl3 commented Oct 30, 2024

mehansen left a comment

emyl3 commented Oct 31, 2024

mehansen commented Oct 31, 2024

emyl3 commented Oct 31, 2024

Support timezones for Puerto Rico #8225

Support timezones for Puerto Rico #8225

Conversation

emyl3 commented Oct 23, 2024 • edited Loading

BACKEND PULL REQUEST

Related Issue

Changes Proposed

Additional Information

Testing

emyl3 Oct 24, 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

mpbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emyl3 commented Oct 28, 2024

sonarqubecloud bot commented Oct 29, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emyl3 commented Oct 29, 2024

mpbrown commented Oct 29, 2024

emyl3 commented Oct 30, 2024

mpbrown commented Oct 30, 2024

mpbrown left a comment

Choose a reason for hiding this comment

emyl3 commented Oct 30, 2024

mehansen left a comment

Choose a reason for hiding this comment

emyl3 commented Oct 31, 2024

mehansen commented Oct 31, 2024

emyl3 commented Oct 31, 2024

emyl3 commented Oct 23, 2024 •

edited

Loading

emyl3 Oct 24, 2024 •

edited

Loading