From c22fb4bd4b7b30a5274116ba3ff19c498d787229 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Thu, 15 Aug 2019 14:00:15 +0800 Subject: [PATCH] MDL-64739 core_analytics: Contexts param for get_analysables_iterator --- analytics/classes/local/analyser/base.php | 33 +++++++++--- .../classes/local/analyser/by_course.php | 19 ++----- analytics/classes/local/analyser/sitewide.php | 3 +- analytics/upgrade.txt | 2 + lib/classes/analytics/analyser/users.php | 5 +- lib/tests/analysers_test.php | 51 +++++++++++++------ 6 files changed, 73 insertions(+), 40 deletions(-) diff --git a/analytics/classes/local/analyser/base.php b/analytics/classes/local/analyser/base.php index 0065f15ab85..729187f7d94 100644 --- a/analytics/classes/local/analyser/base.php +++ b/analytics/classes/local/analyser/base.php @@ -131,9 +131,10 @@ abstract class base { * to ease to implementation of get_analysables_iterator: get_iterator_sql and order_sql. * * @param string|null $action 'prediction', 'training' or null if no specific action needed. + * @param \context[] $contexts Only analysables that depend on the provided contexts. All analysables in the system if empty. * @return \Iterator */ - public function get_analysables_iterator(?string $action = null) { + public function get_analysables_iterator(?string $action = null, array $contexts = []) { debugging('Please overwrite get_analysables_iterator with your own implementation, we only keep this default implementation for backwards compatibility purposes with get_analysables(). note that $action param will @@ -431,9 +432,12 @@ abstract class base { * @param int $contextlevel The context level of the analysable * @param string|null $action * @param string|null $tablealias The table alias + * @param \context[] $contexts Only analysables that depend on the provided contexts. All analysables if empty. * @return array [0] => sql and [1] => params array */ - protected function get_iterator_sql(string $tablename, int $contextlevel, ?string $action = null, ?string $tablealias = null) { + protected function get_iterator_sql(string $tablename, int $contextlevel, ?string $action = null, ?string $tablealias = null, + array $contexts = []) { + global $DB; if (!$tablealias) { $tablealias = 'analysable'; @@ -452,13 +456,30 @@ abstract class base { $params = $params + ['action' => $action]; } - // Adding the 1 = 1 just to have the WHERE part so that all further conditions added by callers can be - // appended to $sql with and ' AND'. $sql = 'SELECT ' . $select . ' FROM {' . $tablename . '} ' . $tablealias . ' ' . $usedanalysablesjoin . ' - JOIN {context} ctx ON (ctx.contextlevel = :contextlevel AND ctx.instanceid = ' . $tablealias . '.id) - WHERE 1 = 1'; + JOIN {context} ctx ON (ctx.contextlevel = :contextlevel AND ctx.instanceid = ' . $tablealias . '.id) '; + + if (!$contexts) { + // Adding the 1 = 1 just to have the WHERE part so that all further conditions + // added by callers can be appended to $sql with and ' AND'. + $sql .= 'WHERE 1 = 1'; + } else { + + $contextsqls = []; + foreach ($contexts as $context) { + $paramkey1 = 'paramctxlike' . $context->id; + $paramkey2 = 'paramctxeq' . $context->id; + $contextsqls[] = $DB->sql_like('ctx.path', ':' . $paramkey1); + $contextsqls[] = 'ctx.path = :' . $paramkey2; + + // This includes the context itself. + $params[$paramkey1] = $context->path . '/%'; + $params[$paramkey2] = $context->path; + } + $sql .= 'WHERE (' . implode(' OR ', $contextsqls) . ')'; + } return [$sql, $params]; } diff --git a/analytics/classes/local/analyser/by_course.php b/analytics/classes/local/analyser/by_course.php index c0d308b0d70..7e6bc66164f 100644 --- a/analytics/classes/local/analyser/by_course.php +++ b/analytics/classes/local/analyser/by_course.php @@ -39,24 +39,13 @@ abstract class by_course extends base { * Return the list of courses to analyse. * * @param string|null $action 'prediction', 'training' or null if no specific action needed. + * @param \context[] $contexts Only analysables that depend on the provided contexts. All analysables in the system if empty. * @return \Iterator */ - public function get_analysables_iterator(?string $action = null) { + public function get_analysables_iterator(?string $action = null, array $contexts = []) { global $DB; - list($sql, $params) = $this->get_iterator_sql('course', CONTEXT_COURSE, $action, 'c'); - - // This will be updated to filter by context as part of MDL-64739. - if (!empty($this->options['filter'])) { - $courses = array(); - foreach ($this->options['filter'] as $courseid) { - $courses[$courseid] = intval($courseid); - } - - list($coursesql, $courseparams) = $DB->get_in_or_equal($courses, SQL_PARAMS_NAMED); - $sql .= " AND c.id $coursesql"; - $params = $params + $courseparams; - } + list($sql, $params) = $this->get_iterator_sql('course', CONTEXT_COURSE, $action, 'c', $contexts); $ordersql = $this->order_sql('sortorder', 'ASC', 'c'); @@ -76,4 +65,4 @@ abstract class by_course extends base { return \core_analytics\course::instance($record, $context); }); } -} \ No newline at end of file +} diff --git a/analytics/classes/local/analyser/sitewide.php b/analytics/classes/local/analyser/sitewide.php index da6ea71db82..28a4db3a4e9 100644 --- a/analytics/classes/local/analyser/sitewide.php +++ b/analytics/classes/local/analyser/sitewide.php @@ -39,9 +39,10 @@ abstract class sitewide extends base { * Return the list of analysables to analyse. * * @param string|null $action 'prediction', 'training' or null if no specific action needed. + * @param \context[] $contexts Ignored here. * @return \Iterator */ - public function get_analysables_iterator(?string $action = null) { + public function get_analysables_iterator(?string $action = null, array $contexts = []) { // We can safely ignore $action as we have 1 single analysable element in this analyser. return new \ArrayIterator([new \core_analytics\site()]); } diff --git a/analytics/upgrade.txt b/analytics/upgrade.txt index 38395c33be4..27c543e8ca3 100644 --- a/analytics/upgrade.txt +++ b/analytics/upgrade.txt @@ -30,6 +30,8 @@ information provided here is intended especially for developers. * Predictions flagged as "Not useful" in models whose targets use analysers that provide multiple samples per analysable (e.g. students at risk or no teaching) have been updated to "Incorrectly flagged". * \core_analytics\predictor::delete_output_dir has a new 2nd parameter, $uniquemodelid. +* Analyser's get_analysables_iterator and get_iterator_sql have a new $contexts parameter to limit the returned analysables to + the ones that depend on the provided contexts. === 3.7 === diff --git a/lib/classes/analytics/analyser/users.php b/lib/classes/analytics/analyser/users.php index 40ba586e762..76bf3942f0a 100644 --- a/lib/classes/analytics/analyser/users.php +++ b/lib/classes/analytics/analyser/users.php @@ -39,14 +39,15 @@ class users extends \core_analytics\local\analyser\base { * The site users are the analysable elements returned by this analyser. * * @param string|null $action 'prediction', 'training' or null if no specific action needed. + * @param \context[] $contexts Only analysables that depend on the provided contexts. All analysables in the system if empty. * @return \Iterator */ - public function get_analysables_iterator(?string $action = null) { + public function get_analysables_iterator(?string $action = null, array $contexts = []) { global $DB, $CFG; $siteadmins = explode(',', $CFG->siteadmins); - list($sql, $params) = $this->get_iterator_sql('user', CONTEXT_USER, $action, 'u'); + list($sql, $params) = $this->get_iterator_sql('user', CONTEXT_USER, $action, 'u', $contexts); $sql .= " AND u.deleted = :deleted AND u.confirmed = :confirmed AND u.suspended = :suspended"; $params = $params + ['deleted' => 0, 'confirmed' => 1, 'suspended' => 0]; diff --git a/lib/tests/analysers_test.php b/lib/tests/analysers_test.php index f52ab0f9636..2c6160326aa 100644 --- a/lib/tests/analysers_test.php +++ b/lib/tests/analysers_test.php @@ -48,16 +48,16 @@ class core_analytics_analysers_testcase extends advanced_testcase { public function test_courses_analyser() { $this->resetAfterTest(true); - $course = $this->getDataGenerator()->create_course(); - $coursecontext = \context_course::instance($course->id); + $course1 = $this->getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course1->id); $target = new test_target_shortname(); $analyser = new \core\analytics\analyser\courses(1, $target, [], [], []); - $analysable = new \core_analytics\course($course); + $analysable = new \core_analytics\course($course1); - $this->assertInstanceOf('\core_analytics\course', $analyser->get_sample_analysable($course->id)); + $this->assertInstanceOf('\core_analytics\course', $analyser->get_sample_analysable($course1->id)); - $this->assertInstanceOf('\context_course', $analyser->sample_access_context($course->id)); + $this->assertInstanceOf('\context_course', $analyser->sample_access_context($course1->id)); // Just 1 sample per course. $class = new ReflectionClass('\core\analytics\analyser\courses'); @@ -66,8 +66,8 @@ class core_analytics_analysers_testcase extends advanced_testcase { list($sampleids, $samplesdata) = $method->invoke($analyser, $analysable); $this->assertCount(1, $sampleids); $sampleid = reset($sampleids); - $this->assertEquals($course->id, $sampleid); - $this->assertEquals($course->fullname, $samplesdata[$sampleid]['course']->fullname); + $this->assertEquals($course1->id, $sampleid); + $this->assertEquals($course1->fullname, $samplesdata[$sampleid]['course']->fullname); $this->assertEquals($coursecontext, $samplesdata[$sampleid]['context']); // To compare it later. @@ -75,6 +75,16 @@ class core_analytics_analysers_testcase extends advanced_testcase { list($sampleids, $samplesdata) = $analyser->get_samples(array($sampleid)); $this->assertEquals($prevsampledata['context'], $samplesdata[$sampleid]['context']); $this->assertEquals($prevsampledata['course']->shortname, $samplesdata[$sampleid]['course']->shortname); + + // Context restriction. + $category1 = $this->getDataGenerator()->create_category(); + $category1context = \context_coursecat::instance($category1->id); + $category2 = $this->getDataGenerator()->create_category(); + $category2context = \context_coursecat::instance($category2->id); + $course2 = $this->getDataGenerator()->create_course(['category' => $category1->id]); + $course3 = $this->getDataGenerator()->create_course(['category' => $category2->id]); + $this->assertCount(2, $analyser->get_analysables_iterator(false, [$category1context, $category2context])); + } /** @@ -130,24 +140,24 @@ class core_analytics_analysers_testcase extends advanced_testcase { $this->resetAfterTest(true); - $course = $this->getDataGenerator()->create_course(); - $coursecontext = \context_course::instance($course->id); + $course1 = $this->getDataGenerator()->create_course(); + $course1context = \context_course::instance($course1->id); $user1 = $this->getDataGenerator()->create_user(); $user2 = $this->getDataGenerator()->create_user(); $user3 = $this->getDataGenerator()->create_user(); // Checking that suspended users are also included. - $this->getDataGenerator()->enrol_user($user1->id, $course->id, 'student'); - $this->getDataGenerator()->enrol_user($user2->id, $course->id, 'student', 'manual', 0, 0, ENROL_USER_SUSPENDED); - $this->getDataGenerator()->enrol_user($user3->id, $course->id, 'editingteacher'); - $enrol = $DB->get_record('enrol', array('courseid' => $course->id, 'enrol' => 'manual')); + $this->getDataGenerator()->enrol_user($user1->id, $course1->id, 'student'); + $this->getDataGenerator()->enrol_user($user2->id, $course1->id, 'student', 'manual', 0, 0, ENROL_USER_SUSPENDED); + $this->getDataGenerator()->enrol_user($user3->id, $course1->id, 'editingteacher'); + $enrol = $DB->get_record('enrol', array('courseid' => $course1->id, 'enrol' => 'manual')); $ue1 = $DB->get_record('user_enrolments', array('userid' => $user1->id, 'enrolid' => $enrol->id)); $ue2 = $DB->get_record('user_enrolments', array('userid' => $user2->id, 'enrolid' => $enrol->id)); $target = new test_target_shortname(); $analyser = new \core\analytics\analyser\student_enrolments(1, $target, [], [], []); - $analysable = new \core_analytics\course($course); + $analysable = new \core_analytics\course($course1); $this->assertInstanceOf('\core_analytics\course', $analyser->get_sample_analysable($ue1->id)); $this->assertInstanceOf('\context_course', $analyser->sample_access_context($ue1->id)); @@ -165,8 +175,8 @@ class core_analytics_analysers_testcase extends advanced_testcase { // Shouldn't matter which one we select. $sampleid = $ue1->id; $this->assertEquals($ue1, $samplesdata[$sampleid]['user_enrolments']); - $this->assertEquals($course->fullname, $samplesdata[$sampleid]['course']->fullname); - $this->assertEquals($coursecontext, $samplesdata[$sampleid]['context']); + $this->assertEquals($course1->fullname, $samplesdata[$sampleid]['course']->fullname); + $this->assertEquals($course1context, $samplesdata[$sampleid]['context']); $this->assertEquals($user1->firstname, $samplesdata[$sampleid]['user']->firstname); // To compare it later. @@ -176,6 +186,15 @@ class core_analytics_analysers_testcase extends advanced_testcase { $this->assertEquals($prevsampledata['context'], $samplesdata[$sampleid]['context']); $this->assertEquals($prevsampledata['course']->shortname, $samplesdata[$sampleid]['course']->shortname); $this->assertEquals($prevsampledata['user']->firstname, $samplesdata[$sampleid]['user']->firstname); + + // Context restriction. + $category1 = $this->getDataGenerator()->create_category(); + $category1context = \context_coursecat::instance($category1->id); + $category2 = $this->getDataGenerator()->create_category(); + $category2context = \context_coursecat::instance($category2->id); + $course2 = $this->getDataGenerator()->create_course(['category' => $category1->id]); + $course3 = $this->getDataGenerator()->create_course(['category' => $category2->id]); + $this->assertCount(2, $analyser->get_analysables_iterator(false, [$category1context, $category2context])); } /**