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

          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