From bb1d1d9aafce40da8a66ad64307e3deb0dcf72a7 Mon Sep 17 00:00:00 2001 From: Maximilian Haye Date: Thu, 5 Dec 2024 18:23:27 +0100 Subject: [PATCH] wip: orphan values --- classes/attempt_ui/dom_utils.php | 1 - classes/attempt_ui/invalid_option_warning.php | 42 +++++++++ .../{ => attempt_ui}/question_metadata.php | 4 +- .../question_ui_metadata_extractor.php | 1 - classes/attempt_ui/question_ui_renderer.php | 91 +++++++++++++++---- classes/attempt_ui/render_result.php | 27 ++++++ classes/constants.php | 2 + classes/utils.php | 12 ++- lang/en/qtype_questionpy.php | 1 + question.php | 4 +- renderer.php | 28 +++++- .../question_ui_metadata_extractor_test.php | 4 +- 12 files changed, 190 insertions(+), 27 deletions(-) create mode 100644 classes/attempt_ui/invalid_option_warning.php rename classes/{ => attempt_ui}/question_metadata.php (96%) create mode 100644 classes/attempt_ui/render_result.php diff --git a/classes/attempt_ui/dom_utils.php b/classes/attempt_ui/dom_utils.php index d889cb0..ac52e75 100644 --- a/classes/attempt_ui/dom_utils.php +++ b/classes/attempt_ui/dom_utils.php @@ -96,7 +96,6 @@ public static function html_to_fragment(DOMDocument $doc, string $html, int $opt * * @param DOMElement $select * @param string $value - * @throws DOMException * @throws coding_exception */ public static function set_select_value(DOMElement $select, string $value): void { diff --git a/classes/attempt_ui/invalid_option_warning.php b/classes/attempt_ui/invalid_option_warning.php new file mode 100644 index 0000000..dc6c105 --- /dev/null +++ b/classes/attempt_ui/invalid_option_warning.php @@ -0,0 +1,42 @@ +. + +namespace qtype_questionpy\attempt_ui; + +use coding_exception; +use html_writer; + +class invalid_option_warning { + public function __construct( + public string $name, + public string $value, + public array $availablevalues + ) { + } + + /** + * @throws coding_exception + */ + public function localize(): string { + $availablevaluesstr = implode(', ', array_map(fn($value) => html_writer::tag('code', format_string($value)), $this->availablevalues)); + + return get_string('render_warning_invalid_value', 'qtype_questionpy', [ + 'name' => html_writer::tag('code', format_string($this->name)), + 'value' => html_writer::tag('code', format_string($this->value)), + 'availablevalues' => $availablevaluesstr, + ]); + } +} diff --git a/classes/question_metadata.php b/classes/attempt_ui/question_metadata.php similarity index 96% rename from classes/question_metadata.php rename to classes/attempt_ui/question_metadata.php index 69eec6d..950a660 100644 --- a/classes/question_metadata.php +++ b/classes/attempt_ui/question_metadata.php @@ -14,9 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Moodle. If not, see . -namespace qtype_questionpy; - -use qtype_questionpy\attempt_ui\question_ui_renderer; +namespace qtype_questionpy\attempt_ui; /** * Metadata about a question attempt, extracted by {@see question_ui_renderer} from the XML. diff --git a/classes/attempt_ui/question_ui_metadata_extractor.php b/classes/attempt_ui/question_ui_metadata_extractor.php index 2cfce6c..fb413de 100644 --- a/classes/attempt_ui/question_ui_metadata_extractor.php +++ b/classes/attempt_ui/question_ui_metadata_extractor.php @@ -21,7 +21,6 @@ use DOMElement; use DOMXPath; use qtype_questionpy\constants; -use qtype_questionpy\question_metadata; /** * Parses the question UI XML and extracts the metadata. diff --git a/classes/attempt_ui/question_ui_renderer.php b/classes/attempt_ui/question_ui_renderer.php index aef713c..f91e8a1 100644 --- a/classes/attempt_ui/question_ui_renderer.php +++ b/classes/attempt_ui/question_ui_renderer.php @@ -27,6 +27,7 @@ use DOMText; use DOMXPath; use qtype_questionpy\constants; +use qtype_questionpy\utils; use qtype_questionpy_question; use question_attempt; use question_display_options; @@ -46,9 +47,6 @@ class question_ui_renderer { /** @var DOMXPath $xpath */ private DOMXPath $xpath; - /** @var string|null $html */ - private ?string $html = null; - /** @var array $placeholders */ private array $placeholders; @@ -58,10 +56,13 @@ class question_ui_renderer { /** @var question_attempt $attempt */ private question_attempt $attempt; + /** @var render_result|null $result */ + private ?render_result $result = null; + /** * Parses the given XML and initializes a new {@see question_ui_renderer} instance. * - * @param string $xml XML as returned by the QPy Server + * @param string $xml XML as returned by the QPy Server * @param array $placeholders string to string mapping of placeholder names to the values * @param question_display_options $options * @param question_attempt $attempt @@ -87,13 +88,12 @@ public function __construct(string $xml, array $placeholders, question_display_o /** * Renders the given XML to HTML. * - * @return string rendered html + * @return render_result * @throws coding_exception - * @throws DOMException */ - public function render(): string { - if (!is_null($this->html)) { - return $this->html; + public function render(): render_result { + if ($this->result) { + return $this->result; } $nextseed = mt_rand(); @@ -110,6 +110,8 @@ public function render(): string { $this->shuffle_contents(); $this->format_floats(); + $availableoptions = $this->extract_available_options(); + // Remove all unhandled custom elements, attributes, comments, and non-default xmlns declarations. $this->clean_up(); @@ -132,8 +134,9 @@ public function render(): string { mt_srand($nextseed); } - $this->html = $this->xml->saveHTML(); - return $this->html; + $warnings = $this->check_for_unknown_options($availableoptions); + $this->result = new render_result($this->xml->saveHTML(), $warnings); + return $this->result; } /** @@ -283,7 +286,7 @@ private function mangle_ids_and_names(): void { * * @return void * - * @throws DOMException|coding_exception + * @throws coding_exception */ private function set_input_values_and_readonly(): void { /** @var DOMElement $element */ @@ -505,9 +508,9 @@ private function hide_if_role(): void { if ( !(in_array('teacher', $allowedroles) && $isteacher - || in_array('proctor', $allowedroles) && $isteacher - || in_array('scorer', $allowedroles) && $isscorer - || in_array('developer', $allowedroles) && $isdeveloper) + || in_array('proctor', $allowedroles) && $isteacher + || in_array('scorer', $allowedroles) && $isscorer + || in_array('developer', $allowedroles) && $isdeveloper) ) { $attr->ownerElement->parentNode->removeChild($attr->ownerElement); } @@ -566,7 +569,7 @@ private function replace_qpy_urls(string $input): string { assert($question instanceof qtype_questionpy_question); return preg_replace_callback( - // The first two path segments are namespace and short name, and so more restrictive. + // The first two path segments are namespace and short name, and so more restrictive. ';qpy://static((?:/[a-z_][a-z0-9_]{0,126}){2}(?:/[\w\-@:%+.~=]+)+);', function (array $match) use ($question) { $path = $match[1]; @@ -584,4 +587,60 @@ function (array $match) use ($question) { $input ); } + + private function extract_available_options(): array { + $optionsbyname = []; + + /** @var DOMElement $select */ + foreach ($this->xpath->query('//xhtml:select') as $select) { + $name = $select->getAttribute('name'); + if (!$name) { + continue; + } + + $values = []; + /** @var DOMElement $option */ + foreach ($this->xpath->query('./xhtml:option | ./xhtml:optgroup/xhtml:option', $select) as $option) { + $values[] = $option->hasAttribute('value') ? $option->getAttribute('value') : $option->textContent; + } + + $optionsbyname[$name] = array_unique($values); + } + + /** @var DOMElement $input */ + foreach ($this->xpath->query('//xhtml:input[(@type="checkbox" or @type="radio") and not(@qpy:warn-on-unknown-option = "no")]') as $input) { + $name = $input->getAttribute('name'); + if (!$name) { + continue; + } + + if (!array_key_exists($name, $optionsbyname)) { + $optionsbyname[$name] = []; + } + + $value = $input->hasAttribute('value') ? $input->getAttribute('value') : 'on'; + if (!in_array($value, $optionsbyname[$name])) { + $optionsbyname[$name][] = $value; + } + } + + return $optionsbyname; + } + + private function check_for_unknown_options(array $availableoptionsbyname): array { + $response = utils::get_last_response($this->attempt); + + $warnings = []; + foreach ($availableoptionsbyname as $name => $availableoptions) { + if (!array_key_exists($name, $response)) { + continue; + } + + $lastvalue = $response[$name]; + if (!in_array($lastvalue, $availableoptions)) { + $warnings[] = new invalid_option_warning($name, $lastvalue, $availableoptions); + } + } + return $warnings; + } } diff --git a/classes/attempt_ui/render_result.php b/classes/attempt_ui/render_result.php new file mode 100644 index 0000000..aa3b771 --- /dev/null +++ b/classes/attempt_ui/render_result.php @@ -0,0 +1,27 @@ +. + +namespace qtype_questionpy\attempt_ui; + +class render_result { + public function __construct( + /** @var string $html */ + public string $html, + /** @var invalid_option_warning[] $warnings */ + public array $warnings + ) { + } +} diff --git a/classes/constants.php b/classes/constants.php index d7ac62a..da1cd25 100644 --- a/classes/constants.php +++ b/classes/constants.php @@ -34,4 +34,6 @@ class constants { public const QT_VAR_ATTEMPT_STATE = '_attemptstate'; /** @var string */ public const QT_VAR_SCORING_STATE = '_scoringstate'; + /** @var string */ + public const QT_VAR_RESPONSE_MARKER = 'qpy_marker'; } diff --git a/classes/utils.php b/classes/utils.php index 48bbd34..7b0eea1 100644 --- a/classes/utils.php +++ b/classes/utils.php @@ -17,6 +17,7 @@ namespace qtype_questionpy; use qtype_questionpy\form\elements\repetition_element; +use question_attempt; /** * Utility functions used in multiple places. @@ -124,6 +125,15 @@ public static function reindex_integer_arrays(array &$array): void { * @see question_attempt_step for the meaning of different step var prefixes */ public static function filter_for_response(array $qtvars): array { - return array_filter($qtvars, fn($key) => !str_starts_with($key, '_'), ARRAY_FILTER_USE_KEY); + return array_filter( + $qtvars, + fn($key) => !str_starts_with($key, '_') && $key !== constants::QT_VAR_RESPONSE_MARKER, + ARRAY_FILTER_USE_KEY + ); + } + + public static function get_last_response(question_attempt $qa): array { + $step = $qa->get_last_step_with_qt_var(constants::QT_VAR_RESPONSE_MARKER); + return self::filter_for_response($step->get_qt_data()); } } diff --git a/lang/en/qtype_questionpy.php b/lang/en/qtype_questionpy.php index 7b13fcc..30398a8 100644 --- a/lang/en/qtype_questionpy.php +++ b/lang/en/qtype_questionpy.php @@ -50,6 +50,7 @@ $string['question_package_upload'] = 'Upload your own'; $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['same_version_different_hash_error'] = 'A package with the same version but different hash already exists.'; $string['search_all_header'] = 'All ({$a})'; $string['search_bar'] = 'Search...'; diff --git a/question.php b/question.php index 1386d6c..65d7319 100644 --- a/question.php +++ b/question.php @@ -220,7 +220,9 @@ public function get_expected_data(): array|string { // There was an error -> get all the submitted data. return question_attempt::USE_RAW_DATA; } - return $this->metadata->extract()->expecteddata; + return array_merge($this->metadata->extract()->expecteddata, [ + constants::QT_VAR_RESPONSE_MARKER => PARAM_ALPHA, + ]); } /** diff --git a/renderer.php b/renderer.php index 560ad84..e08de20 100644 --- a/renderer.php +++ b/renderer.php @@ -23,6 +23,7 @@ */ use qtype_questionpy\attempt_ui\question_ui_renderer; +use qtype_questionpy\constants; /** * Generates the output for QuestionPy questions. @@ -65,7 +66,32 @@ public function formulation_and_controls(question_attempt $qa, question_display_ } $renderer = new question_ui_renderer($question->ui->formulation, $question->ui->placeholders, $options, $qa); - return $renderer->render(); + $renderresult = $renderer->render(); + + $output = ''; + if ($renderresult->warnings) { + // TODO: Might want to make this a template. + $output .= html_writer::start_div('alert alert-warning'); + foreach ($renderresult->warnings as $warning) { + $output .= html_writer::tag('p', $warning->localize()); + // Preserve the invalid value. TODO: Remove this when value is changed? + $output .= html_writer::empty_tag('input', [ + 'type' => 'hidden', + 'name' => $warning->name, + 'value' => $warning->value, + ]); + } + $output .= html_writer::end_div(); + } + + $output .= html_writer::empty_tag('input', [ + 'type' => 'hidden', + 'name' => $qa->get_qt_field_name(constants::QT_VAR_RESPONSE_MARKER), + ]); + + $output .= $renderresult->html; + + return $output; } /** diff --git a/tests/attempt_ui/question_ui_metadata_extractor_test.php b/tests/attempt_ui/question_ui_metadata_extractor_test.php index 23e2e8f..885d8db 100644 --- a/tests/attempt_ui/question_ui_metadata_extractor_test.php +++ b/tests/attempt_ui/question_ui_metadata_extractor_test.php @@ -16,8 +16,6 @@ namespace qtype_questionpy\attempt_ui; -use qtype_questionpy\question_metadata; - /** * Unit tests for {@see question_ui_metadata_extractor}. * @@ -31,7 +29,7 @@ final class question_ui_metadata_extractor_test extends \advanced_testcase { * Tests that metadata is correctly extracted from the UI's input elements. * * @covers \qtype_questionpy\attempt_ui\question_ui_metadata_extractor - * @covers \qtype_questionpy\question_metadata + * @covers \qtype_questionpy\attempt_ui\question_metadata */ public function test_should_extract_correct_metadata(): void { $input = file_get_contents(__DIR__ . '/question_uis/metadata.xhtml');