Redesign enrol restore support

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

Typo:

1) $this->plguins typo.

2) If I don't understood wrong, the idea in manual->restore_user_enrolment() is to apply timestart and timeend from backup only if they lead to "bigger" periods of enrolment, correct? Just guessing if around #350 there is one missing $enrol = true;

3) Perhaps it could be a good idea to change the source to ordered sql also in stables (line #545 exclusively) so, while still including sortorder, we have those backups already ordered?

4) The creation of enrol->restore_role_assignment() is for the cases where one enrol plugin is transformed into another one, correct? So users enrolled originally with, say enrol_ldap, will end being restored with enrol_manual, for example. So only "fallback" enrol plugins will have that method implemented? enrol_ldap will perform its own role_assignments not using those methods at all?

5) Instead of mapping to 0 + "return;" and, later in children XML elements perform the mapping check to decide if process must happen, perhaps this could be a good place to use "return self::SKIP_ALL_CHILDREN;" It has the effect of instructing the parser to automatically skip all the children information, so it will jump automatically to the next enrol instance in the backup file. It makes a difference if there are a lot of children information (user_enrolments) to process. Y think we used that technique with question categories and differences were notable.

6) If it's only possible to have ONE instance of the manual/self enrol plugins in a course... why are we doing all the time get_records + reset ?

7) Those debugging()... perhaps they should be transformed into logs/results and allow the process to show them at the end (together with other incidences in restore). (this->task->add_result() and this->log()). In any case it would be interesting to know a bit more about which enrol was the problematic one or whatever.

8) Finally the setting (enrol_migratetomanual). Not sure why I was thinking the goal of that setting was to "move" enrolments from xxxx to manual when xxxxx can not be restored properly. But it seems that it just "forces" the move and done. Always. Is that correct? So then, who is going to decide those "moves" when the setting is not set? Each enrol plugin? Say ldap can decide to perform the move to manual and database can decide to perform the move to self? (silly example, I know, just trying to imagine who decides the move).

It looks ok, it took a bit of time to get the idea in my mind... I think that it can land once we sort out the points above. Will keep this open, just in case.

Ciao

1/ arrrgh typos - I will fix it when I wake up, sorry

2/ there can be only one manual plugin, that means if there is already one enrolment and you want to restore another it just tries to extend the end of enrolment if necessary

3/ yes, we could sort the tables in stables too if this gets accepted, the restores of older backups would be more predictable

4/ each plugin is responsible for restoring of own roles (those with component and itemid) - the enrol_manual is a tiny exception here, it is designed to accept any other roles and enrolments

5/ only plugin can decide if the children should be skipped or not when restoring, I will have a look if there are some candidates for this return code

6/ theoretically only one manual, but there were some bugs and race conditions that may result in creation of several manual instances

7/ I did not study notification in restore/backup yet, hopefully there should be no problems with enrol plugins - this may need more explanation in early restore stages, I will change it to ->log() asap

8/ No, the forced migration option is designed for restores from other sites and old backups, for example ldap - the restored enrolments would be immediately wiped out - sometimes you want it sometimes not. It is just a setting that "forces everything to be restored as manual enrolments" that can be easily modified later and are not affected by any other plugin. In the "normal" restore plugins may decide to do whatever they want - restore as own enrolments + role, skip, change to manual, ...

1/ fixed typos

5/ the return $this::SKIP_ALL_CHILDREN; requires change of API, I am not going to add this now because it would changes in multiple linked pull issues, if this gets accepted I can do it next week

7/ the result and log things are not documented much, I need more time to study this and I should probably fix the docs at the same time - I suppose this can be also done in a subsequent issue

2/ yeyes, but if extending... isn't #350 missing one "$enrol = true" when expanding timeend up to the infinite (zero)?

So TODO (in followup issues, apart from the 2/ above):

• Enforce the ordering in supported stables, so we get them that way often.
• Analyze if SKIP_ALL_CHILDREN can save some cycles.
• change debugging by results and logs.

2/ if there is already no limit there is no reason to update

and yes, I agree with the TODOs, I have already tried the changes, but I did not have time to retest everything, so next week if everything works as expected

I'm going to integrate and pass this with this list of TODOs:

A- Enforce the ordering in supported stables, so we get them that way often.
B- Analyze if SKIP_ALL_CHILDREN can save some cycles.
C- Change debugging by results and logs.
D- Analyze how the roll dates feature should affect all the user enrollments. Right now the feature is ignored completely and start/end time are not changed ever.
E- When restoring over existing course merging information twice, if the second timewe specify "restore as manual enrollments", the participants end having 2 enrollments, the original (self, cohort...) and one manual duplicate. Does it sense to create those manual dupes if the "same" (course, user, duration) enrollment already exists?

Apart from that I've tested cohorts, self, manual and guest between sites, same site, changing start/end dates, with and without users and everything seems to be working ok (but a little problem commented @ MDL-27856).

Gutta cavat lapidem, non vi sed saepe cadendo - Ovidio

This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

Thanks!

Hello,

i'v noted this issue fix two existing bugs in 2.3 restore process:

• The first is that role assignments were restored before enrolments
• The second is an erroneous test for the component

Any chanches of backport these to 2.3?

Kind regards,
Daniel

No backport of enrolment improvements and fixes is planned, sorry. Please upgrade your servers once 2.4.0 is released. In the meantime please help with QA testing, thanks.

Hello,

We can't wait for 2.4 so here is a link for the fix if someone want to manually apply:

https://github.com/danielneis/moodle/tree/MDL-35071-v23-partial

Kind regards,
Daniel

Hello,

I have confirmed the presence of this bug in version 2.4.6. Can I get a confirmation that the fix made its way to the 2.4 release?

Brianne

Hi Brianne,

Just had a look. The patch was included in 2.4.

If this is still an issue could you please create a separate issue with detailed steps to reproduce as it may be a unique scenario as this did pass testing.

Cheers!