MDL-41669 Restore: Progress bar needs to include more tasks
authorsam marshall <s.marshall@open.ac.uk>
Fri, 16 Aug 2013 18:11:14 +0000 (19:11 +0100)
committersam marshall <s.marshall@open.ac.uk>
Mon, 16 Sep 2013 16:18:12 +0000 (17:18 +0100)
1. Changes progress bar code to allow headings for progress bar (so users have
   some clue what's going on if a page has more than one progress bar).
2. Changes restore code so that a progress bar can display during pre-checks if
   they take longer than 5 seconds.
3. Changes pre-check and restore code so that, in various points where the system
   can take a long time within an individual step, intederminate progress is
   indicated and it won't time out.

backup/import.php
backup/moodle2/restore_stepslib.php
backup/restore.php
backup/util/dbops/restore_dbops.class.php
backup/util/helper/restore_prechecks_helper.class.php
backup/util/plan/restore_structure_step.class.php
backup/util/progress/core_backup_display_progress_if_slow.class.php
backup/util/xml/parser/progressive_parser.class.php
lang/en/backup.php

index 13ad15a..586edd3 100644 (file)
@@ -120,6 +120,10 @@ if ($backup->get_stage() == backup_ui::STAGE_FINAL) {
     // Prepare the restore controller. We don't need a UI here as we will just use what
     // ever the restore has (the user has just chosen).
     $rc = new restore_controller($backupid, $course->id, backup::INTERACTIVE_YES, backup::MODE_IMPORT, $USER->id, $restoretarget);
+
+    // Start a progress section for the restore, which will consist of 2 steps
+    // (the precheck and then the actual restore).
+    $progress->start_progress('Restore process', 2);
     $rc->set_progress($progress);
     // Convert the backup if required.... it should NEVER happed
     if ($rc->get_status() == backup::STATUS_REQUIRE_CONV) {
@@ -155,6 +159,9 @@ if ($backup->get_stage() == backup_ui::STAGE_FINAL) {
     // Delete the temp directory now
     fulldelete($tempdestination);
 
+    // End restore section of progress tracking (restore/precheck).
+    $progress->end_progress();
+
     // All progress complete. Hide progress area.
     $progress->end_progress();
     echo html_writer::end_div();
index 444f9cf..1afa523 100644 (file)
@@ -594,13 +594,17 @@ class restore_load_included_inforef_records extends restore_execution_step {
 
         // Get all the included tasks
         $tasks = restore_dbops::get_included_tasks($this->get_restoreid());
+        $progress = $this->task->get_progress();
+        $progress->start_progress($this->get_name(), count($tasks));
         foreach ($tasks as $task) {
             // Load the inforef.xml file if exists
             $inforefpath = $task->get_taskbasepath() . '/inforef.xml';
             if (file_exists($inforefpath)) {
-                restore_dbops::load_inforef_to_tempids($this->get_restoreid(), $inforefpath); // Load each inforef file to temp_ids
+                // Load each inforef file to temp_ids.
+                restore_dbops::load_inforef_to_tempids($this->get_restoreid(), $inforefpath, $progress);
             }
         }
+        $progress->end_progress();
     }
 }
 
@@ -687,7 +691,8 @@ class restore_load_included_users extends restore_execution_step {
             return;
         }
         $file = $this->get_basepath() . '/users.xml';
-        restore_dbops::load_users_to_tempids($this->get_restoreid(), $file); // Load needed users to temp_ids
+        // Load needed users to temp_ids.
+        restore_dbops::load_users_to_tempids($this->get_restoreid(), $file, $this->task->get_progress());
     }
 }
 
@@ -708,7 +713,8 @@ class restore_process_included_users extends restore_execution_step {
         if (!$this->task->get_setting_value('users')) { // No userinfo being restored, nothing to do
             return;
         }
-        restore_dbops::process_included_users($this->get_restoreid(), $this->task->get_courseid(), $this->task->get_userid(), $this->task->is_samesite());
+        restore_dbops::process_included_users($this->get_restoreid(), $this->task->get_courseid(),
+                $this->task->get_userid(), $this->task->is_samesite(), $this->task->get_progress());
     }
 }
 
index 6040d4b..4e2c06d 100644 (file)
@@ -54,7 +54,7 @@ echo $OUTPUT->header();
 
 // Prepare a progress bar which can display optionally during long-running
 // operations while setting up the UI.
