Fix Scheduled Reports sent one hour late in daylight saving timezones #10443
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sgiehl
added
not-in-changelog
| $dateTimeZone = new \DateTimeZone($siteTimezone); | ||
|
|
||
| $view->timeZoneDifference = -$dateTimeZone->getOffset(new \DateTime()); | ||
| $view->countWebsites = count(APISitesManager::getInstance()->getSitesIdWithAtLeastViewAccess()); |
There was a problem hiding this comment.
Wouldn't this cause a memory hog, if there are lots of sites added?
There was a problem hiding this comment.
If so that would have been there before, as well. I've only added the detection for timezone offset.
|
I think it looks OK in general 👍 |
sgiehl
force-pushed
the
7658_scheduled_reports
branch
3 times, most recently
from
603505e
to
7c6e8c4
Compare
|
@mkrakiewicz I've updated the code according to your feedback. |
|
We got a similar report:
Do you think this PR will fix such issue as well, that a report scheduled for 5pm later today in UTC+12 will run today and not tomorrow (in UTC+12 timezone) ? |
|
Yes, that should fix all time issues regarding scheduled reports. The report hour will always be saved in UTC. And as the scheduling is also done in UTC, the time should always be correct then. |
|
I'll update the PR later, so it will get mergable again. |
sgiehl
force-pushed
the
7658_scheduled_reports
branch
2 times, most recently
from
e52f560
to
8b5d83c
Compare
| @@ -122,10 +131,12 @@ function initManagePdf() { | |||
|
|
|||
| apiParameters.parameters = getReportParametersFunctions[apiParameters.reportType](); | |||
|
|
|||
| var hour = adjustHourToTimezone($('#report_hour').val()); | |||
There was a problem hiding this comment.
Please provide -timeZoneDifference as a second argument for adjustHourToTimezone().
sgiehl
force-pushed
the
7658_scheduled_reports
branch
from
8b5d83c
to
f40956a
Compare
sgiehl
force-pushed
the
7658_scheduled_reports
branch
from
f40956a
to
5ef3110
Compare
mattab
added a commit
that referenced
this pull request
Sep 26, 2016
* Fix Scheduled Reports sent one hour late in daylight saving timezones (#10443) * convert hour to send report to/from UTC, to ensure it isn't affected by daylight savings * adds update script to change existing scheduled reports to use utc time * code improvement * adds missing param * Added new event Archiving.makeNewArchiverObject to allow customising plugin archiving (#10366) * added hook to alllow plugin archiving prevention * cr code style notes * reworked PR to fit CR suggestions * added PHPDoc for hook * Event description more consistent * UI tests: minor changes * Adds test checking if all screenshots are stored in lfs * removed screenshots not stored in lfs * readds screenshots to lfs * 2.16.3-b4
mattab
added a commit
that referenced
this pull request
Sep 29, 2016
* Fix depraction test: use assertDeprecatedMethodIsRemovedInPiwik3 * Fix Scheduled Reports sent one hour late in daylight saving timezones (#10443) * convert hour to send report to/from UTC, to ensure it isn't affected by daylight savings * adds update script to change existing scheduled reports to use utc time * code improvement * adds missing param * Added new event Archiving.makeNewArchiverObject to allow customising plugin archiving (#10366) * added hook to alllow plugin archiving prevention * cr code style notes * reworked PR to fit CR suggestions * added PHPDoc for hook * Event description more consistent * UI tests: minor changes * Comment out Visitor Log UI tests refs #10536 * Adds test checking if all screenshots are stored in lfs * removed screenshots not stored in lfs * readds screenshots to lfs * 2.16.3-b4 * Issue translation updates against 2.x-dev * language update * Fix bug in widget list remove where the JSON object becomes array * 2.16.3-rc1 * console command custom-piwik-js:update should work when directory is writable and file does not exist yet (#10576) * followup #10449 * Fix test (cherry picked from commit fac3d63) * Prevent chmod(): No such file or directory * Automatically update all marketplace plugins when updating Piwik (#10527) * update plugins and piwik at the same time * make sure plugins are updated with piwik * use only one try/catch * reload plugin information once it has been installed * make sure to clear caches after an update * fix ui tests * make sure to use correct php version without any extras * Additional informations passed in the hook "isExcludedVisit" (issue #10415) (#10564) * Additional informations passed in the hook "isExcludedVisit" (issue #10415) * Added better description to the new parameters * Update VisitExcluded.php * Remove two parameters not needed as better to use the Request object * Update VisitExcluded.php * remove extra two parameters in VisitExcluded constructor to prevent confusion (#10593) * Update our libs to latest #10526 * Update composer libraries to latest #10526 * Update log analytics to latest * When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update (#10423) * When updating the config file failed (or when any other file is not writable...), the Updater (for core or plugins) will now automatically throw an error and cancel the update * add integration test to check the correct exception is thrown when config not writabel * New integration test for updater * Make test better * When opening the visitor profile, do not apply the segment (#10533) * When opening the visitor profile, do not apply the segment * added ui test for profile but does work for me * next try to make ui test work * add expected screenshot * added missing doc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Replaces #10318]
This PR aims to fix the issue, that scheduled reports might be sent one hour to late / to early if the reports are saved for an site having a timezone using daylight savings.
The problem occurs because the "hour" the report should be sent at is currently saved in the timezone of the site. That means a new report with the timezone like e.g.
Europe/Berlinwith the setting to send the report at 8 o'clock would be one hour earlier/later when the daylight saving changes.Internal Piwik uses UTC for running the tasks, so it tries to adjust the saved hour using the site's timezone difference. At the moment that would be two hours for
Europe/Berlin, so the report would sent at 6 o'clock UTC. When the daylight saving changes again, the timezone difference will be only an hour. So Piwik will then reduce the saved hour value only by one to get the UTC time, which will be 7 o'clock (which is one hour later).To avoid this in the future, I changed the current behavior to not save the hour value in the site's timezone anymore, but to use the UTC time instead. Doing this, the value can't be affected by daylight saving anymore. I also added an update script to update existing reports to have UTC values.
When creating/editing an report for an site using a timezone with a difference to UTC, Piwik will now should the UTC, aswell:
fixes #7658, refs #5873