ndunand / moodle-enrol_attributes

This plugin allows users to be enrolled according to any value stored in their user profile.
https://moodle.org/plugins/enrol_attributes
18 stars 22 forks source link

Logged_in event is parsing every user who matches the profile field matches not just the checking for the user logging in. #39

Closed turf212 closed 2 years ago

turf212 commented 3 years ago

The SQL which is being generated looks like :

SELECT DISTINCT u.id FROM mdl_user u RIGHT JOIN mdl_user_info_data d1 ON d1.userid = u.id AND d1.fieldid = 1 RIGHT JOIN mdl_user_info_data d2 ON d2.userid = u.id AND d2.fieldid = 1 RIGHT JOIN mdl_user_info_data d3 ON d3.userid = u.id AND d3.fieldid = 1 RIGHT JOIN mdl_user_info_data d4 ON d4.userid = u.id AND d4.fieldid = 1 RIGHT JOIN mdl_user_info_data d5 ON d5.userid = u.id AND d5.fieldid = 1 RIGHT JOIN mdl_user_info_data d6 ON d6.userid = u.id AND d6.fieldid = 1 RIGHT JOIN mdl_user_info_data d7 ON d7.userid = u.id AND d7.fieldid = 1 RIGHT JOIN mdl_user_info_data d8 ON d8.userid = u.id AND d8.fieldid = 1 WHERE u.id=2 AND u.deleted=0
AND (
 d1.data = 'Advanced Nurse Practitioner'
 OR d1.data LIKE '%Advanced Nurse Practitioner'
 OR d1.data LIKE 'Advanced Nurse Practitioner%'
 OR d1.data LIKE '%Advanced Nurse Practitioner%')
OR (
 d2.data = 'Nurse Dispenser'
 OR d2.data LIKE '%Nurse Dispenser'
 OR d2.data LIKE 'Nurse Dispenser%'
 OR d2.data LIKE '%Nurse Dispenser%')
OR (
 d3.data = 'Nurse Practitioner'
 OR d3.data LIKE '%Nurse Practitioner'
 OR d3.data LIKE 'Nurse Practitioner%'
 OR d3.data LIKE '%Nurse Practitioner%')
OR (
 d4.data = 'Nurse Specialist'
 OR d4.data LIKE '%Nurse Specialist'
 OR d4.data LIKE 'Nurse Specialist%'
 OR d4.data LIKE '%Nurse Specialist%')
OR (
 d5.data = 'Nurse Trainee'
 OR d5.data LIKE '%Nurse Trainee'
 OR d5.data LIKE 'Nurse Trainee%'
 OR d5.data LIKE '%Nurse Trainee%')
OR (
 d6.data = 'Nursing Associate'
 OR d6.data LIKE '%Nursing Associate'
 OR d6.data LIKE 'Nursing Associate%'
 OR d6.data LIKE '%Nursing Associate%')
OR (
 d7.data = 'Nursing Partner'
 OR d7.data LIKE '%Nursing Partner'
 OR d7.data LIKE 'Nursing Partner%'
 OR d7.data LIKE '%Nursing Partner%')
OR (
 d8.data = 'Practice Nurse'
 OR d8.data LIKE '%Practice Nurse'
 OR d8.data LIKE 'Practice Nurse%'
 OR d8.data LIKE '%Practice Nurse%');

which is pulling but all of the matching u.ids which are the currently logging in user (id = 2) OR where the set of dN.data = or like matches. So when a user logs in and there is a large number of possible matches across a large number of courses the login time is increased significantly due to the observer re-enrolling everyone into the courses again.

Changing the SQL to encapsulate all of the OR requirements within brackets like :

SELECT DISTINCT u.id FROM mdl_user u RIGHT JOIN mdl_user_info_data d1 ON d1.userid = u.id AND d1.fieldid = 1 RIGHT JOIN mdl_user_info_data d2 ON d2.userid = u.id AND d2.fieldid = 1 RIGHT JOIN mdl_user_info_data d3 ON d3.userid = u.id AND d3.fieldid = 1 RIGHT JOIN mdl_user_info_data d4 ON d4.userid = u.id AND d4.fieldid = 1 RIGHT JOIN mdl_user_info_data d5 ON d5.userid = u.id AND d5.fieldid = 1 RIGHT JOIN mdl_user_info_data d6 ON d6.userid = u.id AND d6.fieldid = 1 RIGHT JOIN mdl_user_info_data d7 ON d7.userid = u.id AND d7.fieldid = 1 RIGHT JOIN mdl_user_info_data d8 ON d8.userid = u.id AND d8.fieldid = 1 WHERE u.id=2 AND u.deleted=0
AND (
(
 d1.data = 'Advanced Nurse Practitioner'
 OR d1.data LIKE '%Advanced Nurse Practitioner'
 OR d1.data LIKE 'Advanced Nurse Practitioner%'
 OR d1.data LIKE '%Advanced Nurse Practitioner%')
OR (
 d2.data = 'Nurse Dispenser'
 OR d2.data LIKE '%Nurse Dispenser'
 OR d2.data LIKE 'Nurse Dispenser%'
 OR d2.data LIKE '%Nurse Dispenser%')
OR (
 d3.data = 'Nurse Practitioner'
 OR d3.data LIKE '%Nurse Practitioner'
 OR d3.data LIKE 'Nurse Practitioner%'
 OR d3.data LIKE '%Nurse Practitioner%')
OR (
 d4.data = 'Nurse Specialist'
 OR d4.data LIKE '%Nurse Specialist'
 OR d4.data LIKE 'Nurse Specialist%'
 OR d4.data LIKE '%Nurse Specialist%')
OR (
 d5.data = 'Nurse Trainee'
 OR d5.data LIKE '%Nurse Trainee'
 OR d5.data LIKE 'Nurse Trainee%'
 OR d5.data LIKE '%Nurse Trainee%')
OR (
 d6.data = 'Nursing Associate'
 OR d6.data LIKE '%Nursing Associate'
 OR d6.data LIKE 'Nursing Associate%'
 OR d6.data LIKE '%Nursing Associate%')
OR (
 d7.data = 'Nursing Partner'
 OR d7.data LIKE '%Nursing Partner'
 OR d7.data LIKE 'Nursing Partner%'
 OR d7.data LIKE '%Nursing Partner%')
OR (
 d8.data = 'Practice Nurse'
 OR d8.data LIKE '%Practice Nurse'
 OR d8.data LIKE 'Practice Nurse%'
 OR d8.data LIKE '%Practice Nurse%')
 );

where the dN.data checks are within the one AND () statement fixes this issue.

I've updated the attrsyntax_tosql() function locally to include a check at the end :

        $where = preg_replace('/^1=1 AND/', '', $where);
        $where = preg_replace('/^1=1 OR/', '', $where);
        $where = preg_replace('/^1=1/', '', $where);

        if (!empty($where)) {
                $where = "( $where ) ";
        }

        return array(
                'select' => $select,
                'where'  => $where,
                'params' => $params
        );

works for logging in events. I've not tested any of the other sync methods (which I suspect may be broken with this anyway as there isn't any extra u.id = X constrictions although a quick test direct on MariaDB appeared to be OK).

turf212 commented 3 years ago

This error is still in the latest version of the code although the code has been updated and is now different from the above.

In line 338 of lib.php it is fixed by changing the code to

if($where === '') { $where = 0; } else { $where = "( $where )"; }

ndunand commented 2 years ago

Thanks @turf212 , I'm looking into this and testing. This seems to not have any negative impact and should make it in the next release. Thanks for sharing.