From 9f3a76cb07ef1137d151af9227e78fa0a72a68cf Mon Sep 17 00:00:00 2001 From: Nelson Moller Date: Thu, 13 Aug 2015 14:13:41 -0400 Subject: [PATCH] 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 10f7c99d2fb76..43ad088795b9e 100644 --- a/lang/en/question.php +++ b/lang/en/question.php @@ -195,6 +195,7 @@ $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 0000000000000..7816bd14af40a --- /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 0397da0279760..f8a1d4812d892 100644 --- a/question/behaviour/rendererbase.php +++ b/question/behaviour/rendererbase.php @@ -68,6 +68,8 @@ public function feedback(question_attempt $qa, question_display_options $options 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 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt '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 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt '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 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt 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 516e3204e0b54..180bee588d687 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -131,6 +131,23 @@ public static function set_max_mark_in_attempts(qubaid_condition $qubaids, $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 @@ public static function set_max_mark_in_attempts(qubaid_condition $qubaids, */ 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 bff43e8d53584..ba3bae61ebc1d 100644 --- a/question/engine/questionattempt.php +++ b/question/engine/questionattempt.php @@ -1054,6 +1054,42 @@ public function get_submitted_var($name, $type, $postdata = null) { } } + + 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.