MDL-58266 core_completion: update code review feedback

NashTechOpenUniversity · Jun 15, 2022 · 5c26538 · 5c26538
1 parent f7b1393
commit 5c26538
Show file tree

Hide file tree

Showing 13 changed files with 248 additions and 76 deletions.
diff --git a/backup/moodle2/backup_stepslib.php b/backup/moodle2/backup_stepslib.php
@@ -1287,28 +1287,65 @@ protected function define_structure() {
 
         $completions = new backup_nested_element('completions');
 
-        $completion = new backup_nested_element('completion', ['id'], ['userid', 'completionstate', 'timemodified']);
-        $completionview = new backup_nested_element('completion_viewed', ['id'], ['userid', 'viewed']);
+        $completion = new backup_nested_element('completion', array('id'), array(
+            'userid', 'completionstate', 'viewed', 'timemodified'));
+
         // Build the tree
 
         $completions->add_child($completion);
-        $completions->add_child($completionview);
 
         // Define sources
 
-        $completion->set_source_table('course_modules_completion', ['coursemoduleid' => backup::VAR_MODID]);
-        $completionview->set_source_table('course_modules_completion_v', ['coursemoduleid' => backup::VAR_MODID]);
+        $completion->set_source_table('course_modules_completion', array('coursemoduleid' => backup::VAR_MODID));
 
         // Define id annotations
 
         $completion->annotate_ids('user', 'userid');
-        $completionview->annotate_ids('user', 'userid');
 
         // Return the root element (completions)
         return $completions;
     }
 }
 
+/**
+ * Structure step in charge if constructing the completion.xml file for all the users completion information in a given activity.
+ */
+class backup_userscompletionview_structure_step extends backup_structure_step {
+
+    /**
+     * Skip completion on the front page.
+     * @return bool
+     */
+    protected function execute_condition() {
+        return ($this->get_courseid() != SITEID);
+    }
+
+    /**
+     * Define structure of xml file.
+     *
+     * @return backup_nested_element
+     */
+    protected function define_structure() {
+
+        // Define each element separated.
+        $completionviews = new backup_nested_element('completionviews');
+
+        $completionview = new backup_nested_element('completionview', ['id'], ['userid', 'viewed', 'timecreated']);
+
+        // Build the tree.
+        $completionviews->add_child($completionview);
+
+        // Define sources.
+        $completionview->set_source_table('course_modules_viewed', ['coursemoduleid' => backup::VAR_MODID]);
+
+        // Define id annotations.
+        $completionview->annotate_ids('user', 'userid');
+
+        // Return the root element (completionviews).
+        return $completionviews;
+    }
+}
+
 /**
  * structure step in charge of constructing the main groups.xml file for all the groups and
  * groupings information already annotated

diff --git a/backup/moodle2/restore_activity_task.class.php b/backup/moodle2/restore_activity_task.class.php
@@ -178,6 +178,7 @@ public function build() {
         // Userscompletion (conditionally)
         if ($this->get_setting_value('userscompletion')) {
             $this->add_step(new restore_userscompletion_structure_step('activity_userscompletion', 'completion.xml'));
+            $this->add_step(new restore_userscompletionview_structure_step('activity_userscompletionview', 'completionview.xml'));
         }
 
         // Logs (conditionally)

diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php
@@ -4670,7 +4670,6 @@ protected function define_structure() {
         $paths = array();
 
         $paths[] = new restore_path_element('completion', '/completions/completion');
-        $paths[] = new restore_path_element('completion_viewed', '/completions/completion_viewed');
 
         return $paths;
     }
@@ -4703,51 +4702,76 @@ protected function process_completion($data) {
         // After MDL-58266 changes.
         // For legacy backup.
         if (isset($data->viewed)) {
-            $existingview = $DB->get_record('course_modules_completion_v', [
-                'coursemoduleid' => $data->coursemoduleid,
-                'userid' => $data->userid]
-                , 'id');
-            $dataview = (object)[];
-            $dataview->viewed = $data->viewed;
-            $dataview->coursemoduleid = $data->coursemoduleid;
-            $dataview->userid = $data->userid;
-            $this->process_completion_viewer_data($existingview, $dataview);
+            $dataview = clone($data);
+            unset($dataview->id);
+            $dataview->timecreated = $data->timemodified;
+
+            $DB->insert_record('course_modules_viewed', $dataview);
         }
     }
+}
 
+/**
+ * Structure step that will process the user activity completion viewed.
+ */
+class restore_userscompletionview_structure_step extends restore_structure_step {
     /**
-     * Process completion viewed row.
-     *
-     * @param stdClass $data
+     * To conditionally decide if this step must be executed
+     * Note the "settings" conditions are evaluated in the
+     * corresponding task. Here we check for other conditions
+     * not being restore settings (files, site settings...)
      */
-    protected function process_completion_viewed($data) {
-        global $DB;
-        $data = (object)$data;
+    protected function execute_condition() {
+        global $CFG;
 
-        $data->coursemoduleid = $this->task->get_moduleid();
-        $data->userid = $this->get_mappingid('user', $data->userid);
+        // Completion disabled in this site, don't execute.
+        if (empty($CFG->enablecompletion)) {
+            return false;
+        }
 
-        $existingview = $DB->get_record('course_modules_completion_v', [
-            'coursemoduleid' => $data->coursemoduleid,
-            'userid' => $data->userid], 'id');
-        $this->process_completion_viewer_data($existingview, $data);
+        // No completion on the front page.
+        if ($this->get_courseid() == SITEID) {
+            return false;
+        }
+
+        // No user completion info found, don't execute.
+        $fullpath = $this->task->get_taskbasepath();
+        $fullpath = rtrim($fullpath, '/') . '/' . $this->filename;
+        if (!file_exists($fullpath)) {
+            return false;
+        }
+
+        // Arrived here, execute the step.
+        return true;
     }
 
     /**
-     * Process completion viewer data.
+     * Define structure for completionview.
      *
-     * @param mixed $existingview Existing record or false.
-     * @param object $data
+     * @return array
+     */
+    protected function define_structure(): array {
+
+        $paths = array();
+
+        $paths[] = new restore_path_element('completionview', '/completionviews/completionview');
+
+        return $paths;
+    }
+
+    /**
+     * Process completion structure in the xml.
      *
+     * @param array $data
      */
-    private function process_completion_viewer_data($existingview, $data): void {
+    protected function process_completionview(array $data) {
         global $DB;
-        if ($existingview) {
-            $data->id = $existingview->id;
-            $DB->update_record('course_modules_completion_v', $data);
-        } else {
-            $DB->insert_record('course_modules_completion_v', $data);
-        }
+
+        $data = (object)$data;
+        $data->coursemoduleid = $this->task->get_moduleid();
+        $data->userid = $this->get_mappingid('user', $data->userid);
+
+        $DB->insert_record('course_modules_viewed', $data);
     }
 }
 

diff --git a/backup/moodle2/tests/restore_stepslib_date_test.php b/backup/moodle2/tests/restore_stepslib_date_test.php
@@ -314,6 +314,39 @@ public function test_usercompletion_date_restore() {
         $this->assertEquals($coursemodulecompletion->timemodified, $newcoursemodulecompletion->timemodified);
     }
 
+    /**
+     * Checking that the user completion of an activity relating to the view field does not change
+     * when doing a course restore.
+     * @covers ::backup_and_restore
+     */
+    public function test_usercompletion_view_restore() {
+        global $DB;
+        // More completion...
+        $course = $this->getDataGenerator()->create_course(['startdate' => time(), 'enablecompletion' => 1]);
+        $assign = $this->getDataGenerator()->create_module('assign', [
+            'course' => $course->id,
+            'completion' => COMPLETION_TRACKING_AUTOMATIC, // Show activity as complete when conditions are met.
+            'completionview' => 1
+        ]);
+        $cm = $DB->get_record('course_modules', ['course' => $course->id, 'instance' => $assign->id]);
+
+        // Mark the activity as completed.
+        $completion = new \completion_info($course);
+        $completion->set_module_viewed($cm);
+
+        $coursemodulecompletion = $DB->get_record('course_modules_viewed', ['coursemoduleid' => $cm->id]);
+
+        // Back up and restore.
+        $newcourseid = $this->backup_and_restore($course);
+        $newcourse = get_course($newcourseid);
+
+        $assignid = $DB->get_field('assign', 'id', ['course' => $newcourseid]);
+        $cm = $DB->get_record('course_modules', ['course' => $newcourse->id, 'instance' => $assignid]);
+        $newcoursemodulecompletion = $DB->get_record('course_modules_viewed', ['coursemoduleid' => $cm->id]);
+
+        $this->assertEquals($coursemodulecompletion->timecreated, $newcoursemodulecompletion->timecreated);
+    }
+
     /**
      * Ensuring that the timemodified field of the question attempt steps table does not change when
      * a course restore is done.

diff --git a/completion/classes/privacy/provider.php b/completion/classes/privacy/provider.php
@@ -69,10 +69,11 @@ public static function get_metadata(collection $collection) : collection {
                 'overrideby' => 'privacy:metadata:overrideby',
                 'timemodified' => 'privacy:metadata:timemodified'
             ], 'privacy:metadata:coursemodulesummary');
-        $collection->add_database_table('course_modules_completion_v', [
+        $collection->add_database_table('course_modules_viewed', [
             'userid' => 'privacy:metadata:userid',
             'coursemoduleid' => 'privacy:metadata:coursemoduleid',
             'viewed' => 'privacy:metadata:viewed',
+            'timecreated' => 'privacy:metadata:timecreated',
         ], 'privacy:metadata:coursemodulesummary');
         $collection->add_database_table('course_completion_crit_compl', [
                 'userid' => 'privacy:metadata:userid',
@@ -95,17 +96,17 @@ public static function get_metadata(collection $collection) : collection {
     public static function get_course_completion_join_sql(int $userid, string $prefix, string $joinfield) : array {
         $cccalias = "{$prefix}_ccc"; // Course completion criteria.
         $cmcalias = "{$prefix}_cmc"; // Course modules completion.
-        $cmcvalias = "{$prefix}_cmcv"; // Course modules completion viewed.
+        $cmvalias = "{$prefix}_cmv"; // Course modules viewed.
         $ccccalias = "{$prefix}_cccc"; // Course completion criteria completion.
 
         $join = "JOIN {course_completion_criteria} {$cccalias} ON {$joinfield} = {$cccalias}.course
              LEFT JOIN {course_modules_completion} {$cmcalias} ON {$cccalias}.moduleinstance = {$cmcalias}.coursemoduleid
                         AND {$cmcalias}.userid = :{$prefix}_moduleuserid
-             LEFT JOIN {course_modules_completion_v} {$cmcvalias} ON {$cccalias}.moduleinstance = {$cmcvalias}.coursemoduleid
-                        AND {$cmcvalias}.userid = :{$prefix}_moduleuserid2
+             LEFT JOIN {course_modules_viewed} {$cmvalias} ON {$cccalias}.moduleinstance = {$cmvalias}.coursemoduleid
+                        AND {$cmvalias}.userid = :{$prefix}_moduleuserid2
              LEFT JOIN {course_completion_crit_compl} {$ccccalias} ON {$ccccalias}.criteriaid = {$cccalias}.id
                         AND {$ccccalias}.userid = :{$prefix}_courseuserid";
-        $where = "{$cmcalias}.id IS NOT NULL OR {$ccccalias}.id IS NOT NULL OR {$cmcvalias}.id IS NOT NULL";
+        $where = "{$cmcalias}.id IS NOT NULL OR {$ccccalias}.id IS NOT NULL OR {$cmvalias}.id IS NOT NULL";
         $params = ["{$prefix}_moduleuserid" => $userid, "{$prefix}_moduleuserid2" => $userid, "{$prefix}_courseuserid" => $userid];
         return [$join, $where, $params];
     }
@@ -132,10 +133,10 @@ public static function add_course_completion_users_to_userlist(userlist $userlis
 
         $userlist->add_from_sql('userid', $sql, $params);
 
-        $sql = "SELECT cmcv.userid
+        $sql = "SELECT cmv.userid
                   FROM {course} c
                   JOIN {course_completion_criteria} ccc ON ccc.course = c.id
-                  JOIN {course_modules_completion_v} cmcv ON cmcv.coursemoduleid = ccc.moduleinstance
+                  JOIN {course_modules_viewed} cmv ON cmv.coursemoduleid = ccc.moduleinstance
                  WHERE c.id = :courseid";
 
         $userlist->add_from_sql('userid', $sql, $params);
@@ -245,7 +246,7 @@ public static function delete_completion(\stdClass $user = null, int $courseid =
         if (isset($courseid)) {
 
             $usersql = isset($user) ? 'AND cmc.userid = :userid' : '';
-            $usercmcvsql = isset($user) ? 'AND cmcv.userid = :userid' : '';
+            $usercmvsql = isset($user) ? 'AND cmv.userid = :userid' : '';
             $params = isset($user) ? ['course' => $courseid, 'userid' => $user->id] : ['course' => $courseid];
 
             // Find records relating to course modules.
@@ -261,16 +262,16 @@ public static function delete_completion(\stdClass $user = null, int $courseid =
                 $DB->delete_records_select('course_modules_completion', $deletesql, $deleteparams);
             }
             // Find records relating to course modules completion viewed.
-            $sql = "SELECT cmcv.id
+            $sql = "SELECT cmv.id
                       FROM {course_completion_criteria} ccc
-                      JOIN {course_modules_completion_v} cmcv ON ccc.moduleinstance = cmcv.coursemoduleid
-                     WHERE ccc.course = :course $usercmcvsql";
+                      JOIN {course_modules_viewed} cmv ON ccc.moduleinstance = cmv.coursemoduleid
+                     WHERE ccc.course = :course $usercmvsql";
             $recordids = $DB->get_records_sql($sql, $params);
             $ids = array_keys($recordids);
             if (!empty($ids)) {
                 list($deletesql, $deleteparams) = $DB->get_in_or_equal($ids);
                 $deletesql = 'id ' . $deletesql;
-                $DB->delete_records_select('course_modules_completion_v', $deletesql, $deleteparams);
+                $DB->delete_records_select('course_modules_viewed', $deletesql, $deleteparams);
             }
 
             $DB->delete_records('course_completion_crit_compl', $params);
@@ -301,7 +302,7 @@ public static function delete_completion_by_approved_userlist(approved_userlist
             // Only delete the record for course modules completion.
             $sql = "coursemoduleid = :coursemoduleid AND userid {$useridsql}";
             $DB->delete_records_select('course_modules_completion', $sql, $params);
-            $DB->delete_records_select('course_modules_completion_v', $sql, $params);
+            $DB->delete_records_select('course_modules_viewed', $sql, $params);
             return;
         }
 
@@ -322,16 +323,16 @@ public static function delete_completion_by_approved_userlist(approved_userlist
             }
 
             // Find records relating to course modules.
-            $sql = "SELECT cmcv.id
+            $sql = "SELECT cmv.id
                       FROM {course_completion_criteria} ccc
-                      JOIN {course_modules_completion_v} cmcv ON ccc.moduleinstance = cmcv.coursemoduleid
-                     WHERE ccc.course = :course AND cmcv.userid {$useridsql}";
+                      JOIN {course_modules_viewed} cmv ON ccc.moduleinstance = cmv.coursemoduleid
+                     WHERE ccc.course = :course AND cmv.userid {$useridsql}";
             $recordids = $DB->get_records_sql($sql, $params);
             $ids = array_keys($recordids);
             if (!empty($ids)) {
                 list($deletesql, $deleteparams) = $DB->get_in_or_equal($ids);
                 $deletesql = 'id ' . $deletesql;
-                $DB->delete_records_select('course_modules_completion_v', $deletesql, $deleteparams);
+                $DB->delete_records_select('course_modules_viewed', $deletesql, $deleteparams);
             }
 
             $sql = "course = :course AND userid {$useridsql}";

diff --git a/course/lib.php b/course/lib.php
@@ -933,7 +933,7 @@ function course_delete_module($cmid, $async = false) {
     // features are not turned on, in case they were turned on previously (these will be
     // very quick on an empty table).
     $DB->delete_records('course_modules_completion', array('coursemoduleid' => $cm->id));
-    $DB->delete_records('course_modules_completion_v', ['coursemoduleid' => $cm->id]);
+    $DB->delete_records('course_modules_viewed', ['coursemoduleid' => $cm->id]);
     $DB->delete_records('course_completion_criteria', array('moduleinstance' => $cm->id,
                                                             'course' => $cm->course,
                                                             'criteriatype' => COMPLETION_CRITERIA_TYPE_ACTIVITY));

diff --git a/lang/en/completion.php b/lang/en/completion.php
@@ -207,6 +207,7 @@
 $string['privacy:metadata:timeenrolled'] = 'The time that the user was enrolled in the course';
 $string['privacy:metadata:timemodified'] = 'The time that the activity completion was modified';
 $string['privacy:metadata:timestarted'] = 'The time the course was started.';
+$string['privacy:metadata:timecreated'] = 'The time that the activity completion was created';
 $string['privacy:metadata:viewed'] = 'If the activity was viewed';
 $string['privacy:metadata:userid'] = 'The user ID of the person with course and activity completion data';
 $string['privacy:metadata:unenroled'] = 'If the user has been unenrolled from the course';