Moodle
  1. Moodle
  2. MDL-34870

enrol_cohort needlessly performs an enrolment sync everytime update_course is called

    Details

      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.

        Gliffy Diagrams

          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 Skoda 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 Skoda 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 Skoda 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 Skoda 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: