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

meta course sync plugin fixes and improvements

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2, 2.3
    • Fix Version/s: 2.3
    • Component/s: Enrolments
    • Labels:
      None
    • Database:
      Any
    • Testing Instructions:
      Hide

      1/ enable enrol meta sync
      2/ set up a few links and test every kind of modifications via enrol UI
      3/ edit code and disable propagation of events (add return true; in sync_with_parent_course()) and now try modification via UI followed by CLI sync, again try all combinations of settings and operations
      4/ try cron executes the sync too (once should be enough if somebody reviews the change in version.php)

      Show
      1/ enable enrol meta sync 2/ set up a few links and test every kind of modifications via enrol UI 3/ edit code and disable propagation of events (add return true; in sync_with_parent_course()) and now try modification via UI followed by CLI sync, again try all combinations of settings and operations 4/ try cron executes the sync too (once should be enough if somebody reviews the change in version.php)
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w02_MDL-29684_m23_metaenrol

      Description

      People who have roles listed in 'nosyncroleids' are still synchronised in child courses (without any role, but still synchronised).

        Gliffy Diagrams

          Issue Links

            Activity

            jboulen Julien Boulen created issue -
            jboulen Julien Boulen made changes -
            Field Original Value New Value
            Priority Minor [ 4 ] Major [ 3 ]
            skodak Petr Skoda made changes -
            Assignee moodle.com [ moodle.com ] Petr Škoda (skodak) [ skodak ]
            skodak Petr Skoda made changes -
            Status Open [ 1 ] Development in progress [ 3 ]
            skodak Petr Skoda made changes -
            Link This issue is duplicated by MDL-30156 [ MDL-30156 ]
            skodak Petr Skoda made changes -
            Link This issue is duplicated by MDL-29043 [ MDL-29043 ]
            Hide
            skodak Petr Skoda added a comment -

            oh, it was a lot bigger than I expected, I ended up rewriting nearly the all sync code and adding new options...

            List of changes:

            • configurable unenrol action
            • new setting for synchronisation of all enrolled users or users with role only
            • cron period increased to 6 hours in order to lower server load, courses should not get out of sync often
            • CLI sync script for debugging or manual sync
            • phpdocs fixes
            • when plugin is disabled all roles are removed, but enrollments are kept now
            • uninstall script
            • other bugfixing

            To integrators: I think it should go to 2.2 because people are not satisfied with the original implementation. I you disagree then at least put it into master only please.

            Show
            skodak Petr Skoda added a comment - oh, it was a lot bigger than I expected, I ended up rewriting nearly the all sync code and adding new options... List of changes: configurable unenrol action new setting for synchronisation of all enrolled users or users with role only cron period increased to 6 hours in order to lower server load, courses should not get out of sync often CLI sync script for debugging or manual sync phpdocs fixes when plugin is disabled all roles are removed, but enrollments are kept now uninstall script other bugfixing To integrators: I think it should go to 2.2 because people are not satisfied with the original implementation. I you disagree then at least put it into master only please.
            skodak Petr Skoda made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            Pull Master Diff URL https://github.com/skodak/moodle/compare/master...w51_MDL-29684_m23_metaenrol
            Pull Master Branch w51_MDL-29684_m23_metaenrol
            Pull from Repository git://github.com/skodak/moodle.git
            Fix Version/s 2.2.1 [ 11456 ]
            Fix Version/s 2.3 [ 10657 ]
            Testing Instructions Here, a discussion which explain full steps : http://moodle.org/mod/forum/discuss.php?d=168701 1/ enable enrol meta sync
            2/ set up a few links and test every kind of modifications via enrol UI
            3/ edit code and disable propagation of events (add return true; in sync_with_parent_course()) and now try modification via UI followed by CLI sync, again try all combinations of settings and operations
            4/ try cron executes the sync too (once should be enough if somebody reviews the change in version.php)
            Pull 2.2 Diff URL https://github.com/skodak/moodle/compare/MOODLE_22_STABLE...w51_MDL-29684_m22_metaenrol
            Pull 2.2 Branch w51_MDL-29684_m22_metaenrol
            Hide
            ray Ray Lawrence added a comment -

            Thanks Petr, it would be great if you could expand upon some of the changes here:

            • configurable unenrol action: Where, system or in the course with metalinked enrolments enabled?
            • new setting for synchronisation of all enrolled users or users with role only: Where, system or in the course with metalinked enrolments enabled?
            • when plugin is disabled all roles are removed, but enrollments are kept now: sorry, but I don't understand what the expected behaviour is here. I read this as: Disable metalink enrolments at system level, all role syncronisations are removed but the users remain enrolled in the course that formerly had meta-linked enrolments enabled. Is that correct? Are they converted to Manual enrolments?

            Comment:

            6 hours is a long time for this. In many cases this will mean that the change will not talk effect within a single school/working day. What's it set to currently? how any reports are that that the current interval is causing server load issues?

            Show
            ray Ray Lawrence added a comment - Thanks Petr, it would be great if you could expand upon some of the changes here: configurable unenrol action: Where, system or in the course with metalinked enrolments enabled? new setting for synchronisation of all enrolled users or users with role only: Where, system or in the course with metalinked enrolments enabled? when plugin is disabled all roles are removed, but enrollments are kept now: sorry, but I don't understand what the expected behaviour is here. I read this as: Disable metalink enrolments at system level, all role syncronisations are removed but the users remain enrolled in the course that formerly had meta-linked enrolments enabled. Is that correct? Are they converted to Manual enrolments? Comment: 6 hours is a long time for this. In many cases this will mean that the change will not talk effect within a single school/working day. What's it set to currently? how any reports are that that the current interval is causing server load issues?
            Hide
            skodak Petr Skoda added a comment - - edited

            1/ configurable unenrol action - when user is unenrolled from the source (aka parent) course you can select to either a/ fully unenrol b/ suspend and remove all roles; this is similar to setting in enrol/database plugin

            2/ originally I implemented the nosyncroleid option so that it only prevented role assignments, but not enrolments - if I change the logic users would most probably complain about data loss because we would have to unenrol users without roles. At the same time the change would prevent sync of users without roles. So now there is a new option "Sync all enrolments".

            3/ there was a problem: if you disable the enrol_meta plugin at the system level what should happen with enrolments and roles in existing courses? Before my patch it was relatively inconsistent - the UI hooks and cron did something a bit different. So I decided to standardise it to - a/ remove all roles assigned by this plugin when you disable the plugin b/ keep enrolments forever in order to prevent data loss. If you reenable the meta plugin everything is synced back to original state.

            4/ meta enrol synchronisation works via events - if you change user enrolment/role in any course enrolment plugins are notified immediately, if you add new meta link it is synced immediately too. Theoretically is should not get out of sync, but if it does it is fixed twice a day which should be enough. In any case you can now trigger the sync via CLI (which includes verbose switch for diagnostics). Please note the sync can be pretty expensive on large sites, you do not want to run on in every cron execution... If you find some operation in UI that does not sync the enrolments immediately please report it as a bug, thanks.

            If this gets integrated we need to document it in docs, but before it is accepted I think it would only confuse ppl there.

            Show
            skodak Petr Skoda added a comment - - edited 1/ configurable unenrol action - when user is unenrolled from the source (aka parent) course you can select to either a/ fully unenrol b/ suspend and remove all roles; this is similar to setting in enrol/database plugin 2/ originally I implemented the nosyncroleid option so that it only prevented role assignments, but not enrolments - if I change the logic users would most probably complain about data loss because we would have to unenrol users without roles. At the same time the change would prevent sync of users without roles. So now there is a new option "Sync all enrolments". 3/ there was a problem: if you disable the enrol_meta plugin at the system level what should happen with enrolments and roles in existing courses? Before my patch it was relatively inconsistent - the UI hooks and cron did something a bit different. So I decided to standardise it to - a/ remove all roles assigned by this plugin when you disable the plugin b/ keep enrolments forever in order to prevent data loss. If you reenable the meta plugin everything is synced back to original state. 4/ meta enrol synchronisation works via events - if you change user enrolment/role in any course enrolment plugins are notified immediately, if you add new meta link it is synced immediately too. Theoretically is should not get out of sync, but if it does it is fixed twice a day which should be enough. In any case you can now trigger the sync via CLI (which includes verbose switch for diagnostics). Please note the sync can be pretty expensive on large sites, you do not want to run on in every cron execution... If you find some operation in UI that does not sync the enrolments immediately please report it as a bug, thanks. If this gets integrated we need to document it in docs, but before it is accepted I think it would only confuse ppl there.
            Hide
            ray Ray Lawrence added a comment -

            1 - Great. Thanks.
            2 - I still don't get this. How can users be enrolled in a course without a role?
            3 - As above, still uncertain about the enrolled but without having a role concept.
            4 - Thanks for that clarification (I didn't think the sync was cron dependant). Agree that interval would be fine to address sync issues.

            Show
            ray Ray Lawrence added a comment - 1 - Great. Thanks. 2 - I still don't get this. How can users be enrolled in a course without a role? 3 - As above, still uncertain about the enrolled but without having a role concept. 4 - Thanks for that clarification (I didn't think the sync was cron dependant). Agree that interval would be fine to address sync issues.
            Hide
            skodak Petr Skoda added a comment -

            2/ enrolments are perfectly valid without roles since Moodle 2.0, "enrolled" means that you participate in course (that is you have grades, can be member of groups, etc.)

            Show
            skodak Petr Skoda added a comment - 2/ enrolments are perfectly valid without roles since Moodle 2.0, "enrolled" means that you participate in course (that is you have grades, can be member of groups, etc.)
            Hide
            ray Ray Lawrence added a comment -

            2 - And your permissions in that course are determined by...?

            Show
            ray Ray Lawrence added a comment - 2 - And your permissions in that course are determined by...?
            Hide
            skodak Petr Skoda added a comment -

            active enrolment - user can enter course + user data is visible
            suspended enrolment - user can not enter course (unless there is some guest plugin or second enrolment of course) + data is not visible in most places (but it is still stored in DB tables)
            unenrolment - most of the user data is purged from course (including grades, group membership, preferences, submissions), only public stuff is kept (such as forum posts and mod/database entries)

            Show
            skodak Petr Skoda added a comment - active enrolment - user can enter course + user data is visible suspended enrolment - user can not enter course (unless there is some guest plugin or second enrolment of course) + data is not visible in most places (but it is still stored in DB tables) unenrolment - most of the user data is purged from course (including grades, group membership, preferences, submissions), only public stuff is kept (such as forum posts and mod/database entries)
            Hide
            skodak Petr Skoda added a comment -

            Permissions are determined via roles - everybody has for example "registered user" role, you may assign extra roles in some activities only. You may even assign roles at system or category contexts. Enrolment plugins may set protected roles at course level, but they do not have to.

            Show
            skodak Petr Skoda added a comment - Permissions are determined via roles - everybody has for example "registered user" role, you may assign extra roles in some activities only. You may even assign roles at system or category contexts. Enrolment plugins may set protected roles at course level, but they do not have to.
            Hide
            ray Ray Lawrence added a comment -

            Thanks for those confirmations. We're have the same understanding but express that in different ways...

            Show
            ray Ray Lawrence added a comment - Thanks for those confirmations. We're have the same understanding but express that in different ways...
            skodak Petr Skoda made changes -
            Link This issue is blocked by MDL-30789 [ MDL-30789 ]
            skodak Petr Skoda made changes -
            Link This issue is blocked by MDL-30789 [ MDL-30789 ]
            Hide
            skodak Petr Skoda added a comment -

            I have found some minor issues here when working on cohort plugin, I will resubmit it later today after more testing, ciao.

            Show
            skodak Petr Skoda added a comment - I have found some minor issues here when working on cohort plugin, I will resubmit it later today after more testing, ciao.
            skodak Petr Skoda made changes -
            Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
            skodak Petr Skoda made changes -
            Status Reopened [ 4 ] Development in progress [ 3 ]
            skodak Petr Skoda made changes -
            Link This issue has been marked as being related by MDL-30509 [ MDL-30509 ]
            skodak Petr Skoda made changes -
            Summary meta course: nosyncroleids doesn't work meta course sync plugin fixes and improvements
            skodak Petr Skoda made changes -
            Hide
            skodak Petr Skoda added a comment -

            I am resubmitting it for integration, I have done some more tweaking and fixing and I did more extensive testing.

            Show
            skodak Petr Skoda added a comment - I am resubmitting it for integration, I have done some more tweaking and fixing and I did more extensive testing.
            skodak Petr Skoda made changes -
            Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
            skodak Petr Skoda made changes -
            Link This issue is blocked by MDL-30945 [ MDL-30945 ]
            Hide
            skodak Petr Skoda added a comment -

            I have added option to completely unenrol suspended users.

            Show
            skodak Petr Skoda added a comment - I have added option to completely unenrol suspended users.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hmmm, I'm stopping my review of this and I'm going to assign it to Eloy for integration. Eloy I think it's best you have a look at this and make the final decision and whether it goes in and on what branches.

            From what I've seen so far it look's OK and it'd get my +1 to backport to 2.2.
            What I have noted:

            • I'm not sure about the necessity of the new allow_unenrol_user. Couldn't user enrolment be an optional argument of allow_unenrol. This would allow unenrol plugins to ensure user by user checking correct?
            • I think there is still improvements to be made in the now generic unenroluser script in order to allow self unenrolment so that we can remove the now out of place unenrolself scripts from those plugins that still have them.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hmmm, I'm stopping my review of this and I'm going to assign it to Eloy for integration. Eloy I think it's best you have a look at this and make the final decision and whether it goes in and on what branches. From what I've seen so far it look's OK and it'd get my +1 to backport to 2.2. What I have noted: I'm not sure about the necessity of the new allow_unenrol_user. Couldn't user enrolment be an optional argument of allow_unenrol. This would allow unenrol plugins to ensure user by user checking correct? I think there is still improvements to be made in the now generic unenroluser script in order to allow self unenrolment so that we can remove the now out of place unenrolself scripts from those plugins that still have them. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
            samhemelryk Sam Hemelryk made changes -
            Integrator samhemelryk stronk7
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

            This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

            This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

            Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Currently in integration Yes [ 10041 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) 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
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s 2.2.2 [ 11552 ]
            Fix Version/s 2.2.1 [ 11456 ]
            skodak Petr Skoda made changes -
            samhemelryk Sam Hemelryk made changes -
            Currently in integration Yes [ 10041 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Some points:

            1) What's the point about to allow to (manually) delete the suspended enrolment in child courses? Next cron execution will re-add it again (as suspended, of course, because parent enrol is suspended). But why do we allow to delete?

            2) Surely the cli/cron output needs to be a bit more "explicit", it shown information like:

            enrolling: 4 ==> 3
            suspending: 4 ==> 3
            unassigning role: 4 ==> 3 as manager

            and it's not clear what is what (course id/shortname, userid, roleid...)

            3) Why are you getting used to add to the JOIN clause all the conditions related with that table? I can understand having some conditions there if we are looking for nulls and friends in OUTER join, but in INNER ones, all the non-joining conditions should go to the WHERE clause.

            4) I have not played with the meta enrol plugin before, so I've one big concern: Are current defaults after upgrade 100% BC with previous behavior? Or anything has changed (apart from bugfixes).

            5) There is one function... user_not_supposed_to_be_here() cannot we find better name, lol, sounds like a joke.

            6) The reuse of the "extremoved...." lang strings in admin interface sounds a bit "forced" to me. While the values in the popup have sense (are neutral), both the label and the description lead to confusion, at least to me, talking about "external" sources. +1 to change that to the usual "parent course" slang (if that's the official name).

            7) I'm initially feeling that this should go only to master, but if BC (point 4) is guaranteed and nobody is against it, I've not problems backporting it to 22_STABLE (right now it applies clean).

            8) This will need documentation for sure about the new cli, the new settings, how the inheritance/suspended/deleted/settings are going to work, and also in release notes and surely some QA tests to cover it (chaining more than 1 parentchild surely). I've added the corresponding labels. They should be implemented immediately after this lands.

            9) I saw above that you talked about 6 hours for cron execution, but finally implemented 1 hour. I've not problem with that, just to confirm it's the desired cron timing.

            10) In one comment above, you say: "unenrolment - most of the user data is purged from course (including grades, group membership, preferences, submissions)". Are really submissions (assignment, workshop...) deleted on suspend? That surprised me a lot! (np with the rest I think).

            And that's all, I'll keep this open for some time, there are not big changes, perhaps the tricky point is to decide about 22_STABLE.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Some points: 1) What's the point about to allow to (manually) delete the suspended enrolment in child courses? Next cron execution will re-add it again (as suspended, of course, because parent enrol is suspended). But why do we allow to delete? 2) Surely the cli/cron output needs to be a bit more "explicit", it shown information like: enrolling: 4 ==> 3 suspending: 4 ==> 3 unassigning role: 4 ==> 3 as manager and it's not clear what is what (course id/shortname, userid, roleid...) 3) Why are you getting used to add to the JOIN clause all the conditions related with that table? I can understand having some conditions there if we are looking for nulls and friends in OUTER join, but in INNER ones, all the non-joining conditions should go to the WHERE clause. 4) I have not played with the meta enrol plugin before, so I've one big concern: Are current defaults after upgrade 100% BC with previous behavior? Or anything has changed (apart from bugfixes). 5) There is one function... user_not_supposed_to_be_here() cannot we find better name, lol, sounds like a joke. 6) The reuse of the "extremoved...." lang strings in admin interface sounds a bit "forced" to me. While the values in the popup have sense (are neutral), both the label and the description lead to confusion, at least to me, talking about "external" sources. +1 to change that to the usual "parent course" slang (if that's the official name). 7) I'm initially feeling that this should go only to master, but if BC (point 4) is guaranteed and nobody is against it, I've not problems backporting it to 22_STABLE (right now it applies clean). 8) This will need documentation for sure about the new cli, the new settings, how the inheritance/suspended/deleted/settings are going to work, and also in release notes and surely some QA tests to cover it (chaining more than 1 parentchild surely). I've added the corresponding labels. They should be implemented immediately after this lands. 9) I saw above that you talked about 6 hours for cron execution, but finally implemented 1 hour. I've not problem with that, just to confirm it's the desired cron timing. 10) In one comment above, you say: "unenrolment - most of the user data is purged from course (including grades, group membership, preferences, submissions)". Are really submissions (assignment, workshop...) deleted on suspend? That surprised me a lot! (np with the rest I think). And that's all, I'll keep this open for some time, there are not big changes, perhaps the tricky point is to decide about 22_STABLE. Ciao
            Hide
            skodak Petr Skoda added a comment -

            1/ if plugin is only suspending and then you one day need to fully unenrol users than the only way is to do it manually which is now possible, I am implementing this in all enrol plugins that suspend instead of unenrol (see the plugin options)

            2/ it is consistent with the rest of auth/enrol plugins - I agree we should improve it in the future

            3/ I like conditions in joins because it is a lot easier to read and copy/paste

            4/ BC - that is why I needed to add new options to fix existing issues - admins decide what is good for them

            5/ hehe, it is internal function name, what is wrong with it? I took me ages to get to this clean abstraction

            6/ those strings are shared by all enrol plugins, we can either try to make them fit better all plugins once I fix all of them or we can introduce new strings as necessary. I think this might be better done together with docs - it should not be a problem if this goes to master only, right?

            7/ I would personally prefer master only because I would like to finish the rest of enrol plugins before throwing this on our production admins

            8/ sure - I must write more docs for the whole /enrol/ area

            9/ yes, I changed it to 1 hour in other plugins, hopefully it should be fast enough now

            10/ module developers decide what gets deleted when you unenrol students from courses - my +1 to delete everything other students do not see (that is to keep only forum posts, glossary entries, etc.)

            Ciao

            Show
            skodak Petr Skoda added a comment - 1/ if plugin is only suspending and then you one day need to fully unenrol users than the only way is to do it manually which is now possible, I am implementing this in all enrol plugins that suspend instead of unenrol (see the plugin options) 2/ it is consistent with the rest of auth/enrol plugins - I agree we should improve it in the future 3/ I like conditions in joins because it is a lot easier to read and copy/paste 4/ BC - that is why I needed to add new options to fix existing issues - admins decide what is good for them 5/ hehe, it is internal function name, what is wrong with it? I took me ages to get to this clean abstraction 6/ those strings are shared by all enrol plugins, we can either try to make them fit better all plugins once I fix all of them or we can introduce new strings as necessary. I think this might be better done together with docs - it should not be a problem if this goes to master only, right? 7/ I would personally prefer master only because I would like to finish the rest of enrol plugins before throwing this on our production admins 8/ sure - I must write more docs for the whole /enrol/ area 9/ yes, I changed it to 1 hour in other plugins, hopefully it should be fast enough now 10/ module developers decide what gets deleted when you unenrol students from courses - my +1 to delete everything other students do not see (that is to keep only forum posts, glossary entries, etc.) Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            1/ Note that I ended to one "suspended" enrolment in the child course because I suspended the corresponding enrolment (to one user) in the parent course. I'm not talking about suspended enrolments caused by enrol_meta configured to suspend. I can understand that in that case, the manual unenrol may be necessary. But if the "suspension" happens because of one suspension in parent, then the manual option should not be available, as far as the 2 courses for that user, continue fully linked.

            2/ so I assume you are going to create a new issue for that.

            3/ I don't, but it's not enforced by law, so who cares.

            4/ yeah, I got the new settings but my question was simply if the default behavior after the upgrade is 100% the same that the behavior before the upgrade.

            5/ np, but... it's funny/crazy.

            6/ so I assume you are going to create a new issue for that.

            7/ np here, but all this rework was ignited by some real bugs, isn't it?. Also... where are you going to document that? We don't have docs 2.3 yet, isn't it?

            8/ so I assume you are going to create a new issue for that.

            9/ perfect

            10/ can be discussed (imagine a timed enrol, later extended... to lose everything by default can lead to some problems), but np, after all we have suspend to keep all the information there forever if decided.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 1/ Note that I ended to one "suspended" enrolment in the child course because I suspended the corresponding enrolment (to one user) in the parent course. I'm not talking about suspended enrolments caused by enrol_meta configured to suspend. I can understand that in that case, the manual unenrol may be necessary. But if the "suspension" happens because of one suspension in parent, then the manual option should not be available, as far as the 2 courses for that user, continue fully linked. 2/ so I assume you are going to create a new issue for that. 3/ I don't, but it's not enforced by law, so who cares. 4/ yeah, I got the new settings but my question was simply if the default behavior after the upgrade is 100% the same that the behavior before the upgrade. 5/ np, but... it's funny/crazy. 6/ so I assume you are going to create a new issue for that. 7/ np here, but all this rework was ignited by some real bugs, isn't it?. Also... where are you going to document that? We don't have docs 2.3 yet, isn't it? 8/ so I assume you are going to create a new issue for that. 9/ perfect 10/ can be discussed (imagine a timed enrol, later extended... to lose everything by default can lead to some problems), but np, after all we have suspend to keep all the information there forever if decided. Ciao
            Hide
            skodak Petr Skoda added a comment -

            1/ hmmm, it should probably not synchronise suspended enrolments at all - in that case unenrol would be fine
            4/ it is not exactly the same because I fixed the code by replacing it with a new one with correct logic and new features related to suspending
            7/ this was ignited by my work on other enrol plugins - namely cohort enrol and ext db enrol

            Show
            skodak Petr Skoda added a comment - 1/ hmmm, it should probably not synchronise suspended enrolments at all - in that case unenrol would be fine 4/ it is not exactly the same because I fixed the code by replacing it with a new one with correct logic and new features related to suspending 7/ this was ignited by my work on other enrol plugins - namely cohort enrol and ext db enrol
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Please create followups for the docs, strings, whatever is necessary. I already have said/objected all I found.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! Please create followups for the docs, strings, whatever is necessary. I already have said/objected all I found.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            Affects Version/s 2.3 [ 10657 ]
            Fix Version/s 2.2 [ 10656 ]
            Fix Version/s 2.1 [ 10370 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Fix Version/s 2.1 [ 10370 ]
            Fix Version/s 2.2 [ 10656 ]
            Affects Version/s 2.2 [ 10656 ]
            Affects Version/s 2.1 [ 10370 ]
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester samhemelryk
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, tested now as described and all works fine.
            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, tested now as described and all works fine. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Resolution Fixed [ 1 ]
            Currently in integration Yes [ 10041 ]
            Integration date 12/Jan/12
            Hide
            minholi Marcelo Minholi added a comment -

            It will be integrated to 22_STABLE? I need this to solve a big problem here.

            Show
            minholi Marcelo Minholi added a comment - It will be integrated to 22_STABLE? I need this to solve a big problem here.
            Hide
            chrisw Chris Wharton added a comment -

            We have had serious issues with client upgrades that use metacourses. Is there any reason not to integrate this to 22_stable?

            Show
            chrisw Chris Wharton added a comment - We have had serious issues with client upgrades that use metacourses. Is there any reason not to integrate this to 22_stable?
            Hide
            danmarsden Dan Marsden added a comment -

            I just blogged about the issues we had here if anyone's interested:
            http://danmarsden.com/blog/2012/01/23/meta-course-enrolments-and-failed-moodle-2-upgrades-oh-my/

            Show
            danmarsden Dan Marsden added a comment - I just blogged about the issues we had here if anyone's interested: http://danmarsden.com/blog/2012/01/23/meta-course-enrolments-and-failed-moodle-2-upgrades-oh-my/
            Hide
            ray Ray Lawrence added a comment -

            Ouch. Thanks for writing that up.

            Show
            ray Ray Lawrence added a comment - Ouch. Thanks for writing that up.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Dan, could you create separate issue about "Consider backporting MDL-29684 to 22_STABLE". Having it separate would help to vote, spam, discuss, review the implications of such a goal.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Dan, could you create separate issue about "Consider backporting MDL-29684 to 22_STABLE". Having it separate would help to vote, spam, discuss, review the implications of such a goal. TIA and ciao
            danmarsden Dan Marsden made changes -
            Link This issue is a clone of MDL-31374 [ MDL-31374 ]
            Hide
            danmarsden Dan Marsden added a comment -

            done - MDL-31374

            thanks Eloy.

            Show
            danmarsden Dan Marsden added a comment - done - MDL-31374 thanks Eloy.
            skodak Petr Skoda made changes -
            Link This issue is duplicated by MDL-31705 [ MDL-31705 ]
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            here at moodle.ufsc.br we had problems with long time execution of meta enrol cron routines on MySQL 5.5 and managed to lower it creating a temporary table instead of doing a subselect. Here's the diff:

            diff --git a/enrol/meta/locallib.php b/enrol/meta/locallib.php
            index 454c974..624a8e3 100644
            --- a/enrol/meta/locallib.php
            +++ b/enrol/meta/locallib.php
            @@ -474,15 +474,20 @@ function enrol_meta_sync($courseid = NULL, $verbose = false) {
                 $onecourse = $courseid ? "AND e.courseid = :courseid" : "";
                 list($enabled, $params) = $DB->get_in_or_equal(explode(',', $CFG->enrol_plugins_enabled), SQL_PARAMS_NAMED, 'e');
                 $params['courseid'] = $courseid;
            +
            +    $sql = "CREATE TEMPORARY TABLE sub_temp
            +            SELECT xpue.userid, xpe.courseid
            +              FROM {user_enrolments} xpue
            +              JOIN {enrol} xpe ON (xpe.id = xpue.enrolid AND xpe.enrol <> 'meta' AND xpe.enrol $enabled)";
            +    $DB->execute($sql, $params);
            +    $DB->execute("alter table sub_temp add index i_userid(userid)");
                 $sql = "SELECT ue.*
                           FROM {user_enrolments} ue
                           JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'meta' $onecourse)
            -         LEFT JOIN (SELECT xpue.userid, xpe.courseid
            -                      FROM {user_enrolments} xpue
            -                      JOIN {enrol} xpe ON (xpe.id = xpue.enrolid AND xpe.enrol <> 'meta' AND xpe.enrol $enabled)
            -                   ) pue ON (pue.courseid = e.customint1 AND pue.userid = ue.userid)
            +         LEFT JOIN sub_temp pue ON (pue.courseid = e.customint1 AND pue.userid = ue.userid)
                          WHERE pue.userid IS NULL";
                 $rs = $DB->get_recordset_sql($sql, $params);
            +    $DB->execute("DROP TABLE sub_temp");
                 foreach($rs as $ue) {
                     if (!isset($instances[$ue->enrolid])) {
                         $instances[$ue->enrolid] = $DB->get_record('enrol', array('id'=>$ue->enrolid));

            Hope that helps,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, here at moodle.ufsc.br we had problems with long time execution of meta enrol cron routines on MySQL 5.5 and managed to lower it creating a temporary table instead of doing a subselect. Here's the diff: diff --git a/enrol/meta/locallib.php b/enrol/meta/locallib.php index 454c974..624a8e3 100644 --- a/enrol/meta/locallib.php +++ b/enrol/meta/locallib.php @@ -474,15 +474,20 @@ function enrol_meta_sync($courseid = NULL, $verbose = false) { $onecourse = $courseid ? "AND e.courseid = :courseid" : ""; list($enabled, $params) = $DB->get_in_or_equal(explode(',', $CFG->enrol_plugins_enabled), SQL_PARAMS_NAMED, 'e'); $params['courseid'] = $courseid; + + $sql = "CREATE TEMPORARY TABLE sub_temp + SELECT xpue.userid, xpe.courseid + FROM {user_enrolments} xpue + JOIN {enrol} xpe ON (xpe.id = xpue.enrolid AND xpe.enrol <> 'meta' AND xpe.enrol $enabled)"; + $DB->execute($sql, $params); + $DB->execute("alter table sub_temp add index i_userid(userid)"); $sql = "SELECT ue.* FROM {user_enrolments} ue JOIN {enrol} e ON (e.id = ue.enrolid AND e.enrol = 'meta' $onecourse) - LEFT JOIN (SELECT xpue.userid, xpe.courseid - FROM {user_enrolments} xpue - JOIN {enrol} xpe ON (xpe.id = xpue.enrolid AND xpe.enrol <> 'meta' AND xpe.enrol $enabled) - ) pue ON (pue.courseid = e.customint1 AND pue.userid = ue.userid) + LEFT JOIN sub_temp pue ON (pue.courseid = e.customint1 AND pue.userid = ue.userid) WHERE pue.userid IS NULL"; $rs = $DB->get_recordset_sql($sql, $params); + $DB->execute("DROP TABLE sub_temp"); foreach($rs as $ue) { if (!isset($instances[$ue->enrolid])) { $instances[$ue->enrolid] = $DB->get_record('enrol', array('id'=>$ue->enrolid)); Hope that helps, Daniel
            Subversion JIRA

            Links Hierarchy

             Documentation

            Invalid license: EXPIRED

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12