From 6189fda47ffefba8255dfff16c68d1a8528f3532 Mon Sep 17 00:00:00 2001 From: John Beedell Date: Wed, 4 Jul 2018 12:21:44 +0100 Subject: [PATCH] MDL-62708 question: Add idnumbers to question and question category --- backup/moodle2/backup_stepslib.php | 4 +- backup/moodle2/restore_stepslib.php | 13 ++ lang/en/question.php | 2 + lib/db/install.xml | 6 +- lib/db/upgrade.php | 39 ++++++ lib/questionlib.php | 23 +++- question/category.php | 4 +- question/category_class.php | 28 +++- question/category_form.php | 33 +++++ question/engine/tests/helpers.php | 2 + question/format.php | 7 + question/format/xml/format.php | 3 + question/format/xml/tests/xmlformat_test.php | 130 ++++++++++++++++++ question/tests/backup_test.php | 13 +- .../question_categories_idnumber.feature | 122 ++++++++++++++++ question/tests/generator/lib.php | 2 + question/tests/generator_test.php | 29 ++++ .../type/ddwtos/tests/questiontype_test.php | 4 + question/type/edit_question_form.php | 30 ++++ .../gapselect/tests/questiontype_test.php | 3 + .../type/match/tests/questiontype_test.php | 1 + .../missingtype/tests/missingtype_test.php | 1 + question/type/questiontypebase.php | 14 ++ version.php | 2 +- 24 files changed, 502 insertions(+), 13 deletions(-) create mode 100644 question/tests/behat/question_categories_idnumber.feature diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php index 3793d9b7ce7..b90490ac420 100644 --- a/backup/moodle2/backup_stepslib.php +++ b/backup/moodle2/backup_stepslib.php @@ -2211,7 +2211,7 @@ class backup_questions_structure_step extends backup_structure_step { $qcategory = new backup_nested_element('question_category', array('id'), array( 'name', 'contextid', 'contextlevel', 'contextinstanceid', 'info', 'infoformat', 'stamp', 'parent', - 'sortorder')); + 'sortorder', 'idnumber')); $questions = new backup_nested_element('questions'); @@ -2219,7 +2219,7 @@ class backup_questions_structure_step extends backup_structure_step { 'parent', 'name', 'questiontext', 'questiontextformat', 'generalfeedback', 'generalfeedbackformat', 'defaultmark', 'penalty', 'qtype', 'length', 'stamp', 'version', - 'hidden', 'timecreated', 'timemodified', 'createdby', 'modifiedby')); + 'hidden', 'timecreated', 'timemodified', 'createdby', 'modifiedby', 'idnumber')); // attach qtype plugin structure to $question element, only one allowed $this->add_plugin_structure('qtype', $question, false); diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index 844317424c7..250aeb23fb4 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -4434,6 +4434,12 @@ class restore_create_categories_and_questions extends restore_structure_step { $data->stamp = make_unique_id_code(); } + // The idnumber if it exists also needs to be unique within a context or reset it to null. + if (!empty($data->idnumber) && $DB->record_exists('question_categories', + ['idnumber' => $data->idnumber, 'contextid' => $data->contextid])) { + unset($data->idnumber); + } + // Let's create the question_category and save mapping. $newitemid = $DB->insert_record('question_categories', $data); $this->set_mapping('question_category', $oldid, $newitemid); @@ -4479,6 +4485,13 @@ class restore_create_categories_and_questions extends restore_structure_step { // With newitemid = 0, let's create the question if (!$questionmapping->newitemid) { + + // The idnumber if it exists also needs to be unique within a category or reset it to null. + if (!empty($data->idnumber) && $DB->record_exists('question', + ['idnumber' => $data->idnumber, 'category' => $data->category])) { + unset($data->idnumber); + } + $newitemid = $DB->insert_record('question', $data); $this->set_mapping('question', $oldid, $newitemid); // Also annotate them as question_created, we need diff --git a/lang/en/question.php b/lang/en/question.php index fb8fe82ff33..b853063a13c 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -170,6 +170,8 @@ $string['getcontextfromfile'] = 'Get context from file'; $string['changepublishstatuscat'] = 'Category "{$a->name}" in course "{$a->coursename}" will have it\'s sharing status changed from {$a->changefrom} to {$a->changeto}.'; $string['chooseqtypetoadd'] = 'Choose a question type to add'; $string['editquestions'] = 'Edit questions'; +$string['idnumber'] = 'ID number'; +$string['idnumber_help'] = 'If used, the ID number must be unique within each question category. It provides another way of identifying a question which is sometimes useful, but can usually be left blank.'; $string['ignorebroken'] = 'Ignore broken links'; $string['impossiblechar'] = 'Impossible character {$a} detected as parenthesis character'; $string['import'] = 'Import'; diff --git a/lib/db/install.xml b/lib/db/install.xml index c7d1a8191d6..2101c71de19 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -1333,6 +1333,7 @@ + @@ -1341,6 +1342,7 @@ + @@ -1364,6 +1366,7 @@ + @@ -1374,6 +1377,7 @@ +
@@ -3861,4 +3865,4 @@
- \ No newline at end of file + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index fc5e7773f2c..7f6a1f11cb3 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2360,5 +2360,44 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2018091700.01); } + // Add idnumber fields to question and question_category tables. + // This is done in four parts to aid error recovery during upgrade, should that occur. + if ($oldversion < 2018092100.01) { + $table = new xmldb_table('question'); + $field = new xmldb_field('idnumber', XMLDB_TYPE_CHAR, '100', null, null, null, null, 'modifiedby'); + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + upgrade_main_savepoint(true, 2018092100.01); + } + + if ($oldversion < 2018092100.02) { + $table = new xmldb_table('question'); + $index = new xmldb_index('categoryidnumber', XMLDB_INDEX_UNIQUE, array('category, idnumber')); + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + upgrade_main_savepoint(true, 2018092100.02); + } + + if ($oldversion < 2018092100.03) { + $table = new xmldb_table('question_categories'); + $field = new xmldb_field('idnumber', XMLDB_TYPE_CHAR, '100', null, null, null, null, 'sortorder'); + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + upgrade_main_savepoint(true, 2018092100.03); + } + + if ($oldversion < 2018092100.04) { + $table = new xmldb_table('question_categories'); + $index = new xmldb_index('contextididnumber', XMLDB_INDEX_UNIQUE, array('contextid, idnumber')); + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + // Main savepoint reached. + upgrade_main_savepoint(true, 2018092100.04); + } + return true; } diff --git a/lib/questionlib.php b/lib/questionlib.php index 268c066ed4b..1439ea87364 100644 --- a/lib/questionlib.php +++ b/lib/questionlib.php @@ -673,7 +673,7 @@ function question_move_questions_to_category($questionids, $newcategoryid) { array('id' => $newcategoryid)); list($questionidcondition, $params) = $DB->get_in_or_equal($questionids); $questions = $DB->get_records_sql(" - SELECT q.id, q.qtype, qc.contextid + SELECT q.id, q.qtype, qc.contextid, q.idnumber FROM {question} q JOIN {question_categories} qc ON q.category = qc.id WHERE q.id $questionidcondition", $params); @@ -682,6 +682,27 @@ function question_move_questions_to_category($questionids, $newcategoryid) { question_bank::get_qtype($question->qtype)->move_files( $question->id, $question->contextid, $newcontextid); } + // Check whether there could be a clash of idnumbers in the new category. + if (((string) $question->idnumber !== '') && + $DB->record_exists('question', ['idnumber' => $question->idnumber, 'category' => $newcategoryid])) { + $rec = $DB->get_records_select('question', "category = ? AND idnumber LIKE ?", + [$newcategoryid, $question->idnumber . '_%'], 'idnumber DESC', 'id, idnumber', 0, 1); + $unique = 1; + if (count($rec)) { + $rec = reset($rec); + $idnumber = $rec->idnumber; + if (strpos($idnumber, '_') !== false) { + $unique = substr($idnumber, strpos($idnumber, '_') + 1) + 1; + } + } + // For the move process, add a numerical increment to the idnumber. This means that if a question is + // mistakenly moved then the idnumber will not be completely lost. + $q = new stdClass(); + $q->id = $question->id; + $q->category = $newcategoryid; + $q->idnumber = $question->idnumber . '_' . $unique; + $DB->update_record('question', $q); + } } // Move the questions themselves. diff --git a/question/category.php b/question/category.php index 77ffa39bd58..8e4ae0e7af7 100644 --- a/question/category.php +++ b/question/category.php @@ -118,10 +118,10 @@ if ($qcobject->catform->is_cancelled()) { $catformdata->info = $catformdata->info['text']; if (!$catformdata->id) {//new category $qcobject->add_category($catformdata->parent, $catformdata->name, - $catformdata->info, false, $catformdata->infoformat); + $catformdata->info, false, $catformdata->infoformat, $catformdata->idnumber); } else { $qcobject->update_category($catformdata->id, $catformdata->parent, - $catformdata->name, $catformdata->info, $catformdata->infoformat); + $catformdata->name, $catformdata->info, $catformdata->infoformat, $catformdata->idnumber); } redirect($thispageurl); } else if ((!empty($param->delete) and (!$questionstomove) and confirm_sesskey())) { diff --git a/question/category_class.php b/question/category_class.php index e7f6ccb6a56..3bccfc9b08e 100644 --- a/question/category_class.php +++ b/question/category_class.php @@ -404,7 +404,8 @@ class question_category_object { /** * Creates a new category with given params */ - public function add_category($newparent, $newcategory, $newinfo, $return = false, $newinfoformat = FORMAT_HTML) { + public function add_category($newparent, $newcategory, $newinfo, $return = false, $newinfoformat = FORMAT_HTML, + $idnumber = null) { global $DB; if (empty($newcategory)) { print_error('categorynamecantbeblank', 'question'); @@ -419,6 +420,14 @@ class question_category_object { } } + if (((string) $idnumber !== '') && !empty($contextid)) { + // While this check already exists in the form validation, this is a backstop preventing unnecessary errors. + if ($DB->record_exists('question_categories', + ['idnumber' => $idnumber, 'contextid' => $contextid])) { + $idnumber = null; + } + } + $cat = new stdClass(); $cat->parent = $parentid; $cat->contextid = $contextid; @@ -427,6 +436,9 @@ class question_category_object { $cat->infoformat = $newinfoformat; $cat->sortorder = 999; $cat->stamp = make_unique_id_code(); + if ($idnumber) { + $cat->idnumber = $idnumber; + } $categoryid = $DB->insert_record("question_categories", $cat); // Log the creation of this category. @@ -447,7 +459,8 @@ class question_category_object { /** * Updates an existing category with given params */ - public function update_category($updateid, $newparent, $newname, $newinfo, $newinfoformat = FORMAT_HTML) { + public function update_category($updateid, $newparent, $newname, $newinfo, $newinfoformat = FORMAT_HTML, + $idnumber = null) { global $CFG, $DB; if (empty($newname)) { print_error('categorynamecantbeblank', 'question'); @@ -480,6 +493,14 @@ class question_category_object { } } + if (((string) $idnumber !== '') && !empty($tocontextid)) { + // While this check already exists in the form validation, this is a backstop preventing unnecessary errors. + if ($DB->record_exists('question_categories', + ['idnumber' => $idnumber, 'contextid' => $tocontextid])) { + $idnumber = null; + } + } + // Update the category record. $cat = new stdClass(); $cat->id = $updateid; @@ -488,6 +509,9 @@ class question_category_object { $cat->infoformat = $newinfoformat; $cat->parent = $parentid; $cat->contextid = $tocontextid; + if ($idnumber) { + $cat->idnumber = $idnumber; + } if ($newstamprequired) { $cat->stamp = make_unique_id_code(); } diff --git a/question/category_form.php b/question/category_form.php index 14ccf19a9f0..c0a36aa56f8 100644 --- a/question/category_form.php +++ b/question/category_form.php @@ -63,6 +63,10 @@ class question_category_edit_form extends moodleform { $mform->setDefault('info', ''); $mform->setType('info', PARAM_RAW); + $mform->addElement('text', 'idnumber', get_string('idnumber', 'question'), 'maxlength="100" size="10"'); + $mform->addHelpButton('idnumber', 'idnumber', 'question'); + $mform->setType('idnumber', PARAM_RAW); + $this->add_action_buttons(false, get_string('addcategory', 'question')); $mform->addElement('hidden', 'id', 0); @@ -81,4 +85,33 @@ class question_category_edit_form extends moodleform { } parent::set_data($current); } + + /** + * Validation. + * + * @param array $data + * @param array $files + * @return array the errors that were found + */ + public function validation($data, $files) { + global $DB; + + $errors = parent::validation($data, $files); + + // Add field validation check for duplicate idnumber. + list($parentid, $contextid) = explode(',', $data['parent']); + if (((string) $data['idnumber'] !== '') && !empty($contextid)) { + $conditions = 'contextid = ? AND idnumber = ?'; + $params = [$contextid, $data['idnumber']]; + if (!empty($data['id'])) { + $conditions .= ' AND id <> ?'; + $params[] = $data['id']; + } + if ($DB->record_exists_select('question_categories', $conditions, $params)) { + $errors['idnumber'] = get_string('idnumbertaken', 'error'); + } + } + + return $errors; + } } diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index 6877d454de8..93d3ecb9a52 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -170,6 +170,7 @@ class test_question_maker { $q->id = 0; $q->category = 0; + $q->idnumber = null; $q->parent = 0; $q->questiontextformat = FORMAT_HTML; $q->generalfeedbackformat = FORMAT_HTML; @@ -190,6 +191,7 @@ class test_question_maker { $qdata->id = 0; $qdata->category = 0; + $qdata->idnumber = null; $qdata->contextid = 0; $qdata->parent = 0; $qdata->questiontextformat = FORMAT_HTML; diff --git a/question/format.php b/question/format.php index da31797397f..0ba280ef8f0 100644 --- a/question/format.php +++ b/question/format.php @@ -406,6 +406,13 @@ class qformat_default { $question->timecreated = time(); $question->modifiedby = $USER->id; $question->timemodified = time(); + if (isset($question->idnumber) && (string) $question->idnumber !== '') { + if ($DB->record_exists('question', ['idnumber' => $question->idnumber, 'category' => $question->category])) { + // We cannot have duplicate idnumbers in a category. + unset($question->idnumber); + } + } + $fileoptions = array( 'subdirs' => true, 'maxfiles' => -1, diff --git a/question/format/xml/format.php b/question/format/xml/format.php index 93ab228df88..6ea5f5f290b 100644 --- a/question/format/xml/format.php +++ b/question/format/xml/format.php @@ -236,6 +236,8 @@ class qformat_xml extends qformat_default { $qo->questiontext .= ' '; } + $qo->idnumber = $this->getpath($question, ['#', 'idnumber', 0, '#'], null); + // Restore files in generalfeedback. $generalfeedback = $this->import_text_with_files($question, array('#', 'generalfeedback', 0), $qo->generalfeedback, $this->get_format($qo->questiontextformat)); @@ -1217,6 +1219,7 @@ class qformat_xml extends qformat_default { } $expout .= " {$question->penalty}\n"; $expout .= " {$question->hidden}\n"; + $expout .= " {$question->idnumber}\n"; // The rest of the output depends on question type. switch($question->qtype) { diff --git a/question/format/xml/tests/xmlformat_test.php b/question/format/xml/tests/xmlformat_test.php index 6732cbc9545..c85fff02b26 100644 --- a/question/format/xml/tests/xmlformat_test.php +++ b/question/format/xml/tests/xmlformat_test.php @@ -48,6 +48,7 @@ class qformat_xml_test extends question_testcase { $q = new stdClass(); $q->id = 0; $q->contextid = 0; + $q->idnumber = null; $q->category = 0; $q->parent = 0; $q->questiontextformat = FORMAT_HTML; @@ -342,6 +343,7 @@ END; $qdata->length = 0; $qdata->penalty = 0; $qdata->hidden = 0; + $qdata->idnumber = null; $exporter = new qformat_xml(); $xml = $exporter->writequestion($qdata); @@ -360,6 +362,7 @@ END; 0 0 0 + '; @@ -487,6 +490,7 @@ END; $qdata->length = 1; $qdata->penalty = 0; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->id = 456; $qdata->options->questionid = 123; @@ -516,6 +520,7 @@ END; 1 0 0 + monospaced 0 42 @@ -649,6 +654,7 @@ END; $qdata->length = 1; $qdata->penalty = 0.3333333; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->shuffleanswers = 1; @@ -709,6 +715,7 @@ END; 1 0.3333333 0 + true Well done. @@ -880,6 +887,7 @@ END; $qdata->length = 1; $qdata->penalty = 0.3333333; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->single = 0; @@ -922,6 +930,7 @@ END; 2 0.3333333 0 + false false abc @@ -1053,6 +1062,7 @@ END; $qdata->length = 1; $qdata->penalty = 0.1; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->answers = array( @@ -1083,6 +1093,7 @@ END; 1 0.1 0 + 42 @@ -1183,6 +1194,7 @@ END; $qdata->length = 1; $qdata->penalty = 0.3333333; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->usecase = 0; @@ -1214,6 +1226,7 @@ END; 1 0.3333333 0 + 0 Beta @@ -1290,6 +1303,59 @@ END; $this->assert(new question_check_specified_fields_expectation($expectedq), $q); } + public function test_import_truefalse_with_idnumber() { + $xml = ' + + True false question + + + The answer is true. + + + General feedback: You should have chosen true. + + 1 + 1 + 0 + TestIdNum1 + + true + + Well done! + + + + false + + Doh! + + + '; + $xmldata = xmlize($xml); + + $importer = new qformat_xml(); + $q = $importer->import_truefalse($xmldata['question']); + + $expectedq = new stdClass(); + $expectedq->qtype = 'truefalse'; + $expectedq->name = 'True false question'; + $expectedq->questiontext = 'The answer is true.'; + $expectedq->questiontextformat = FORMAT_HTML; + $expectedq->generalfeedback = 'General feedback: You should have chosen true.'; + $expectedq->defaultmark = 1; + $expectedq->length = 1; + $expectedq->penalty = 1; + $expectedq->idnumber = 'TestIdNum1'; + + $expectedq->feedbacktrue = array('text' => 'Well done!', + 'format' => FORMAT_HTML); + $expectedq->feedbackfalse = array('text' => 'Doh!', + 'format' => FORMAT_HTML); + $expectedq->correctanswer = true; + + $this->assert(new question_check_specified_fields_expectation($expectedq), $q); + } + public function test_export_truefalse() { $qdata = new stdClass(); $qdata->id = 12; @@ -1304,6 +1370,7 @@ END; $qdata->length = 1; $qdata->penalty = 1; $qdata->hidden = 0; + $qdata->idnumber = null; $qdata->options = new stdClass(); $qdata->options->answers = array( @@ -1330,6 +1397,67 @@ END; 1 1 0 + + + true + + Well done! + + + + false + + Doh! + + + +'; + + $this->assert_same_xml($expectedxml, $xml); + } + + public function test_export_truefalse_with_idnumber() { + $qdata = new stdClass(); + $qdata->id = 12; + $qdata->contextid = \context_system::instance()->id; + $qdata->qtype = 'truefalse'; + $qdata->name = 'True false question'; + $qdata->questiontext = 'The answer is true.'; + $qdata->questiontextformat = FORMAT_HTML; + $qdata->generalfeedback = 'General feedback: You should have chosen true.'; + $qdata->generalfeedbackformat = FORMAT_HTML; + $qdata->defaultmark = 1; + $qdata->length = 1; + $qdata->penalty = 1; + $qdata->hidden = 0; + $qdata->idnumber = 'TestIDNum2'; + + $qdata->options = new stdClass(); + $qdata->options->answers = array( + 1 => new question_answer(1, 'True', 1, 'Well done!', FORMAT_HTML), + 2 => new question_answer(2, 'False', 0, 'Doh!', FORMAT_HTML), + ); + $qdata->options->trueanswer = 1; + $qdata->options->falseanswer = 2; + + $exporter = new qformat_xml(); + $xml = $exporter->writequestion($qdata); + + $expectedxml = ' + + + True false question + + + The answer is true. + + + General feedback: You should have chosen true. + + 1 + 1 + 0 + TestIDNum2 true @@ -1474,6 +1602,7 @@ END; 0.3333333 0 + Hint 1 @@ -1505,6 +1634,7 @@ END; 0.3333333 0 + '; diff --git a/question/tests/backup_test.php b/question/tests/backup_test.php index 9695571ce6f..389880cfd0f 100644 --- a/question/tests/backup_test.php +++ b/question/tests/backup_test.php @@ -115,8 +115,8 @@ class core_question_backup_testcase extends advanced_testcase { $qgen = $this->getDataGenerator()->get_plugin_generator('core_question'); $context = context_coursecat::instance($category1->id); $qcat = $qgen->create_question_category(array('contextid' => $context->id)); - $question1 = $qgen->create_question('shortanswer', null, array('category' => $qcat->id)); - $question2 = $qgen->create_question('shortanswer', null, array('category' => $qcat->id)); + $question1 = $qgen->create_question('shortanswer', null, array('category' => $qcat->id, 'idnumber' => 'q1')); + $question2 = $qgen->create_question('shortanswer', null, array('category' => $qcat->id, 'idnumber' => 'q2')); // Tag the questions with 2 question tags and 2 course level question tags. $qcontext = context::instance_by_id($qcat->contextid); @@ -144,10 +144,11 @@ class core_question_backup_testcase extends advanced_testcase { // The questions should remain in the question category they were which is // a question category belonging to a course category context. - $questions = $DB->get_records('question', array('category' => $qcat->id)); + $questions = $DB->get_records('question', array('category' => $qcat->id), 'idnumber'); $this->assertCount(2, $questions); // Retrieve tags for each question and check if they are assigned at the right context. + $qcount = 1; foreach ($questions as $question) { $tags = core_tag_tag::get_item_tags('core_question', 'question', $question->id); @@ -162,6 +163,10 @@ class core_question_backup_testcase extends advanced_testcase { } $this->assertEquals($expected, $tag->taginstancecontextid); } + + // Also check idnumbers have been backed up and restored. + $this->assertEquals('q' . $qcount, $question->idnumber); + $qcount++; } // Now, again, delete everything including the course category. @@ -203,4 +208,4 @@ class core_question_backup_testcase extends advanced_testcase { } } -} \ No newline at end of file +} diff --git a/question/tests/behat/question_categories_idnumber.feature b/question/tests/behat/question_categories_idnumber.feature new file mode 100644 index 00000000000..f5717be9dae --- /dev/null +++ b/question/tests/behat/question_categories_idnumber.feature @@ -0,0 +1,122 @@ +@core @core_question +Feature: A teacher can put questions with idnumbers in categories with idnumbers in the question bank + In order to organize my questions + As a teacher + I create and edit categories and move questions between them (now with idnumbers) + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + And the following "courses" exist: + | fullname | shortname | format | + | Course 1 | C1 | weeks | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + And I log in as "teacher1" + And I am on "Course 1" course homepage + + Scenario: A new question category can only be created with a unique idnumber for a context + # Note need to create the top category each time. + When the following "question categories" exist: + | contextlevel | reference | questioncategory | name | idnumber | + | Course | C1 | Top | top | | + | Course | C1 | top | Used category | c1used | + And I navigate to "Question bank > Categories" in current page administration + And I set the following fields to these values: + | Name | Sub used category | + | Parent category | Used category | + | Category info | Created as a test | + | ID number | c1used | + And I press "Add category" + # Standard warning. + Then I should see "This ID number is already in use" + # Correction to a unique idnumber for the context. + And I set the field "ID number" to "c1unused" + And I press "Add category" + Then I should see "Sub used category (0)" + And I should see "Created as a test" in the "Sub used category" "list_item" + + Scenario: A question category can be edited and saved without changing the idnumber + When the following "question categories" exist: + | contextlevel | reference | questioncategory | name | idnumber | + | Course | C1 | Top | top | | + | Course | C1 | top | Used category | c1used | + And I navigate to "Question bank > Categories" in current page administration + And I click on "Edit" "link" in the "Used category" "list_item" + And I press "Save changes" + Then I should not see "This ID number is already in use" + + Scenario: A question can only have a unique idnumber within a category + When the following "question categories" exist: + | contextlevel | reference | questioncategory | name | idnumber | + | Course | C1 | Top | top | | + | Course | C1 | top | Used category | c1used | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | idnumber | + | Used category | essay | Test question 1 | Write about whatever you want | q1 | + | Used category | essay | Test question 2 | Write about whatever you want | q2 | + And I navigate to "Question bank > Questions" in current page administration + And I click on "Edit" "link" in the "Test question 2" "table_row" + And I set the field "ID number" to "q1" + And I press "submitbutton" + # This is the standard form warning reminding the user that the idnumber needs to be unique for a category. + Then I should see "This ID number is already in use" + + Scenario: A question can be edited and saved without changing the idnumber + When the following "question categories" exist: + | contextlevel | reference | questioncategory | name | idnumber | + | Course | C1 | Top | top | | + | Course | C1 | top | Used category | c1used | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | idnumber | + | Used category | essay | Test question 1 | Write about whatever you want | q1 | + And I navigate to "Question bank > Questions" in current page administration + And I click on "Edit" "link" in the "Test question 1" "table_row" + And I press "Save changes" + Then I should not see "This ID number is already in use" + + Scenario: Question idnumber conficts found when saving to a different category. + When the following "question categories" exist: + | contextlevel | reference | questioncategory | name | + | Course | C1 | Top | top | + | Course | C1 | top | Category 1 | + | Course | C1 | top | Category 2 | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | idnumber | + | Category 1 | essay | Question to edit | Write about whatever you want | q1 | + | Category 2 | essay | Other question | Write about whatever you want | q2 | + And I navigate to "Question bank > Questions" in current page administration + And I click on "Edit" "link" in the "Question to edit" "table_row" + And I set the following fields to these values: + | Use this category | 0 | + | ID number | q2 | + | Save in category | Category 2 | + And I press "Save changes" + Then I should see "This ID number is already in use" + + @javascript + Scenario: Moving a question between categories can force a change to the idnumber + And the following "question categories" exist: + | contextlevel | reference | questioncategory | name | idnumber | + | Course | C1 | Top | top | | + | Course | C1 | top | Subcategory | c1sub | + | Course | C1 | top | Used category | c1used | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | idnumber | + | Used category | essay | Test question 1 | Write about whatever you want | q1 | + | Used category | essay | Test question 2 | Write about whatever you want | q2 | + | Subcategory | essay | Test question 3 | Write about whatever you want | q3 | + When I navigate to "Question bank > Questions" in current page administration + And I click on "Edit" "link" in the "Test question 3" "table_row" + # The q1 idnumber is allowed for this question while it is in the Subcategory. + And I set the field "ID number" to "q1" + And I press "submitbutton" + # Javascript is required for the next step. + And I click on "Test question 3" "checkbox" in the "Test question 3" "table_row" + And I set the field "Question category" to "Used category" + And I press "Move to >>" + And I click on "Edit" "link" in the "Test question 3" "table_row" + # The question just moved into this category needs to have a unique idnumber, so a number is appended. + Then the field "ID number" matches value "q1_1" diff --git a/question/tests/generator/lib.php b/question/tests/generator/lib.php index f807c93416e..bdbcca596fc 100644 --- a/question/tests/generator/lib.php +++ b/question/tests/generator/lib.php @@ -51,6 +51,7 @@ class core_question_generator extends component_generator_base { 'infoformat' => FORMAT_HTML, 'stamp' => make_unique_id_code(), 'sortorder' => 999, + 'idnumber' => null ); $record = $this->datagenerator->combine_defaults_and_record($defaults, $record); @@ -87,6 +88,7 @@ class core_question_generator extends component_generator_base { $question->category = $fromform->category; $question->qtype = $qtype; $question->createdby = 0; + $question->idnumber = null; return $this->update_question($question, $which, $overrides); } diff --git a/question/tests/generator_test.php b/question/tests/generator_test.php index 50f7ce9b770..945c9839aa8 100644 --- a/question/tests/generator_test.php +++ b/question/tests/generator_test.php @@ -51,4 +51,33 @@ class core_question_generator_testcase extends advanced_testcase { $this->assertSame('My category', $cat->name); $this->assertSame(1, $cat->sortorder); } + + public function test_idnumbers_in_categories_and_questions() { + $this->resetAfterTest(); + $generator = $this->getDataGenerator()->get_plugin_generator('core_question'); + list($category, $course, $qcat, $questions) = $generator->setup_course_and_questions(); + // Check default idnumbers in question_category and questions. + $this->assertNull($qcat->idnumber); + $this->assertNull($questions[0]->idnumber); + $this->assertNull($questions[1]->idnumber); + // Check created idnumbers. + $qcat1 = $generator->create_question_category(array( + 'name' => 'My category', 'sortorder' => 1, 'idnumber' => 'myqcat')); + $this->assertSame('myqcat', $qcat1->idnumber); + $quest1 = $generator->update_question($questions[0], null, ['idnumber' => 'myquest']); + $this->assertSame('myquest', $quest1->idnumber); + $quest3 = $generator->create_question('shortanswer', null, + ['name' => 'sa1', 'category' => $qcat1->id, 'idnumber' => 'myquest_3']); + $this->assertSame('myquest_3', $quest3->idnumber); + // Check idnumbers of questions moved. Note have to use load_question_data or we only get to see old cached data. + question_move_questions_to_category([$quest1->id], $qcat1->id); + $this->assertSame('myquest', question_bank::load_question_data($quest1->id)->idnumber); + // Can only change idnumber of quest2 once quest1 has been moved to another category. + $quest2 = $generator->update_question($questions[1], null, ['idnumber' => 'myquest']); + question_move_questions_to_category([$quest2->id], $qcat1->id); + $this->assertSame('myquest_4', question_bank::load_question_data($quest2->id)->idnumber); + // Check can add an idnumber of 0. + $quest4 = $generator->create_question('shortanswer', null, ['name' => 'sa1', 'category' => $qcat1->id, 'idnumber' => '0']); + $this->assertSame('0', $quest4->idnumber); + } } diff --git a/question/type/ddwtos/tests/questiontype_test.php b/question/type/ddwtos/tests/questiontype_test.php index 222c1869116..ded8ebef094 100644 --- a/question/type/ddwtos/tests/questiontype_test.php +++ b/question/type/ddwtos/tests/questiontype_test.php @@ -75,6 +75,7 @@ class qtype_ddwtos_test extends question_testcase { $dd->stamp = make_unique_id_code(); $dd->version = make_unique_id_code(); $dd->hidden = 0; + $dd->idnumber = null; $dd->timecreated = time(); $dd->timemodified = time(); $dd->createdby = $USER->id; @@ -122,6 +123,7 @@ class qtype_ddwtos_test extends question_testcase { $expected = test_question_maker::make_question('ddwtos'); $expected->stamp = $qdata->stamp; $expected->version = $qdata->version; + $expected->idnumber = null; $q = $this->qtype->make_question($qdata); @@ -247,6 +249,7 @@ class qtype_ddwtos_test extends question_testcase { $qdata = new stdClass(); $qdata->id = 123; $qdata->contextid = \context_system::instance()->id; + $qdata->idnumber = null; $qdata->qtype = 'ddwtos'; $qdata->name = 'A drag-and-drop question'; $qdata->questiontext = 'Put these in order: [[1]], [[2]], [[3]].'; @@ -304,6 +307,7 @@ class qtype_ddwtos_test extends question_testcase { 3 0.3333333 0 + 1 Your answer is correct.

]]> diff --git a/question/type/edit_question_form.php b/question/type/edit_question_form.php index 44302ed038a..7b5fb9cde0f 100644 --- a/question/type/edit_question_form.php +++ b/question/type/edit_question_form.php @@ -198,6 +198,10 @@ abstract class question_edit_form extends question_wizard_form { $mform->setType('generalfeedback', PARAM_RAW); $mform->addHelpButton('generalfeedback', 'generalfeedback', 'question'); + $mform->addElement('text', 'idnumber', get_string('idnumber', 'question'), 'maxlength="100" size="10"'); + $mform->addHelpButton('idnumber', 'idnumber', 'question'); + $mform->setType('idnumber', PARAM_RAW); + // Any questiontype specific fields. $this->definition_inner($mform); @@ -791,6 +795,8 @@ abstract class question_edit_form extends question_wizard_form { } public function validation($fromform, $files) { + global $DB; + $errors = parent::validation($fromform, $files); if (empty($fromform['makecopy']) && isset($this->question->id) && ($this->question->formoptions->canedit || @@ -810,6 +816,30 @@ abstract class question_edit_form extends question_wizard_form { $errors['defaultmark'] = get_string('defaultmarkmustbepositive', 'question'); } + // Can only have one idnumber per category. + if (strpos($fromform['category'], ',') !== false) { + list($category, $categorycontextid) = explode(',', $fromform['category']); + } else { + $category = $fromform['category']; + } + if (isset($fromform['idnumber']) && ((string) $fromform['idnumber'] !== '')) { + if (empty($fromform['usecurrentcat']) && !empty($fromform['categorymoveto'])) { + $categoryinfo = $fromform['categorymoveto']; + } else { + $categoryinfo = $fromform['category']; + } + list($categoryid, $notused) = explode(',', $categoryinfo); + $conditions = 'category = ? AND idnumber = ?'; + $params = [$categoryid, $fromform['idnumber']]; + if (!empty($this->question->id)) { + $conditions .= ' AND id <> ?'; + $params[] = $this->question->id; + } + if ($DB->record_exists_select('question', $conditions, $params)) { + $errors['idnumber'] = get_string('idnumbertaken', 'error'); + } + } + return $errors; } diff --git a/question/type/gapselect/tests/questiontype_test.php b/question/type/gapselect/tests/questiontype_test.php index 0a85673b8f4..6eea6cbdfa3 100644 --- a/question/type/gapselect/tests/questiontype_test.php +++ b/question/type/gapselect/tests/questiontype_test.php @@ -79,6 +79,7 @@ class qtype_gapselect_test extends question_testcase { $gapselect->stamp = make_unique_id_code(); $gapselect->version = make_unique_id_code(); $gapselect->hidden = 0; + $gapselect->idnumber = null; $gapselect->timecreated = time(); $gapselect->timemodified = time(); $gapselect->createdby = $USER->id; @@ -243,6 +244,7 @@ class qtype_gapselect_test extends question_testcase { $qdata = new stdClass(); $qdata->id = 123; $qdata->contextid = \context_system::instance()->id; + $qdata->idnumber = null; $qdata->qtype = 'gapselect'; $qdata->name = 'A select missing words question'; $qdata->questiontext = 'Put these in order: [[1]], [[2]], [[3]].'; @@ -294,6 +296,7 @@ class qtype_gapselect_test extends question_testcase { 3 0.3333333 0 + 1 Your answer is correct.

]]> diff --git a/question/type/match/tests/questiontype_test.php b/question/type/match/tests/questiontype_test.php index 2a8af385245..d60a0b4fb8d 100644 --- a/question/type/match/tests/questiontype_test.php +++ b/question/type/match/tests/questiontype_test.php @@ -68,6 +68,7 @@ class qtype_match_test extends advanced_testcase { $q->stamp = make_unique_id_code(); $q->version = make_unique_id_code(); $q->hidden = 0; + $q->idnumber = null; $q->timecreated = time(); $q->timemodified = time(); $q->createdby = $USER->id; diff --git a/question/type/missingtype/tests/missingtype_test.php b/question/type/missingtype/tests/missingtype_test.php index 76b289f243a..cc2a922db37 100644 --- a/question/type/missingtype/tests/missingtype_test.php +++ b/question/type/missingtype/tests/missingtype_test.php @@ -58,6 +58,7 @@ class qtype_missing_test extends question_testcase { $questiondata->stamp = make_unique_id_code(); $questiondata->version = make_unique_id_code(); $questiondata->hidden = 0; + $questiondata->idnumber = null; $questiondata->timecreated = 0; $questiondata->timemodified = 0; $questiondata->createdby = 0; diff --git a/question/type/questiontypebase.php b/question/type/questiontypebase.php index c812900adde..294975d1ee9 100644 --- a/question/type/questiontypebase.php +++ b/question/type/questiontypebase.php @@ -350,6 +350,19 @@ class question_type { $question->defaultmark = $form->defaultmark; } + if (isset($form->idnumber) && ((string) $form->idnumber !== '')) { + // While this check already exists in the form validation, this is a backstop preventing unnecessary errors. + if (strpos($form->category, ',') !== false) { + list($category, $categorycontextid) = explode(',', $form->category); + } else { + $category = $form->category; + } + if (!$DB->record_exists('question', + ['idnumber' => $form->idnumber, 'category' => $category])) { + $question->idnumber = $form->idnumber; + } + } + // If the question is new, create it. if (empty($question->id)) { // Set the unique code. @@ -851,6 +864,7 @@ class question_type { $question->stamp = $questiondata->stamp; $question->version = $questiondata->version; $question->hidden = $questiondata->hidden; + $question->idnumber = $questiondata->idnumber; $question->timecreated = $questiondata->timecreated; $question->timemodified = $questiondata->timemodified; $question->createdby = $questiondata->createdby; diff --git a/version.php b/version.php index b33f98b9864..a6e8909f8b4 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2018092000.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2018092100.04; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.