Moodle
  1. Moodle
  2. MDL-30484

Regrading quiz causes essay attachments to disappear

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.1.1, 2.2.1
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Questions
    • Labels:
    • Environment:
      Linux, PHP 5.3
    • Database:
      Any
    • Testing Instructions:
      Hide

      This is a fairly fundamental change, so it would be good to subject it to careful testing:

      A. Test the reported problem is fixed.
      1. Create a quiz using essay questions that are set to allow attachments, or the use of the filepicker in the HTML editor. (Ideally make a quiz with questions that test all possibilities.)
      2. Attempt the quiz one or more times as a student - perhaps leave some of the attempts un-submitted.
      3. As teacher, grade some, but not all of the responses.
      4. Review the attempts and note what you see.
      5. In the quiz Grades report, do Regrade all.
      6. Then review all the attempts again, and make sure nothing has changed during the regrade. In particular, make sure the embedded files and attachments are still there.

      B. Run all the unit tests in /question and /mod/quiz.

      C. Next we need to check that these fundamental changes have not broken anything.
      1. Create a few questions of different types in the question bank. Try to include at least one question with some hints.
      2. Preview each of these questions in the question-bank preview pop-up. Preview each question using all the different behaviours. Well, at least preview one question with each behaviour, trying a good mixture in total.
      3. Make sure everything works as expected, and in particular that there are no database or other errors, and that no data is lost ever.

      D. Similar to C. but put the new questions in one or more quizzes, using a few different behaviours, and both preview the quiz as a teacher, and attempt it as a student. Make sure nothing appears to be broken.

      Show
      This is a fairly fundamental change, so it would be good to subject it to careful testing: A. Test the reported problem is fixed. 1. Create a quiz using essay questions that are set to allow attachments, or the use of the filepicker in the HTML editor. (Ideally make a quiz with questions that test all possibilities.) 2. Attempt the quiz one or more times as a student - perhaps leave some of the attempts un-submitted. 3. As teacher, grade some, but not all of the responses. 4. Review the attempts and note what you see. 5. In the quiz Grades report, do Regrade all. 6. Then review all the attempts again, and make sure nothing has changed during the regrade. In particular, make sure the embedded files and attachments are still there. B. Run all the unit tests in /question and /mod/quiz. C. Next we need to check that these fundamental changes have not broken anything. 1. Create a few questions of different types in the question bank. Try to include at least one question with some hints. 2. Preview each of these questions in the question-bank preview pop-up. Preview each question using all the different behaviours. Well, at least preview one question with each behaviour, trying a good mixture in total. 3. Make sure everything works as expected, and in particular that there are no database or other errors, and that no data is lost ever. D. Similar to C. but put the new questions in one or more quizzes, using a few different behaviours, and both preview the quiz as a teacher, and attempt it as a student. Make sure nothing appears to be broken.
    • Workaround:
      Hide

      Don't regrade any quiz involving essays with attachments until this has been fixed.

      Show
      Don't regrade any quiz involving essays with attachments until this has been fixed.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33171

      Description

      Regrading a quiz that contains an essay question with attachments causes those attachments to disappear.

      1. smurf_master.xml
        20 kB
        Eloy Lafuente (stronk7)

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          There are two problems here:

          1. The regrade code was calling question_engine_data_mapper::delete_steps_for_question_attempts and that was deleting the files, even though they are still needed when you are regrading.

          2. The itemid for these files is the question_attempt_steps.id, and this id changes during the regrade, which breaks the link to the file.

          I have a solution to 1., but not 2. yet.

          Show
          Tim Hunt added a comment - There are two problems here: 1. The regrade code was calling question_engine_data_mapper::delete_steps_for_question_attempts and that was deleting the files, even though they are still needed when you are regrading. 2. The itemid for these files is the question_attempt_steps.id, and this id changes during the regrade, which breaks the link to the file. I have a solution to 1., but not 2. yet.
          Hide
          Tim Hunt added a comment -

          OK I think this patch fixes the problem.

          It does have to alter some complex, and critical code right at the heart of the question engine, so I don't really know who might be able to peer-review this realistically.

          Still, if anyone is brave enough to test this a bit, that would be very helpful.

          Show
          Tim Hunt added a comment - OK I think this patch fixes the problem. It does have to alter some complex, and critical code right at the heart of the question engine, so I don't really know who might be able to peer-review this realistically. Still, if anyone is brave enough to test this a bit, that would be very helpful.
          Hide
          Tim Hunt added a comment -

          Col, Mahmoud, if ether of you are prepared to peer-review this change, that would be great.

          This change is likely to be very hard to understand. question/engine/simpletest/testunitofwork.php might be a good place to start. So would be the Unit of work chapter of Patterns of Enterprise Application Architecture, which is sitting on my bookshelf.

          Show
          Tim Hunt added a comment - Col, Mahmoud, if ether of you are prepared to peer-review this change, that would be great. This change is likely to be very hard to understand. question/engine/simpletest/testunitofwork.php might be a good place to start. So would be the Unit of work chapter of Patterns of Enterprise Application Architecture, which is sitting on my bookshelf.
          Hide
          Tim Hunt added a comment -

          Note that the unit test changes in MDL-31065 break these new unit tests, so care needs to be taken when merging both fixes.

          Show
          Tim Hunt added a comment - Note that the unit test changes in MDL-31065 break these new unit tests, so care needs to be taken when merging both fixes.
          Hide
          Tim Hunt added a comment -

          Just to note that the Phil Butcher at the OU did some testing:

          Tim,
          1. I created a test with one MC and one essay question.
          2. I ran the test as a student.
          3. I amended the MC scoring.
          4. I regraded.
          5. The MC was regraded but I could still see the attachments on the essays.

          Show
          Tim Hunt added a comment - Just to note that the Phil Butcher at the OU did some testing: Tim, 1. I created a test with one MC and one essay question. 2. I ran the test as a student. 3. I amended the MC scoring. 4. I regraded. 5. The MC was regraded but I could still see the attachments on the essays.
          Hide
          Mahmoud Kassaei added a comment -

          Lots of chnages and refactoring. It looks fine to me.

          Just an obeservation (because it is PHP)

          In /question/engine/datalib.php in the following protected methods

          is_step_added() and is_step_modified() you are returnning the array index ($key) within the loop (lines 1149 and 1164 respectively) and returning false at the end of each method. I would assume that $key is greater than zero and you may need the value of it when calling respective method, otherwise you could have return true instead of $key.

          Show
          Mahmoud Kassaei added a comment - Lots of chnages and refactoring. It looks fine to me. Just an obeservation (because it is PHP) In /question/engine/datalib.php in the following protected methods is_step_added() and is_step_modified() you are returnning the array index ($key) within the loop (lines 1149 and 1164 respectively) and returning false at the end of each method. I would assume that $key is greater than zero and you may need the value of it when calling respective method, otherwise you could have return true instead of $key.
          Hide
          Tim Hunt added a comment -

          Yes, that is intentional. Moodle ids start at 1 and increase, so they are always true.

          Show
          Tim Hunt added a comment - Yes, that is intentional. Moodle ids start at 1 and increase, so they are always true.
          Hide
          Tim Hunt added a comment -

          Now rebased, and unit test merge issue resolved.

          Show
          Tim Hunt added a comment - Now rebased, and unit test merge issue resolved.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Adding smurf file revealing various coding and docs problems, for reference.

          Show
          Eloy Lafuente (stronk7) added a comment - Adding smurf file revealing various coding and docs problems, for reference.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Tim Hunt added a comment -

          Thanks for the smurf file, but ... I hope this feedback is useful ...

          1. It is complaining about line-length in unit test files. Yes. I know, I should get this right, but I am not that bothered.

          2. It is complaining that test methods in unit test classes do not have PHP docs. I don't really see the point of writing PHP docs for these methods.

          3. It is complaining that methods in subclasses like question_engine_unit_of_work::notify_step_added do not have PHP docs. Well, that is correct. That API is defined in the subclass.

          4. In amongst all that noise, I think there are some valid issues but they are hard to see.

          Overall, only 3. is a serious problem.

          Show
          Tim Hunt added a comment - Thanks for the smurf file, but ... I hope this feedback is useful ... 1. It is complaining about line-length in unit test files. Yes. I know, I should get this right, but I am not that bothered. 2. It is complaining that test methods in unit test classes do not have PHP docs. I don't really see the point of writing PHP docs for these methods. 3. It is complaining that methods in subclasses like question_engine_unit_of_work::notify_step_added do not have PHP docs. Well, that is correct. That API is defined in the subclass. 4. In amongst all that noise, I think there are some valid issues but they are hard to see. Overall, only 3. is a serious problem.
          Hide
          Rossiani Wijaya added a comment -

          This is working great.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories.

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your collaboration, this code has been integrated upstream and it's available in all the repositories. Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: