MDL-37361 completion: automatic completion disabled when overridden

Changes:
- Activities with auto completion and a completion status overridden to
COMPLETION_COMPLETE are no longer processed by normal completion
triggers.
- All activities can still be completed by students when their
completion status has been overridden to COMPLETION_INCOMPLETE, via
either auto or manual triggers.
- Completion unit tests updated

completion/tests/externallib_test.php

@@ -191,99 +191,58 @@ class core_completion_externallib_testcase extends externallib_advanced_testcase
      */
     public function test_override_activity_completion_status() {
         global $DB, $CFG;
-
         $this->resetAfterTest(true);
 
+        // Create course with teacher and student enrolled.
         $CFG->enablecompletion = true;
+        $course  = $this->getDataGenerator()->create_course(['enablecompletion' => 1]);
         $student = $this->getDataGenerator()->create_user();
         $teacher = $this->getDataGenerator()->create_user();
-
-        $course = $this->getDataGenerator()->create_course(array('enablecompletion' => 1));
-
-        $data = $this->getDataGenerator()->create_module('data', array('course' => $course->id),
-                                                             array('completion' => 1));
-        $forum = $this->getDataGenerator()->create_module('forum',  array('course' => $course->id),
-                                                             array('completion' => 2));
-
-        $cmdata = get_coursemodule_from_id('data', $data->cmid);
-        $cmforum = get_coursemodule_from_id('forum', $forum->cmid);
-
         $studentrole = $DB->get_record('role', array('shortname' => 'student'));
         $this->getDataGenerator()->enrol_user($student->id, $course->id, $studentrole->id);
-
         $teacherrole = $DB->get_record('role', array('shortname' => 'teacher'));
         $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id);
 
-        $completion = new completion_info($course);
+        // Create 2 activities, one with manual completion (data), one with automatic completion triggered by viewiung it (forum).
+        $data    = $this->getDataGenerator()->create_module('data', ['course' => $course->id], ['completion' => 1]);
+        $forum   = $this->getDataGenerator()->create_module('forum',  ['course' => $course->id],
+                                                            ['completion' => 2, 'completionview' => 1]);
+        $cmdata = get_coursemodule_from_id('data', $data->cmid);
+        $cmforum = get_coursemodule_from_id('forum', $forum->cmid);
 
+        // Manually complete the data activity as the student.
         $this->setUser($student);
-
-        // Mark data activity complete by student.
+        $completion = new completion_info($course);
         $completion->update_state($cmdata, COMPLETION_COMPLETE);
-        $completiondata = $completion->get_data($cmdata);
 
+        // Test overriding the status of the manual-completion-activity 'incomplete'.
         $this->setUser($teacher);
-
-        // Override data completion state to incomplete.
         $result = core_completion_external::override_activity_completion_status($student->id, $data->cmid, COMPLETION_INCOMPLETE);
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(
-            core_completion_external::override_activity_completion_status_returns(), $result);
+        $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result);
         $this->assertTrue($result['status']);
-
-        // Check in DB.
-        $this->assertEquals(COMPLETION_INCOMPLETE, $DB->get_field('course_modules_completion', 'completionstate',
-                            array('coursemoduleid' => $data->cmid)));
-
-        // Check using the API.
         $completiondata = $completion->get_data($cmdata, false, $student->id);
         $this->assertEquals(COMPLETION_INCOMPLETE, $completiondata->completionstate);
 
-        // Override data completion state to complete.
+        // Test overriding the status of the manual-completion-activity back to 'complete'.
         $result = core_completion_external::override_activity_completion_status($student->id, $data->cmid, COMPLETION_COMPLETE);
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(
-            core_completion_external::override_activity_completion_status_returns(), $result);
+        $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result);
         $this->assertTrue($result['status']);
-
-        // Check in DB.
-        $this->assertEquals(COMPLETION_COMPLETE, $DB->get_field('course_modules_completion', 'completionstate',
-                            array('coursemoduleid' => $data->cmid)));
-
-        // Check using the API.
         $completiondata = $completion->get_data($cmdata, false, $student->id);
         $this->assertEquals(COMPLETION_COMPLETE, $completiondata->completionstate);
 
-        // Override forum completion state to complete.
+        // Test overriding the status of the auto-completion-activity to 'complete'.
         $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, COMPLETION_COMPLETE);
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(
-            core_completion_external::override_activity_completion_status_returns(), $result);
+        $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result);
         $this->assertTrue($result['status']);
-
-        // Check in DB.
-        $this->assertEquals(COMPLETION_COMPLETE, $DB->get_field('course_modules_completion', 'completionstate',
-                            array('coursemoduleid' => $forum->cmid)));
-
-        // Check using the API.
         $completionforum = $completion->get_data($cmforum, false, $student->id);
         $this->assertEquals(COMPLETION_COMPLETE, $completionforum->completionstate);
 
-        // Override forum completion state to incomplete.
+        // Test overriding the status of the auto-completion-activity to 'incomplete'.
         $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, COMPLETION_INCOMPLETE);
-        // We need to execute the return values cleaning process to simulate the web service server.
-        $result = external_api::clean_returnvalue(
-            core_completion_external::override_activity_completion_status_returns(), $result);
+        $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result);
         $this->assertTrue($result['status']);
-
-        // Check in DB.
-        $this->assertEquals(COMPLETION_INCOMPLETE, $DB->get_field('course_modules_completion', 'completionstate',
-                            array('coursemoduleid' => $forum->cmid)));
-
-        // Check using the API.
         $completionforum = $completion->get_data($cmforum, false, $student->id);
         $this->assertEquals(COMPLETION_INCOMPLETE, $completionforum->completionstate);
