Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support timezones for Puerto Rico #8225

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Support timezones for Puerto Rico #8225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2d2beb4
Support timezones for Puerto Rico
emyl3 Oct 23, 2024
1720d18
Address feedback
emyl3 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion backend/src/main/java/gov/cdc/usds/simplereport/service/AddressValidationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static gov.cdc.usds.simplereport.api.Translators.parseState;
import static gov.cdc.usds.simplereport.api.Translators.parseString;
import static gov.cdc.usds.simplereport.utils.DateTimeUtils.commonNameZoneIdMap;

import com.smartystreets.api.ClientBuilder;
import com.smartystreets.api.exceptions.SmartyException;
Expand Down Expand Up @@ -144,6 +145,12 @@ public ZoneId getZoneIdByLookup(Lookup lookup) {
return null;
}

return ZoneId.of("US/" + timezoneInfo.timezoneCommonName);
Copy link
Collaborator Author

@emyl3 emyl3 Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😩

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

String timezoneCommonName = timezoneInfo.timezoneCommonName.toLowerCase();
if (commonNameZoneIdMap.containsKey(timezoneCommonName)) {
return commonNameZoneIdMap.get(timezoneCommonName);
} else {
log.error("Unsupported timezone common name: {}", timezoneCommonName);
return null;
}
}
}
35 changes: 31 additions & 4 deletions backend/src/main/java/gov/cdc/usds/simplereport/utils/DateTimeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,33 @@ 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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👀


