Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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()) {

        Gliffy Diagrams

          Activity

          Hide
          skodak Petr Skoda 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
          skodak Petr Skoda 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
          nmares 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
          nmares 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
          skodak Petr Skoda added a comment -

          Thanks for the report.

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

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

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hi, is there any reason not to backport this to 22_STABLE, where it was initially reported?
          Hide
          skodak Petr Skoda 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
          skodak Petr Skoda 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 and master), thanks!

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

          This looks great.

          Test passed.

          Show
          rwijaya Rossiani Wijaya added a comment - This looks great. Test passed.
          Hide
          samhemelryk 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
          samhemelryk 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:
                Fix Release Date:
                9/Jul/12