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
Custom reports: show server hours in site's timezone #16565
Conversation
$siteSearch = 1, $searchKeywordParameters = null, | ||
$searchCategoryParameters = null, $timezone = 'Pacific/Auckland'); | ||
$hourConverted = VisitLastActionTime::convertHourToHourInSiteTimezone(5, $idSite); | ||
$this->assertEquals(18, $hourConverted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity: Will this test fail when the time is changed due to daylight savings? Guess Auckland has summer/winter time as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it will. I reckon it'll be easy to change though then. Do you maybe know a timezone that doesn't have daylight savings? happy to change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asia shouldn't have daylight saving afaik. Guess you could use eg Asia/Jakarta
(UTC+7)
} catch (\Exception $e) { | ||
// ignore any error eg if values are wrong... | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed to use the date/period if we're just getting the hour later? Could we just use a dummy date or today()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diosmosis We'll try to use the date in case of daylight savings to make sure it is being considered. If we just used today then it wouldn't always be correct (see method description)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's unfortunate.
fix dev-2020
Wasn't sure where to put the method. Initially had it in the formatter but it's not formatting the hour it is converting. Also re the behaviour with period/date being used it might be actually ok to put this into a dimension class.
Once merged I'd also adjust eg in Media Analytics