From 2d0c5c46b469bef7a2481f29e51ff6529772fa81 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Mon, 24 Apr 2023 12:11:11 +0100 Subject: [PATCH 1/7] MDL-77965 competency: Correct order of expectations The expected value was being passed second which meant that the messages sent by PHPUnit when this fails are the wrong way round. --- competency/tests/privacy/provider_test.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/competency/tests/privacy/provider_test.php b/competency/tests/privacy/provider_test.php index 6ec1f8e03a1..66f0e4b84c7 100644 --- a/competency/tests/privacy/provider_test.php +++ b/competency/tests/privacy/provider_test.php @@ -46,6 +46,7 @@ use core_competency\privacy\provider; * @copyright 2018 Frédéric Massart * @author Frédéric Massart * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_competency\privacy\provider */ class provider_test extends provider_testcase { @@ -2598,7 +2599,7 @@ class provider_test extends provider_testcase { $expectedids = array_unique(array_map(function($item) { return $item instanceof \context ? $item->id : $id; }, $expectedcontextsorids)); - $this->assert_array_match($contextids, $expectedids); + $this->assert_array_match($expectedids, $contextids); } /** From 6de5117a81a2669ff1ed371f84dda5a096d8a5a1 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Mon, 24 Apr 2023 14:47:38 +0100 Subject: [PATCH 2/7] MDL-77965 competency: Improve query performance and data This change removes LEFT JOINS to improve the performance when run against Moodle sites with very large data sets. It will also cause some additional contexts to be returned: 1. When a user has modified a user_evidence_competency without modifying its associated user_evidence record --- competency/classes/privacy/provider.php | 248 +++++++++++++++------ competency/tests/privacy/provider_test.php | 18 +- 2 files changed, 200 insertions(+), 66 deletions(-) diff --git a/competency/classes/privacy/provider.php b/competency/classes/privacy/provider.php index 534fc785f9d..a8911e62615 100644 --- a/competency/classes/privacy/provider.php +++ b/competency/classes/privacy/provider.php @@ -223,16 +223,28 @@ class provider implements $sql = " SELECT DISTINCT ctx.id FROM {context} ctx - JOIN {" . competency_framework::TABLE . "} cf - ON cf.contextid = ctx.id - LEFT JOIN {" . competency::TABLE . "} c - ON c.competencyframeworkid = cf.id - LEFT JOIN {" . related_competency::TABLE . "} cr - ON cr.competencyid = c.id - WHERE cf.usermodified = :userid1 - OR c.usermodified = :userid2 - OR cr.usermodified = :userid3"; - $params = ['userid1' => $userid, 'userid2' => $userid, 'userid3' => $userid]; + JOIN {" . competency_framework::TABLE . "} cf ON cf.contextid = ctx.id + WHERE cf.usermodified = :userid1"; + $params = ['userid1' => $userid]; + $contextlist->add_from_sql($sql, $params); + + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . competency_framework::TABLE . "} cf ON cf.contextid = ctx.id + JOIN {" . competency::TABLE . "} c ON c.competencyframeworkid = cf.id + WHERE c.usermodified = :userid2"; + $params = ['userid2' => $userid]; + $contextlist->add_from_sql($sql, $params); + + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . competency_framework::TABLE . "} cf ON cf.contextid = ctx.id + JOIN {" . competency::TABLE . "} c ON c.competencyframeworkid = cf.id + JOIN {" . related_competency::TABLE . "} cr ON cr.competencyid = c.id + WHERE cr.usermodified = :userid3"; + $params = ['userid3' => $userid]; $contextlist->add_from_sql($sql, $params); // Find the contexts of the templates, and related data, modified by the user. @@ -241,16 +253,30 @@ class provider implements FROM {context} ctx JOIN {" . template::TABLE . "} tpl ON tpl.contextid = ctx.id - LEFT JOIN {" . template_cohort::TABLE . "} tch + WHERE tpl.usermodified = :userid1"; + $params = ['userid1' => $userid]; + $contextlist->add_from_sql($sql, $params); + + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . template::TABLE . "} tpl + ON tpl.contextid = ctx.id + JOIN {" . template_cohort::TABLE . "} tch ON tch.templateid = tpl.id - AND tch.usermodified = :userid2 - LEFT JOIN {" . template_competency::TABLE . "} tc + AND tch.usermodified = :userid2"; + $params = ['userid2' => $userid]; + $contextlist->add_from_sql($sql, $params); + + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . template::TABLE . "} tpl + ON tpl.contextid = ctx.id + JOIN {" . template_competency::TABLE . "} tc ON tc.templateid = tpl.id - AND tc.usermodified = :userid3 - WHERE tpl.usermodified = :userid1 - OR tch.id IS NOT NULL - OR tc.id IS NOT NULL"; - $params = ['userid1' => $userid, 'userid2' => $userid, 'userid3' => $userid]; + AND tc.usermodified = :userid3"; + $params = ['userid3' => $userid]; $contextlist->add_from_sql($sql, $params); // Find the possible course contexts. @@ -302,21 +328,52 @@ class provider implements JOIN {context} ctx ON ctx.instanceid = p.userid AND ctx.contextlevel = :userlevel - LEFT JOIN {" . plan_competency::TABLE . "} pc - ON pc.planid = p.id - AND pc.usermodified = :userid3 - LEFT JOIN {" . user_competency_plan::TABLE . "} upc - ON upc.planid = p.id - AND upc.usermodified = :userid4 - WHERE p.usermodified = :userid1 - OR p.reviewerid = :userid2 - OR pc.id IS NOT NULL - OR upc.id IS NOT NULL"; + WHERE p.usermodified = :userid1"; $params = [ 'userlevel' => CONTEXT_USER, 'userid1' => $userid, + ]; + + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {" . plan::TABLE . "} p + JOIN {context} ctx + ON ctx.instanceid = p.userid + AND ctx.contextlevel = :userlevel + WHERE p.reviewerid = :userid2"; + $params = [ + 'userlevel' => CONTEXT_USER, 'userid2' => $userid, + ]; + + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {" . plan::TABLE . "} p + JOIN {context} ctx + ON ctx.instanceid = p.userid + AND ctx.contextlevel = :userlevel + JOIN {" . plan_competency::TABLE . "} pc + ON pc.planid = p.id + AND pc.usermodified = :userid3"; + $params = [ + 'userlevel' => CONTEXT_USER, 'userid3' => $userid, + ]; + + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {" . plan::TABLE . "} p + JOIN {context} ctx + ON ctx.instanceid = p.userid + AND ctx.contextlevel = :userlevel + JOIN {" . user_competency_plan::TABLE . "} upc + ON upc.planid = p.id + AND upc.usermodified = :userid4"; + $params = [ + 'userlevel' => CONTEXT_USER, 'userid4' => $userid, ]; $contextlist->add_from_sql($sql, $params); @@ -325,32 +382,78 @@ class provider implements $sql = " SELECT DISTINCT ctx.id FROM {context} ctx - LEFT JOIN {" . user_competency::TABLE . "} uc + JOIN {" . user_competency::TABLE . "} uc ON uc.userid = ctx.instanceid AND ctx.contextlevel = :userlevel1 - LEFT JOIN {" . evidence::TABLE . "} e - ON e.usercompetencyid = uc.id - AND (e.usermodified = :userid3 OR e.actionuserid = :userid4) - LEFT JOIN {" . user_evidence::TABLE . "} ue - ON ue.userid = ctx.instanceid - AND ctx.contextlevel = :userlevel2 - AND ue.usermodified = :userid5 - LEFT JOIN {" . user_evidence_competency::TABLE . "} uec - ON uec.userevidenceid = ue.id - AND uec.usermodified = :userid6 - WHERE uc.usermodified = :userid1 - OR uc.reviewerid = :userid2 - OR e.id IS NOT NULL - OR ue.id IS NOT NULL - OR uec.id IS NOT NULL"; + WHERE uc.usermodified = :userid1"; $params = [ 'userlevel1' => CONTEXT_USER, - 'userlevel2' => CONTEXT_USER, 'userid1' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_competency::TABLE . "} uc + ON uc.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel1 + WHERE uc.reviewerid = :userid2"; + $params = [ + 'userlevel1' => CONTEXT_USER, 'userid2' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_competency::TABLE . "} uc + ON uc.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel1 + JOIN {" . evidence::TABLE . "} e + ON e.usercompetencyid = uc.id + AND e.usermodified = :userid3"; + $params = [ + 'userlevel1' => CONTEXT_USER, 'userid3' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_competency::TABLE . "} uc + ON uc.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel1 + JOIN {" . evidence::TABLE . "} e + ON e.usercompetencyid = uc.id + AND e.actionuserid = :userid4"; + $params = [ + 'userlevel1' => CONTEXT_USER, 'userid4' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_evidence::TABLE . "} ue + ON ue.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel2 + AND ue.usermodified = :userid5"; + $params = [ + 'userlevel2' => CONTEXT_USER, 'userid5' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_evidence::TABLE . "} ue + ON ue.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel2 + JOIN {" . user_evidence_competency::TABLE . "} uec + ON uec.userevidenceid = ue.id + AND uec.usermodified = :userid6"; + $params = [ + 'userlevel2' => CONTEXT_USER, 'userid6' => $userid, ]; $contextlist->add_from_sql($sql, $params); @@ -360,33 +463,48 @@ class provider implements $sql = " SELECT DISTINCT ctx.id FROM {context} ctx - LEFT JOIN {" . plan::TABLE . "} p + JOIN {" . plan::TABLE . "} p ON p.userid = ctx.instanceid AND ctx.contextlevel = :userlevel1 - LEFT JOIN {" . user_competency::TABLE . "} uc - ON uc.userid = ctx.instanceid - AND ctx.contextlevel = :userlevel2 - AND uc.userid = :userid2 - LEFT JOIN {" . user_evidence::TABLE . "} ue - ON ue.userid = ctx.instanceid - AND ctx.contextlevel = :userlevel3 - AND ue.userid = :userid3 - LEFT JOIN {" . user_competency_course::TABLE . "} ucc - ON ucc.courseid = ctx.instanceid - AND ctx.contextlevel = :courselevel - AND ucc.userid = :userid4 - WHERE p.userid = :userid1 - OR uc.id IS NOT NULL - OR ue.id IS NOT NULL - OR ucc.id IS NOT NULL"; + WHERE p.userid = :userid1"; $params = [ 'userlevel1' => CONTEXT_USER, - 'userlevel2' => CONTEXT_USER, - 'userlevel3' => CONTEXT_USER, - 'courselevel' => CONTEXT_COURSE, 'userid1' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_competency::TABLE . "} uc + ON uc.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel2 + AND uc.userid = :userid2"; + $params = [ + 'userlevel2' => CONTEXT_USER, 'userid2' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_evidence::TABLE . "} ue + ON ue.userid = ctx.instanceid + AND ctx.contextlevel = :userlevel3 + AND ue.userid = :userid3"; + $params = [ + 'userlevel3' => CONTEXT_USER, 'userid3' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {" . user_competency_course::TABLE . "} ucc + ON ucc.courseid = ctx.instanceid + AND ctx.contextlevel = :courselevel + AND ucc.userid = :userid4"; + $params = [ + 'courselevel' => CONTEXT_COURSE, 'userid4' => $userid, ]; $contextlist->add_from_sql($sql, $params); diff --git a/competency/tests/privacy/provider_test.php b/competency/tests/privacy/provider_test.php index 66f0e4b84c7..8ec02a78e05 100644 --- a/competency/tests/privacy/provider_test.php +++ b/competency/tests/privacy/provider_test.php @@ -667,6 +667,7 @@ class provider_test extends provider_testcase { $u2 = $dg->create_user(); $u3 = $dg->create_user(); $u4 = $dg->create_user(); + $u5 = $dg->create_user(); $c1ctx = \context_course::instance($c1->id); $u1ctx = \context_user::instance($u1->id); @@ -682,30 +683,45 @@ class provider_test extends provider_testcase { $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), []); $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), []); $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), []); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), []); $ccg->create_plan(['userid' => $u1->id]); $this->assert_contextlist(provider::get_contexts_for_userid($u1->id), [$u1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), []); $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), []); $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), []); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), []); $ccg->create_user_competency(['userid' => $u2->id, 'competencyid' => $comp1->get('id')]); $this->assert_contextlist(provider::get_contexts_for_userid($u1->id), [$u1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), [$u2ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), []); $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), []); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), []); $ccg->create_user_competency_course(['userid' => $u3->id, 'competencyid' => $comp1->get('id'), 'courseid' => $c1->id]); $this->assert_contextlist(provider::get_contexts_for_userid($u1->id), [$u1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), [$u2ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), [$c1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), []); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), []); - $ccg->create_user_evidence(['userid' => $u4->id]); + $ue = $ccg->create_user_evidence(['userid' => $u4->id]); $this->assert_contextlist(provider::get_contexts_for_userid($u1->id), [$u1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), [$u2ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), [$c1ctx]); $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), [$u4ctx]); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), []); + + // A user editing a context relationship. + $this->setUser($u5); + $ccg->create_user_evidence_competency(['userevidenceid' => $ue->get('id'), 'competencyid' => $comp1->get('id')]); + $this->setAdminUser(); + $this->assert_contextlist(provider::get_contexts_for_userid($u1->id), [$u1ctx]); + $this->assert_contextlist(provider::get_contexts_for_userid($u2->id), [$u2ctx]); + $this->assert_contextlist(provider::get_contexts_for_userid($u3->id), [$c1ctx]); + $this->assert_contextlist(provider::get_contexts_for_userid($u4->id), [$u4ctx]); + $this->assert_contextlist(provider::get_contexts_for_userid($u5->id), [$u4ctx]); } public function test_get_users_in_context_with_actual_data_and_actual_data_is_goooood() { From fed4c20d68688f33cff3e27f0e42c39b54d7e332 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Thu, 20 Apr 2023 11:56:44 +0100 Subject: [PATCH 3/7] MDL-77965 calendar: Improve performance of data extract The UNION caused the query to be run in a way that is very inefficient on MySQL, separating the queries causes each of them to run in a much more efficient form. Any duplicated will be filtered out on the PHP side instead of in the database. On large Moodle sites this is preferable as the extract is likely to be performed on a server dedicated to running the Moodle cron and so there will be less of effect on resources that are used to serve end users. --- calendar/classes/privacy/provider.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/calendar/classes/privacy/provider.php b/calendar/classes/privacy/provider.php index 63f4b72ed04..d15b6b81823 100644 --- a/calendar/classes/privacy/provider.php +++ b/calendar/classes/privacy/provider.php @@ -118,9 +118,9 @@ class provider implements (e.courseid = ctx.instanceid AND e.eventtype = 'course' AND ctx.contextlevel = :coursecontext) OR (e.courseid = ctx.instanceid AND e.eventtype = 'group' AND ctx.contextlevel = :groupcontext) OR (e.userid = ctx.instanceid AND e.eventtype = 'user' AND ctx.contextlevel = :usercontext) - WHERE e.userid = :cuserid - UNION - SELECT ctx.id + WHERE e.userid = :cuserid"; + $contextlist->add_from_sql($sql, $params); + $sql = "SELECT ctx.id FROM {context} ctx JOIN {course_modules} cm ON cm.id = ctx.instanceid AND ctx.contextlevel = :modulecontext JOIN {modules} m ON m.id = cm.module From 7a6e47070d9c9cf2e204c5f9f4d10ef0967b5145 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Fri, 21 Apr 2023 13:58:38 +0100 Subject: [PATCH 4/7] MDL-77965 notes: Improve data export performance The UNION caused the query to be run in a way that is very inefficient on MySQL, separating the queries causes each of them to run in a much more efficient form. Any duplicated will be filtered out on the PHP side instead of in the database. On large Moodle sites this is preferable as the extract is likely to be performed on a server dedicated to running the Moodle cron and so there will be less of effect on resources that are used to serve end users. --- notes/classes/privacy/provider.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/notes/classes/privacy/provider.php b/notes/classes/privacy/provider.php index 1a661beb261..6cbe8ce5714 100644 --- a/notes/classes/privacy/provider.php +++ b/notes/classes/privacy/provider.php @@ -95,9 +95,16 @@ class provider implements FROM {context} c INNER JOIN {post} p ON p.courseid = c.instanceid AND c.contextlevel = :contextcoursewrittenby WHERE p.module = 'notes' - AND p.usermodified = :usermodified - UNION - SELECT c.id + AND p.usermodified = :usermodified"; + + $params = [ + 'contextcoursewrittenby' => CONTEXT_COURSE, + 'usermodified' => $userid, + ]; + + $contextlist->add_from_sql($sql, $params); + + $sql = "SELECT c.id FROM {context} c INNER JOIN {post} p ON p.courseid = c.instanceid AND c.contextlevel = :contextcoursewrittenfor WHERE p.module = 'notes' @@ -105,8 +112,6 @@ class provider implements AND p.publishstate {$publishstatesql}"; $params = [ - 'contextcoursewrittenby' => CONTEXT_COURSE, - 'usermodified' => $userid, 'contextcoursewrittenfor' => CONTEXT_COURSE, 'userid' => $userid ]; From 8f995f9adc5804b80f96608354d0f959eefce87b Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Mon, 24 Apr 2023 10:26:35 +0100 Subject: [PATCH 5/7] MDL-77965 grade: Improve data export performance The existing query was inefficient using an index scan and index merges. This change splits the query up into many individually much more efficient queries. This should significantly improve performance on large sites. --- grade/classes/privacy/provider.php | 93 +++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 26 deletions(-) diff --git a/grade/classes/privacy/provider.php b/grade/classes/privacy/provider.php index 070af6fde0a..aad1f0d62f6 100644 --- a/grade/classes/privacy/provider.php +++ b/grade/classes/privacy/provider.php @@ -170,39 +170,80 @@ class provider implements $sql = " SELECT DISTINCT ctx.id FROM {context} ctx - LEFT JOIN {grade_outcomes_history} goh ON goh.loggeduser = :userid1 AND ( - (goh.courseid > 0 AND goh.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel1) - OR ((goh.courseid IS NULL OR goh.courseid < 1) AND ctx.id = :syscontextid1) - ) - LEFT JOIN {grade_categories_history} gch ON gch.loggeduser = :userid2 AND ( - gch.courseid = ctx.instanceid - AND ctx.contextlevel = :courselevel2 - ) - LEFT JOIN {grade_items_history} gih ON gih.loggeduser = :userid3 AND ( - gih.courseid = ctx.instanceid - AND ctx.contextlevel = :courselevel3 - ) - LEFT JOIN {scale_history} sh - ON (sh.userid = :userid4 OR sh.loggeduser = :userid5) - AND ( - (sh.courseid > 0 AND sh.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel4) - OR (sh.courseid = 0 AND ctx.id = :syscontextid2) - ) - WHERE goh.id IS NOT NULL - OR gch.id IS NOT NULL - OR gih.id IS NOT NULL - OR sh.id IS NOT NULL"; + JOIN {grade_outcomes_history} goh ON goh.loggeduser = :userid1 AND goh.courseid > 0 + AND goh.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel1"; + $params = [ + 'courselevel1' => CONTEXT_COURSE, + 'userid1' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {grade_outcomes_history} goh ON goh.loggeduser = :userid1 + AND (goh.courseid IS NULL OR goh.courseid < 1) AND ctx.id = :syscontextid1"; $params = [ 'syscontextid1' => SYSCONTEXTID, - 'syscontextid2' => SYSCONTEXTID, 'courselevel1' => CONTEXT_COURSE, - 'courselevel2' => CONTEXT_COURSE, - 'courselevel3' => CONTEXT_COURSE, - 'courselevel4' => CONTEXT_COURSE, 'userid1' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {grade_categories_history} gch ON gch.loggeduser = :userid2 + AND gch.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel2"; + $params = [ + 'courselevel2' => CONTEXT_COURSE, 'userid2' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {grade_items_history} gih ON gih.loggeduser = :userid3 + AND gih.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel3"; + $params = [ + 'courselevel3' => CONTEXT_COURSE, 'userid3' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {scale_history} sh ON sh.userid = :userid4 + AND sh.courseid > 0 AND sh.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel4"; + $params = [ + 'courselevel4' => CONTEXT_COURSE, 'userid4' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {scale_history} sh ON sh.loggeduser = :userid5 + AND sh.courseid > 0 AND sh.courseid = ctx.instanceid AND ctx.contextlevel = :courselevel4"; + $params = [ + 'courselevel4' => CONTEXT_COURSE, + 'userid5' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {scale_history} sh ON sh.userid = :userid4 AND sh.courseid = 0 AND ctx.id = :syscontextid2"; + $params = [ + 'syscontextid2' => SYSCONTEXTID, + 'userid4' => $userid, + ]; + $contextlist->add_from_sql($sql, $params); + $sql = " + SELECT DISTINCT ctx.id + FROM {context} ctx + JOIN {scale_history} sh ON sh.loggeduser = :userid5 AND sh.courseid = 0 AND ctx.id = :syscontextid2"; + $params = [ + 'syscontextid2' => SYSCONTEXTID, 'userid5' => $userid, ]; $contextlist->add_from_sql($sql, $params); From ce125ae0e69cd63743af64bfcae7b3fa750591bb Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Mon, 24 Apr 2023 10:48:17 +0100 Subject: [PATCH 6/7] MDL-77965 quiz: Improve efficiency of the data export Before this change the query used an index scan to perform the export, this could be really expensive on the database. After the change the the query will use far more efficient joins, on a large MySQL instance this can take minutes off of the query time. In additon the query was returning two columns that are not used in the function. The layout column was causing the UNION to fail on Oracle as it cannot compare text columns, removing these columns from the returned data should further increase performance further. --- mod/quiz/classes/privacy/provider.php | 57 ++++++++++++++++++++------- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/mod/quiz/classes/privacy/provider.php b/mod/quiz/classes/privacy/provider.php index c0ca490e9ab..70b560fb1f4 100644 --- a/mod/quiz/classes/privacy/provider.php +++ b/mod/quiz/classes/privacy/provider.php @@ -480,31 +480,60 @@ class provider implements $userid = $contextlist->get_user()->id; list($contextsql, $contextparams) = $DB->get_in_or_equal($contextlist->get_contextids(), SQL_PARAMS_NAMED); - $qubaid = \core_question\privacy\provider::get_related_question_usages_for_user('rel', 'mod_quiz', 'qa.uniqueid', $userid); + $qubaid1 = \core_question\privacy\provider::get_related_question_usages_for_user( + 'rel1', + 'mod_quiz', + 'qa.uniqueid', + $userid + ); + $qubaid2 = \core_question\privacy\provider::get_related_question_usages_for_user( + 'rel2', + 'mod_quiz', + 'qa.uniqueid', + $userid + ); + + // The layout column causes the union in the following query to fail on Oracle, it also appears to not be used. + // So we can filter the return values to be only those used to generate the data, this will have the benefit + // improving performance on all databases as we will no longer be returning a text field for each row. + $attemptfields = 'qa.id, qa.quiz, qa.userid, qa.attempt, qa.uniqueid, qa.preview, qa.state, qa.timestart, ' . + 'qa.timefinish, qa.timemodified, qa.timemodifiedoffline, qa.timecheckstate, qa.sumgrades, ' . + 'qa.gradednotificationsenttime'; $sql = "SELECT c.id AS contextid, cm.id AS cmid, - qa.* + $attemptfields FROM {context} c - JOIN {course_modules} cm ON cm.id = c.instanceid AND c.contextlevel = :contextlevel + JOIN {course_modules} cm ON cm.id = c.instanceid AND c.contextlevel = :contextlevel1 JOIN {modules} m ON m.id = cm.module AND m.name = 'quiz' JOIN {quiz} q ON q.id = cm.instance JOIN {quiz_attempts} qa ON qa.quiz = q.id - " . $qubaid->from. " - WHERE ( - qa.userid = :qauserid OR - " . $qubaid->where() . " - ) AND qa.preview = 0 + " . $qubaid1->from. " + WHERE qa.userid = :qauserid AND qa.preview = 0 + UNION + SELECT + c.id AS contextid, + cm.id AS cmid, + $attemptfields + FROM {context} c + JOIN {course_modules} cm ON cm.id = c.instanceid AND c.contextlevel = :contextlevel2 + JOIN {modules} m ON m.id = cm.module AND m.name = 'quiz' + JOIN {quiz} q ON q.id = cm.instance + JOIN {quiz_attempts} qa ON qa.quiz = q.id + " . $qubaid2->from. " + WHERE " . $qubaid2->where() . " AND qa.preview = 0 "; $params = array_merge( - [ - 'contextlevel' => CONTEXT_MODULE, - 'qauserid' => $userid, - ], - $qubaid->from_where_params() - ); + [ + 'contextlevel1' => CONTEXT_MODULE, + 'contextlevel2' => CONTEXT_MODULE, + 'qauserid' => $userid, + ], + $qubaid1->from_where_params(), + $qubaid2->from_where_params(), + ); $attempts = $DB->get_recordset_sql($sql, $params); foreach ($attempts as $attempt) { From af1e548adcea8a24287baafa73a4cf1748a25210 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Fri, 23 Feb 2024 09:28:51 +0000 Subject: [PATCH 7/7] MDL-77965 unit tests: Add covers annotation --- calendar/tests/privacy/provider_test.php | 1 + grade/tests/privacy/provider_test.php | 1 + mod/quiz/tests/privacy/provider_test.php | 1 + notes/tests/privacy/provider_test.php | 1 + 4 files changed, 4 insertions(+) diff --git a/calendar/tests/privacy/provider_test.php b/calendar/tests/privacy/provider_test.php index 0d4b57a908a..12f8ac248cf 100644 --- a/calendar/tests/privacy/provider_test.php +++ b/calendar/tests/privacy/provider_test.php @@ -42,6 +42,7 @@ use core_privacy\local\request\approved_userlist; * * @copyright 2018 Zig Tan * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_calendar\privacy\provider */ class provider_test extends provider_testcase { diff --git a/grade/tests/privacy/provider_test.php b/grade/tests/privacy/provider_test.php index 58990e2c20e..27c3ea8a280 100644 --- a/grade/tests/privacy/provider_test.php +++ b/grade/tests/privacy/provider_test.php @@ -44,6 +44,7 @@ require_once($CFG->libdir . '/gradelib.php'); * @copyright 2018 Frédéric Massart * @author Frédéric Massart * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_grades\privacy\provider */ class provider_test extends provider_testcase { diff --git a/mod/quiz/tests/privacy/provider_test.php b/mod/quiz/tests/privacy/provider_test.php index 559ea67d732..742cb6c6726 100644 --- a/mod/quiz/tests/privacy/provider_test.php +++ b/mod/quiz/tests/privacy/provider_test.php @@ -41,6 +41,7 @@ require_once($CFG->dirroot . '/question/tests/privacy_helper.php'); * @package mod_quiz * @copyright 2018 Andrew Nicols * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \mod_quiz\privacy\provider */ class provider_test extends \core_privacy\tests\provider_testcase { diff --git a/notes/tests/privacy/provider_test.php b/notes/tests/privacy/provider_test.php index e56079bc4f3..632e0ba7f9f 100644 --- a/notes/tests/privacy/provider_test.php +++ b/notes/tests/privacy/provider_test.php @@ -39,6 +39,7 @@ use core_privacy\local\request\approved_userlist; * * @copyright 2018 Zig Tan * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \core_notes\privacy\provider */ class provider_test extends \core_privacy\tests\provider_testcase {