Moodle

I'm not sure if this is still relevant in 1.9, pushing to 2.0

UPDATE role_assignments SET nowactive = (timestart < $now AND (timeend >= $now OR timeend=0 ) )

This patch applies (currently! It will rot fast so I'd like to check it in if ok) to 2.0 HEAD.

It does the following:

1) Adds the 'active' field to role_asssignments ('current' is a reserved word according to xmldb). Default value is 1. This means behaviour will not change for any existing role assignments (even if they have an existing timestart value that is currently being ignored, it will continue to be ignored).

2) In cron, updates this field based on timestart. The change only alters rows that need changing, and does not take notice of timeend since that is already handled correctly in cron by deleting those assignments.

3) In accesslib, for functions which are used to determine permissions, changes the queries to include 'AND ra.active=1' or similar.

4) In role_assign, roles that are assigned with timestart > now have their 'active' field set to 0.

5) In require_login, an additional check is added so that when you don't have access to a course and it would normally take you to the 'enrol' screen, if you have a not-yet-started role on the course, it instead tells you that you have to wait, and gives the date. (This requires a new language string.) Otherwise when you have a not-started role, clicking on a course takes you to an enrol.php loop ('Do you want to enrol? Yes? checks You're already enrolled, redirect to course... Do you want to enrol?')

I had a skim of the code and your comments. Makes good sense to me!

Eloy and Petr can you have a quick look?

How does everyone feel about this in 1.9? It's really late, I know, but it does fix a bug.

Some comments about the patch:

1) From a DB perspective, I think we are on time. Right now both 1.9 and 2.0 are on sync, so we CAN update.

2) While looking to the patch, I think that it's overall ok (I'm not a roles expert) although I think all this should be fulfilled with it:

---- 2A) All the functions in accessib accessing to role_assignments should add that active = 1 condition (excerpt the ones wanting to return "disabled-by-time" roles, of course). Not sure if current patch modifies all the needed places. I leave this for experts.

---- 2B) When inserting/updating role_assignments from UI.... I think we should check both startime and endtime to calculate the "active" correct value. Currently only starttime is checked (I know it hasn't too much sense to insert two dates in the past, but...).

----2C) One minor detail, when looking for non-active role_assignments, the "AND ra.timestart<>0" condition seems to be useless. Not critical, but a detail, if I'm not wrong.

3) If I've understand the thing properly the idea is that, in cron.php, any inactive role having its starttime in the past will be activated. And we are skipping timeend on purpose because the same cron.php script is responsible for deleting "expired" assignments, correct? Then, I've saw that right now, that part of the cron.php script ("Removing expired enrolments") has one curious inter-relation with course->enrolperiod, deleting only assignments belonging to courses with enrolperiod. And that has sounded me really UN-BALANCED, please check an verify if that approach is correct (i.e. if there is any relation between those concepts). I feel thay shouldn't be related at all, but I'm not 100% sure.

And that's all I can say. I think it's an interesting feature to have working properly and pre-calculating active assignments sounds reasonable (you know we are assuming some delay until cron execution, of course) to avoid those inequalities in queries.

Hope this helps. Ciao

2A - I thought about this but actually I think it should ONLY be the parts of accesslib that are concerned with whether you have access to do something, not the ones that might be used to obtain a list of people. The reason is that otherwise you would add somebody with a timestart, and they wouldn't even show up in the list! It would probably let you add them twice, etc. The places I changed are the ones used for has_capability and two other permission checks (one for whether you can assign roles, one for whether you are admin).

2B - Could do that, but as you note it won't ever happen so...! For now I thought it's easier to make 'active' deal only with starttime.

2C - I think you are right.

3- Yes somebody needs to think about this (probably not me as I am not familiar enough with the enrol period stuff). In particular does the UI let you add timeend for courses that don't have enrolperiod, etc.

The attached is an updated (not significantly changed) patch to enable timeend support via an 'active' flag, based on code I have also checked into our OU codebase. We delayed implementing this here because of an issue in update. It is now going into our next release, live in July and due to enter testing w/ live data in about 2 weeks.

