From: mjollnir_ <mjollnir_>
Date: Fri, 12 Sep 2008 11:22:15 +0000 (+0000)
Subject: MDL-15666 - change all the portfolio plugins and callers to use exceptions
X-Git-Url: http://git.mjollnir.org/gw?a=commitdiff_plain;h=37f03ea00bfb4bcfe1ceb7db8cf022c3e90ddb40;p=moodle.git

MDL-15666 - change all the portfolio plugins and callers to use exceptions
rather than relying on return values (send_package and prepare_package)

move zipping of temporary files into the exporter class so it can be overridden during unit tests

fix a small todo in mahara plugin
---

diff --git a/lang/en_utf8/portfolio.php b/lang/en_utf8/portfolio.php
index 2a7b1356e6..d711599fbc 100644
--- a/lang/en_utf8/portfolio.php
+++ b/lang/en_utf8/portfolio.php
@@ -6,7 +6,7 @@ $string['addtoportfolio'] = 'Add to portfolio';
 $string['addalltoportfolio'] = 'Add all to portfolio';
 $string['alreadyexporting'] = 'You already have an active portfolio export in this session. Please <a href=\"$a->finish\">complete that first</a>, or <a href=\"$a->cancel\">click here</a> to cancel it.';
 $string['availableformats'] = 'Available export formats';
-$string['callercouldnotpackage'] = 'Failed to package up your data for export';
+$string['callercouldnotpackage'] = 'Failed to package up your data for export: original error was $a';
 $string['cannotsetvisible'] = 'Cannot set this to visible - the plugin has been completely disabled because of a misconfiguration';
 $string['configexport'] = 'Configure exported data';
 $string['configplugin'] = 'Configure portfolio plugin';
@@ -28,7 +28,8 @@ $string['exportqueued'] = 'Portfolio export has been successfully queued for tra
 $string['exportqueuedforced'] = 'Portfolio export has been successfully queued for transfer (the remote system has enforced queued transfers)';
 $string['exportedpreviously'] = 'Previous exports';
 $string['exportexceptionnoexporter'] = 'A portfolio_export_exception was thrown with an active session but no exporter object';
-$string['failedtosendpackage'] = 'Failed to send your data to the selected portfolio system!';
+$string['failedtosendpackage'] = 'Failed to send your data to the selected portfolio system: original error was $a';
+$string['failedtopackage'] = 'Could not find files to package';
 $string['filedenied'] = 'Access denied to this file';
 $string['filenotfound'] = 'File not found';
 $string['format_file'] = 'File';
@@ -79,7 +80,7 @@ $string['nouploaddirectory'] = 'Could not create a temporary directory to packag
 $string['portfolio'] = 'Portfolio';
 $string['portfolios'] = 'Portfolios';
 $string['plugin'] = 'Portfolio Plugin';
-$string['plugincouldnotpackage'] = 'Failed to package up your data for export';
+$string['plugincouldnotpackage'] = 'Failed to package up your data for export: original error was $a';
 $string['returntowhereyouwere'] = 'Return to where you were';
 $string['save'] = 'Save';
 $string['selectedformat'] = 'Selected export format';
diff --git a/lang/en_utf8/portfolio_boxnet.php b/lang/en_utf8/portfolio_boxnet.php
index 92e57fa314..1c22963a76 100644
--- a/lang/en_utf8/portfolio_boxnet.php
+++ b/lang/en_utf8/portfolio_boxnet.php
@@ -19,5 +19,6 @@ $string['sharefolder'] = 'Share this new folder?';
 $string['targetfolder'] = 'Target folder';
 $string['tobecreated'] = 'To be created';
 $string['username'] = 'Your box.net username (will not be stored)';
+$string['sendfailed'] = 'Failed to send content to box.net: $a';
 
 ?>
diff --git a/lib/portfolio/exporter.php b/lib/portfolio/exporter.php
index 2519cd577a..2c4e4cfb7a 100644
--- a/lib/portfolio/exporter.php
+++ b/lib/portfolio/exporter.php
@@ -426,11 +426,22 @@ class portfolio_exporter {
         // now we've agreed on a format,
         // the caller is given control to package it up however it wants
         // and then the portfolio plugin is given control to do whatever it wants.
-        if (!$this->caller->prepare_package()) {
-            throw new portfolio_export_exception($this, 'callercouldnotpackage', 'portfolio');
+        try {
+            $this->caller->prepare_package();
+        } catch (portfolio_exception $e) {
+            throw new portfolio_export_exception($this, 'callercouldnotpackage', 'portfolio', null, $e->getMessage());
+        }
+        catch (file_exception $e) {
+            throw new portfolio_export_exception($this, 'callercouldnotpackage', 'portfolio', null, $e->getMessage());
+        }
+        try {
+            $this->instance->prepare_package();
         }
-        if (!$package = $this->instance->prepare_package()) {
-            throw new portfolio_export_exception($this, 'plugincouldnotpackage', 'portfolio');
+        catch (portfolio_exception $e) {
+            throw new portfolio_export_exception($this, 'plugincouldnotpackage', 'portfolio', null, $e->getMessage());
+        }
+        catch (file_exception $e) {
+            throw new portfolio_export_exception($this, 'plugincouldnotpackage', 'portfolio', null, $e->getMessage());
         }
         return true;
     }
@@ -465,8 +476,13 @@ class portfolio_exporter {
     */
     public function process_stage_send() {
         // send the file
-        if (!$this->instance->send_package()) {
-            throw new portfolio_export_exception($this, 'failedtosendpackage', 'portfolio');
+        try {
+            $this->instance->send_package();
+        }
+        catch (portfolio_plugin_exception $e) {
+            // not catching anything more general here. plugins with dependencies on other libraries that throw exceptions should catch and rethrow.
+            // eg curl exception
+            throw new portfolio_export_exception($this, 'failedtosendpackage', 'portfolio', null, $e->getMessage());
         }
         // log the transfer
         global $DB;
@@ -493,7 +509,7 @@ class portfolio_exporter {
         $extras = $this->instance->get_extra_finish_options();
 
         $key = 'exportcomplete';
-        if ($queued) {
+        if ($queued || $this->forcequeue) {
             $key = 'exportqueued';
             if ($this->forcequeue) {
                 $key = 'exportqueuedforced';
@@ -660,6 +676,17 @@ class portfolio_exporter {
         return $fs->create_file_from_string($file_record, $content);
     }
 
+    public function zip_tempfiles($filename='portfolio-export.zip', $filepath='/final/') {
+        $zipper = new zip_packer();
+
+        list ($contextid, $filearea, $itemid) = array_values($this->get_base_filearea());
+        if ($newfile = $zipper->archive_to_storage($files, $contextid, $filearea, $itemid, $filepath, $filename, $this->user->id)) {
+            return $newfile;
+        }
+        return false;
+
+    }
+
     /**
     * returns an arary of files in the temporary working directory
     * for this export
diff --git a/mod/assignment/lib.php b/mod/assignment/lib.php
index 84864d648a..06302a1c5d 100644
--- a/mod/assignment/lib.php
+++ b/mod/assignment/lib.php
@@ -3200,7 +3200,6 @@ class assignment_portfolio_caller extends portfolio_module_caller_base {
         foreach ($this->files as $file) {
             $this->exporter->copy_existing_file($file);
         }
-        return true;
     }
 
     public function get_sha1() {
diff --git a/mod/assignment/type/online/assignment.class.php b/mod/assignment/type/online/assignment.class.php
index eb6b4eed9b..5879abcef9 100644
--- a/mod/assignment/type/online/assignment.class.php
+++ b/mod/assignment/type/online/assignment.class.php
@@ -275,7 +275,7 @@ class assignment_online extends assignment_base {
 
     function portfolio_prepare_package($exporter, $userid=0) {
         $submission = $this->get_submission($userid);
-        return $exporter->write_new_file(format_text($submission->data1, $submission->data2), 'assignment.html');
+        $exporter->write_new_file(format_text($submission->data1, $submission->data2), 'assignment.html');
     }
 
     function portfolio_supported_formats() {
diff --git a/mod/chat/lib.php b/mod/chat/lib.php
index b4976007c6..03e34dc5e2 100644
--- a/mod/chat/lib.php
+++ b/mod/chat/lib.php
@@ -899,7 +899,7 @@ class chat_portfolio_caller extends portfolio_module_caller_base {
         }
         $content = preg_replace('/\<img[^>]*\>/', '', $content);
 
-        return $this->exporter->write_new_file($content, clean_filename($this->cm->name . '-session.html'));
+        $this->exporter->write_new_file($content, clean_filename($this->cm->name . '-session.html'));
     }
 
     public static function display_name() {
diff --git a/mod/forum/lib.php b/mod/forum/lib.php
index 07d0a13e2d..6b3923c28e 100644
--- a/mod/forum/lib.php
+++ b/mod/forum/lib.php
@@ -7301,16 +7301,14 @@ class forum_portfolio_caller extends portfolio_module_caller_base {
                 $content .= '<br /><br />' . $this->prepare_post($post);
                 $this->copy_files($this->allfiles[$post->id]);
             }
-            $this->get('exporter')->write_new_file($content, 'discussion.html');
-        } else {
-            $this->copy_files($this->postfiles, $this->attachment);
-            if ($this->attachment) {
-                return true; // all we need to do
-            }
-            $post = $this->prepare_post($this->post);
-            $this->get('exporter')->write_new_file($post, 'post.html');
+            return $this->get('exporter')->write_new_file($content, 'discussion.html');
         }
-        return true;
+        $this->copy_files($this->postfiles, $this->attachment);
+        if ($this->attachment) {
+            return; // all we need to do
+        }
+        $post = $this->prepare_post($this->post);
+        $this->get('exporter')->write_new_file($post, 'post.html');
     }
 
     private function copy_files($files, $justone=false) {
diff --git a/portfolio/type/boxnet/lib.php b/portfolio/type/boxnet/lib.php
index 45a5e6e34b..e90eb5190f 100644
--- a/portfolio/type/boxnet/lib.php
+++ b/portfolio/type/boxnet/lib.php
@@ -23,11 +23,10 @@ class portfolio_plugin_boxnet extends portfolio_plugin_push_base {
             $this->folders[$created['folder_id']] = $created['folder_name'];
             $this->set_export_config(array('folder' => $created['folder_id']));
         }
-        return true; // don't do anything else for this plugin, we want to send all files as they are.
+        // don't do anything else for this plugin, we want to send all files as they are.
     }
 
     public function send_package() {
-        $ret = array();
         foreach ($this->exporter->get_tempfiles() as $file) {
             $return = $this->boxclient->uploadFile(
                 array(
@@ -38,14 +37,13 @@ class portfolio_plugin_boxnet extends portfolio_plugin_push_base {
             );
             if (array_key_exists('status', $return) && $return['status'] == 'upload_ok'
                 && array_key_exists('id', $return) && count($return['id']) == 1) {
-                $return['rename'] = $this->rename_file($return['id'][array_pop(array_keys($return['id']))], $file->get_filename());
-                $ret[] = $return;
+                $this->rename_file($return['id'][array_pop(array_keys($return['id']))], $file->get_filename());
+                // if this fails, the file was sent but not renamed - this triggers a warning but is not fatal.
             }
         }
         if ($this->boxclient->isError()) {
-            return false;
+            throw new portfolio_plugin_exception('sendfailed', 'portfolio_boxnet', $this->boxclient->getErrorMsg());
         }
-        return is_array($ret) && !empty($ret);
     }
 
     public function get_export_summary() {
diff --git a/portfolio/type/download/lib.php b/portfolio/type/download/lib.php
index d98ea9c3e8..d5a9386262 100644
--- a/portfolio/type/download/lib.php
+++ b/portfolio/type/download/lib.php
@@ -25,18 +25,9 @@ class portfolio_plugin_download extends portfolio_plugin_pull_base {
 
         if (count($files) == 1) {
             $this->set('file', array_shift($files));
-            return true;
+        } else {
+            $this->set('file', $this->exporter->zip_tempfiles());  // this will throw a file_exception which the exporter catches separately.
         }
-
-        $zipper = new zip_packer();
-
-        $filename = 'portfolio-export.zip';
-        list ($contextid, $filearea, $itemid) = array_values($this->get('exporter')->get_base_filearea());
-        if ($newfile = $zipper->archive_to_storage($files, $contextid, $filearea, $itemid, '/final/', $filename, $this->user->id)) {
-            $this->set('file', $newfile);
-            return true;
-        }
-        return false;
     }
 
     public function get_extra_finish_options() {
@@ -44,9 +35,7 @@ class portfolio_plugin_download extends portfolio_plugin_pull_base {
         return array($this->get_base_file_url() => get_string('downloadfile', 'portfolio_download'));
     }
 
-    public function send_package() {
-        return true;
-    }
+    public function send_package() {}
 
     public function verify_file_request_params($params) {
         // for download plugin the only thing we need to verify is that
@@ -63,4 +52,3 @@ class portfolio_plugin_download extends portfolio_plugin_pull_base {
     }
 }
 
-?>
diff --git a/portfolio/type/flickr/lib.php b/portfolio/type/flickr/lib.php
index 60cf91f1c7..fde801c681 100755
--- a/portfolio/type/flickr/lib.php
+++ b/portfolio/type/flickr/lib.php
@@ -16,11 +16,10 @@ class portfolio_plugin_flickr extends portfolio_plugin_push_base {
 
     public function prepare_package() {
         $this->flickr = new phpFlickr($this->get_config('apikey'), $this->get_config('sharedsecret'));
-        return true; // don't do anything else for this plugin, we want to send all files as they are.
     }
 
     public function send_package() {
-
+        throw new portfolio_plugin_exception('notimplemented', 'portfolio', null, 'flickr');
     }
 
     public static function allows_multiple() {
diff --git a/portfolio/type/mahara/lib.php b/portfolio/type/mahara/lib.php
index d5d95c58f9..e7b4e17729 100644
--- a/portfolio/type/mahara/lib.php
+++ b/portfolio/type/mahara/lib.php
@@ -145,14 +145,8 @@ class portfolio_plugin_mahara extends portfolio_plugin_pull_base {
             );
             $this->totalsize += $f->get_filesize();
         }
-        $zipper = new zip_packer();
 
-        $filename = 'portfolio-export.zip';
-        if ($newfile = $zipper->archive_to_storage($files, SYSCONTEXTID, 'portfolio_exporter', $this->exporter->get('id'), '/final/', $filename, $this->user->id)) {
-            $this->set('file', $newfile);
-            return true;
-        }
-        return false;
+        $this->set('file', $this->exporter->zip_tempfiles());  // this will throw a file_exception which the exporter catches separately.
     }
 
     private function ensure_environment() {
@@ -194,9 +188,9 @@ class portfolio_plugin_mahara extends portfolio_plugin_pull_base {
         if (!$response->status) {
             throw new portfolio_export_exception($this->get('exporter'), 'failedtoping', 'portfolio_mahara');
         }
-        // @todo penny we should check $response->type here, it will tell us  'queued' or 'complete'
-        // and we might want to tell the user if it's queued.
-        return true;
+        if ($response->type =='queued') {
+            $this->exporter->set('forcequeue', true);
+        }
     }
 
     public function get_continue_url() {