Moodle
  1. Moodle
  2. MDL-27233

Performing the restore of a course, from a teacher's perspective, flushs the role assignements

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1/ backup course without enrolments and roles
      2/ restore the course into the same course deleting data using teacher account
      3/ verify teacher is reenrolled into course during restore
      4/ enrol some users and create some groups
      5/ restore again overriding current course but keeping enrols and groups (new options in course settings)

      Show
      1/ backup course without enrolments and roles 2/ restore the course into the same course deleting data using teacher account 3/ verify teacher is reenrolled into course during restore 4/ enrol some users and create some groups 5/ restore again overriding current course but keeping enrols and groups (new options in course settings)
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w47_MDL-27233_m22_restoreenrol
    • Rank:
      16910

      Description

      Performing the restore of a course, from a teacher's perspective, flushs the role assignements. Permissions have been checked for the teacher to attribute role assignements to users in a restore process.

      Steps to replicate the bug :

      1- As administrator, Create an "empty course" for the test
      2- Assign another user as teacher of the course.
      3- Log in as this user and perform a backup of the course. Assure that "Include enrolled users" and "Include user role assignments" are checked.
      4- Perform the restore of the course. Choose "Delete the contents of this course and then restore" and continue.Assure that "Include enrolled users" and "Include user role assignments" are checked.

      After restoring, its says "This course is currently unavailable to students".

      If you go back as administrator, you can see that the user is still there but he is no more teacher of the course.

      --------------------------------------------------------------------------

      I dug the code a bit and found these things :

      1- The function process_assignement

      "/moodle/backup/moodle2/restore_stepslib.php"
      public function process_assignment($data) {
         global $DB;
      
         $data = (object)$data;
      
         // Check roleid, userid are one of the mapped ones
         if (!$newroleid = $this->get_mappingid('role', $data->roleid)) {
            return;
         }
      
         ...
      

      The method get_mappingid returns 0 when it should return 3 and so the assignment fails.
      get_mappingid is checking on the column newitemid of the temporary table backup_ids_temp. If I output the table content, I can clearly see that it is effectively set to 0.

      "backup_ids_temp content"
      ...
      
      [8] => stdClass Object ( [id] => 8 [backupid] => c07aa265cba1cccd0e58119906ca6576 [itemname] => role [itemid] => 3 [newitemid]=> 0
      
      ... 
      

      2- The functions that affect the content of the table "backup_ids_temp"

      "/moodle/backup/util/dbops/restore_dbops.php"
      protected static function get_best_assignable_role($role, $courseid, $userid, $samesite) {
         global $CFG, $DB;
      
         // Gather various information about roles
         $coursectx = get_context_instance(CONTEXT_COURSE, $courseid);
         $allroles = $DB->get_records('role');
         $assignablerolesshortname = get_assignable_roles($coursectx, ROLENAME_SHORT, false, $userid);
      
         ...
      

      get_assignable_roles returns an empty array of role.

      I think the problem may be that the role assignments are deleted before the call to the function, so the match can't be done.

      Hope it helps!

        Issue Links

          Activity

          Hide
          Kathryn Fortin added a comment -

          We have clients experiencing this problem too.

          Show
          Kathryn Fortin added a comment - We have clients experiencing this problem too.
          Hide
          Michael Blake added a comment -

          This issue has been reported as causing problems for a MP client. Please give it priority.

          Show
          Michael Blake added a comment - This issue has been reported as causing problems for a MP client. Please give it priority.
          Hide
          Anthony Borrow added a comment -

          The issue here is that the course roles are being wiped out by the deletion process including the teacher role and thus the teacher loses access. More details are provided about where in the code that I believe the problem exists. In particular, remove_course_contents in moodlelib.php is calling role_unassign_all. I would advocate for removing content and removing roles as being separate functions. Peace - Anthony

          Show
          Anthony Borrow added a comment - The issue here is that the course roles are being wiped out by the deletion process including the teacher role and thus the teacher loses access. More details are provided about where in the code that I believe the problem exists. In particular, remove_course_contents in moodlelib.php is calling role_unassign_all. I would advocate for removing content and removing roles as being separate functions. Peace - Anthony
          Hide
          Eloy Lafuente (stronk7) added a comment -

          note for me: look how 1.9 was doing this.

          Show
          Eloy Lafuente (stronk7) added a comment - note for me: look how 1.9 was doing this.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Update: we have created MDL-29450 about to make course_remove_contents() to be able to work the same way it used to do before 2.0 (aka, not deleting role assignments and enrolment methods) via new param in the function.

          Once that gets fixed, we can decide here if we want that behavior in restore always or it's something that should be optional.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Update: we have created MDL-29450 about to make course_remove_contents() to be able to work the same way it used to do before 2.0 (aka, not deleting role assignments and enrolment methods) via new param in the function. Once that gets fixed, we can decide here if we want that behavior in restore always or it's something that should be optional. Ciao
          Hide
          Rajesh Taneja added a comment -

          Eloy,

          Without changing the delete_course function, can we cache the list of all enrolled users in the existing course before deleting it and then have them restored (with an optional restore settings).

          Show
          Rajesh Taneja added a comment - Eloy, Without changing the delete_course function, can we cache the list of all enrolled users in the existing course before deleting it and then have them restored (with an optional restore settings).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, I think it's not so easy like one simple list of users, it's all their role/caps and enrolments (which plugin gave them access, which type of access, until when, which overrides does the user have in the course...), aka, a lot of information.

          I really think that fixing / extending MDL-29450 is the way, both to fix this issue and others in the future. For example, recently we were discussing about the posibility to leave the course level question banks also undeleted and it will be require the same sort of solution than the one needed here (be able to skip different parts in the execution of the function).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, I think it's not so easy like one simple list of users, it's all their role/caps and enrolments (which plugin gave them access, which type of access, until when, which overrides does the user have in the course...), aka, a lot of information. I really think that fixing / extending MDL-29450 is the way, both to fix this issue and others in the future. For example, recently we were discussing about the posibility to leave the course level question banks also undeleted and it will be require the same sort of solution than the one needed here (be able to skip different parts in the execution of the function). Ciao
          Hide
          Michael de Raadt added a comment -

          Reverting the affected versions as they were changed in error.

          Show
          Michael de Raadt added a comment - Reverting the affected versions as they were changed in error.
          Hide
          Michael de Raadt added a comment -

          This issue may be delayed as we are waiting on MDL-29450 to be completed first. Hopefully both can be resolved soon.

          Show
          Michael de Raadt added a comment - This issue may be delayed as we are waiting on MDL-29450 to be completed first. Hopefully both can be resolved soon.
          Hide
          Petr Škoda added a comment -

          I believe that keeping any enrol/roles info from the old course is wrong approach. After discussion with Eloy there are following use cases:

          1/ teacher has a course with students and data - they want to purge everything from the course keeping the same course id and restore only content from backup (no new users) - they expect that course level roles and groups will not be changed

          2/ teacher has a course with students and data - they want to purge everything from course and restore a new course with data and users from the restore - they expect they will be able to use the course even when they are not included in the restore file

          I am not going to think about why would anybody want to do 1, Eloy thinks it is great, I say it is very wrong workflow...

          Solutions:

          1/ We would have to skip roles/enrolment changes when removing course content - this would be usable only when restoring WITHOUT the enrolments/roles; we need a new restore option "Keep user roles and enrolments". The huge problem here is that a lot of stuff is linked via course context id - that means it is extremely hard to remove everything but the roles/enrolments. Workaround is to create new course, restore the course there and then enrol the same users into the new course and hide the old one - actually this is my preferred solution.

          2/ We would purge everything and let the enrol plugins deal with roles and enrolments. After the enrol restore step we would verify the current user may access the course and if not enrol them (this is similar to Creators' role in new course) using manual enrol instance. Here again we probably need a new setting for the role assigned automatically during restore.

          Show
          Petr Škoda added a comment - I believe that keeping any enrol/roles info from the old course is wrong approach. After discussion with Eloy there are following use cases: 1/ teacher has a course with students and data - they want to purge everything from the course keeping the same course id and restore only content from backup (no new users) - they expect that course level roles and groups will not be changed 2/ teacher has a course with students and data - they want to purge everything from course and restore a new course with data and users from the restore - they expect they will be able to use the course even when they are not included in the restore file I am not going to think about why would anybody want to do 1, Eloy thinks it is great, I say it is very wrong workflow... Solutions: 1/ We would have to skip roles/enrolment changes when removing course content - this would be usable only when restoring WITHOUT the enrolments/roles; we need a new restore option "Keep user roles and enrolments". The huge problem here is that a lot of stuff is linked via course context id - that means it is extremely hard to remove everything but the roles/enrolments. Workaround is to create new course, restore the course there and then enrol the same users into the new course and hide the old one - actually this is my preferred solution. 2/ We would purge everything and let the enrol plugins deal with roles and enrolments. After the enrol restore step we would verify the current user may access the course and if not enrol them (this is similar to Creators' role in new course) using manual enrol instance. Here again we probably need a new setting for the role assigned automatically during restore.
          Hide
          Petr Škoda added a comment -

          In any case I think that neither 1/ nor 2/ can be fixed in any stable branch because it will require API changes and new settings.

          Show
          Petr Škoda added a comment - In any case I think that neither 1/ nor 2/ can be fixed in any stable branch because it will require API changes and new settings.
          Hide
          Robert Russo added a comment -

          See MDL-29988 for a sane approach to deal with both scenarios.

          Show
          Robert Russo added a comment - See MDL-29988 for a sane approach to deal with both scenarios.
          Hide
          Petr Škoda added a comment -

          Hmm, after discussion with Eloy I think I found a way to enable different restore scenarios, working on a patch...

          Show
          Petr Škoda added a comment - Hmm, after discussion with Eloy I think I found a way to enable different restore scenarios, working on a patch...
          Hide
          moodle.com added a comment -

          Taking this out of the STABLE sprint as it is now assigned to Petr. (Thanks, Petr.)

          Show
          moodle.com added a comment - Taking this out of the STABLE sprint as it is now assigned to Petr. (Thanks, Petr.)
          Hide
          Petr Škoda added a comment -

          To reviewers: the pull does not contain main version bump, visit admin/index.php to get the new setting initialised, ciao.

          Show
          Petr Škoda added a comment - To reviewers: the pull does not contain main version bump, visit admin/index.php to get the new setting initialised, ciao.
          Hide
          Petr Škoda added a comment -

          global $USER fixed, thanks Eloy

          Show
          Petr Škoda added a comment - global $USER fixed, thanks Eloy
          Hide
          Mateusz Wójcik added a comment -

          Thanks for solving the problem!
          Please integrate this solution/patch with MOODLE_20_STABLE version.

          Show
          Mateusz Wójcik added a comment - Thanks for solving the problem! Please integrate this solution/patch with MOODLE_20_STABLE version.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao PS: Note this is the last message of this type that you will receive along the whole November month, because we are running continuous integration this weeks while QA tests for release of Moodle 2.2 (Dec 1st) are being performed.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Mateusz,

          no way this can go to stable releases (2.0.x and 2.1.x), sorry. It relies in some changes (MDL-29450, MDL-29602...) not easily backportable and only available in 2.2.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Mateusz, no way this can go to stable releases (2.0.x and 2.1.x), sorry. It relies in some changes ( MDL-29450 , MDL-29602 ...) not easily backportable and only available in 2.2.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note to integrator: This has to be inegrated after MDL-29450.

          Q for Petr, while the new settings and delete_course_contents() are hardly backportable... perhaps the "restorer" commit can be considered for backporting? I think it could solve a bunch of self-lock-out problems also in 2.0 and 2.1

          https://github.com/skodak/moodle/commit/08d5057bc23d4e068c915ba3f7a53d5bfb88f2ea

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note to integrator: This has to be inegrated after MDL-29450 . Q for Petr, while the new settings and delete_course_contents() are hardly backportable... perhaps the "restorer" commit can be considered for backporting? I think it could solve a bunch of self-lock-out problems also in 2.0 and 2.1 https://github.com/skodak/moodle/commit/08d5057bc23d4e068c915ba3f7a53d5bfb88f2ea Ciao
          Hide
          Petr Škoda added a comment -

          we have short release cycles now, that means less backporting to me....

          Show
          Petr Škoda added a comment - we have short release cycles now, that means less backporting to me....
          Hide
          Petr Škoda added a comment -

          rebased

          Show
          Petr Škoda added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL, this was driving me crazy!

          Why everything (the settings, their labels, the $options in remove_course_contents() are keep_xxxxx and you are using some purge_xxxxx variables in code.

          I don't know if your applied defaults (false) are ok, or the opposite are the desired ones (keep = true, at least for enrolments)

          Offtopic:

          Surely I'm going to integrate this only to master (2.2) and create one followup about to backport the "restorer" thing to avoid teachers to became locked-out from the course. If the followup is tested and people likes it, then will be nackported.

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, this was driving me crazy! Why everything (the settings, their labels, the $options in remove_course_contents() are keep_xxxxx and you are using some purge_xxxxx variables in code. I don't know if your applied defaults (false) are ok, or the opposite are the desired ones (keep = true, at least for enrolments) Offtopic: Surely I'm going to integrate this only to master (2.2) and create one followup about to backport the "restorer" thing to avoid teachers to became locked-out from the course. If the followup is tested and people likes it, then will be nackported.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for amending that Petr, integrating...

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for amending that Petr, integrating...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho Petr, I've spent the last hours testing this with different roles and settings and all seems to work ok BUT one combination.

          • If the backup contains question banks @ system or category level, then there are various steps that are executed by restore_root_task for any type of restore operation. And that step is the only one in all the restore code checking for permissions to decide where those question banks will be created.
          • And, in that exact moment, if the course contents have been deleted, the user may be lacking permissions (if was originally teacher), leading to one situation where the question bank cannot be created, throwing error and aborting the restore.

          So the only possible solution to this is to move the restore_fix_restorer_access_step, currently in restore_course_task to restore_root_task, before any of those checks happen.

          That way we ensure that the user is back with all his permissions before using them later in the same task.

          So I'm adding one extra small commit moving that step on top of the root task. Cannot imagine any problem with that...

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho Petr, I've spent the last hours testing this with different roles and settings and all seems to work ok BUT one combination. If the backup contains question banks @ system or category level, then there are various steps that are executed by restore_root_task for any type of restore operation. And that step is the only one in all the restore code checking for permissions to decide where those question banks will be created. And, in that exact moment, if the course contents have been deleted, the user may be lacking permissions (if was originally teacher), leading to one situation where the question bank cannot be created, throwing error and aborting the restore. So the only possible solution to this is to move the restore_fix_restorer_access_step, currently in restore_course_task to restore_root_task, before any of those checks happen. That way we ensure that the user is back with all his permissions before using them later in the same task. So I'm adding one extra small commit moving that step on top of the root task. Cannot imagine any problem with that... Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          An this has been integrated... going to test a few more combinations...

          Show
          Eloy Lafuente (stronk7) added a comment - An this has been integrated... going to test a few more combinations...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing this, I've tried as admin and as editing teacher, both adding and deleting, and keeping and not keeping enrollments and groups and all those combinations seem to be working ok.

          Show
          Eloy Lafuente (stronk7) added a comment - Passing this, I've tried as admin and as editing teacher, both adding and deleting, and keeping and not keeping enrollments and groups and all those combinations seem to be working ok.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has landed upstream, just on time for the upcoming new releases week. Thanks for it!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has landed upstream, just on time for the upcoming new releases week. Thanks for it! Ciao
          Hide
          Chris Follin added a comment -

          Regarding this comment from Eloy: "Surely I'm going to integrate this only to master (2.2) and create one followup about to backport the "restorer" thing to avoid teachers to became locked-out from the course. If the followup is tested and people likes it, then will be nackported."

          Is there a ticket number for that followup since this issue is now closed?

          Show
          Chris Follin added a comment - Regarding this comment from Eloy: "Surely I'm going to integrate this only to master (2.2) and create one followup about to backport the "restorer" thing to avoid teachers to became locked-out from the course. If the followup is tested and people likes it, then will be nackported." Is there a ticket number for that followup since this issue is now closed?
          Hide
          Justin Litalien added a comment -

          We recently upgraded to 2.2 yesterday (20120105) and this issue still exists. Creating a backup, importing it into a new course, and restoring (with the delete option on) still removes all enrollments.

          How successful have others been on this issue since 2.2 was released? Am I the only one still experiencing this?

          Show
          Justin Litalien added a comment - We recently upgraded to 2.2 yesterday (20120105) and this issue still exists. Creating a backup, importing it into a new course, and restoring (with the delete option on) still removes all enrollments. How successful have others been on this issue since 2.2 was released? Am I the only one still experiencing this?
          Hide
          Susana Leitão added a comment -

          Issue still on Moodle 2.2.1+ (Build: 20120217)

          Show
          Susana Leitão added a comment - Issue still on Moodle 2.2.1+ (Build: 20120217)
          Hide
          mikehas added a comment -

          We just upgraded to 2.3.6, and as others have noted we still see "This course is currently unavailable to students" after selecting delete and restore option within a course. Is there a nice workaround for this that anyone has found. I'm thinking about simply disabling this option for our instructors so they don't dig themselves into a hole. We may implement a new permission for preventing this behavior e.g., 'moodle/restore:deleteandrestore' that would be excellent. What do others think about this workaround?

          Show
          mikehas added a comment - We just upgraded to 2.3.6, and as others have noted we still see "This course is currently unavailable to students" after selecting delete and restore option within a course. Is there a nice workaround for this that anyone has found. I'm thinking about simply disabling this option for our instructors so they don't dig themselves into a hole. We may implement a new permission for preventing this behavior e.g., 'moodle/restore:deleteandrestore' that would be excellent. What do others think about this workaround?

            People

            • Votes:
              28 Vote for this issue
              Watchers:
              17 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: