From cb2afc668dc0159372d62868c08b76f9e1771f87 Mon Sep 17 00:00:00 2001 From: Peter Dias Date: Mon, 18 Oct 2021 13:35:22 +0800 Subject: [PATCH 1/2] MDL-72827 completion: Revert completion flow logic - Reverts the custom flow logic introduced in the 52206 - Expand unit tests to cover different grade based completion settings --- completion/upgrade.txt | 7 --- lib/completionlib.php | 30 +++------- lib/tests/completionlib_test.php | 95 ++++++++++++++++++++++++++++++++ mod/quiz/lib.php | 9 --- 4 files changed, 102 insertions(+), 39 deletions(-) diff --git a/completion/upgrade.txt b/completion/upgrade.txt index c836f09ebdb..e4e576c162c 100644 --- a/completion/upgrade.txt +++ b/completion/upgrade.txt @@ -4,15 +4,8 @@ information provided here is intended especially for developers. === 4.0 === * New method mark_course_completions_activity_criteria() has been added to mark course completions instantly. It is based on cron for completion_criteria_activity.php which is refactored to use it as well. -* Modified completion criteria to allow plugins to override core completion logic. * Core now passes an additional parameter to '_get_completion_state'. This is an array representation of the completion results that have already been tested. Currently contains - viewed, usegrade, passgrade. Any plugin that are dependent on these criteria can now check this array instead of retesting it. -* Introduced a new plugin function - '_get_completion_aggregation_state', that would indicate the aggregation type/relationship between the plugin and core - completion criteria. This callback should either return a COMPLETION_STANDARD_FLOW / COMPLETION_CUSTOM_MODULE_FLOW. The former for default existing core - behaviour while the latter enforces the override logic from the plugin. Defaults to COMPLETION_STANDARD_FLOW if not defined. This is useful when plugins - need to override the core completion criteria in cases where it may be dependent on them. In these cases, the 'source of truth' would be the response - from the plugin's 'get_completion_state' function. e.g. Quiz's completion defines a criteria of 'requires passing grade OR all attempts AND min attempts - reached.' In these cases, even if a passing grade has not been achieved, the activity should be marked as completed if the no.of attempts have been reached. * The method \completion_criteria_completion::mark_complete() now has the optional $timecompleted parameter to specify when the criteria was completed. diff --git a/lib/completionlib.php b/lib/completionlib.php index e9b5007f40b..77c34bad9cf 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -132,16 +132,6 @@ define('COMPLETION_OR', false); */ define('COMPLETION_AND', true); -/** - * When a module implements this, completion state is dependent to the - * module's _get_completion_state callback and activity_custom_completion class. - */ -define('COMPLETION_CUSTOM_MODULE_FLOW', true); -/** - * Standard flow indicates ALL conditions need to be met for completion to be marked as done. - */ -define('COMPLETION_STANDARD_FLOW', false); - /** * Course completion criteria aggregation method. */ @@ -733,28 +723,22 @@ class completion_info { $response = $function($this->course, $cm, $userid, COMPLETION_AND, $completionstate); } - // Get the relationship between the core_completion and plugin_completion criteria. - $aggregationtype = COMPLETION_STANDARD_FLOW; - if ($aggregationfn = component_callback_exists("mod_$cminfo->modname", 'get_completion_aggregation_state')) { - $aggregationtype = $aggregationfn(); - } - // If the module aggregates using COMPLETION_STANDARD_FLOW, it requires ALL conditions to be met. - // If the aggregation type is COMPLETION_CUSTOM_MODULE_FLOW, completion can be overridden by the plugin. - if (!$response && $aggregationtype == COMPLETION_STANDARD_FLOW) { + if (!$response) { return COMPLETION_INCOMPLETE; - } else if ($aggregationtype == COMPLETION_CUSTOM_MODULE_FLOW) { - return ($response ? COMPLETION_COMPLETE : COMPLETION_INCOMPLETE); } } if ($completionstate) { // We have allowed the plugins to do it's thing and run their own checks. // We have now reached a state where we need to AND all the calculated results. + // Preference for COMPLETION_COMPLETE_PASS over COMPLETION_COMPLETE for proper indication in reports. $newstate = array_reduce($completionstate, function($carry, $value) { - if ($carry == COMPLETION_INCOMPLETE) { - return $carry; + if (in_array(COMPLETION_INCOMPLETE, [$carry, $value])) { + return COMPLETION_INCOMPLETE; + } else if (in_array(COMPLETION_COMPLETE_FAIL, [$carry, $value])) { + return COMPLETION_COMPLETE_FAIL; } else { - return $value; + return in_array(COMPLETION_COMPLETE_PASS, [$carry, $value]) ? COMPLETION_COMPLETE_PASS : $value; } }, COMPLETION_COMPLETE); diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index 91f3c8340e9..00ae923f469 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -375,6 +375,101 @@ class core_completionlib_testcase extends advanced_testcase { $this->assertEquals($expectedstate, $completioninfo->internal_get_state($cm, $userid, $current)); } + /** + * Provider for the test_internal_get_state_with_grade_criteria. + * + * @return array + */ + public function test_internal_get_state_with_grade_criteria_provider() { + return [ + "Passing grade enabled and achieve. State should be COMPLETION_COMPLETE_PASS" => [ + [ + 'completionusegrade' => 1, + 'completionpassgrade' => 1, + 'gradepass' => 50, + ], + 50, + COMPLETION_COMPLETE_PASS + ], + "Passing grade enabled and not achieve. State should be COMPLETION_COMPLETE_FAIL" => [ + [ + 'completionusegrade' => 1, + 'completionpassgrade' => 1, + 'gradepass' => 50, + ], + 40, + COMPLETION_COMPLETE_FAIL + ], + "Passing grade not enabled with passing grade set." => [ + [ + 'completionusegrade' => 1, + 'gradepass' => 50, + ], + 50, + COMPLETION_COMPLETE_PASS + ], + "Passing grade not enabled with passing grade not set." => [ + [ + 'completionusegrade' => 1, + ], + 90, + COMPLETION_COMPLETE + ], + "Passing grade not enabled with passing grade not set. No submission made." => [ + [ + 'completionusegrade' => 1, + ], + null, + COMPLETION_INCOMPLETE + ], + ]; + } + + /** + * Tests that the right completion state is being set based on the grade criteria. + * + * @dataProvider test_internal_get_state_with_grade_criteria_provider + * @param array $completioncriteria The completion criteria to use + * @param int|null $studentgrade Grade to assign to student + * @param int $expectedstate Expected completion state + */ + public function test_internal_get_state_with_grade_criteria(array $completioncriteria, ?int $studentgrade, int $expectedstate) { + $this->setup_data(); + + /** @var \mod_assign_generator $assigngenerator */ + $assigngenerator = $this->getDataGenerator()->get_plugin_generator('mod_assign'); + $assign = $assigngenerator->create_instance([ + 'course' => $this->course->id, + 'completion' => COMPLETION_ENABLED, + ] + $completioncriteria); + + $userid = $this->user->id; + + $cm = get_coursemodule_from_instance('assign', $assign->id); + $usercm = cm_info::create($cm, $userid); + + // Create a teacher account. + $teacher = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($teacher->id, $this->course->id, 'editingteacher'); + // Log in as the teacher. + $this->setUser($teacher); + + // Grade the student for this assignment. + $assign = new assign($usercm->context, $cm, $cm->course); + if ($studentgrade) { + $data = (object)[ + 'sendstudentnotifications' => false, + 'attemptnumber' => 1, + 'grade' => $studentgrade, + ]; + $assign->save_grade($userid, $data); + } + + // The target user already received a grade, so internal_get_state should be already complete. + $completioninfo = new completion_info($this->course); + $this->assertEquals($expectedstate, $completioninfo->internal_get_state($cm, $userid, null)); + } + /** * Covers the case where internal_get_state() is being called for a user different from the logged in user. */ diff --git a/mod/quiz/lib.php b/mod/quiz/lib.php index fe3a60d1855..63acbf6a679 100644 --- a/mod/quiz/lib.php +++ b/mod/quiz/lib.php @@ -1908,15 +1908,6 @@ function quiz_get_navigation_options() { ); } -/** - * Get the aggregation state for the module. - * - * @return bool - */ -function quiz_get_completion_aggregation_state() { - return COMPLETION_CUSTOM_MODULE_FLOW; -} - /** * Check if the module has any update that affects the current user since a given time. * From 44d06f8218d07b3519c8a9b7d79441c78853302a Mon Sep 17 00:00:00 2001 From: Peter Dias Date: Mon, 18 Oct 2021 15:14:54 +0800 Subject: [PATCH 2/2] MDL-72827 quiz: Expand completion behat tests - Update the behat tests to more precisely check the state in the completion report. - Check combination of completion conditions with passing grade --- .../completion_condition_attempts_used.feature | 15 ++++++++++----- .../behat/quiz_activity_completion.feature | 17 ++++++++++++++--- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/mod/quiz/tests/behat/completion_condition_attempts_used.feature b/mod/quiz/tests/behat/completion_condition_attempts_used.feature index 63e3b611b27..f79805fd5e3 100644 --- a/mod/quiz/tests/behat/completion_condition_attempts_used.feature +++ b/mod/quiz/tests/behat/completion_condition_attempts_used.feature @@ -34,7 +34,7 @@ Feature: Set a quiz to be marked complete when the student uses all attempts all | slot | response | | 1 | False | - Scenario: student1 uses up both attempts without passing + Scenario Outline: Student attempts the quiz - pass and fails When I log in as "student1" And I am on "Course 1" course homepage And the "Receive a grade" completion condition of "Test quiz name" is displayed as "done" @@ -42,16 +42,16 @@ Feature: Set a quiz to be marked complete when the student uses all attempts all And the "Receive a pass grade or complete all available attempts" completion condition of "Test quiz name" is displayed as "todo" And I follow "Test quiz name" And I press "Re-attempt quiz" - And I set the field "False" to "1" + And I set the field "" to "1" And I press "Finish attempt ..." And I press "Submit all and finish" And I am on "Course 1" course homepage Then the "Receive a grade" completion condition of "Test quiz name" is displayed as "done" - And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "failed" + And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "" And the "Receive a pass grade or complete all available attempts" completion condition of "Test quiz name" is displayed as "done" And I follow "Test quiz name" And the "Receive a grade" completion condition of "Test quiz name" is displayed as "done" - And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "failed" + And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "" And the "Receive a pass grade or complete all available attempts" completion condition of "Test quiz name" is displayed as "done" And I log out And I log in as "teacher1" @@ -60,4 +60,9 @@ Feature: Set a quiz to be marked complete when the student uses all attempts all And "Test quiz name" should have the "Receive a pass grade or complete all available attempts" completion condition And I am on "Course 1" course homepage And I navigate to "Reports > Activity completion" in current page administration - And "Completed" "icon" should exist in the "Student 1" "table_row" + And "" "icon" should exist in the "Student 1" "table_row" + + Examples: + | answer | passcompletionexpected | expectedactivitycompletion | + | False | failed | Completed (did not achieve pass grade) | + | True | done | Completed (achieved pass grade) | diff --git a/mod/quiz/tests/behat/quiz_activity_completion.feature b/mod/quiz/tests/behat/quiz_activity_completion.feature index 63dc6ac21f1..dc4212fbf9b 100644 --- a/mod/quiz/tests/behat/quiz_activity_completion.feature +++ b/mod/quiz/tests/behat/quiz_activity_completion.feature @@ -43,7 +43,7 @@ Feature: View activity completion in the quiz activity | question | page | | First question | 1 | - Scenario: View automatic completion items as a student + Scenario Outline: View automatic completion items as a student When I log in as "student1" And I am on "Course 1" course homepage And I follow "Test quiz name" @@ -63,12 +63,23 @@ Feature: View activity completion in the quiz activity And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "failed" And the "Receive a pass grade or complete all available attempts" completion condition of "Test quiz name" is displayed as "todo" And I press "Re-attempt quiz" - And I set the field "True" to "1" + And I set the field "" to "1" And I press "Finish attempt ..." And I press "Submit all and finish" And I follow "Finish review" And the "View" completion condition of "Test quiz name" is displayed as "done" And the "Make attempts: 1" completion condition of "Test quiz name" is displayed as "done" And the "Receive a grade" completion condition of "Test quiz name" is displayed as "done" - And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "done" + And the "Receive a passing grade" completion condition of "Test quiz name" is displayed as "" And the "Receive a pass grade or complete all available attempts" completion condition of "Test quiz name" is displayed as "done" + And I log out + And I log in as "teacher1" + And I am on "Course 1" course homepage + And I navigate to "Reports > Activity completion" in current page administration + And I pause + And "" "icon" should exist in the "Student 1" "table_row" + + Examples: + | answer | passcompletionexpected | expectedactivitycompletion | + | False | failed | Completed (did not achieve pass grade) | + | True | done | Completed (achieved pass grade) |