Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.3
    • Fix Version/s: 2.4
    • Component/s: Backup, Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      1/ backup course with several cohort sync instances (different cohorts and different roles)
      2/ do a default restore on different site - no enrolments or instances should be restore
      3/ restore on different site with "Restore as manual enrolments" options - all enrolments should be restored as manual
      4/ restore on the same site - all cohort related data should be restored 100%
      5/ delete one of the used cohorts
      6/ in enrol_cohort admin settings select "Unenrol user from course" - restore in the same site and verify the removed cohort sync instances and enrolments are not there
      7/ in enrol_cohort admin settings select "Disable course enrolment and delete roles" - restore in the same site and verify users from deleted cohorts are still enrolled, but without roles and suspended

      Show
      1/ backup course with several cohort sync instances (different cohorts and different roles) 2/ do a default restore on different site - no enrolments or instances should be restore 3/ restore on different site with "Restore as manual enrolments" options - all enrolments should be restored as manual 4/ restore on the same site - all cohort related data should be restored 100% 5/ delete one of the used cohorts 6/ in enrol_cohort admin settings select "Unenrol user from course" - restore in the same site and verify the removed cohort sync instances and enrolments are not there 7/ in enrol_cohort admin settings select "Disable course enrolment and delete roles" - restore in the same site and verify users from deleted cohorts are still enrolled, but without roles and suspended
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w37_MDL-27856_m24_cohortrestore

      Description

      This line is firing off for enrol/cohort:

      debugging("Skipping '{$data->enrol}' enrolment plugin. Will be implemented before 2.0 release", DEBUG_DEVELOPER);

      Other core enrol plugins should also be addressed (As of filing this ticket, I think only 3 core ones have the hook)

      In order to re-produce, I think you only have to enrol a user with the cohort plugin and then backup/restore that course.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              bushido Mark Nielsen created issue -
              bushido Mark Nielsen made changes -
              Field Original Value New Value
              Description This line is firing off for enrol/cohort:
              {noformat}
              debugging("Skipping '{$data->enrol}' enrolment plugin. Will be implemented before 2.0 release", DEBUG_DEVELOPER);
              {noformat}

              Other core enrol plugins should also be addressed (As of filing this ticket, I think only 3 core ones have the hook)
              This line is firing off for enrol/cohort:
              {noformat}
              debugging("Skipping '{$data->enrol}' enrolment plugin. Will be implemented before 2.0 release", DEBUG_DEVELOPER);
              {noformat}

              Other core enrol plugins should also be addressed (As of filing this ticket, I think only 3 core ones have the hook)

              In order to re-produce, I think you only have to enrol a user with the cohort plugin and then backup/restore that course.
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for reporting this.

              I've put it on our backlog and we'll try to get to it as soon as we can.

              Show
              salvetore Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can.
              salvetore Michael de Raadt made changes -
              Fix Version/s STABLE backlog [ 10463 ]
              Priority Minor [ 4 ] Critical [ 2 ]
              Labels partner triaged
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              I think this duplicates MDL-22148 but I'm keeping it open for now.

              Really this needs to be addressed, should be since 2.0, but it's not clear/defined, the way to follow, grrr.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited I think this duplicates MDL-22148 but I'm keeping it open for now. Really this needs to be addressed, should be since 2.0, but it's not clear/defined, the way to follow, grrr.
              stronk7 Eloy Lafuente (stronk7) made changes -
              Link This issue duplicates MDL-22148 [ MDL-22148 ]
              dobedobedoh Andrew Nicols made changes -
              Link This issue has a clone MDL-30701 [ MDL-30701 ]
              skodak Petr Skoda made changes -
              Assignee Eloy Lafuente (stronk7) [ stronk7 ] Petr Škoda (skodak) [ skodak ]
              skodak Petr Skoda made changes -
              Parent MDL-34696 [ 57979 ]
              Rank (Obsolete) 162810000000
              Issue Type Bug [ 1 ] Sub-task [ 5 ]
              skodak Petr Skoda made changes -
              Link This issue is blocked by MDL-35071 [ MDL-35071 ]
              skodak Petr Skoda made changes -
              Summary Cohort enrol plugin missing supports callback Cohort restore support
              Fix Version/s DEV backlog [ 10464 ]
              Fix Version/s STABLE backlog [ 10463 ]
              Priority Critical [ 2 ] Major [ 3 ]
              Affects Version/s 2.3 [ 10657 ]
              Hide
              skodak Petr Skoda added a comment - - edited

              MDL-35071 allows you to restore any enrolment as manual enrolment, I am going to implement restore on the same site in this issue, restore from other site is problematic because we do not have any info about the cohort on other site.

              Show
              skodak Petr Skoda added a comment - - edited MDL-35071 allows you to restore any enrolment as manual enrolment, I am going to implement restore on the same site in this issue, restore from other site is problematic because we do not have any info about the cohort on other site.
              skodak Petr Skoda made changes -
              Status Open [ 1 ] Development in progress [ 3 ]
              skodak Petr Skoda made changes -
              Summary Cohort restore support Cohort enrolment restore support
              skodak Petr Skoda made changes -
              Link This issue duplicates MDL-22148 [ MDL-22148 ]
              skodak Petr Skoda made changes -
              Link This issue has been marked as being related by MDL-22148 [ MDL-22148 ]
              skodak Petr Skoda made changes -
              Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
              Pull Master Diff URL https://github.com/skodak/moodle/compare/w36_MDL-35071_m24_enrolrestore...w36_MDL-27856_m24_cohortrestore
              Pull Master Branch w36_MDL-27856_m24_cohortrestore
              Pull from Repository git://github.com/skodak/moodle.git
              Fix Version/s 2.4 [ 12255 ]
              Fix Version/s DEV backlog [ 10464 ]
              Testing Instructions 1/ backup course with several cohort sync instances
              2/ restore course on different site - no enrolments or instances should be restore
              3/ restore on the same site - everything should be restored as is
              4/ delete on of the used cohorts
              5/ in enrol_cohort admin settings select "Unenrol user from course" - restore and verify the removed cohort is not in restores site
              6/ in enrol_cohort admin settings select "Disable course enrolment and delete roles" - restore and verify users from deleted cohorts are still enrolled, but without roles and suspended
              skodak Petr Skoda made changes -
              Testing Instructions 1/ backup course with several cohort sync instances
              2/ restore course on different site - no enrolments or instances should be restore
              3/ restore on the same site - everything should be restored as is
              4/ delete on of the used cohorts
              5/ in enrol_cohort admin settings select "Unenrol user from course" - restore and verify the removed cohort is not in restores site
              6/ in enrol_cohort admin settings select "Disable course enrolment and delete roles" - restore and verify users from deleted cohorts are still enrolled, but without roles and suspended
              1/ backup course with several cohort sync instances (different cohorts and different roles)
              2/ do a default restore on different site - no enrolments or instances should be restore
              3/ restore on different site with "Restore as manual enrolments" options - all enrolments should be restored as manual
              4/ restore on the same site - all cohort related data should be restored 100%
              5/ delete one of the used cohorts
              6/ in enrol_cohort admin settings select "Unenrol user from course" - restore in the same site and verify the removed cohort sync instances and enrolments are not there
              7/ in enrol_cohort admin settings select "Disable course enrolment and delete roles" - restore in the same site and verify users from deleted cohorts are still enrolled, but without roles and suspended
              samhemelryk Sam Hemelryk made changes -
              Currently in integration Yes [ 10041 ]
              skodak Petr Skoda made changes -
              Summary Cohort enrolment restore support Cohort enrolment restore support !!!!!IMPORTANT!!!!!!
              skodak Petr Skoda made changes -
              Summary Cohort enrolment restore support !!!!!IMPORTANT!!!!!! Cohort enrolment restore support
              Hide
              stronk7 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
              stronk7 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
              stronk7 Eloy Lafuente (stronk7) made changes -
              Currently in integration Yes [ 10041 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Currently in integration Yes [ 10041 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
              Integrator stronk7
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This looks ok, just some of the comments @ MDL-35071 apply here too, I think:

              1) the return self::SKIP_ALL_CHILDREN if applicable.
              2) I assume enrol_cohort_sync() performs all the tasks (user enrolments and role assignments), so restore does not need ever to perform any of those actions explicitly ever.
              3) the cohort enrolment does NOT have any fallback to other enrolments (manual, self) at all. Correct?

              I think this can be integrated safely after MDL-35071 lands.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This looks ok, just some of the comments @ MDL-35071 apply here too, I think: 1) the return self::SKIP_ALL_CHILDREN if applicable. 2) I assume enrol_cohort_sync() performs all the tasks (user enrolments and role assignments), so restore does not need ever to perform any of those actions explicitly ever. 3) the cohort enrolment does NOT have any fallback to other enrolments (manual, self) at all. Correct? I think this can be integrated safely after MDL-35071 lands. Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated with one extra commit fixing one incorrect "id" assignment.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated with one extra commit fixing one incorrect "id" assignment.
              stronk7 Eloy Lafuente (stronk7) made changes -
              Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
              stronk7 Eloy Lafuente (stronk7) made changes -
              Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
              Tester stronk7
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Everything seems to work perfectly, so I'm passing this but with this TODO (note the second point may be important!:

              A) the return self::SKIP_ALL_CHILDREN if applicable.
              B) restore does not perform any validation of the cohort context. So I was able to restore one course with cohorts in category A into category B without problems (A and B belonging to different branches 100%). It seems that the UI does not allow to pick cohorts transversally, so my understanding is that restore shouldn't either.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Everything seems to work perfectly, so I'm passing this but with this TODO (note the second point may be important!: A) the return self::SKIP_ALL_CHILDREN if applicable. B) restore does not perform any validation of the cohort context. So I was able to restore one course with cohorts in category A into category B without problems (A and B belonging to different branches 100%). It seems that the UI does not allow to pick cohorts transversally, so my understanding is that restore shouldn't either. Ciao
              stronk7 Eloy Lafuente (stronk7) made changes -
              Status Testing in progress [ 10011 ] Tested [ 10006 ]
              Hide
              skodak Petr Skoda added a comment -

              The cohort access is verified only when crating new things, later you can move around the cohorts in any way. Here it is tricky because you can gain access to cohort via hacked backup file, but I guess you can do a lot worse there already.

              Show
              skodak Petr Skoda added a comment - The cohort access is verified only when crating new things, later you can move around the cohorts in any way. Here it is tricky because you can gain access to cohort via hacked backup file, but I guess you can do a lot worse there already.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              But in one course @ category B it's now allowed (via UI) to pick any cohort from category A. And restore is allowing that. It should be prevented IMO.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - But in one course @ category B it's now allowed (via UI) to pick any cohort from category A. And restore is allowing that. It should be prevented IMO.
              Hide
              skodak Petr Skoda added a comment - - edited

              Internally cohort sync may depend on any existing cohort - there is no capability check once it is set up. The question is do you want to have 100% matching restore or mess it up with capabilities - please note that cohort capabilities are outside of the the course and the role of the restoring user may not be set up yet. That means you are proposing to "break" cohort sync restore for most people, for me personally there is no security problem in restoring of ANY cohort sync without any special cap - remember there might be XSS everywhere in backup files which gives you admin access easily...

              Show
              skodak Petr Skoda added a comment - - edited Internally cohort sync may depend on any existing cohort - there is no capability check once it is set up. The question is do you want to have 100% matching restore or mess it up with capabilities - please note that cohort capabilities are outside of the the course and the role of the restoring user may not be set up yet. That means you are proposing to "break" cohort sync restore for most people, for me personally there is no security problem in restoring of ANY cohort sync without any special cap - remember there might be XSS everywhere in backup files which gives you admin access easily...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited

              Nono, I don't "want" nor am proposing anything. I'm just asking!

              From your comment I conclude:

              A) There is nothing wrong about having transversal cohorts sync, the system allows it.
              B) But for some reason, that (transversal cohorts sync) is not allowed via UI.

              And then, simply, I ask:

              C) Which one should be restore behavior? Lead to allowing transversal cohort sync (A) or prevent transversal cohort sync (B).

              Simply that, because I found A and B contradictory, nothing else. Perhaps, in other words... what happens when you move one course with cohort syncs to another course category, A or B ?

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - - edited Nono, I don't "want" nor am proposing anything. I'm just asking! From your comment I conclude: A) There is nothing wrong about having transversal cohorts sync, the system allows it. B) But for some reason, that (transversal cohorts sync) is not allowed via UI. And then, simply, I ask: C) Which one should be restore behavior? Lead to allowing transversal cohort sync (A) or prevent transversal cohort sync (B). Simply that, because I found A and B contradictory, nothing else. Perhaps, in other words... what happens when you move one course with cohort syncs to another course category, A or B ? Ciao
              Hide
              skodak Petr Skoda added a comment -

              Ah! the UI is designed to limit the number of cohorts in a selection box - that is it looks into parent categories only + for security it uses capabilities. My expected behaviour for restore is to use anything that is in the backup file.

              Your "transversal" cohort thing is allowed because we would otherwise have to deal with cohorts when rearranging the course categories or courses in the context tree.

              Show
              skodak Petr Skoda added a comment - Ah! the UI is designed to limit the number of cohorts in a selection box - that is it looks into parent categories only + for security it uses capabilities. My expected behaviour for restore is to use anything that is in the backup file. Your "transversal" cohort thing is allowed because we would otherwise have to deal with cohorts when rearranging the course categories or courses in the context tree.
              Hide
              stronk7 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
              stronk7 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!
              stronk7 Eloy Lafuente (stronk7) made changes -
              Status Tested [ 10006 ] Closed [ 6 ]
              Resolution Fixed [ 1 ]
              Currently in integration Yes [ 10041 ]
              Integration date 14/Sep/12

                People

                • Votes:
                  2 Vote for this issue
                  Watchers:
                  3 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12