public static final Map<String, ZoneId> validTimeZoneIdMap =
/**
* 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 =
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strange that they use this list instead of something more standard

Map.ofEntries(
Map.entry("Alaska".toLowerCase(), alaskaTimeZoneId),
Map.entry("Atlantic".toLowerCase(), puertoRicoTimeZoneId),
Map.entry("Central".toLowerCase(), centralTimeZoneId),
Map.entry("Eastern".toLowerCase(), easternTimeZoneId),
Map.entry("Hawaii".toLowerCase(), hawaiiTimeZoneId),
Map.entry("Mountain".toLowerCase(), mountainTimeZoneId),
Map.entry("None".toLowerCase(), FALLBACK_TIMEZONE_ID),
Map.entry("Pacific".toLowerCase(), pacificTimeZoneId),
Map.entry("Samoa".toLowerCase(), samoaTimeZoneId),
Map.entry("UTC+9".toLowerCase(), ZoneId.of("+9")),
Map.entry("UTC+10".toLowerCase(), ZoneId.of("+10")),
Map.entry("UTC+11".toLowerCase(), ZoneId.of("+11")),
Map.entry("UTC+12".toLowerCase(), ZoneId.of("+12")));

/**
* 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 =
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ renamed from validTimeZoneIdMap to hopefully be more descriptive 😅

Map.ofEntries(
Map.entry("UTC", ZoneOffset.UTC),
Map.entry("UT", ZoneOffset.UTC),
Expand All @@ -64,7 +89,9 @@ public class DateTimeUtils {
Map.entry("HDT", aleutianTimeZoneId),
Map.entry("AS", samoaTimeZoneId),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added "AS" and "ASM" back

Map.entry("ASM", samoaTimeZoneId),
Map.entry("SST", samoaTimeZoneId));
Map.entry("SST", samoaTimeZoneId),
Map.entry("ADT", puertoRicoTimeZoneId),
Map.entry("AST", puertoRicoTimeZoneId));

public static ZonedDateTime convertToZonedDateTime(
String dateString,
Expand Down Expand Up @@ -111,8 +138,8 @@ public static boolean hasTimezoneSubstring(String value) {
}

public static ZoneId parseZoneId(String timezoneCode) {
if (validTimeZoneIdMap.containsKey(timezoneCode.toUpperCase())) {
return validTimeZoneIdMap.get(timezoneCode.toUpperCase());
if (timezoneAbbreviationZoneIdMap.containsKey(timezoneCode.toUpperCase())) {
return timezoneAbbreviationZoneIdMap.get(timezoneCode.toUpperCase());
}
return ZoneId.of(timezoneCode);
}
Expand Down
4 changes: 2 additions & 2 deletions backend/src/main/java/gov/cdc/usds/simplereport/validators/CsvValidatorUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import static gov.cdc.usds.simplereport.db.model.PersonUtils.WORK_ENVIRONMENT_SNOMED;
import static gov.cdc.usds.simplereport.db.model.PersonUtils.getGenderIdentityAbbreviationMap;
import static gov.cdc.usds.simplereport.utils.DateTimeUtils.TIMEZONE_SUFFIX_REGEX;
import static gov.cdc.usds.simplereport.utils.DateTimeUtils.validTimeZoneIdMap;
import static gov.cdc.usds.simplereport.utils.DateTimeUtils.timezoneAbbreviationZoneIdMap;
import static java.util.stream.Collectors.toSet;
import static java.util.stream.Stream.concat;

Expand Down Expand Up @@ -462,7 +462,7 @@ public static List<FeedbackMessage> validateDateTimeZoneCode(ValueOrError input)
String value = input.getValue();
String timezoneCode = value.substring(value.lastIndexOf(' ')).trim();
if (!ZoneId.getAvailableZoneIds().contains(timezoneCode)
&& !validTimeZoneIdMap.containsKey(timezoneCode.toUpperCase())) {
&& !timezoneAbbreviationZoneIdMap.containsKey(timezoneCode.toUpperCase())) {
errors.add(
FeedbackMessage.builder()
.scope(ITEM_SCOPE)
Expand Down
37 changes: 31 additions & 6 deletions backend/src/test/java/gov/cdc/usds/simplereport/service/AddressValidationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package gov.cdc.usds.simplereport.service;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.doNothing;
Expand Down Expand Up @@ -69,15 +70,39 @@ void doesNotCorrectStreet() throws SmartyException, IOException {
}

@Test
void returnsCorrectZoneIdByLookup() {
void withSupportedTimezoneCommonName_returnsCorrectZoneIdByLookup() {
ArrayList<Candidate> results = new ArrayList<Candidate>();
results.add(getMockTimeZoneInfoResult());
results.add(getMockTimeZoneInfoResult("Central", -5.0));
Lookup lookup = mock(Lookup.class);
when(lookup.getResult()).thenReturn(results);

ZoneId zoneId = s.getZoneIdByLookup(lookup);

assertEquals(zoneId, ZoneId.of("US/Central"));
assertEquals(ZoneId.of("US/Central"), zoneId);
}

@Test
void withSupportedUtcOffsetTimezone_returnsCorrectZoneIdByLookup() {
ArrayList<Candidate> results = new ArrayList<Candidate>();
results.add(getMockTimeZoneInfoResult("UTC+9", +9.0));
Lookup lookup = mock(Lookup.class);
when(lookup.getResult()).thenReturn(results);

ZoneId zoneId = s.getZoneIdByLookup(lookup);

assertEquals(ZoneId.of("+9"), zoneId);
}

@Test
void withUnsupportedTimezoneCommonName_returnsNull() {
ArrayList<Candidate> results = new ArrayList<Candidate>();
results.add(getMockTimeZoneInfoResult("FakeZoneId", -25.0));
Lookup lookup = mock(Lookup.class);
when(lookup.getResult()).thenReturn(results);

ZoneId zoneId = s.getZoneIdByLookup(lookup);

assertNull(zoneId);
}

@Test
Expand All @@ -103,10 +128,10 @@ void throwsInvalidAddressExceptionOnEmptyResults() {
assertThrows(InvalidAddressException.class, () -> s.getTimezoneInfoByLookup(lookup));
}

private Candidate getMockTimeZoneInfoResult() {
private Candidate getMockTimeZoneInfoResult(String commonName, Double utcOffset) {
Metadata metadata = mock(Metadata.class);
when(metadata.getTimeZone()).thenReturn("Central");
when(metadata.getUtcOffset()).thenReturn(-5.0);
when(metadata.getTimeZone()).thenReturn(commonName);
when(metadata.getUtcOffset()).thenReturn(utcOffset);
when(metadata.obeysDst()).thenReturn(true);

Candidate result = mock(Candidate.class);
Expand Down
Loading