Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major 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
    • Rank:
      17508

      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.

        Issue Links

          Activity

          Mark Nielsen created issue -
          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
          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
          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.
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Priority Minor [ 4 ] Critical [ 2 ]
          Labels partner triaged
          Hide
          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
          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.
          Eloy Lafuente (stronk7) made changes -
          Link This issue duplicates MDL-22148 [ MDL-22148 ]
          Andrew Nicols made changes -
          Link This issue has a clone MDL-30701 [ MDL-30701 ]
          Petr Škoda made changes -
          Assignee Eloy Lafuente (stronk7) [ stronk7 ] Petr Škoda (skodak) [ skodak ]
          Petr Škoda made changes -
          Parent MDL-34696 [ 57979 ]
          Rank (Obsolete) 162810000000
          Issue Type Bug [ 1 ] Sub-task [ 5 ]
          Petr Škoda made changes -
          Link This issue is blocked by MDL-35071 [ MDL-35071 ]
          Petr Škoda 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
          Petr Škoda 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
          Petr Škoda 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.
          Petr Škoda made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Petr Škoda made changes -
          Summary Cohort restore support Cohort enrolment restore support
          Petr Škoda made changes -
          Link This issue duplicates MDL-22148 [ MDL-22148 ]
          Petr Škoda made changes -
          Link This issue has been marked as being related by MDL-22148 [ MDL-22148 ]
          Petr Škoda 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
          Petr Škoda 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
          Sam Hemelryk made changes -
          Currently in integration Yes [ 10041 ]
          Petr Škoda made changes -
          Summary Cohort enrolment restore support Cohort enrolment restore support !!!!!IMPORTANT!!!!!!
          Petr Škoda made changes -
          Summary Cohort enrolment restore support !!!!!IMPORTANT!!!!!! Cohort enrolment restore support
          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
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes [ 10041 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator stronk7
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

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

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated with one extra commit fixing one incorrect "id" assignment.
          Eloy Lafuente (stronk7) made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Eloy Lafuente (stronk7) made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Tester stronk7
          Hide
          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
          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
          Eloy Lafuente (stronk7) made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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
          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
          Petr Škoda 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
          Petr Škoda 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
          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!
          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: