Moodle
  1. Moodle
  2. MDL-33876

Database enrolment plugin throws exception when syncing enrolments for deleted user and not mapping on username field

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.3, 2.3
    • Fix Version/s: 2.2.4, 2.3.1
    • Component/s: Enrolments
    • Labels:
      None
    • Testing Instructions:
      Hide

      1/ setup enrol_database sync
      2/ sync all users from CLI
      3/ edit one user record with db editor - set one user to deleted=1
      4/ sync again
      5/ it is expected that the deleted user will be skipped

      Show
      1/ setup enrol_database sync 2/ sync all users from CLI 3/ edit one user record with db editor - set one user to deleted=1 4/ sync again 5/ it is expected that the deleted user will be skipped
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w27_MDL-33876_m24_delenrol
    • Rank:
      41978

      Description

      When running the database enrolment sync script:

      /usr/bin/php enrol/database/cli/sync.php

      I get the following exception:

      Default exception handler: Coding error detected, it must be fixed by a programmer: User ID does not exist or is deleted! Debug: userid:2183

      • line 1595 of /lib/accesslib.php: coding_exception thrown
      • line 1267 of /lib/enrollib.php: call to role_assign()
      • line 458 of /enrol/database/lib.php: call to enrol_plugin->enrol_user()
      • line 79 of /enrol/database/cli/sync.php: call to enrol_database_plugin->sync_enrolments()

      !!! Coding error detected, it must be fixed by a programmer: User ID does not exist or is deleted! !!!

      If the external enrolments table includes enrolments for a Moodle user that has been deleted this exception is thrown.

      My external enrolment table looks like this:

      course_id | contact_id | role
      -----------------------------------
      100000109 | 100005005 | editingteacher
      100000250 | 100005005 | editingteacher

      The relevant mdl_user record is:

      id | idnumber | deleted
      ----------------------
      2183 | 100005005 | 1

      Relevent database enrolment plugin settings are:

      Remote user field: contact_id
      Local user field: idnumber

      There appears to be a bug in the enrol/database/lib.php sync_enrolments(). Looking at that function around line 413 in Moodle 2.2 (look for usersearch) from what I can tell if the localuserfield is set to username then users that have been deleted are ignored. I suspect I'm hitting this error because I'm matching on the idnumber field instead. I can't see anywhere where deleted users are being excluded if searching on that field. So a fix should be pretty simple. Instead of:

      if ($localuserfield === 'username') {
      $usersearch = array('mnethostid'=>$CFG->mnet_localhost_id, 'deleted' =>0);
      }
      while ($fields = $rs->FetchRow()) {

      That block of code should look like this:

      if ($localuserfield === 'username') {
      $usersearch = array('mnethostid'=>$CFG->mnet_localhost_id, 'deleted' =>0);
      } else {
      $usersearch = array('deleted' => 0);
      }
      while ($fields = $rs->FetchRow()) {

        Activity

        Hide
        Petr Škoda added a comment -

        Oh, the idnumber field should be purged when deleting user, I am going to review the code and make sure the core delete code removes the idnumber. There are some borked auth plugins that do not delete users properly, hopefully this is not the case for you.

        Show
        Petr Škoda added a comment - Oh, the idnumber field should be purged when deleting user, I am going to review the code and make sure the core delete code removes the idnumber. There are some borked auth plugins that do not delete users properly, hopefully this is not the case for you.
        Hide
        Nathan Mares added a comment -

        Thanks Petr for that info. That account was an ldap account but this is for a Moodle database that has just been migrated from 1.9 so the problem may have been in a much older version of Moodle. On top of that the account was last modified a year ago...

        Show
        Nathan Mares added a comment - Thanks Petr for that info. That account was an ldap account but this is for a Moodle database that has just been migrated from 1.9 so the problem may have been in a much older version of Moodle. On top of that the account was last modified a year ago...
        Hide
        Petr Škoda added a comment -

        Thanks for the report.

        Show
        Petr Škoda added a comment - Thanks for the report.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi, is there any reason not to backport this to 22_STABLE, where it was initially reported?

        Show
        Eloy Lafuente (stronk7) added a comment - Hi, is there any reason not to backport this to 22_STABLE, where it was initially reported?
        Hide
        Petr Škoda added a comment -

        No special reason, for me this was more like improvement because it works fine if you delete users properly. Feel free to cherry pick it.

        Show
        Petr Škoda added a comment - No special reason, for me this was more like improvement because it works fine if you delete users properly. Feel free to cherry pick it.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (22, 23 and master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 and master), thanks!
        Hide
        Rossiani Wijaya added a comment -

        This looks great.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This looks great. Test passed.
        Hide
        Sam Hemelryk added a comment -

        Congratulations your code is upstream - gold star for you!

        This issue + 79 others made it in in time for the minor releases.
        Thank you everyone involved for your exuberant efforts.

        Show
        Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: