Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35562

enrol/database - sync_enrolments exits pre-maturely.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            danmarsden Dan Marsden added a comment -

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

            Show
            danmarsden Dan Marsden added a comment - well - it causes more dataloss by halting the sync than allowing it to continue doesn't it?
            Hide
            danmarsden 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
            danmarsden 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
            danmarsden 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
            danmarsden 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
            skodak Petr Skoda added a comment -

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

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

            +1 here too

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

            cool - thanks.

            Show
            danmarsden Dan Marsden added a comment - cool - thanks.
            Hide
            poltawski 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
            poltawski 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
            danmarsden Dan Marsden added a comment -

            rebased

            Show
            danmarsden Dan Marsden added a comment - rebased
            Hide
            stronk7 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
            stronk7 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
            timb 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
            timb 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Closing as fixed, many thanks for your awesome collaboration.

            Show
            stronk7 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:
                  Fix Release Date:
                  12/Nov/12