Moodle
  1. Moodle
  2. MDL-32062

regrading pre-existing quiz attempts in a moodle upgraded from 1.9 to 2.2 makes the grades become 0.0

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Upgrade a Moodle having some quiz attempts from the latest 1.9 to the latest 2.2.

      Regrade a quiz attempt.

      Expected result:
      The attempts are regraded; assuming no changes to the questions, this should result in no change to the grades / question marks.

      Actual result:
      The attempts are regraded, all non-manually graded question marks go to 0, and the quiz grade goes to 0.0 ( 0.0 plus the points for any manually graded questions). However, on the quiz attempt review screen, the answers given by the student are correctly reflected (the answers can be correct, but the mark for the question is 0).

      Show
      Upgrade a Moodle having some quiz attempts from the latest 1.9 to the latest 2.2. Regrade a quiz attempt. Expected result: The attempts are regraded; assuming no changes to the questions, this should result in no change to the grades / question marks. Actual result: The attempts are regraded, all non-manually graded question marks go to 0, and the quiz grade goes to 0.0 ( 0.0 plus the points for any manually graded questions). However, on the quiz attempt review screen, the answers given by the student are correctly reflected (the answers can be correct, but the mark for the question is 0).
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      38754

      Description

      To test this, I installed a fresh Moodle 1.9, created quizzes, attempted them, and then upgraded to Moodle 2.2. After the upgrade, the quiz and attempt review screens were showing the correct scores and results. I then selected some attempts and regraded them. This caused all question grades for these attempts to become 0.0 (with the exception of a manually-graded essay question). Intestingly, the quiz attempt review screens still show the given answers, but the grades are 0, even if the given answers were correct.

      The question types were:

      • matching
      • shortanswer
      • multichoice
      • truefalse

      This also effects the gradebook. The grades went down for the regraded quizzes.

      This was with freshly installed Moodle, version '1.9.17 (Build: 20120312)', that was upgraded to '2.2.2 (Build: 20120312)'.

        Activity

        Hide
        Brian King added a comment - - edited

        Added quiz-regrade-debug.zip with the before and after output from http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#Detailed_data_about_an_attempt, as requested in http://moodle.org/mod/forum/discuss.php?d=198337#p864793

        This contains 2 attempts each for 2 quizzes, before and after regrading.

        Show
        Brian King added a comment - - edited Added quiz-regrade-debug.zip with the before and after output from http://docs.moodle.org/dev/Overview_of_the_Moodle_question_engine#Detailed_data_about_an_attempt , as requested in http://moodle.org/mod/forum/discuss.php?d=198337#p864793 This contains 2 attempts each for 2 quizzes, before and after regrading.
        Hide
        Tim Hunt added a comment -

        Right. I can see what is going on now. This problem will only affect quizzes in adaptive mode.

        The upgrade code creates an attempt with two steps:

        1. Start the attempt
        2. Save the responses, and do submit all and finish, in a single step.

        If you try to recreate the same things in Moodle 2.1+ (This is best done using a timed quiz. Input the responses to the question and wait for time to expire, so the quiz is submitted directly from the attempt.php page) then you get an attempt with three steps:

        1. Start the attempt
        2. Save the responses
        3. Submit all and finish (These last two steps have the same timestamp.)

        The regrade code cannot cope with the sort of attempts that the upgrade code generates.

        The question is how to fix this. Either:
        1. Fix the upgrade code to generate proper attempts. This might be easier, but does not help people who have already upgraded.

        2. Improve adaptive mode behaviour so it can cope with the type of attempt history that comes from the upgrade, even though in future, Moodle will never generate that type of history.

        I think 2. is the better approach. Still, not easy.

        Show
        Tim Hunt added a comment - Right. I can see what is going on now. This problem will only affect quizzes in adaptive mode. The upgrade code creates an attempt with two steps: Start the attempt Save the responses, and do submit all and finish, in a single step. If you try to recreate the same things in Moodle 2.1+ (This is best done using a timed quiz. Input the responses to the question and wait for time to expire, so the quiz is submitted directly from the attempt.php page) then you get an attempt with three steps: Start the attempt Save the responses Submit all and finish (These last two steps have the same timestamp.) The regrade code cannot cope with the sort of attempts that the upgrade code generates. The question is how to fix this. Either: 1. Fix the upgrade code to generate proper attempts. This might be easier, but does not help people who have already upgraded. 2. Improve adaptive mode behaviour so it can cope with the type of attempt history that comes from the upgrade, even though in future, Moodle will never generate that type of history. I think 2. is the better approach. Still, not easy.
        Hide
        Tim Hunt added a comment -

        By the way, thank you very much Brian for providing that example data. That is exactly what I needed.

        Show
        Tim Hunt added a comment - By the way, thank you very much Brian for providing that example data. That is exactly what I needed.
        Hide
        Brian King added a comment -

        Thanks for looking into it, Tim.

        If your suggestion 1 is relatively easy ... I'm not sure if it's possible, but how about writing a script that would fix the upgraded attempts, for people who have already upgraded? Is there enough data present after upgrade to recreate the necessary structure in the database?

        Show
        Brian King added a comment - Thanks for looking into it, Tim. If your suggestion 1 is relatively easy ... I'm not sure if it's possible, but how about writing a script that would fix the upgraded attempts, for people who have already upgraded? Is there enough data present after upgrade to recreate the necessary structure in the database?
        Hide
        Tim Hunt added a comment -

        I don't think 1. is much easier to code, and 2. is infinitely easier to test, so I think 2. is the way to go.

        The way to go about it is to write a unit test that created a question_attempt object like the one we get from the upgrade (look at the existing tests for the load_from_records function); and then regrades it and asserts that the result is right. Then we just have to change the code to make that test pass without breaking anything else.

        I have given this outline because I am not at all sure I will find time to work on this myself any time soon, but I am happy to help.

        Show
        Tim Hunt added a comment - I don't think 1. is much easier to code, and 2. is infinitely easier to test, so I think 2. is the way to go. The way to go about it is to write a unit test that created a question_attempt object like the one we get from the upgrade (look at the existing tests for the load_from_records function); and then regrades it and asserts that the result is right. Then we just have to change the code to make that test pass without breaking anything else. I have given this outline because I am not at all sure I will find time to work on this myself any time soon, but I am happy to help.
        Hide
        Hugh Campbell added a comment -

        FYI have also observed this situation with Moodle ver 1.9 to 2.1 user attempts that were regraded.

        Also I tried backing up the course and then restoring into a new course on a separate Moodle ver 2.2 site, but when I did this all attempts that were scored a 0.0 where not restored whereas those that had a mark were restored correctly ie selective restoration occured and this probably will cause some issues.

        Show
        Hugh Campbell added a comment - FYI have also observed this situation with Moodle ver 1.9 to 2.1 user attempts that were regraded. Also I tried backing up the course and then restoring into a new course on a separate Moodle ver 2.2 site, but when I did this all attempts that were scored a 0.0 where not restored whereas those that had a mark were restored correctly ie selective restoration occured and this probably will cause some issues.
        Hide
        Brian Quinn added a comment -

        Tim, I'm not sure this issue affects quizzes in adaptive mode.

        I have also recently upgraded from 1.9 to 2.2 and have seen this happen when 'How questions behave' is set to "Deferred feedback". I should be able to export the course affected if you're looking to re-create?

        Show
        Brian Quinn added a comment - Tim, I'm not sure this issue affects quizzes in adaptive mode. I have also recently upgraded from 1.9 to 2.2 and have seen this happen when 'How questions behave' is set to "Deferred feedback". I should be able to export the course affected if you're looking to re-create?
        Hide
        Matt Jenner added a comment - - edited

        Dare I ask if any progress has been made on this? It may seem minor but we're discovering that it affects people's grades when quizzes are regraded - maybe it's only for upgraded quizzes but a lot of UK universities are upgrading so maybe it's not been widely discovered yet? (I am aware of other countries existence).

        Show
        Matt Jenner added a comment - - edited Dare I ask if any progress has been made on this? It may seem minor but we're discovering that it affects people's grades when quizzes are regraded - maybe it's only for upgraded quizzes but a lot of UK universities are upgrading so maybe it's not been widely discovered yet? (I am aware of other countries existence).
        Hide
        Tim Hunt added a comment -

        Sorry, I have had not time to look at this yet.

        Tim.

        Show
        Tim Hunt added a comment - Sorry, I have had not time to look at this yet. Tim.
        Hide
        Tim Hunt added a comment -

        OK. Thanks to Jessica Gramp for sending me more test data.

        I have now worked out exactly what is going wrong, and I think I have found a way to work-around the problem that exists in the upgraded data in the regrade code.

        It would be really helpful if people who are experiencing this problem could try my proposed fix (see the Pull branches above) and report back on whether it works.

        Also, if anyone can peer-review the change. Thanks.

        Show
        Tim Hunt added a comment - OK. Thanks to Jessica Gramp for sending me more test data. I have now worked out exactly what is going wrong, and I think I have found a way to work-around the problem that exists in the upgraded data in the regrade code. It would be really helpful if people who are experiencing this problem could try my proposed fix (see the Pull branches above) and report back on whether it works. Also, if anyone can peer-review the change. Thanks.
        Hide
        Brian King added a comment -

        as Samuel noted here http://moodle.org/mod/forum/discuss.php?d=196758#p895402 , we tested this with success on Moodle 2.2.

        Show
        Brian King added a comment - as Samuel noted here http://moodle.org/mod/forum/discuss.php?d=196758#p895402 , we tested this with success on Moodle 2.2.
        Hide
        Dan Poltawski added a comment -

        Hi Tim,

        This looks good and makes sense.

        One comment about the comment.. which might make it clearer to people not familiar with the QE like me. You mention two different pieces of terminology for the bit that needs to be split in two:

        'are in the same step.'
        'two separate actions here.'

        Are you referring to the same 'thing' there, or are an action and a step different things? If they are the same then it'd be clearer if they used the same terminology in that comment.

        Show
        Dan Poltawski added a comment - Hi Tim, This looks good and makes sense. One comment about the comment.. which might make it clearer to people not familiar with the QE like me. You mention two different pieces of terminology for the bit that needs to be split in two: 'are in the same step.' 'two separate actions here.' Are you referring to the same 'thing' there, or are an action and a step different things? If they are the same then it'd be clearer if they used the same terminology in that comment.
        Hide
        Tim Hunt added a comment -

        You are right, the last line of the comment should change actions -> steps.

        I will rebase, amend, and make a 2.3 branch today, then submit for integration.

        Show
        Tim Hunt added a comment - You are right, the last line of the comment should change actions -> steps. I will rebase, amend, and make a 2.3 branch today, then submit for integration.
        Hide
        Tim Hunt added a comment -

        Rebased. 2.3 branch made. Comment in the code clarified. Typo in the commit comment fix. 2.3 branch made.

        Submitting for integration.

        Show
        Tim Hunt added a comment - Rebased. 2.3 branch made. Comment in the code clarified. Typo in the commit comment fix. 2.3 branch made. Submitting for integration.
        Hide
        Heikki Wilenius added a comment -

        We tested this as well, and based on one quiz which had the problem, the fix works.

        Show
        Heikki Wilenius added a comment - We tested this as well, and based on one quiz which had the problem, the fix works.
        Hide
        Sam Hemelryk added a comment -

        Thanks Tim, changes looked spot on and this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Tim, changes looked spot on and this has been integrated now.
        Hide
        Sam Hemelryk added a comment -

        Failing this sorry Tim, its triggering one of the phpunit tests.

        phpunit question_engine_unit_of_work_test question/engine/tests/unitofwork_test.php
        
        Show
        Sam Hemelryk added a comment - Failing this sorry Tim, its triggering one of the phpunit tests. phpunit question_engine_unit_of_work_test question/engine/tests/unitofwork_test.php
        Hide
        Tim Hunt added a comment -

        That is really weird. I will investigate. Probably something easy to fix.

        Show
        Tim Hunt added a comment - That is really weird. I will investigate. Probably something easy to fix.
        Hide
        Tim Hunt added a comment -

        Well, that test was borked. Some idiot had added code like

        if (!isset($this->quba->get_question($this->slot)->answer)) {
            $this->quba->get_question($this->slot)->answer = array();
        }
        if (!isset($this->quba->get_question($this->slot)->answer[14])) {
            $this->quba->get_question($this->slot)->answer[14] = new stdClass();
        }
        

        Presumably to hide some errors. Of course, the errors were making it clear that the test was buggy.

        Show
        Tim Hunt added a comment - Well, that test was borked. Some idiot had added code like if (!isset($ this ->quba->get_question($ this ->slot)->answer)) { $ this ->quba->get_question($ this ->slot)->answer = array(); } if (!isset($ this ->quba->get_question($ this ->slot)->answer[14])) { $ this ->quba->get_question($ this ->slot)->answer[14] = new stdClass(); } Presumably to hide some errors. Of course, the errors were making it clear that the test was buggy.
        Hide
        Tim Hunt added a comment -

        Yes. there was a typo in the tests. It should have been modifying ->answers[14] - plural.

        It still fails, even once fixed.

        Show
        Tim Hunt added a comment - Yes. there was a typo in the tests. It should have been modifying ->answers [14] - plural. It still fails, even once fixed.
        Hide
        Tim Hunt added a comment -
        Show
        Tim Hunt added a comment - https://github.com/timhunt/moodle/compare/master...MDL-32062_fixup ready for integration.
        Hide
        Dan Poltawski added a comment -

        Thanks Tim, i've pulled that change in. Passing fine here now. Will wait for the ci servers verdict.

        Show
        Dan Poltawski added a comment - Thanks Tim, i've pulled that change in. Passing fine here now. Will wait for the ci servers verdict.
        Hide
        Dan Poltawski added a comment -

        Yippie, build fixed!

        Show
        Dan Poltawski added a comment - Yippie, build fixed!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just to note that 21 and 22 stables (simple) tests are also borked.

        This commit seems to fix them:

        https://github.com/stronk7/moodle/commit/16ce52bb7dfc12efdf8775d3fceef9d54e08bfc6

        Awaiting for confirmation from Tim...

        Show
        Eloy Lafuente (stronk7) added a comment - Just to note that 21 and 22 stables (simple) tests are also borked. This commit seems to fix them: https://github.com/stronk7/moodle/commit/16ce52bb7dfc12efdf8775d3fceef9d54e08bfc6 Awaiting for confirmation from Tim...
        Hide
        Tim Hunt added a comment -

        +1 for that commit. Sorry, I should have thought of this myself.

        Show
        Tim Hunt added a comment - +1 for that commit. Sorry, I should have thought of this myself.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Sent to integration (21 and 22). And confirmed now all branches are passing their unittests completely. Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Sent to integration (21 and 22). And confirmed now all branches are passing their unittests completely. Thanks!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Verified that current moodle.git code leads to zeros and verified that integration.git performs the recalculations (individual, all and dry) perfectly. Also it "resurrects" previously borked calculations.

        So passing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Verified that current moodle.git code leads to zeros and verified that integration.git performs the recalculations (individual, all and dry) perfectly. Also it "resurrects" previously borked calculations. So passing, thanks!
        Hide
        Sam Hemelryk added a comment -

        Congratulations your code is upstream - gold star for you!

        This issue + 79 others made it in in time for the minor releases.
        Thank you everyone involved for your exuberant efforts.

        Show
        Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.
        Hide
        Jessica Gramp added a comment -

        Hi. We are trying to implement this fix on our Moodle 2.2.3 version, but all of the git hub links above are broken. Is someone able to point me in the direction of the code that will fix this bug in version 2.2.3.

        Thanks.

        Jess

        Show
        Jessica Gramp added a comment - Hi. We are trying to implement this fix on our Moodle 2.2.3 version, but all of the git hub links above are broken. Is someone able to point me in the direction of the code that will fix this bug in version 2.2.3. Thanks. Jess
        Hide
        Tim Hunt added a comment -

        The links stop working after the fix is integrated. There are then two options:

        1. Use the 'Source' tab above.

        2. Use a git command like

        git log -p --grep MDL-32062 origin/MOODLE_22_STABLE
        
        Show
        Tim Hunt added a comment - The links stop working after the fix is integrated. There are then two options: 1. Use the 'Source' tab above. 2. Use a git command like git log -p --grep MDL-32062 origin/MOODLE_22_STABLE
        Hide
        Jessica Gramp added a comment -

        Ok. Thanks for clarifying Tim.

        Show
        Jessica Gramp added a comment - Ok. Thanks for clarifying Tim.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: