From 1700bd4dea94e151d07eb8dc47f9ad73dcd11413 Mon Sep 17 00:00:00 2001 From: M Kassaei Date: Wed, 3 Dec 2014 11:24:46 +0000 Subject: [PATCH 1/4] MDL-40992 quiz: let students to redo questions within an attempt --- lang/en/question.php | 2 + mod/quiz/attempt.php | 6 +++ mod/quiz/attemptlib.php | 38 +++++++++++++ mod/quiz/db/install.xml | 1 + mod/quiz/db/upgrade.php | 14 +++++ mod/quiz/lang/en/quiz.php | 3 ++ mod/quiz/mod_form.php | 13 +++++ mod/quiz/processattempt.php | 6 +++ mod/quiz/renderer.php | 1 + mod/quiz/settings.php | 5 ++ mod/quiz/styles.css | 4 ++ .../tests/behat/reattemptquestions.feature | 53 +++++++++++++++++++ mod/quiz/version.php | 2 +- question/behaviour/behaviourtypebase.php | 8 +++ .../behaviour/immediatecbm/behaviourtype.php | 7 +++ .../immediatefeedback/behaviourtype.php | 7 +++ .../behaviour/interactive/behaviourtype.php | 9 ++++ question/engine/datalib.php | 46 ++++++++++++++-- question/engine/questionusage.php | 24 +++++++++ 19 files changed, 245 insertions(+), 4 deletions(-) create mode 100644 mod/quiz/tests/behat/reattemptquestions.feature diff --git a/lang/en/question.php b/lang/en/question.php index 0e7927c1444..1edd07787aa 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -397,6 +397,7 @@ $string['questiontext'] = 'Question text'; $string['requiresgrading'] = 'Requires grading'; $string['responsehistory'] = 'Response history'; $string['restart'] = 'Start again'; +$string['restartquestion'] = 'Restart question'; $string['restartwiththeseoptions'] = 'Start again with these options'; $string['updatedisplayoptions'] = 'Update display options'; $string['rightanswer'] = 'Right answer'; @@ -436,6 +437,7 @@ $string['unknownquestion'] = 'Unknown question: {$a}.'; $string['unknownquestioncatregory'] = 'Unknown question category: {$a}.'; $string['unknownquestiontype'] = 'Unknown question type: {$a}.'; $string['unusedcategorydeleted'] = 'This category has been deleted because, after deleting the course, its questions weren\'t used any more.'; +$string['updatedisplayoptions'] = 'Update display options'; $string['whethercorrect'] = 'Whether correct'; $string['whethercorrect_help'] = 'This covers both the textual description \'Correct\', \'Partially correct\' or \'Incorrect\', and any coloured highlighting that conveys the same information.'; $string['whichtries'] = 'Which tries'; diff --git a/mod/quiz/attempt.php b/mod/quiz/attempt.php index 181c9bc80c4..5ac9659b239 100644 --- a/mod/quiz/attempt.php +++ b/mod/quiz/attempt.php @@ -125,6 +125,12 @@ if ($attemptobj->get_currentpage() != $page) { $DB->set_field('quiz_attempts', 'currentpage', $page, array('id' => $attemptid)); } +// Process replace question action, when user press on 'Replace question' link. +$replacequestioninslot = optional_param('replacequestioninslot', 0, PARAM_INT); +if ($replacequestioninslot) { + $attemptobj->process_replace_question_actions($replacequestioninslot, time()); +} + // Initialise the JavaScript. $headtags = $attemptobj->get_html_head_contributions($page); $PAGE->requires->js_init_call('M.mod_quiz.init_attempt_form', null, false, quiz_get_js_module()); diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 91ac3d2ac47..75a603307dc 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -1527,6 +1527,44 @@ class quiz_attempt { $transaction->allow_commit(); } + /** + * Process replace question action + * @param int $slot + * @param int $timestamp + */ + public function process_replace_question_actions($slot, $timestamp) { + global $DB; + + $transaction = $DB->start_delegated_transaction(); + + $this->quba->replace_question($slot); + question_engine::save_questions_usage_by_activity($this->quba); + + $transaction->allow_commit(); + } + + /** + * Return a button which allows students reattempting the current question + * + * @param int $slot, the number of the current slot + */ + public function restart_question_button($slot) { + // If 'reattemptgradedquestions' field is not set, do not display the 'Restart question' button. + if (!$this->get_quiz()->reattemptgradedquestions) { + return; + } + $qa = $this->get_question_attempt($slot); + + // If question is not graded, do not display the 'Restart question' button. + if (!$qa->get_state()->is_graded()) { + return; + } + $buttonvalue = get_string('restartquestion', 'question'); + return html_writer::tag('div', + " + "); + } + /** * Process all the autosaved data that was part of the current request. * diff --git a/mod/quiz/db/install.xml b/mod/quiz/db/install.xml index cf2207fed9e..0cfa40de4a2 100644 --- a/mod/quiz/db/install.xml +++ b/mod/quiz/db/install.xml @@ -17,6 +17,7 @@ + diff --git a/mod/quiz/db/upgrade.php b/mod/quiz/db/upgrade.php index acbdbf01140..b40fe080133 100644 --- a/mod/quiz/db/upgrade.php +++ b/mod/quiz/db/upgrade.php @@ -821,5 +821,19 @@ function xmldb_quiz_upgrade($oldversion) { upgrade_mod_savepoint(true, 2015030500, 'quiz'); } + if ($oldversion < 2015030900) { + // Define field reattemptgradedquestions to be added to quiz. + $table = new xmldb_table('quiz'); + $field = new xmldb_field('reattemptgradedquestions', XMLDB_TYPE_INTEGER, '4', null, null, null, 0, 'completionpass'); + + // Conditionally launch add field completionpass. + if (!$dbman->field_exists($table, $field)) { + $dbman->add_field($table, $field); + } + + // Quiz savepoint reached. + upgrade_mod_savepoint(true, 2015030900, 'quiz'); + } + return true; } diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index 17699845718..c45c1f1f3ca 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -180,6 +180,7 @@ $string['configpenaltyscheme'] = 'Penalty subtracted for each wrong response in $string['configpopup'] = 'Force the attempt to open in a popup window, and use JavaScript tricks to try to restrict copy and paste, etc. during quiz attempts.'; $string['configrequirepassword'] = 'Students must enter this password before they can attempt the quiz.'; $string['configrequiresubnet'] = 'Students can only attempt the quiz from these computers.'; +$string['configrestartgradedquestions'] = 'If enabled, it allows students to restart graded questions in \'Immediate feedback\', \'Immediate feedback with CBM\' and \'Interactive with multiple tries behaviours\''; $string['configreviewoptions'] = 'These options control what information users can see when they review a quiz attempt or look at the quiz reports.'; $string['configshowblocks'] = 'Show blocks during quiz attempts.'; $string['configshowuserpicture'] = 'Show the user\'s picture on screen during attempts.'; @@ -754,6 +755,8 @@ $string['reviewduring'] = 'During the attempt'; $string['reviewimmediately'] = 'Immediately after the attempt'; $string['marks'] = 'Marks'; $string['marks_help'] = 'The numerical marks for each question, and the overall attempt score.'; +$string['restartgradedquestions'] = 'Restart graded questions'; +$string['restartgradedquestions_help'] = 'If enabled, it allows students to restart graded questions in \'Immediate feedback\', \'Immediate feedback with CBM\' and \'Interactive with multiple tries behaviours\''; $string['reviewnever'] = 'Never allow review'; $string['reviewofattempt'] = 'Review of attempt {$a}'; $string['reviewofpreview'] = 'Review of preview'; diff --git a/mod/quiz/mod_form.php b/mod/quiz/mod_form.php index 694ab98ff93..e1fcb1edb9f 100644 --- a/mod/quiz/mod_form.php +++ b/mod/quiz/mod_form.php @@ -205,6 +205,19 @@ class mod_quiz_mod_form extends moodleform_mod { $mform->addHelpButton('preferredbehaviour', 'howquestionsbehave', 'question'); $mform->setDefault('preferredbehaviour', $quizconfig->preferredbehaviour); + // TODO: Store the 'reattemptgradedquestions' field when new DB structure in place. + $mform->addElement('selectyesno', 'reattemptgradedquestions', get_string('restartgradedquestions', 'quiz')); + $mform->addHelpButton('reattemptgradedquestions', 'restartgradedquestions', 'quiz'); + $mform->setAdvanced('reattemptgradedquestions', $quizconfig->reattemptgradedquestions_adv); + $mform->setDefault('reattemptgradedquestions', $quizconfig->reattemptgradedquestions); + foreach ($behaviours as $behaviour => $notused) { + $qbt = question_engine::get_behaviour_type($behaviour); + if (!$qbt->user_can_reattempt_graded_question()) { + $mform->disabledIf('reattemptgradedquestions', 'preferredbehaviour', 'eq', $behaviour); + } + + } + // Each attempt builds on last. $mform->addElement('selectyesno', 'attemptonlast', get_string('eachattemptbuildsonthelast', 'quiz')); diff --git a/mod/quiz/processattempt.php b/mod/quiz/processattempt.php index edf6d8c617c..ff6cedcd581 100644 --- a/mod/quiz/processattempt.php +++ b/mod/quiz/processattempt.php @@ -44,6 +44,12 @@ $finishattempt = optional_param('finishattempt', false, PARAM_BOOL); $timeup = optional_param('timeup', 0, PARAM_BOOL); // True if form was submitted by timer. $scrollpos = optional_param('scrollpos', '', PARAM_RAW); +// Process replace question action, when user press on 'Replace question' link. +if (isset($_POST['restartquestioninslot'])) { + redirect(new moodle_url('/mod/quiz/attempt.php', + array('attempt' => $attemptid, 'page' => $thispage, 'replacequestioninslot' => $_POST['restartquestionincurrentslot']))); +} + $transaction = $DB->start_delegated_transaction(); $attemptobj = quiz_attempt::create($attemptid); diff --git a/mod/quiz/renderer.php b/mod/quiz/renderer.php index 9b746718655..5174dbf79ca 100644 --- a/mod/quiz/renderer.php +++ b/mod/quiz/renderer.php @@ -461,6 +461,7 @@ class mod_quiz_renderer extends plugin_renderer_base { foreach ($slots as $slot) { $output .= $attemptobj->render_question($slot, false, $attemptobj->attempt_url($slot, $page)); + $output .= $attemptobj->restart_question_button($slot); } $output .= html_writer::start_tag('div', array('class' => 'submitbtns')); diff --git a/mod/quiz/settings.php b/mod/quiz/settings.php index fc4a28fd6b5..4ef3dc4e711 100644 --- a/mod/quiz/settings.php +++ b/mod/quiz/settings.php @@ -128,6 +128,11 @@ if ($ADMIN->fulltree) { get_string('howquestionsbehave', 'question'), get_string('howquestionsbehave_desc', 'quiz'), 'deferredfeedback')); + // Restart completed questions (reattemptgradedquestions). + $quizsettings->add(new admin_setting_configcheckbox_with_advanced('quiz/reattemptgradedquestions', + get_string('restartgradedquestions', 'quiz'), get_string('configrestartgradedquestions', 'quiz'), + array('value' => 0, 'adv' => true))); + // Each attempt builds on last. $quizsettings->add(new admin_setting_configcheckbox_with_advanced('quiz/attemptonlast', get_string('eachattemptbuildsonthelast', 'quiz'), diff --git a/mod/quiz/styles.css b/mod/quiz/styles.css index e4fdb227758..817caea6366 100644 --- a/mod/quiz/styles.css +++ b/mod/quiz/styles.css @@ -24,6 +24,10 @@ text-align: right; } +#page-mod-quiz-attempt .resatrt-question-btn { + font-size: 0.75em; +} + #page-mod-quiz-attempt .mod_quiz-blocked_question_warning .que .formulation, #page-mod-quiz-review .mod_quiz-blocked_question_warning .que .formulation { background: #eee; diff --git a/mod/quiz/tests/behat/reattemptquestions.feature b/mod/quiz/tests/behat/reattemptquestions.feature new file mode 100644 index 00000000000..0943cc6b3fd --- /dev/null +++ b/mod/quiz/tests/behat/reattemptquestions.feature @@ -0,0 +1,53 @@ +@mod @mod_quiz +Feature: Add a quiz + In order to allow students re-attempting graded question + As a teacher + I need to create a quiz, set 'Restart question' field to 'Yes', add questions to the quiz which can be graded automatically. + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | T1 | Teacher1 | teacher1@moodle.com | + | student1 | S1 | Student1 | student1@moodle.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | teacher1 | C1 | editingteacher | + | student1 | C1 | student | + When I log in as "teacher1" + And I follow "Course 1" + And I turn editing mode on + + And I add a "Quiz" to section "1" and I fill the form with: + | Name | Quiz 1 | + | Description | Quiz 1 description | + | How questions behave | Immediate feedback | + | Restart graded questions | Yes | + + And I add a "True/False" question to the "Quiz 1" quiz with: + | Question name | TF001 | + | Question text | Answer question TF001 | + | General feedback | Thank you, this is the general feedback | + | Correct answer | False | + | Feedback for the response 'True'. | So you think it is true | + | Feedback for the response 'False'. | So you think it is false | + And I log out + + @javascript + Scenario: Log in as a student, attempt the quiz and checking whether you can re-attempt a graded question in the appropriate behaviour settings + And I log in as "student1" + And I follow "Course 1" + And I follow "Quiz 1" + And I press "Attempt quiz now" + Then I should see "TF001" + And I should see "Answer question TF001" + And I set the field "True" to "1" + And I press "Check" + And I should see "Incorrect" + Then I press "Restart question" + And I should see "Not complete" + And I set the field "False" to "1" + And I press "Check" + And I should see "Correct" diff --git a/mod/quiz/version.php b/mod/quiz/version.php index d9ddae57917..e0cd1fd5f6f 100644 --- a/mod/quiz/version.php +++ b/mod/quiz/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2015030500; +$plugin->version = 2015030900; $plugin->requires = 2014110400; $plugin->component = 'mod_quiz'; $plugin->cron = 60; diff --git a/question/behaviour/behaviourtypebase.php b/question/behaviour/behaviourtypebase.php index 8cc0c7dc927..37a8e1af474 100644 --- a/question/behaviour/behaviourtypebase.php +++ b/question/behaviour/behaviourtypebase.php @@ -102,6 +102,14 @@ abstract class question_behaviour_type { public function allows_multiple_submitted_responses() { return false; } + + /** + * Allow user to reattmpt graded question during a quiz attempt + * @return boolean + */ + public function user_can_reattempt_graded_question() { + return false; + } } diff --git a/question/behaviour/immediatecbm/behaviourtype.php b/question/behaviour/immediatecbm/behaviourtype.php index 01a2ec336ff..fa586a09933 100644 --- a/question/behaviour/immediatecbm/behaviourtype.php +++ b/question/behaviour/immediatecbm/behaviourtype.php @@ -39,4 +39,11 @@ class qbehaviour_immediatecbm_type extends qbehaviour_deferredcbm_type { public function get_unused_display_options() { return array(); } + + /** + * Allow user to re-attempt graded questions during a quiz attempt + */ + public function user_can_reattempt_graded_question() { + return true; + } } diff --git a/question/behaviour/immediatefeedback/behaviourtype.php b/question/behaviour/immediatefeedback/behaviourtype.php index c366fe33126..71b6d6f2cdd 100644 --- a/question/behaviour/immediatefeedback/behaviourtype.php +++ b/question/behaviour/immediatefeedback/behaviourtype.php @@ -36,4 +36,11 @@ class qbehaviour_immediatefeedback_type extends question_behaviour_type { public function is_archetypal() { return true; } + + /** + * Allow user to re-attempt graded questions during a quiz attempt + */ + public function user_can_reattempt_graded_question() { + return true; + } } diff --git a/question/behaviour/interactive/behaviourtype.php b/question/behaviour/interactive/behaviourtype.php index 3b443586103..641208dd356 100644 --- a/question/behaviour/interactive/behaviourtype.php +++ b/question/behaviour/interactive/behaviourtype.php @@ -40,4 +40,13 @@ class qbehaviour_interactive_type extends question_behaviour_type { public function allows_multiple_submitted_responses() { return true; } + + /** + * Allow user to re-attempt graded questions during a quiz attempt + */ + public function user_can_reattempt_graded_question() { + return true; + } + + } diff --git a/question/engine/datalib.php b/question/engine/datalib.php index 9574743e853..ab41023ed70 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -880,6 +880,16 @@ ORDER BY $this->db->update_record('question_attempts', $record); } + /** + * Delete a question_attempts row to reflect any changes in a question_attempt + * (but not any of its steps). + * @param question_attempt $qa the question attempt that has been deleted. + */ + public function delete_question_attempt(question_attempt $qa) { + $conditions = array('questionusageid' => $qa->get_usage_id(), 'slot' => $qa->get_slot()); + $this->db->delete_records('question_attempts', $conditions); + } + /** * Delete a question_usage_by_activity and all its associated * @@ -1250,6 +1260,12 @@ class question_engine_unit_of_work implements question_usage_observer { */ protected $attemptsmodified = array(); + /** + * @var array list of slot => {@link question_attempt}s that + * were already in the usage, and which have been deleted. + */ + protected $attemptsdeleted = array(); + /** * @var array list of slot => {@link question_attempt}s that * have been added to the usage. @@ -1293,8 +1309,28 @@ class question_engine_unit_of_work implements question_usage_observer { } } + /** + * Notify when attempt deleted + * + * @see question_usage_observer::notify_attempt_deleted() + */ + public function notify_attempt_deleted(question_attempt $qa) { + $slot = $qa->get_slot(); + if (!array_key_exists($slot, $this->attemptsadded)) { + $this->attemptsdeleted[$slot] = $qa; + } + } + + /** + * Notify when attempt added + * + * @see question_usage_observer::notify_attempt_added() + */ public function notify_attempt_added(question_attempt $qa) { - $this->attemptsadded[$qa->get_slot()] = $qa; + $slot = $qa->get_slot(); + if (!array_key_exists($slot, $this->attemptsadded)) { + $this->attemptsadded[$slot] = $qa; + } } public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) { @@ -1437,6 +1473,10 @@ class question_engine_unit_of_work implements question_usage_observer { $step, $questionattemptid, $seq, $this->quba->get_owning_context()); } + foreach ($this->attemptsdeleted as $qa) { + $dm->delete_question_attempt($qa); + } + foreach ($this->attemptsadded as $qa) { $stepdata[] = $dm->insert_question_attempt( $qa, $this->quba->get_owning_context()); @@ -1598,9 +1638,9 @@ class question_file_loader implements question_response_files { protected $name; /** - * @var string the value to stored in the question_attempt_step_data to + * @var string the value to stored in the question_attempt_step_data to * represent these files. - */ + */ protected $value; /** @var int the context id that the files belong to. */ diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index 536c8808c82..d5a213b390f 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -822,6 +822,22 @@ class question_usage_by_activity { $this->observer->notify_attempt_modified($newqa); } + /** + * Replace a question in this usage. + * @param int $slot the number used to identify this question within this usage.* + */ + public function replace_question($slot) { + global $OUTPUT; + $oldqa = $this->get_question_attempt($slot); + $newqa = new question_attempt($oldqa->get_question(), $oldqa->get_usage_id(), $this->observer); + $newqa->set_database_id($oldqa->get_database_id()); + $newqa->set_slot($oldqa->get_slot()); + $this->questionattempts[$slot] = $newqa; + $this->observer->notify_attempt_deleted($oldqa); + $this->observer->notify_attempt_added($newqa); + $this->start_question($slot); + } + /** * Regrade all the questions in this usage (without changing their max mark). * @param bool $finished whether each question should be forced to be finished @@ -979,6 +995,12 @@ interface question_usage_observer { */ public function notify_attempt_added(question_attempt $qa); + /** + * Called when the fields of a question attempt in this usage are deleted. + * @param question_attempt $qa + */ + public function notify_attempt_deleted(question_attempt $qa); + /** * Called when a new step is added to a question attempt in this usage. * @param question_attempt_step $step the new step. @@ -1017,6 +1039,8 @@ class question_usage_null_observer implements question_usage_observer { } public function notify_attempt_modified(question_attempt $qa) { } + public function notify_attempt_deleted(question_attempt $qa) { + } public function notify_attempt_added(question_attempt $qa) { } public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) { From 47be39ef413d3fd5abf270766471af3d4fa42b49 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 9 Mar 2015 18:51:02 +0000 Subject: [PATCH 2/4] MDL-40992 qbehaviour: new type method, can Qs can finish naturally --- .../adaptive/tests/behaviourtype_test.php | 4 ++++ question/behaviour/behaviourtypebase.php | 17 +++++++++-------- .../deferredcbm/tests/behaviourtype_test.php | 4 ++++ .../tests/behaviourtype_test.php | 4 ++++ .../behaviour/immediatecbm/behaviourtype.php | 5 +---- .../immediatecbm/tests/behaviourtype_test.php | 4 ++++ .../immediatefeedback/behaviourtype.php | 5 +---- .../tests/behaviourtype_test.php | 4 ++++ .../tests/behaviourtype_test.php | 4 ++++ .../behaviour/interactive/behaviourtype.php | 7 +------ .../interactive/tests/behaviourtype_test.php | 4 ++++ .../tests/behaviourtype_test.php | 4 ++++ .../manualgraded/tests/behaviourtype_test.php | 4 ++++ .../missing/tests/behaviourtype_test.php | 4 ++++ question/behaviour/upgrade.txt | 12 +++++++++--- question/engine/lib.php | 10 ++++++++++ question/engine/tests/questionengine_test.php | 5 +++++ 17 files changed, 76 insertions(+), 25 deletions(-) diff --git a/question/behaviour/adaptive/tests/behaviourtype_test.php b/question/behaviour/adaptive/tests/behaviourtype_test.php index 4b44ac77cee..9071ae5d804 100644 --- a/question/behaviour/adaptive/tests/behaviourtype_test.php +++ b/question/behaviour/adaptive/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_adaptive_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/behaviourtypebase.php b/question/behaviour/behaviourtypebase.php index 37a8e1af474..c5ceb9f6725 100644 --- a/question/behaviour/behaviourtypebase.php +++ b/question/behaviour/behaviourtypebase.php @@ -59,6 +59,15 @@ abstract class question_behaviour_type { return array(); } + /** + * With this behaviour, is it possible that a question might finish as the student + * interacts with it, without a call to the {@link question_attempt::finish()} method? + * @return bool whether with this behaviour, questions may finish naturally. + */ + public function can_questions_finish_during_the_attempt() { + return false; + } + /** * Adjust a random guess score for a question using this model. You have to * do this without knowing details of the specific question, or which usage @@ -102,14 +111,6 @@ abstract class question_behaviour_type { public function allows_multiple_submitted_responses() { return false; } - - /** - * Allow user to reattmpt graded question during a quiz attempt - * @return boolean - */ - public function user_can_reattempt_graded_question() { - return false; - } } diff --git a/question/behaviour/deferredcbm/tests/behaviourtype_test.php b/question/behaviour/deferredcbm/tests/behaviourtype_test.php index d0f44c08e85..addcf4b8d53 100644 --- a/question/behaviour/deferredcbm/tests/behaviourtype_test.php +++ b/question/behaviour/deferredcbm/tests/behaviourtype_test.php @@ -57,6 +57,10 @@ class qbehaviour_deferredcbm_type_test extends qbehaviour_walkthrough_test_base $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/deferredfeedback/tests/behaviourtype_test.php b/question/behaviour/deferredfeedback/tests/behaviourtype_test.php index 85225054b84..6b63e2223ae 100644 --- a/question/behaviour/deferredfeedback/tests/behaviourtype_test.php +++ b/question/behaviour/deferredfeedback/tests/behaviourtype_test.php @@ -51,6 +51,10 @@ class qbehaviour_deferredfeedback_type_test extends qbehaviour_walkthrough_test_ $this->assertTrue($this->behaviourtype->is_archetypal()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_get_unused_display_options() { $this->assertEquals(array('correctness', 'marks', 'specificfeedback', 'generalfeedback', 'rightanswer'), $this->behaviourtype->get_unused_display_options()); diff --git a/question/behaviour/immediatecbm/behaviourtype.php b/question/behaviour/immediatecbm/behaviourtype.php index fa586a09933..1e8a7efb5f3 100644 --- a/question/behaviour/immediatecbm/behaviourtype.php +++ b/question/behaviour/immediatecbm/behaviourtype.php @@ -40,10 +40,7 @@ class qbehaviour_immediatecbm_type extends qbehaviour_deferredcbm_type { return array(); } - /** - * Allow user to re-attempt graded questions during a quiz attempt - */ - public function user_can_reattempt_graded_question() { + public function can_questions_finish_during_the_attempt() { return true; } } diff --git a/question/behaviour/immediatecbm/tests/behaviourtype_test.php b/question/behaviour/immediatecbm/tests/behaviourtype_test.php index 009bf12ec98..f292b06ec1d 100644 --- a/question/behaviour/immediatecbm/tests/behaviourtype_test.php +++ b/question/behaviour/immediatecbm/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_immediatecbm_type_test extends qbehaviour_walkthrough_test_base $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertTrue($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/immediatefeedback/behaviourtype.php b/question/behaviour/immediatefeedback/behaviourtype.php index 71b6d6f2cdd..7e0653013ba 100644 --- a/question/behaviour/immediatefeedback/behaviourtype.php +++ b/question/behaviour/immediatefeedback/behaviourtype.php @@ -37,10 +37,7 @@ class qbehaviour_immediatefeedback_type extends question_behaviour_type { return true; } - /** - * Allow user to re-attempt graded questions during a quiz attempt - */ - public function user_can_reattempt_graded_question() { + public function can_questions_finish_during_the_attempt() { return true; } } diff --git a/question/behaviour/immediatefeedback/tests/behaviourtype_test.php b/question/behaviour/immediatefeedback/tests/behaviourtype_test.php index cdbf3249e2f..f78ff93a621 100644 --- a/question/behaviour/immediatefeedback/tests/behaviourtype_test.php +++ b/question/behaviour/immediatefeedback/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_immediatefeedback_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertTrue($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/informationitem/tests/behaviourtype_test.php b/question/behaviour/informationitem/tests/behaviourtype_test.php index 8fca870e8d2..529f4b14181 100644 --- a/question/behaviour/informationitem/tests/behaviourtype_test.php +++ b/question/behaviour/informationitem/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_informationitem_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/interactive/behaviourtype.php b/question/behaviour/interactive/behaviourtype.php index 641208dd356..b57bef8d35c 100644 --- a/question/behaviour/interactive/behaviourtype.php +++ b/question/behaviour/interactive/behaviourtype.php @@ -41,12 +41,7 @@ class qbehaviour_interactive_type extends question_behaviour_type { return true; } - /** - * Allow user to re-attempt graded questions during a quiz attempt - */ - public function user_can_reattempt_graded_question() { + public function can_questions_finish_during_the_attempt() { return true; } - - } diff --git a/question/behaviour/interactive/tests/behaviourtype_test.php b/question/behaviour/interactive/tests/behaviourtype_test.php index 353dfe04536..ad1f672e698 100644 --- a/question/behaviour/interactive/tests/behaviourtype_test.php +++ b/question/behaviour/interactive/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_interactive_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertTrue($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/interactivecountback/tests/behaviourtype_test.php b/question/behaviour/interactivecountback/tests/behaviourtype_test.php index 7b937e2a966..992605b335e 100644 --- a/question/behaviour/interactivecountback/tests/behaviourtype_test.php +++ b/question/behaviour/interactivecountback/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_interactivecountback_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertTrue($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/manualgraded/tests/behaviourtype_test.php b/question/behaviour/manualgraded/tests/behaviourtype_test.php index 9957a8a967b..e5718dd3f9b 100644 --- a/question/behaviour/manualgraded/tests/behaviourtype_test.php +++ b/question/behaviour/manualgraded/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_manualgraded_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/missing/tests/behaviourtype_test.php b/question/behaviour/missing/tests/behaviourtype_test.php index fe18caec4a5..ad1482ea162 100644 --- a/question/behaviour/missing/tests/behaviourtype_test.php +++ b/question/behaviour/missing/tests/behaviourtype_test.php @@ -56,6 +56,10 @@ class qbehaviour_missing_type_test extends basic_testcase { $this->behaviourtype->get_unused_display_options()); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse($this->behaviourtype->can_questions_finish_during_the_attempt()); + } + public function test_adjust_random_guess_score() { $this->assertEquals(0, $this->behaviourtype->adjust_random_guess_score(0)); $this->assertEquals(1, $this->behaviourtype->adjust_random_guess_score(1)); diff --git a/question/behaviour/upgrade.txt b/question/behaviour/upgrade.txt index 1b496d8cd67..5c530cd8ebe 100644 --- a/question/behaviour/upgrade.txt +++ b/question/behaviour/upgrade.txt @@ -2,10 +2,16 @@ This files describes API changes for question behaviour plugins. === 2.9 === -1) New method question_behaviour::can_finish_during_attempt. This returns false - by default. You should override it if, with your behaviour, questions may +1) There are new methods question_behaviour::can_finish_during_attempt and + question_behaviour_type::can_finish_during_attempt. These methods both return + false by default. You should override it if, with your behaviour, questions may finish just through the student interacting with them (e.g. by clicking the - Check button within the question.) + Check button within the question). + + The behaviour type method answers the the question for this behaviour in + general, without reference to a specific question. The method on the + behaviour class answers the question for a specific attempt at a specific + question. === 2.7 === diff --git a/question/engine/lib.php b/question/engine/lib.php index 2ff5fd32edb..03d6dae8a0c 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -200,6 +200,16 @@ abstract class question_engine { return self::get_behaviour_type($behaviour)->get_unused_display_options(); } + /** + * With this behaviour, is it possible that a question might finish as the student + * interacts with it, without a call to the {@link question_attempt::finish()} method? + * @param string $behaviour the name of a behaviour. E.g. 'deferredfeedback'. + * @return bool whether with this behaviour, questions may finish naturally. + */ + public static function can_questions_finish_during_the_attempt($behaviour) { + return self::get_behaviour_type($behaviour)->can_questions_finish_during_the_attempt(); + } + /** * Create a behaviour for a particular type. If that type cannot be * found, return an instance of qbehaviour_missing. diff --git a/question/engine/tests/questionengine_test.php b/question/engine/tests/questionengine_test.php index 1a5505d9aba..4f63d582ed1 100644 --- a/question/engine/tests/questionengine_test.php +++ b/question/engine/tests/questionengine_test.php @@ -62,6 +62,11 @@ class question_engine_test extends advanced_testcase { question_engine::get_behaviour_unused_display_options('manualgraded')); } + public function test_can_questions_finish_during_the_attempt() { + $this->assertFalse(question_engine::can_questions_finish_during_the_attempt('deferredfeedback')); + $this->assertTrue(question_engine::can_questions_finish_during_the_attempt('interactive')); + } + public function test_sort_behaviours() { $in = array('b1' => 'Behave 1', 'b2' => 'Behave 2', 'b3' => 'Behave 3', 'b4' => 'Behave 4', 'b5' => 'Behave 5', 'b6' => 'Behave 6'); From f6579bea9417609836d089b52ccf88dc4904ad4e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Fri, 13 Mar 2015 19:07:44 +0000 Subject: [PATCH 3/4] MDL-40992 question engine: new ways modify question usages * A method to change the max mark for one question_attempt in the usage * A method to replace one question in a usage with another, moving the old question_attempt to the end. * Methods to set and get metadata (string name value pairs) for each question_attempt in the usage. This gets stored in the first step in a way that should not interfere with anything else. --- question/engine/datalib.php | 188 +++++++++++----- question/engine/questionattempt.php | 39 +++- question/engine/questionattemptstep.php | 42 ++++ question/engine/questionusage.php | 124 ++++++++--- question/engine/tests/helpers.php | 8 + question/engine/tests/unitofwork_test.php | 253 +++++++++++++++++++++- question/engine/upgrade.txt | 41 +++- question/upgrade.txt | 2 +- 8 files changed, 610 insertions(+), 87 deletions(-) diff --git a/question/engine/datalib.php b/question/engine/datalib.php index ab41023ed70..55be7668a27 100644 --- a/question/engine/datalib.php +++ b/question/engine/datalib.php @@ -252,6 +252,48 @@ class question_engine_data_mapper { return $this->prepare_step_data($step, $record->id, $context); } + /** + * Store new metadata for an existing {@link question_attempt} in the database. + * + * Private method, only for use by other parts of the question engine. + * + * @param question_attempt $qa the question attempt to store meta data for. + * @param array $names the names of the metadata variables to store. + * @return array of question_attempt_step_data rows, that still need to be inserted. + */ + public function insert_question_attempt_metadata(question_attempt $qa, array $names) { + $firststep = $qa->get_step(0); + + $rows = array(); + foreach ($names as $name) { + $data = new stdClass(); + $data->attemptstepid = $firststep->get_id(); + $data->name = ':_' . $name; + $data->value = $firststep->get_metadata_var($name); + $rows[] = $data; + } + + return $rows; + } + + /** + * Updates existing metadata for an existing {@link question_attempt} in the database. + * + * Private method, only for use by other parts of the question engine. + * + * @param question_attempt $qa the question attempt to store meta data for. + * @param array $names the names of the metadata variables to store. + * @return array of question_attempt_step_data rows, that still need to be inserted. + */ + public function update_question_attempt_metadata(question_attempt $qa, array $names) { + global $DB; + list($condition, $params) = $DB->get_in_or_equal($names); + $params[] = $qa->get_step(0)->get_id(); + $DB->delete_records_select('question_attempt_step_data', + 'name ' . $condition . ' AND attemptstepid = ?', $params); + return $this->insert_question_attempt_metadata($qa, $names); + } + /** * Load a {@link question_attempt_step} from the database. * @@ -867,6 +909,7 @@ ORDER BY public function update_question_attempt(question_attempt $qa) { $record = new stdClass(); $record->id = $qa->get_database_id(); + $record->slot = $qa->get_slot(); $record->variant = $qa->get_variant(); $record->maxmark = $qa->get_max_mark(); $record->minfraction = $qa->get_min_fraction(); @@ -880,16 +923,6 @@ ORDER BY $this->db->update_record('question_attempts', $record); } - /** - * Delete a question_attempts row to reflect any changes in a question_attempt - * (but not any of its steps). - * @param question_attempt $qa the question attempt that has been deleted. - */ - public function delete_question_attempt(question_attempt $qa) { - $conditions = array('questionusageid' => $qa->get_usage_id(), 'slot' => $qa->get_slot()); - $this->db->delete_records('question_attempts', $conditions); - } - /** * Delete a question_usage_by_activity and all its associated * @@ -1254,24 +1287,18 @@ class question_engine_unit_of_work implements question_usage_observer { /** @var boolean whether any of the fields of the usage have been changed. */ protected $modified = false; - /** - * @var array list of slot => {@link question_attempt}s that - * were already in the usage, and which have been modified. - */ - protected $attemptsmodified = array(); - - /** - * @var array list of slot => {@link question_attempt}s that - * were already in the usage, and which have been deleted. - */ - protected $attemptsdeleted = array(); - /** * @var array list of slot => {@link question_attempt}s that * have been added to the usage. */ protected $attemptsadded = array(); + /** + * @var array list of slot => {@link question_attempt}s that + * were already in the usage, and which have been modified. + */ + protected $attemptsmodified = array(); + /** * @var array of array(question_attempt_step, question_attempt id, seq number) * of steps that have been added to question attempts in this usage. @@ -1290,6 +1317,16 @@ class question_engine_unit_of_work implements question_usage_observer { */ protected $stepsdeleted = array(); + /** + * @var array int slot => string name => question_attempt. + */ + protected $metadataadded = array(); + + /** + * @var array int slot => string name => question_attempt. + */ + protected $metadatamodified = array(); + /** * Constructor. * @param question_usage_by_activity $quba the usage to track. @@ -1302,6 +1339,10 @@ class question_engine_unit_of_work implements question_usage_observer { $this->modified = true; } + public function notify_attempt_added(question_attempt $qa) { + $this->attemptsadded[$qa->get_slot()] = $qa; + } + public function notify_attempt_modified(question_attempt $qa) { $slot = $qa->get_slot(); if (!array_key_exists($slot, $this->attemptsadded)) { @@ -1309,27 +1350,27 @@ class question_engine_unit_of_work implements question_usage_observer { } } - /** - * Notify when attempt deleted - * - * @see question_usage_observer::notify_attempt_deleted() - */ - public function notify_attempt_deleted(question_attempt $qa) { - $slot = $qa->get_slot(); - if (!array_key_exists($slot, $this->attemptsadded)) { - $this->attemptsdeleted[$slot] = $qa; - } - } + public function notify_attempt_moved(question_attempt $qa, $oldslot) { + $newslot = $qa->get_slot(); - /** - * Notify when attempt added - * - * @see question_usage_observer::notify_attempt_added() - */ - public function notify_attempt_added(question_attempt $qa) { - $slot = $qa->get_slot(); - if (!array_key_exists($slot, $this->attemptsadded)) { - $this->attemptsadded[$slot] = $qa; + if (array_key_exists($oldslot, $this->attemptsadded)) { + unset($this->attemptsadded[$oldslot]); + $this->attemptsadded[$newslot] = $qa; + return; + } + + if (array_key_exists($oldslot, $this->attemptsmodified)) { + unset($this->attemptsmodified[$oldslot]); + } + $this->attemptsmodified[$newslot] = $qa; + + if (array_key_exists($oldslot, $this->metadataadded)) { + $this->metadataadded[$newslot] = $this->metadataadded[$oldslot]; + unset($this->metadataadded[$oldslot]); + } + if (array_key_exists($oldslot, $this->metadatamodified)) { + $this->metadatamodified[$newslot] = $this->metadatamodified[$oldslot]; + unset($this->metadatamodified[$oldslot]); } } @@ -1407,6 +1448,42 @@ class question_engine_unit_of_work implements question_usage_observer { $this->stepsdeleted[$stepid] = $step; } + public function notify_metadata_added(question_attempt $qa, $name) { + if (array_key_exists($qa->get_slot(), $this->attemptsadded)) { + return; + } + + if ($this->is_step_added($qa->get_step(0)) !== false) { + return; + } + + if (isset($this->metadataadded[$qa->get_slot()][$name])) { + return; + } + + $this->metadataadded[$qa->get_slot()][$name] = $qa; + } + + public function notify_metadata_modified(question_attempt $qa, $name) { + if (array_key_exists($qa->get_slot(), $this->attemptsadded)) { + return; + } + + if ($this->is_step_added($qa->get_step(0)) !== false) { + return; + } + + if (isset($this->metadataadded[$qa->get_slot()][$name])) { + return; + } + + if (isset($this->metadatamodified[$qa->get_slot()][$name])) { + return; + } + + $this->metadatamodified[$qa->get_slot()][$name] = $qa; + } + /** * @param question_attempt_step $step a step * @return int|false if the step is in the list of steps to be added, return @@ -1473,8 +1550,8 @@ class question_engine_unit_of_work implements question_usage_observer { $step, $questionattemptid, $seq, $this->quba->get_owning_context()); } - foreach ($this->attemptsdeleted as $qa) { - $dm->delete_question_attempt($qa); + foreach ($this->attemptsmodified as $qa) { + $dm->update_question_attempt($qa); } foreach ($this->attemptsadded as $qa) { @@ -1482,18 +1559,31 @@ class question_engine_unit_of_work implements question_usage_observer { $qa, $this->quba->get_owning_context()); } - foreach ($this->attemptsmodified as $qa) { - $dm->update_question_attempt($qa); + foreach ($this->metadataadded as $info) { + $qa = reset($info); + $stepdata[] = $dm->insert_question_attempt_metadata($qa, array_keys($info)); + } + + foreach ($this->metadatamodified as $info) { + $qa = reset($info); + $stepdata[] = $dm->update_question_attempt_metadata($qa, array_keys($info)); } if ($this->modified) { $dm->update_questions_usage_by_activity($this->quba); } - if (!$stepdata) { - return; + if ($stepdata) { + $dm->insert_all_step_data(call_user_func_array('array_merge', $stepdata)); } - $dm->insert_all_step_data(call_user_func_array('array_merge', $stepdata)); + + $this->stepsdeleted = array(); + $this->stepsmodified = array(); + $this->stepsadded = array(); + $this->attemptsdeleted = array(); + $this->attemptsadded = array(); + $this->attemptsmodified = array(); + $this->modified = false; } } diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index 8a43d90c745..bff43e8d535 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -362,7 +362,7 @@ class question_attempt { /** * Get one of the steps in this attempt. * - * @param int $i the step number. + * @param int $i the step number, which counts from 0. * @return question_attempt_step */ public function get_step($i) { @@ -748,6 +748,30 @@ class question_attempt { return $this->behaviour->summarise_action($step); } + /** + * Return one of the bits of metadata for a this question attempt. + * @param string $name the name of the metadata variable to return. + * @return string the value of that metadata variable. + */ + public function get_metadata($name) { + return $this->get_step(0)->get_metadata_var($name); + } + + /** + * Set some metadata for this question attempt. + * @param string $name the name of the metadata variable to return. + * @param string $value the value to set that metadata variable to. + */ + public function set_metadata($name, $value) { + $firststep = $this->get_step(0); + if (!$firststep->has_metadata_var($name)) { + $this->observer->notify_metadata_added($this, $name); + } else if ($value !== $firststep->get_metadata_var($name)) { + $this->observer->notify_metadata_modified($this, $name); + } + $firststep->set_metadata_var($name, $value); + } + /** * Helper function used by {@link rewrite_pluginfile_urls()} and * {@link rewrite_response_pluginfile_urls()}. @@ -931,6 +955,10 @@ class question_attempt { public function start($preferredbehaviour, $variant, $submitteddata = array(), $timestamp = null, $userid = null, $existingstepid = null) { + if ($this->get_num_steps() > 0) { + throw new coding_exception('Cannot start a question that is already started.'); + } + // Initialise the behaviour. $this->variant = $variant; if (is_string($preferredbehaviour)) { @@ -1266,6 +1294,15 @@ class question_attempt { } } + /** + * Change the max mark for this question_attempt. + * @param float $maxmark the new max mark. + */ + public function set_max_mark($maxmark) { + $this->maxmark = $maxmark; + $this->observer->notify_attempt_modified($this); + } + /** * Perform a manual grading action on this attempt. * @param string $comment the comment being added. diff --git a/question/engine/questionattemptstep.php b/question/engine/questionattemptstep.php index 101a37f5098..c0bb6fd67c3 100644 --- a/question/engine/questionattemptstep.php +++ b/question/engine/questionattemptstep.php @@ -370,6 +370,48 @@ class question_attempt_step { return $this->data; } + /** + * Set a metadata variable. + * + * Do not call this method directly from your code. It is for internal + * use only. You should call {@link question_usage::set_question_attempt_metadata()}. + * + * @param string $name the name of the variable to set. [a-z][a-z0-9]*. + * @param string $value the value to set. + */ + public function set_metadata_var($name, $value) { + $this->data[':_' . $name] = $value; + } + + /** + * Whether this step has a metadata variable. + * + * Do not call this method directly from your code. It is for internal + * use only. You should call {@link question_usage::get_question_attempt_metadata()}. + * + * @param string $name the name of the variable to set. [a-z][a-z0-9]*. + * @return bool the value to set previously, or null if this variable was never set. + */ + public function has_metadata_var($name) { + return isset($this->data[':_' . $name]); + } + + /** + * Get a metadata variable. + * + * Do not call this method directly from your code. It is for internal + * use only. You should call {@link question_usage::get_question_attempt_metadata()}. + * + * @param string $name the name of the variable to set. [a-z][a-z0-9]*. + * @return string the value to set previously, or null if this variable was never set. + */ + public function get_metadata_var($name) { + if (!$this->has_metadata_var($name)) { + return null; + } + return $this->data[':_' . $name]; + } + /** * Create a question_attempt_step from records loaded from the database. * @param Iterator $records Raw records loaded from the database. diff --git a/question/engine/questionusage.php b/question/engine/questionusage.php index d5a213b390f..49c0ef51638 100644 --- a/question/engine/questionusage.php +++ b/question/engine/questionusage.php @@ -172,6 +172,42 @@ class question_usage_by_activity { return $qa->get_slot(); } + /** + * Add another question to this usage, in the place of an existing slot. + * The question_attempt that was in that slot is moved to the end at a new + * slot number, which is returned. + * + * The added question is not started until you call {@link start_question()} + * on it. + * + * @param int $slot the slot-number of the question to replace. + * @param question_definition $question the question to add. + * @param number $maxmark the maximum this question will be marked out of in + * this attempt (optional). If not given, the max mark from the $qa we + * are replacing is used. + * @return int the new slot number of the question that was displaced. + */ + public function add_question_in_place_of_other($slot, question_definition $question, $maxmark = null) { + $newslot = $this->next_slot_number(); + + $oldqa = $this->get_question_attempt($slot); + $oldqa->set_slot($newslot); + $this->questionattempts[$newslot] = $oldqa; + + if ($maxmark === null) { + $maxmark = $oldqa->get_max_mark(); + } + + $qa = new question_attempt($question, $this->get_id(), $this->observer, $maxmark); + $qa->set_slot($slot); + $this->questionattempts[$slot] = $qa; + + $this->observer->notify_attempt_moved($oldqa, $slot); + $this->observer->notify_attempt_added($qa); + + return $newslot; + } + /** * The slot number that will be allotted to the next question added. */ @@ -377,6 +413,27 @@ class question_usage_by_activity { return $this->get_question_attempt($slot)->get_right_answer_summary(); } + /** + * Return one of the bits of metadata for a particular question attempt in + * this usage. + * @param int $slot the slot number of the question of inereest. + * @param string $name the name of the metadata variable to return. + * @return string the value of that metadata variable. + */ + public function get_question_attempt_metadata($slot, $name) { + return $this->get_question_attempt($slot)->get_metadata($name); + } + + /** + * Set some metadata for a particular question attempt in this usage. + * @param int $slot the slot number of the question of inerest. + * @param string $name the name of the metadata variable to return. + * @param string $value the value to set that metadata variable to. + */ + public function set_question_attempt_metadata($slot, $name, $value) { + $this->get_question_attempt($slot)->set_metadata($name, $value); + } + /** * Get the {@link core_question_renderer}, in collaboration with appropriate * {@link qbehaviour_renderer} and {@link qtype_renderer} subclasses, to generate the @@ -822,22 +879,6 @@ class question_usage_by_activity { $this->observer->notify_attempt_modified($newqa); } - /** - * Replace a question in this usage. - * @param int $slot the number used to identify this question within this usage.* - */ - public function replace_question($slot) { - global $OUTPUT; - $oldqa = $this->get_question_attempt($slot); - $newqa = new question_attempt($oldqa->get_question(), $oldqa->get_usage_id(), $this->observer); - $newqa->set_database_id($oldqa->get_database_id()); - $newqa->set_slot($oldqa->get_slot()); - $this->questionattempts[$slot] = $newqa; - $this->observer->notify_attempt_deleted($oldqa); - $this->observer->notify_attempt_added($newqa); - $this->start_question($slot); - } - /** * Regrade all the questions in this usage (without changing their max mark). * @param bool $finished whether each question should be forced to be finished @@ -849,6 +890,15 @@ class question_usage_by_activity { } } + /** + * Change the max mark for this question_attempt. + * @param int $slot the slot number of the question of inerest. + * @param float $maxmark the new max mark. + */ + public function set_max_mark($slot, $maxmark) { + $this->get_question_attempt($slot)->set_max_mark($maxmark); + } + /** * Create a question_usage_by_activity from records loaded from the database. * @@ -983,12 +1033,6 @@ interface question_usage_observer { /** Called when a field of the question_usage_by_activity is changed. */ public function notify_modified(); - /** - * Called when the fields of a question attempt in this usage are modified. - * @param question_attempt $qa the newly added question attempt. - */ - public function notify_attempt_modified(question_attempt $qa); - /** * Called when a new question attempt is added to this usage. * @param question_attempt $qa the newly added question attempt. @@ -996,10 +1040,17 @@ interface question_usage_observer { public function notify_attempt_added(question_attempt $qa); /** - * Called when the fields of a question attempt in this usage are deleted. - * @param question_attempt $qa + * Called when the fields of a question attempt in this usage are modified. + * @param question_attempt $qa the newly added question attempt. */ - public function notify_attempt_deleted(question_attempt $qa); + public function notify_attempt_modified(question_attempt $qa); + + /** + * Called when a question_attempt has been moved to a new slot. + * @param question_attempt $qa The question attempt that was moved. + * @param int $oldslot The previous slot number of that attempt. + */ + public function notify_attempt_moved(question_attempt $qa, $oldslot); /** * Called when a new step is added to a question attempt in this usage. @@ -1024,6 +1075,19 @@ interface question_usage_observer { */ public function notify_step_deleted(question_attempt_step $step, question_attempt $qa); + /** + * Called when a new metadata variable is set on a question attempt in this usage. + * @param question_attempt $qa the question attempt the metadata is being added to. + * @param int $name the name of the metadata variable added. + */ + public function notify_metadata_added(question_attempt $qa, $name); + + /** + * Called when a metadata variable on a question attempt in this usage is updated. + * @param question_attempt $qa the question attempt where the metadata is being modified. + * @param int $name the name of the metadata variable modified. + */ + public function notify_metadata_modified(question_attempt $qa, $name); } @@ -1037,11 +1101,11 @@ interface question_usage_observer { class question_usage_null_observer implements question_usage_observer { public function notify_modified() { } + public function notify_attempt_added(question_attempt $qa) { + } public function notify_attempt_modified(question_attempt $qa) { } - public function notify_attempt_deleted(question_attempt $qa) { - } - public function notify_attempt_added(question_attempt $qa) { + public function notify_attempt_moved(question_attempt $qa, $oldslot) { } public function notify_step_added(question_attempt_step $step, question_attempt $qa, $seq) { } @@ -1049,4 +1113,8 @@ class question_usage_null_observer implements question_usage_observer { } public function notify_step_deleted(question_attempt_step $step, question_attempt $qa) { } + public function notify_metadata_added(question_attempt $qa, $name) { + } + public function notify_metadata_modified(question_attempt $qa, $name) { + } } diff --git a/question/engine/tests/helpers.php b/question/engine/tests/helpers.php index f596ab037de..c472e1eb9ea 100644 --- a/question/engine/tests/helpers.php +++ b/question/engine/tests/helpers.php @@ -84,6 +84,14 @@ class testable_question_engine_unit_of_work extends question_engine_unit_of_work public function get_steps_deleted() { return $this->stepsdeleted; } + + public function get_metadata_added() { + return $this->metadataadded; + } + + public function get_metadata_modified() { + return $this->metadatamodified; + } } diff --git a/question/engine/tests/unitofwork_test.php b/question/engine/tests/unitofwork_test.php index 17cec0d3660..873b8570c3f 100644 --- a/question/engine/tests/unitofwork_test.php +++ b/question/engine/tests/unitofwork_test.php @@ -91,8 +91,8 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233720, 1, '-submit', 1), array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233720, 1, '-_triesleft', 1), array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 3, 2, 'todo', null, 1256233740, 1, '-tryagain', 1), - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', null, 1256233790, 1, 'answer', 'frog'), - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', 1.0000000, 1256233790, 1, '-submit', 1), + array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', 0.6666667, 1256233790, 1, 'answer', 'frog'), + array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 1.0000000, 0.0000000, 1.0000000, 0, '', '', '', 1256233790, 5, 3, 'gradedright', 0.6666667, 1256233790, 1, '-submit', 1), ); } @@ -103,6 +103,8 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { $this->assertEquals(0, count($this->observer->get_steps_added())); $this->assertEquals(0, count($this->observer->get_steps_modified())); $this->assertEquals(0, count($this->observer->get_steps_deleted())); + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_update_usage() { @@ -120,6 +122,9 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { $this->assertEquals(1, count($newattempts)); $this->assertTrue($this->quba->get_question_attempt($slot) === reset($newattempts)); $this->assertSame($slot, key($newattempts)); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_add_and_start_question() { @@ -136,6 +141,9 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { $this->assertTrue($this->quba->get_question_attempt($slot) === reset($newattempts)); $this->assertSame($slot, key($newattempts)); $this->assertEquals(0, count($this->observer->get_steps_added())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_process_action() { @@ -157,6 +165,9 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { list($newstep, $qaid, $seq) = reset($newsteps); $this->assertSame($this->quba->get_question_attempt($this->slot)->get_last_step(), $newstep); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_regrade_same_steps() { @@ -184,6 +195,9 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { $this->assertSame(array($step, $updatedattempt->get_database_id(), $seq), $updatedsteps[$seq]); } + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_regrade_losing_steps() { @@ -220,6 +234,9 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { $seconddeletedstep = end($deletedsteps); $this->assertEquals(array('answer' => 'frog', '-submit' => 1), $seconddeletedstep->get_all_data()); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); } public function test_tricky_regrade() { @@ -258,5 +275,237 @@ class question_engine_unit_of_work_test extends data_loading_method_test_base { } $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_move_question() { + + $q = test_question_maker::make_question('truefalse'); + $newslot = $this->quba->add_question_in_place_of_other($this->slot, $q); + $this->quba->start_question($this->slot); + + $addedattempts = $this->observer->get_attempts_added(); + $this->assertEquals(1, count($addedattempts)); + $addedattempt = reset($addedattempts); + $this->assertSame($this->quba->get_question_attempt($this->slot), $addedattempt); + + $updatedattempts = $this->observer->get_attempts_modified(); + $this->assertEquals(1, count($updatedattempts)); + $updatedattempt = reset($updatedattempts); + $this->assertSame($this->quba->get_question_attempt($newslot), $updatedattempt); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_move_question_then_modify() { + + $q = test_question_maker::make_question('truefalse'); + $newslot = $this->quba->add_question_in_place_of_other($this->slot, $q); + $this->quba->start_question($this->slot); + $this->quba->process_action($this->slot, array('answer' => 'frog', '-submit' => 1)); + $this->quba->manual_grade($newslot, 'Test', 0.5, FORMAT_HTML); + + $addedattempts = $this->observer->get_attempts_added(); + $this->assertEquals(1, count($addedattempts)); + $addedattempt = reset($addedattempts); + $this->assertSame($this->quba->get_question_attempt($this->slot), $addedattempt); + + $updatedattempts = $this->observer->get_attempts_modified(); + $this->assertEquals(1, count($updatedattempts)); + $updatedattempt = reset($updatedattempts); + $this->assertSame($this->quba->get_question_attempt($newslot), $updatedattempt); + + $newsteps = $this->observer->get_steps_added(); + $this->assertEquals(1, count($newsteps)); + list($newstep, $qaid, $seq) = reset($newsteps); + $this->assertSame($this->quba->get_question_attempt($newslot)->get_last_step(), $newstep); + + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_move_question_then_move_again() { + $originalqa = $this->quba->get_question_attempt($this->slot); + + $q1 = test_question_maker::make_question('truefalse'); + $newslot = $this->quba->add_question_in_place_of_other($this->slot, $q1); + $this->quba->start_question($this->slot); + + $q2 = test_question_maker::make_question('truefalse'); + $newslot2 = $this->quba->add_question_in_place_of_other($newslot, $q2); + $this->quba->start_question($newslot); + + $addedattempts = $this->observer->get_attempts_added(); + $this->assertEquals(2, count($addedattempts)); + + $updatedattempts = $this->observer->get_attempts_modified(); + $this->assertEquals(1, count($updatedattempts)); + $updatedattempt = reset($updatedattempts); + $this->assertSame($originalqa, $updatedattempt); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_max_mark() { + $this->quba->set_max_mark($this->slot, 6.0); + $this->assertEquals(4.0, $this->quba->get_total_mark(), '', 0.0000005); + + $this->assertEquals(0, count($this->observer->get_attempts_added())); + + $updatedattempts = $this->observer->get_attempts_modified(); + $this->assertEquals(1, count($updatedattempts)); + $updatedattempt = reset($updatedattempts); + $this->assertSame($this->quba->get_question_attempt($this->slot), $updatedattempt); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_question_attempt_metadata() { + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $this->assertEquals('a value', $this->quba->get_question_attempt_metadata($this->slot, 'metathingy')); + + $this->assertEquals(0, count($this->observer->get_attempts_added())); + $this->assertEquals(0, count($this->observer->get_attempts_modified())); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(array($this->slot => array('metathingy' => $this->quba->get_question_attempt($this->slot))), + $this->observer->get_metadata_added()); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_question_attempt_metadata_then_change() { + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'different value'); + $this->assertEquals('different value', $this->quba->get_question_attempt_metadata($this->slot, 'metathingy')); + + $this->assertEquals(0, count($this->observer->get_attempts_added())); + $this->assertEquals(0, count($this->observer->get_attempts_modified())); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(array($this->slot => array('metathingy' => $this->quba->get_question_attempt($this->slot))), + $this->observer->get_metadata_added()); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_metadata_previously_set_but_dont_actually_change() { + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $this->observer = new testable_question_engine_unit_of_work($this->quba); + $this->quba->set_observer($this->observer); + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $this->assertEquals('a value', $this->quba->get_question_attempt_metadata($this->slot, 'metathingy')); + + $this->assertEquals(0, count($this->observer->get_attempts_added())); + $this->assertEquals(0, count($this->observer->get_attempts_modified())); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_metadata_previously_set() { + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $this->observer = new testable_question_engine_unit_of_work($this->quba); + $this->quba->set_observer($this->observer); + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'different value'); + $this->assertEquals('different value', $this->quba->get_question_attempt_metadata($this->slot, 'metathingy')); + + $this->assertEquals(0, count($this->observer->get_attempts_added())); + $this->assertEquals(0, count($this->observer->get_attempts_modified())); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(array($this->slot => array('metathingy' => $this->quba->get_question_attempt($this->slot))), + $this->observer->get_metadata_modified()); + } + + public function test_set_metadata_in_new_question() { + $newslot = $this->quba->add_question(test_question_maker::make_question('truefalse')); + $this->quba->start_question($newslot); + $this->quba->set_question_attempt_metadata($newslot, 'metathingy', 'a value'); + $this->assertEquals('a value', $this->quba->get_question_attempt_metadata($newslot, 'metathingy')); + + $this->assertEquals(array($newslot => $this->quba->get_question_attempt($newslot)), + $this->observer->get_attempts_added()); + $this->assertEquals(0, count($this->observer->get_attempts_modified())); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(0, count($this->observer->get_metadata_added())); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_set_metadata_then_move() { + $this->quba->set_question_attempt_metadata($this->slot, 'metathingy', 'a value'); + $q = test_question_maker::make_question('truefalse'); + $newslot = $this->quba->add_question_in_place_of_other($this->slot, $q); + $this->quba->start_question($this->slot); + $this->assertEquals('a value', $this->quba->get_question_attempt_metadata($newslot, 'metathingy')); + + $this->assertEquals(array($this->slot => $this->quba->get_question_attempt($this->slot)), + $this->observer->get_attempts_added()); + $this->assertEquals(array($newslot => $this->quba->get_question_attempt($newslot)), + $this->observer->get_attempts_modified()); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(array($newslot => array('metathingy' => $this->quba->get_question_attempt($newslot))), + $this->observer->get_metadata_added()); + $this->assertEquals(0, count($this->observer->get_metadata_modified())); + } + + public function test_move_then_set_metadata() { + $q = test_question_maker::make_question('truefalse'); + $newslot = $this->quba->add_question_in_place_of_other($this->slot, $q); + $this->quba->start_question($this->slot); + $this->quba->set_question_attempt_metadata($newslot, 'metathingy', 'a value'); + $this->assertEquals('a value', $this->quba->get_question_attempt_metadata($newslot, 'metathingy')); + + $this->assertEquals(array($this->slot => $this->quba->get_question_attempt($this->slot)), + $this->observer->get_attempts_added()); + $this->assertEquals(array($newslot => $this->quba->get_question_attempt($newslot)), + $this->observer->get_attempts_modified()); + + $this->assertEquals(0, count($this->observer->get_steps_added())); + $this->assertEquals(0, count($this->observer->get_steps_modified())); + $this->assertEquals(0, count($this->observer->get_steps_deleted())); + + $this->assertEquals(array($newslot => array('metathingy' => $this->quba->get_question_attempt($newslot))), + $this->observer->get_metadata_added()); } } diff --git a/question/engine/upgrade.txt b/question/engine/upgrade.txt index bb773396e55..07cc42376ff 100644 --- a/question/engine/upgrade.txt +++ b/question/engine/upgrade.txt @@ -1,18 +1,47 @@ -This files describes API changes for the core question system. +This files describes API changes for the core question engine. + + +=== 2.9 === + +1) Some new methods on the question_usage class (and corresponding methods on + question_attempt, question_attempt_step, question_usage_observer, ... requried + to implement them, but almost certainly you should only be calling the + question_usage methods from your code. + + * question_usage::add_question_in_place_of_other($slot, $question, $maxmark = null) + + This creates a new questoin_attempt in place of an existing one, moving the + existing question_attempt to the end of the usage, in a new slot number. + The new slot number is returned. The goal is to replace the old attempt, but + not lose the old data. + + * question_usage::set_question_max_mark($slot, $maxmark) + + Sets the max mark for one question in this usage. Previously, you could + only change this using the bulk operation question_usage::set_max_mark_in_attempts; + + * question_usage::set_question_attempt_metadata($slot, $name, $value); + question_usage::get_question_attempt_metadata($slot, $name); + + You can now record metadata, that is, values stored by name, against + question_attempts. The question engine ignores this data (other than storing + and loading it) but you may find it useful in your code. + + To see examples of where these are used, look at the chagnes from MDL-40992. === 2.6 === 1) The method question_behaviour::is_manual_grade_in_range and move and become -question_engine::is_manual_grade_in_range. + question_engine::is_manual_grade_in_range. 2) The arguments to core_question_renderer::mark_summary changed from -($qa, $options) to ($qa, $behaviouroutput, $options). If you have overridden -that method you will need to update your code. + ($qa, $options) to ($qa, $behaviouroutput, $options). If you have overridden + that method you will need to update your code. 3) Heading level for number(), add_part_heading() and respond_history() -has been lowered by one level. These changes are part of improving the page -accessibility and making heading to have proper nesting. (MDL-41615) + has been lowered by one level. These changes are part of improving the page + accessibility and making heading to have proper nesting. (MDL-41615) === Earlier changes === diff --git a/question/upgrade.txt b/question/upgrade.txt index b71c27b8aa7..6e65e504a48 100644 --- a/question/upgrade.txt +++ b/question/upgrade.txt @@ -22,7 +22,7 @@ This files describes API changes for code that uses the question API. === 2.8 === -1) This is jsut a warning that some methods of the question_engine_data_mapper +1) This is just a warning that some methods of the question_engine_data_mapper class have changed. All these methods are ones that you should not have been calling directly from your code, so this should not cause any problems. The changed methods are: From 5e63b335feb18b24a71a653ba6be9e7bb30d340e Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 9 Mar 2015 18:40:18 +0000 Subject: [PATCH 4/4] MDL-40992 quiz: option to let students redo questions within an attempt This feature is designed for use on pracice or formative quizzes. It is available for quizzes that use Interactive or Immediate feedback behaviour. If the teacher turns this on in the quiz settings, then once a student has finished a question, they get a 'Redo question' button beside the question. If they click it, then the question they finished is replaced by a new one so they can try again to practise that particul skill or bit of knowledge a bit more. When randomisation is involved, the studnets will be given a question or variant that they have not seen before if possible. --- lang/en/question.php | 2 - mod/quiz/attempt.php | 6 - mod/quiz/attemptlib.php | 266 +++++++++++++++--- .../backup/moodle2/backup_quiz_stepslib.php | 2 +- mod/quiz/db/install.xml | 2 +- mod/quiz/db/upgrade.php | 4 +- mod/quiz/lang/en/quiz.php | 16 +- mod/quiz/mod_form.php | 17 +- mod/quiz/processattempt.php | 14 +- mod/quiz/renderer.php | 38 ++- mod/quiz/settings.php | 9 +- mod/quiz/styles.css | 8 +- ...{attempt.feature => attempt_basic.feature} | 0 .../behat/attempt_redo_questions.feature | 110 ++++++++ .../tests/behat/reattemptquestions.feature | 53 ---- mod/quiz/upgrade.txt | 10 + .../behaviour/informationitem/renderer.php | 2 +- question/engine/lib.php | 15 + question/engine/renderer.php | 7 +- question/engine/upgrade.txt | 5 + 20 files changed, 436 insertions(+), 150 deletions(-) rename mod/quiz/tests/behat/{attempt.feature => attempt_basic.feature} (100%) create mode 100644 mod/quiz/tests/behat/attempt_redo_questions.feature delete mode 100644 mod/quiz/tests/behat/reattemptquestions.feature diff --git a/lang/en/question.php b/lang/en/question.php index 1edd07787aa..10f7c99d2fb 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -397,9 +397,7 @@ $string['questiontext'] = 'Question text'; $string['requiresgrading'] = 'Requires grading'; $string['responsehistory'] = 'Response history'; $string['restart'] = 'Start again'; -$string['restartquestion'] = 'Restart question'; $string['restartwiththeseoptions'] = 'Start again with these options'; -$string['updatedisplayoptions'] = 'Update display options'; $string['rightanswer'] = 'Right answer'; $string['rightanswer_help'] = 'an automatically generated summary of the correct response. This can be limited, so you may wish to consider explaining the correct solution in the general feedback for the question, and turning this option off.'; $string['saved'] = 'Saved: {$a}'; diff --git a/mod/quiz/attempt.php b/mod/quiz/attempt.php index 5ac9659b239..181c9bc80c4 100644 --- a/mod/quiz/attempt.php +++ b/mod/quiz/attempt.php @@ -125,12 +125,6 @@ if ($attemptobj->get_currentpage() != $page) { $DB->set_field('quiz_attempts', 'currentpage', $page, array('id' => $attemptid)); } -// Process replace question action, when user press on 'Replace question' link. -$replacequestioninslot = optional_param('replacequestioninslot', 0, PARAM_INT); -if ($replacequestioninslot) { - $attemptobj->process_replace_question_actions($replacequestioninslot, time()); -} - // Initialise the JavaScript. $headtags = $attemptobj->get_html_head_contributions($page); $PAGE->requires->js_init_call('M.mod_quiz.init_attempt_form', null, false, quiz_get_js_module()); diff --git a/mod/quiz/attemptlib.php b/mod/quiz/attemptlib.php index 75a603307dc..aeb882f2793 100644 --- a/mod/quiz/attemptlib.php +++ b/mod/quiz/attemptlib.php @@ -951,11 +951,11 @@ class quiz_attempt { } /** - * Return the list of question ids for either a given page of the quiz, or for the + * Return the list of slot numbers for either a given page of the quiz, or for the * whole quiz. * * @param mixed $page string 'all' or integer page number. - * @return array the reqested list of question ids. + * @return array the requested list of slot numbers. */ public function get_slots($page = 'all') { if ($page === 'all') { @@ -969,6 +969,23 @@ class quiz_attempt { } } + /** + * Return the list of slot numbers for either a given page of the quiz, or for the + * whole quiz. + * + * @param mixed $page string 'all' or integer page number. + * @return array the requested list of slot numbers. + */ + public function get_active_slots($page = 'all') { + $activeslots = array(); + foreach ($this->get_slots($page) as $slot) { + if (!$this->is_blocked_by_previous_question($slot)) { + $activeslots[] = $slot; + } + } + return $activeslots; + } + /** * Get the question_attempt object for a particular question in this attempt. * @param int $slot the number used to identify this question within this attempt. @@ -978,6 +995,22 @@ class quiz_attempt { return $this->quba->get_question_attempt($slot); } + /** + * Get the question_attempt object for a particular question in this attempt. + * @param int $slot the number used to identify this question within this attempt. + * @return question_attempt + */ + public function all_question_attempts_originally_in_slot($slot) { + $qas = array(); + foreach ($this->quba->get_attempt_iterator() as $qa) { + if ($qa->get_metadata('originalslot') == $slot) { + $qas[] = $qa; + } + } + $qas[] = $this->quba->get_question_attempt($slot); + return $qas; + } + /** * Is a particular question in this attempt a real question, or something like a description. * @param int $slot the number used to identify this question within this attempt. @@ -1004,13 +1037,39 @@ class quiz_attempt { * @return bool whether the previous question must have been completed before this one can be seen. */ public function is_blocked_by_previous_question($slot) { - return $slot > 1 && $this->slots[$slot]->requireprevious && + return $slot > 1 && isset($this->slots[$slot]) && $this->slots[$slot]->requireprevious && !$this->get_quiz()->shufflequestions && $this->get_navigation_method() != QUIZ_NAVMETHOD_SEQ && - !$this->quba->get_question_state($slot - 1)->is_finished() && + !$this->get_question_state($slot - 1)->is_finished() && $this->quba->can_question_finish_during_attempt($slot - 1); } + /** + * Is it possible for this question to be re-started within this attempt? + * + * @param int $slot the number used to identify this question within this attempt. + * @return whether the student should be given the option to restart this question now. + */ + public function can_question_be_redone_now($slot) { + return $this->get_quiz()->canredoquestions && !$this->is_finished() && + $this->get_question_state($slot)->is_finished(); + } + + /** + * Given a slot in this attempt, which may or not be a redone question, return the original slot. + * + * @param int $slot identifies a particular question in this attempt. + * @return int the slot where this question was originally. + */ + public function get_original_slot($slot) { + $originalslot = $this->quba->get_question_attempt_metadata($slot, 'originalslot'); + if ($originalslot) { + return $originalslot; + } else { + return $slot; + } + } + /** * Get the displayed question number for a slot. * @param int $slot the number used to identify this question within this attempt. @@ -1042,6 +1101,16 @@ class quiz_attempt { return $this->quba->get_question($slot)->name; } + /** + * Return the {@link question_state} that this question is in. + * + * @param int $slot the number used to identify this question within this attempt. + * @return question_state the state this question is in. + */ + public function get_question_state($slot) { + return $this->quba->get_question_state($slot); + } + /** * Return the grade obtained on a particular question, if the user is permitted * to see it. You must previously have called load_question_states to load the @@ -1275,12 +1344,13 @@ class quiz_attempt { * Generate the HTML that displayes the question in its current state, with * the appropriate display options. * - * @param int $id the id of a question in this quiz attempt. + * @param int $slot identifies the question in the attempt. * @param bool $reviewing is the being printed on an attempt or a review page. + * @param mod_quiz_renderer $renderer the quiz renderer. * @param moodle_url $thispageurl the URL of the page this question is being printed on. * @return string HTML for the question in its current state. */ - public function render_question($slot, $reviewing, $thispageurl = null) { + public function render_question($slot, $reviewing, mod_quiz_renderer $renderer, $thispageurl = null) { if ($this->is_blocked_by_previous_question($slot)) { $placeholderqa = $this->make_blocked_question_placeholder($slot); @@ -1290,20 +1360,64 @@ class quiz_attempt { $displayoptions->readonly = true; return html_writer::div($placeholderqa->render($displayoptions, - $this->get_question_number($slot)), + $this->get_question_number($this->get_original_slot($slot))), 'mod_quiz-blocked_question_warning'); } - return $this->quba->render_question($slot, - $this->get_display_options_with_edit_link($reviewing, $slot, $thispageurl), - $this->get_question_number($slot)); + return $this->render_question_helper($slot, $reviewing, $thispageurl, $renderer, null); + } + + /** + * Helper used by {@link render_question()} and {@link render_question_at_step()}. + * + * @param int $slot identifies the question in the attempt. + * @param bool $reviewing is the being printed on an attempt or a review page. + * @param moodle_url $thispageurl the URL of the page this question is being printed on. + * @param mod_quiz_renderer $renderer the quiz renderer. + * @param int|null $seq the seq number of the past state to display. + * @return string HTML fragment. + */ + protected function render_question_helper($slot, $reviewing, $thispageurl, mod_quiz_renderer $renderer, $seq) { + $originalslot = $this->get_original_slot($slot); + $number = $this->get_question_number($originalslot); + $displayoptions = $this->get_display_options_with_edit_link($reviewing, $slot, $thispageurl); + + if ($slot != $originalslot) { + $originalmaxmark = $this->get_question_attempt($slot)->get_max_mark(); + $this->get_question_attempt($slot)->set_max_mark($this->get_question_attempt($originalslot)->get_max_mark()); + } + + if ($this->can_question_be_redone_now($slot)) { + $displayoptions->extrainfocontent = $renderer->redo_question_button( + $slot, $displayoptions->readonly); + } + + if ($displayoptions->history && $displayoptions->questionreviewlink) { + $links = $this->links_to_other_redos($slot, $displayoptions->questionreviewlink); + if ($links) { + $displayoptions->extrahistorycontent = html_writer::tag('p', + get_string('redoesofthisquestion', 'quiz', $renderer->render($links))); + } + } + + if ($seq === null) { + $output = $this->quba->render_question($slot, $displayoptions, $number); + } else { + $output = $this->quba->render_question_at_step($slot, $seq, $displayoptions, $number); + } + + if ($slot != $originalslot) { + $this->get_question_attempt($slot)->set_max_mark($originalmaxmark); + } + + return $output; } /** * Create a fake question to be displayed in place of a question that is blocked * until the previous question has been answered. * - * @param unknown $slot int slot number of the question to replace. + * @param int $slot int slot number of the question to replace. * @return question_definition the placeholde question. */ protected function make_blocked_question_placeholder($slot) { @@ -1345,13 +1459,12 @@ class quiz_attempt { * @param int $id the id of a question in this quiz attempt. * @param int $seq the seq number of the past state to display. * @param bool $reviewing is the being printed on an attempt or a review page. + * @param mod_quiz_renderer $renderer the quiz renderer. * @param string $thispageurl the URL of the page this question is being printed on. * @return string HTML for the question in its current state. */ - public function render_question_at_step($slot, $seq, $reviewing, $thispageurl = '') { - return $this->quba->render_question_at_step($slot, $seq, - $this->get_display_options_with_edit_link($reviewing, $slot, $thispageurl), - $this->get_question_number($slot)); + public function render_question_at_step($slot, $seq, $reviewing, mod_quiz_renderer $renderer, $thispageurl = '') { + return $this->render_question_helper($slot, $reviewing, $thispageurl, $renderer, $seq); } /** @@ -1401,11 +1514,18 @@ class quiz_attempt { } /** - * Given a URL containing attempt={this attempt id}, return an array of variant URLs + * Return an array of variant URLs to other attempts at this quiz. + * + * The $url passed in must contain an attempt parameter. + * + * The {@link mod_quiz_links_to_other_attempts} object returned contains an + * array with keys that are the attempt number, 1, 2, 3. + * The array values are either a {@link moodle_url} with the attmept parameter + * updated to point to the attempt id of the other attempt, or null corresponding + * to the current attempt number. + * * @param moodle_url $url a URL. - * @return string HTML fragment. Comma-separated list of links to the other - * attempts with the attempt number as the link text. The curent attempt is - * included but is not a link. + * @return mod_quiz_links_to_other_attempts containing array int => null|moodle_url. */ public function links_to_other_attempts(moodle_url $url) { $attempts = quiz_get_user_attempts($this->get_quiz()->id, $this->attempt->userid, 'all'); @@ -1424,6 +1544,47 @@ class quiz_attempt { return $links; } + /** + * Return an array of variant URLs to other redos of the question in a particular slot. + * + * The $url passed in must contain a slot parameter. + * + * The {@link mod_quiz_links_to_other_attempts} object returned contains an + * array with keys that are the redo number, 1, 2, 3. + * The array values are either a {@link moodle_url} with the slot parameter + * updated to point to the slot that has that redo of this question; or null + * corresponding to the redo identified by $slot. + * + * @param int $slot identifies a question in this attempt. + * @param moodle_url $baseurl the base URL to modify to generate each link. + * @return mod_quiz_links_to_other_attempts|null containing array int => null|moodle_url, + * or null if the question in this slot has not been redone. + */ + public function links_to_other_redos($slot, moodle_url $baseurl) { + $originalslot = $this->get_original_slot($slot); + + $qas = $this->all_question_attempts_originally_in_slot($originalslot); + if (count($qas) <= 1) { + return null; + } + + $links = new mod_quiz_links_to_other_attempts(); + $index = 1; + foreach ($qas as $qa) { + if ($qa->get_slot() == $slot) { + $links->links[$index] = null; + } else { + $url = new moodle_url($baseurl, array('slot' => $qa->get_slot())); + $links->links[$index] = new action_link($url, $index, + new popup_action('click', $url, 'reviewquestion', + array('width' => 450, 'height' => 650)), + array('title' => get_string('reviewresponse', 'question'))); + } + $index++; + } + return $links; + } + // Methods for processing ================================================== /** @@ -1528,43 +1689,56 @@ class quiz_attempt { } /** - * Process replace question action - * @param int $slot - * @param int $timestamp + * Replace a question in an attempt with a new attempt at the same qestion. + * @param int $slot the questoin to restart. + * @param int $timestamp the timestamp to record for this action. */ - public function process_replace_question_actions($slot, $timestamp) { + public function process_redo_question($slot, $timestamp) { global $DB; + if (!$this->can_question_be_redone_now($slot)) { + throw new coding_exception('Attempt to restart the question in slot ' . $slot . + ' when it is not in a state to be restarted.'); + } + + $qubaids = new \mod_quiz\question\qubaids_for_users_attempts( + $this->get_quizid(), $this->get_userid()); + $transaction = $DB->start_delegated_transaction(); - $this->quba->replace_question($slot); + $questiondata = $DB->get_record('question', + array('id' => $this->slots[$slot]->questionid)); + if ($questiondata->qtype != 'random') { + $newqusetionid = $questiondata->id; + } else { + $randomloader = new \core_question\bank\random_question_loader($qubaids, array()); + $newqusetionid = $randomloader->get_next_question_id($questiondata->category, + (bool) $questiondata->questiontext); + if ($newqusetionid === null) { + throw new moodle_exception('notenoughrandomquestions', 'quiz', + $quizobj->view_url(), $questiondata); + } + } + + $newquestion = question_bank::load_question($newqusetionid); + if ($newquestion->get_num_variants() == 1) { + $variant = 1; + } else { + $variantstrategy = new core_question\engine\variants\least_used_strategy( + $this->quba, $qubaids); + $variant = $variantstrategy->choose_variant($newquestion->get_num_variants(), + $newquestion->get_variants_selection_seed()); + } + + $newslot = $this->quba->add_question_in_place_of_other($slot, $newquestion); + $this->quba->start_question($slot); + $this->quba->set_max_mark($newslot, 0); + $this->quba->set_question_attempt_metadata($newslot, 'originalslot', $slot); question_engine::save_questions_usage_by_activity($this->quba); $transaction->allow_commit(); } - /** - * Return a button which allows students reattempting the current question - * - * @param int $slot, the number of the current slot - */ - public function restart_question_button($slot) { - // If 'reattemptgradedquestions' field is not set, do not display the 'Restart question' button. - if (!$this->get_quiz()->reattemptgradedquestions) { - return; - } - $qa = $this->get_question_attempt($slot); - - // If question is not graded, do not display the 'Restart question' button. - if (!$qa->get_state()->is_graded()) { - return; - } - $buttonvalue = get_string('restartquestion', 'question'); - return html_writer::tag('div', - " - "); - } - /** * Process all the autosaved data that was part of the current request. * diff --git a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php index a15bb810d1d..79fe698f4bb 100644 --- a/mod/quiz/backup/moodle2/backup_quiz_stepslib.php +++ b/mod/quiz/backup/moodle2/backup_quiz_stepslib.php @@ -41,7 +41,7 @@ class backup_quiz_activity_structure_step extends backup_questions_activity_stru // Define each element separated. $quiz = new backup_nested_element('quiz', array('id'), array( 'name', 'intro', 'introformat', 'timeopen', 'timeclose', 'timelimit', - 'overduehandling', 'graceperiod', 'preferredbehaviour', 'attempts_number', + 'overduehandling', 'graceperiod', 'preferredbehaviour', 'canredoquestions', 'attempts_number', 'attemptonlast', 'grademethod', 'decimalpoints', 'questiondecimalpoints', 'reviewattempt', 'reviewcorrectness', 'reviewmarks', 'reviewspecificfeedback', 'reviewgeneralfeedback', diff --git a/mod/quiz/db/install.xml b/mod/quiz/db/install.xml index 0cfa40de4a2..21bd11977a3 100644 --- a/mod/quiz/db/install.xml +++ b/mod/quiz/db/install.xml @@ -17,7 +17,7 @@ - + diff --git a/mod/quiz/db/upgrade.php b/mod/quiz/db/upgrade.php index b40fe080133..c9b503585cf 100644 --- a/mod/quiz/db/upgrade.php +++ b/mod/quiz/db/upgrade.php @@ -822,9 +822,9 @@ function xmldb_quiz_upgrade($oldversion) { } if ($oldversion < 2015030900) { - // Define field reattemptgradedquestions to be added to quiz. + // Define field canredoquestions to be added to quiz. $table = new xmldb_table('quiz'); - $field = new xmldb_field('reattemptgradedquestions', XMLDB_TYPE_INTEGER, '4', null, null, null, 0, 'completionpass'); + $field = new xmldb_field('canredoquestions', XMLDB_TYPE_INTEGER, '4', null, XMLDB_NOTNULL, null, 0, 'preferredbehaviour'); // Conditionally launch add field completionpass. if (!$dbman->field_exists($table, $field)) { diff --git a/mod/quiz/lang/en/quiz.php b/mod/quiz/lang/en/quiz.php index c45c1f1f3ca..60575e3d813 100644 --- a/mod/quiz/lang/en/quiz.php +++ b/mod/quiz/lang/en/quiz.php @@ -140,6 +140,14 @@ $string['cannotstartgradesmismatch'] = 'Cannot start an attempt at this quiz. Th $string['cannotstartmissingquestion'] = 'Cannot start an attempt at this quiz. The quiz definition includes a question that does not exist.'; $string['cannotstartnoquestions'] = 'Cannot start an attempt at this quiz. The quiz has not been set up yet. No questions have been added.'; $string['cannotwrite'] = 'Cannot write to export file ({$a})'; +$string['canredoquestions'] = 'Allow redo within an attempt'; +$string['canredoquestions_desc'] = 'If enabled, then when students have finished attempting particular question, they will see a Redo question button. This allows them to attempt another version of the same question, without having to submit the entire quiz attempt and start another one. This option is mainly useful for practice quizzes. + +This setting only affects questions (for example not Essay questions) and behaviours (for example Immediate feedback, or Interactive with multiple tries) where it is possible for student to finish the question before the attempt is submitted.'; +$string['canredoquestions_help'] = 'If enabled, then when students have finished attempting particular question, they will see a Redo question button. This allows them to attempt another version of the same question, without having to submit the entire quiz attempt and start another one. This option is mainly useful for practice quizzes. + +This setting only affects questions (for example not Essay questions) and behaviours (for example Immediate feedback, or Interactive with multiple tries) where it is possible for student to finish the question before the attempt is submitted.'; +$string['canredoquestionsyes'] = 'Students may redo another version of any finished question'; $string['caseno'] = 'No, case is unimportant'; $string['casesensitive'] = 'Case sensitivity'; $string['caseyes'] = 'Yes, case must match'; @@ -180,7 +188,6 @@ $string['configpenaltyscheme'] = 'Penalty subtracted for each wrong response in $string['configpopup'] = 'Force the attempt to open in a popup window, and use JavaScript tricks to try to restrict copy and paste, etc. during quiz attempts.'; $string['configrequirepassword'] = 'Students must enter this password before they can attempt the quiz.'; $string['configrequiresubnet'] = 'Students can only attempt the quiz from these computers.'; -$string['configrestartgradedquestions'] = 'If enabled, it allows students to restart graded questions in \'Immediate feedback\', \'Immediate feedback with CBM\' and \'Interactive with multiple tries behaviours\''; $string['configreviewoptions'] = 'These options control what information users can see when they review a quiz attempt or look at the quiz reports.'; $string['configshowblocks'] = 'Show blocks during quiz attempts.'; $string['configshowuserpicture'] = 'Show the user\'s picture on screen during attempts.'; @@ -449,6 +456,7 @@ $string['manualgrading'] = 'Grading'; $string['mark'] = 'Submit'; $string['markall'] = 'Submit page'; $string['marks'] = 'Marks'; +$string['marks_help'] = 'The numerical marks for each question, and the overall attempt score.'; $string['match'] = 'Matching'; $string['matchanswer'] = 'Matching answer'; $string['matchanswerno'] = 'Matching answer {$a}'; @@ -689,6 +697,8 @@ $string['readytosend'] = 'You are about to send your whole quiz to be graded. A $string['reattemptquiz'] = 'Re-attempt quiz'; $string['recentlyaddedquestion'] = 'Recently added question!'; $string['recurse'] = 'Include questions from subcategories too'; +$string['redoquestion'] = 'Redo question'; +$string['redoesofthisquestion'] = 'Other questions attempted here: {$a}'; $string['regrade'] = 'Regrade all attempts'; $string['regradecomplete'] = 'All attempts have been regraded'; $string['regradecount'] = '{$a->changed} out of {$a->attempt} grades were changed'; @@ -753,10 +763,6 @@ $string['reviewbefore'] = 'Allow review while quiz is open'; $string['reviewclosed'] = 'After the quiz is closed'; $string['reviewduring'] = 'During the attempt'; $string['reviewimmediately'] = 'Immediately after the attempt'; -$string['marks'] = 'Marks'; -$string['marks_help'] = 'The numerical marks for each question, and the overall attempt score.'; -$string['restartgradedquestions'] = 'Restart graded questions'; -$string['restartgradedquestions_help'] = 'If enabled, it allows students to restart graded questions in \'Immediate feedback\', \'Immediate feedback with CBM\' and \'Interactive with multiple tries behaviours\''; $string['reviewnever'] = 'Never allow review'; $string['reviewofattempt'] = 'Review of attempt {$a}'; $string['reviewofpreview'] = 'Review of preview'; diff --git a/mod/quiz/mod_form.php b/mod/quiz/mod_form.php index e1fcb1edb9f..2cbd1f18f12 100644 --- a/mod/quiz/mod_form.php +++ b/mod/quiz/mod_form.php @@ -205,17 +205,16 @@ class mod_quiz_mod_form extends moodleform_mod { $mform->addHelpButton('preferredbehaviour', 'howquestionsbehave', 'question'); $mform->setDefault('preferredbehaviour', $quizconfig->preferredbehaviour); - // TODO: Store the 'reattemptgradedquestions' field when new DB structure in place. - $mform->addElement('selectyesno', 'reattemptgradedquestions', get_string('restartgradedquestions', 'quiz')); - $mform->addHelpButton('reattemptgradedquestions', 'restartgradedquestions', 'quiz'); - $mform->setAdvanced('reattemptgradedquestions', $quizconfig->reattemptgradedquestions_adv); - $mform->setDefault('reattemptgradedquestions', $quizconfig->reattemptgradedquestions); + // Can redo completed questions. + $redochoices = array(0 => get_string('no'), 1 => get_string('canredoquestionsyes', 'quiz')); + $mform->addElement('select', 'canredoquestions', get_string('canredoquestions', 'quiz'), $redochoices); + $mform->addHelpButton('canredoquestions', 'canredoquestions', 'quiz'); + $mform->setAdvanced('canredoquestions', $quizconfig->canredoquestions_adv); + $mform->setDefault('canredoquestions', $quizconfig->canredoquestions); foreach ($behaviours as $behaviour => $notused) { - $qbt = question_engine::get_behaviour_type($behaviour); - if (!$qbt->user_can_reattempt_graded_question()) { - $mform->disabledIf('reattemptgradedquestions', 'preferredbehaviour', 'eq', $behaviour); + if (!question_engine::can_questions_finish_during_the_attempt($behaviour)) { + $mform->disabledIf('canredoquestions', 'preferredbehaviour', 'eq', $behaviour); } - } // Each attempt builds on last. diff --git a/mod/quiz/processattempt.php b/mod/quiz/processattempt.php index ff6cedcd581..1b5070ed2cb 100644 --- a/mod/quiz/processattempt.php +++ b/mod/quiz/processattempt.php @@ -44,12 +44,6 @@ $finishattempt = optional_param('finishattempt', false, PARAM_BOOL); $timeup = optional_param('timeup', 0, PARAM_BOOL); // True if form was submitted by timer. $scrollpos = optional_param('scrollpos', '', PARAM_RAW); -// Process replace question action, when user press on 'Replace question' link. -if (isset($_POST['restartquestioninslot'])) { - redirect(new moodle_url('/mod/quiz/attempt.php', - array('attempt' => $attemptid, 'page' => $thispage, 'replacequestioninslot' => $_POST['restartquestionincurrentslot']))); -} - $transaction = $DB->start_delegated_transaction(); $attemptobj = quiz_attempt::create($attemptid); @@ -150,6 +144,14 @@ if (!$finishattempt) { $attemptobj->attempt_url(null, $thispage), $e->getMessage(), $debuginfo); } + if (!$becomingoverdue) { + foreach ($attemptobj->get_slots() as $slot) { + if (optional_param('redoslot' . $slot, false, PARAM_BOOL)) { + $attemptobj->process_redo_question($slot, $timenow); + } + } + } + } else { // The student is too late. $attemptobj->process_going_overdue($timenow, true); diff --git a/mod/quiz/renderer.php b/mod/quiz/renderer.php index 5174dbf79ca..89832c58118 100644 --- a/mod/quiz/renderer.php +++ b/mod/quiz/renderer.php @@ -79,9 +79,9 @@ class mod_quiz_renderer extends plugin_renderer_base { $output .= $this->review_summary_table($summarydata, 0); if (!is_null($seq)) { - $output .= $attemptobj->render_question_at_step($slot, $seq, true); + $output .= $attemptobj->render_question_at_step($slot, $seq, true, $this); } else { - $output .= $attemptobj->render_question($slot, true); + $output .= $attemptobj->render_question($slot, true, $this); } $output .= $this->close_window_button(); @@ -182,7 +182,7 @@ class mod_quiz_renderer extends plugin_renderer_base { mod_quiz_display_options $displayoptions) { $output = ''; foreach ($slots as $slot) { - $output .= $attemptobj->render_question($slot, $reviewing, + $output .= $attemptobj->render_question($slot, $reviewing, $this, $attemptobj->review_url($slot, $page, $showall)); } return $output; @@ -382,10 +382,12 @@ class mod_quiz_renderer extends plugin_renderer_base { mod_quiz_links_to_other_attempts $links) { $attemptlinks = array(); foreach ($links->links as $attempt => $url) { - if ($url) { - $attemptlinks[] = html_writer::link($url, $attempt); - } else { + if (!$url) { $attemptlinks[] = html_writer::tag('strong', $attempt); + } else if ($url instanceof renderable) { + $attemptlinks[] = $this->render($url); + } else { + $attemptlinks[] = html_writer::link($url, $attempt); } } return implode(', ', $attemptlinks); @@ -459,9 +461,8 @@ class mod_quiz_renderer extends plugin_renderer_base { // Print all the questions. foreach ($slots as $slot) { - $output .= $attemptobj->render_question($slot, false, - $attemptobj->attempt_url($slot, $page)); - $output .= $attemptobj->restart_question_button($slot); + $output .= $attemptobj->render_question($slot, false, $this, + $attemptobj->attempt_url($slot, $page), $this); } $output .= html_writer::start_tag('div', array('class' => 'submitbtns')); @@ -487,7 +488,7 @@ class mod_quiz_renderer extends plugin_renderer_base { // if you navigate before the form has finished loading, it does not wipe all // the student's answers. $output .= html_writer::empty_tag('input', array('type' => 'hidden', 'name' => 'slots', - 'value' => implode(',', $slots))); + 'value' => implode(',', $attemptobj->get_active_slots($page)))); // Finish the form. $output .= html_writer::end_tag('div'); @@ -498,6 +499,22 @@ class mod_quiz_renderer extends plugin_renderer_base { return $output; } + /** + * Render a button which allows students to redo a question in the attempt. + * + * @param int $slot the number of the slot to generate the button for. + * @param bool $disabled if true, output the button disabled. + * @return string HTML fragment. + */ + public function redo_question_button($slot, $disabled) { + $attributes = array('type' => 'submit', 'name' => 'redoslot' . $slot, + 'value' => get_string('redoquestion', 'quiz'), 'class' => 'mod_quiz-redo_question_button'); + if ($disabled) { + $attributes['disabled'] = 'disabled'; + } + return html_writer::div(html_writer::empty_tag('input', $attributes)); + } + /** * Output the JavaScript required to initialise the countdown timer. * @param int $timerstartvalue time remaining, in seconds. @@ -1185,6 +1202,7 @@ class mod_quiz_renderer extends plugin_renderer_base { class mod_quiz_links_to_other_attempts implements renderable { /** * @var array string attempt number => url, or null for the current attempt. + * url may be either a moodle_url, or a renderable. */ public $links = array(); } diff --git a/mod/quiz/settings.php b/mod/quiz/settings.php index 4ef3dc4e711..63602172fe4 100644 --- a/mod/quiz/settings.php +++ b/mod/quiz/settings.php @@ -128,10 +128,11 @@ if ($ADMIN->fulltree) { get_string('howquestionsbehave', 'question'), get_string('howquestionsbehave_desc', 'quiz'), 'deferredfeedback')); - // Restart completed questions (reattemptgradedquestions). - $quizsettings->add(new admin_setting_configcheckbox_with_advanced('quiz/reattemptgradedquestions', - get_string('restartgradedquestions', 'quiz'), get_string('configrestartgradedquestions', 'quiz'), - array('value' => 0, 'adv' => true))); + // Can redo completed questions. + $quizsettings->add(new admin_setting_configselect_with_advanced('quiz/canredoquestions', + get_string('canredoquestions', 'quiz'), get_string('canredoquestions_desc', 'quiz'), + array('value' => 0, 'adv' => true), + array(0 => get_string('no'), 1 => get_string('canredoquestionsyes', 'quiz')))); // Each attempt builds on last. $quizsettings->add(new admin_setting_configcheckbox_with_advanced('quiz/attemptonlast', diff --git a/mod/quiz/styles.css b/mod/quiz/styles.css index 817caea6366..7f578648a7e 100644 --- a/mod/quiz/styles.css +++ b/mod/quiz/styles.css @@ -24,8 +24,12 @@ text-align: right; } -#page-mod-quiz-attempt .resatrt-question-btn { - font-size: 0.75em; +.path-mod-quiz .mod_quiz-redo_question_button { + margin: 0; +} +.path-mod-quiz input[type="submit"].mod_quiz-redo_question_button { + padding: 2px 0.8em; + font-size: 1em; } #page-mod-quiz-attempt .mod_quiz-blocked_question_warning .que .formulation, diff --git a/mod/quiz/tests/behat/attempt.feature b/mod/quiz/tests/behat/attempt_basic.feature similarity index 100% rename from mod/quiz/tests/behat/attempt.feature rename to mod/quiz/tests/behat/attempt_basic.feature diff --git a/mod/quiz/tests/behat/attempt_redo_questions.feature b/mod/quiz/tests/behat/attempt_redo_questions.feature new file mode 100644 index 00000000000..ec38b60c9e0 --- /dev/null +++ b/mod/quiz/tests/behat/attempt_redo_questions.feature @@ -0,0 +1,110 @@ +@mod @mod_quiz +Feature: Allow students to redo questions in a practice quiz, without starting a whole new attempt + In order to practice particular skills I am struggling with + As a student + I need to be able to redo each question in a quiz as often as necessary without starting a whole new attempt, if my teacher allows it. + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | student | Student | One | student@moodle.com | + | teacher | Teacher | One | teacher@moodle.com | + And the following "courses" exist: + | fullname | shortname | category | + | Course 1 | C1 | 0 | + And the following "course enrolments" exist: + | user | course | role | + | student | C1 | student | + | teacher | C1 | teacher | + And the following "question categories" exist: + | contextlevel | reference | name | + | Course | C1 | Test questions | + And the following "questions" exist: + | questioncategory | qtype | name | questiontext | + | Test questions | truefalse | TF1 | First question | + | Test questions | truefalse | TF2 | Second question | + And the following "activities" exist: + | activity | name | intro | course | idnumber | preferredbehaviour | canredoquestions | + | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | immediatefeedback | 1 | + And quiz "Quiz 1" contains the following questions: + | question | page | maxmark | + | TF1 | 1 | 2 | + | TF2 | 1 | 1 | + And I log in as "student" + And I follow "Course 1" + + @javascript + Scenario: After completing a question, there is a redo question button that restarts the question + When I follow "Quiz 1" + And I press "Attempt quiz now" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I press "Redo question" + Then the state of "First question" question is shown as "Not complete" + And I should see "Marked out of 2.00" in the "First question" "question" + + @javascript + Scenario: The redo question button is visible but disabled for teachers + When I follow "Quiz 1" + And I press "Attempt quiz now" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I log out + And I log in as "teacher" + And I follow "Course 1" + And I follow "Quiz 1" + And I follow "Attempts: 1" + And I follow "Review attempt" + Then the "Redo question" "button" should be disabled + + @javascript + Scenario: The redo question buttons are no longer visible after the attempt is submitted. + When I follow "Quiz 1" + And I press "Attempt quiz now" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I press "Next" + And I press "Submit all and finish" + And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue" + Then "Redo question" "button" should not exist + + @javascript + Scenario: Teachers reviewing can see all the qestions attempted in a slot + When I follow "Quiz 1" + And I press "Attempt quiz now" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I press "Redo question" + And I press "Next" + And I press "Submit all and finish" + And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue" + And I log out + And I log in as "teacher" + And I follow "Course 1" + And I follow "Quiz 1" + And I follow "Attempts: 1" + And I follow "Review attempt" + And I click on "1" "link" in the "First question" "question" + And I switch to "reviewquestion" window + Then the state of "First question" question is shown as "Incorrect" + And I click on "1" "link" in the "First question" "question" + And the state of "First question" question is shown as "Not complete" + And I switch to the main window + And the state of "First question" question is shown as "Not answered" + And I should not see "Submit" in the ".history" "css_element" + And I navigate to "Statistics" node in "Quiz administration > Results" + And I follow "TF1" + And "False" row "Frequency" column of "quizresponseanalysis" table should contain "100.00%" + And "True" row "Frequency" column of "quizresponseanalysis" table should contain "0.00%" + And "[No response]" row "Frequency" column of "quizresponseanalysis" table should contain "100.00%" + + @javascript + Scenario: Redoing question 1 should save any changes to question 2 on the same page + When I follow "Quiz 1" + And I press "Attempt quiz now" + And I click on "False" "radio" in the "First question" "question" + And I click on "Check" "button" in the "First question" "question" + And I click on "True" "radio" in the "Second question" "question" + And I press "Redo question" + And I click on "Check" "button" in the "Second question" "question" + Then the state of "Second question" question is shown as "Correct" diff --git a/mod/quiz/tests/behat/reattemptquestions.feature b/mod/quiz/tests/behat/reattemptquestions.feature deleted file mode 100644 index 0943cc6b3fd..00000000000 --- a/mod/quiz/tests/behat/reattemptquestions.feature +++ /dev/null @@ -1,53 +0,0 @@ -@mod @mod_quiz -Feature: Add a quiz - In order to allow students re-attempting graded question - As a teacher - I need to create a quiz, set 'Restart question' field to 'Yes', add questions to the quiz which can be graded automatically. - - Background: - Given the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | T1 | Teacher1 | teacher1@moodle.com | - | student1 | S1 | Student1 | student1@moodle.com | - And the following "courses" exist: - | fullname | shortname | category | - | Course 1 | C1 | 0 | - And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | - | student1 | C1 | student | - When I log in as "teacher1" - And I follow "Course 1" - And I turn editing mode on - - And I add a "Quiz" to section "1" and I fill the form with: - | Name | Quiz 1 | - | Description | Quiz 1 description | - | How questions behave | Immediate feedback | - | Restart graded questions | Yes | - - And I add a "True/False" question to the "Quiz 1" quiz with: - | Question name | TF001 | - | Question text | Answer question TF001 | - | General feedback | Thank you, this is the general feedback | - | Correct answer | False | - | Feedback for the response 'True'. | So you think it is true | - | Feedback for the response 'False'. | So you think it is false | - And I log out - - @javascript - Scenario: Log in as a student, attempt the quiz and checking whether you can re-attempt a graded question in the appropriate behaviour settings - And I log in as "student1" - And I follow "Course 1" - And I follow "Quiz 1" - And I press "Attempt quiz now" - Then I should see "TF001" - And I should see "Answer question TF001" - And I set the field "True" to "1" - And I press "Check" - And I should see "Incorrect" - Then I press "Restart question" - And I should see "Not complete" - And I set the field "False" to "1" - And I press "Check" - And I should see "Correct" diff --git a/mod/quiz/upgrade.txt b/mod/quiz/upgrade.txt index 5db9d21c156..57842803821 100644 --- a/mod/quiz/upgrade.txt +++ b/mod/quiz/upgrade.txt @@ -11,6 +11,16 @@ This files describes API changes in the quiz code. + initialise_editing_javascript has had some redundant arguments removed. Hopefully, with these changes, we will have less need to make other changes in future. +* Due to MDL-40992, you should be aware that extra slots can get added to an attempt. + You may get slot numbers beyone the end of the original quiz layout, and you + may want to call $attemptobj->get_original_slot to find where the question + originally came from. + +* You now need to pass an instance of the mod_quiz_renderer if you call + $attemptobj->render_question or $attemptobj->render_question_at_step. + +* The array values in mod_quiz_links_to_other_attempts may now be either a moodle_url, + or renderable (or null). Previously they could only be a moodle_url or null. === 2.8 === diff --git a/question/behaviour/informationitem/renderer.php b/question/behaviour/informationitem/renderer.php index 29c5f8c0829..99a78529868 100644 --- a/question/behaviour/informationitem/renderer.php +++ b/question/behaviour/informationitem/renderer.php @@ -36,7 +36,7 @@ defined('MOODLE_INTERNAL') || die(); */ class qbehaviour_informationitem_renderer extends qbehaviour_renderer { public function controls(question_attempt $qa, question_display_options $options) { - if ($qa->get_state() != question_state::$todo) { + if ($options->readonly || $qa->get_state() != question_state::$todo) { return ''; } diff --git a/question/engine/lib.php b/question/engine/lib.php index 03d6dae8a0c..516e3204e0b 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -591,6 +591,21 @@ class question_display_options { */ public $history = self::HIDDEN; + /** + * @since 2.9 + * @var string extra HTML to include in the info box of the question display. + * This is normally shown after the information about the question, and before + * any controls like the flag or the edit icon. + */ + public $extrainfocontent = ''; + + /** + * @since 2.9 + * @var string extra HTML to include in the history box of the question display, + * if it is shown. + */ + public $extrahistorycontent = ''; + /** * If not empty, then a link to edit the question will be included in * the info box for the question. diff --git a/question/engine/renderer.php b/question/engine/renderer.php index 4d781e52c7d..ce87306ef05 100644 --- a/question/engine/renderer.php +++ b/question/engine/renderer.php @@ -144,6 +144,7 @@ class core_question_renderer extends plugin_renderer_base { $output .= $this->number($number); $output .= $this->status($qa, $behaviouroutput, $options); $output .= $this->mark_summary($qa, $behaviouroutput, $options); + $output .= $options->extrainfocontent; $output .= $this->question_flag($qa, $options->flags); $output .= $this->edit_question_link($qa, $options); return $output; @@ -485,8 +486,10 @@ class core_question_renderer extends plugin_renderer_base { } return html_writer::tag('h4', get_string('responsehistory', 'question'), - array('class' => 'responsehistoryheader')) . html_writer::tag('div', - html_writer::table($table, true), array('class' => 'responsehistoryheader')); + array('class' => 'responsehistoryheader')) . + $options->extrahistorycontent . + html_writer::tag('div', html_writer::table($table, true), + array('class' => 'responsehistoryheader')); } } diff --git a/question/engine/upgrade.txt b/question/engine/upgrade.txt index 07cc42376ff..86f1529cd41 100644 --- a/question/engine/upgrade.txt +++ b/question/engine/upgrade.txt @@ -29,6 +29,11 @@ This files describes API changes for the core question engine. To see examples of where these are used, look at the chagnes from MDL-40992. +2) New fields in question_display_options, ->extrainfocontent and ->extrahistorycontent. + These default to blank, but can be used to inject extra content into those parts + of the question display. If you have overridden the methods in + core_question_renderer that use these fields, you may need to update your renderer. + === 2.6 ===