Skip to content

Commit

Permalink
MDL-28035 problems with upgrade/restore of ataptive quizzes from 2.0
Browse files Browse the repository at this point in the history 
There were two main problems:

1. The unit tests for upgrading adaptive quiz attempts had slighly the
wrong $expectedqa, and so matching that the upgrade was doing the wrong
thing in certain situations. The main issue was that it was setting
-_try = 1 on the first step, which broke the penalty calculation when
the quiz was regraded. There were also some other subtleties with
incrementing -_try that were not right before.

2. It was possible in 2.0 and earlier for two question_states to get the
same seq_number, and restoring 2.0 backups was rashly assuming that that
was unique.
  • Loading branch information
@timhunt
timhunt committed Jun 27, 2011
1 parent 7fde489 commit 0366ef2
Show file tree
Hide file tree
Showing 9 changed files with 434 additions and 45 deletions.
7 changes: 6 additions & 1 deletion backup/moodle2/restore_stepslib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2846,10 +2846,15 @@ protected function find_question_session_and_states($data, $questionid) {
$qstates = array();
foreach ($data->states['state'] as $state) {
if ($state['question'] == $questionid) {
$qstates[$state['seq_number']] = (object) $state;
// It would be natural to use $state['seq_number'] as the array-key
// here, but it seems that buggy behaviour in 2.0 and early can
// mean that that is not unique, so we use id, which is guaranteed
// to be unique.
$qstates[$state['id']] = (object) $state;
}
}
ksort($qstates);
$qstates = array_values($qstates);

return array($qsession, $qstates);
}
Expand Down
2 changes: 1 addition & 1 deletion local/qeupgradehelper/locallib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ function local_qeupgradehelper_generate_unit_test($questionsessionid, $namesuffi
if (!local_qeupgradehelper_is_upgraded()) {
if (!$quiz->optionflags) {
$quiz->preferredbehaviour = 'deferredfeedback';
} else if (!$quiz->penaltyscheme) {
} else if ($quiz->penaltyscheme) {
$quiz->preferredbehaviour = 'adaptive';
} else {
$quiz->preferredbehaviour = 'adaptivenopenalty';
Expand Down
10 changes: 8 additions & 2 deletions question/engine/upgrade/behaviourconverters.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@ protected function process8($step, $state) {

class qbehaviour_adaptive_converter extends question_behaviour_attempt_updater {
protected $try;
protected $laststepwasatry = false;
protected $finished = false;
protected $bestrawgrade = 0;

Expand All @@ -455,7 +456,7 @@ protected function finish_up() {

protected function process0($step, $state) {
$this->try = 1;
$step->data['-_try'] = $this->try;
$this->laststepwasatry = false;
parent::process0($step, $state);
}

Expand All @@ -471,6 +472,7 @@ protected function process2($step, $state) {
$step->fraction = $state->grade / $this->question->maxmark;
}

$this->laststepwasatry = false;
parent::process2($step, $state);
}

Expand All @@ -488,8 +490,9 @@ protected function process3($step, $state) {

$this->bestrawgrade = max($state->raw_grade, $this->bestrawgrade);

$this->try += 1;
$step->data['-_try'] = $this->try;
$this->try += 1;
$this->laststepwasatry = true;
if ($this->question->maxmark > 0) {
$step->data['-_rawfraction'] = $state->raw_grade / $this->question->maxmark;
} else {
Expand Down Expand Up @@ -524,6 +527,9 @@ protected function process6($step, $state) {
}

$step->data['-finish'] = 1;
if ($this->laststepwasatry) {
$this->try -= 1;
}
$step->data['-_try'] = $this->try;
if ($this->question->maxmark > 0) {
$step->data['-_rawfraction'] = $state->raw_grade / $this->question->maxmark;
Expand Down
14 changes: 7 additions & 7 deletions question/type/calculated/db/simpletest/testupgradelibnewqe.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ public function test_calculated_adaptive_qsession97() {
'fraction' => null,
'timecreated' => 1305830650,
'userid' => 4,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '7.5', '_var_b' => '4.9'),
),
1 => (object) array(
Expand Down Expand Up @@ -442,7 +442,7 @@ public function test_calculated_adaptive_qsession100() {
'fraction' => null,
'timecreated' => 1305830661,
'userid' => 4,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '5.1', '_var_b' => '4.5'),
),
1 => (object) array(
Expand All @@ -451,7 +451,7 @@ public function test_calculated_adaptive_qsession100() {
'fraction' => 0.5,
'timecreated' => 1305830714,
'userid' => 4,
'data' => array('answer' => 9.6, '-_try' => 2,
'data' => array('answer' => 9.6, '-_try' => 1,
'-_rawfraction' => 0.5, '-submit' => 1),
),
2 => (object) array(
Expand All @@ -460,7 +460,7 @@ public function test_calculated_adaptive_qsession100() {
'fraction' => 0.9,
'timecreated' => 1305830722,
'userid' => 4,
'data' => array('answer' => '9.6 m', '-_try' => 3,
'data' => array('answer' => '9.6 m', '-_try' => 2,
'-_rawfraction' => 1, '-submit' => 1),
),
3 => (object) array(
Expand All @@ -469,7 +469,7 @@ public function test_calculated_adaptive_qsession100() {
'fraction' => 0.9,
'timecreated' => 1305830722,
'userid' => 4,
'data' => array('answer' => '9.6 m', '-_try' => 3,
'data' => array('answer' => '9.6 m', '-_try' => 2,
'-_rawfraction' => 1, '-finish' => 1),
),
),
Expand Down Expand Up @@ -672,7 +672,7 @@ public function test_calculated_adaptive_qsession103() {
'fraction' => null,
'timecreated' => 1305830744,
'userid' => 3,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '9.9', '_var_b' => '2.5'),
),
1 => (object) array(
Expand All @@ -681,7 +681,7 @@ public function test_calculated_adaptive_qsession103() {
'fraction' => 0,
'timecreated' => 1305830775,
'userid' => 3,
'data' => array('answer' => '123 cm', '-_try' => 2,
'data' => array('answer' => '123 cm', '-_try' => 1,
'-_rawfraction' => 0, '-submit' => 1),
),
2 => (object) array(
Expand Down
23 changes: 10 additions & 13 deletions question/type/calculatedmulti/db/simpletest/testupgradelibnewqe.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,7 @@ public function test_calculatedmulti_adaptive_qsession96() {
'fraction' => null,
'timecreated' => 1305830650,
'userid' => 4,
'data' => array('-_try' => 1,
'_order' => '24,26,27,25', '_var_a' => '4.3', '_var_b' => '5.4'),
'data' => array('_order' => '24,26,27,25', '_var_a' => '4.3', '_var_b' => '5.4'),
),
1 => (object) array(
'sequencenumber' => 1,
Expand Down Expand Up @@ -476,24 +475,23 @@ public function test_calculatedmulti_adaptive_qsession99() {
'fraction' => null,
'timecreated' => 1305830661,
'userid' => 4,
'data' => array('-_try' => 1,
'_order' => '25,24,27,26', '_var_a' => '3.7', '_var_b' => '6.0'),
'data' => array('_order' => '25,24,27,26', '_var_a' => '3.7', '_var_b' => '6.0'),
),
1 => (object) array(
'sequencenumber' => 1,
'state' => 'complete',
'fraction' => 1,
'timecreated' => 1305830699,
'userid' => 4,
'data' => array('answer' => '0', '-submit' => 1, '-_try' => 2, '-_rawfraction' => 1),
'data' => array('answer' => '0', '-submit' => 1, '-_try' => 1, '-_rawfraction' => 1),
),
2 => (object) array(
'sequencenumber' => 2,
'state' => 'gradedright',
'fraction' => 1,
'timecreated' => 1305830699,
'userid' => 4,
'data' => array('answer' => '0', '-finish' => 1, '-_try' => 2, '-_rawfraction' => 1),
'data' => array('answer' => '0', '-finish' => 1, '-_try' => 1, '-_rawfraction' => 1),
),
),
);
Expand Down Expand Up @@ -754,48 +752,47 @@ public function test_calculatedmulti_adaptive_qsession102() {
'fraction' => null,
'timecreated' => 1305830744,
'userid' => 3,
'data' => array('-_try' => 1,
'_order' => '26,24,25,27', '_var_a' => '4.4', '_var_b' => '8.2'),
'data' => array('_order' => '26,24,25,27', '_var_a' => '4.4', '_var_b' => '8.2'),
),
1 => (object) array(
'sequencenumber' => 1,
'state' => 'todo',
'fraction' => 0,
'timecreated' => 1305830759,
'userid' => 3,
'data' => array('answer' => '3', '-submit' => 1, '-_try' => 2, '-_rawfraction' => 0),
'data' => array('answer' => '3', '-submit' => 1, '-_try' => 1, '-_rawfraction' => 0),
),
2 => (object) array(
'sequencenumber' => 2,
'state' => 'todo',
'fraction' => 0,
'timecreated' => 1305830761,
'userid' => 3,
'data' => array('answer' => '1', '-submit' => 1, '-_try' => 3, '-_rawfraction' => 0),
'data' => array('answer' => '1', '-submit' => 1, '-_try' => 2, '-_rawfraction' => 0),
),
3 => (object) array(
'sequencenumber' => 3,
'state' => 'todo',
'fraction' => 0,
'timecreated' => 1305830764,
'userid' => 3,
'data' => array('answer' => '0', '-submit' => 1, '-_try' => 4, '-_rawfraction' => 0),
'data' => array('answer' => '0', '-submit' => 1, '-_try' => 3, '-_rawfraction' => 0),
),
4 => (object) array(
'sequencenumber' => 4,
'state' => 'todo',
'fraction' => 0.7,
'timecreated' => 1305830766,
'userid' => 3,
'data' => array('answer' => '2', '-submit' => 1, '-_try' => 5, '-_rawfraction' => 1),
'data' => array('answer' => '2', '-submit' => 1, '-_try' => 4, '-_rawfraction' => 1),
),
5 => (object) array(
'sequencenumber' => 5,
'state' => 'todo',
'fraction' => 0.7,
'timecreated' => 1305830768,
'userid' => 3,
'data' => array('answer' => '1', '-submit' => 1, '-_try' => 6, '-_rawfraction' => 0),
'data' => array('answer' => '1', '-submit' => 1, '-_try' => 5, '-_rawfraction' => 0),
),
),
);
Expand Down
12 changes: 6 additions & 6 deletions question/type/calculatedsimple/db/simpletest/testupgradelibnewqe.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ public function test_calculatedsimple_adaptive_qsession95() {
'fraction' => null,
'timecreated' => 1305830650,
'userid' => 4,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '3', '_var_b' => '6'),
),
1 => (object) array(
Expand Down Expand Up @@ -416,7 +416,7 @@ public function test_calculatedsimple_adaptive_qsession98() {
'fraction' => null,
'timecreated' => 1305830661,
'userid' => 4,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '6.4', '_var_b' => '9'),
),
1 => (object) array(
Expand All @@ -425,23 +425,23 @@ public function test_calculatedsimple_adaptive_qsession98() {
'fraction' => 0,
'timecreated' => 1305830668,
'userid' => 4,
'data' => array('answer' => '9.00', '-submit' => 1, '-_try' => 2, '-_rawfraction' => 0),
'data' => array('answer' => '9.00', '-submit' => 1, '-_try' => 1, '-_rawfraction' => 0),
),
2 => (object) array(
'sequencenumber' => 2,
'state' => 'todo',
'fraction' => 0.9,
'timecreated' => 1305830679,
'userid' => 4,
'data' => array('answer' => '15.40', '-submit' => 1, '-_try' => 3, '-_rawfraction' => 1),
'data' => array('answer' => '15.40', '-submit' => 1, '-_try' => 2, '-_rawfraction' => 1),
),
3 => (object) array(
'sequencenumber' => 3,
'state' => 'gradedright',
'fraction' => 0.9,
'timecreated' => 1305830679,
'userid' => 4,
'data' => array('answer' => '15.40', '-finish' => 1, '-_try' => 3, '-_rawfraction' => 1),
'data' => array('answer' => '15.40', '-finish' => 1, '-_try' => 2, '-_rawfraction' => 1),
),
),
);
Expand Down Expand Up @@ -618,7 +618,7 @@ public function test_calculatedsimple_adaptive_qsession101() {
'fraction' => null,
'timecreated' => 1305830744,
'userid' => 3,
'data' => array('-_try' => 1, '_separators' => '.$,',
'data' => array('_separators' => '.$,',
'_var_a' => '6.1', '_var_b' => '7'),
),
1 => (object) array(
Expand Down
Loading

0 comments on commit 0366ef2

Please sign in to comment.