-
     }
 
     /**

lib/completionlib.php

@@ -570,9 +570,17 @@ class completion_info {
             return;
         }
 
+        // For auto tracking, if the status is overridden to 'COMPLETION_COMPLETE', then disallow further changes,
+        // unless processing another override.
+        // Basically, we want those activities which have been overridden to COMPLETE to hold state, and those which have been
+        // overridden to INCOMPLETE to still be processed by normal completion triggers.
+        if ($cm->completion == COMPLETION_TRACKING_AUTOMATIC && !is_null($current->overrideby)
+            && $current->completionstate == COMPLETION_COMPLETE && !$override) {
+            return;
+        }
+
+        // For manual tracking, or if overriding the completion state, we set the state directly.
         if ($cm->completion == COMPLETION_TRACKING_MANUAL || $override) {
-            // For manual tracking, or if overriding the completion state manually,
-            // we set the result directly.
             switch($possibleresult) {
                 case COMPLETION_COMPLETE:
                 case COMPLETION_INCOMPLETE:
@@ -583,15 +591,7 @@ class completion_info {
             }
 
         } else {
-            // Automatic tracking.
-            if ($current->overrideby) {
-                // If the current completion state has been set by override, do nothing
-                // as we don't want it to be changed automatically.
-                return;
-            } else {
-                // Get new state.
-                $newstate = $this->internal_get_state($cm, $userid, $current);
-            }
+            $newstate = $this->internal_get_state($cm, $userid, $current);
         }
 
         // If changed, update
@@ -708,8 +708,9 @@ class completion_info {
         // Get current completion state
         $data = $this->get_data($cm, false, $userid);
 
-        // If we already viewed it, don't do anything
-        if ($data->viewed == COMPLETION_VIEWED) {
+        // If we already viewed it, don't do anything unless the completion status is overridden.
+        // If the completion status is overridden, then we need to allow this 'view' to trigger automatic completion again.
+        if ($data->viewed == COMPLETION_VIEWED && empty($data->overrideby)) {
             return;
         }
 

lib/tests/completionlib_test.php

@@ -243,8 +243,15 @@ class core_completionlib_testcase extends advanced_testcase {
             ->method('internal_set_data')
             ->with($cm, $comparewith);
         $c->update_state($cm, COMPLETION_COMPLETE, 100, true);
+        // And confirm that the status can be changed back to incomplete without an override.
+        $c->update_state($cm, COMPLETION_INCOMPLETE, 100);
+        $c->expects($this->at(0))
+            ->method('get_data')
+            ->with($cm, false, 100)
+            ->will($this->returnValue($current));
+        $c->get_data($cm, false, 100);
 
-        // Auto tracking, change state by overriding it manually.
+        // Auto, change state via override, incomplete to complete.
         $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC);
         $current = (object)array('completionstate' => COMPLETION_INCOMPLETE, 'overrideby' => null);
         $c->expects($this->at(0))
@@ -265,8 +272,22 @@ class core_completionlib_testcase extends advanced_testcase {
             ->method('internal_set_data')
             ->with($cm, $comparewith);
         $c->update_state($cm, COMPLETION_COMPLETE, 100, true);
+        $c->expects($this->at(0))
+            ->method('get_data')
+            ->with($cm, false, 100)
+            ->will($this->returnValue($changed));
+        $c->get_data($cm, false, 100);
 
-        // Auto tracking, but current state is set by override, so do nothing.
+        // Now confirm that the status cannot be changed back to incomplete without an override.
+        // I.e. test that automatic completion won't trigger a change back to COMPLETION_INCOMPLETE when overridden.
+        $c->update_state($cm, COMPLETION_INCOMPLETE, 100);
+        $c->expects($this->at(0))
+            ->method('get_data')
+            ->with($cm, false, 100)
+            ->will($this->returnValue($changed));
+        $c->get_data($cm, false, 100);
+
+        // Now confirm the status can be changed back from complete to incomplete using an override.
         $cm = (object)array('id' => 13, 'course' => 42, 'completion' => COMPLETION_TRACKING_AUTOMATIC);
         $current = (object)array('completionstate' => COMPLETION_COMPLETE, 'overrideby' => 2);
         $c->expects($this->at(0))
@@ -275,10 +296,23 @@ class core_completionlib_testcase extends advanced_testcase {
             ->will($this->returnValue(true));
         $c->expects($this->at(1))
             ->method('get_data')
-            ->with($cm, false, 0)
+            ->with($cm, false, 100)
             ->will($this->returnValue($current));
-        $c->update_state($cm, COMPLETION_COMPLETE_PASS);
-
+        $changed = clone($current);
+        $changed->timemodified = time();
+        $changed->completionstate = COMPLETION_INCOMPLETE;
+        $changed->overrideby = 314159;
+        $comparewith = new phpunit_constraint_object_is_equal_with_exceptions($changed);
+        $comparewith->add_exception('timemodified', 'assertGreaterThanOrEqual');
+        $c->expects($this->at(2))
+            ->method('internal_set_data')
+            ->with($cm, $comparewith);
+        $c->update_state($cm, COMPLETION_INCOMPLETE, 100, true);
+        $c->expects($this->at(0))
+            ->method('get_data')
+            ->with($cm, false, 100)
+            ->will($this->returnValue($changed));
+        $c->get_data($cm, false, 100);
     }
 
     public function test_internal_get_state() {