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

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

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: