Moodle
  1. Moodle
  2. MDL-27562

Timezone option not considered in dateselector and datetimeselector form element

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.12, 2.0
    • Fix Version/s: 2.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide

      run php unit test for dateselector and datetimeselector

      phpunit dateselector_form_element_testcase lib/form/tests/dateselector_test.php
      phpunit datetimeselector_form_element_testcase lib/form/tests/datetimeselector_test.php
      
      Show
      run php unit test for dateselector and datetimeselector phpunit dateselector_form_element_testcase lib/form/tests/dateselector_test.php phpunit datetimeselector_form_element_testcase lib/form/tests/datetimeselector_test.php
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-27562
    • Rank:
      17227

      Description

      /lib/form/dateselector.php doesn't pass configurable option 'timezone' to function usergetdate() as 2nd parameter.

      In function: onQuickFormEvent($event, $arg, &$caller)
      Change:
      
          if (!is_array($value)) {
              $currentdate = usergetdate($value);
              $value = array(
      
      To:
      
          if (!is_array($value)) {
              $currentdate = usergetdate($value, $this->_options['timezone']);
              $value = array(
      
      1. test.php
        0.8 kB
        Rajesh Taneja
      2. test-dst.php
        1.0 kB
        Rajesh Taneja

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment - - edited

          Thanks for reporting this Brent, I am adding this to next sprint.

          Show
          Rajesh Taneja added a comment - - edited Thanks for reporting this Brent, I am adding this to next sprint.
          Hide
          Rajesh Taneja added a comment -

          In 22 and 21 (Stable branches), usergetdate was not accepting applydst option. Hence applydst option is added in master branch only(change in api is acceptable in master only).

          Show
          Rajesh Taneja added a comment - In 22 and 21 (Stable branches), usergetdate was not accepting applydst option. Hence applydst option is added in master branch only(change in api is acceptable in master only).
          Hide
          Jason Fowler added a comment -

          Looks good Raj

          Show
          Jason Fowler added a comment - Looks good Raj
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Rajesh Taneja added a comment -

          branches re-based

          Show
          Rajesh Taneja added a comment - branches re-based
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          There is a bug with your code.
          In your test case for master try removing the default values and instead setting a value for the fields through $mform->set_value().
          By doing that I generated several notes like:

          Notice: Undefined index: month in /var/www/integration/lib/form/dateselector.php on line 234

          Looking at the output and debugging the values I can see that before your change the values for day and month are not prefixed by a 0 for single digits, and after your changes sometimes they are and sometimes they are not. Looks like a bug with usergetdate that will be highlighted by this change. Not necessarily related. But definitely a blocker.
          On the flip side I asked Dan to have a look at this issue for me as I am very suspicious of the changes and he was aware of MDL-32234, the exact issue I had noticed and one that you had also fixed. Dan has now linked the issue.

          I turned this up while trying to research the effects of this change.
          Its a small but potentially significant change in that we are now using timezone when updating a value, and using it again when returning the value.
          To compound it this change is to be made to a pretty well used bit of code and the only test case is a test script that worked even when there were issues.
          Could you please double check everything for me, and find some test cases in Moodle. I assume those dst and timezone arguments are being used and it would be great to identify existing areas that do use them and focus on those for testing.

          I've chosen to leave this in integration atm, however I will ask Eloy to look at it when he comes online as he is much more familiar with timezone handling than I am.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, There is a bug with your code. In your test case for master try removing the default values and instead setting a value for the fields through $mform->set_value(). By doing that I generated several notes like: Notice: Undefined index: month in /var/www/integration/lib/form/dateselector.php on line 234 Looking at the output and debugging the values I can see that before your change the values for day and month are not prefixed by a 0 for single digits, and after your changes sometimes they are and sometimes they are not. Looks like a bug with usergetdate that will be highlighted by this change. Not necessarily related. But definitely a blocker. On the flip side I asked Dan to have a look at this issue for me as I am very suspicious of the changes and he was aware of MDL-32234 , the exact issue I had noticed and one that you had also fixed. Dan has now linked the issue. I turned this up while trying to research the effects of this change. Its a small but potentially significant change in that we are now using timezone when updating a value, and using it again when returning the value. To compound it this change is to be made to a pretty well used bit of code and the only test case is a test script that worked even when there were issues. Could you please double check everything for me, and find some test cases in Moodle. I assume those dst and timezone arguments are being used and it would be great to identify existing areas that do use them and focus on those for testing. I've chosen to leave this in integration atm, however I will ask Eloy to look at it when he comes online as he is much more familiar with timezone handling than I am. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          (stopping review until Eloy has a chance to look at this)

          Show
          Sam Hemelryk added a comment - (stopping review until Eloy has a chance to look at this)
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,
          As you mentioned bug is coming from timezone, and have been reported in MDL-32234.
          Can you please try testcase without the patch and AFAIK you should observe the same error.

          Show
          Rajesh Taneja added a comment - Thanks Sam, As you mentioned bug is coming from timezone, and have been reported in MDL-32234 . Can you please try testcase without the patch and AFAIK you should observe the same error.
          Hide
          Sam Hemelryk added a comment - - edited

          Haha yes, I was about to fail this because of that linked issue, Dan saved it from the rejection pile because he was aware of that other issue and after reading about it and testing it we confirmed it was the same issue.
          I tested with your test files with and without the changes on MDL-32234 applied. The problem with those test files is that they are of very limited scope considering how widely those two fields are used.
          I would be a lot more confident with this issue if there was an actual test in Moodle, one that involved getting information from the db, setting the form with it, editing it in the form, and viewing the result in normal output would be great.

          Show
          Sam Hemelryk added a comment - - edited Haha yes, I was about to fail this because of that linked issue, Dan saved it from the rejection pile because he was aware of that other issue and after reading about it and testing it we confirmed it was the same issue. I tested with your test files with and without the changes on MDL-32234 applied. The problem with those test files is that they are of very limited scope considering how widely those two fields are used. I would be a lot more confident with this issue if there was an actual test in Moodle, one that involved getting information from the db, setting the form with it, editing it in the form, and viewing the result in normal output would be great.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, basically what I don't get here... is which usecase is this trying to fix/improve.

          If I'm not wrong, current applicable timezone is always applied by usergetdate(), so why do we need to be able to pass any timezone/dstapply to it?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, basically what I don't get here... is which usecase is this trying to fix/improve. If I'm not wrong, current applicable timezone is always applied by usergetdate(), so why do we need to be able to pass any timezone/dstapply to it? Ciao
          Hide
          Rajesh Taneja added a comment -

          At present, there is no use-case in moodle.
          It's a loss of functionality for datetimeselector as it has timezone options, but doesn't consider them.

          If user want to create date-time converter to check student timezone etc., He/she have to do this using code, whereas datetime element should support this (as per current options in form element)

          Show
          Rajesh Taneja added a comment - At present, there is no use-case in moodle. It's a loss of functionality for datetimeselector as it has timezone options, but doesn't consider them. If user want to create date-time converter to check student timezone etc., He/she have to do this using code, whereas datetime element should support this (as per current options in form element)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          After chatting with Rajesh a bit about this, we have agreed to:

          1) accept the 1st commit (add support for timezone option, defaulting to 99 to ensure everything continues working without problem).
          2) explain/document the "timezones" option a bit more stating is not needed for "normal" operations at all. Only "border" cases like one "time converter" may need to use this.
          3) delete the applydst option completely and change any use to true. I has no sense to perform any date conversion ignoring dst changes.
          4) build some tests between 2 well know timezones to demo it's working as expected. Instantiate forms, look for values... whatever can be done programmatically.
          5) Sugest the attached tests above as something to be added to "selenium" tests to TimB. To ensure it "visually" works ok.

          All this only for master. Keep stables without modification. If anybody wants to implement any "border" use case like that, it will need to be 2.3 and upwards.

          Thanks Raj!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited After chatting with Rajesh a bit about this, we have agreed to: 1) accept the 1st commit (add support for timezone option, defaulting to 99 to ensure everything continues working without problem). 2) explain/document the "timezones" option a bit more stating is not needed for "normal" operations at all. Only "border" cases like one "time converter" may need to use this. 3) delete the applydst option completely and change any use to true. I has no sense to perform any date conversion ignoring dst changes. 4) build some tests between 2 well know timezones to demo it's working as expected. Instantiate forms, look for values... whatever can be done programmatically. 5) Sugest the attached tests above as something to be added to "selenium" tests to TimB. To ensure it "visually" works ok. All this only for master. Keep stables without modification. If anybody wants to implement any "border" use case like that, it will need to be 2.3 and upwards. Thanks Raj!
          Hide
          Hubert Chathi added a comment -

          In regards to use case, IIRC, this came up in ELIS when we were doing scheduled reports (based on http://docs.moodle.org/dev/Scheduled_Tasks_Proposal – which, BTW, someone from RL should send over the code that was written for that ), which allowed reports to be scheduled for different times, and would allow a user to select the timezone for the schedule, which could be different from the user's default timezone (especially if there are multiple admins, in different timezones).

          Show
          Hubert Chathi added a comment - In regards to use case, IIRC, this came up in ELIS when we were doing scheduled reports (based on http://docs.moodle.org/dev/Scheduled_Tasks_Proposal – which, BTW, someone from RL should send over the code that was written for that ), which allowed reports to be scheduled for different times, and would allow a user to select the timezone for the schedule, which could be different from the user's default timezone (especially if there are multiple admins, in different timezones).
          Hide
          Rajesh Taneja added a comment -

          Thanks Everyone,

          will update the branches

          Show
          Rajesh Taneja added a comment - Thanks Everyone, will update the branches
          Hide
          Rajesh Taneja added a comment -

          I was looking at this again, and found few links, which state about DST usage.
          Not sure, if removing applydst option is a good idea.
          Sorry Eloy, I am opening this again for discussion here.

          http://www.webexhibits.org/daylightsaving/congressionalResearchService.html

          Allowed states split by time zone boundaries to exempt the entire state or that part of the state in a different time zone from DST. The states affected are: Alaska, Florida, Idaho, Indiana, Kansas, Kentucky, Oregon, Nebraska, North Dakota, South Dakota, Tennessee, and Texas.
          

          Looking at https://sas.elluminate.com/site/external/isoTimeZoneConverter (user gets the option to use dst, while converting time)

          Show
          Rajesh Taneja added a comment - I was looking at this again, and found few links, which state about DST usage. Not sure, if removing applydst option is a good idea. Sorry Eloy, I am opening this again for discussion here. http://www.webexhibits.org/daylightsaving/congressionalResearchService.html Allowed states split by time zone boundaries to exempt the entire state or that part of the state in a different time zone from DST. The states affected are: Alaska, Florida, Idaho, Indiana, Kansas, Kentucky, Oregon, Nebraska, North Dakota, South Dakota, Tennessee, and Texas. Looking at https://sas.elluminate.com/site/external/isoTimeZoneConverter (user gets the option to use dst, while converting time)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think we should keep the DST completely out from the equation. Mainly because there are 2 types of timezones covering that:

          • Real timezones: For example (note the example is invented) if in Spain there is one region not working with dst, or having a different one from the "main" one (Europe/Madrid), then surely there will be another timezone (Europe/THE_REGION_OF_SPAIN) with the timezone defined that way. It sounds to me that, for some regions in the USA there were already specific timezones for the exceptions.
          • UTC timezones: If you don't want to use DSTs, then simply use UTC+-xxx timezones. They don't have that.

          So, while it's possible to find some "rare" (unreal?) use cases requiring to avoid applying DSTs, I think 99.99% of times it's not needed (in fact can be more a trouble-maker than an nice option). Also, note this is about our default time/date form elements, if anyone wants to extend them to support DST, np here, same if someone wants the moon-phases or any other time/date related information, lol.

          But not really needed for our standard forms, IMO. Optional timezones support, yes. Dsts one, no.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think we should keep the DST completely out from the equation. Mainly because there are 2 types of timezones covering that: Real timezones: For example (note the example is invented) if in Spain there is one region not working with dst, or having a different one from the "main" one (Europe/Madrid), then surely there will be another timezone (Europe/THE_REGION_OF_SPAIN) with the timezone defined that way. It sounds to me that, for some regions in the USA there were already specific timezones for the exceptions. UTC timezones: If you don't want to use DSTs, then simply use UTC+-xxx timezones. They don't have that. So, while it's possible to find some "rare" (unreal?) use cases requiring to avoid applying DSTs, I think 99.99% of times it's not needed (in fact can be more a trouble-maker than an nice option). Also, note this is about our default time/date form elements, if anyone wants to extend them to support DST, np here, same if someone wants the moon-phases or any other time/date related information, lol. But not really needed for our standard forms, IMO. Optional timezones support, yes. Dsts one, no. Ciao
          Hide
          Rajesh Taneja added a comment -

          Hello Jason,

          Can you please review this code again.
          Thanks.

          Show
          Rajesh Taneja added a comment - Hello Jason, Can you please review this code again. Thanks.
          Hide
          Jason Fowler added a comment -

          looks good to me Raj

          Show
          Jason Fowler added a comment - looks good to me Raj
          Hide
          Rajesh Taneja added a comment -

          Changed "Issue type" from bug to improvement.

          Show
          Rajesh Taneja added a comment - Changed "Issue type" from bug to improvement.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jason and Eloy

          Show
          Rajesh Taneja added a comment - Thanks Jason and Eloy
          Hide
          Sam Hemelryk added a comment -

          Oh I like this solution a LOT more, it makes sense!

          Show
          Sam Hemelryk added a comment - Oh I like this solution a LOT more, it makes sense!
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Raj, this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Tested during integration review/

          Show
          Sam Hemelryk added a comment - Tested during integration review/
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          Just wondering if we have any open issue to create similar unit test for other form elements.

          Show
          Rajesh Taneja added a comment - Thanks Sam, Just wondering if we have any open issue to create similar unit test for other form elements.
          Hide
          Sam Hemelryk added a comment -

          Probably not, best ask Petr I think he is perhaps planning to write more tests although I don't know whether he would worry about form elements and/or whether he has created any issues.

          Show
          Sam Hemelryk added a comment - Probably not, best ask Petr I think he is perhaps planning to write more tests although I don't know whether he would worry about form elements and/or whether he has created any issues.
          Hide
          Rajesh Taneja added a comment -

          Thanks again
          Will ask Petr.

          Show
          Rajesh Taneja added a comment - Thanks again Will ask Petr.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao
          Hide
          Helen Foster added a comment -

          Please could anyone advise on exactly what needs documenting for this issue or feel free to add something to http://docs.moodle.org/23/en/Location or perhaps it's more a dev docs required issue?

          Show
          Helen Foster added a comment - Please could anyone advise on exactly what needs documenting for this issue or feel free to add something to http://docs.moodle.org/23/en/Location or perhaps it's more a dev docs required issue?
          Hide
          Rajesh Taneja added a comment -

          Hello Helen,

          date_selector and date_time_selector needs updating.

          We just need to remove

          'applydst'  => true,
          

          from the array, as it is not supported.

          FYI:
          I have updated the docs.

          Show
          Rajesh Taneja added a comment - Hello Helen, date_selector and date_time_selector needs updating. We just need to remove 'applydst' => true, from the array, as it is not supported. FYI: I have updated the docs.
          Hide
          Helen Foster added a comment -

          Thanks Raj, I'll remove the docs_required label now.

          Show
          Helen Foster added a comment - Thanks Raj, I'll remove the docs_required label now.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: