From 58d7de73ac9640bbadb896edc0997235b5a15ebd Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Mon, 21 Sep 2015 15:12:10 +0100 Subject: [PATCH] MDL-51476 questions: manual grading validation for ungraded Qs The change in MDL-51090 broke manually commenting on questions for which no mark is given (max mark == 0). This fixes it, with unit tests. Thanks to Nick Phillips for the original suggestion of the fix. --- .../manualgraded/tests/walkthrough_test.php | 57 +++++++++++++++++++ question/engine/lib.php | 3 +- question/engine/tests/questionengine_test.php | 36 +++++++++++- 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/question/behaviour/manualgraded/tests/walkthrough_test.php b/question/behaviour/manualgraded/tests/walkthrough_test.php index 7eeb44b81a1..d7abd66ec95 100644 --- a/question/behaviour/manualgraded/tests/walkthrough_test.php +++ b/question/behaviour/manualgraded/tests/walkthrough_test.php @@ -204,6 +204,63 @@ class qbehaviour_manualgraded_walkthrough_testcase extends qbehaviour_walkthroug new question_pattern_expectation('/' . preg_quote('Not good enough!', '/') . '/')); } + public function test_manual_grade_ungraded_question() { + 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', 0); + + // 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)); + + // Process a manual comment. Note: null mark is the whole point here. + $this->manual_grade('Not good enough!', null, FORMAT_HTML); + + // Verify. + // I am pretty sure this next assertion is incorrect. We should change + // the question state to indicate that this quetion has now been commented + // on. However, that is tricky, because what if, after that, the mam mark + // for the qusetions is changed. So, for now, this assertion verifies + // the current behaviour. + $this->check_current_state(question_state::$needsgrading); + $this->check_current_mark(null); + $this->check_current_output( + new question_pattern_expectation('/' . preg_quote('Not good enough!', '/') . '/')); + } + public function test_manual_graded_ignore_repeat_sumbission() { // Create an essay question. $essay = test_question_maker::make_an_essay_question(); diff --git a/question/engine/lib.php b/question/engine/lib.php index e2164cfc928..7d1449c1125 100644 --- a/question/engine/lib.php +++ b/question/engine/lib.php @@ -144,7 +144,8 @@ abstract class question_engine { $minfraction = optional_param($prefix . ':minfraction', null, PARAM_FLOAT); $maxfraction = optional_param($prefix . ':maxfraction', null, PARAM_FLOAT); return $mark === '' || - ($mark !== null && $mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark); + ($mark !== null && $mark >= $minfraction * $maxmark && $mark <= $maxfraction * $maxmark) || + ($mark === null && $maxmark === null); } /** diff --git a/question/engine/tests/questionengine_test.php b/question/engine/tests/questionengine_test.php index 1a5505d9aba..e119cdf34e5 100644 --- a/question/engine/tests/questionengine_test.php +++ b/question/engine/tests/questionengine_test.php @@ -91,4 +91,38 @@ class question_engine_test extends advanced_testcase { // Ignore unknown input in the disabled argument. $this->assertSame($in, question_engine::sort_behaviours($in, '', 'unknown', '')); } -} \ No newline at end of file + + public function test_is_manual_grade_in_range() { + $_POST[] = array('q1:2_-mark' => 0.5, 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => 0, 'q1:2_:maxfraction' => 1); + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } + + public function test_is_manual_grade_in_range_bottom_end() { + $_POST[] = array('q1:2_-mark' => -1.0, 'q1:2_-maxmark' => 2.0, + 'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1); + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } + + public function test_is_manual_grade_in_range_too_low() { + $_POST[] = array('q1:2_-mark' => -1.1, 'q1:2_-maxmark' => 2.0, + 'q1:2_:minfraction' => -0.5, 'q1:2_:maxfraction' => 1); + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } + + public function test_is_manual_grade_in_range_top_end() { + $_POST[] = array('q1:2_-mark' => 3.0, 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0); + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } + + public function test_is_manual_grade_in_range_too_high() { + $_POST[] = array('q1:2_-mark' => 3.1, 'q1:2_-maxmark' => 1.0, + 'q1:2_:minfraction' => -6.0, 'q1:2_:maxfraction' => 3.0); + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } + + public function test_is_manual_grade_in_range_ungraded() { + $this->assertTrue(question_engine::is_manual_grade_in_range(1, 2)); + } +}