Moodle
  1. Moodle
  2. MDL-34870

enrol_cohort needlessly performs an enrolment sync everytime update_course is called

    Details

    • Rank:
      43392

      Description

      Any time update_course is called (i.e. from the core update course webservice or from the GUI in the edit settings page), the last thing it does is tell the enrolment methods it's been updated.

      This allows guest methods and such to update passwords and such, which is good. However, the enrol_cohort plugin performs a enrol_cohort_sync on the course! If the course has many cohorts and/or a large number of users in the cohorts, this can take a very long time to process, and so the websvc call or save action takes several minutes to return a response.

      As far as I can tell, there's no reason to ever do a sync at this point. Syncs are already done in cron, and when the cohort is first added or removed from the course. Even adding or removing a user from a cohort ensures that user is promptly [un]enrolled from the courses its in.

      Test for this change would be to add a cohort with several hundred users to a course, then to update the course description and hit save. Without the fix, it'll take quite awhile to return while it performs a sync (depending on the cohort size). With the fix, it'll return much faster.

        Issue Links

          Activity

          Hide
          Tom Lanyon added a comment -

          This causes all sorts of performance problems for us when triggered via web services for a course with a large cohort, as the cohort sync will take a while to complete and hold up any further course updates.

          Show
          Tom Lanyon added a comment - This causes all sorts of performance problems for us when triggered via web services for a course with a large cohort, as the cohort sync will take a while to complete and hold up any further course updates.
          Hide
          Petr Škoda added a comment -

          Ah, right, there is no relation between cohort enrol and course editing, makes sense. Thanks for the report and fix, submitting for integration.

          I ma going to remove the handler completely during the next code cleanup, thanks again for spotting this.

          Show
          Petr Škoda added a comment - Ah, right, there is no relation between cohort enrol and course editing, makes sense. Thanks for the report and fix, submitting for integration. I ma going to remove the handler completely during the next code cleanup, thanks again for spotting this.
          Hide
          Dan Poltawski added a comment -

          Hi,

          Erm, well this sure makes sense. But having an if statement that does nothing like this looks a bit silly? We are just wasting a cpu cycle

          Can't we remove the handler completely now?

          Show
          Dan Poltawski added a comment - Hi, Erm, well this sure makes sense. But having an if statement that does nothing like this looks a bit silly? We are just wasting a cpu cycle Can't we remove the handler completely now?
          Hide
          Adam Olley added a comment -

          I see no reason not to just remove it completely. Up to you and Petr

          Show
          Adam Olley added a comment - I see no reason not to just remove it completely. Up to you and Petr
          Hide
          Dan Poltawski added a comment -

          Petr, am I missing something? Can we just remove it?

          Show
          Dan Poltawski added a comment - Petr, am I missing something? Can we just remove it?
          Hide
          Petr Škoda added a comment -

          Removing in stable is a bit tricky because at the moment if you do not upgrade immediately the event handler processing will break, I think I just saw a new issue for it.

          Show
          Petr Škoda added a comment - Removing in stable is a bit tricky because at the moment if you do not upgrade immediately the event handler processing will break, I think I just saw a new issue for it.
          Hide
          Dan Poltawski added a comment -

          Ok, well how about we remove the if statement and just leave a comment pointing to this issue in its place.

          Show
          Dan Poltawski added a comment - Ok, well how about we remove the if statement and just leave a comment pointing to this issue in its place.
          Hide
          Dan Poltawski added a comment -

          Right, in order to avoid dealying this issue anymore i've done that myself.

          Integrated to 22, 23 and master. Thanks guys!

          Show
          Dan Poltawski added a comment - Right, in order to avoid dealying this issue anymore i've done that myself. Integrated to 22, 23 and master. Thanks guys!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nobody tested this, following testing instructions (or not following them).

          Show
          Eloy Lafuente (stronk7) added a comment - Nobody tested this, following testing instructions (or not following them).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

            • Votes:
              6 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: