Details

    • Testing Instructions:
      Hide

      This issue requires new QA tests, previously there was not much to test because only restore of manual enrolments worked properly.

      1/ create backups on different sites using different settings and enrolment instances (with/without users)

      2/ verify different restore types (new course, override and add content) with the "Restore as manual enrolments" enabled - it is expected that all enrolments are merged into one manual enrol instance

      3/ verify different restore types (new course, override and add content) with the "Restore as manual enrolments" disabled

      • manual enrolments are restored
      • self enrolments may be merged with existing instances if role matches
      • there is max 1 guest enrol instance
      • all other enrolment types are not supposed to be restored (yet)
      • verify instance sortorder matches the original course

      Note: MDL-27856 implements cohort restore, it could be tested together with this issue.

      Show
      This issue requires new QA tests, previously there was not much to test because only restore of manual enrolments worked properly. 1/ create backups on different sites using different settings and enrolment instances (with/without users) 2/ verify different restore types (new course, override and add content) with the "Restore as manual enrolments" enabled - it is expected that all enrolments are merged into one manual enrol instance 3/ verify different restore types (new course, override and add content) with the "Restore as manual enrolments" disabled manual enrolments are restored self enrolments may be merged with existing instances if role matches there is max 1 guest enrol instance all other enrolment types are not supposed to be restored (yet) verify instance sortorder matches the original course Note: MDL-27856 implements cohort restore, it could be tested together with this issue.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w37_MDL-35071_m24_enrolrestore
    • Rank:
      43690

      Description

      List of solved problems:

      • no way to restore enrolments from disabled/nonexistent/nonrestorable plugins - solution is to add option to "Restore as manual enrolments" options which migrates data to standard enrol_manual plugin
      • no way to map/alter custom fields - such as roles or userid stored in customintX fields
      • roles need to be restored AFTER the enrolments because they need new enrolid
      • enrol->sortorder should be converted to XML field order, we do not need the value in backup file at all
      • support for multiple enrol-self instances
      • trigger automatic sync of all enrol plugins before the restore of first instance - this creates course category enrolments for example, it might trigger LDAP sync, etc.
      • let enrol plugins create instances, enrolments and role assignments - we need as much flexibility as possible, let plugins decide eveything

      Not included in this issue:

      • ability to backup/restore extra tables of enrol plugins
      • only self, manual and guest plugins are fully implemented, the rest will follow if this gets integrated

        Issue Links

          Activity

          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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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

          Show
          Eloy Lafuente (stronk7) added a comment - 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
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - 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, ...
          Hide
          Petr Škoda added a comment -

          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

          Show
          Petr Škoda added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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.
          Show
          Eloy Lafuente (stronk7) added a comment - 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.
          Hide
          Petr Škoda added a comment -

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

          Show
          Petr Škoda added a comment - 2/ if there is already no limit there is no reason to update
          Hide
          Petr Škoda added a comment -

          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

          Show
          Petr Škoda added a comment - 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
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - 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 ).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          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!

          Show
          Eloy Lafuente (stronk7) added a comment - 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!
          Hide
          Daniel Neis added a comment -

          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

          Show
          Daniel Neis added a comment - 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
          Hide
          Petr Škoda added a comment -

          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.

          Show
          Petr Škoda added a comment - 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.
          Hide
          Daniel Neis added a comment -

          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

          Show
          Daniel Neis added a comment - 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
          Hide
          Brianne Thompson added a comment -

          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

          Show
          Brianne Thompson added a comment - 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
          Hide
          Mark Nelson added a comment -

          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!

          Show
          Mark Nelson added a comment - 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!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: