MDL-63752 question import: duplicate key error when idnumbers blank

And extra checks to head off possible idnumber='' issues in two other places.

diff --git a/question/category_class.php b/question/category_class.php
index 3bccfc9..8876429 100644 (file)
--- a/question/category_class.php
+++ b/question/category_class.php
@@ -420,7 +420,9 @@ class question_category_object {
             }
         }
 
-        if (((string) $idnumber !== '') && !empty($contextid)) {
+        if ((string) $idnumber === '') {
+            $idnumber = null;
+        } else if (!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])) {
@@ -493,7 +495,9 @@ class question_category_object {
             }
         }
 
-        if (((string) $idnumber !== '') && !empty($tocontextid)) {
+        if ((string) $idnumber === '') {
+            $idnumber = null;
+        } else if (!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])) {

diff --git a/question/format.php b/question/format.php
index 0ba280e..d2261db 100644 (file)
--- a/question/format.php
+++ b/question/format.php
@@ -406,10 +406,16 @@ 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.
+            if (isset($question->idnumber)) {
+                if ((string) $question->idnumber === '') {
+                    // Id number not really set. Get rid of it.
                     unset($question->idnumber);
+                } else {
+                    if ($DB->record_exists('question',
+                            ['idnumber' => $question->idnumber, 'category' => $question->category])) {
+                        // We cannot have duplicate idnumbers in a category. Just remove it.
+                        unset($question->idnumber);
+                    }
                 }
             }
 

diff --git a/question/format/xml/tests/fixtures/nested_categories_with_questions.xml b/question/format/xml/tests/fixtures/nested_categories_with_questions.xml
index 0ea1753..db729e9 100644 (file)
--- a/question/format/xml/tests/fixtures/nested_categories_with_questions.xml
+++ b/question/format/xml/tests/fixtures/nested_categories_with_questions.xml
@@ -63,7 +63,7 @@
     <defaultgrade>1.0000000</defaultgrade>
     <penalty>0.0000000</penalty>
     <hidden>0</hidden>
-    <idnumber>K1</idnumber>
+    <idnumber></idnumber>
     <responseformat>editor</responseformat>
     <responserequired>1</responserequired>
     <responsefieldlines>10</responsefieldlines>
@@ -91,7 +91,7 @@
     <defaultgrade>1.0000000</defaultgrade>
     <penalty>1.0000000</penalty>
     <hidden>0</hidden>
-    <idnumber>K2</idnumber>
+    <idnumber></idnumber>
     <answer fraction="100" format="moodle_auto_format">
       <text>true</text>
       <feedback format="html">

diff --git a/question/format/xml/tests/qformat_xml_import_export_test.php b/question/format/xml/tests/qformat_xml_import_export_test.php
index 44d3091..13724c3 100644 (file)
--- a/question/format/xml/tests/qformat_xml_import_export_test.php
+++ b/question/format/xml/tests/qformat_xml_import_export_test.php
@@ -366,7 +366,7 @@ class qformat_xml_import_export_test extends advanced_testcase {
                 'attachmentsrequired' => 0,
                 'graderinfo' => ['format' => '1', 'text' => ''],
                 'responsetemplate' => ['format' => '1', 'text' => ''],
-                'idnumber' => 'K1']);
+                'idnumber' => '']);
         $kappaquestion1  = $generator->create_question('truefalse', null, [
                 'category' => $categorykappa->id,
                 'name' => 'Kappa Question',
@@ -378,7 +378,7 @@ class qformat_xml_import_export_test extends advanced_testcase {
                 'feedbacktrue' => ['format' => '1', 'text' => ''],
                 'feedbackfalse' => ['format' => '1', 'text' => ''],
                 'penalty' => '1',
-                'idnumber' => 'K2']);
+                'idnumber' => '']);
         $categorylambda = $generator->create_question_category([
                 'name' => 'Lambda',
                 'contextid' => '2',