Moodle
  1. Moodle
  2. MDL-42597

New maintenance mode countdown counts down to 0 instead of 1

    Details

    • Testing Instructions:
      Hide

      Test 1

      1. Run php admin/cli/maintenance.php --enablelater=2
      2. Check site and you should "This site will be switched to maintenance mode in 1 mins and xx secs" (on bottom right of page)
      3. Make sure timer is counting down.
      4. When 30 sec left color of msg should change.
      5. After time elapse msg should change to "The site is undergoing maintenance and is currently not available"

      Test 2

      1. Test Maintenance mode timer is displayed properly on all core themes (especially z-index)
      Show
      Test 1 Run php admin/cli/maintenance.php --enablelater=2 Check site and you should "This site will be switched to maintenance mode in 1 mins and xx secs" (on bottom right of page) Make sure timer is counting down. When 30 sec left color of msg should change. After time elapse msg should change to "The site is undergoing maintenance and is currently not available" Test 2 Test Maintenance mode timer is displayed properly on all core themes (especially z-index)
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      wip-mdl-42597
    • Story Points:
      13
    • Rank:
      54425
    • Sprint:
      BACKEND Sprint 7

      Description

      If I enable the new maintenance mode countdown with
      sudo -u apache php admin/cli/maintenance.php --enablelater=2
      there is a countdown messagebox at the top of every Moodle page until the maintenance mode is enabled.

      Unfortunately, the message in this message box counts down to 0 instead of 1, so in the last minute before maintenance mode activation, there is a rather confusing message saying
      "Site is switching to maintenance mode in 0 minutes".

      I would like to propose to add 1 to the minutes in lib/outputrenderers.php, function maintenance_warning(), to solve this cosmetic issue.

        Issue Links

          Activity

          Hide
          Rajesh Taneja added a comment -

          Thanks for reporting this Alexander,

          I've put that on the backlog.

          In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.

          Show
          Rajesh Taneja added a comment - Thanks for reporting this Alexander, I've put that on the backlog. In the meantime feel free to help us work on this issue. If you are able to provide a patch or links to your Git repository branch, please add a patch label so we will spot it.
          Hide
          Alexander Bias added a comment -

          Dear Rajesh,

          I think this can be easily patched by changing in lib/outputrenderers.php, function maintenance_warning(), the line

          $output .= get_string('maintenancemodeisscheduled', 'admin', (int)(($CFG->maintenance_later-time())/60));

          to

          $output .= get_string('maintenancemodeisscheduled', 'admin', (int)(($CFG->maintenance_later-time())/60)+1);

          Alex

          Show
          Alexander Bias added a comment - Dear Rajesh, I think this can be easily patched by changing in lib/outputrenderers.php, function maintenance_warning(), the line $output .= get_string('maintenancemodeisscheduled', 'admin', (int)(($CFG->maintenance_later-time())/60)); to $output .= get_string('maintenancemodeisscheduled', 'admin', (int)(($CFG->maintenance_later-time())/60)+1); Alex
          Hide
          Rajesh Taneja added a comment -

          Thanks for the patch Alexander,

          I have created github branches on your behalf and pushing it for peer-review.

          Show
          Rajesh Taneja added a comment - Thanks for the patch Alexander, I have created github branches on your behalf and pushing it for peer-review.
          Hide
          Petr Škoda added a comment -

          Hi, we have discussed it already today during the meeting: the adding of 1 minute is incorrect because it makes user believe there is more time before the maintenance mode kicks in. I proposed to use a new string when time < 1min explaining that users must save whatever the are doing and that in less than 60 seconds the site goes down.

          Show
          Petr Škoda added a comment - Hi, we have discussed it already today during the meeting: the adding of 1 minute is incorrect because it makes user believe there is more time before the maintenance mode kicks in. I proposed to use a new string when time < 1min explaining that users must save whatever the are doing and that in less than 60 seconds the site goes down.
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr,

          I have modified the patch to show different string.

          Added Helen as watcher to get her feedback on strings (maintenancemodeisscheduledwithinonemin and maintenancemodeisscheduled)

          Show
          Rajesh Taneja added a comment - Thanks Petr, I have modified the patch to show different string. Added Helen as watcher to get her feedback on strings (maintenancemodeisscheduledwithinonemin and maintenancemodeisscheduled)
          Hide
          Helen Foster added a comment -

          Rajesh, thanks for asking for my feedback on the language strings. I think they are fine, though could just be improved a tiny bit to:

          'This site will be switched to maintenance mode in {$a} minutes.'
          'This site will be switched to maintenance mode in less than 1 minute.'

          The issue reminded me of the countdown timer we recently implemented on all of our demo sites which reset each hour - MDLSITE-2570. I don't know whether a JavaScript timer with seconds is possible in this case?

          Show
          Helen Foster added a comment - Rajesh, thanks for asking for my feedback on the language strings. I think they are fine, though could just be improved a tiny bit to: 'This site will be switched to maintenance mode in {$a} minutes.' 'This site will be switched to maintenance mode in less than 1 minute.' The issue reminded me of the countdown timer we recently implemented on all of our demo sites which reset each hour - MDLSITE-2570 . I don't know whether a JavaScript timer with seconds is possible in this case?
          Hide
          Rajesh Taneja added a comment -

          Thanks for the help Helen,

          MDLSITE-2570 looks grt, so I have replicated similar behaviour for maintenance mode msg.

          1. Replaced string to include seconds and changes suggested by Helen
          2. Applied style to display nicely.
          3. Added JS to show count down.
          Show
          Rajesh Taneja added a comment - Thanks for the help Helen, MDLSITE-2570 looks grt, so I have replicated similar behaviour for maintenance mode msg. Replaced string to include seconds and changes suggested by Helen Applied style to display nicely. Added JS to show count down.
          Hide
          Petr Škoda added a comment -

          Hi, the JS coding style looks a bit obsolete, I suppose it would be a lot better to use the new YUI module for this. Anyway a few issue related to the legacy coding style:

          1. /lib/javascript-static.js is always included
          2. always use M.util.get_string() - do not try to do its job

          My -1 for backport, this is relatively big improvement, it should go into master only in my opinion.

          Andrew Nicols: ideas, how hard would it be to use the recommended new coding style?

          Show
          Petr Škoda added a comment - Hi, the JS coding style looks a bit obsolete, I suppose it would be a lot better to use the new YUI module for this. Anyway a few issue related to the legacy coding style: /lib/javascript-static.js is always included always use M.util.get_string() - do not try to do its job My -1 for backport, this is relatively big improvement, it should go into master only in my opinion. Andrew Nicols : ideas, how hard would it be to use the recommended new coding style?
          Hide
          Rajesh Taneja added a comment -

          Thanks Petr,

          I have modified JS integrating your suggestions.

          Requesting another round of review.

          Show
          Rajesh Taneja added a comment - Thanks Petr, I have modified JS integrating your suggestions. Requesting another round of review.
          Hide
          Petr Škoda added a comment -

          please add instructions to test the z-index in more themes, there is also one capital letter problem in comment and whitespace issue

          the rest looks good, +1 for integration when fixed

          I guess it would help integrators if they saw a screenshot of the result, I think this might be handy and safe improvement for 2.6 stable.

          Show
          Petr Škoda added a comment - please add instructions to test the z-index in more themes, there is also one capital letter problem in comment and whitespace issue the rest looks good, +1 for integration when fixed I guess it would help integrators if they saw a screenshot of the result, I think this might be handy and safe improvement for 2.6 stable.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the review Petr,

          1. Fixed typo
          2. Added screen shots.
          3. Updated test instructions to test in all core themes for z-index.

          Pushing it for integration.

          Show
          Rajesh Taneja added a comment - Thanks for the review Petr, Fixed typo Added screen shots. Updated test instructions to test in all core themes for z-index. Pushing it for integration.
          Hide
          Dan Poltawski added a comment -

          Hi Raj,

          This seems nice, but i'm confused by why we need the zindex in base and not bootstrap base and indeed the need for the z-index completely?

          Show
          Dan Poltawski added a comment - Hi Raj, This seems nice, but i'm confused by why we need the zindex in base and not bootstrap base and indeed the need for the z-index completely?
          Hide
          Rajesh Taneja added a comment -

          Hello Dan,

          As this is first div which gets rendered by browser, anything rendered after this is visible above it (like blocks).
          I didn't observe this issue in clean and that's why didn't add z-index for clean.

          I should have mentioned my approach for resolving this issue:
          It was a simple issue which could be resolved by changing one string, but after looking at the code and getting introduced to what is happening on demo site (for reset), it was discussed with Petr to improve it and make it more usable.

          Currently it has been decided not to include hr and day, as not sure of usage. If we come across any improvement for hr/day then we can look at supporting it.

          Show
          Rajesh Taneja added a comment - Hello Dan, As this is first div which gets rendered by browser, anything rendered after this is visible above it (like blocks). I didn't observe this issue in clean and that's why didn't add z-index for clean. I should have mentioned my approach for resolving this issue: It was a simple issue which could be resolved by changing one string, but after looking at the code and getting introduced to what is happening on demo site (for reset), it was discussed with Petr to improve it and make it more usable. Currently it has been decided not to include hr and day, as not sure of usage. If we come across any improvement for hr/day then we can look at supporting it.
          Hide
          Rajesh Taneja added a comment -

          After taking with Jason Fowler, it was decided that z-index = 1 with moodle-has-zindex class on div should be best for this. As this should be visible on all pages, but there is no need to obstruct filepicker or help containers which have higher z-index.

          Modified patch to use z-index=1 in both standard and bootstrap theme.

          Show
          Rajesh Taneja added a comment - After taking with Jason Fowler , it was decided that z-index = 1 with moodle-has-zindex class on div should be best for this. As this should be visible on all pages, but there is no need to obstruct filepicker or help containers which have higher z-index. Modified patch to use z-index=1 in both standard and bootstrap theme.
          Hide
          Jason Fowler added a comment -

          Thanks raj - the details as to why we use moodle-has-zindex are discussed here: https://moodle.org/mod/forum/discuss.php?d=243747

          Show
          Jason Fowler added a comment - Thanks raj - the details as to why we use moodle-has-zindex are discussed here: https://moodle.org/mod/forum/discuss.php?d=243747
          Hide
          Dan Poltawski added a comment -

          Thanks Raj, integrated to master.

          Note that the integration servers discovered stale yui files which you had added when the yui module was added. Please be careful to only commit the needed production files.

          Show
          Dan Poltawski added a comment - Thanks Raj, integrated to master. Note that the integration servers discovered stale yui files which you had added when the yui module was added. Please be careful to only commit the needed production files.
          Hide
          Marina Glancy added a comment -

          works good on all themes

          Message is above all elements on all themes except popups opened by filepicker (which does not look like a reason to fail the test)

          Show
          Marina Glancy added a comment - works good on all themes Message is above all elements on all themes except popups opened by filepicker (which does not look like a reason to fail the test)
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions, this change is now upstream!

          “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

          Show
          Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile