Moodle
  1. Moodle
  2. MDL-29684

meta course sync plugin fixes and improvements

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
    • Rank:
      19193

      Description

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

        Issue Links

          Activity

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

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

          Show
          Ray Lawrence added a comment - 2 - And your permissions in that course are determined by...?
          Hide
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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 Lawrence added a comment -

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

          Show
          Ray Lawrence added a comment - Thanks for those confirmations. We're have the same understanding but express that in different ways...
          Petr Škoda made changes -
          Link This issue is blocked by MDL-30789 [ MDL-30789 ]
          Petr Škoda made changes -
          Link This issue is blocked by MDL-30789 [ MDL-30789 ]
          Hide
          Petr Škoda 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
          Petr Škoda 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.
          Petr Škoda made changes -
          Status Waiting for integration review [ 10010 ] Reopened [ 4 ]
          Petr Škoda made changes -
          Status Reopened [ 4 ] Development in progress [ 3 ]
          Petr Škoda made changes -
          Link This issue has been marked as being related by MDL-30509 [ MDL-30509 ]
          Petr Škoda made changes -
          Summary meta course: nosyncroleids doesn't work meta course sync plugin fixes and improvements
          Petr Škoda made changes -
          Hide
          Petr Škoda added a comment -

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

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

          I have added option to completely unenrol suspended users.

          Show
          Petr Škoda added a comment - I have added option to completely unenrol suspended users.
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          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
          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
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Waiting for integration review [ 10010 ]
          Sam Hemelryk made changes -
          Integrator samhemelryk stronk7
          Hide
          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
          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
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Hide
          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
          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
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.2.2 [ 11552 ]
          Fix Version/s 2.2.1 [ 11456 ]
          Petr Škoda made changes -
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Hide
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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.
          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 ]
          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 ]
          Sam Hemelryk made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester samhemelryk
          Hide
          Sam Hemelryk added a comment -

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

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

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

          Closing, ciao

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

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

          Show
          Marcelo Minholi added a comment - It will be integrated to 22_STABLE? I need this to solve a big problem here.
          Hide
          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
          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
          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
          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 Lawrence added a comment -

          Ouch. Thanks for writing that up.

          Show
          Ray Lawrence added a comment - Ouch. Thanks for writing that up.
          Hide
          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
          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
          Dan Marsden made changes -
          Link This issue is a clone of MDL-31374 [ MDL-31374 ]
          Hide
          Dan Marsden added a comment -

          done - MDL-31374

          thanks Eloy.

          Show
          Dan Marsden added a comment - done - MDL-31374 thanks Eloy.
          Petr Škoda made changes -
          Link This issue is duplicated by MDL-31705 [ MDL-31705 ]
          Hide
          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
          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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: