MDL-75809 customfields: select type should store value instead of index

admin/tool/uploadcourse/tests/course_test.php

@@ -1356,7 +1356,7 @@ public function test_custom_fields_data_required() {
 
         // Try again with a default value.
         $defaults = [
-            'customfield_myselect' => 2, // Our second option: Dog.
+            'customfield_myselect' => 'Dog', // Our second option: Dog.
         ];
 
         $uploader = new tool_uploadcourse_course($mode, $updatemode, $dataupload, $defaults);

admin/tool/uploadcourse/tests/helper_test.php

@@ -288,7 +288,7 @@ public function test_get_custom_course_field_data() {
         $expected = [
             'customfield_mycheckbox' => '1',
             'customfield_mydate' => strtotime('2019-10-01'),
-            'customfield_myselect' => 2,
+            'customfield_myselect' => 'Green',
             'customfield_mytext' => 'Hello',
         ];
 

backup/util/ui/tests/behat/restore_moodle2_courses.feature

@@ -48,6 +48,74 @@ Feature: Restore Moodle 2 course backups
     And the field "id_format" matches value "Custom sections"
     And I press "Cancel"
 
+  @javascript
+  Scenario: Restore a course with a customfield in a new course
+    Given the following "custom field categories" exist:
+      | name              | component   | area   | itemid |
+      | Category for test | core_course | course | 0      |
+    When I navigate to "Courses > Course custom fields" in site administration
+    And I click on "Add a new custom field" "link"
+    And I click on "Dropdown menu" "link"
+    And I set the following fields to these values:
+      | Name       | Test field1 |
+      | Short name | testfield1  |
+      | Locked     | No          |
+    And I set the field "Menu options (one per line)" to multiline:
+    """
+    a
+    b
+    """
+    And I click on "Save changes" "button" in the "Adding a new Dropdown menu" "dialogue"
+    And I am on "Course 1" course homepage
+    And I navigate to "Settings" in current page administration
+    And I set the following fields to these values:
+      | Test field1 | b |
+    And I press "Save and display"
+    And I am on "Course 1" course homepage with editing mode on
+    When I backup "Course 1" course using this options:
+      | Confirmation | Filename | test_backup.mbz |
+    And I restore "test_backup.mbz" backup into a new course using this options:
+      | Schema | Course name | Course 1 restored in a new course |
+      | Schema | Course short name | Course 1 restored in a new course |
+    And I am on "Course 1 restored in a new course" course homepage
+    And I navigate to "Settings" in current page administration
+    And I expand all fieldsets
+    Then the field "id_customfield_testfield1" matches value "b"
+    And I press "Cancel"
+
+  @javascript @_file_upload
+  Scenario: Restore a course with a customfield from a backup before MDL-75809
+    Given the following "custom field categories" exist:
+      | name              | component   | area   | itemid |
+      | Category for test | core_course | course | 0      |
+    When I navigate to "Courses > Course custom fields" in site administration
+    And I click on "Add a new custom field" "link"
+    And I click on "Dropdown menu" "link"
+    And I set the following fields to these values:
+      | Name       | Test field1  |
+      | Short name | dropdownmenu |
+      | Locked     | No           |
+    And I set the field "Menu options (one per line)" to multiline:
+    """
+    First
+    Second
+    Third
+    Fourth
+    """
+    And I click on "Save changes" "button" in the "Adding a new Dropdown menu" "dialogue"
+    And I am on the "Course 1" "restore" page logged in as "admin"
+    And I press "Manage backup files"
+    And I upload "backup/moodle2/tests/fixtures/pre-mdl-75809.mbz" file to "Files" filemanager
+    And I press "Save changes"
+    And I restore "pre-mdl-75809.mbz" backup into a new course using this options:
+      | Schema | Course name       | Course 5 |
+      | Schema | Course short name | C5       |
+    And I am on "C5" course homepage
+    And I navigate to "Settings" in current page administration
+    And I expand all fieldsets
+    Then the field "id_customfield_dropdownmenu" matches value "Second"
+    And I press "Cancel"
+
   @javascript
   Scenario: Restore a backup into the same course
     When I backup "Course 3" course using this options:

course/tests/courselib_test.php

@@ -5054,7 +5054,7 @@ public function get_course_filter_courses_by_customfield_test_cases() {
                 'shortname' => 'C1',
                 'customfield_checkboxfield' => 1,
                 'customfield_datefield' => strtotime('2001-02-01T12:00:00Z'),
-                'customfield_selectfield' => 1,
+                'customfield_selectfield' => 'Option 1',
                 'customfield_textfield' => 'fish',
             ],
             [
@@ -5066,19 +5066,19 @@ public function get_course_filter_courses_by_customfield_test_cases() {
                 'shortname' => 'C3',
                 'customfield_checkboxfield' => 0,
                 'customfield_datefield' => strtotime('2001-02-01T12:00:00Z'),
-                'customfield_selectfield' => 2,
+                'customfield_selectfield' => 'Option 2',
                 'customfield_textfield' => 'dog',
             ],
             [
                 'shortname' => 'C4',
                 'customfield_checkboxfield' => 1,
-                'customfield_selectfield' => 3,
+                'customfield_selectfield' => 'Option 3',
                 'customfield_textfield' => 'cat',
             ],
             [
                 'shortname' => 'C5',
                 'customfield_datefield' => strtotime('1980-08-06T13:00:00Z'),
-                'customfield_selectfield' => 2,
+                'customfield_selectfield' => 'Option 2',
                 'customfield_textfield' => 'fish',
             ],
         ];
@@ -5141,7 +5141,7 @@ public function get_course_filter_courses_by_customfield_test_cases() {
             'select Option 1' => [
                 'coursedata' => $coursedata,
                 'customfield' => 'selectfield',
-                'customfieldvalue' => 1,
+                'customfieldvalue' => 'Option 1',
                 'limit' => 10,
                 'offset' => 0,
                 'expectedcourses' => ['C1'],
@@ -5150,7 +5150,7 @@ public function get_course_filter_courses_by_customfield_test_cases() {
             'select Option 2' => [
                 'coursedata' => $coursedata,
                 'customfield' => 'selectfield',
-                'customfieldvalue' => 2,
+                'customfieldvalue' => 'Option 2',
                 'limit' => 10,
                 'offset' => 0,
                 'expectedcourses' => ['C3', 'C5'],

course/tests/customfield_test.php

@@ -61,7 +61,7 @@ public function test_create_course() {
         $c1 = $dg->create_course(['shortname' => 'SN', 'fullname' => 'FN',
             'summary' => 'DESC', 'summaryformat' => FORMAT_MOODLE,
             'customfield_f1' => 'some text', 'customfield_f2' => 1,
-            'customfield_f3' => $now, 'customfield_f4' => 2,
+            'customfield_f3' => $now, 'customfield_f4' => 'b',
             'customfield_f5_editor' => ['text' => 'test', 'format' => FORMAT_HTML]]);
 
         $data = \core_course\customfield\course_handler::create()->export_instance_data_object($c1->id);

customfield/field/select/classes/data_controller.php

@@ -40,7 +40,7 @@ class data_controller extends \core_customfield\data_controller {
      * @return string
      */
     public function datafield() : string {
-        return 'intvalue';
+        return 'charvalue';
     }
 
     /**
@@ -49,34 +49,21 @@ public function datafield() : string {
      * @return mixed
      */
     public function get_default_value() {
-        $defaultvalue = $this->get_field()->get_configdata_property('defaultvalue');
-        if ('' . $defaultvalue !== '') {
-            $key = array_search($defaultvalue, $this->get_field()->get_options());
-            if ($key !== false) {
-                return $key;
-            }
-        }
-        return 0;
+        return $this->get_field()->get_configdata_property('defaultvalue');
     }
 
     /**
-     * Add fields for editing a textarea field.
+     * Add fields for editing a select field.
      *
      * @param \MoodleQuickForm $mform
      */
     public function instance_form_definition(\MoodleQuickForm $mform) {
         $field = $this->get_field();
         $config = $field->get('configdata');
         $options = $field->get_options();
-        $formattedoptions = array();
-        $context = $this->get_field()->get_handler()->get_configuration_context();
-        foreach ($options as $key => $option) {
-            // Multilang formatting with filters.
-            $formattedoptions[$key] = format_string($option, true, ['context' => $context]);
-        }
 
         $elementname = $this->get_form_element_name();
-        $mform->addElement('select', $elementname, $this->get_field()->get_formatted_name(), $formattedoptions);
+        $mform->addElement('select', $elementname, $this->get_field()->get_formatted_name(), $options);
 
         if (($defaultkey = array_search($config['defaultvalue'], $options)) !== false) {
             $mform->setDefault($elementname, $defaultkey);
@@ -114,15 +101,39 @@ public function export_value() {
         $value = $this->get_value();
 
         if ($this->is_empty($value)) {
-            return null;
+            return $this->get_default_value();
         }
 
         $options = $this->get_field()->get_options();
-        if (array_key_exists($value, $options)) {
+        if (in_array($value, $options)) {
             return format_string($options[$value], true,
                 ['context' => $this->get_field()->get_handler()->get_configuration_context()]);
+        } else {
+            return $this->get_default_value();
         }
+    }
 
-        return null;
+    /**
+     * Returns the value as it is stored in the database or default value if data record is not present
+     *
+     * @return mixed
+     */
+    public function get_value() {
+        if (!$this->get('id')) {
+            return $this->get_default_value();
+        }
+        $value = $this->get($this->datafield());
+        $options = $this->get_field()->get_options();
+        if (isset($options[$value])) {
+            return $value;
+        }
+        // This is to deal with courses restored from backups made pre MDL-75809.
+        if (is_numeric($value) && !in_array($value, $options)) {
+            $optionsarray = array_values($options);
+            if (isset($optionsarray[$value])) {
+                $value = $optionsarray[$value];
+            }
+        }
+        return $value;
     }
 }

customfield/field/select/classes/field_controller.php

@@ -66,7 +66,12 @@ public function get_options(): array {
         } else {
             $options = array();
         }
-        return array_merge([''], $options);
+        $optionslist = ['' => ''];
+        $context = $this->get_handler()->get_configuration_context();
+        foreach ($options as $o) {
+            $optionslist[$o] = format_string($o, true, ['context' => $context]);
+        }
+        return $optionslist;
     }
 
     /**
@@ -118,14 +123,4 @@ public function course_grouping_format_values($values): array {
             $this->get_formatted_name());
         return $ret;
     }
-
-    /**
-     * Locate the value parameter in the field options array, and return it's index
-     *
-     * @param string $value
-     * @return int
-     */
-    public function parse_value(string $value) {
-        return (int) array_search($value, $this->get_options());
-    }
-}
+}

customfield/field/select/tests/plugin_test.php

@@ -59,8 +59,8 @@ public function setUp(): void {
         $this->courses[2] = $this->getDataGenerator()->create_course();
         $this->courses[3] = $this->getDataGenerator()->create_course();
 
-        $this->cfdata[1] = $this->get_generator()->add_instance_data($this->cfields[1], $this->courses[1]->id, 1);
-        $this->cfdata[2] = $this->get_generator()->add_instance_data($this->cfields[1], $this->courses[2]->id, 1);
+        $this->cfdata[1] = $this->get_generator()->add_instance_data($this->cfields[1], $this->courses[1]->id, 'a');
+        $this->cfdata[2] = $this->get_generator()->add_instance_data($this->cfields[1], $this->courses[2]->id, 'a');
 
         $this->setUser($this->getDataGenerator()->create_user());
     }
@@ -125,7 +125,7 @@ public function test_instance_form() {
         $this->assertFalse($form->is_validated());
 
         // Now with required field.
-        $submitdata['customfield_myfield2'] = 1;
+        $submitdata['customfield_myfield2'] = 'a';
         core_customfield_test_instance_form::mock_submit($submitdata, []);
         $form = new core_customfield_test_instance_form('POST',
             ['handler' => $handler, 'instance' => $this->courses[1]]);
@@ -141,51 +141,15 @@ public function test_instance_form() {
      * Test for data_controller::get_value and export_value
      */
     public function test_get_export_value() {
-        $this->assertEquals(1, $this->cfdata[1]->get_value());
+        $this->assertEquals('a', $this->cfdata[1]->get_value());
         $this->assertEquals('a', $this->cfdata[1]->export_value());
 
         // Field without data but with a default value.
         $d = \core_customfield\data_controller::create(0, null, $this->cfields[3]);
-        $this->assertEquals(2, $d->get_value());
+        $this->assertEquals('b', $d->get_value());
         $this->assertEquals('b', $d->export_value());
     }
 
-    /**
-     * Data provider for {@see test_parse_value}
-     *
-     * @return array
-     */
-    public function parse_value_provider() : array {
-        return [
-            ['Red', 1],
-            ['Blue', 2],
-            ['Green', 3],
-            ['Mauve', 0],
-        ];
-    }
-
-    /**
-     * Test field parse_value method
-     *
-     * @param string $value
-     * @param int $expected
-     * @return void
-     *
-     * @dataProvider parse_value_provider
-     */
-    public function test_parse_value(string $value, int $expected) {
-        $field = $this->get_generator()->create_field([
-            'categoryid' => $this->cfcat->get('id'),
-            'type' => 'select',
-            'shortname' => 'myselect',
-            'configdata' => [
-                'options' => "Red\nBlue\nGreen",
-            ],
-        ]);
-
-        $this->assertSame($expected, $field->parse_value($value));
-    }
-
     /**
      * Deleting fields and data
      */

customfield/tests/generator_test.php

@@ -96,7 +96,7 @@ public function test_add_instance_data() {
         $this->assertNotEmpty($data2->get('id'));
         $this->assertEquals(1546300800, $data2->get_value());
         $this->assertNotEmpty($data3->get('id'));
-        $this->assertEquals(2, $data3->get_value());
+        $this->assertEquals('b', $data3->get_value());
         $this->assertNotEmpty($data4->get('id'));
         $this->assertEquals('Hello', $data4->get_value());
         $this->assertNotEmpty($data5->get('id'));

lib/db/upgrade.php

@@ -474,6 +474,7 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2023062200.00);
     }
 
+
     if ($oldversion < 2023062700.01) {
         // Define field name to be added to external_tokens.
         $table = new xmldb_table('external_tokens');
@@ -979,5 +980,33 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2024012300.00);
     }
 
+    if ($oldversion < 2024012500.01) {
+        if ($fields = $DB->get_records('customfield_field', ['type' => 'select'])) {
+            foreach ($fields as $f) {
+                $configdata = json_decode($f->configdata);
+                $options = preg_split("/\s*\n\s*/", trim($configdata->options));
+                if ($data = $DB->get_records('customfield_data', ['fieldid' => $f->id])) {
+                    $total = count($data);
+                    if ($total > 0) {
+                        $total = 1; // Avoid division by zero.
+                        $pbar = new progress_bar('upgradeselectcustomfielddata', 500, true);
+                        $i = 0;
+                        foreach ($data as $d) {
+                            if (empty($d->charvalue)) {
+                                $d->charvalue = $options[$d->intvalue - 1]; // Old code added an 'empty' to beginning of the list.
+                                $DB->update_record('customfield_data', $d);
+                            }
+                            // Update progress.
+                            $i++;
+                            $pbar->update($i, $ftotal, "Migrating customfield data - {$i}/{$ftotal}.");
+                        }
+                    }
+                }
+            }
+        }
+        // Main savepoint reached.
+        upgrade_main_savepoint(true, 2024012500.01);
+    }
+
     return true;
 }