Moodle
  1. Moodle
  2. MDL-42504

If the quiz auto-save detects that the connection to the server has been lost, we should warn students

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.2
    • Fix Version/s: 2.5.4, 2.6.1
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      To test this, you need to be able to simulate losing internet connection while attempting a quiz. As well as the obvious

      • pull out the network cable, or
      • disconnect wifi,
        you can also
      • edit mod/quiz/autosave.ajax.php and add a syntax error. (e.g. random letter near the top).

      0. Turn on quiz auto-save on Site administration ► Plugins ► Activity modules ► Quiz.

      In fact, you may want to force the value in config.php, so that you can have a shorter delay than 1 minute:

      $CFG->forced_plugin_settings = array('quiz' => array('autosaveperiod' => 10));
      

      1. Create a simple quiz (e.g. one shortanswer question.)

      2. Attempt the quiz as a student, and open the Firebug console (or eqivialent.) If you have developer debug on, that will show you what is going on.

      3. Input a reasonse to the question and wait. It should be auto-saved.

      4. Now simulate losing internet.

      5. Change your response and wait.

      6. This time the save should fail, and the alert should appear at the top of the screen. In firebug, you should see it continue to try to auto-save every so often (the configured delay.)

      7. Simulate re-connecting to the internet.

      8. after the delay, the alert should change to a green all is well message.

      9. Change you response again, and wait some more. The green OK message will disappear on the next auto-save.

      Show
      To test this, you need to be able to simulate losing internet connection while attempting a quiz. As well as the obvious pull out the network cable, or disconnect wifi, you can also edit mod/quiz/autosave.ajax.php and add a syntax error. (e.g. random letter near the top). 0. Turn on quiz auto-save on Site administration ► Plugins ► Activity modules ► Quiz. In fact, you may want to force the value in config.php, so that you can have a shorter delay than 1 minute: $CFG->forced_plugin_settings = array('quiz' => array('autosaveperiod' => 10)); 1. Create a simple quiz (e.g. one shortanswer question.) 2. Attempt the quiz as a student, and open the Firebug console (or eqivialent.) If you have developer debug on, that will show you what is going on. 3. Input a reasonse to the question and wait. It should be auto-saved. 4. Now simulate losing internet. 5. Change your response and wait. 6. This time the save should fail, and the alert should appear at the top of the screen. In firebug, you should see it continue to try to auto-save every so often (the configured delay.) 7. Simulate re-connecting to the internet. 8. after the delay, the alert should change to a green all is well message. 9. Change you response again, and wait some more. The green OK message will disappear on the next auto-save.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.5 Branch:
    • Pull Master Branch:
    • Rank:
      54312

      Description

      If auto-saving has been turned on in the quiz, then we are able to detect when the student's internet connection has dropped, and warn them before they do something that causes them to lose their responses.

      The warning needs to be quite discreet, so as not to distract or confuse studens too much during their test, but clear enough that it serves the purpose.

      I think the warning should appear in the question navigation block, just above the 'End test...' link and probably also just above the Next button at the bottom of the page. Something like:

      Warning! connection to the server lost.

      The help icon would display slighly more information:

      Something went wrong when automatically saving your responses. This suggests that you have lost your internet connection.

      You should make a note of any responses you have input recently, then try to re-establish your connection before continuing.

      We will probably only display this warning after two or three successive auto-saves have failed, to reduce false postitives.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          I think it is worth doing this in addition to MDL-38537, which also reduces the risk of dataloss in this situation.

          This is also related to MDL-38538 and MDL-14963.

          Show
          Tim Hunt added a comment - I think it is worth doing this in addition to MDL-38537 , which also reduces the risk of dataloss in this situation. This is also related to MDL-38538 and MDL-14963 .
          Hide
          Tim Hunt added a comment -

          Forum thread with good suggestions:

          https://moodle.org/mod/forum/discuss.php?d=243638

          Show
          Tim Hunt added a comment - Forum thread with good suggestions: https://moodle.org/mod/forum/discuss.php?d=243638
          Hide
          Paul Nicholls added a comment -

          I haven't looked at the quiz auto-save code, so I'm not sure how applicable this is, but I just came across Offline.js and thought it might be useful for this.

          Show
          Paul Nicholls added a comment - I haven't looked at the quiz auto-save code, so I'm not sure how applicable this is, but I just came across Offline.js and thought it might be useful for this.
          Hide
          Tim Hunt added a comment -

          Thanks for the suggestion.

          I think that Offline.js is worth knowing about in general, so I am adding Andrew Nicols as a watcher here. I mean we might want to apply the Offline.js approach uniformly across Moodle at some point, but probably re-implemented using YUI.

          However, what I am looking for here is a minimal fix that I can back-port to Moodle 2.5 or 2.4 at least for the OU, so I am going to code this one myself, and leave longer term thinking to Andrew.

          Show
          Tim Hunt added a comment - Thanks for the suggestion. I think that Offline.js is worth knowing about in general, so I am adding Andrew Nicols as a watcher here. I mean we might want to apply the Offline.js approach uniformly across Moodle at some point, but probably re-implemented using YUI. However, what I am looking for here is a minimal fix that I can back-port to Moodle 2.5 or 2.4 at least for the OU, so I am going to code this one myself, and leave longer term thinking to Andrew.
          Hide
          Tim Hunt added a comment -

          OK Colin, here is the work so far, it would be great if you could do some work on improving the UI. Thanks.

          Show
          Tim Hunt added a comment - OK Colin, here is the work so far, it would be great if you could do some work on improving the UI. Thanks.
          Hide
          Tim Hunt added a comment -

          Actually, I ended up doing more work on the UI, after a useful discussion with Colin.

          We now have an alert stuck to the top of the screen. See the screen-grab in https://moodle.org/mod/forum/discuss.php?d=244507.

          And, I remembered to add ARIA attributes to make it more accessible. I tested those with NVDA, and they seem to work.

          I also did a quick test in IE8 and that works.

          I thik this is ready for peer review, once I write some testing instructions.

          Show
          Tim Hunt added a comment - Actually, I ended up doing more work on the UI, after a useful discussion with Colin. We now have an alert stuck to the top of the screen. See the screen-grab in https://moodle.org/mod/forum/discuss.php?d=244507 . And, I remembered to add ARIA attributes to make it more accessible. I tested those with NVDA, and they seem to work. I also did a quick test in IE8 and that works. I thik this is ready for peer review, once I write some testing instructions.
          Hide
          Tim Hunt added a comment -

          Requesting peer review, hopefully from Andrew Nicols. I am sure there are things about the JS that could be improved.

          Show
          Tim Hunt added a comment - Requesting peer review, hopefully from Andrew Nicols. I am sure there are things about the JS that could be improved.
          Hide
          Andrew Nicols added a comment -

          Hi Tim,

          Thanks for working on this. A few comments.

          mod/quiz/yui/src/autosave/js/autosave.js

          I know we haven't yet finalised the JavaScript Coding guidelines, but the Moodle coding guidelines should still apply so technically variables names should not be underscored (except for function names). That said, I know that the surrounding code uses underscores though so I'll let you use your own judgement.

          WRT the docblock at line 65 of the JS, it would be better to make this a proper docblock with a

          @property

          tag.

          Lines 226, 227, 230, 244, and 256 should be using the SELECTORS constant at the top of the file.

          We should introduce a new constant for line 229 - what does -1 mean?

          I've just realised it's not documented in the new coding guidelines, but for the Y.log calls we should specify the loglevel ('debug'), and log source. The log source should be the YUI module name ('moodle-mod_quiz-autosave').

          mod/quiz/autosave.ajax.php

          We shouldn't echo on an AJAX_SCRIPT, we should return a json_encoded string and handle both success and failure in the Y.io call.

          mod/quiz/styles.css

          Width the percentage width, how does this work with bootstrap on a mobile device? Do we need a min-width too?
          Is the z-index of 10,000 reached in a particular way?

          Cheers,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Tim, Thanks for working on this. A few comments. mod/quiz/yui/src/autosave/js/autosave.js I know we haven't yet finalised the JavaScript Coding guidelines, but the Moodle coding guidelines should still apply so technically variables names should not be underscored (except for function names). That said, I know that the surrounding code uses underscores though so I'll let you use your own judgement. WRT the docblock at line 65 of the JS, it would be better to make this a proper docblock with a @property tag. Lines 226, 227, 230, 244, and 256 should be using the SELECTORS constant at the top of the file. We should introduce a new constant for line 229 - what does -1 mean? I've just realised it's not documented in the new coding guidelines, but for the Y.log calls we should specify the loglevel ('debug'), and log source. The log source should be the YUI module name ('moodle-mod_quiz-autosave'). mod/quiz/autosave.ajax.php We shouldn't echo on an AJAX_SCRIPT, we should return a json_encoded string and handle both success and failure in the Y.io call. mod/quiz/styles.css Width the percentage width, how does this work with bootstrap on a mobile device? Do we need a min-width too? Is the z-index of 10,000 reached in a particular way? Cheers, Andrew
          Hide
          Tim Hunt added a comment -

          Thanks for the comments Andrew.

          To quickly answer a few points:

          Outputting OK to indicate success: Well, look at the corresponding JS code. Just a simple string comparison to see if we succeeded. No need to load the Y.JSON module. Seems like a good solution to me.

          Mobile device: Good point, though how do I think centre it?

          z-index in Moodle is a bloody mess, and I have been asking for simple guidelines that we can all follow for years. I just did a search of the code for 'z-index' and picked a value biggere than almost everything else I could see without being too big. (Some browser can't cope with more than 2^15.)

          Show
          Tim Hunt added a comment - Thanks for the comments Andrew. To quickly answer a few points: Outputting OK to indicate success: Well, look at the corresponding JS code. Just a simple string comparison to see if we succeeded. No need to load the Y.JSON module. Seems like a good solution to me. Mobile device: Good point, though how do I think centre it? z-index in Moodle is a bloody mess, and I have been asking for simple guidelines that we can all follow for years. I just did a search of the code for 'z-index' and picked a value biggere than almost everything else I could see without being too big. (Some browser can't cope with more than 2^15.)
          Hide
          Andrew Nicols added a comment -

          Outputting OK:
          Yup, but if we get an exception thrown but some part of the autosave code, we should probably think about how we handle it. They're automatically returned as a json encoded string we can be passed to the appropriate dialogue already so this is something we may want to consider anyway.

          Mobile devices: I had the same thought. Then writing this reply, I remembered the nested div trick: http://jsfiddle.net/uzNVT/1/ using margin: auto

          I agree about z-index. I was just trying to find the max number because I knew it was fairly low but couldn't lay my hand on it quickly. I guess that's probably a reasonable number.

          Cheers,

          A

          Show
          Andrew Nicols added a comment - Outputting OK: Yup, but if we get an exception thrown but some part of the autosave code, we should probably think about how we handle it. They're automatically returned as a json encoded string we can be passed to the appropriate dialogue already so this is something we may want to consider anyway. Mobile devices: I had the same thought. Then writing this reply, I remembered the nested div trick: http://jsfiddle.net/uzNVT/1/ using margin: auto I agree about z-index. I was just trying to find the max number because I knew it was fairly low but couldn't lay my hand on it quickly. I guess that's probably a reasonable number. Cheers, A
          Hide
          Tim Hunt added a comment -

          Right, I think that takes care of the peer review comments.

          I did not do the Y.log thing, becuase it was silly to do that without changing all Y.logs in the file, and that would have added a lot of noise. I will do that as a bulk change some other time. Also, there is no 'debug'. Did you mean 'info'?

          Show
          Tim Hunt added a comment - Right, I think that takes care of the peer review comments. I did not do the Y.log thing, becuase it was silly to do that without changing all Y.logs in the file, and that would have added a lot of noise. I will do that as a bulk change some other time. Also, there is no 'debug'. Did you mean 'info'?
          Hide
          Tim Hunt added a comment -

          I forgot to say, I did not like the margin auto trick, because it would have needed extra divs. I just increased the width to 80%, which I think is better anyway. (Min-width would be really bad on a screen that was narrower than the min-width. A better rule would be a responsive-design type switch to 100% width at some size, but I am not going there.)

          Show
          Tim Hunt added a comment - I forgot to say, I did not like the margin auto trick, because it would have needed extra divs. I just increased the width to 80%, which I think is better anyway. (Min-width would be really bad on a screen that was narrower than the min-width. A better rule would be a responsive-design type switch to 100% width at some size, but I am not going there.)
          Hide
          Tim Hunt added a comment - - edited

          I have now rebased / backported, and am submitting for intergration.

          Show
          Tim Hunt added a comment - - edited I have now rebased / backported, and am submitting for intergration.
          Hide
          Dan Poltawski added a comment -

          Holding this issue, looks not related to 'release goals' for now. Please get MD to unblock it if feels necessary.

          Show
          Dan Poltawski added a comment - Holding this issue, looks not related to 'release goals' for now. Please get MD to unblock it if feels necessary.
          Hide
          Andrew Nicols added a comment -

          Tim Hunt, when you say 'there is no debug', what do you mean? If you're referring to http://yuilibrary.com/yui/docs/api/classes/YUI.html#method_log, it's a bug in the documentation which is fixed in https://github.com/yui/yui3/pull/1292.

          A

          Show
          Andrew Nicols added a comment - Tim Hunt , when you say 'there is no debug', what do you mean? If you're referring to http://yuilibrary.com/yui/docs/api/classes/YUI.html#method_log , it's a bug in the documentation which is fixed in https://github.com/yui/yui3/pull/1292 . A
          Hide
          Tim Hunt added a comment - - edited

          Dan Poltawski providing it is OK for this to go into 2.4.x - 2.6.1, then that is OK by me. If you later tell me that this is a UI change that can only go into 2.7, I will not be happy.

          Andrew Nicols I was referring to the documentation. It was not obvious that it was a bug in the docs.

          Show
          Tim Hunt added a comment - - edited Dan Poltawski providing it is OK for this to go into 2.4.x - 2.6.1, then that is OK by me. If you later tell me that this is a UI change that can only go into 2.7, I will not be happy. Andrew Nicols I was referring to the documentation. It was not obvious that it was a bug in the docs.
          Hide
          Andrew Nicols added a comment -

          Tim Hunt that's fine - I was just wondering what you were referring to and wanted to check

          Show
          Andrew Nicols added a comment - Tim Hunt that's fine - I was just wondering what you were referring to and wanted to check
          Hide
          Dan Poltawski added a comment -

          Not debating that now. Code submitted for integration 5 days before release gets no guarantees.

          (Nobody will complain about the instability of master at this time more than me, so please don't).

          Show
          Dan Poltawski added a comment - Not debating that now. Code submitted for integration 5 days before release gets no guarantees. (Nobody will complain about the instability of master at this time more than me, so please don't).
          Hide
          Damyon Wiese added a comment -

          Removed integration_held as this is valid for on-sync period.

          Show
          Damyon Wiese added a comment - Removed integration_held as this is valid for on-sync period.
          Hide
          Damyon Wiese added a comment -

          Thanks Tim,

          Stopping this review - this issue has a blocker link.

          I also don't like z-index: 10000 - I posted in the dev forum about z-indexes linking to the policy issue. Hopefully we can get some outcome on that soon so we can write a clear policy.

          Show
          Damyon Wiese added a comment - Thanks Tim, Stopping this review - this issue has a blocker link. I also don't like z-index: 10000 - I posted in the dev forum about z-indexes linking to the policy issue. Hopefully we can get some outcome on that soon so we can write a clear policy.
          Hide
          Damyon Wiese added a comment -

          Thanks Tim,

          Reopening (to take it out of integration). Please resubmit once the blocker is cleared. I think the solution for the z-index is to use "M.core.alert" instead of a "roll your own" alert.

          Show
          Damyon Wiese added a comment - Thanks Tim, Reopening (to take it out of integration). Please resubmit once the blocker is cleared. I think the solution for the z-index is to use "M.core.alert" instead of a "roll your own" alert.
          Hide
          Andrew Nicols added a comment -

          Hi Damyon,

          I disagree wrt moodle-core-notification-alert. Any images included in the dialogue won't be loaded until the dialogue is displayed, and if there's no connection to the server available then it will just fail. This is one of the reasons that Tim went for this option.

          Show
          Andrew Nicols added a comment - Hi Damyon, I disagree wrt moodle-core-notification-alert. Any images included in the dialogue won't be loaded until the dialogue is displayed, and if there's no connection to the server available then it will just fail. This is one of the reasons that Tim went for this option.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tim Hunt added a comment -

          Sorry, blocker link is now spurious. I realised that waiting for that was a non-starter, because it was complex (thank you IIS) so I ended up coding this a different way.

          Show
          Tim Hunt added a comment - Sorry, blocker link is now spurious. I realised that waiting for that was a non-starter, because it was complex (thank you IIS) so I ended up coding this a different way.
          Hide
          Tim Hunt added a comment -

          Andrew already explained about the reason for a custom alert (it most work when your browser is offline, so everything must be loaded during page-load).

          Please note that for internal, OU release schedule reasons, it would make my life much easier if this got integrated this week, and I have already waited some time not pushing this while you folks were trying to get 2.6 finished.

          Show
          Tim Hunt added a comment - Andrew already explained about the reason for a custom alert (it most work when your browser is offline, so everything must be loaded during page-load). Please note that for internal, OU release schedule reasons, it would make my life much easier if this got integrated this week, and I have already waited some time not pushing this while you folks were trying to get 2.6 finished.
          Hide
          Dan Poltawski added a comment -

          Pulling back into integration due to special deal with Tim

          Show
          Dan Poltawski added a comment - Pulling back into integration due to special deal with Tim
          Hide
          Dan Poltawski added a comment -

          Thanks, i've integrated it. Two thoughts I had while reviewing:

          1. I think that perhaps that reasoning could've been made more clearly in the comments (to avoid someone else wondering the same thing).

          2. I wondered if it would be better to call the string _help or _desc as a sort of hint that its markdown formatted. But I can't find anything in the docs which says this is mandatory really.

          Show
          Dan Poltawski added a comment - Thanks, i've integrated it. Two thoughts I had while reviewing: 1. I think that perhaps that reasoning could've been made more clearly in the comments (to avoid someone else wondering the same thing). 2. I wondered if it would be better to call the string _help or _desc as a sort of hint that its markdown formatted. But I can't find anything in the docs which says this is mandatory really.
          Hide
          Tim Hunt added a comment -

          Well, the string is neither a help string, nor the description of an admin setting, so I don't think using either of those would have been a good idea. It might be worth considering a new convention for strings like this, but I am not sure what a good suffix might be for this case.

          Show
          Tim Hunt added a comment - Well, the string is neither a help string, nor the description of an admin setting, so I don't think using either of those would have been a good idea. It might be worth considering a new convention for strings like this, but I am not sure what a good suffix might be for this case.
          Hide
          Frédéric Massart added a comment -

          Passing. However please note that I am against plugin specific CSS. At least when they involve colour and fixed positioning. While this looks OK in standard, this will definitely not look nice in other environments.

          I am not saying that this patch should have been delayed, nor that there is a straightforward way to implement such notifications (though I'd hope there is), but for the future we should use a centralised notification service, which will take care of the positioning, the animation, the colours, and so on.

          Show
          Frédéric Massart added a comment - Passing. However please note that I am against plugin specific CSS. At least when they involve colour and fixed positioning. While this looks OK in standard, this will definitely not look nice in other environments. I am not saying that this patch should have been delayed, nor that there is a straightforward way to implement such notifications (though I'd hope there is), but for the future we should use a centralised notification service, which will take care of the positioning, the animation, the colours, and so on.
          Hide
          Frédéric Massart added a comment -

          Side note, this might look horrendous on mobile, especially with the hardcoded z-index.

          We need a proper notification service!

          (Not blaming anyone here)

          Show
          Frédéric Massart added a comment - Side note, this might look horrendous on mobile, especially with the hardcoded z-index. We need a proper notification service! (Not blaming anyone here)
          Hide
          Tim Hunt added a comment -

          Fred, it might look bad on mobile devices. It might look great. You could test it. (I did a bit, it looks OK) speculation is not very helpful.

          I look forward to MDL-36558 finally getting done. Then we will have a library of reusable UI components that we can point people to, to increase consistency. Also, if a new UI element is required, it can be added to the library first, before it is used anywhere. Andrew Nicols told me that was a priority for the frontend team in 2.7. Anything you can do to help with that would be appreciated.

          Show
          Tim Hunt added a comment - Fred, it might look bad on mobile devices. It might look great. You could test it. (I did a bit, it looks OK) speculation is not very helpful. I look forward to MDL-36558 finally getting done. Then we will have a library of reusable UI components that we can point people to, to increase consistency. Also, if a new UI element is required, it can be added to the library first, before it is used anywhere. Andrew Nicols told me that was a priority for the frontend team in 2.7. Anything you can do to help with that would be appreciated.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ...
          But still, I thank you, for you made me stronger…

          Stronger as the beast that roar in the wild
          Stronger as the storm across the ocean
          Stronger as the diamond that won’t break
          Stronger enough to take all heart aches….

          I thank you my friend, for you made me stronger…

          ---- Juneah Landicho

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao
          Hide
          Helen Foster added a comment -

          Removing docs_required label as this improvement is now mentioned in http://docs.moodle.org/en/Using_Quiz

          Show
          Helen Foster added a comment - Removing docs_required label as this improvement is now mentioned in http://docs.moodle.org/en/Using_Quiz

            People

            • Votes:
              7 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: