Moodle

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.

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?

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.

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.

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.)

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

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)

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.

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

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

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

I have added option to completely unenrol suspended users.

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

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

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

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

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

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

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

Integrated (master only), thanks!

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

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

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

Closing, ciao

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

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

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/

Ouch. Thanks for writing that up.

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

done - MDL-31374

thanks Eloy.