From e83452a8559589b16ebdcdaddc70986c9ddbafb0 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 081bfd0a9d4cebb8c696e4536638d34f7e1f3804 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 18a964324b66c97dcadf5bf3693251276fa775be 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 8a95d08ff334c529cac301068fe3373e98679a54 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 32a915ecd264ac17b6333128e174878c9b10c03c 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 79150cf9bb1edada8297c1f802630645d67c797c 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 6ff5e7aabdee68005af54b97d5233359222de01a 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 {