From 52b08f5fce1d5980a718219584d5bcd810625ee3 Mon Sep 17 00:00:00 2001 From: Nelson Moller Date: Thu, 13 Aug 2015 14:13:41 -0400 Subject: [PATCH 1/2] MDL-51090: mod_quiz grading validation of an essay question An invalid format is casted to 0 (if a string) or to some truncated value in other cases (ex: 10..5). --- lang/en/question.php | 1 + ...alidate_manual_mark_correct_format.feature | 59 +++++++++++++++++++ question/behaviour/rendererbase.php | 18 ++++-- question/engine/lib.php | 20 +++++++ question/engine/questionattempt.php | 36 +++++++++++ 5 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 mod/quiz/tests/behat/validate_manual_mark_correct_format.feature diff --git a/lang/en/question.php b/lang/en/question.php index 126fb084406..2ce0f2c175d 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -195,6 +195,7 @@ $string['lasttry'] = 'Last try'; $string['linkedfiledoesntexist'] = 'Linked file {$a} doesn\'t exist'; $string['makechildof'] = 'Make child of \'{$a}\''; $string['maketoplevelitem'] = 'Move to top level'; +$string['manualgradeinvalidformat'] = 'That is not a valid number.'; $string['matchgrades'] = 'Match grades'; $string['matchgradeserror'] = 'Error if grade not listed'; $string['matchgradesnearest'] = 'Nearest grade if not listed'; diff --git a/mod/quiz/tests/behat/validate_manual_mark_correct_format.feature b/mod/quiz/tests/behat/validate_manual_mark_correct_format.feature new file mode 100644 index 00000000000..7816bd14af4 --- /dev/null +++ b/mod/quiz/tests/behat/validate_manual_mark_correct_format.feature @@ -0,0 +1,59 @@ +@mod @mod_quiz +Feature: In order to mannually mark a question I want + As a teacher + I must be able to set the marks I want on the Rapport question page. + + Background: + Given the following "users" exist: + | username | firstname | lastname | email | + | teacher1 | Teacher | 1 | teacher1@example.com | + | student1 | Student | 1 | student0@example.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 | + And the following "question categories" exist: + | contextlevel | reference | name | + | Course | C1 | Test questions | + Given the following "questions" exist: + | questioncategory | qtype | name | questiontext | defaultmark | + | Test questions | essay | TF1 | First question | 20 | + And the following "activities" exist: + | activity | name | intro | course | idnumber | grade | + | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | 20 | + And quiz "Quiz 1" contains the following questions: + | question | page | requireprevious | + | TF1 | 1 | 0 | + And I log in as "student1" + And I follow "Course 1" + And I follow "Quiz 1" + And I press "Attempt quiz now" + And I follow "Finish attempt ..." + And I press "Submit all and finish" + # Pop-up confirmation + And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue" + And I log out + And I log in as "teacher1" + And I follow "Course 1" + And I follow "Quiz 1" + And I follow "Attempts: 1" + And I follow "Review attempt" + + @javascript + Scenario: Validating the marking of an essay question attempt. + When I follow "Make comment or override mark" + And I switch to "commentquestion" window + And I set the field "Mark" to "25" + And I press "Save" + Then I should see "This grade is outside the valid range." + And I set the field "Mark" to "aa" + And I press "Save" + Then I should see "That is not a valid number." + And I set the field "Mark" to "10" + And I press "Save" + Then I should see "Changes saved" + + diff --git a/question/behaviour/rendererbase.php b/question/behaviour/rendererbase.php index 0397da02797..f8a1d4812d8 100644 --- a/question/behaviour/rendererbase.php +++ b/question/behaviour/rendererbase.php @@ -68,6 +68,8 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { return ''; } + + public function manual_comment_fields(question_attempt $qa, question_display_options $options) { $inputname = $qa->get_behaviour_field_name('comment'); $id = $inputname . '_id'; @@ -124,10 +126,15 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { 'name' => $markfield, 'id'=> $markfield ); - if (!is_null($currentmark)) { + if (!is_null($currentmark) && $qa->manual_mark_format_is_ok() ) { $attributes['value'] = $qa->format_fraction_as_mark( $currentmark / $maxmark, $options->markdp); } + // We want that the wrong entry is shown. + else if (!is_null($currentmark)){ + $attributes['value'] = $qa->get_submitted_var( + $qa->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED); + } $a = new stdClass(); $a->max = $qa->format_max_mark($options->markdp); $a->mark = html_writer::empty_tag('input', $attributes); @@ -146,12 +153,10 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { 'value' => $qa->get_max_fraction(), )); + $error = $qa->get_validation_message(); $errorclass = ''; - $error = ''; - if ($currentmark > $maxmark * $qa->get_max_fraction() || $currentmark < $maxmark * $qa->get_min_fraction()) { - $errorclass = ' error'; - $error = html_writer::tag('span', get_string('manualgradeoutofrange', 'question'), - array('class' => 'error')) . html_writer::empty_tag('br'); + if ($error !== ''){ + $erroclass = 'error'; } $mark = html_writer::tag('div', html_writer::tag('div', @@ -167,6 +172,7 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { array('class' => 'fcontainer clearfix')), array('class' => 'hidden')); } + public function manual_comment_view(question_attempt $qa, question_display_options $options) { $output = ''; if ($qa->has_manual_comment()) { diff --git a/question/engine/lib.php b/question/engine/lib.php index a33d4bc70ee..c4095151383 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -131,6 +131,23 @@ abstract class question_engine { $dm->set_max_mark_in_attempts($qubaids, $slot, $newmaxmark); } + /** + * MDL-51090 + * Validate that the manual grade submitted for a particular question is a + * float. + * + * @param string prefix as constructed in is_manual_grade_in_range. + * @return bool whether the submitted data is a float. + */ + public static function is_manual_grade_float($prefix) { + $val = optional_param($prefix . '-mark', null, PARAM_RAW_TRIMMED); + $mark = unformat_float($val, true); + if (is_float($mark)) { + return true; + } + return false; + } + /** * Validate that the manual grade submitted for a particular question is in range. * @param int $qubaid the question_usage id. @@ -139,6 +156,9 @@ abstract class question_engine { */ public static function is_manual_grade_in_range($qubaid, $slot) { $prefix = 'q' . $qubaid . ':' . $slot . '_'; + if (!self::is_manual_grade_float($prefix)) { + return false; + } $mark = question_utils::optional_param_mark($prefix . '-mark'); $maxmark = optional_param($prefix . '-maxmark', null, PARAM_FLOAT); $minfraction = optional_param($prefix . ':minfraction', null, PARAM_FLOAT); diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index bff43e8d535..ba3bae61ebc 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1054,6 +1054,42 @@ class question_attempt { } } + + public function get_validation_message() { + + if (!$this->manual_mark_format_is_ok()) { + $errorclass = ' error'; + return html_writer::tag('span', get_string('manualgradeinvalidformat', 'question'), + array('class' => 'error')) . html_writer::empty_tag('br'); + } + $currentmark = $this->get_current_manual_mark(); + $maxmark = $this->get_max_mark(); + if ($currentmark > $maxmark * $this->get_max_fraction() || $currentmark < $maxmark * $this->get_min_fraction()) { + $errorclass = ' error'; + return html_writer::tag('span', get_string('manualgradeoutofrange', 'question'), + array('class' => 'error')) . html_writer::empty_tag('br'); + } + + return ''; + + } + + /** + * Validates that the manual mark parameter is a float. + * @return bool. + */ + public function manual_mark_format_is_ok() { + $val = $this->get_submitted_var($this->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED); + + // If it is empy, we get null (it is always the case on start) + if ($val===null) { + return true; + } + $mark = unformat_float($val, true); + + return is_float($mark); + } + /** * Handle a submitted variable representing uploaded files. * @param string $name the field name. From b2694c0219e81bba2c04c25e7300a497749a6927 Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Thu, 20 Aug 2015 21:17:24 +0100 Subject: [PATCH 2/2] MDL-51090 question: further refinements to validating manual grades --- ...feature => manually_mark_question.feature} | 20 ++-- question/behaviour/behaviourbase.php | 26 +++-- .../manualgraded/tests/walkthrough_test.php | 94 +++++++++++++++++++ question/behaviour/rendererbase.php | 21 ++--- question/engine/lib.php | 36 +++---- question/engine/questionattempt.php | 61 +++++------- .../engine/tests/questionattempt_test.php | 30 ------ .../tests/questionattempt_with_steps_test.php | 16 ++++ question/engine/tests/questionutils_test.php | 1 + question/engine/upgrade.txt | 6 ++ 10 files changed, 185 insertions(+), 126 deletions(-) rename mod/quiz/tests/behat/{validate_manual_mark_correct_format.feature => manually_mark_question.feature} (82%) diff --git a/mod/quiz/tests/behat/validate_manual_mark_correct_format.feature b/mod/quiz/tests/behat/manually_mark_question.feature similarity index 82% rename from mod/quiz/tests/behat/validate_manual_mark_correct_format.feature rename to mod/quiz/tests/behat/manually_mark_question.feature index 7816bd14af4..dba965283aa 100644 --- a/mod/quiz/tests/behat/validate_manual_mark_correct_format.feature +++ b/mod/quiz/tests/behat/manually_mark_question.feature @@ -1,7 +1,8 @@ @mod @mod_quiz -Feature: In order to mannually mark a question I want +Feature: Teachers can override the grade for any question As a teacher - I must be able to set the marks I want on the Rapport question page. + In order to correct errors + I must be able to override the grades that Moodle gives. Background: Given the following "users" exist: @@ -18,22 +19,21 @@ Feature: In order to mannually mark a question I want And the following "question categories" exist: | contextlevel | reference | name | | Course | C1 | Test questions | - Given the following "questions" exist: + And the following "questions" exist: | questioncategory | qtype | name | questiontext | defaultmark | | Test questions | essay | TF1 | First question | 20 | And the following "activities" exist: | activity | name | intro | course | idnumber | grade | | quiz | Quiz 1 | Quiz 1 description | C1 | quiz1 | 20 | And quiz "Quiz 1" contains the following questions: - | question | page | requireprevious | - | TF1 | 1 | 0 | + | question | page | + | TF1 | 1 | And I log in as "student1" And I follow "Course 1" And I follow "Quiz 1" And I press "Attempt quiz now" And I follow "Finish attempt ..." And I press "Submit all and finish" - # Pop-up confirmation And I click on "Submit all and finish" "button" in the "Confirmation" "dialogue" And I log out And I log in as "teacher1" @@ -51,9 +51,7 @@ Feature: In order to mannually mark a question I want Then I should see "This grade is outside the valid range." And I set the field "Mark" to "aa" And I press "Save" - Then I should see "That is not a valid number." - And I set the field "Mark" to "10" + And I should see "That is not a valid number." + And I set the field "Mark" to "10.0" And I press "Save" - Then I should see "Changes saved" - - + And I should see "Changes saved" diff --git a/question/behaviour/behaviourbase.php b/question/behaviour/behaviourbase.php index 8daaab0a200..d73328dbb49 100644 --- a/question/behaviour/behaviourbase.php +++ b/question/behaviour/behaviourbase.php @@ -204,7 +204,7 @@ abstract class question_behaviour { $vars = array('comment' => PARAM_RAW, 'commentformat' => PARAM_INT); if ($this->qa->get_max_mark()) { - $vars['mark'] = question_attempt::PARAM_MARK; + $vars['mark'] = PARAM_RAW_TRIMMED; $vars['maxmark'] = PARAM_FLOAT; } return $vars; @@ -477,15 +477,25 @@ abstract class question_behaviour { } if ($pendingstep->has_behaviour_var('mark')) { - $fraction = $pendingstep->get_behaviour_var('mark') / - $pendingstep->get_behaviour_var('maxmark'); - if ($pendingstep->get_behaviour_var('mark') === '') { + $mark = question_utils::clean_param_mark($pendingstep->get_behaviour_var('mark')); + if ($mark === null) { + throw new coding_exception('Inalid number format ' . $pendingstep->get_behaviour_var('mark') . + ' when processing a manual grading action.', 'Question ' . $this->question->id . + ', slot ' . $this->qa->get_slot()); + + } else if ($mark === '') { $fraction = null; - } else if ($fraction > $this->qa->get_max_fraction() || $fraction < $this->qa->get_min_fraction()) { - throw new coding_exception('Score out of range when processing ' . - 'a manual grading action.', 'Question ' . $this->question->id . - ', slot ' . $this->qa->get_slot() . ', fraction ' . $fraction); + + } else { + $fraction = $pendingstep->get_behaviour_var('mark') / + $pendingstep->get_behaviour_var('maxmark'); + if ($fraction > $this->qa->get_max_fraction() || $fraction < $this->qa->get_min_fraction()) { + throw new coding_exception('Score out of range when processing ' . + 'a manual grading action.', 'Question ' . $this->question->id . + ', slot ' . $this->qa->get_slot() . ', fraction ' . $fraction); + } } + $pendingstep->set_fraction($fraction); } diff --git a/question/behaviour/manualgraded/tests/walkthrough_test.php b/question/behaviour/manualgraded/tests/walkthrough_test.php index 0e2aec82a32..95660479a1d 100644 --- a/question/behaviour/manualgraded/tests/walkthrough_test.php +++ b/question/behaviour/manualgraded/tests/walkthrough_test.php @@ -384,4 +384,98 @@ class qbehaviour_manualgraded_walkthrough_testcase extends qbehaviour_walkthroug $this->displayoptions->manualcomment = question_display_options::VISIBLE; $this->check_output_contains('This should only appear if the displya options allow it'); } + + public function test_manual_graded_invalid_value_throws_exception() { + global $PAGE; + + // The current text editor depends on the users profile setting - so it needs a valid user. + $this->setAdminUser(); + // Required to init a text editor. + $PAGE->set_url('/'); + + // Create an essay question. + $essay = test_question_maker::make_an_essay_question(); + $this->start_attempt_at_question($essay, 'deferredfeedback', 10); + + // Check the right model is being used. + $this->assertEquals('manualgraded', $this->quba->get_question_attempt( + $this->slot)->get_behaviour_name()); + + // Check the initial state. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_current_output($this->get_contains_question_text_expectation($essay), + $this->get_does_not_contain_feedback_expectation()); + + // Simulate some data submitted by the student. + $this->process_submission(array('answer' => 'This is my wonderful essay!', 'answerformat' => FORMAT_HTML)); + + // Verify. + $this->check_current_state(question_state::$complete); + $this->check_current_mark(null); + $this->check_current_output( + new question_contains_tag_with_attribute('textarea', 'name', + $this->quba->get_question_attempt($this->slot)->get_qt_field_name('answer')), + $this->get_does_not_contain_feedback_expectation()); + + // Finish the attempt. + $this->quba->finish_all_questions(); + + // Verify. + $this->check_current_state(question_state::$needsgrading); + $this->check_current_mark(null); + $this->assertEquals('This is my wonderful essay!', + $this->quba->get_response_summary($this->slot)); + + // Try to process a an invalid grade. + $this->setExpectedException('coding_exception'); + $this->manual_grade('Comment', 'frog', FORMAT_HTML); + } + + public function test_manual_graded_out_of_range_throws_exception() { + global $PAGE; + + // The current text editor depends on the users profile setting - so it needs a valid user. + $this->setAdminUser(); + // Required to init a text editor. + $PAGE->set_url('/'); + + // Create an essay question. + $essay = test_question_maker::make_an_essay_question(); + $this->start_attempt_at_question($essay, 'deferredfeedback', 10); + + // Check the right model is being used. + $this->assertEquals('manualgraded', $this->quba->get_question_attempt( + $this->slot)->get_behaviour_name()); + + // Check the initial state. + $this->check_current_state(question_state::$todo); + $this->check_current_mark(null); + $this->check_current_output($this->get_contains_question_text_expectation($essay), + $this->get_does_not_contain_feedback_expectation()); + + // Simulate some data submitted by the student. + $this->process_submission(array('answer' => 'This is my wonderful essay!', 'answerformat' => FORMAT_HTML)); + + // Verify. + $this->check_current_state(question_state::$complete); + $this->check_current_mark(null); + $this->check_current_output( + new question_contains_tag_with_attribute('textarea', 'name', + $this->quba->get_question_attempt($this->slot)->get_qt_field_name('answer')), + $this->get_does_not_contain_feedback_expectation()); + + // Finish the attempt. + $this->quba->finish_all_questions(); + + // Verify. + $this->check_current_state(question_state::$needsgrading); + $this->check_current_mark(null); + $this->assertEquals('This is my wonderful essay!', + $this->quba->get_response_summary($this->slot)); + + // Try to process a an invalid grade. + $this->setExpectedException('coding_exception'); + $this->manual_grade('Comment', '10.1', FORMAT_HTML); + } } diff --git a/question/behaviour/rendererbase.php b/question/behaviour/rendererbase.php index f8a1d4812d8..c6c69bad13d 100644 --- a/question/behaviour/rendererbase.php +++ b/question/behaviour/rendererbase.php @@ -68,8 +68,6 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { return ''; } - - public function manual_comment_fields(question_attempt $qa, question_display_options $options) { $inputname = $qa->get_behaviour_field_name('comment'); $id = $inputname . '_id'; @@ -126,14 +124,8 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { 'name' => $markfield, 'id'=> $markfield ); - if (!is_null($currentmark) && $qa->manual_mark_format_is_ok() ) { - $attributes['value'] = $qa->format_fraction_as_mark( - $currentmark / $maxmark, $options->markdp); - } - // We want that the wrong entry is shown. - else if (!is_null($currentmark)){ - $attributes['value'] = $qa->get_submitted_var( - $qa->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED); + if (!is_null($currentmark)) { + $attributes['value'] = $currentmark; } $a = new stdClass(); $a->max = $qa->format_max_mark($options->markdp); @@ -153,10 +145,12 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { 'value' => $qa->get_max_fraction(), )); - $error = $qa->get_validation_message(); + $error = $qa->validate_manual_mark($currentmark); $errorclass = ''; - if ($error !== ''){ - $erroclass = 'error'; + if ($error !== '') { + $erroclass = ' error'; + $error = html_writer::tag('span', $error, + array('class' => 'error')) . html_writer::empty_tag('br'); } $mark = html_writer::tag('div', html_writer::tag('div', @@ -172,7 +166,6 @@ abstract class qbehaviour_renderer extends plugin_renderer_base { array('class' => 'fcontainer clearfix')), array('class' => 'hidden')); } - public function manual_comment_view(question_attempt $qa, question_display_options $options) { $output = ''; if ($qa->has_manual_comment()) { diff --git a/question/engine/lib.php b/question/engine/lib.php index c4095151383..45c4a7fdfb1 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -131,23 +131,6 @@ abstract class question_engine { $dm->set_max_mark_in_attempts($qubaids, $slot, $newmaxmark); } - /** - * MDL-51090 - * Validate that the manual grade submitted for a particular question is a - * float. - * - * @param string prefix as constructed in is_manual_grade_in_range. - * @return bool whether the submitted data is a float. - */ - public static function is_manual_grade_float($prefix) { - $val = optional_param($prefix . '-mark', null, PARAM_RAW_TRIMMED); - $mark = unformat_float($val, true); - if (is_float($mark)) { - return true; - } - return false; - } - /** * Validate that the manual grade submitted for a particular question is in range. * @param int $qubaid the question_usage id. @@ -156,14 +139,12 @@ abstract class question_engine { */ public static function is_manual_grade_in_range($qubaid, $slot) { $prefix = 'q' . $qubaid . ':' . $slot . '_'; - if (!self::is_manual_grade_float($prefix)) { - return false; - } $mark = question_utils::optional_param_mark($prefix . '-mark'); $maxmark = optional_param($prefix . '-maxmark', null, PARAM_FLOAT); $minfraction = optional_param($prefix . ':minfraction', null, PARAM_FLOAT); $maxfraction = optional_param($prefix . ':maxfraction', null, PARAM_FLOAT); - return is_null($mark) || ($mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark); + return $mark === '' || + ($mark !== null && $mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark); } /** @@ -924,8 +905,9 @@ abstract class question_utils { /** * Typically, $mark will have come from optional_param($name, null, PARAM_RAW_TRIMMED). * This method copes with: - * - keeping null or '' input unchanged. - * - nubmers that were typed as either 1.00 or 1,00 form. + * - keeping null or '' input unchanged - important to let teaches set a question back to requries grading. + * - numbers that were typed as either 1.00 or 1,00 form. + * - invalid things, which get turned into null. * * @param string|null $mark raw use input of a mark. * @return float|string|null cleaned mark as a float if possible. Otherwise '' or null. @@ -935,7 +917,13 @@ abstract class question_utils { return $mark; } - return clean_param(str_replace(',', '.', $mark), PARAM_FLOAT); + $mark = str_replace(',', '.', $mark); + // This regexp should match the one in validate_param. + if (!preg_match('/^[\+-]?[0-9]*\.?[0-9]*(e[-+]?[0-9]+)?$/i', $mark)) { + return null; + } + + return clean_param($mark, PARAM_FLOAT); } /** diff --git a/question/engine/questionattempt.php b/question/engine/questionattempt.php index ba3bae61ebc..bd252801cff 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -49,10 +49,10 @@ class question_attempt { const USE_RAW_DATA = 'use raw data'; /** - * @var string special value used by manual grading because {@link PARAM_FLOAT} - * converts '' to 0. + * @var string Should not longer be used. + * @deprecated since Moodle 3.0 */ - const PARAM_MARK = 'parammark'; + const PARAM_MARK = PARAM_RAW_TRIMMED; /** * @var string special value to indicate a response variable that is uploaded @@ -651,13 +651,12 @@ class question_attempt { * This is used by the manual grading code, particularly in association with * validation. If there is a mark submitted in the request, then use that, * otherwise use the latest mark for this question. - * @return number the current mark for this question. - * {@link get_fraction()} * {@link get_max_mark()}. + * @return number the current manual mark for this question, formatted for display. */ public function get_current_manual_mark() { - $mark = $this->get_submitted_var($this->get_behaviour_field_name('mark'), question_attempt::PARAM_MARK); + $mark = $this->get_submitted_var($this->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED); if (is_null($mark)) { - return $this->get_mark(); + return format_float($this->get_mark(), 7, true, true); } else { return $mark; } @@ -1030,9 +1029,6 @@ class question_attempt { */ public function get_submitted_var($name, $type, $postdata = null) { switch ($type) { - case self::PARAM_MARK: - // Special case to work around PARAM_FLOAT converting '' to 0. - return question_utils::clean_param_mark($this->get_submitted_var($name, PARAM_RAW_TRIMMED, $postdata)); case self::PARAM_FILES: return $this->process_response_files($name, $name, $postdata); @@ -1054,40 +1050,27 @@ class question_attempt { } } - - public function get_validation_message() { - - if (!$this->manual_mark_format_is_ok()) { - $errorclass = ' error'; - return html_writer::tag('span', get_string('manualgradeinvalidformat', 'question'), - array('class' => 'error')) . html_writer::empty_tag('br'); + /** + * Validate the manual mark for a question. + * @param unknown $currentmark the user input (e.g. '1,0', '1,0' or 'invalid'. + * @return string any errors with the value, or '' if it is OK. + */ + public function validate_manual_mark($currentmark) { + if ($currentmark === null || $currentmark === '') { + return ''; } - $currentmark = $this->get_current_manual_mark(); + + $mark = question_utils::clean_param_mark($currentmark); + if ($mark === null) { + return get_string('manualgradeinvalidformat', 'question'); + } + $maxmark = $this->get_max_mark(); - if ($currentmark > $maxmark * $this->get_max_fraction() || $currentmark < $maxmark * $this->get_min_fraction()) { - $errorclass = ' error'; - return html_writer::tag('span', get_string('manualgradeoutofrange', 'question'), - array('class' => 'error')) . html_writer::empty_tag('br'); + if ($mark > $maxmark * $this->get_max_fraction() || $mark < $maxmark * $this->get_min_fraction()) { + return get_string('manualgradeoutofrange', 'question'); } return ''; - - } - - /** - * Validates that the manual mark parameter is a float. - * @return bool. - */ - public function manual_mark_format_is_ok() { - $val = $this->get_submitted_var($this->get_behaviour_field_name('mark'), PARAM_RAW_TRIMMED); - - // If it is empy, we get null (it is always the case on start) - if ($val===null) { - return true; - } - $mark = unformat_float($val, true); - - return is_float($mark); } /** diff --git a/question/engine/tests/questionattempt_test.php b/question/engine/tests/questionattempt_test.php index 21f6ad9fd7e..3469ec6f067 100644 --- a/question/engine/tests/questionattempt_test.php +++ b/question/engine/tests/questionattempt_test.php @@ -109,36 +109,6 @@ class question_attempt_testcase extends advanced_testcase { 'reallyunlikelyvariablename', PARAM_BOOL)); } - public function test_get_submitted_var_param_mark_not_present() { - $this->assertNull($this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array())); - } - - public function test_get_submitted_var_param_mark_blank() { - $this->assertSame('', $this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array('name' => ''))); - } - - public function test_get_submitted_var_param_mark_number() { - $this->assertSame(123.0, $this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array('name' => '123'))); - } - - public function test_get_submitted_var_param_mark_number_uk_decimal() { - $this->assertSame(123.45, $this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array('name' => '123.45'))); - } - - public function test_get_submitted_var_param_mark_number_eu_decimal() { - $this->assertSame(123.45, $this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array('name' => '123,45'))); - } - - public function test_get_submitted_var_param_mark_invalid() { - $this->assertSame(0.0, $this->qa->get_submitted_var( - 'name', question_attempt::PARAM_MARK, array('name' => 'frog'))); - } - public function test_get_all_submitted_qt_vars() { $this->qa->set_usage_id('MDOgzdhS4W'); $this->qa->set_slot(1); diff --git a/question/engine/tests/questionattempt_with_steps_test.php b/question/engine/tests/questionattempt_with_steps_test.php index b5f1d4b729e..a5cddbd9313 100644 --- a/question/engine/tests/questionattempt_with_steps_test.php +++ b/question/engine/tests/questionattempt_with_steps_test.php @@ -165,4 +165,20 @@ class question_attempt_with_steps_test extends advanced_testcase { $this->setExpectedException('moodle_exception'); $qa->get_max_fraction(); } + + public function test_validate_manual_mark() { + $this->qa->set_min_fraction(0); + $this->qa->set_max_fraction(1); + $this->assertSame('', $this->qa->validate_manual_mark(null)); + $this->assertSame('', $this->qa->validate_manual_mark('')); + $this->assertSame('', $this->qa->validate_manual_mark('0')); + $this->assertSame('', $this->qa->validate_manual_mark('0.0')); + $this->assertSame('', $this->qa->validate_manual_mark('2,0')); + $this->assertSame(get_string('manualgradeinvalidformat', 'question'), + $this->qa->validate_manual_mark('frog')); + $this->assertSame(get_string('manualgradeoutofrange', 'question'), + $this->qa->validate_manual_mark('2.1')); + $this->assertSame(get_string('manualgradeoutofrange', 'question'), + $this->qa->validate_manual_mark('-0,01')); + } } diff --git a/question/engine/tests/questionutils_test.php b/question/engine/tests/questionutils_test.php index e5935f40424..ab4559c7e7e 100644 --- a/question/engine/tests/questionutils_test.php +++ b/question/engine/tests/questionutils_test.php @@ -194,6 +194,7 @@ class question_utils_test extends advanced_testcase { public function test_clean_param_mark() { $this->assertNull(question_utils::clean_param_mark(null)); + $this->assertNull(question_utils::clean_param_mark('frog')); $this->assertSame('', question_utils::clean_param_mark('')); $this->assertSame(0.0, question_utils::clean_param_mark('0')); $this->assertSame(1.5, question_utils::clean_param_mark('1.5')); diff --git a/question/engine/upgrade.txt b/question/engine/upgrade.txt index 86f1529cd41..09c9d790000 100644 --- a/question/engine/upgrade.txt +++ b/question/engine/upgrade.txt @@ -1,5 +1,11 @@ This files describes API changes for the core question engine. +=== 3.0, 2.9.2, 2.8.8 === + +1) The extra internal PARAM constant question_attempt::PARAM_MARK should no + longer be used. (It should not have been used outside the core of the + question system). See MDL-51090 if you want more explanation. + === 2.9 ===