Moodle
  1. Moodle
  2. MDL-27790

Temporary course remains after restore

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.2
    • Fix Version/s: 2.1.3
    • Component/s: Backup
    • Labels:
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Select an existing course or create new course.
      3. Backup an existing/new course (settings -> backup)
      4. Restore this course as new course
        • Start restore process (settings -> restore -> click on restore link)
        • Confirm details (Press continue)
        • Restore as a new course (In top section "Restore as a new course" select a category and click continue), it should show notification.
        • Click continue.
        • On settings screen press Cancel and on confirmation screen press cancel
        • Make sure no new hidden course is created.
        • Repeat first 3 steps and now complete restore process by clicking next and you should see a new course.
      Show
      Log in as admin Select an existing course or create new course. Backup an existing/new course (settings -> backup) Restore this course as new course Start restore process (settings -> restore -> click on restore link) Confirm details (Press continue) Restore as a new course (In top section "Restore as a new course" select a category and click continue), it should show notification. Click continue. On settings screen press Cancel and on confirmation screen press cancel Make sure no new hidden course is created. Repeat first 3 steps and now complete restore process by clicking next and you should see a new course.
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-mdl-27790-modified
    • Rank:
      17451

      Description

      I have just restored a 2.0 course in a 2.1 test site and, as well as creating the course from the backup, there is a another course called "Course restoration in progress", which is empty.

        Issue Links

          Activity

          Hide
          Ray Lawrence added a comment -

          Yup, this shows to non-logged in visitors in the courses list. Fugly.

          Show
          Ray Lawrence added a comment - Yup, this shows to non-logged in visitors in the courses list. Fugly.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          I've put it on our backlog and promoted it.

          In the meantime adding more information, such as replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and promoted it. In the meantime adding more information, such as replication instructions, fix test instructions, a workaround or even a code solution, will help us and other users.
          Hide
          moodle.com added a comment -

          Seems to be an artifact left behind when a restore doesn't complete 100%.

          Some thoughts:

          1) Can be made hidden by default
          2) Can be deleted immediately on "cancel"
          3) Look for places to delete the temp course on failure too.

          Show
          moodle.com added a comment - Seems to be an artifact left behind when a restore doesn't complete 100%. Some thoughts: 1) Can be made hidden by default 2) Can be deleted immediately on "cancel" 3) Look for places to delete the temp course on failure too.
          Hide
          moodle.com added a comment -

          Hi, Eloy.

          Can you please comment on this and reassign if necessary.

          Show
          moodle.com added a comment - Hi, Eloy. Can you please comment on this and reassign if necessary.
          Hide
          Aparup Banerjee added a comment - - edited

          i'm leaning towards (1) making temporary courses hidden by default after the restore.

          There must've been a reason why Eloy left the courses around knowing full well that anyone could re-run the restoration from the backup files. (maybe to report issues?)

          (hopefully people trying to arbitrarily access this course during a restore has got nothing to do with this.)

          Show
          Aparup Banerjee added a comment - - edited i'm leaning towards (1) making temporary courses hidden by default after the restore. There must've been a reason why Eloy left the courses around knowing full well that anyone could re-run the restoration from the backup files. (maybe to report issues?) (hopefully people trying to arbitrarily access this course during a restore has got nothing to do with this.)
          Hide
          Aparup Banerjee added a comment -

          MDL-28453 created to hide the temporary course (as a quick temporary fix).

          The big task of possibly not needing a temporary course id (or maybe deleting via cancel) will be dealt with here.

          Show
          Aparup Banerjee added a comment - MDL-28453 created to hide the temporary course (as a quick temporary fix). The big task of possibly not needing a temporary course id (or maybe deleting via cancel) will be dealt with here.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Just a bit of info on this issue.
          The `Course restoration in progress` course is created during a restore as soon as the restore controller is needed which is round about step 2.
          If for any reason the restore is not completed then this course remains. A problem during restore such as permissions errors, or simply the user failing to complete the restore (closing the browser) lead to this orphaned course.

          As mentioned by Apu, the course is now created as a hidden course which aids the problem however it is more of a band aid than a complete solution.

          From memory when the restore was being worked on we discussed how we might deal with unwanted courses leading from incomplete restores and the following solution is what was settled on.
          During the cron routine where we clean up old backup + restore controllers from the backup_controllers table we could look for incomplete controllers and then check the associated courseid and remove if the course has not been touched since the restore. These checks are really just going to be checking that the course hasn't been repurposed (by a user or another restore attempt). This is a less drastic change requiring just an improvement of our cron routine, however it has the disadvantage that courses will still exist until the next cron run cleans up backup controllers.

          Eloy does this ring bells with you?

          If you are still happy with the solution being in this form let me know and I'll start work on it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Just a bit of info on this issue. The `Course restoration in progress` course is created during a restore as soon as the restore controller is needed which is round about step 2. If for any reason the restore is not completed then this course remains. A problem during restore such as permissions errors, or simply the user failing to complete the restore (closing the browser) lead to this orphaned course. As mentioned by Apu, the course is now created as a hidden course which aids the problem however it is more of a band aid than a complete solution. From memory when the restore was being worked on we discussed how we might deal with unwanted courses leading from incomplete restores and the following solution is what was settled on. During the cron routine where we clean up old backup + restore controllers from the backup_controllers table we could look for incomplete controllers and then check the associated courseid and remove if the course has not been touched since the restore. These checks are really just going to be checking that the course hasn't been repurposed (by a user or another restore attempt). This is a less drastic change requiring just an improvement of our cron routine, however it has the disadvantage that courses will still exist until the next cron run cleans up backup controllers. Eloy does this ring bells with you? If you are still happy with the solution being in this form let me know and I'll start work on it. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I think that Sam's comment for remainining "in progress" courses (aka, unfinished controllers) can help for sure. But with some clarifications:

          1) When restoring to new course ($restore->execute())... surely we could try/catch the whole restore execution and, on exception, safely delete that course. We know it is a new one, just created.

          2) Obviously 1) is not applicable to restore operations done against existing courses, those should not be deleted.

          3) After 1) and 2) we can utilize Sam's approach above, just to delete remaining ones if the problem didn't happen in $restore->execute() but in another point. But once again, only when restoring to new course.

          4) In any case, we should try to get as much info as possible when the error happens because the goal is to minimize such failures to a minimum and surely there are more and more prechecks that we can execute and / or UI bugfixes in order to pre-detect wrong situations that will lead to error (and phantom courses). Surely this point is worth another issue.

          Just my 2cents. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I think that Sam's comment for remainining "in progress" courses (aka, unfinished controllers) can help for sure. But with some clarifications: 1) When restoring to new course ($restore->execute())... surely we could try/catch the whole restore execution and, on exception, safely delete that course. We know it is a new one, just created. 2) Obviously 1) is not applicable to restore operations done against existing courses, those should not be deleted. 3) After 1) and 2) we can utilize Sam's approach above, just to delete remaining ones if the problem didn't happen in $restore->execute() but in another point. But once again, only when restoring to new course. 4) In any case, we should try to get as much info as possible when the error happens because the goal is to minimize such failures to a minimum and surely there are more and more prechecks that we can execute and / or UI bugfixes in order to pre-detect wrong situations that will lead to error (and phantom courses). Surely this point is worth another issue. Just my 2cents. Ciao
          Hide
          Ray Lawrence added a comment -

          Hi,

          Are you also removing the visible notice about the restore from the front in this fix?

          Show
          Ray Lawrence added a comment - Hi, Are you also removing the visible notice about the restore from the front in this fix?
          Hide
          Rajesh Taneja added a comment -

          Can you please review this

          Show
          Rajesh Taneja added a comment - Can you please review this
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, it's looking great some comments:

          A) Trivial: I would not use "AND bc.status < " (minor) but different instead, we don't know if tomorrow we'll be creating new statuses with other values.

          B) Trivial: The sortorder and timecreated conditions seem a bit unnecessary (do not add anything IMO).

          C) The cron deletion: I think we should keep that 100% out. There is no way to know what has happened between the execution of the restore and the execution of the cron. Perhaps the teacher has edited the course and performed other restore operations on it, or has added activities, or has been reused by the admin to create another course... Basically we only can guarantee that the deletion is safe when it happens under the life of the controller, aka, on restore exception and on user cancel. Out of there, any deletion can lead to serious problems. So my -1 for the cron part.

          And that's all. About the JS bits I cannot say much, sure they work as expected. Thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, it's looking great some comments: A) Trivial: I would not use "AND bc.status < " (minor) but different instead, we don't know if tomorrow we'll be creating new statuses with other values. B) Trivial: The sortorder and timecreated conditions seem a bit unnecessary (do not add anything IMO). C) The cron deletion: I think we should keep that 100% out. There is no way to know what has happened between the execution of the restore and the execution of the cron. Perhaps the teacher has edited the course and performed other restore operations on it, or has added activities, or has been reused by the admin to create another course... Basically we only can guarantee that the deletion is safe when it happens under the life of the controller, aka, on restore exception and on user cancel. Out of there, any deletion can lead to serious problems. So my -1 for the cron part. And that's all. About the JS bits I cannot say much, sure they work as expected. Thanks! Ciao
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Eloy.
          Can you please suggest what can be an alternative for point A. Well, I can remove this check as well, because we are calling cleanup only form controller and making sure that it's creating new course. So probably bc.status check can be removed as well.

          Also, I will be looking at this again to see if course creation can be moved in execution. I tried that before, but it seems very complex. Will give it a try again

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Eloy. Can you please suggest what can be an alternative for point A. Well, I can remove this check as well, because we are calling cleanup only form controller and making sure that it's creating new course. So probably bc.status check can be removed as well. Also, I will be looking at this again to see if course creation can be moved in execution. I tried that before, but it seems very complex. Will give it a try again
          Hide
          Andrew Davis added a comment -

          I'm not terribly pleased with the prospect of the temporary course being left behind if the user just shuts the browser, if their computer crashes, if they reboot, it they lose their Internet connection, if their session times out etc. I realize its complicated however all we're doing is shifting responsibility from us to the user who really shouldn't have to worry about cleaning up stuff that was created automatically.

          "There is no way to know what has happened between the execution of the restore and the execution of the cron. Perhaps the teacher has edited the course and performed other restore operations on it, or has added activities, or has been reused by the admin to create another course..."

          This problem is even worse for an actual human being in a large institution. If you're an admin or one of many teachers and you come across a bunch of temp courses you also have no way to tell which you can safely delete.

          What about something like adding a flag somewhere, maybe a column in the course table, that indicates that it is a temporary course. When the flag is set we display a big warning on the course page sayings "this is a temporary course" and make the course read only from the UI. Then cron can safely remove any courses with that flag set after a given period of time has passed.

          Show
          Andrew Davis added a comment - I'm not terribly pleased with the prospect of the temporary course being left behind if the user just shuts the browser, if their computer crashes, if they reboot, it they lose their Internet connection, if their session times out etc. I realize its complicated however all we're doing is shifting responsibility from us to the user who really shouldn't have to worry about cleaning up stuff that was created automatically. "There is no way to know what has happened between the execution of the restore and the execution of the cron. Perhaps the teacher has edited the course and performed other restore operations on it, or has added activities, or has been reused by the admin to create another course..." This problem is even worse for an actual human being in a large institution. If you're an admin or one of many teachers and you come across a bunch of temp courses you also have no way to tell which you can safely delete. What about something like adding a flag somewhere, maybe a column in the course table, that indicates that it is a temporary course. When the flag is set we display a big warning on the course page sayings "this is a temporary course" and make the course read only from the UI. Then cron can safely remove any courses with that flag set after a given period of time has passed.
          Hide
          Andrew Davis added a comment -

          If we must stick with this current scheme the English in this string isn't quite right.

          $string['confirmnewcoursecontinuequestion'] = 'A temporary (hidden) course will be created for restoration process, please do not close browser. If you donot want to continue later use cancel button';
          

          Should probably be something like this.

          $string['confirmnewcoursecontinuequestion'] = 'A temporary (hidden) course will be created by the course restoration process. To abort restoration click cancel. Do not close the browser while restoring.';
          
          Show
          Andrew Davis added a comment - If we must stick with this current scheme the English in this string isn't quite right. $string['confirmnewcoursecontinuequestion'] = 'A temporary (hidden) course will be created for restoration process, please do not close browser. If you donot want to continue later use cancel button'; Should probably be something like this. $string['confirmnewcoursecontinuequestion'] = 'A temporary (hidden) course will be created by the course restoration process. To abort restoration click cancel. Do not close the browser while restoring.';
          Hide
          Rajesh Taneja added a comment -

          Thanks for the feedback Andrew
          String has been modified as per your suggestion. Putting it back for peer-review, so that Eloy can provide some more inputs.

          Show
          Rajesh Taneja added a comment - Thanks for the feedback Andrew String has been modified as per your suggestion. Putting it back for peer-review, so that Eloy can provide some more inputs.
          Hide
          Rajesh Taneja added a comment -

          After talking to Eloy, we decided:

          1. apply current patch fixing the ones under "restore controller". (cancels + exceptions)
          2. prevent user execution stop from the browser. (subtask)
          3. analyze the exact moment when we are creating the course and its life (subtask).

          Having 2 and 3 as subtask will be more safer to make sure correct course gets deleted.

          Show
          Rajesh Taneja added a comment - After talking to Eloy, we decided: apply current patch fixing the ones under "restore controller". (cancels + exceptions) prevent user execution stop from the browser. (subtask) analyze the exact moment when we are creating the course and its life (subtask). Having 2 and 3 as subtask will be more safer to make sure correct course gets deleted.
          Hide
          Michael de Raadt added a comment -

          Carrying this over to the current sprint as it is still in progress.

          Show
          Michael de Raadt added a comment - Carrying this over to the current sprint as it is still in progress.
          Hide
          Aparup Banerjee added a comment -

          Hi Rajesh, i was just having a look through this :-

          1) i don't know how likely but we need to note that public function cancel_restore() --> public function cancel_process() is a killer for STABLE branches. There could be class abc extends restore_ui out there that we would end up devastating comment?

          2) please create the sub-tasks mentioned

          3) we need a good string of test instructions for this.

          4) i assume we're not doing any clean up for the temporary courses that people may already have ( admin's responsibility ? )

          Show
          Aparup Banerjee added a comment - Hi Rajesh, i was just having a look through this :- 1) i don't know how likely but we need to note that public function cancel_restore() --> public function cancel_process() is a killer for STABLE branches. There could be class abc extends restore_ui out there that we would end up devastating comment? 2) please create the sub-tasks mentioned 3) we need a good string of test instructions for this. 4) i assume we're not doing any clean up for the temporary courses that people may already have ( admin's responsibility ? )
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that Aparup,
          I have added cancel_restore function to avoid any regression. Added required subtasks and added test instructions.

          Yes, we are not doing any cleanup as we can't be sure if the course is a temporary course and has been modified since it was created and cron was run. This should be handled by MDL-30177

          Show
          Rajesh Taneja added a comment - Thanks for pointing that Aparup, I have added cancel_restore function to avoid any regression. Added required subtasks and added test instructions. Yes, we are not doing any cleanup as we can't be sure if the course is a temporary course and has been modified since it was created and cron was run. This should be handled by MDL-30177
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          With regards to the STABLE patch, i don't think we even need to wrap cancel_restore within cancel_process, it seems that the wrapping is totally redundant for 21_STABLE. It is perfectly fine and bc staying as cancel_restore() alone. I'm not even sure for the pressing need to rename the function in master but lets not add wrappers to functions that don't do anything real for STABLE. That said, I'm happy to integrate this once we do away with the redundant wrapping in STABLE.

          Show
          Aparup Banerjee added a comment - Hi Raj, With regards to the STABLE patch, i don't think we even need to wrap cancel_restore within cancel_process, it seems that the wrapping is totally redundant for 21_STABLE. It is perfectly fine and bc staying as cancel_restore() alone. I'm not even sure for the pressing need to rename the function in master but lets not add wrappers to functions that don't do anything real for STABLE. That said, I'm happy to integrate this once we do away with the redundant wrapping in STABLE.
          Hide
          Rajesh Taneja added a comment -

          Thanks for comment Aparup,

          During cancellation of restore process, every stage call cancel_process except settings stage (where cancel_restore is called).
          So if user cancels restore on settings stage cancel_restore will clean the temporary course and if it's called from review stage cancel_process has to clean temporary course.
          There are two ways to solve it:

          1. Have both the functions and do the same job of cleaning and redirecting.
          2. Use clean_restore for all the stages.

          Please suggest what is expected in stable branch.

          FYI: Base class base_ui has cancel_process, so according to my understanding, course should be cleaned in cancel_process.

          Show
          Rajesh Taneja added a comment - Thanks for comment Aparup, During cancellation of restore process, every stage call cancel_process except settings stage (where cancel_restore is called). So if user cancels restore on settings stage cancel_restore will clean the temporary course and if it's called from review stage cancel_process has to clean temporary course. There are two ways to solve it: Have both the functions and do the same job of cleaning and redirecting. Use clean_restore for all the stages. Please suggest what is expected in stable branch. FYI: Base class base_ui has cancel_process, so according to my understanding, course should be cleaned in cancel_process.
          Hide
          Aparup Banerjee added a comment -

          ok spoketo Raj, we're going with the wrapper, this is required so that we purposefully override base_ui's cancel_process(). we'll keep cancel_restore() as a wrapper to cancel_process()i the restore class for bc in stable.

          Show
          Aparup Banerjee added a comment - ok spoketo Raj, we're going with the wrapper, this is required so that we purposefully override base_ui's cancel_process(). we'll keep cancel_restore() as a wrapper to cancel_process()i the restore class for bc in stable.
          Hide
          Aparup Banerjee added a comment -

          Awesome, thats integrated and up for testing now.

          Show
          Aparup Banerjee added a comment - Awesome, thats integrated and up for testing now.
          Hide
          Rajesh Taneja added a comment -

          Thanks Aparup

          Show
          Rajesh Taneja added a comment - Thanks Aparup
          Hide
          Ankit Agarwal added a comment -

          Works perfectly!
          Thanks!

          Show
          Ankit Agarwal added a comment - Works perfectly! Thanks!
          Hide
          David Mudrak added a comment -

          Sorry guys but I disagree with the solution at all. From the user's point of view, the new message is just confusing. If I am the user who just chose to restore to a new course, the message "A temporary (hidden) course will be created by the course restoration process." is nothing but confusing. Let me also query triaging this as a blocker. How does it block Moodle features?

          Show
          David Mudrak added a comment - Sorry guys but I disagree with the solution at all. From the user's point of view, the new message is just confusing. If I am the user who just chose to restore to a new course, the message "A temporary (hidden) course will be created by the course restoration process." is nothing but confusing. Let me also query triaging this as a blocker. How does it block Moodle features?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
          Hide
          Aparup Banerjee added a comment -

          Hi David,

          • 'disagree with solution at all' means entire solution disagreement or just the string (perhaps we don't really need that popup or could rephrase it? i did find it slightly 'Too much info'-ish )
          • i agree with your comment about triaging this incorrectly.
          Show
          Aparup Banerjee added a comment - Hi David, 'disagree with solution at all' means entire solution disagreement or just the string (perhaps we don't really need that popup or could rephrase it? i did find it slightly 'Too much info'-ish ) i agree with your comment about triaging this incorrectly.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Sorry but I have to agree with David, for sure that dialogue warning is more confusing that what is actually going on.
          I really think we should consider removing that dialogue.

          In a related matter (and how I ended up here) I have opened a new bug MDL-30391 which is a regression caused by these changes.
          The exceptions that get thrown now by restore get horribly mangled.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Sorry but I have to agree with David, for sure that dialogue warning is more confusing that what is actually going on. I really think we should consider removing that dialogue. In a related matter (and how I ended up here) I have opened a new bug MDL-30391 which is a regression caused by these changes. The exceptions that get thrown now by restore get horribly mangled. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          I've created MDL-30394 to reduce the bright and glaring dialogue.

          Show
          Aparup Banerjee added a comment - I've created MDL-30394 to reduce the bright and glaring dialogue.
          Hide
          Rajesh Taneja added a comment -

          Thanks Everyone for pointing that.
          I think, if we are not willing to have a dialog, then it should go in user docs to make sure user knows the consequence of existing the process.

          Show
          Rajesh Taneja added a comment - Thanks Everyone for pointing that. I think, if we are not willing to have a dialog, then it should go in user docs to make sure user knows the consequence of existing the process.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I want to participate here too, lol:

          I've created MDL-42321, MDL-42322, MDL-42323 and MDL-42324 about to reduce a bit more the patch.

          (Joking! Sorry Rajesh could not resist, thanks for your patience, hahaha!)

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I want to participate here too, lol: I've created MDL-42321 , MDL-42322 , MDL-42323 and MDL-42324 about to reduce a bit more the patch. (Joking! Sorry Rajesh could not resist, thanks for your patience, hahaha!) Ciao
          Hide
          Rajesh Taneja added a comment -

          There is hardly any code left in the patch to remove, Eloy
          It do make sense to modify docs, then to add dialog and irritate people.
          Thanks to David, for raising this

          Show
          Rajesh Taneja added a comment - There is hardly any code left in the patch to remove, Eloy It do make sense to modify docs, then to add dialog and irritate people. Thanks to David, for raising this
          Hide
          Hugh Davenport added a comment -

          Not sure if this needs to be a new bug or not, but in backup/util/dbops/restore_dbops.class.php line ~1347 function create_new_course, there is a comment saying // forcing skeleton courses to be hidden instead of going by $category->visible , until MDL-27790 is resolved.
          Seeming this has now been resolved, should we fix this up to use $category->visible instead?

          Show
          Hugh Davenport added a comment - Not sure if this needs to be a new bug or not, but in backup/util/dbops/restore_dbops.class.php line ~1347 function create_new_course, there is a comment saying // forcing skeleton courses to be hidden instead of going by $category->visible , until MDL-27790 is resolved. Seeming this has now been resolved, should we fix this up to use $category->visible instead?
          Hide
          Rajesh Taneja added a comment -

          Hello Hugh,

          Sorry this patch was not the full solution for temporary course creation issue. MDL-30178 and MDL-30177 should be resolved before making $category->visible. I will create an issue to update the comment.

          Show
          Rajesh Taneja added a comment - Hello Hugh, Sorry this patch was not the full solution for temporary course creation issue. MDL-30178 and MDL-30177 should be resolved before making $category->visible. I will create an issue to update the comment.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: