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.

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

Clarify EnvironmentService handling of invalid xhAppTimeZone config #380

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

minusleaf
Copy link
Contributor

Fixes the issue where the log would never call because configZoneId given a bad xhAppTimeZone value would return the defaultZone. This would then be considered valid as an available ID by the conditional and therefore not log if we use the default or not.

@minusleaf minusleaf requested a review from amcclain August 5, 2024 18:06
@minusleaf minusleaf changed the title App tz logging App TimeZone logging Aug 5, 2024
@minusleaf minusleaf closed this Aug 5, 2024
@minusleaf minusleaf reopened this Aug 5, 2024
@@ -89,10 +89,11 @@ class EnvironmentService extends BaseService {
//---------------------
private TimeZone calcAppTimeZone() {
def defaultZone = 'UTC',
configZoneId = configService.getString('xhAppTimeZone', defaultZone)
configZoneId = configService.getString('xhAppTimeZone')
Copy link
Member

Choose a reason for hiding this comment

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

It's OK (and good) to remove this default here, but note that's only because we always ensure that the xhAppTimeZone config is created - that happens in Bootstrap.groovy. Before we had that code, this second arg was important as otherwise configService would throw if the config was not found in the DB.

Given the bootstrap routine, we know the config will always be there, but we now are defending against it being some unexpected/unknown zone.

As we discussed (and for the record here), TimeZone.getTimeZone('someBadZoneString') actually does default to GMT, which is functionally the same as UTC although not identical in Java world (different actual TimeZone objects, same offset).

So the code we had before would work, but it was a bit odd and did not get you exactly what it promised, so I'm still happy to have our improvement here.

CHANGELOG.md Outdated
### 🐞 Bug Fixes

* Fix in `EnvironmentService` to show error log when passed in an invalid time zone ID by setting to
default time zone after the check.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite correct - we were always printing that error log, the code now is just a bit more explicit and exact in terms of how it is falling back to the default.

But TBH I don't think we need a CL entry here - this is a very minor code cleanup, not really a bug, and no functional difference. Pls remove it and then happy to merge. Thanks!

@amcclain amcclain changed the title App TimeZone logging Clarify EnvironmentService handling of invalid xhAppTimeZone config Aug 5, 2024
@amcclain amcclain merged commit a66cb04 into develop Aug 5, 2024
4 checks passed
@amcclain amcclain deleted the appTzLogging branch August 5, 2024 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants