Moodle
  1. Moodle
  2. MDL-33863

error_question_hint_missing_in_db when Importing Quizes into New Course

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.1, 2.2.3
    • Fix Version/s: 2.2.7, 2.3.4
    • Component/s: Backup, Quiz
    • Labels:
    • Testing Instructions:
      Hide

      It may be worth testing this at the same time as MDL-36683, which is similar. As for MDL-36683, it is probably worth verifying that you can reproduce the original problem before you test the fix.

      1. Create a question of any type, adding some hints that contain multiple paragraphs. (Most importantly, the hint text much contain at least on \r character.)

      2. Create a quiz in that course.

      3. Try to duplicate that quiz. Verify that it works.

      Show
      It may be worth testing this at the same time as MDL-36683 , which is similar. As for MDL-36683 , it is probably worth verifying that you can reproduce the original problem before you test the fix. 1. Create a question of any type, adding some hints that contain multiple paragraphs. (Most importantly, the hint text much contain at least on \r character.) 2. Create a quiz in that course. 3. Try to duplicate that quiz. Verify that it works.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      41964

      Description

      From http://moodle.org/mod/forum/discuss.php?d=203973

      I can successfully copy everything but quizzes. When I try to import the quizzes, bam, this error. I can successfully recreate the quizzes in the new course and everything behaves as it should, to the best of my knowledge.

      I recently did transition to using the "Question Sharer" role and saving my question bank at the category level instead of the course level.

      To Reproduce:
      Here is what I've done to replicate this problem. I don't know how many of these steps are important.

      I created two courses, both blank "test bed" courses.

      I created a new category and a single MC question inside the question bank. This category and question are at the category level (so they could be shared across many courses).

      I created a quiz in one of my blank courses. I left all of the settings in the quiz as defaults, including "How questions behave" as "Deferred feedback."

      I added my single MC question to this quiz.

      I went into my second blank course and attempted to import only the quiz I just created from my first blank course.

      I got the "error_question_hint_missing_in_db" error at that point.

      I think this is especially interesting because Question Hints are thoroughly optional. 99.5% of my questions do not have hints in them, and the quiz activity is perfectly fine with having things in the hint blocks or not. While the import function is somehow looking for hints as a required element?

        Issue Links

          Activity

          Hide
          Mike Holzer added a comment -

          We did some debug testing around the method throwing the error ( process_question_hint() ) and found that during the import/restore the method was processing other questions/hints from the question category that were NOT part of the quizes in the course.

          We are using category level questions. It appears that the import/restore function is processing all the questions in the categories, not just the questions actually used in the quiz.

          So to demonstrate my theory, I added some debug in process_question_hint() right before the exception is thrown. The debug simply output the $newquestionid and $oldquestionid variables and did a var_dump() of $data. I then inserted a die().

          The data output on the screen was a hint for a question that was NOT part of the quiz I was importing. I ran the following SQL query:

          select * from mdl_question_hints where questionid in (select id from mdl_question where category = 295) LIMIT 1

          which returned the same data as what was being output on my screen.

          So I think there is a problem with importing category level questions due to the processing of extraneous questions not part of the quiz being imported.

          Show
          Mike Holzer added a comment - We did some debug testing around the method throwing the error ( process_question_hint() ) and found that during the import/restore the method was processing other questions/hints from the question category that were NOT part of the quizes in the course. We are using category level questions. It appears that the import/restore function is processing all the questions in the categories, not just the questions actually used in the quiz. So to demonstrate my theory, I added some debug in process_question_hint() right before the exception is thrown. The debug simply output the $newquestionid and $oldquestionid variables and did a var_dump() of $data. I then inserted a die(). The data output on the screen was a hint for a question that was NOT part of the quiz I was importing. I ran the following SQL query: select * from mdl_question_hints where questionid in (select id from mdl_question where category = 295) LIMIT 1 which returned the same data as what was being output on my screen. So I think there is a problem with importing category level questions due to the processing of extraneous questions not part of the quiz being imported.
          Hide
          Tim Hunt added a comment -

          I don't think the extraneous questions are the problem. It is supposed to work like that. If you back up a given question category, the backup should include all the questions in that category. (This may not be ideal, but it is the way it is supposed to work.)

          http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+%7E+%22%2Bbackup%22+OR+description+%7E+%22%2Bbackup%22%29+AND+issuetype+%3D+Bug+AND+resolution+%3D+Unresolved+AND+component+in+%28Questions%2C+Quiz%29 seems to find a lot of similar bugs.

          Show
          Tim Hunt added a comment - I don't think the extraneous questions are the problem. It is supposed to work like that. If you back up a given question category, the backup should include all the questions in that category. (This may not be ideal, but it is the way it is supposed to work.) http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDL+AND+%28summary+%7E+%22%2Bbackup%22+OR+description+%7E+%22%2Bbackup%22%29+AND+issuetype+%3D+Bug+AND+resolution+%3D+Unresolved+AND+component+in+%28Questions%2C+Quiz%29 seems to find a lot of similar bugs.
          Hide
          Matt Fedorko added a comment -

          I know little to nothing about coding, so please take what I say with a grain of salt, but it seems to me that when I want to move a quiz from one course to the next, I'm asking for something much more similar to the "duplicate" feature than the "backup" feature.

          In fact, what I and a lot of people who are using category level and system level question banks (the comments on MDL-26442, for instance) is to copy a quiz, not a question. In fact, we have all deliberately taken the step of moving questions to the category or system level so that we did not create hundreds of copies of the questions all over our system, and instead have one place where questions that would be used in multiple courses would be housed. Thus, when I try to copy a quiz from one course to another, all I'm trying to copy is an activity that maps those questions to a structure. Now, I understand that not all quizzes would use questions exclusively from category/system level QBs, so there should be a step that checks one way or the other, but I do not understand, even in this instance, why copying the entire course level QB over to the new course seemed like a good idea.

          Actually, I do understand, and it's because this process is an add-on to the backup/restore functionality. I would argue that's a mistake. And I especially think it's a mistake when you start having hundreds and hundreds of questions in your QB, as many users obviously do – you now ask a process that should look at 10 question quiz, quickly determine if any of those 10 questions exist on only the course level and if so copy just those, and then copy the Quiz's settings, and instead ask it to go through 900 questions – and error out when it does? Why even start on the Question Bank end of things, anyway? Isn't it the quiz I'm trying to move over?

          Oh, to be at the mercy of someone else's decisions! All I know is that every 6 weeks I have to manually recreate quizzes instead of moving them over from the source courses, and all because I took the (very smart!) advice of the Moodle docs to create the Question Sharer role, and move my questions to the system/category level, etc and so on.

          Show
          Matt Fedorko added a comment - I know little to nothing about coding, so please take what I say with a grain of salt, but it seems to me that when I want to move a quiz from one course to the next, I'm asking for something much more similar to the "duplicate" feature than the "backup" feature. In fact, what I and a lot of people who are using category level and system level question banks (the comments on MDL-26442 , for instance) is to copy a quiz, not a question. In fact, we have all deliberately taken the step of moving questions to the category or system level so that we did not create hundreds of copies of the questions all over our system, and instead have one place where questions that would be used in multiple courses would be housed. Thus, when I try to copy a quiz from one course to another, all I'm trying to copy is an activity that maps those questions to a structure. Now, I understand that not all quizzes would use questions exclusively from category/system level QBs, so there should be a step that checks one way or the other, but I do not understand, even in this instance, why copying the entire course level QB over to the new course seemed like a good idea. Actually, I do understand, and it's because this process is an add-on to the backup/restore functionality. I would argue that's a mistake. And I especially think it's a mistake when you start having hundreds and hundreds of questions in your QB, as many users obviously do – you now ask a process that should look at 10 question quiz, quickly determine if any of those 10 questions exist on only the course level and if so copy just those, and then copy the Quiz's settings, and instead ask it to go through 900 questions – and error out when it does? Why even start on the Question Bank end of things, anyway? Isn't it the quiz I'm trying to move over? Oh, to be at the mercy of someone else's decisions! All I know is that every 6 weeks I have to manually recreate quizzes instead of moving them over from the source courses, and all because I took the (very smart!) advice of the Moodle docs to create the Question Sharer role, and move my questions to the system/category level, etc and so on.
          Hide
          Matt Fedorko added a comment -

          As my comment on this bug says – MDL-26442 – I commented out the lines of code proposed there by Rob and then successfully imported a quiz between two courses. Hopefully no other problems crop up.

          Show
          Matt Fedorko added a comment - As my comment on this bug says – MDL-26442 – I commented out the lines of code proposed there by Rob and then successfully imported a quiz between two courses. Hopefully no other problems crop up.
          Hide
          R. Müller added a comment -

          The same issue occurred with version 2.3.2. Moving all questions from the system level to course categories solved the issue.

          Show
          R. Müller added a comment - The same issue occurred with version 2.3.2. Moving all questions from the system level to course categories solved the issue.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          I tested a fix similar to MDL-36683 and it seems to work

                      // Not able to find the hint, let's try cleaning the hint text
                      // of all the question's hints in DB as slower fallback. MDL-33863.
                      if (!$newitemid) {
                           $potentialhints = $DB->get_records('question_hints', array('questionid' => $newquestionid), '', 'id, hint');
                          foreach ($potentialhints as $potentialhint) {
                              // Clean in the same way than {@link xml_writer::xml_safe_utf8()}.
                              $cleanhint = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is','', $potentialhint->hint); // Clean CTRL chars.
                              $cleanhint = preg_replace("/\r\n|\r/", "\n", $cleanhint); // Normalize line ending.
                              if ($cleanhint === $data->hint) {
                                  $newitemid = $data->id;
                              }
                          }
                      }
          
          Show
          Jean-Michel Vedrine added a comment - Hello Tim, I tested a fix similar to MDL-36683 and it seems to work // Not able to find the hint, let's try cleaning the hint text // of all the question's hints in DB as slower fallback. MDL-33863. if (!$newitemid) { $potentialhints = $DB->get_records('question_hints', array('questionid' => $newquestionid), '', 'id, hint'); foreach ($potentialhints as $potentialhint) { // Clean in the same way than {@link xml_writer::xml_safe_utf8()}. $cleanhint = preg_replace('/[\x-\x8\xb-\xc\xe-\x1f\x7f]/is','', $potentialhint->hint); // Clean CTRL chars. $cleanhint = preg_replace( "/\r\n|\r/" , "\n" , $cleanhint); // Normalize line ending. if ($cleanhint === $data->hint) { $newitemid = $data->id; } } }
          Hide
          Jean-Michel Vedrine added a comment -

          But I don't really see why we need to create a mapping for the hint itemid ? Where elsewhere in the code is it needed ?
          And apparently I am not alone as there is a comment
          // Create mapping (I'm not sure if this is really needed?)
          This would explain why people commenting this line don't find any problem.

          Show
          Jean-Michel Vedrine added a comment - But I don't really see why we need to create a mapping for the hint itemid ? Where elsewhere in the code is it needed ? And apparently I am not alone as there is a comment // Create mapping (I'm not sure if this is really needed?) This would explain why people commenting this line don't find any problem.
          Hide
          Tim Hunt added a comment -

          It is conceivable that in future, some third-party question type might want to make a new database table like qtype_complex_hint_extension (id, questionhintid, ... other columns ...) in the same way that the question_numerical table extends the question_answers table. That is why we store the mapping.

          I will now review your code, and hopefully turn it into a git commit.

          Show
          Tim Hunt added a comment - It is conceivable that in future, some third-party question type might want to make a new database table like qtype_complex_hint_extension (id, questionhintid, ... other columns ...) in the same way that the question_numerical table extends the question_answers table. That is why we store the mapping. I will now review your code, and hopefully turn it into a git commit.
          Hide
          Tim Hunt added a comment -

          Since Jean-Michel wrote the code, I did the commit in his name. I hope that is OK.

          Submitting for integration now.

          Show
          Tim Hunt added a comment - Since Jean-Michel wrote the code, I did the commit in his name. I hope that is OK. Submitting for integration now.
          Hide
          Tim Hunt added a comment -

          Making this a blocker, like MDL-36683.

          Show
          Tim Hunt added a comment - Making this a blocker, like MDL-36683 .
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Tim,
          yes you are right, I completely forgot that question plugins can extend core question tables !!
          Thanks for assigning to me but the code is a simple adaptation of the one in MDL-36683 !

          Show
          Jean-Michel Vedrine added a comment - Hello Tim, yes you are right, I completely forgot that question plugins can extend core question tables !! Thanks for assigning to me but the code is a simple adaptation of the one in MDL-36683 !
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Tim.

          Show
          Dan Poltawski added a comment - Integrated, thanks Tim.
          Hide
          Jean-Michel Vedrine added a comment -

          Hello Rajesh,
          Thanks a lot for testing this.
          As Tim has said in the testing instructions: "It may be worth testing this at the same time as MDL-36683, which is similar"

          Show
          Jean-Michel Vedrine added a comment - Hello Rajesh, Thanks a lot for testing this. As Tim has said in the testing instructions: "It may be worth testing this at the same time as MDL-36683 , which is similar"
          Hide
          Rajesh Taneja added a comment -

          Thanks Jean-Michel,

          I will check with Andrew (who is currently assigned tester) tomorrow and take that for testing.

          Show
          Rajesh Taneja added a comment - Thanks Jean-Michel, I will check with Andrew (who is currently assigned tester) tomorrow and take that for testing.
          Hide
          Rajesh Taneja added a comment -

          Thanks Jean-Michel and Tim,

          I tried replicating the issue first and it was easy to replicate error. Then with patch no error came and quiz was duplicated without problem.

          Show
          Rajesh Taneja added a comment - Thanks Jean-Michel and Tim, I tried replicating the issue first and it was easy to replicate error. Then with patch no error came and quiz was duplicated without problem.
          Hide
          Jean-Michel Vedrine added a comment -

          Thanks Rajesh

          Show
          Jean-Michel Vedrine added a comment - Thanks Rajesh
          Hide
          Dan Poltawski added a comment -

          Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week!

          ciao
          Dan

          Show
          Dan Poltawski added a comment - Congratulations! Another bug solved.. only another 7330 to go, thanks for contributing to contributing to 0.8% of all bugs being fixed this week! ciao Dan

            People

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

              Dates

              • Created:
                Updated:
                Resolved: