question bank: MDL-12719, MDL-15774, MDL-17105, and MDL-18174

These are all to do with the combination of the $form->category, and $form->categorymoveto fields on the question editing forms since Moodle 1.9. These are now resolved to a single ->category value in question.php, which simplifies the logic for the individual question types.

MDL-15774 and MDL-12719 errors editing Cloze question
MDL-17105 problems with qustion saving logic.
MDL-18174 'Select from subcategories' option not preserved when editing a random question.

question/question.php

@@ -17,15 +17,13 @@ require_once($CFG->libdir . '/formslib.php');
 $id = optional_param('id', 0, PARAM_INT); // question id
 $qtype = optional_param('qtype', '', PARAM_FILE);
 $categoryid = optional_param('category', 0, PARAM_INT);
-
 $cmid = optional_param('cmid', 0, PARAM_INT);
 $courseid = optional_param('courseid', 0, PARAM_INT);
-$wizardnow =  optional_param('wizardnow', '', PARAM_ALPHA);
-$movecontext =  optional_param('movecontext', 0, PARAM_BOOL);//switch to make question
-                    //uneditable - form is displayed to edit category only
+$wizardnow = optional_param('wizardnow', '', PARAM_ALPHA);
+$movecontext = optional_param('movecontext', 0, PARAM_BOOL); // Switch to make
+        // question uneditable - form is displayed to edit category only
 $returnurl = optional_param('returnurl', 0, PARAM_LOCALURL);
 $appendqnumstring = optional_param('appendqnumstring', '', PARAM_ALPHA);
-
 $inpopup = optional_param('inpopup', 0, PARAM_BOOL);
 
 if ($movecontext && !$id){
@@ -146,20 +144,39 @@ if ($mform->is_cancelled()){
     } else {
         redirect($returnurl);
     }
-} elseif ($fromform = $mform->get_data()){
-    $returnurl = new moodle_url($returnurl);
-    //select category that question has been saved in / moved to when we return to question bank
-    if (!empty($fromform->categorymoveto)){
-        $returnurl->param('category', $fromform->categorymoveto);
-    } else if (!empty($fromform->category)){
-        $returnurl->param('category', $fromform->category);
-    }
-    $returnurl = $returnurl->out();
+} elseif ($fromform = $mform->get_data()) {
+    /// If we are saving as a copy, break the connection to the old question.
     if (!empty($fromform->makecopy)) {
-        $question->id = 0;  // causes a new question to be created.
+        $question->id = 0;
         $question->hidden = 0; // Copies should not be hidden
     }
-    if ($movecontext){
+
+    /// Process the combination of usecurrentcat, categorymoveto and category form
+    /// fields, so the save_question method only has to consider $fromform->category
+    if (!empty($fromform->usecurrentcat)) {
+        // $fromform->category is the right category to save in.
+    } else {
+        if (!empty($fromform->categorymoveto)) {
+            $fromform->category = $fromform->categorymoveto;
+        } else {
+            // $fromform->category is the right category to save in.
+        }
+    }
+
+    /// If we are moving a question, check we have permission to move it from
+    /// whence it came. (Where we are moving to is validated by the form.)
+    list($newcatid) = explode(',', $fromform->category);
+    if (!empty($question->id) && $newcatid != $question->category) {
+        question_require_capability_on($question, 'move');
+    }
+
+    /// Ensure we redirect back to the category the question is being saved into.
+    $returnurl = new moodle_url($returnurl);
+    $returnurl->param('category', $fromform->category);
+    $returnurl = $returnurl->out();
+
+    /// Call the appropriate method.
+    if ($movecontext) {
         list($tocatid, $tocontextid) = explode(',', $fromform->categorymoveto);
         $tocontext = get_context_instance_by_id($tocontextid);
         require_capability('moodle/question:add', $tocontext);

question/type/multianswer/questiontype.php

@@ -173,7 +173,6 @@ class embedded_cloze_qtype extends default_questiontype {
             $question->id = $authorizedquestion->id;
         }
 
-
         $question->category = $authorizedquestion->category;
         $form->course = $course; // To pass the course object to
                                  // save_question_options, where it is

question/type/questiontype.php

@@ -266,22 +266,28 @@ class default_questiontype {
     }
 
     /**
-    * Saves or updates a question after editing by a teacher
+    * Saves (creates or updates) a question.
     *
     * Given some question info and some data about the answers
     * this function parses, organises and saves the question
     * It is used by {@link question.php} when saving new data from
     * a form, and also by {@link import.php} when importing questions
     * This function in turn calls {@link save_question_options}
-    * to save question-type specific options
-    * @param object $question the question object which should be updated
-    * @param object $form the form submitted by the teacher
-    * @param object $course the course we are in
+    * to save question-type specific data.
+    *
+    * Whether we are saving a new question or updating an existing one can be
+    * determined by testing !empty($question->id). If it is not empty, we are updating.
+    *
+    * The question will be saved in category $form->category.
+    *
+    * @param object $question the question object which should be updated. For a new question will be mostly empty.
+    * @param object $form the object containing the information to save, as if from the question editing form.
+    * @param object $course not really used any more.
     * @return object On success, return the new question object. On failure,
     *       return an object as follows. If the error object has an errors field,
     *       display that as an error message. Otherwise, the editing form will be
     *       redisplayed with validation errors, from validation_errors field, which
-    *       is itself an object, shown next to the form fields.
+    *       is itself an object, shown next to the form fields. (I don't think this is accurate any more.)
     */
     function save_question($question, $form, $course) {
         global $USER, $DB;
@@ -289,15 +295,15 @@ class default_questiontype {
         // question types.
 
         // First, save the basic question itself
-        $question->name               = trim($form->name);
-        $question->questiontext       = trim($form->questiontext);
+        $question->name = trim($form->name);
+        $question->questiontext = trim($form->questiontext);
         $question->questiontextformat = $form->questiontextformat;
-        $question->parent             = isset($form->parent)? $form->parent : 0;
+        $question->parent = isset($form->parent) ? $form->parent : 0;
         $question->length = $this->actual_number_of_questions($question);
         $question->penalty = isset($form->penalty) ? $form->penalty : 0;
 
         if (empty($form->image)) {
-            $question->image = "";
+            $question->image = '';
         } else {
             $question->image = $form->image;
         }
@@ -323,33 +329,18 @@ class default_questiontype {
             $question->defaultgrade = $form->defaultgrade;
         }
 
-        if (!empty($question->id) && !empty($form->categorymoveto)) { // Question already exists
-            list($movetocategory, $movetocontextid) = explode(',', $form->categorymoveto);
-            if ($movetocategory != $question->category){
-                question_require_capability_on($question, 'move');
-                $question->category = $movetocategory;
-                //don't need to test add permission of category we are moving question to.
-                //Only categories that we have permission to add
-                //a question to will get through the form cleaning code for the select box.
-            }
-            // keep existing unique stamp code
-            $question->stamp = $DB->get_field('question', 'stamp', array('id' => $question->id));
+        list($question->category) = explode(',', $form->category);
+
+        if (!empty($question->id)) {
+        /// Question already exists, update.
             $question->modifiedby = $USER->id;
             $question->timemodified = time();
             if (!$DB->update_record('question', $question)) {
                 print_error('cannotupdatequestion', 'question');
             }
-        } else {         // Question is a new one
-            if (isset($form->categorymoveto)){
-                // Doing save as new question, and we have move rights.
-                list($question->category, $notused) = explode(',', $form->categorymoveto);
-                //don't need to test add permission of category we are moving question to.
-                //Only categories that we have permission to add
-                //a question to will get through the form cleaning code for the select box.
-            } else {
-                // Really a new question.
-                list($question->category, $notused) = explode(',', $form->category);
-            }
+
+        } else {
+        /// New question.
             // Set the unique code
             $question->stamp = make_unique_id_code();
             $question->createdby = $USER->id;
@@ -362,7 +353,6 @@ class default_questiontype {
         }
 
         // Now to save all the answers and type-specific options
-
         $form->id = $question->id;
         $form->qtype = $question->qtype;
         $form->category = $question->category;

question/type/random/edit_random_form.php

@@ -36,6 +36,14 @@ class question_edit_random_form extends question_edit_form {
 
         $mform->addElement('advcheckbox', 'questiontext', get_string("recurse", "quiz"), null, null, array(0, 1));
 
+        $mform->addElement('hidden', 'name');
+        $mform->setType('name', PARAM_ALPHA);
+        $mform->setDefault('name', '');
+
+        $mform->addElement('hidden', 'tags[]');
+        $mform->setType('tags[]', PARAM_ALPHA);
+        $mform->setDefault('tags[]', '');
+
         // Standard fields at the end of the form.
         $mform->addElement('hidden', 'questiontextformat', 0);
         $mform->setType('questiontextformat', PARAM_INT);

question/type/random/questiontype.php

@@ -108,31 +108,22 @@ class random_qtype extends default_questiontype {
         return get_string('random', 'quiz') .' ('. $category->name .')';
     }
 
-    function save_question($question, $form, $course) {
+    function save_question_options($question) {
         global $DB;
-        // If the category is changing, set things up as default_questiontype::save_question expects.
-        list($formcategory, $unused) = explode(',', $form->category);
-        if (isset($question->id) && $formcategory != $question->category) {
-            $form->categorymoveto = $form->category;
-        }
-        $form->name = '';
-        $question = parent::save_question($question, $form, $course);
+
+        // No options, as such, but we set the parent field to the question's
+        // own id. Setting the parent field has the effect of hiding this
+        // question in various places.
+        $updateobject = new stdClass;
+        $updateobject->id = $question->id;
+        $updateobject->parent = $question->id;
+
+        // We also force the question name to be 'Random (categoryname)'.
         if (!$category = $DB->get_record('question_categories', array('id' => $question->category))) {
             print_error('cannotretrieveqcat', 'question');
         }
-        $question->name = $this->question_name($category);
-        if (!$DB->set_field('question', 'name', $question->name, array('id' => $question->id))) {
-            print_error('cannotupdaterandomqname', 'question');
-        }
-        return $question;
-    }
-
-    function save_question_options($question) {
-        global $DB;
-        // No options, but we set the parent field to the question's own id.
-        // Setting the parent field has the effect of hiding this question in
-        // various places.
-        return ($DB->set_field('question', 'parent', $question->id, array('id' => $question->id)) ? true : false);
+        $updateobject->name = $this->question_name($category);
+        return $DB->update_record('question', $updateobject);
     }
 
     /**