danmarsden / moodle-mod_attendance

Allows an attendance log to be kept in Moodle
https://moodle.org/plugins/mod_attendance
65 stars 157 forks source link

Attendance Session issue in different time-zone #683

Open ishara084 opened 1 year ago

ishara084 commented 1 year ago

Moodle default timezone is America/Newyork. My timezone is Asia/Kolkata standard timezone. When I create a session for selecting a future date, it always creating the session a day behind. 24 hours different.

For an example, I set a session on 20th May. When I hit 'Add', session is creating on 19th May. This is only happening when user is in another timezone rather than the system timezone. Even if we take time-different in to account it shouldn't be 24 different.

Can you confirm if this is an existing issue?

Thank you.

danmarsden commented 1 year ago

The attendance plugin uses Moodle's standard date handling apis - what time do you set the session "open" to? - could you include some screenshots of the time you set in the attendance session and how it is being displayed to the users?

I note there is a 9hr difference in timezones between New York and Kolkata - so an 8am Kolkata time could very well 11pm on the day before so it will show as starting on the day before.

ishara084 commented 1 year ago

@danmarsden Let me give an example.

mod_attendance version - 2022083108 Moodle version - 4.0

In my application, these are the configurations. Moodle Default timezone - 'America/New_York' Force timezone - 'Users can choose their own timezone' User's timezone - 'Asia/Kolkata'

Scenario I tested:

  1. Clicked on 'Add Session'
  2. Set Date - 20th May 2023, Time - from 07:00 to 10:00
  3. Hit 'Add'
  4. In the Session list, session has been created. But the date is Fri 19 May 2023 7AM - 10AM

I observed that this is not occurring when the user is in the same system timezone(Moodle default timezone - 'America/New_York' in this case). Even if we assumed it could be happening due to the timezone different it shouldn't be 24 hours difference.

Additionally - let's assume that, after this, I go to my profile and update my user timezone to 'America/New_York', session date changes to 'Thu 18 May 2023 9:30PM - 12:30AM'. Now that's correctly reflecting timezone hour different when I change my local timezone. But the issue is happening when initially create a session while my local timezone and system timezone is different.

Could you please check and confirm if it's the same for you. Appreciate your reply.

Thank you.

danmarsden commented 1 year ago

Hi @ishara084 thanks for providing that extra information - it allowed me to reproduce and track down the issue. I've pushed a fix into github here for the 4.0 and higher branches, next time I push out a release to the plugins database this fix will be included.

thanks!

wiktorwandachowicz commented 1 year ago

Hello @danmarsden - this solution essentially reverts #581 where there is another problem with Daily Saving Time days.

Essentially when user is exactly on the day when DST switch is made, one cannot save correct hour in a session (it is always 1 hour or 2 hours off - in European timezones). And on the next day the problem disappears.

If you take a look at make_timestamp() function it has support for timezones and DST as well - but attendance code doesn't seem to take advantage of it:

// lib/moodlelib.php:
function make_timestamp($year, $month=1, $day=1, $hour=0, $minute=0, $second=0, $timezone=99, $applydst=true) 

So by reverting previous solution, we come back to the other problem again. Please consider another patch which would cover both situations. Thank you!

wiktorwandachowicz commented 1 year ago

By the way, if not only a single session is created but series of sessions, then make_timestamp is still used in code. Just take a look at the top part of attendance_construct_sessions_data_for_add() function.

If it covers for example user's timezone 'Asia/Kolkata' - some sessions in series may be wrong. For another user in 'Europe/Warsaw' timezone for DST days - some sessions may also be wrong.

Hope this helps a little to understand the scale of this problem.

wiktorwandachowicz commented 1 year ago

After giving the problem some thinking, the solution may be a little different and should support both cases. The easiest would be to use Moodle's function userdate() instead of plain PHP's date() - like this:

diff --git a/locallib.php b/locallib.php
index 91a0db4..45c9f24 100644
--- a/locallib.php
+++ b/locallib.php
@@ -856,9 +856,9 @@ function attendance_construct_sessions_data_for_add($formdata, mod_attendance_st
     } else {
         $sess = new stdClass();
         $sess->sessdate = make_timestamp(
-            date("Y", $formdata->sessiondate),
-            date("m", $formdata->sessiondate),
-            date("d", $formdata->sessiondate),
+            userdate($formdata->sessiondate, '%Y'),
+            userdate($formdata->sessiondate, '%m'),
+            userdate($formdata->sessiondate, '%d'),
             $formdata->sestime['starthour'],
             $formdata->sestime['startminute']
         );

The $formdata->sessiondate is a correct timestamp. But the make_timestamp() requires year, month and day from user's perspective, including timezone from preferences. To take care of that userdate() can be used because it too is using user's timezone from preferences. In the end all functions used come from Moodle and match the same problem domain.

Experiment

To test the above change I have added seven attendance sessions in the same period of local time: 13:00-14:30. But each time before adding session I changed the timezone for the user - starting from the West and going to the East. Results are astonishing - just take a look at columns sessdate and timemodified:

Adminer screenshot 2023-05-31 111801

Analysis

When user is in Asia/Kolkata the dates and times displayed are very different. Only the marked session in the "current" timezone shows the time correctly. And in some timezones there is already next day displayed (which is correct, BTW):

Attendance Week Asia-Kolkata User profile - Timezone

But when the user is in Europe/Paris again the dates and times are different:

Attendance Week Europe-Paris

References

If necessary you may consult visual map of timezones, like I did:

https://upload.wikimedia.org/wikipedia/commons/8/88/World_Time_Zones_Map.png

danmarsden commented 1 year ago

missed this in the backlog - thanks for the further detail - shows we should ideally have some automated tests to cover this stuff and prevent future regressions too! - it looks like you have some internal development capabiltiies so feel free to submit a Pull request with the suggested changes at some point and I'll take a look! - thanks for the assist!

wiktorwandachowicz commented 1 year ago

All right, I will look into submitting a pull request.

I wonder if there is a good place for adding unit tests for different timezones. There are quite a number of behat tests plus one for external web services. But there is no place for other unit tests so far... Any ideas?

danmarsden commented 1 year ago

yeah - I wish we had a lot more unit tests in the attendance code... you'll probably just need to create some new classes for them if you manage to create some!