From 8bcf74e9bc8cd34780d14e00537ea1cdf0190608 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 12 Jun 2020 08:46:46 +0800 Subject: [PATCH 1/2] MDL-69026 user: Wrap sub-query in brackets It is perfectly valid to have a query like: Match None of the following: - Role is ANY of the following: -- 'Teacher' -- 'Editing teacher' -- 'Manager'; AND - Keyword is NONE of the following: -- 'Kevin' However, due to the way in which the query is constructed, this leads to a query which includes WHERE NOT ef.id IS NOT NULL AND NOT u.id IN (SELECT userid FROM {role_assignments} WHERE roleid IN (...) AND contextid IN (...)) AND NOT NOT (u.firstname || ' ' || u.lastname LIKE '%Kevin%') The use of NOT NOT is valid in Postgres, MariaDB, MySQL, and MSSQL, but not in Oracle. To counter this when the outer jointype is of type NONE, we must wrap each of the inner WHERE clauses in a set of brackets, which makes the query: WHERE NOT ef.id IS NOT NULL AND NOT (u.id IN (SELECT userid FROM {role_assignments} WHERE roleid IN (...) AND contextid IN (...))) AND NOT (NOT (u.firstname || ' ' || u.lastname LIKE '%Kevin%')) Whilst Oracle does not support the use of `AND NOT NOT ...`, it does support `AND NOT (NOT ...)` --- user/classes/table/participants_search.php | 8 +++++++ user/tests/table/participants_search_test.php | 23 ++++++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/user/classes/table/participants_search.php b/user/classes/table/participants_search.php index 33122ccc369..7946247f96b 100644 --- a/user/classes/table/participants_search.php +++ b/user/classes/table/participants_search.php @@ -283,6 +283,14 @@ class participants_search { case $this->filterset::JOINTYPE_NONE: $wherenot = ' NOT '; $wheresjoin = ' AND NOT '; + + // Some of the $where conditions may begin with `NOT` which results in `AND NOT NOT ...`. + // To prevent this from breaking on Oracle the inner WHERE clause is wrapped in brackets, making it + // `AND NOT (NOT ...)` which is valid in all DBs. + $wheres = array_map(function($where) { + return "({$where})"; + }, $wheres); + break; default: // Default to 'Any' jointype. diff --git a/user/tests/table/participants_search_test.php b/user/tests/table/participants_search_test.php index 66ce32c6bc7..2722357ccdc 100644 --- a/user/tests/table/participants_search_test.php +++ b/user/tests/table/participants_search_test.php @@ -3203,7 +3203,7 @@ class participants_search_test extends advanced_testcase { 'accesssince' => [ 'values' => ['-6 months'], 'jointype' => filter::JOINTYPE_ALL, - ], + ], ], 'jointype' => filter::JOINTYPE_NONE, 'count' => 1, @@ -3211,6 +3211,27 @@ class participants_search_test extends advanced_testcase { 'barbara.bennett', ], ], + 'NONE: Filterset combining several filter types and a double-negative on keyword' => (object) [ + 'jointype' => filter::JOINTYPE_NONE, + 'filterdata' => [ + // Note: This is a jointype NONE on the parent jointype NONE. + // The result therefore negated in this instance. + // Include Adam and Anthony. + 'keywords' => [ + 'values' => ['ant'], + 'jointype' => filter::JOINTYPE_NONE, + ], + // Excludes Tony. + 'status' => [ + 'values' => [ENROL_USER_SUSPENDED], + 'jointype' => filter::JOINTYPE_ALL, + ], + ], + 'count' => 1, + 'expectedusers' => [ + 'adam.ant', + ], + ], ], ], ]; From 8d5926beada20875b4fb24e9b5ddb17a6baf1677 Mon Sep 17 00:00:00 2001 From: Andrew Nicols Date: Fri, 12 Jun 2020 09:39:25 +0800 Subject: [PATCH 2/2] MDL-69026 user: Correct statuses => status in test --- user/tests/table/participants_search_test.php | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/user/tests/table/participants_search_test.php b/user/tests/table/participants_search_test.php index 2722357ccdc..89f720e39e5 100644 --- a/user/tests/table/participants_search_test.php +++ b/user/tests/table/participants_search_test.php @@ -1253,8 +1253,8 @@ class participants_search_test extends advanced_testcase { foreach ($usersdata as $username => $userdata) { $user = $this->getDataGenerator()->create_user(['username' => $username]); - if (array_key_exists('statuses', $userdata)) { - foreach ($userdata['statuses'] as $enrolmethod => $status) { + if (array_key_exists('status', $userdata)) { + foreach ($userdata['status'] as $enrolmethod => $status) { $this->getDataGenerator()->enrol_user($user->id, $course->id, 'student', $enrolmethod, 0, 0, $status); } } @@ -1304,27 +1304,27 @@ class participants_search_test extends advanced_testcase { 'Users with different enrolment statuses' => (object) [ 'users' => [ 'a' => [ - 'statuses' => [ + 'status' => [ 'manual' => ENROL_USER_ACTIVE, ] ], 'b' => [ - 'statuses' => [ + 'status' => [ 'self' => ENROL_USER_ACTIVE, ] ], 'c' => [ - 'statuses' => [ + 'status' => [ 'manual' => ENROL_USER_SUSPENDED, ] ], 'd' => [ - 'statuses' => [ + 'status' => [ 'self' => ENROL_USER_SUSPENDED, ] ], 'e' => [ - 'statuses' => [ + 'status' => [ 'manual' => ENROL_USER_ACTIVE, 'self' => ENROL_USER_SUSPENDED, ] @@ -1333,7 +1333,7 @@ class participants_search_test extends advanced_testcase { 'expect' => [ // Tests for jointype: ANY. 'ANY: No filter' => (object) [ - 'statuses' => [], + 'status' => [], 'jointype' => filter::JOINTYPE_ANY, 'count' => 5, 'expectedusers' => [ @@ -1345,7 +1345,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ANY: Filter on active only' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE], + 'status' => [ENROL_USER_ACTIVE], 'jointype' => filter::JOINTYPE_ANY, 'count' => 3, 'expectedusers' => [ @@ -1355,7 +1355,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ANY: Filter on suspended only' => (object) [ - 'statuses' => [ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ANY, 'count' => 3, 'expectedusers' => [ @@ -1365,7 +1365,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ANY: Filter on multiple statuses' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ANY, 'count' => 5, 'expectedusers' => [ @@ -1379,7 +1379,7 @@ class participants_search_test extends advanced_testcase { // Tests for jointype: ALL. 'ALL: No filter' => (object) [ - 'statuses' => [], + 'status' => [], 'jointype' => filter::JOINTYPE_ALL, 'count' => 5, 'expectedusers' => [ @@ -1391,7 +1391,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ALL: Filter on active only' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE], + 'status' => [ENROL_USER_ACTIVE], 'jointype' => filter::JOINTYPE_ALL, 'count' => 3, 'expectedusers' => [ @@ -1401,7 +1401,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ALL: Filter on suspended only' => (object) [ - 'statuses' => [ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ALL, 'count' => 3, 'expectedusers' => [ @@ -1411,7 +1411,7 @@ class participants_search_test extends advanced_testcase { ], ], 'ALL: Filter on multiple statuses' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ALL, 'count' => 1, 'expectedusers' => [ @@ -1421,7 +1421,7 @@ class participants_search_test extends advanced_testcase { // Tests for jointype: NONE. 'NONE: No filter' => (object) [ - 'statuses' => [], + 'status' => [], 'jointype' => filter::JOINTYPE_NONE, 'count' => 5, 'expectedusers' => [ @@ -1433,7 +1433,7 @@ class participants_search_test extends advanced_testcase { ], ], 'NONE: Filter on active only' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE], + 'status' => [ENROL_USER_ACTIVE], 'jointype' => filter::JOINTYPE_NONE, 'count' => 3, 'expectedusers' => [ @@ -1443,7 +1443,7 @@ class participants_search_test extends advanced_testcase { ], ], 'NONE: Filter on suspended only' => (object) [ - 'statuses' => [ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_NONE, 'count' => 3, 'expectedusers' => [ @@ -1453,7 +1453,7 @@ class participants_search_test extends advanced_testcase { ], ], 'NONE: Filter on multiple statuses' => (object) [ - 'statuses' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], + 'status' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_NONE, 'count' => 0, 'expectedusers' => [], @@ -1467,7 +1467,7 @@ class participants_search_test extends advanced_testcase { foreach ($testdata->expect as $expectname => $expectdata) { $finaltests["{$testname} => {$expectname}"] = [ 'users' => $testdata->users, - 'statuses' => $expectdata->statuses, + 'status' => $expectdata->status, 'jointype' => $expectdata->jointype, 'count' => $expectdata->count, 'expectedusers' => $expectdata->expectedusers, @@ -3044,8 +3044,8 @@ class participants_search_test extends advanced_testcase { 'jointype' => filter::JOINTYPE_ALL, ], // Match Sarah only. - 'statuses' => [ - 'values' => ['active', 'suspended'], + 'status' => [ + 'values' => [ENROL_USER_ACTIVE, ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ALL, ], // Match Colin only. @@ -3119,8 +3119,8 @@ class participants_search_test extends advanced_testcase { 'jointype' => filter::JOINTYPE_NONE, ], // Exclude Colin and Tony. - 'statuses' => [ - 'values' => ['active'], + 'status' => [ + 'values' => [ENROL_USER_ACTIVE], 'jointype' => filter::JOINTYPE_ALL, ], // Exclude Barbara. @@ -3190,8 +3190,8 @@ class participants_search_test extends advanced_testcase { 'jointype' => filter::JOINTYPE_NONE, ], // Excludes Colin, Tony and Sarah. - 'statuses' => [ - 'values' => ['suspended'], + 'status' => [ + 'values' => [ENROL_USER_SUSPENDED], 'jointype' => filter::JOINTYPE_ALL, ], // Excludes Adam, Colin, Tony, Sarah, Morgan and Jonathan.