-$slowprogress = new core_backup_display_progress_if_slow();
+$slowprogress = new core_backup_display_progress_if_slow(get_string('preparingui', 'backup'));
 // Depending on the code branch above, $restore may be a restore_ui or it may
 // be a restore_ui_independent_stage. Either way, this function exists.
 $restore->set_progress_reporter($slowprogress);
@@ -65,13 +65,20 @@ if (!$restore->is_independent() && $restore->enforce_changed_dependencies()) {
 }
 
 if (!$restore->is_independent()) {
+    // Use a temporary (disappearing) progress bar to show the precheck progress if any.
+    $precheckprogress = new core_backup_display_progress_if_slow(get_string('preparingdata', 'backup'));
+    $restore->get_controller()->set_progress($precheckprogress);
     if ($restore->get_stage() == restore_ui::STAGE_PROCESS && !$restore->requires_substage()) {
         try {
-            // Display an extra progress bar so that we can show the progress first.
+            // Div used to hide the 'progress' step once the page gets onto 'finished'.
             echo html_writer::start_div('', array('id' => 'executionprogress'));
+            // Show the current restore state (header with bolded item).
             echo $renderer->progress_bar($restore->get_progress_bar());
-            $restore->get_controller()->set_progress(new core_backup_display_progress());
+            // Start displaying the actual progress bar percentage.
+            $restore->get_controller()->set_progress(new core_backup_display_progress(true));
+            // Do actual restore.
             $restore->execute();
+            // Hide this section because we are now going to make the page show 'finished'.
             echo html_writer::end_div();
             echo html_writer::script('document.getElementById("executionprogress").style.display = "none";');
         } catch(Exception $e) {
index 0e5fabc..b1561d8 100644 (file)
@@ -109,18 +109,34 @@ abstract class restore_dbops {
 
     /**
      * Load one inforef.xml file to backup_ids table for future reference
+     *
+     * @param string $restoreid Restore id
+     * @param string $inforeffile File path
+     * @param core_backup_progress $progress Progress tracker
      */
-    public static function load_inforef_to_tempids($restoreid, $inforeffile) {
+    public static function load_inforef_to_tempids($restoreid, $inforeffile,
+            core_backup_progress $progress = null) {
 
         if (!file_exists($inforeffile)) { // Shouldn't happen ever, but...
             throw new backup_helper_exception('missing_inforef_xml_file', $inforeffile);
         }
+
+        // Set up progress tracking (indeterminate).
+        if (!$progress) {
+            $progress = new core_backup_null_progress();
+        }
+        $progress->start_progress('Loading inforef.xml file');
+
         // Let's parse, custom processor will do its work, sending info to DB
         $xmlparser = new progressive_parser();
         $xmlparser->set_file($inforeffile);
         $xmlprocessor = new restore_inforef_parser_processor($restoreid);
         $xmlparser->set_processor($xmlprocessor);
+        $xmlparser->set_progress($progress);
         $xmlparser->process();
+
+        // Finish progress
+        $progress->end_progress();
     }
 
     /**
@@ -400,18 +416,34 @@ abstract class restore_dbops {
 
     /**
      * Load the needed users.xml file to backup_ids table for future reference
+     *
+     * @param string $restoreid Restore id
+     * @param string $usersfile File path
+     * @param core_backup_progress $progress Progress tracker
      */
-    public static function load_users_to_tempids($restoreid, $usersfile) {
+    public static function load_users_to_tempids($restoreid, $usersfile,
+            core_backup_progress $progress = null) {
 
         if (!file_exists($usersfile)) { // Shouldn't happen ever, but...
             throw new backup_helper_exception('missing_users_xml_file', $usersfile);
         }
+
+        // Set up progress tracking (indeterminate).
+        if (!$progress) {
+            $progress = new core_backup_null_progress();
+        }
+        $progress->start_progress('Loading users into temporary table');
+
         // Let's parse, custom processor will do its work, sending info to DB
         $xmlparser = new progressive_parser();
         $xmlparser->set_file($usersfile);
         $xmlprocessor = new restore_users_parser_processor($restoreid);
         $xmlparser->set_processor($xmlprocessor);
+        $xmlparser->set_progress($progress);
         $xmlparser->process();
+
+        // Finish progress.
+        $progress->end_progress();
     }
 
     /**
@@ -1385,8 +1417,15 @@ abstract class restore_dbops {
      * for each one (mapping / creation) and returning one array
      * of problems in case something is wrong (lack of permissions,
      * conficts)
+     *
+     * @param string $restoreid Restore id
+     * @param int $courseid Course id
+     * @param int $userid User id
+     * @param bool $samesite True if restore is to same site
+     * @param core_backup_progress $progress Progress reporter
      */
-    public static function precheck_included_users($restoreid, $courseid, $userid, $samesite) {
+    public static function precheck_included_users($restoreid, $courseid, $userid, $samesite,
+            core_backup_progress $progress) {
         global $CFG, $DB;
 
         // To return any problem found
@@ -1409,8 +1448,14 @@ abstract class restore_dbops {
             $cancreateuser = true;
         }
 
+        // Prepare for reporting progress.
+        $conditions = array('backupid' => $restoreid, 'itemname' => 'user');
+        $max = $DB->count_records('backup_ids_temp', $conditions);
+        $done = 0;
+        $progress->start_progress('Checking users', $max);
+
         // Iterate over all the included users
-        $rs = $DB->get_recordset('backup_ids_temp', array('backupid' => $restoreid, 'itemname' => 'user'), '', 'itemid, info');
+        $rs = $DB->get_recordset('backup_ids_temp', $conditions, '', 'itemid, info');
         foreach ($rs as $recuser) {
             $user = (object)backup_controller_dbops::decode_backup_temp_info($recuser->info);
 
@@ -1447,8 +1492,11 @@ abstract class restore_dbops {
             } else { // Shouldn't arrive here ever, something is for sure wrong. Exception
                 throw new restore_dbops_exception('restore_error_processing_user', $user->username);
             }
+            $done++;
+            $progress->progress($done);
         }
         $rs->close();
+        $progress->end_progress();
         return $problems;
     }
 
@@ -1458,12 +1506,19 @@ abstract class restore_dbops {
      *
      * Just wrap over precheck_included_users(), returning
      * exception if any problem is found
+     *
+     * @param string $restoreid Restore id
+     * @param int $courseid Course id
+     * @param int $userid User id
+     * @param bool $samesite True if restore is to same site
+     * @param core_backup_progress $progress Optional progress tracker
      */
-    public static function process_included_users($restoreid, $courseid, $userid, $samesite) {
+    public static function process_included_users($restoreid, $courseid, $userid, $samesite,
+            core_backup_progress $progress = null) {
         global $DB;
 
         // Just let precheck_included_users() to do all the hard work
-        $problems = self::precheck_included_users($restoreid, $courseid, $userid, $samesite);
+        $problems = self::precheck_included_users($restoreid, $courseid, $userid, $samesite, $progress);
 
         // With problems, throw exception, shouldn't happen if prechecks were originally
         // executed, so be radical here.
index 95bc968..a80a80b 100644 (file)
@@ -43,7 +43,7 @@ abstract class restore_prechecks_helper {
      *
      * Returns empty array or warnings/errors array
      */
-    public static function execute_prechecks($controller, $droptemptablesafter = false) {
+    public static function execute_prechecks(restore_controller $controller, $droptemptablesafter = false) {
         global $CFG;
 
         $errors = array();
@@ -57,16 +57,31 @@ abstract class restore_prechecks_helper {
         $courseid = $controller->get_courseid();
         $userid = $controller->get_userid();
         $rolemappings = $controller->get_info()->role_mappings;
+        $progress = $controller->get_progress();
+
+        // Start tracking progress. There are currently 8 major steps, corresponding
+        // to $majorstep++ lines in this code; we keep track of the total so as to
+        // verify that it's still correct. If you add a major step, you need to change
+        // the total here.
+        $majorstep = 1;
+        $majorsteps = 8;
+        $progress->start_progress('Carrying out pre-restore checks', $majorsteps);
+
         // Load all the included tasks to look for inforef.xml files
         $inforeffiles = array();
         $tasks = restore_dbops::get_included_tasks($restoreid);
+        $progress->start_progress('Listing inforef files', count($tasks));
+        $minorstep = 1;
         foreach ($tasks as $task) {
             // Add the inforef.xml file if exists
             $inforefpath = $task->get_taskbasepath() . '/inforef.xml';
             if (file_exists($inforefpath)) {
                 $inforeffiles[] = $inforefpath;
             }
+            $progress->progress($minorstep++);
         }
+        $progress->end_progress();
+        $progress->progress($majorstep++);
 
         // Create temp tables
         restore_controller_dbops::create_restore_temp_tables($controller->get_restoreid());
@@ -108,18 +123,31 @@ abstract class restore_prechecks_helper {
         }
 
         // Load all the inforef files, we are going to need them
+        $progress->start_progress('Loading temporary IDs', count($inforeffiles));
+        $minorstep = 1;
         foreach ($inforeffiles as $inforeffile) {
-            restore_dbops::load_inforef_to_tempids($restoreid, $inforeffile); // Load each inforef file to temp_ids
+            // Load each inforef file to temp_ids.
+            restore_dbops::load_inforef_to_tempids($restoreid, $inforeffile, $progress);
+            $progress->progress($minorstep++);
         }
+        $progress->end_progress();
+        $progress->progress($majorstep++);
 
         // If restoring users, check we are able to create all them
         if ($restoreusers) {
             $file = $controller->get_plan()->get_basepath() . '/users.xml';
-            restore_dbops::load_users_to_tempids($restoreid, $file); // Load needed users to temp_ids
-            if ($problems = restore_dbops::precheck_included_users($restoreid, $courseid, $userid, $samesite)) {
+             // Load needed users to temp_ids.
+            restore_dbops::load_users_to_tempids($restoreid, $file, $progress);
+            $progress->progress($majorstep++);
+            if ($problems = restore_dbops::precheck_included_users($restoreid, $courseid, $userid, $samesite, $progress)) {
                 $errors = array_merge($errors, $problems);
             }
+        } else {
+            // To ensure consistent number of steps in progress tracking,
+            // mark progress even though we didn't do anything.
+            $progress->progress($majorstep++);
         }
+        $progress->progress($majorstep++);
 
         // Note: restore won't create roles at all. Only mapping/skip!
         $file = $controller->get_plan()->get_basepath() . '/roles.xml';
@@ -128,6 +156,7 @@ abstract class restore_prechecks_helper {
             $errors = array_key_exists('errors', $problems) ? array_merge($errors, $problems['errors']) : $errors;
             $warnings = array_key_exists('warnings', $problems) ? array_merge($warnings, $problems['warnings']) : $warnings;
         }
+        $progress->progress($majorstep++);
 
         // Check we are able to restore and the categories and questions
         $file = $controller->get_plan()->get_basepath() . '/questions.xml';
@@ -136,8 +165,9 @@ abstract class restore_prechecks_helper {
             $errors = array_key_exists('errors', $problems) ? array_merge($errors, $problems['errors']) : $errors;
             $warnings = array_key_exists('warnings', $problems) ? array_merge($warnings, $problems['warnings']) : $warnings;
         }
+        $progress->progress($majorstep++);
 
-        // Prepare results and return
+        // Prepare results.
         $results = array();
         if (!empty($errors)) {
             $results['errors'] = $errors;
@@ -149,6 +179,14 @@ abstract class restore_prechecks_helper {
         if (!empty($results) || $droptemptablesafter) {
             restore_controller_dbops::drop_restore_temp_tables($controller->get_restoreid());
         }
+
+        // Finish progress and check we got the initial number of steps right.
+        $progress->progress($majorstep++);
+        if ($majorstep != $majorsteps) {
+            throw new coding_exception('Progress step count wrong: ' . $majorstep);
+        }
+        $progress->end_progress();
+
         return $results;
     }
 }
index 0fb1a40..822c71b 100644 (file)
@@ -101,8 +101,14 @@ abstract class restore_structure_step extends restore_step {
             $xmlprocessor->add_path($element->get_path(), $element->is_grouped());
         }
 
+        // Set up progress tracking.
+        $progress = $this->get_task()->get_progress();
+        $progress->start_progress($this->get_name(), core_backup_progress::INDETERMINATE);
+        $xmlparser->set_progress($progress);
+
         // And process it, dispatch to target methods in step will start automatically
         $xmlparser->process();
+        $progress->end_progress();
 
         // Have finished, launch the after_execute method of all the processing objects
         $this->launch_after_execute_methods();
index ee9511e..fdf8cfc 100644 (file)
@@ -43,6 +43,11 @@ class core_backup_display_progress_if_slow extends core_backup_display_progress
      */
     protected $id;
 
+    /**
+     * @var string Text to display in heading if bar appears
+     */
+    protected $heading;
+
     /**
      * @var int Time at which the progress bar should display (if it isn't yet)
      */
@@ -52,23 +57,38 @@ class core_backup_display_progress_if_slow extends core_backup_display_progress
      * Constructs the progress reporter. This will not output HTML just yet,
      * until the required delay time expires.
      *
+     * @param string $heading Text to display above bar (if it appears); '' for none
      * @param int $delay Delay time (default 5 seconds)
      */
-    public function __construct($delay = self::DEFAULT_DISPLAY_DELAY) {
+    public function __construct($heading, $delay = self::DEFAULT_DISPLAY_DELAY) {
         // Set start time based on delay.
         $this->starttime = time() + $delay;
+        $this->heading = $heading;
         parent::__construct(false);
     }
 
     /**
-     * Adds a div around the parent display so it can be hidden.
+     * Starts displaying the progress bar, with optional heading and a special
+     * div so it can be hidden later.
      *
      * @see core_backup_display_progress::start_html()
      */
     public function start_html() {
+        global $OUTPUT;
         $this->id = 'core_backup_display_progress_if_slow' . self::$nextid;
         self::$nextid++;
-        echo html_writer::start_div('', array('id' => $this->id));
+
+        // Containing div includes a CSS class so that it can be themed if required,
+        // and an id so it can be automatically hidden at end.
+        echo html_writer::start_div('core_backup_display_progress_if_slow',
+                array('id' => $this->id));
+
+        // Display optional heading.
+        if ($this->heading !== '') {
+            echo $OUTPUT->heading($this->heading, 3);
+        }
+
+        // Use base class to display progress bar.
         parent::start_html();
     }
 
index d465aba..882852a 100644 (file)
@@ -67,6 +67,11 @@ class progressive_parser {
     protected $prevlevel;  // level of the previous tag processed - to detect pushing places
     protected $currtag;    // name/value/attributes of the tag being processed
 
+    /**
+     * @var core_backup_progress Progress tracker called for each action
+     */
+    protected $progress;
+
     public function __construct($case_folding = false) {
         $this->xml_parser = xml_parser_create('UTF-8');
         xml_parser_set_option($this->xml_parser, XML_OPTION_CASE_FOLDING, $case_folding);
@@ -118,6 +123,18 @@ class progressive_parser {
         $this->processor = $processor;
     }
 
+    /**
+     * Sets the progress tracker for the parser. If set, the tracker will be
+     * called to report indeterminate progress for each chunk of XML.
+     *
+     * The caller should have already called start_progress on the progress tracker.
+     *
+     * @param core_backup_progress $progress Progress tracker
+     */
+    public function set_progress(core_backup_progress $progress) {
+        $this->progress = $progress;
+    }
+
     /*
      * Process the XML, delegating found chunks to the @progressive_parser_processor
      */
@@ -167,6 +184,10 @@ class progressive_parser {
 
     protected function publish($data) {
         $this->processor->receive_chunk($data);
+        if (!empty($this->progress)) {
+            // Report indeterminate progress.
+            $this->progress->progress();
+        }
     }
 
     /**
index d2de209..feb518e 100644 (file)
@@ -174,6 +174,8 @@ $string['nomatchingcourses'] = 'There are no courses to display';
 $string['norestoreoptions'] = 'There are no categories or existing courses you can restore to.';
 $string['originalwwwroot'] = 'URL of backup';
 $string['previousstage'] = 'Previous';
+$string['preparingui'] = 'Preparing to display page';
+$string['preparingdata'] = 'Preparing data';
 $string['qcategory2coursefallback'] = 'The questions category "{$a->name}", originally at system/course category context in backup file, will be created at course context by restore';
 $string['qcategorycannotberestored'] = 'The questions category "{$a->name}" cannot be created by restore';
 $string['question2coursefallback'] = 'The questions category "{$a->name}", originally at system/course category context in backup file, will be created at course context by restore';