MDL-68348 user: User filter match types support - keywords/last access

The last access implementation also fixes an existing bug,
where it was assumed never accessed would be 0, when it also needed to
handle null to return correct results. Related userlib unit tests also
updated to reflect this, as well as some incorrect comment wording.
This commit is contained in:
Michael Hawkins
2020-05-25 18:35:08 +08:00
parent 4157e2d4f3
commit 25d9dabdcf
4 changed files with 449 additions and 48 deletions
+92 -14
View File
@@ -32,6 +32,10 @@ use moodle_recordset;
use stdClass;
use user_picture;
defined('MOODLE_INTERNAL') || die;
require_once($CFG->dirroot . '/user/lib.php');
/**
* Class used to fetch participants based on a filterset.
*
@@ -127,8 +131,20 @@ class participants_search {
* @return array
*/
protected function get_participants_sql(string $additionalwhere, array $additionalparams): array {
$isfrontpage = ($this->course->id == SITEID);
$accesssince = $this->filterset->has_filter('accesssince') ? $this->filterset->get_filter('accesssince')->current() : 0;
$isfrontpage = ($this->courseid == SITEID);
$accesssince = 0;
// Whether to match on users who HAVE accessed since the given time (ie false is 'inactive for more than x').
$matchaccesssince = false;
if ($this->filterset->has_filter('accesssince')) {
$accesssince = $this->filterset->get_filter('accesssince')->current();
// Last access filtering only supports matching or not matching, not any/all/none.
$jointypenone = $this->filterset->get_filter('accesssince')::JOINTYPE_NONE;
if ($this->filterset->get_filter('accesssince')->get_join_type() === $jointypenone) {
$matchaccesssince = true;
}
}
[
'sql' => $esql,
@@ -144,7 +160,7 @@ class participants_search {
$select = "SELECT $userfieldssql, u.lastaccess";
$joins[] = "JOIN ($esql) e ON e.id = u.id"; // Everybody on the frontpage usually.
if ($accesssince) {
$wheres[] = user_get_user_lastaccess_sql($accesssince);
$wheres[] = user_get_user_lastaccess_sql($accesssince, 'u', $matchaccesssince);
}
} else {
$select = "SELECT $userfieldssql, COALESCE(ul.timeaccess, 0) AS lastaccess";
@@ -153,7 +169,7 @@ class participants_search {
$joins[] = 'LEFT JOIN {user_lastaccess} ul ON (ul.userid = u.id AND ul.courseid = :courseid)';
$params['courseid'] = $this->course->id;
if ($accesssince) {
$wheres[] = user_get_course_lastaccess_sql($accesssince);
$wheres[] = user_get_course_lastaccess_sql($accesssince, 'ul', $matchaccesssince);
}
}
@@ -183,12 +199,12 @@ class participants_search {
// Apply any keyword text searches.
if ($this->filterset->has_filter('keywords')) {
[
'wheres' => $keywordswheres,
'where' => $keywordswhere,
'params' => $keywordsparams,
] = $this->get_keywords_search_sql();
if (!empty($keywordswheres)) {
$wheres = array_merge($wheres, $keywordswheres);
if (!empty($keywordswhere)) {
$wheres[] = $keywordswhere;
}
if (!empty($keywordsparams)) {
@@ -552,19 +568,37 @@ class participants_search {
}
/**
* Prepare SQL where clauses and associated parameters for any keyword searches being performed.
* Prepare SQL where clause and associated parameters for any keyword searches being performed.
*
* @return array SQL query data in the format ['wheres' => [], 'params' => []].
* @return array SQL query data in the format ['where' => '', 'params' => []].
*/
protected function get_keywords_search_sql(): array {
global $CFG, $DB, $USER;
$keywords = [];
$wheres = [];
$where = '';
$params = [];
$keywordsfilter = $this->filterset->get_filter('keywords');
$jointype = $keywordsfilter->get_join_type();
$notjoin = false;
// Determine how to match values in the query.
switch ($jointype) {
case $keywordsfilter::JOINTYPE_ALL:
$wherejoin = ' AND ';
break;
case $keywordsfilter::JOINTYPE_NONE:
$wherejoin = ' AND NOT ';
$notjoin = true;
break;
default:
// Default to 'Any' jointype.
$wherejoin = ' OR ';
break;
}
if ($this->filterset->has_filter('keywords')) {
$keywords = $this->filterset->get_filter('keywords')->get_filter_values();
$keywords = $keywordsfilter->get_filter_values();
}
foreach ($keywords as $index => $keyword) {
@@ -583,6 +617,11 @@ class participants_search {
// Search by email.
$email = $DB->sql_like('email', ':' . $searchkey2, false, false);
if ($notjoin) {
$email = "(email IS NOT NULL AND {$email})";
}
if (!in_array('email', $this->userfields)) {
$maildisplay = 'maildisplay' . $index;
$userid1 = 'userid' . $index . '1';
@@ -590,15 +629,21 @@ class participants_search {
// who aren't allowed to see hidden email addresses.
$email = "(". $email ." AND (" .
"u.maildisplay <> :$maildisplay " .
"OR u.id = :$userid1". // User can always find himself.
"OR u.id = :$userid1". // Users can always find themselves.
"))";
$params[$maildisplay] = core_user::MAILDISPLAY_HIDE;
$params[$userid1] = $USER->id;
}
$conditions[] = $email;
// Search by idnumber.
$idnumber = $DB->sql_like('idnumber', ':' . $searchkey3, false, false);
if ($notjoin) {
$idnumber = "(idnumber IS NOT NULL AND {$idnumber})";
}
if (!in_array('idnumber', $this->userfields)) {
$userid2 = 'userid' . $index . '2';
// Users who aren't allowed to see idnumbers should at most find themselves
@@ -606,6 +651,7 @@ class participants_search {
$idnumber = "(". $idnumber . " AND u.id = :$userid2)";
$params[$userid2] = $USER->id;
}
$conditions[] = $idnumber;
if (!empty($CFG->showuseridentity)) {
@@ -619,6 +665,11 @@ class participants_search {
$param = $searchkey3 . $extrasearchfield;
$condition = $DB->sql_like($extrasearchfield, ':' . $param, false, false);
$params[$param] = "%$keyword%";
if ($notjoin) {
$condition = "($extrasearchfield IS NOT NULL AND {$condition})";
}
if (!in_array($extrasearchfield, $this->userfields)) {
// User cannot see this field, but allow match if their own account.
$userid3 = 'userid' . $index . '3' . $extrasearchfield;
@@ -631,21 +682,48 @@ class participants_search {
// Search by middlename.
$middlename = $DB->sql_like('middlename', ':' . $searchkey4, false, false);
if ($notjoin) {
$middlename = "(middlename IS NOT NULL AND {$middlename})";
}
$conditions[] = $middlename;
// Search by alternatename.
$alternatename = $DB->sql_like('alternatename', ':' . $searchkey5, false, false);
if ($notjoin) {
$alternatename = "(alternatename IS NOT NULL AND {$alternatename})";
}
$conditions[] = $alternatename;
// Search by firstnamephonetic.
$firstnamephonetic = $DB->sql_like('firstnamephonetic', ':' . $searchkey6, false, false);
if ($notjoin) {
$firstnamephonetic = "(firstnamephonetic IS NOT NULL AND {$firstnamephonetic})";
}
$conditions[] = $firstnamephonetic;
// Search by lastnamephonetic.
$lastnamephonetic = $DB->sql_like('lastnamephonetic', ':' . $searchkey7, false, false);
if ($notjoin) {
$lastnamephonetic = "(lastnamephonetic IS NOT NULL AND {$lastnamephonetic})";
}
$conditions[] = $lastnamephonetic;
$wheres[] = "(". implode(" OR ", $conditions) .") ";
if (!empty($where)) {
$where .= $wherejoin;
} else if ($jointype === $keywordsfilter::JOINTYPE_NONE) {
// Join type 'None' requires the WHERE to begin with NOT.
$where .= ' NOT ';
}
$where .= "(". implode(" OR ", $conditions) .") ";
$params[$searchkey1] = "%$keyword%";
$params[$searchkey2] = "%$keyword%";
$params[$searchkey3] = "%$keyword%";
@@ -656,7 +734,7 @@ class participants_search {
}
return [
'wheres' => $wheres,
'where' => $where,
'params' => $params,
];
}
+45 -18
View File
@@ -1537,41 +1537,68 @@ function user_get_participants($courseid, $groupid = 0, $accesssince, $roleid, $
}
/**
* Returns SQL that can be used to limit a query to a period where the user last accessed a course.
* Returns SQL that can be used to limit a query to a period where the user last accessed / did not access a course.
*
* @param int $accesssince The time since last access
* @param int $accesssince The unix timestamp to compare to users' last access
* @param string $tableprefix
* @param bool $haveaccessed Whether to match against users who HAVE accessed since $accesssince (optional)
* @return string
*/
function user_get_course_lastaccess_sql($accesssince = null, $tableprefix = 'ul') {
if (empty($accesssince)) {
return '';
}
if ($accesssince == -1) { // Never.
return $tableprefix . '.timeaccess = 0';
} else {
return $tableprefix . '.timeaccess != 0 AND ' . $tableprefix . '.timeaccess < ' . $accesssince;
}
function user_get_course_lastaccess_sql($accesssince = null, $tableprefix = 'ul', $haveaccessed = false) {
return user_get_lastaccess_sql('timeaccess', $accesssince, $tableprefix, $haveaccessed);
}
/**
* Returns SQL that can be used to limit a query to a period where the user last accessed the system.
* Returns SQL that can be used to limit a query to a period where the user last accessed / did not access the system.
*
* @param int $accesssince The time since last access
* @param int $accesssince The unix timestamp to compare to users' last access
* @param string $tableprefix
* @param bool $haveaccessed Whether to match against users who HAVE accessed since $accesssince (optional)
* @return string
*/
function user_get_user_lastaccess_sql($accesssince = null, $tableprefix = 'u') {
function user_get_user_lastaccess_sql($accesssince = null, $tableprefix = 'u', $haveaccessed = false) {
return user_get_lastaccess_sql('lastaccess', $accesssince, $tableprefix, $haveaccessed);
}
/**
* Returns SQL that can be used to limit a query to a period where the user last accessed or
* did not access something recorded by a given table.
*
* @param string $columnname The name of the access column to check against
* @param int $accesssince The unix timestamp to compare to users' last access
* @param string $tableprefix The query prefix of the table to check
* @param bool $haveaccessed Whether to match against users who HAVE accessed since $accesssince (optional)
* @return string
*/
function user_get_lastaccess_sql($columnname, $accesssince, $tableprefix, $haveaccessed = false) {
if (empty($accesssince)) {
return '';
}
if ($accesssince == -1) { // Never.
return $tableprefix . '.lastaccess = 0';
// Only users who have accessed since $accesssince.
if ($haveaccessed) {
if ($accesssince == -1) {
// Include all users who have logged in at some point.
$sql = "({$tableprefix}.{$columnname} IS NOT NULL AND {$tableprefix}.{$columnname} != 0)";
} else {
// Users who have accessed since the specified time.
$sql = "{$tableprefix}.{$columnname} IS NOT NULL AND {$tableprefix}.{$columnname} != 0
AND {$tableprefix}.{$columnname} >= {$accesssince}";
}
} else {
return $tableprefix . '.lastaccess != 0 AND ' . $tableprefix . '.lastaccess < ' . $accesssince;
// Only users who have not accessed since $accesssince.
if ($accesssince == -1) {
// Users who have never accessed.
$sql = "({$tableprefix}.{$columnname} IS NULL OR {$tableprefix}.{$columnname} = 0)";
} else {
// Users who have not accessed since the specified time.
$sql = "({$tableprefix}.{$columnname} IS NULL
OR ({$tableprefix}.{$columnname} != 0 AND {$tableprefix}.{$columnname} < {$accesssince}))";
}
}
return $sql;
}
/**
@@ -1131,4 +1131,299 @@ class participants_search_test extends advanced_testcase {
return $finaltests;
}
/**
* Ensure that the last access filter works as expected with the provided test cases.
*
* @param array $usersdata The list of users to create
* @param array $accesssince The last access data to filter by
* @param int $jointype The join type to use when combining filter values
* @param int $count The expected count
* @param array $expectedusers
* @dataProvider accesssince_provider
*/
public function test_accesssince_filter(array $usersdata, array $accesssince, int $jointype, int $count,
array $expectedusers): void {
$course = $this->getDataGenerator()->create_course();
$coursecontext = context_course::instance($course->id);
$users = [];
foreach ($usersdata as $username => $userdata) {
$usertimestamp = empty($userdata['lastlogin']) ? 0 : strtotime($userdata['lastlogin']);
$user = $this->getDataGenerator()->create_user(['username' => $username]);
$this->getDataGenerator()->enrol_user($user->id, $course->id, 'student');
// Create the record of the user's last access to the course.
if ($usertimestamp > 0) {
$this->getDataGenerator()->create_user_course_lastaccess($user, $course, $usertimestamp);
}
$users[$username] = $user;
}
// Create a secondary course with users. We should not see these users.
$this->create_course_with_users(1, 1, 1, 1);
// Create the basic filter.
$filterset = new participants_filterset();
$filterset->add_filter(new integer_filter('courseid', null, [(int) $course->id]));
// Create the last access filter.
$lastaccessfilter = new integer_filter('accesssince');
$filterset->add_filter($lastaccessfilter);
// Configure the filter.
foreach ($accesssince as $accessstring) {
$lastaccessfilter->add_filter_value(strtotime($accessstring));
}
$lastaccessfilter->set_join_type($jointype);
// Run the search.
$search = new participants_search($course, $coursecontext, $filterset);
$rs = $search->get_participants();
$this->assertInstanceOf(moodle_recordset::class, $rs);
$records = $this->convert_recordset_to_array($rs);
$this->assertCount($count, $records);
$this->assertEquals($count, $search->get_total_participants_count());
foreach ($expectedusers as $expecteduser) {
$this->assertArrayHasKey($users[$expecteduser]->id, $records);
}
}
/**
* Data provider for last access filter tests.
*
* @return array
*/
public function accesssince_provider(): array {
$tests = [
// Users with different last access times.
'Users in different groups' => (object) [
'users' => [
'a' => [
'lastlogin' => '-3 days',
],
'b' => [
'lastlogin' => '-2 weeks',
],
'c' => [
'lastlogin' => '-5 months',
],
'd' => [
'lastlogin' => '-11 months',
],
'e' => [
// Never logged in.
'lastlogin' => '',
],
],
'expect' => [
// Tests for jointype: ANY.
'ANY: No filter' => (object) [
'accesssince' => [],
'jointype' => filter::JOINTYPE_ANY,
'count' => 5,
'expectedusers' => [
'a',
'b',
'c',
'd',
'e',
],
],
'ANY: Filter on last login more than 1 year ago' => (object) [
'accesssince' => ['-1 year'],
'jointype' => filter::JOINTYPE_ANY,
'count' => 1,
'expectedusers' => [
'e',
],
],
'ANY: Filter on last login more than 6 months ago' => (object) [
'accesssince' => ['-6 months'],
'jointype' => filter::JOINTYPE_ANY,
'count' => 2,
'expectedusers' => [
'd',
'e',
],
],
'ANY: Filter on last login more than 3 weeks ago' => (object) [
'accesssince' => ['-3 weeks'],
'jointype' => filter::JOINTYPE_ANY,
'count' => 3,
'expectedusers' => [
'c',
'd',
'e',
],
],
'ANY: Filter on last login more than 5 days ago' => (object) [
'accesssince' => ['-5 days'],
'jointype' => filter::JOINTYPE_ANY,
'count' => 4,
'expectedusers' => [
'b',
'c',
'd',
'e',
],
],
'ANY: Filter on last login more than 2 days ago' => (object) [
'accesssince' => ['-2 days'],
'jointype' => filter::JOINTYPE_ANY,
'count' => 5,
'expectedusers' => [
'a',
'b',
'c',
'd',
'e',
],
],
// Tests for jointype: ALL.
'ALL: No filter' => (object) [
'accesssince' => [],
'jointype' => filter::JOINTYPE_ALL,
'count' => 5,
'expectedusers' => [
'a',
'b',
'c',
'd',
'e',
],
],
'ALL: Filter on last login more than 1 year ago' => (object) [
'accesssince' => ['-1 year'],
'jointype' => filter::JOINTYPE_ALL,
'count' => 1,
'expectedusers' => [
'e',
],
],
'ALL: Filter on last login more than 6 months ago' => (object) [
'accesssince' => ['-6 months'],
'jointype' => filter::JOINTYPE_ALL,
'count' => 2,
'expectedusers' => [
'd',
'e',
],
],
'ALL: Filter on last login more than 3 weeks ago' => (object) [
'accesssince' => ['-3 weeks'],
'jointype' => filter::JOINTYPE_ALL,
'count' => 3,
'expectedusers' => [
'c',
'd',
'e',
],
],
'ALL: Filter on last login more than 5 days ago' => (object) [
'accesssince' => ['-5 days'],
'jointype' => filter::JOINTYPE_ALL,
'count' => 4,
'expectedusers' => [
'b',
'c',
'd',
'e',
],
],
'ALL: Filter on last login more than 2 days ago' => (object) [
'accesssince' => ['-2 days'],
'jointype' => filter::JOINTYPE_ALL,
'count' => 5,
'expectedusers' => [
'a',
'b',
'c',
'd',
'e',
],
],
// Tests for jointype: NONE.
'NONE: No filter' => (object) [
'accesssince' => [],
'jointype' => filter::JOINTYPE_NONE,
'count' => 5,
'expectedusers' => [
'a',
'b',
'c',
'd',
'e',
],
],
'NONE: Filter on last login more than 1 year ago' => (object) [
'accesssince' => ['-1 year'],
'jointype' => filter::JOINTYPE_NONE,
'count' => 4,
'expectedusers' => [
'a',
'b',
'c',
'd',
],
],
'NONE: Filter on last login more than 6 months ago' => (object) [
'accesssince' => ['-6 months'],
'jointype' => filter::JOINTYPE_NONE,
'count' => 3,
'expectedusers' => [
'a',
'b',
'c',
],
],
'NONE: Filter on last login more than 3 weeks ago' => (object) [
'accesssince' => ['-3 weeks'],
'jointype' => filter::JOINTYPE_NONE,
'count' => 2,
'expectedusers' => [
'a',
'b',
],
],
'NONE: Filter on last login more than 5 days ago' => (object) [
'accesssince' => ['-5 days'],
'jointype' => filter::JOINTYPE_NONE,
'count' => 1,
'expectedusers' => [
'a',
],
],
'NONE: Filter on last login more than 2 days ago' => (object) [
'accesssince' => ['-2 days'],
'jointype' => filter::JOINTYPE_NONE,
'count' => 0,
'expectedusers' => [],
],
],
],
];
$finaltests = [];
foreach ($tests as $testname => $testdata) {
foreach ($testdata->expect as $expectname => $expectdata) {
$finaltests["{$testname} => {$expectname}"] = [
'users' => $testdata->users,
'accesssince' => $expectdata->accesssince,
'jointype' => $expectdata->jointype,
'count' => $expectdata->count,
'expectedusers' => $expectdata->expectedusers,
];
}
}
return $finaltests;
}
}
+17 -16
View File
@@ -889,7 +889,7 @@ class core_userliblib_testcase extends advanced_testcase {
groups_add_member($group->id, $student1->id);
groups_add_member($group->id, $student2->id);
// Set it so the teacher and two of the students have accessed the courses within the last day,
// Set it so the teacher and two of the students have not accessed the courses within the last day,
// but only one of the students is in the group.
$accesssince = time() - DAYSECS;
$lastaccess = new stdClass();
@@ -904,12 +904,12 @@ class core_userliblib_testcase extends advanced_testcase {
$lastaccess->userid = $student3->id;
$DB->insert_record('user_lastaccess', $lastaccess);
// Now, when we perform the following search we should only return 1 user. A student who belongs to
// the group and has the name 'searchforthis' and has also accessed the course in the last day.
// Now, when we perform the following search we should only return 2 users. Student who belong to
// the group and have the name 'searchforthis' and have not accessed the course in the last day.
$count = user_get_total_participants($course->id, $group->id, $accesssince + 1, $roleids['student'], 0, -1,
'searchforthis');
$this->assertEquals(1, $count);
$this->assertEquals(2, $count);
}
/**
@@ -918,14 +918,14 @@ class core_userliblib_testcase extends advanced_testcase {
public function test_user_get_total_participants_on_front_page() {
$this->resetAfterTest();
// Set it so that only 3 users have accessed the site within the last day.
// Set it so that only 3 users have not accessed the site within the last day (including one which has never accessed it).
$accesssince = time() - DAYSECS;
// Create a bunch of users.
$user1 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user2 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user3 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis']);
$user4 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user3 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => time()]);
$user4 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis']);
// Create a group.
$group = self::getDataGenerator()->create_group(array('courseid' => SITEID));
@@ -936,7 +936,7 @@ class core_userliblib_testcase extends advanced_testcase {
groups_add_member($group->id, $user3->id);
// Now, when we perform the following search we should only return 2 users. Users who belong to
// the group and have the name 'searchforthis' and have also accessed the site in the last day.
// the group and have the name 'searchforthis' and have not accessed the site in the last day.
$count = user_get_total_participants(SITEID, $group->id, $accesssince + 1, 0, 0, -1, 'searchforthis');
$this->assertEquals(2, $count);
@@ -978,8 +978,8 @@ class core_userliblib_testcase extends advanced_testcase {
groups_add_member($group->id, $student1->id);
groups_add_member($group->id, $student2->id);
// Set it so the teacher and two of the students have accessed the course within the last day, but only one of
// the students is in the group.
// Set it so the teacher and two of the students have not accessed the course within the last day, but only one of
// the students is in the group (student 3 has never accessed the course).
$accesssince = time() - DAYSECS;
$lastaccess = new stdClass();
$lastaccess->userid = $teacher->id;
@@ -990,11 +990,12 @@ class core_userliblib_testcase extends advanced_testcase {
$lastaccess->userid = $student1->id;
$DB->insert_record('user_lastaccess', $lastaccess);
$lastaccess->userid = $student3->id;
$lastaccess->userid = $student2->id;
$lastaccess->timeaccess = time();
$DB->insert_record('user_lastaccess', $lastaccess);
// Now, when we perform the following search we should only return 1 user. A student who belongs to
// the group and has the name 'searchforthis' and has also accessed the course in the last day.
// the group and has the name 'searchforthis' and has not accessed the course in the last day.
$userset = user_get_participants($course->id, $group->id, $accesssince + 1, $roleids['student'], 0, -1, 'searchforthis');
$this->assertEquals($student1->id, $userset->current()->id);
@@ -1013,14 +1014,14 @@ class core_userliblib_testcase extends advanced_testcase {
public function test_user_get_participants_on_front_page() {
$this->resetAfterTest();
// Set it so that only 3 users have accessed the site within the last day.
// Set it so that only 3 users have not accessed the site within the last day (user 4 has never accessed the site).
$accesssince = time() - DAYSECS;
// Create a bunch of users.
$user1 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user2 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user3 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis']);
$user4 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => $accesssince]);
$user3 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis', 'lastaccess' => time()]);
$user4 = self::getDataGenerator()->create_user(['firstname' => 'searchforthis']);
// Create a group.
$group = self::getDataGenerator()->create_group(array('courseid' => SITEID));
@@ -1031,7 +1032,7 @@ class core_userliblib_testcase extends advanced_testcase {
groups_add_member($group->id, $user3->id);
// Now, when we perform the following search we should only return 2 users. Users who belong to
// the group and have the name 'searchforthis' and have also accessed the site in the last day.
// the group and have the name 'searchforthis' and have not accessed the site in the last day.
$userset = user_get_participants(SITEID, $group->id, $accesssince + 1, 0, 0, -1, 'searchforthis', '', array(),
'ORDER BY id ASC');