Moodle
  1. Moodle
  2. MDL-35557

enrol/database - sync_courses should get distinct list

    Details

    • Testing Instructions:
      Hide

      set up an external table with shortname/fullname/idnumber for some courses to add.
      make sure some of those rows are complete duplicates - more than 2 duplicates will help with testing.
      Connect external db enrol to that table
      run cli script /enrol/database/cli/sync.php using -v for verbose output.

      before the patch (and a fresh run with none of the courses in your external table created) you will see on each duplicate a message to say the course exists already.

      after the patch on fresh run (make sure you delete any courses previously created by the script) it will show 1 sucessful add and no duplicate messages for the same course.

      Note: - the duplicates must duplicate shortname/idnumber/description - if one of those fields is different then it will still report a duplicate was found. This is correct behaviour.

      Show
      set up an external table with shortname/fullname/idnumber for some courses to add. make sure some of those rows are complete duplicates - more than 2 duplicates will help with testing. Connect external db enrol to that table run cli script /enrol/database/cli/sync.php using -v for verbose output. before the patch (and a fresh run with none of the courses in your external table created) you will see on each duplicate a message to say the course exists already. after the patch on fresh run (make sure you delete any courses previously created by the script) it will show 1 sucessful add and no duplicate messages for the same course. Note: - the duplicates must duplicate shortname/idnumber/description - if one of those fields is different then it will still report a duplicate was found. This is correct behaviour.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-35557

      Description

      The sync_courses function in enrol/database/lib.php uses the following call to get a list of the courses that need to be imported:
      $sql = $this->db_get_sql($table, array(), $sqlfields);

      if the same table is used that contains the enrolment data this will return all rows in the table and the code iterates over each one to see if it needs to create the course - db_get_sql(enrol/database function) has a 4th param $distinct which is used in sync_enrolments already so we should use it here as well.

      It would improve performance of the sync_courses function a lot if we only obtained the records that we needed from the table by using the extra distinct param.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Dan Marsden added a comment -

            bouncing up for peer review - would be good to get +1 from Petr if poss?

            Show
            Dan Marsden added a comment - bouncing up for peer review - would be good to get +1 from Petr if poss?
            Hide
            Petr Skoda added a comment -

            +1 because there is no chance there would be a TEXT field in $fullname, $shortname, $category or $idnumber

            Show
            Petr Skoda added a comment - +1 because there is no chance there would be a TEXT field in $fullname, $shortname, $category or $idnumber
            Hide
            Dan Poltawski added a comment -

            Thats an easy +1 then

            Though, open question: will adding this distinct have an impact on the speed of the query on large sites?

            Show
            Dan Poltawski added a comment - Thats an easy +1 then Though, open question: will adding this distinct have an impact on the speed of the query on large sites?
            Hide
            Dan Marsden added a comment -

            yeah - it's heaps faster!!! - it completes in seconds on my large install instead of minutes!

            Show
            Dan Marsden added a comment - yeah - it's heaps faster!!! - it completes in seconds on my large install instead of minutes!
            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 and master), thanks!

            PS: Not really clear if this should be considered just an improvement... hope it won't break. Also.. perhaps it should be documented that the same table can be used both as source for courses and enrollments (or is it the "normal" way") ?

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (22, 23 and master), thanks! PS: Not really clear if this should be considered just an improvement... hope it won't break. Also.. perhaps it should be documented that the same table can be used both as source for courses and enrollments (or is it the "normal" way") ?
            Hide
            Tim Barker added a comment -

            Tested with 2 duplicates for 2 courses in the external db and 1 non-duplicate control record and one instance of each course was created. Good work

            Show
            Tim Barker added a comment - Tested with 2 duplicates for 2 courses in the external db and 1 non-duplicate control record and one instance of each course was created. Good work
            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: