Moodle
  1. Moodle
  2. MDL-34351

Bad questions can break overdue quiz cron

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.3.2
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a quiz containing one calculated question; a short time-limit; and overdue handling set to 'Attempts must be submitted before time expires, or they are not counted'.

      2. Create another quiz that is the same, except that it uses another question (e.g. a truefalse question.)

      3. Start an attempt at the calculated quiz as a student, leave the attempt without submitting and before time expires.

      4. Start an attempt at the other quiz as a student, leave the attempt without submitting and before time expires.

      5. Go directly into your database, and find the calculated question you are using. Edit the question text to add something invalid like

      {=1+}

      in the question text.

      6. Wait until time has expired on both student attempts, and then a few more minutes. (Probably, by the time you have finished faffing around in the DB, enough time will have passed.)

      7. Run cron. You should see a bit about 'overdue attempt handling' with an exception stack trace, but the rest of cron should still run.

      8. Go to the another quiz as teacher, and verify that the attempt is now in the 'Never submitted' state.

      (The attempt at the calculated quiz will stay in the open state. That is unavoidable.)

      Show
      1. Create a quiz containing one calculated question; a short time-limit; and overdue handling set to 'Attempts must be submitted before time expires, or they are not counted'. 2. Create another quiz that is the same, except that it uses another question (e.g. a truefalse question.) 3. Start an attempt at the calculated quiz as a student, leave the attempt without submitting and before time expires. 4. Start an attempt at the other quiz as a student, leave the attempt without submitting and before time expires. 5. Go directly into your database, and find the calculated question you are using. Edit the question text to add something invalid like {=1+} in the question text. 6. Wait until time has expired on both student attempts, and then a few more minutes. (Probably, by the time you have finished faffing around in the DB, enough time will have passed.) 7. Run cron. You should see a bit about 'overdue attempt handling' with an exception stack trace, but the rest of cron should still run. 8. Go to the another quiz as teacher, and verify that the attempt is now in the 'Never submitted' state. (The attempt at the calculated quiz will stay in the open state. That is unavoidable.)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      42723

      Description

      This is split off from MDL-34251. Tõnis Tartes reports:

      Tim, could you please help?
      After upgrade to 2.3.1+ i have got the same problem that my cron never finishes.

      Log:
      Processing module function quiz_cron ...
      Looking for quiz overdue quiz attempts between neljapäev, 1. jaanuar 1970, 02:00 and esmaspäev, 16. juuli 2012, 11:20...
      !!! Vigane valemi süntaks algusega '0.1*(((8.0)(2*(0.09)))*2) ((2.6)(0.09)) !!!
      !!
      Error code: illegalformulasyntax !!
      !! Stack trace:

      • line 356 of /question/type/calculated/question.php: moodle_exception thrown
      • line 344 of /question/type/calculated/question.php: call to qtype_calculated_variable_substituter->calculate_raw()
      • line 70 of /question/type/calculated/question.php: call to qtype_calculated_variable_substituter->calculate()
      • line 150 of /question/type/calculated/question.php: call to qtype_calculated_question->calculate_all_expressions()
      • line 59 of /question/type/calculated/question.php: call to qtype_calculated_question_helper::apply_attempt_state()
      • line 1220 of /question/engine/questionattempt.php: call to qtype_calculated_question->apply_attempt_state()
      • line 721 of /question/engine/questionusage.php: call to question_attempt::load_from_records()
      • line 344 of /question/engine/datalib.php: call to question_usage_by_activity::load_from_records()
      • line 78 of /question/engine/lib.php: call to question_engine_data_mapper->load_questions_usage_by_activity()
      • line 468 of /mod/quiz/attemptlib.php: call to question_engine::load_questions_usage_by_activity()
      • line 78 of /mod/quiz/cronlib.php: call to quiz_attempt->__construct()
      • line 469 of /mod/quiz/lib.php: call to mod_quiz_overdue_attempt_updater->update_overdue_attempts()
      • line 259 of /lib/cronlib.php: call to quiz_cron()
      • line 88 of /admin/cron.php: call to cron_run()
        !!

      For what i understand, it has wrong formula syntax, but could you give me any suggestions, what is wrong with the syntax and how to fix it?

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          So, the problem is that a bad question definition can cause processing to throw an exception, and at the moment, that exception is fatal.

          We need to make cron more fault-tolerant.

          Show
          Tim Hunt added a comment - So, the problem is that a bad question definition can cause processing to throw an exception, and at the moment, that exception is fatal. We need to make cron more fault-tolerant.
          Hide
          Tim Hunt added a comment -

          Please could someone give this a quick peer-review. In particular, is that the right way to handle exceptions during cron?

          Show
          Tim Hunt added a comment - Please could someone give this a quick peer-review. In particular, is that the right way to handle exceptions during cron?
          Hide
          Tõnis Tartes added a comment -

          "Good news everyone!" you just read this in Professor Farnsworth voice(from Futurama).

          I tried this patch and it worked fine on my production site.
          The cron did not stop anymore at the illegal formula syntax, instead it shows the problematic quiz IDs which i can investigate myself in DB.

          Thanks Tim!

          Show
          Tõnis Tartes added a comment - "Good news everyone!" you just read this in Professor Farnsworth voice(from Futurama). I tried this patch and it worked fine on my production site. The cron did not stop anymore at the illegal formula syntax, instead it shows the problematic quiz IDs which i can investigate myself in DB. Thanks Tim!
          Hide
          Tim Hunt added a comment -

          It turns out that really the try/catch block needs to cover more of the code. See http://moodle.org/mod/forum/discuss.php?d=207273#p903787.

          Anyway, I have now rebased to the latest master, and am submitting for integration.

          git diff -b or https://github.com/timhunt/moodle/compare/master...MDL-34351?w=1 makes this change easier to review.

          Show
          Tim Hunt added a comment - It turns out that really the try/catch block needs to cover more of the code. See http://moodle.org/mod/forum/discuss.php?d=207273#p903787 . Anyway, I have now rebased to the latest master, and am submitting for integration. git diff -b or https://github.com/timhunt/moodle/compare/master...MDL-34351?w=1 makes this change easier to review.
          Hide
          Dan Poltawski added a comment -

          Thanks Tim. I've integrated this now.

          (thanks also for the -b flag, saved me having to search for it )

          Show
          Dan Poltawski added a comment - Thanks Tim. I've integrated this now. (thanks also for the -b flag, saved me having to search for it )
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Tim,
          Works Great.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Tim, Works Great.
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: