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
    • Rank:
      44270

      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.

        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 Škoda added a comment -

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

          Show
          Petr Škoda 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: