MDL-38596 Optimise SQL in preloading course contacts for number of courses
authorMarina Glancy <marina@moodle.com>
Mon, 6 May 2013 04:15:22 +0000 (14:15 +1000)
committerMarina Glancy <marina@moodle.com>
Mon, 6 May 2013 04:15:22 +0000 (14:15 +1000)
lib/coursecatlib.php
lib/tests/coursecatlib_test.php

index 591b669..53ec5f9 100644 (file)
@@ -668,13 +668,13 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate {
             return;
         }
         $managerroles = explode(',', $CFG->coursecontact);
-        /*
-        // TODO MDL-38596, this commented code is similar to get_courses_wmanagers()
-        // It bulk-preloads course contacts for all courses BUT it does not check enrolments
+
+        // List of ids of courses for which we need to retrieve contacts
+        $courseids = array_keys($courses);
 
         // first build the array of all context ids of the courses and their categories
         $allcontexts = array();
-        foreach (array_keys($courses) as $id) {
+        foreach ($courseids as $id) {
             $context = context_course::instance($id);
             $courses[$id]->managers = array();
             foreach (preg_split('|/|', $context->path, 0, PREG_SPLIT_NO_EMPTY) as $ctxid) {
@@ -685,6 +685,7 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate {
             }
         }
 
+        // fetch list of all users with course contact roles in any of the courses contexts or parent contexts
         list($sql1, $params1) = $DB->get_in_or_equal(array_keys($allcontexts), SQL_PARAMS_NAMED, 'ctxid');
         list($sql2, $params2) = $DB->get_in_or_equal($managerroles, SQL_PARAMS_NAMED, 'rid');
         list($sort, $sortparams) = users_order_by_sql('u');
@@ -698,21 +699,86 @@ class coursecat implements renderable, cacheable_object, IteratorAggregate {
                 WHERE  ra.contextid ". $sql1." AND ra.roleid ". $sql2."
              ORDER BY r.sortorder, $sort";
         $rs = $DB->get_recordset_sql($sql, $params1 + $params2 + $sortparams);
+        $checkenrolments = array();
         foreach($rs as $ra) {
             foreach ($allcontexts[$ra->contextid] as $id) {
                 $courses[$id]->managers[$ra->raid] = $ra;
+                if (!isset($checkenrolments[$id])) {
+                    $checkenrolments[$id] = array();
+                }
+                $checkenrolments[$id][] = $ra->id;
             }
         }
         $rs->close();
-        */
-        list($sort, $sortparams) = users_order_by_sql('u');
-        foreach (array_keys($courses) as $id) {
-            $context = context_course::instance($id);
-            $courses[$id]->managers = get_role_users($managerroles, $context, true,
-                'ra.id AS raid, u.id, u.username, u.firstname, u.lastname, rn.name AS rolecoursealias,
-                 r.name AS rolename, r.sortorder, r.id AS roleid, r.shortname AS roleshortname',
-                'r.sortorder ASC, ' . $sort, false, '', '', '', '', $sortparams);
+
+        // remove from course contacts users who are not enrolled in the course
+        $enrolleduserids = self::ensure_users_enrolled($checkenrolments);
+        foreach ($checkenrolments as $id => $userids) {
+            if (empty($enrolleduserids[$id])) {
+                $courses[$id]->managers = array();
+            } else if ($notenrolled = array_diff($userids, $enrolleduserids[$id])) {
+                foreach ($courses[$id]->managers as $raid => $ra) {
+                    if (in_array($ra->id, $notenrolled)) {
+                        unset($courses[$id]->managers[$raid]);
+                    }
+                }
+            }
+        }
+    }
+
+    /**
+     * Verify user enrollments for multiple course-user combinations
+     *
+     * @param array $courseusers array where keys are course ids and values are array
+     *     of users in this course whose enrolment we wish to verify
+     * @return array same structure as input array but values list only users from input
+     *     who are enrolled in the course
+     */
+    protected static function ensure_users_enrolled($courseusers) {
+        global $DB;
+        // If the input array is too big, split it into chunks
+        $maxcoursesinquery = 20;
+        if (count($courseusers) > $maxcoursesinquery) {
+            $rv = array();
+            for ($offset = 0; $offset < count($courseusers); $offset += $maxcoursesinquery) {
+                $chunk = array_slice($courseusers, $offset, $maxcoursesinquery, true);
+                $rv = $rv + self::ensure_users_enrolled($chunk);
+            }
+            return $rv;
+        }
+
+        // create a query verifying valid user enrolments for the number of courses
+        $sql = "SELECT DISTINCT e.courseid, ue.userid
+          FROM {user_enrolments} ue
+          JOIN {enrol} e ON e.id = ue.enrolid
+          WHERE ue.status = :active
+            AND e.status = :enabled
+            AND ue.timestart < :now1 AND (ue.timeend = 0 OR ue.timeend > :now2)";
+        $now = round(time(), -2); // rounding helps caching in DB
+        $params = array('enabled' => ENROL_INSTANCE_ENABLED,
+            'active' => ENROL_USER_ACTIVE,
+            'now1' => $now, 'now2' => $now);
+        $cnt = 0;
+        $subsqls = array();
+        $enrolled = array();
+        foreach ($courseusers as $id => $userids) {
+            $enrolled[$id] = array();
+            if (count($userids)) {
+                list($sql2, $params2) = $DB->get_in_or_equal($userids, SQL_PARAMS_NAMED, 'userid'.$cnt.'_');
+                $subsqls[] = "(e.courseid = :courseid$cnt AND ue.userid ".$sql2.")";
+                $params = $params + array('courseid'.$cnt => $id) + $params2;
+                $cnt++;
+            }
+        }
+        if (count($subsqls)) {
+            $sql .= "AND (". join(' OR ', $subsqls).")";
+            $rs = $DB->get_recordset_sql($sql, $params);
+            foreach ($rs as $record) {
+                $enrolled[$record->courseid][] = $record->userid;
+            }
+            $rs->close();
         }
+        return $enrolled;
     }
 
     /**
index baa5ecd..d47d79e 100644 (file)
@@ -426,4 +426,110 @@ class coursecatlib_testcase extends advanced_testcase {
         $this->assertEquals(array($c3->id, $c6->id), array_keys($res));
         $this->assertEquals(2, coursecat::search_courses_count(array('search' => 'Математика'), array()));
     }
+
+    public function test_course_contacts() {
+        global $DB, $CFG;
+        $teacherrole = $DB->get_record('role', array('shortname'=>'editingteacher'));
+        $managerrole = $DB->get_record('role', array('shortname'=>'manager'));
+        $studentrole = $DB->get_record('role', array('shortname'=>'student'));
+        $oldcoursecontact = $CFG->coursecontact;
+
+        $CFG->coursecontact = $managerrole->id. ','. $teacherrole->id;
+
+        /**
+         * User is listed in course contacts for the course if he has one of the
+         * "course contact" roles ($CFG->coursecontact) AND is enrolled in the course.
+         * If the user has several roles only the highest is displayed.
+         */
+
+        // Test case:
+        //
+        // == Cat1 (user2 has teacher role)
+        //   == Cat2
+        //     -- course21 (user2 is enrolled as manager) | [Expected] Manager: F2 L2
+        //     -- course22 (user2 is enrolled as student) | [Expected] Teacher: F2 L2
+        //     == Cat4 (user2 has manager role)
+        //       -- course41 (user4 is enrolled as teacher, user5 is enrolled as manager) | [Expected] Manager: F5 L5, Teacher: F4 L4
+        //       -- course42 (user2 is enrolled as teacher) | [Expected] Manager: F2 L2
+        //   == Cat3 (user3 has manager role)
+        //     -- course31 (user3 is enrolled as student) | [Expected] Manager: F3 L3
+        //     -- course32                                | [Expected]
+        //   -- course11 (user1 is enrolled as teacher)   | [Expected] Teacher: F1 L1
+        //   -- course12 (user1 has teacher role)         | [Expected]
+        //                also user4 is enrolled as teacher but enrolment is not active
+        $category = $course = $enrol = $user = array();
+        $category[1] = coursecat::create(array('name' => 'Cat1'))->id;
+        $category[2] = coursecat::create(array('name' => 'Cat2', 'parent' => $category[1]))->id;
+        $category[3] = coursecat::create(array('name' => 'Cat3', 'parent' => $category[1]))->id;
+        $category[4] = coursecat::create(array('name' => 'Cat4', 'parent' => $category[2]))->id;
+        foreach (array(1,2,3,4) as $catid) {
+            foreach (array(1,2) as $courseid) {
+                $course[$catid][$courseid] = $this->getDataGenerator()->create_course(array('idnumber' => 'id'.$catid.$courseid,
+                    'category' => $category[$catid]))->id;
+                $enrol[$catid][$courseid] = $DB->get_record('enrol', array('courseid'=>$course[$catid][$courseid], 'enrol'=>'manual'), '*', MUST_EXIST);
+            }
+        }
+        foreach (array(1,2,3,4,5) as $userid) {
+            $user[$userid] = $this->getDataGenerator()->create_user(array('firstname' => 'F'.$userid, 'lastname' => 'L'.$userid))->id;
+        }
+
+        $manual = enrol_get_plugin('manual');
+
+        // Cat1 (user2 has teacher role)
+        role_assign($teacherrole->id, $user[2], context_coursecat::instance($category[1]));
+        // course21 (user2 is enrolled as manager)
+        $manual->enrol_user($enrol[2][1], $user[2], $managerrole->id);
+        // course22 (user2 is enrolled as student)
+        $manual->enrol_user($enrol[2][2], $user[2], $studentrole->id);
+        // Cat4 (user2 has manager role)
+        role_assign($managerrole->id, $user[2], context_coursecat::instance($category[4]));
+        // course41 (user4 is enrolled as teacher, user5 is enrolled as manager)
+        $manual->enrol_user($enrol[4][1], $user[4], $teacherrole->id);
+        $manual->enrol_user($enrol[4][1], $user[5], $managerrole->id);
+        // course42 (user2 is enrolled as teacher)
+        $manual->enrol_user($enrol[4][2], $user[2], $teacherrole->id);
+        // Cat3 (user3 has manager role)
+        role_assign($managerrole->id, $user[3], context_coursecat::instance($category[3]));
+        // course31 (user3 is enrolled as student)
+        $manual->enrol_user($enrol[3][1], $user[3], $studentrole->id);
+        // course11 (user1 is enrolled as teacher)
+        $manual->enrol_user($enrol[1][1], $user[1], $teacherrole->id);
+        // -- course12 (user1 has teacher role)
+        //                also user4 is enrolled as teacher but enrolment is not active
+        role_assign($teacherrole->id, $user[1], context_course::instance($course[1][2]));
+        $manual->enrol_user($enrol[1][2], $user[4], $teacherrole->id, 0, 0, ENROL_USER_SUSPENDED);
+
+        $allcourses = coursecat::get(0)->get_courses(array('recursive' => true, 'coursecontacts' => true, 'sort' => array('idnumber' => 1)));
+        // Simplify the list of contacts for each course (similar as renderer would do)
+        $contacts = array();
+        foreach (array(1,2,3,4) as $catid) {
+            foreach (array(1,2) as $courseid) {
+                $tmp = array();
+                foreach ($allcourses[$course[$catid][$courseid]]->get_course_contacts() as $contact) {
+                    $tmp[] = $contact['rolename']. ': '. $contact['username'];
+                }
+                $contacts[$catid][$courseid] = join(', ', $tmp);
+            }
+        }
+
+        // Assert:
+        //     -- course21 (user2 is enrolled as manager) | Manager: F2 L2
+        $this->assertEquals('Manager: F2 L2', $contacts[2][1]);
+        //     -- course22 (user2 is enrolled as student) | Teacher: F2 L2
+        $this->assertEquals('Teacher: F2 L2', $contacts[2][2]);
+        //       -- course41 (user4 is enrolled as teacher, user5 is enrolled as manager) | Manager: F5 L5, Teacher: F4 L4
+        $this->assertEquals('Manager: F5 L5, Teacher: F4 L4', $contacts[4][1]);
+        //       -- course42 (user2 is enrolled as teacher) | [Expected] Manager: F2 L2
+        $this->assertEquals('Manager: F2 L2', $contacts[4][2]);
+        //     -- course31 (user3 is enrolled as student) | Manager: F3 L3
+        $this->assertEquals('Manager: F3 L3', $contacts[3][1]);
+        //     -- course32                                |
+        $this->assertEquals('', $contacts[3][2]);
+        //   -- course11 (user1 is enrolled as teacher)   | Teacher: F1 L1
+        $this->assertEquals('Teacher: F1 L1', $contacts[1][1]);
+        //   -- course12 (user1 has teacher role)         |
+        $this->assertEquals('', $contacts[1][2]);
+
+        $CFG->coursecontact = $oldcoursecontact;
+    }
 }
\ No newline at end of file