Database enrolment fixes collected!

Re: Database enrolment fixes collected!

by Martín Langhoff -
Number of replies: 0
Used a mix of both (thanks to both Scott and Jeff for their patches and perseverance!). It does _not_ include the cron bit, because while it is mostly correct, it is not scalable. Very not scalable ;)

I have a enrol/db cron based on what we already have for enrol/ldap that I'm polishing and will eventially make into 1.5.x or 1.6. We normally work with installs with several tens of thousands of users, so it _has_ to be scalable, even if that means that it is a bit harder to understand & work on.

I chose to give enrol_student() a default of 'manual' ... which is what it had when I committed the addition of the enrol column to user_student -- it got lost at some point closer to 1.5 and there's where problems started smile

This is important: defaulting to manual makes sense because the plugins that handle external auth are a narrow set of cases, and they know to set their own type. There's a wider use of the function across the codebase (like course/students.php) and those changes to enrolments are completely invalid if marked as an external enrolment because they haven't been recorded in the external data source, and thus will be removed when the plugin kicks in.

And at any rate, user_students() doesn't have enough info to know better, so it should default to something simple/safe.

Now, there are some TODOs left that I'm interested in getting patches for if people are keen:

- We need an 'enrol_is_internal()' function similar to is_internal() but for enrolment plugins.

- Hunt down places where enrol_student() is being called outside the enrol plugins, and make those actions conditional on (enrol_is_internal($CFG->enrol) || $CFG->enrol_allowinternal). For example, course/students.php shoud only allow to enrol/unenrol based on that condition.

- In the enrol/db/enrol.php I committed, the way of removing 'stale' db enrolments is _very_ inefficient: it'll hit the database once per pending enrolment entry to check their 'enrol' field. enrol/ldap/enrol.php has a better approach that hits the database only once... OR perhaps a better fix is to pull the 'enrol' field earlier on, from enrol.class, and put it in the 'value' of the array entry, which is currently set to something stupid like "1".

So yay! - This is getting a cleanup at last. You guys have shown good taste in your patches... I'm keen on seeing more ;)

(I replied to this post yesterday, but my session must have timed out or something. Bah, Computers! ;)