From: Penny Leach Date: Wed, 18 Nov 2009 12:27:15 +0000 (+0000) Subject: portfolio MDL-20872 elegant handling of cleanup/display race condition X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=5d0dbf130de7a92ed48932e50d0023fbe3624612;p=moodle.git portfolio MDL-20872 elegant handling of cleanup/display race condition lots of work to elegantly resolve the issue between interactive browser sessions and "pull" portfolio plugins that cause a race condition where we lose the ability to display information to the user if the pull cleans up the export first. this also improves the portfolio transfer log display for later --- diff --git a/lang/en_utf8/portfolio.php b/lang/en_utf8/portfolio.php index ec958abb5a..3cf2f806e0 100644 --- a/lang/en_utf8/portfolio.php +++ b/lang/en_utf8/portfolio.php @@ -29,6 +29,8 @@ $string['dontwait'] = 'Don\'t wait'; $string['err_uniquename'] = 'Portfolio name must be unique (per plugin)'; $string['enabled'] = 'Enable portfolios'; $string['enableddesc'] = 'This will allow administrators to configure remote systems for users to export content to'; +$string['exportalreadyfinished']= 'Portfolio export complete!'; +$string['exportalreadyfinisheddesc'] = 'Portfolio export complete!'; $string['exporting'] = 'Exporting to portfolio'; $string['exportingcontentfrom'] = 'Exporting content from $a'; $string['exportcomplete'] = 'Portfolio export complete!'; diff --git a/lib/portfolio/exporter.php b/lib/portfolio/exporter.php index ae74fdce47..f542698118 100644 --- a/lib/portfolio/exporter.php +++ b/lib/portfolio/exporter.php @@ -117,6 +117,12 @@ class portfolio_exporter { */ private $expirytime; + /** + * deleted - this is set during the cleanup routine + * so that subsequent save() calls can detect it + */ + private $deleted = false; + /** * construct a new exporter for use * @@ -485,6 +491,7 @@ class portfolio_exporter { $DB->delete_records('portfolio_tempdata', array('id' => $this->id)); $fs = get_file_storage(); $fs->delete_area_files(SYSCONTEXTID, 'portfolio_exporter', $this->id); + $this->deleted = true; return true; } @@ -523,11 +530,23 @@ class portfolio_exporter { 'caller_file' => $this->callerfile, 'caller_sha1' => $this->caller->get_sha1(), 'caller_class' => get_class($this->caller), + 'continueurl' => $this->instance->get_static_continue_url(), + 'returnurl' => $this->caller->get_return_url(), + 'tempdataid' => $this->id, 'time' => time(), ); $DB->insert_record('portfolio_log', $l); } + /** + * in some cases (mahara) we need to update this after the log has been done + * because of MDL-20872 + */ + public function update_log_url($url) { + global $DB; + $DB->set_field('portfolio_log', 'continueurl', $url, array('tempdataid' => $this->id)); + } + /** * processes the 'finish' stage of the export * @@ -536,7 +555,7 @@ class portfolio_exporter { public function process_stage_finished($queued=false) { global $OUTPUT; $returnurl = $this->caller->get_return_url(); - $continueurl = $this->instance->get_continue_url(); + $continueurl = $this->instance->get_interactive_continue_url(); $extras = $this->instance->get_extra_finish_options(); $key = 'exportcomplete'; @@ -616,7 +635,9 @@ class portfolio_exporter { $this->save(); // call again so that id gets added to the save data. } else { if (!$r = $DB->get_record('portfolio_tempdata', array('id' => $this->id))) { - debugging("tried to save current object, but failed - see MDL-20872"); + if (!$this->deleted) { + debugging("tried to save current object, but failed - see MDL-20872"); + } return; } $r->data = base64_encode(serialize($this)); @@ -774,7 +795,7 @@ class portfolio_exporter { * * @return array of stored_file objects keyed by name */ - public function get_tempfiles() { + public function get_tempfiles($skipfile='portfolio-export.zip') { $fs = get_file_storage(); $files = $fs->get_area_files(SYSCONTEXTID, 'portfolio_exporter', $this->id, '', false); if (empty($files)) { @@ -782,6 +803,9 @@ class portfolio_exporter { } $returnfiles = array(); foreach ($files as $f) { + if ($f->get_filename() == $skipfile) { + continue; + } $returnfiles[$f->get_filepath() . '/' . $f->get_filename()] = $f; } return $returnfiles; @@ -856,15 +880,18 @@ class portfolio_exporter { return $DB->get_records_sql_menu($sql, $values); } - public static function print_cleaned_export($log) { + public static function print_cleaned_export($log, $instance=null) { global $CFG, $OUTPUT, $PAGE; + if (empty($instance) || !$instance instanceof portfolio_plugin) { + $instance = portfolio_instance($log->portfolio); + } $title = get_string('exportalreadyfinished', 'portfolio'); $PAGE->navbar->add($title); $PAGE->set_title($title); $PAGE->set_heading($title); echo $OUTPUT->header(); echo $OUTPUT->notification(get_string('exportalreadyfinished', 'portfolio')); - self::print_finish_info($log->returnurl, $log->continueurl); + self::print_finish_info($log->returnurl, $instance->resolve_static_continue_url($log->continueurl)); echo $OUTPUT->continue_button($CFG->wwwroot); echo $OUTPUT->footer(); exit; diff --git a/lib/portfolio/plugin.php b/lib/portfolio/plugin.php index a8a91349bb..fd0dbe9666 100644 --- a/lib/portfolio/plugin.php +++ b/lib/portfolio/plugin.php @@ -325,10 +325,29 @@ abstract class portfolio_plugin_base { /** * the url for the user to continue to their portfolio + * during the lifecycle of the request * * @return string url or false. */ - public abstract function get_continue_url(); + public abstract function get_interactive_continue_url(); + + /** + * the url to save in the log as the continue url + * this is passed through resolve_static_continue_url() + * at display time to the user. + */ + public function get_static_continue_url() { + return $this->get_interactive_continue_url(); + } + + /** + * override this function if you need to add something on to the url + * for post-export continues (eg from the log page) + * mahara does this, for example, to start a jump session + */ + public function resolve_static_continue_url($url) { + return $url; + } /** * mform to display to the user in their profile diff --git a/lib/simpletest/portfolio_testclass.php b/lib/simpletest/portfolio_testclass.php index 40586d7d2c..29aa3db592 100644 --- a/lib/simpletest/portfolio_testclass.php +++ b/lib/simpletest/portfolio_testclass.php @@ -51,7 +51,7 @@ class portfolio_plugin_test extends portfolio_plugin_push_base { return true; } - public function get_continue_url() { + public function get_interactive_continue_url() { return ''; } diff --git a/portfolio/type/boxnet/lib.php b/portfolio/type/boxnet/lib.php index f03278c419..408c7faeee 100644 --- a/portfolio/type/boxnet/lib.php +++ b/portfolio/type/boxnet/lib.php @@ -60,7 +60,7 @@ class portfolio_plugin_boxnet extends portfolio_plugin_push_base { ); } - public function get_continue_url() { + public function get_interactive_continue_url() { return 'http://box.net/files#0:f:' . $this->get_export_config('folder'); } diff --git a/portfolio/type/download/lib.php b/portfolio/type/download/lib.php index 1f175c952b..3ad63109a7 100644 --- a/portfolio/type/download/lib.php +++ b/portfolio/type/download/lib.php @@ -48,7 +48,7 @@ class portfolio_plugin_download extends portfolio_plugin_pull_base { return true; } - public function get_continue_url() { + public function get_interactive_continue_url() { return false; } } diff --git a/portfolio/type/flickr/lib.php b/portfolio/type/flickr/lib.php index 2ce4df1817..4fdd703ad8 100755 --- a/portfolio/type/flickr/lib.php +++ b/portfolio/type/flickr/lib.php @@ -55,7 +55,7 @@ class portfolio_plugin_flickr extends portfolio_plugin_push_base { return false; } - public function get_continue_url() { + public function get_interactive_continue_url() { // return $this->flickr->urls_getUserPhotos(); return "http://www.flickr.com/tools/uploader_edit.gne?ids="; // Add ids of uploaded files } diff --git a/portfolio/type/googledocs/lib.php b/portfolio/type/googledocs/lib.php index adfcc5507c..db02fd1313 100644 --- a/portfolio/type/googledocs/lib.php +++ b/portfolio/type/googledocs/lib.php @@ -23,7 +23,7 @@ class portfolio_plugin_googledocs extends portfolio_plugin_push_base { return true; } - public function get_continue_url(){ + public function get_interactive_continue_url(){ return 'http://docs.google.com/'; } diff --git a/portfolio/type/mahara/lib.php b/portfolio/type/mahara/lib.php index 2105cbeafe..8688644e18 100644 --- a/portfolio/type/mahara/lib.php +++ b/portfolio/type/mahara/lib.php @@ -195,22 +195,40 @@ class portfolio_plugin_mahara extends portfolio_plugin_pull_base { if (isset($response->querystring)) { $this->continueurl = $response->querystring; } + // if we're not queuing the logging might have already happened + $this->exporter->update_log_url($this->get_static_continue_url()); } - public function get_continue_url() { - $this->ensure_mnethost(); - $this->ensure_environment(); - $mnetauth = get_auth_plugin('mnet'); + public function get_static_continue_url() { $remoteurl = '/artefact/file/';// @todo penny this might change later when we change formats. if (isset($this->continueurl)) { $remoteurl .= $this->continueurl; } + return $remoteurl; + } + + public function resolve_static_continue_url($remoteurl) { + static $sessions = array(); + // if this is called mutliple times for the same host, stuff breaks + // so we have to keep track and just replace the wantsurl bit + // in case things go to different plugins or whatever + if (array_key_exists($this->get_config('mnethostid'), $sessions)) { + return preg_replace('/wantsurl=[^&]*&/', 'wantsurl=' . urlencode($remoteurl) . '&', $sessions[$this->get_config('mnethostid')]); + } + $this->ensure_mnethost(); + $this->ensure_environment(); + $mnetauth = get_auth_plugin('mnet'); if (!$url = $mnetauth->start_jump_session($this->get_config('mnethostid'), $remoteurl)) { return false; } + $sessions[$this->get_config('mnethostid')] = $url; return $url; } + public function get_interactive_continue_url() { + return $this->resolve_static_continue_url($this->get_static_continue_url()); + } + public function steal_control($stage) { if ($stage != PORTFOLIO_STAGE_CONFIG) { return false; diff --git a/portfolio/type/picasa/lib.php b/portfolio/type/picasa/lib.php index 763945a527..0632a7a0ee 100644 --- a/portfolio/type/picasa/lib.php +++ b/portfolio/type/picasa/lib.php @@ -24,7 +24,7 @@ class portfolio_plugin_picasa extends portfolio_plugin_push_base { return true; } - public function get_continue_url(){ + public function get_interactive_continue_url(){ return 'http://picasaweb.google.com/'; } diff --git a/user/portfoliologs.php b/user/portfoliologs.php index 078b622efc..b059d9e670 100644 --- a/user/portfoliologs.php +++ b/user/portfoliologs.php @@ -130,14 +130,19 @@ if ($logcount > 0) { $pluginname = ''; try { $plugin = portfolio_instance($log->portfolio); - $pluginname = $plugin->get('name'); + $url = $plugin->resolve_static_continue_url($log->continueurl); + if ($url) { + $pluginname = '' . $plugin->get('name') . ''; + } else { + $pluginname = $plugin->get('name'); + } } catch (portfolio_exception $e) { // may have been deleted $pluginname = get_string('unknownplugin', 'portfolio'); } $table->data[] = array( $pluginname, - call_user_func(array($class, 'display_name')), + '' . call_user_func(array($class, 'display_name')) . '', userdate($log->time), ); }