Moodle
  1. Moodle
  2. MDL-35562

enrol/database - sync_enrolments exits pre-maturely.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3, 2.4
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      set up an external table for user enrolment sync - for the course idnumber field use an integer based field in the external db

      create some courses in moodle with char based idnumber

      run the sync process - it should continue when it gets to a course with a char in the idnumber.

      Show
      set up an external table for user enrolment sync - for the course idnumber field use an integer based field in the external db create some courses in moodle with char based idnumber run the sync process - it should continue when it gets to a course with a char in the idnumber.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-35562
    • Rank:
      44275

      Description

      sync_enrolments has this function which tries to get a list of users for "this" course to enrol

                  $sql = $this->db_get_sql($table, array($coursefield=>$course->mapping), $sqlfields);
                  if ($rs = $extdb->Execute($sql)) {
      ...
                  } else {
                      mtrace('Error while communicating with external enrolment database');
                      $extdb->Close();
                      return;
                  }
      

      problem is if the idnumber in a course contains something that would cause the sql to fail - eg char instead of expected int - the enrolment process halts at this point and stops any further enrolment.

      it would be better to "continue" at this point instead of close/return.

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          I am not sure it is always good to skip problems like this, ignoring of problems may easily lead to data loss...

          Show
          Petr Škoda added a comment - I am not sure it is always good to skip problems like this, ignoring of problems may easily lead to data loss...
          Hide
          Dan Marsden added a comment -

          well - it causes more dataloss by halting the sync than allowing it to continue doesn't it?

          Show
          Dan Marsden added a comment - well - it causes more dataloss by halting the sync than allowing it to continue doesn't it?
          Hide
          Dan Marsden added a comment -

          more detailed scenario:
          Course A (old course) has an idnumber like "SA1234"
          Course b (new course) has an idnumber like "1234"

          enrolment sync runs against external db which has an "int" field - when it hits the old course it gives a failed sql call and halts the sync.

          Show
          Dan Marsden added a comment - more detailed scenario: Course A (old course) has an idnumber like "SA1234" Course b (new course) has an idnumber like "1234" enrolment sync runs against external db which has an "int" field - when it hits the old course it gives a failed sql call and halts the sync.
          Hide
          Dan Marsden added a comment -

          bouncing up for peer review (pending Petr's +1) - Petr I'm not sure how this change could cause dataloss - can you elaborate?

          Show
          Dan Marsden added a comment - bouncing up for peer review (pending Petr's +1) - Petr I'm not sure how this change could cause dataloss - can you elaborate?
          Hide
          Petr Škoda added a comment -

          hmm, right, in this particular case the continue is a valid action when SQL error encountered, thanks +1

          Show
          Petr Škoda added a comment - hmm, right, in this particular case the continue is a valid action when SQL error encountered, thanks +1
          Hide
          Dan Poltawski added a comment -

          +1 here too

          Show
          Dan Poltawski added a comment - +1 here too
          Hide
          Dan Marsden added a comment -

          cool - thanks.

          Show
          Dan Marsden added a comment - cool - thanks.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Marsden added a comment -

          rebased

          Show
          Dan Marsden added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (22, 23 & master), thanks!

          Note: Plz. amend testing instructions to describe new, correct approach (mtrace + continue), instead of old one (mtrace + halt).

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 & master), thanks! Note: Plz. amend testing instructions to describe new, correct approach (mtrace + continue), instead of old one (mtrace + halt).
          Hide
          Tim Barker added a comment -

          Tested with three courses with the id_numbers: 1, X and 2 in that order. Ran the sync from an external DB. User ID's in the external DB were enrolled in the courses 1 and 2. The sync script jumped over course X.

          Show
          Tim Barker added a comment - Tested with three courses with the id_numbers: 1, X and 2 in that order. Ran the sync from an external DB. User ID's in the external DB were enrolled in the courses 1 and 2. The sync script jumped over course X.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: