From c351dbb13b28f7f326c247224be6982e67882707 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Dang Date: Mon, 30 Sep 2024 23:03:12 +0700 Subject: [PATCH 1/2] Matrix: incomplete answer list not marked as expected. #826739 --- .github/workflows/ci.yml | 2 +- question.php | 91 +++++++++++++++++++++----------- tests/question_multiple_test.php | 9 ++-- tests/question_single_test.php | 10 +++- 4 files changed, 76 insertions(+), 36 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c5c3909..8cb5341 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,7 +9,7 @@ jobs: matrix: include: - php: '8.3' - moodle-branch: 'master' + moodle-branch: 'main' database: 'mariadb' - php: '8.2' moodle-branch: 'MOODLE_404_STABLE' diff --git a/question.php b/question.php index b13589b..29162f2 100644 --- a/question.php +++ b/question.php @@ -120,8 +120,13 @@ protected function init_roworder(question_attempt $qa) { } #[\Override] - public function is_gradable_response(array $response) { - return $this->is_complete_response($response); + public function is_complete_response(array $response): bool { + return $this->validate_response_fields($response, true); + } + + #[\Override] + public function is_gradable_response(array $response): bool { + return $this->validate_response_fields($response, false); } #[\Override] @@ -181,6 +186,22 @@ public function has_specific_feedback(): bool { } return false; } + + /** + * Validates the presence of response fields based on the provided condition. + * This function checks whether the fields in `$response` match the required fields in `$this->roworder`. + * If `$requireall` is true, it ensures that all required fields are present. + * If `$requireall` is false, it checks if at least one required field is present. + * + * @param array $response The response array to be checked. + * @param bool $requireall Whether all fields must be present (true) or if at least one field is sufficient (false). + * @return bool Returns true if the response satisfies the required condition: + * - If `$requireall` is true, returns true if all fields in `$this->roworder` are present in the response. + * - If `$requireall` is false, returns true if at least one field is present in the response. + */ + protected function validate_response_fields(array $response, bool $requireall): bool { + return false; + } } /** @@ -264,17 +285,6 @@ public function summarise_response(array $response): ?string { return implode('; ', $responsewords); } - #[\Override] - public function is_complete_response(array $response): bool { - foreach ($this->roworder as $key => $rownumber) { - $fieldname = $this->field($key); - if (!array_key_exists($fieldname, $response)) { - return false; - } - } - return true; - } - #[\Override] public function classify_response(array $response): array { $classifiedresponse = []; @@ -342,6 +352,22 @@ public function get_num_parts_right(array $response): array { } return [$numright, count($this->rows)]; } + + #[\Override] + protected function validate_response_fields(array $response, bool $requireall): bool { + foreach ($this->roworder as $key => $rownumber) { + $fieldname = $this->field($key); + $answerexist = array_key_exists($fieldname, $response); + if ($requireall && !$answerexist) { + return false; + } + if (!$requireall && $answerexist) { + return true; + } + } + + return $requireall; + } } /** @@ -433,23 +459,6 @@ public function summarise_response(array $response): ?string { return implode('; ', $responsewords); } - #[\Override] - public function is_complete_response(array $response): bool { - foreach ($this->roworder as $key => $rownumber) { - $inputresponse = false; - foreach ($this->columns as $column) { - $fieldname = $this->field($key, $column->number); - if (array_key_exists($fieldname, $response)) { - $inputresponse = true; - } - } - if (!$inputresponse) { - return $inputresponse; - } - } - return true; - } - #[\Override] public function classify_response(array $response) { $classifiedresponse = []; @@ -597,4 +606,26 @@ public function get_num_correct_choices(): int { } return $numcorrect; } + + #[\Override] + protected function validate_response_fields(array $response, bool $requireall): bool { + foreach ($this->roworder as $key => $rownumber) { + $inputresponse = false; + foreach ($this->columns as $column) { + $fieldname = $this->field($key, $column->number); + if (array_key_exists($fieldname, $response)) { + $inputresponse = true; + } + } + if ($requireall && !$inputresponse) { + return false; + } + + if (!$requireall && $inputresponse) { + return true; + } + } + + return $requireall; + } } diff --git a/tests/question_multiple_test.php b/tests/question_multiple_test.php index 09a341f..476dd74 100644 --- a/tests/question_multiple_test.php +++ b/tests/question_multiple_test.php @@ -92,13 +92,16 @@ public function test_is_gradable_response(): void { // If all rows (sub-questions) are answered then the response is gradable. $response = ['rowanswers0_1' => '1']; - $this->assertFalse($question->is_gradable_response($response), $question->is_complete_response($response)); + $this->assertTrue($question->is_gradable_response($response)); $response = ['rowanswers0_1' => '1', 'rowanswers1_2' => '1']; - $this->assertFalse($question->is_gradable_response($response), $question->is_complete_response($response)); + $this->assertTrue($question->is_gradable_response($response)); $response = ['rowanswers0_1' => '1', 'rowanswers1_1' => '1', 'rowanswers2_1' => '1']; - $this->assertTrue($question->is_gradable_response($response), $question->is_complete_response($response)); + $this->assertTrue($question->is_gradable_response($response)); + + $response = []; + $this->assertFalse($question->is_gradable_response($response)); } public function test_classify_response_multiple(): void { diff --git a/tests/question_single_test.php b/tests/question_single_test.php index a97c3ae..e0d1835 100644 --- a/tests/question_single_test.php +++ b/tests/question_single_test.php @@ -56,10 +56,16 @@ public function test_is_gradable_response(): void { $question->start_attempt(new question_attempt_step(), 1); $response = ['rowanswers0' => '1', 'rowanswers1' => '1', 'rowanswers2' => '2', 'rowanswers3' => '2']; - $this->assertEquals($question->is_gradable_response($response), $question->is_complete_response($response)); + $this->assertTrue($question->is_gradable_response($response)); $response = ['rowanswers0' => '1', 'rowanswers1' => '1', 'rowanswers2' => '2']; - $this->assertEquals($question->is_gradable_response($response), $question->is_complete_response($response)); + $this->assertTrue($question->is_gradable_response($response)); + + $response = ['rowanswers0' => '1']; + $this->assertTrue($question->is_gradable_response($response)); + + $response = []; + $this->assertFalse($question->is_gradable_response($response)); } public function test_classify_response_single(): void { From 5435ea881180be73bc281b41bd338d099c730013 Mon Sep 17 00:00:00 2001 From: Khoa Nguyen Dang Date: Wed, 2 Oct 2024 17:32:11 +0700 Subject: [PATCH 2/2] Fix code review #826739 --- question.php | 117 +++++++++++++++++++++++---------------------------- 1 file changed, 53 insertions(+), 64 deletions(-) diff --git a/question.php b/question.php index 29162f2..691451f 100644 --- a/question.php +++ b/question.php @@ -119,16 +119,6 @@ protected function init_roworder(question_attempt $qa) { } } - #[\Override] - public function is_complete_response(array $response): bool { - return $this->validate_response_fields($response, true); - } - - #[\Override] - public function is_gradable_response(array $response): bool { - return $this->validate_response_fields($response, false); - } - #[\Override] public function check_file_access($qa, $options, $component, $filearea, $args, $forcedownload) { if ($component == 'question' && in_array($filearea, @@ -186,22 +176,6 @@ public function has_specific_feedback(): bool { } return false; } - - /** - * Validates the presence of response fields based on the provided condition. - * This function checks whether the fields in `$response` match the required fields in `$this->roworder`. - * If `$requireall` is true, it ensures that all required fields are present. - * If `$requireall` is false, it checks if at least one required field is present. - * - * @param array $response The response array to be checked. - * @param bool $requireall Whether all fields must be present (true) or if at least one field is sufficient (false). - * @return bool Returns true if the response satisfies the required condition: - * - If `$requireall` is true, returns true if all fields in `$this->roworder` are present in the response. - * - If `$requireall` is false, returns true if at least one field is present in the response. - */ - protected function validate_response_fields(array $response, bool $requireall): bool { - return false; - } } /** @@ -285,6 +259,29 @@ public function summarise_response(array $response): ?string { return implode('; ', $responsewords); } + #[\Override] + public function is_complete_response(array $response): bool { + foreach ($this->roworder as $key => $rownumber) { + $fieldname = $this->field($key); + if (!array_key_exists($fieldname, $response)) { + return false; + } + } + return true; + } + + #[\Override] + public function is_gradable_response(array $response): bool { + foreach ($this->roworder as $key => $rownumber) { + $fieldname = $this->field($key); + if (array_key_exists($fieldname, $response)) { + return true; + } + } + + return false; + } + #[\Override] public function classify_response(array $response): array { $classifiedresponse = []; @@ -352,22 +349,6 @@ public function get_num_parts_right(array $response): array { } return [$numright, count($this->rows)]; } - - #[\Override] - protected function validate_response_fields(array $response, bool $requireall): bool { - foreach ($this->roworder as $key => $rownumber) { - $fieldname = $this->field($key); - $answerexist = array_key_exists($fieldname, $response); - if ($requireall && !$answerexist) { - return false; - } - if (!$requireall && $answerexist) { - return true; - } - } - - return $requireall; - } } /** @@ -459,6 +440,36 @@ public function summarise_response(array $response): ?string { return implode('; ', $responsewords); } + #[\Override] + public function is_complete_response(array $response): bool { + foreach ($this->roworder as $key => $rownumber) { + $inputresponse = false; + foreach ($this->columns as $column) { + $fieldname = $this->field($key, $column->number); + if (array_key_exists($fieldname, $response)) { + $inputresponse = true; + } + } + if (!$inputresponse) { + return $inputresponse; + } + } + return true; + } + + #[\Override] + public function is_gradable_response(array $response): bool { + foreach ($this->roworder as $key => $rownumber) { + foreach ($this->columns as $column) { + $fieldname = $this->field($key, $column->number); + if (array_key_exists($fieldname, $response)) { + return true; + } + } + } + return false; + } + #[\Override] public function classify_response(array $response) { $classifiedresponse = []; @@ -606,26 +617,4 @@ public function get_num_correct_choices(): int { } return $numcorrect; } - - #[\Override] - protected function validate_response_fields(array $response, bool $requireall): bool { - foreach ($this->roworder as $key => $rownumber) { - $inputresponse = false; - foreach ($this->columns as $column) { - $fieldname = $this->field($key, $column->number); - if (array_key_exists($fieldname, $response)) { - $inputresponse = true; - } - } - if ($requireall && !$inputresponse) { - return false; - } - - if (!$requireall && $inputresponse) { - return true; - } - } - - return $requireall; - } }