Skip to content

Commit 66de66f

Browse files
timhunttimhunt
committed
MDL-46148 qtype_calculated: fix validation when importing.
In order to do this in a sane way, I cleaned up a lot of old mess, inclduing: 1. Previously, qtype_calcuated used ->answeres when importing, and ->answer when saving the form. This was crazy, so I fixed it, and stripped out the code that made the alternative variable name work. 2. Similarly, it could handle ->answer being either an array, such as you would get form the HTML editor, or a simple string, which is what you get form the form. I simplified that too. 3. Finally, I made import use a transaction around saving each question, so we don't get half questions in the database when an error occurs.
1 parent 770d3ce commit 66de66f

File tree

6 files changed

+51
-46
lines changed

6 files changed

+51
-46
lines changed

Diff for: ‎question/format.php

+6
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,7 @@ public function importprocess($category) {
357357
$count = 0;
358358

359359
foreach ($questions as $question) { // Process and store each question
360+
$transaction = $DB->start_delegated_transaction();
360361

361362
// reset the php timeout
362363
core_php_time_limit::raise();
@@ -429,9 +430,14 @@ public function importprocess($category) {
429430

430431
if (!empty($result->error)) {
431432
echo $OUTPUT->notification($result->error);
433+
// Can't use $transaction->rollback(); since it requires an exception,
434+
// and I don't want to rewrite this code to change the error handling now.
435+
$DB->force_transaction_rollback();
432436
return false;
433437
}
434438

439+
$transaction->allow_commit();
440+
435441
if (!empty($result->notice)) {
436442
echo $OUTPUT->notification($result->notice);
437443
return true;

Diff for: ‎question/format/webct/format.php

+2-5
Original file line numberDiff line numberDiff line change
@@ -606,12 +606,9 @@ public function readquestions ($lines) {
606606
// Calculated Question.
607607
$question = $this->defaultquestion();
608608
$question->qtype = 'calculated';
609-
$question->answers = array(); // No problem as they go as :FORMULA: from webct.
609+
$question->answer = array(); // No problem as they go as :FORMULA: from webct.
610610
$question->units = array();
611611
$question->dataset = array();
612-
613-
// To make us pass the end-of-question sanity checks.
614-
$question->answer = array('dummy');
615612
$question->fraction = array('1.0');
616613
$question->feedback = array();
617614

@@ -739,7 +736,7 @@ public function readquestions ($lines) {
739736
if (preg_match('~^:FORMULA:(.*)~i', $line, $webctoptions)) {
740737
// Answer for a calculated question.
741738
++$currentchoice;
742-
$question->answers[$currentchoice] =
739+
$question->answer[$currentchoice] =
743740
qformat_webct_convert_formula($webctoptions[1]);
744741

745742
// Default settings.

Diff for: ‎question/format/xml/format.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -779,7 +779,7 @@ public function import_calculated($question) {
779779

780780
// Get answers array.
781781
$answers = $question['#']['answer'];
782-
$qo->answers = array();
782+
$qo->answer = array();
783783
$qo->feedback = array();
784784
$qo->fraction = array();
785785
$qo->tolerance = array();
@@ -793,7 +793,7 @@ public function import_calculated($question) {
793793
if (empty($ans->answer['text'])) {
794794
$ans->answer['text'] = '*';
795795
}
796-
$qo->answers[] = $ans->answer;
796+
$qo->answer[] = $ans->answer['text'];
797797
$qo->feedback[] = $ans->feedback;
798798
$qo->tolerance[] = $answer['#']['tolerance'][0]['#'];
799799
// Fraction as a tag is deprecated.

Diff for: ‎question/type/calculated/questiontype.php

+27-27
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,13 @@ public function get_datasets_for_export($question) {
131131

132132
public function save_question_options($question) {
133133
global $CFG, $DB;
134+
135+
// Make it impossible to save bad formulas anywhere.
136+
$this->validate_question_data($question);
137+
134138
// The code is used for calculated, calculatedsimple and calculatedmulti qtypes.
135139
$context = $question->context;
136-
if (isset($question->answer) && !isset($question->answers)) {
137-
$question->answers = $question->answer;
138-
}
140+
139141
// Calculated options.
140142
$update = true;
141143
$options = $DB->get_record('question_calculated_options',
@@ -185,14 +187,7 @@ public function save_question_options($question) {
185187
$units = $result->units;
186188
}
187189

188-
// Insert all the new answers.
189-
if (isset($question->answer) && !isset($question->answers)) {
190-
$question->answers = $question->answer;
191-
}
192-
foreach ($question->answers as $key => $answerdata) {
193-
if (is_array($answerdata)) {
194-
$answerdata = $answerdata['text'];
195-
}
190+
foreach ($question->answer as $key => $answerdata) {
196191
if (trim($answerdata) == '') {
197192
continue;
198193
}
@@ -471,6 +466,25 @@ protected function validate_answer($answer) {
471466
}
472467
}
473468

469+
/**
470+
* Validate data before save.
471+
* @param stdClass $question data from the form / import file.
472+
*/
473+
protected function validate_question_data($question) {
474+
$this->validate_text($question->questiontext); // Yes, really no ['text'].
475+
476+
if (isset($question->generalfeedback['text'])) {
477+
$this->validate_text($question->generalfeedback['text']);
478+
} else if (isset($question->generalfeedback)) {
479+
$this->validate_text($question->generalfeedback); // Because question import is weird.
480+
}
481+
482+
foreach ($question->answer as $key => $answer) {
483+
$this->validate_answer($answer);
484+
$this->validate_text($question->feedback[$key]['text']);
485+
}
486+
}
487+
474488
/**
475489
* This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
476490
* so that they can be saved
@@ -483,13 +497,13 @@ protected function validate_answer($answer) {
483497
* @param object $form
484498
* @param int $questionfromid default = '0'
485499
*/
486-
public function preparedatasets($form , $questionfromid = '0') {
500+
public function preparedatasets($form, $questionfromid = '0') {
487501

488502
// The dataset names present in the edit_question_form and edit_calculated_form
489503
// are retrieved.
490504
$possibledatasets = $this->find_dataset_names($form->questiontext);
491505
$mandatorydatasets = array();
492-
foreach ($form->answers as $key => $answer) {
506+
foreach ($form->answer as $key => $answer) {
493507
$mandatorydatasets += $this->find_dataset_names($answer);
494508
}
495509
// If there are identical datasetdefs already saved in the original question
@@ -579,12 +593,6 @@ public function addnamecategory(&$question) {
579593
public function save_question($question, $form) {
580594
global $DB;
581595

582-
if (isset($form->correctfeedback)) {
583-
$this->validate_text($form->correctfeedback['text']);
584-
$this->validate_text($form->partiallycorrectfeedback['text']);
585-
$this->validate_text($form->incorrectfeedback['text']);
586-
}
587-
588596
if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
589597
$question = parent::save_question($question, $form);
590598
return $question;
@@ -605,14 +613,6 @@ public function save_question($question, $form) {
605613
case '' :
606614
case 'question': // Coming from the first page, creating the second.
607615
if (empty($form->id)) { // or a new question $form->id is empty.
608-
// Make it impossible to save bad formulas anywhere.
609-
$this->validate_text($form->questiontext['text']);
610-
$this->validate_text($form->generalfeedback['text']);
611-
foreach ($form->answer as $key => $answer) {
612-
$this->validate_answer($answer);
613-
$this->validate_text($form->feedback[$key]['text']);
614-
}
615-
616616
$question = parent::save_question($question, $form);
617617
// Prepare the datasets using default $questionfromid.
618618
$this->preparedatasets($form);

Diff for: ‎question/type/calculatedmulti/questiontype.php

+11-4
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ public function save_question_options($question) {
4242
global $CFG, $DB;
4343
$context = $question->context;
4444

45+
// Make it impossible to save bad formulas anywhere.
46+
$this->validate_question_data($question);
47+
4548
// Calculated options.
4649
$update = true;
4750
$options = $DB->get_record('question_calculated_options',
@@ -72,10 +75,7 @@ public function save_question_options($question) {
7275
}
7376

7477
// Insert all the new answers.
75-
if (isset($question->answer) && !isset($question->answers)) {
76-
$question->answers = $question->answer;
77-
}
78-
foreach ($question->answers as $key => $answerdata) {
78+
foreach ($question->answer as $key => $answerdata) {
7979
if (is_array($answerdata)) {
8080
$answerdata = $answerdata['text'];
8181
}
@@ -163,6 +163,13 @@ protected function validate_answer($answer) {
163163
}
164164
}
165165

166+
protected function validate_question_data($question) {
167+
parent::validate_question_data($question);
168+
$this->validate_text($question->correctfeedback['text']);
169+
$this->validate_text($question->partiallycorrectfeedback['text']);
170+
$this->validate_text($question->incorrectfeedback['text']);
171+
}
172+
166173
protected function make_question_instance($questiondata) {
167174
question_bank::load_question_definition_classes($this->name());
168175
if ($questiondata->options->single) {

Diff for: ‎question/type/calculatedsimple/questiontype.php

+3-8
Original file line numberDiff line numberDiff line change
@@ -43,11 +43,9 @@ class qtype_calculatedsimple extends qtype_calculated {
4343
public function save_question_options($question) {
4444
global $CFG, $DB;
4545
$context = $question->context;
46-
// Get old answers.
4746

48-
if (isset($question->answer) && !isset($question->answers)) {
49-
$question->answers = $question->answer;
50-
}
47+
// Make it impossible to save bad formulas anywhere.
48+
$this->validate_question_data($question);
5149

5250
// Get old versions of the objects.
5351
if (!$oldanswers = $DB->get_records('question_answers',
@@ -69,10 +67,7 @@ public function save_question_options($question) {
6967
$units = &$result->units;
7068
}
7169
// Insert all the new answers.
72-
if (isset($question->answer) && !isset($question->answers)) {
73-
$question->answers = $question->answer;
74-
}
75-
foreach ($question->answers as $key => $answerdata) {
70+
foreach ($question->answer as $key => $answerdata) {
7671
if (is_array($answerdata)) {
7772
$answerdata = $answerdata['text'];
7873
}

0 commit comments

Comments
 (0)