Skip to content

Commit

Permalink
MDL-51090 question: further refinements to validating manual grades
Browse files Browse the repository at this point in the history
  • Loading branch information
@timhunt
timhunt committed Sep 1, 2015
1 parent df0c2e9 commit 5c97a8d
Show file tree
Hide file tree
Showing 10 changed files with 184 additions and 125 deletions.
20 changes: 9 additions & 11 deletions ...lidate_manual_mark_correct_format.feature → ...ests/behat/manually_mark_question.feature
View file
@@ -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:
Expand All @@ -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"
Expand All @@ -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"
26 changes: 18 additions & 8 deletions question/behaviour/behaviourbase.php
View file
Expand Up @@ -194,7 +194,7 @@ public function get_expected_data() {

$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;
Expand Down Expand Up @@ -467,15 +467,25 @@ public function process_comment(question_attempt_pending_step $pendingstep) {
}

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);
}

Expand Down
94 changes: 94 additions & 0 deletions question/behaviour/manualgraded/tests/walkthrough_test.php
View file
Expand Up @@ -384,4 +384,98 @@ public function test_manual_graded_respects_display_options() {
$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);
}
}
21 changes: 7 additions & 14 deletions question/behaviour/rendererbase.php
View file
Expand Up @@ -68,8 +68,6 @@ 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';
Expand Down Expand Up @@ -126,14 +124,8 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt
'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);
Expand All @@ -153,10 +145,12 @@ public function manual_comment_fields(question_attempt $qa, question_display_opt
'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',
Expand All @@ -172,7 +166,6 @@ 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()) {
Expand Down
36 changes: 12 additions & 24 deletions question/engine/lib.php
View file
Expand Up @@ -131,23 +131,6 @@ 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.
Expand All @@ -156,14 +139,12 @@ public static function is_manual_grade_float($prefix) {
*/
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);
}

/**
Expand Down Expand Up @@ -887,8 +868,9 @@ public static function int_to_roman($number) {
/**
* 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.
Expand All @@ -898,7 +880,13 @@ public static function clean_param_mark($mark) {
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);
}

/**
Expand Down

0 comments on commit 5c97a8d

Please sign in to comment.