fix: don't try to preserve invalid values for non-selects

Author: MHajoha

classes/attempt_ui/invalid_option_warning.php

@@ -34,19 +34,26 @@ class invalid_option_warning {
      * @param string $name name of the input field in question
      * @param string $value value from the last response, for which no option was found in the UI
      * @param array $availablevalues the available options present in the UI
+     * @param bool $preserved true indicates that the HTML was modified in such a way that the invalid value will still be present
+     *                        in future submissions unless explicitly changed
      */
     public function __construct(
         /** @var string $name name of the input field in question */
         public string $name,
         /** @var string $value value from the last response, for which no option was found in the UI */
         public string $value,
         /** @var array $availablevalues the available options present in the UI */
-        public array $availablevalues
+        public array $availablevalues,
+        /**
+         * @var bool $preserved true indicates that the HTML was modified in such a way that the invalid value will still be present
+         *                      in future submissions unless explicitly changed
+         */
+        public bool $preserved,
     ) {
     }
 
     /**
-     * Return a localized string describing this warning to humans. Name and values are escaped.
+     * Return a localized HTML string describing this warning to humans. Name and values are escaped.
      *
      * @return string
      * @throws coding_exception
@@ -57,10 +64,18 @@ public function localize(): string {
             $this->availablevalues
         ));
 
-        return get_string('render_warning_invalid_value', 'qtype_questionpy', [
+        $msg = get_string('render_warning_invalid_value', 'qtype_questionpy', [
             'name' => html_writer::tag('code', s($this->name)),
             'value' => html_writer::tag('code', s($this->value)),
             'availablevalues' => $availablevaluesstr,
         ]);
+
+        if ($this->preserved) {
+            $msg .= ' ' . get_string('render_warning_invalid_value_preserved', 'qtype_questionpy');
+        } else {
+            $msg .= ' ' . get_string('render_warning_invalid_value_not_preserved', 'qtype_questionpy');
+        }
+
+        return $msg;
     }
 }

classes/attempt_ui/question_ui_renderer.php

@@ -795,15 +795,19 @@ private function check_for_and_preserve_unknown_options(array $availableoptsinfo
 
             foreach ($lastvalues as $lastvalue) {
                 if (!in_array($lastvalue, $info->availableoptions)) {
-                    $warnings[] = new invalid_option_warning($name, $lastvalue, $info->availableoptions);
-                    if ($info->type !== 'select') {
-                        // Selects are handled in dom_utils::set_select_value.
-                        // This should work for single-valued and multi-valued fields alike: The invalid checkbox(es) and radio(s)
-                        // will have been unchecked by set_input_values_and_readonly, so this hidden input will be the only one,
-                        // ensuring that single-valued fields only receive a single value. For multi-valued fields, the hidden input
-                        // value will simply be added to the others in JS (addIframeFormDataOnSubmit).
-                        dom_utils::add_hidden_input($this->xml->documentElement, $name, $lastvalue);
-                    }
+                    // We don't preserve values for any type other than selects because it would be difficult to then remove the
+                    // invalid value:
+                    // For multi-valued fields, there would be no way for us to know whether the intention was to replace the
+                    // invalid value or add a new one.
+                    // For single-valued fields, while we can assume that any valid value should overwrite the invalid value, that
+                    // would add a fair bit of complexity for little benefit.
+
+                    $warnings[] = new invalid_option_warning(
+                        $name,
+                        $lastvalue,
+                        $info->availableoptions,
+                        preserved: $info->type === 'select'
+                    );
                 }
             }
         }

lang/en/qtype_questionpy.php

@@ -50,8 +50,9 @@
 $string['remove_packages_button'] = 'Remove Packages';
 $string['render_error_section'] = 'An error occurred';
 $string['render_warning_invalid_value'] = 'The last submission set the field {$a->name} to the value {$a->value}, but that option is no longer available. The available options are: {$a->availablevalues}.';
+$string['render_warning_invalid_value_not_preserved'] = 'The invalid value will be overwritten the next time you save your answer.';
+$string['render_warning_invalid_value_preserved'] = 'You may set the field to a valid value, or leave it untouched to preserve the invalid value.';
 $string['render_warnings_hint_contact_teachers'] = 'If you believe this to be in error, contact your teachers. They will also see this notice and may decide to override whichever score you receive.';
-$string['render_warnings_hint_editable'] = 'You may set the mentioned field(s) to a valid value, or leave them untouched to preserve the invalid value.';
 $string['request_error'] = 'The {$a->requestmethod} request to "{$a->uri}" failed with the error code "{$a->errorcode}" and'
     . ' status code {$a->statuscode} ({$a->reasonphrase}).';
 $string['same_version_different_hash_error'] = 'A package with the same version but different hash already exists.';

renderer.php

@@ -205,7 +205,6 @@ protected function formulation_controls_feedback_in_iframe(question_attempt $qa,
                 'warnings' => $renderer->warnings,
                 'should_use_list' => count($renderer->warnings) > 1,
                 'should_show_hint_contact_teachers' => $isstudent,
-                'should_show_hint_editable' => $isstudent && !$options->readonly,
             ]);
         }
 

templates/render_warnings.mustache

@@ -22,7 +22,6 @@
     Context variables required for this template:
     * warnings - List of invalid_option_warnings. The localize() method will be called.
     * should_use_list - To use an HTML list. Otherwise, just outputs paragraphs.
-    * should_show_hint_editable - To show a hint about the possibility of fixing the invalid values.
     * should_show_hint_contact_teachers - To show a hint about contacting teachers.
 
     Example context (json):
@@ -31,7 +30,6 @@
                 "localize": "The last submission set the field <code>my_select</code> to ..."
         }],
         "should_use_list": false,
-        "should_show_hint_editable": true,
         "should_show_hint_contact_teachers": true
     }
 }}
@@ -49,9 +47,6 @@
         {{/warnings}}
     {{/should_use_list}}
 
-    {{#should_show_hint_editable}}
-        <p>{{#str}} render_warnings_hint_editable, qtype_questionpy {{/str}}</p>
-    {{/should_show_hint_editable}}
     {{#should_show_hint_contact_teachers}}
         <p>{{#str}} render_warnings_hint_contact_teachers, qtype_questionpy {{/str}}</p>
     {{/should_show_hint_contact_teachers}}

tests/attempt_ui/question_ui_renderer_test.php

@@ -495,10 +495,10 @@ public function test_should_warn_about_invalid_values(): void {
 
         $result = question_ui_renderer::render($input, [], new \question_display_options(), $qa);
         $this->assertEqualsCanonicalizing([
-            new invalid_option_warning('my_checkbox_value', 'other_value', ['value']),
-            new invalid_option_warning('my_checkbox_on', 'schmon', ['on']),
-            new invalid_option_warning('my_radio', 'value13', ['value1', 'value2']),
-            new invalid_option_warning('my_select', 'value42', ['value1', 'value2', 'value3']),
+            new invalid_option_warning('my_checkbox_value', 'other_value', ['value'], preserved: false),
+            new invalid_option_warning('my_checkbox_on', 'schmon', ['on'], preserved: false),
+            new invalid_option_warning('my_radio', 'value13', ['value1', 'value2'], preserved: false),
+            new invalid_option_warning('my_select', 'value42', ['value1', 'value2', 'value3'], preserved: true),
         ], $result->warnings);
     }