This patch is for Moodle HEAD.

Based on the earlier comments I think it is ok to check this in. I fixed Eloy's 2C, explained why 2A isn't the case, didn't bother with 2B (cron will clear this up because if timeend is past, the expired assignment will be deleted - so this is consistent with other cases where end date = delete, start date = toggle active value), and am ignoring 3 as it's not directly related (orthogonal) to this change.

One note is that I have now tested upgrade behaviour (I had concerns about it before - that's why we delayed our release). I did a fresh install of HEAD from just before this patch, then did the patch, then went to the admin page to update: no problems. (Between the code change and visiting admin page, users will not be able to access anything and will get SQL errors because of the field that hasn't been added yet, however user access should be disabled during updates anyway.)

So, I'll check this into HEAD tomorrow if nobody objects. My suggestion would be that this be left in HEAD for a while so that any obvious problems can be caught, then somebody can then make a decision as to whether it should go into 1.9.x. (I am happy to port the patch to 1.9.x at that point if needed.)

Well, I've one objection. And it's exactly 3) above.

If you apply your patch, Moodle will:

a) Activate enrolments at timestart. (this is ok, and provided by your patch)
b) Remove enrolments at timend but ONLY then the course has one enrolperiod defined.

This will leave enrolments enabled forever if the course doesn't have enrolperiod defined. And that isn't a correct behaviour IMO, can cause a lot of unexpected results. So, before committing this, I'd suggest to decide how we balance the a) and b) conditions. There are two simple solutions:

1) We add the "the course has one enrolperiod defined" condition to a)
2) We delete the "the course has one enrolperiod defined" condition from b)

But they must be balanced before committing or we'll forget this in the future, for sure. My vote is about to apply 2), but need Martin or Petr confirmation (I really don't know if the condition can be simply removed or no).

The rest looks robust enough IMO. Plz... comment... ciao

OK, I understand your objection. I still think this is a separate issue but I agree if we could agree to remove the enrolperiod restriction for deleting old entries, then it might be better to do that at the same point.

However I can imagine that might not be agreed because presumably that is intended so that if you turn off the 'limit enrolment length' option, it stops unenrolling students. If that should still work, but we also want to remove the condition on timeend, then we would need to do something different - for example make that enrol expiry option not use timeend at all, but use the enrolperiod length + timestart (which we would have to make sure is set right). So we would expire enrolments for 2 cases: timeend < $now or timestart < $now-$course->enrolperiod. That would actually be better and would allow enrolperiod to work alongside timeend being set for other reasons outside the UI. [Which the OU does, incidentally; we commented out the enrolperiod restriction.] Of course it would mean a change to the way the system works in that the enrolment period is defined dynamically rather than being fixed as it was at the time each student was enrolled... Another possibility would be to add a second timeend field. One or other of these options is necessary if we want to allow timeend to be used for other reasons than enrolperiod.

But that's, really, a separate issue from the timestart stuff in general. I think they are not related and do not need to be balanced. Timestart is about not letting students get into the course before it's ready, whereas enrolperiod (and timeend, while it's used for enrolperiod) are about making it so that a student's enrolment in the course lasts a certain number of days/whatever. Those are really separate issues - you might have a course that doesn't limit the length of time a student belongs to it, but you still don't want to let students into the course until the official start date.

I may have misunderstood something but that's what I think at present!

My +1 to have a bigger discussion about course enrolments in general after we finish the datalib changes. There are several other major problems related to enrolments
I do not think we should be committing any 2.0 specific changes/fixes yet.

Patches and wiki docs/proposals are welcome anyway.

OK, that is a bit disappointing because we have already done this in our own code - yet another difference, and in accesslib too which is a bit of a vital part - but as long as somebody catches this later and does make it actually work, I guess I don't care whether it is this patch or another one.

(I won't be making any more changes to it, so feel free to use this patch, or not.)

If there is definitely no intention to fix this in 1.9, somebody should remove/hide the UI related to timestart since it has no effect.

Enrolment tables now contains this information, closing as fixed :-D