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

Fix Scheduled Reports sent one hour late in daylight saving timezones #10318

Closed
wants to merge 3 commits into from
Closed

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jul 16, 2016

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/Berlin with 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:

image

fixes #7658, refs #5873

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jul 16, 2016
@@ -29,11 +29,16 @@ function formSetEditReport(idReport) {

toggleReportType(report.type);

var hour = (24 + parseInt(report.hour) - timeZoneDifference) % 24;
Copy link
Member

Choose a reason for hiding this comment

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

this little logic can be refactored in a method

@mattab mattab added this to the 2.16.2 milestone Jul 18, 2016
@mattab
Copy link
Member

mattab commented Jul 18, 2016

Hi @sgiehl - Good find!

Review:

  • left a comment
  • are the file ScheduledReportsModel and its tests missing from the commit?

@mattab mattab modified the milestones: 2.16.x (LTS), 2.16.2, 2.16.3 Jul 18, 2016
@sgiehl
Copy link
Member Author

sgiehl commented Jul 18, 2016

are the file ScheduledReportsModel and its tests missing from the commit?

It uses the Model of ScheduledReports plugin, which should already be tested...

@mattab mattab modified the milestone: 2.16.3 Jul 25, 2016
@tsteur
Copy link
Member

tsteur commented Jul 30, 2016

I was wondering as well if there's a chance to create some tests for it but likely already kind of tested. Have you checked if maybe other scheduled reports have a similar issue?

@sgiehl
Copy link
Member Author

sgiehl commented Jul 31, 2016

Mobile messaging uses the same codebase for sending the reports, so the issue should then be fixed there, as well

$dateTimeZone = new \DateTimeZone($timezone);

$timeZoneDifference = -ceil($dateTimeZone->getOffset(new \DateTime()) / 3600);
$report['hour'] += $timeZoneDifference;
Copy link
Member Author

Choose a reason for hiding this comment

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

needs to be replaced with $report['hour'] = (24 + $report['hour'] + $timeZoneDifference) % 24;
to ensure the number doesn't become negative

@mattab
Copy link
Member

mattab commented Aug 23, 2016

Hi @adaqus

Did you review this Pull request and could you please confirm it works for you? ie. that it fixes the existing reports as expected, as well as fixes the issue for newly created reports?

@sgiehl
Copy link
Member Author

sgiehl commented Aug 25, 2016

I will replace this PR with a new one, as the source repo doesn't exist anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants