MDL-63086 block_rss_client: Move skip time calculation to task
authorAndrew Nicols <andrew@nicols.co.uk>
Thu, 16 Aug 2018 00:54:13 +0000 (08:54 +0800)
committerAndrew Nicols <andrew@nicols.co.uk>
Mon, 27 Aug 2018 07:49:27 +0000 (15:49 +0800)
blocks/rss_client/block_rss_client.php
blocks/rss_client/classes/task/refreshfeeds.php
blocks/rss_client/tests/cron_test.php

index 2a2fd78..3b33049 100644 (file)
@@ -30,9 +30,6 @@
  */
 
  class block_rss_client extends block_base {
-    /** The maximum time in seconds that cron will wait between attempts to retry failing RSS feeds. */
-    const CLIENT_MAX_SKIPTIME = 43200; // 60 * 60 * 12 seconds.
-
     /** @var bool track whether any of the output feeds have recorded failures */
     private $hasfailedfeeds = false;
 
             return core_text::substr($title, 0, $max - 3) . '...';
         }
     }
-
-    /**
-     * Calculates a new skip time for a record based on the current skip time.
-     *
-     * @param int $currentskip The curreent skip time of a record.
-     * @return int A new skip time that should be set.
-     */
-    public function calculate_skiptime($currentskip) {
-        // The default time to skiptime.
-        $newskiptime = $this->cron * 1.1;
-        if ($currentskip > 0) {
-            // Double the last time.
-            $newskiptime = $currentskip * 2;
-        }
-        if ($newskiptime > self::CLIENT_MAX_SKIPTIME) {
-            // Do not allow the skip time to increase indefinatly.
-            $newskiptime = self::CLIENT_MAX_SKIPTIME;
-        }
-        return $newskiptime;
-    }
 }
index 3e34e1b..3e12ab2 100644 (file)
@@ -37,6 +37,9 @@ defined('MOODLE_INTERNAL') || die();
  */
 class refreshfeeds extends \core\task\scheduled_task {
 
+    /** The maximum time in seconds that cron will wait between attempts to retry failing RSS feeds. */
+    const CLIENT_MAX_SKIPTIME = HOURSECS * 12;
+
     /**
      * Name for this task.
      *
@@ -65,7 +68,7 @@ class refreshfeeds extends \core\task\scheduled_task {
      */
     public function execute() {
         global $CFG, $DB;
-        require_once($CFG->libdir.'/simplepie/moodle_simplepie.php');
+        require_once("{$CFG->libdir}/simplepie/moodle_simplepie.php");
 
         // We are going to measure execution times.
         $starttime = microtime();
@@ -84,22 +87,11 @@ class refreshfeeds extends \core\task\scheduled_task {
                 continue;
             }
 
-            // Fetch the rss feed, using standard simplepie caching
-            // so feeds will be renewed only if cache has expired.
-            \core_php_time_limit::raise(60);
-
-            $feed = new \moodle_simplepie();
-            // Set timeout for longer than normal to be agressive at
-            // fetching feeds if possible..
-            $feed->set_timeout(40);
-            $feed->set_cache_duration(0);
-            $feed->set_feed_url($rec->url);
-            $feed->init();
+            $feed = $this->fetch_feed($rec->url);
 
             if ($feed->error()) {
                 // Skip this feed (for an ever-increasing time if it keeps failing).
-                $block = new \block_rss_client();
-                $rec->skiptime = $block->calculate_skiptime($rec->skiptime);
+                $rec->skiptime = $this->calculate_skiptime($rec->skiptime);
                 $rec->skipuntil = time() + $rec->skiptime;
                 $DB->update_record('block_rss_client', $rec);
                 mtrace("Error: could not load/find the RSS feed - skipping for {$rec->skiptime} seconds.");
@@ -119,6 +111,41 @@ class refreshfeeds extends \core\task\scheduled_task {
 
         // Show times.
         mtrace($counter . ' feeds refreshed (took ' . microtime_diff($starttime, microtime()) . ' seconds)');
+    }
+
+    /**
+     * Fetch a feed for the specified URL.
+     *
+     * @param   string  $url The URL to fetch
+     * @return  \moodle_simplepie
+     */
+    protected function fetch_feed(string $url) : \moodle_simplepie {
+        // Fetch the rss feed, using standard simplepie caching so feeds will be renewed only if cache has expired.
+        \core_php_time_limit::raise(60);
+
+        $feed = new \moodle_simplepie();
+
+        // Set timeout for longer than normal to be agressive at fetching feeds if possible..
+        $feed->set_timeout(40);
+        $feed->set_cache_duration(0);
+        $feed->set_feed_url($url);
+        $feed->init();
+
+        return $feed;
+    }
+
+    /**
+     * Calculates a new skip time for a record based on the current skip time.
+     *
+     * @param   int     $currentskip The current skip time of a record.
+     * @return  int     The newly calculated skip time.
+     */
+    protected function calculate_skiptime(int $currentskip) : int {
+        // If the feed has never failed, then the initial skiptime will be 0. We use a default of 5 minutes in this case.
+        // If the feed has previously failed then we double that time.
+        $newskiptime = max(MINSECS * 5, ($currentskip * 2));
 
+        // Max out at the CLIENT_MAX_SKIPTIME.
+        return min($newskiptime, self::CLIENT_MAX_SKIPTIME);
     }
 }
index bae4cde..3b27084 100644 (file)
@@ -68,82 +68,74 @@ class block_rss_client_cron_testcase extends advanced_testcase {
         $this->assertContains('0 feeds refreshed (took ', $cronoutput);
     }
 
+    /**
+     * Data provider for skip time tests.
+     *
+     * @return  array
+     */
+    public function skip_time_increase_provider() : array {
+        return [
+            'Never failed' => [
+                'skiptime' => 0,
+                'skipuntil' => 0,
+                'newvalue' => MINSECS * 5,
+            ],
+            'Failed before' => [
+                // This should just double the time.
+                'skiptime' => 330,
+                'skipuntil' => time(),
+                'newvalue' => 660,
+            ],
+            'Near max' => [
+                'skiptime' => \block_rss_client\task\refreshfeeds::CLIENT_MAX_SKIPTIME - 5,
+                'skipuntil' => time(),
+                'newvalue' => \block_rss_client\task\refreshfeeds::CLIENT_MAX_SKIPTIME,
+            ],
+        ];
+    }
+
     /**
      * Test that when a feed has an error the skip time is increased correctly.
+     *
+     * @dataProvider    skip_time_increase_provider
      */
-    public function test_error() {
+    public function test_error($skiptime, $skipuntil, $newvalue) {
         global $DB, $CFG;
         $this->resetAfterTest();
+
+        require_once("{$CFG->libdir}/simplepie/moodle_simplepie.php");
+
         $time = time();
         // A record that has failed before.
-        $record = (object) array(
+        $record = (object) [
             'userid' => 1,
             'title' => 'Skip test feed',
             'preferredtitle' => '',
             'description' => 'A feed to test the skip time.',
             'shared' => 0,
             'url' => 'http://example.com/rss',
-            'skiptime' => 330,
-            'skipuntil' => $time - 300,
-        );
+            'skiptime' => $skiptime,
+            'skipuntil' => $skipuntil,
+        ];
         $record->id = $DB->insert_record('block_rss_client', $record);
 
-        // A record that has not failed before.
-        $record2 = (object) array(
-            'userid' => 1,
-            'title' => 'Skip test feed',
-            'preferredtitle' => '',
-            'description' => 'A feed to test the skip time.',
-            'shared' => 0,
-            'url' => 'http://example.com/rss2',
-            'skiptime' => 0,
-            'skipuntil' => 0,
-        );
-        $record2->id = $DB->insert_record('block_rss_client', $record2);
+        // Run the scheduled task and have it fail.
+        $task = $this->getMockBuilder(\block_rss_client\task\refreshfeeds::class)
+            ->setMethods(['fetch_feed'])
+            ->getMock();
 
-        // A record that is near the maximum wait time.
-        $record3 = (object) array(
-            'userid' => 1,
-            'title' => 'Skip test feed',
-            'preferredtitle' => '',
-            'description' => 'A feed to test the skip time.',
-            'shared' => 0,
-            'url' => 'http://example.com/rss3',
-            'skiptime' => block_rss_client::CLIENT_MAX_SKIPTIME - 5,
-            'skipuntil' => $time - 1,
-        );
-        $record3->id = $DB->insert_record('block_rss_client', $record3);
+        $piemock = $this->getMockBuilder(\moodle_simplepie::class)
+            ->setMethods(['error'])
+            ->getMock();
 
-        // Run the scheduled task.
-        $task = new \block_rss_client\task\refreshfeeds();
-        ob_start();
+        $piemock->method('error')
+            ->willReturn(true);
 
-        // Silence SimplePie php notices.
-        $errorlevel = error_reporting($CFG->debug & ~E_USER_NOTICE);
-        $task->execute();
-        error_reporting($errorlevel);
+        $task->method('fetch_feed')
+            ->willReturn($piemock);
 
-        $cronoutput = ob_get_clean();
-        $skiptime1 = $record->skiptime * 2;
-        $message1 = 'http://example.com/rss Error: could not load/find the RSS feed - skipping for ' . $skiptime1 . ' seconds.';
-        $this->assertContains($message1, $cronoutput);
-        $skiptime2 = 0;
-        $message2 = 'http://example.com/rss2 Error: could not load/find the RSS feed - skipping for ' . $skiptime2 . ' seconds.';
-        $this->assertContains($message2, $cronoutput);
-        $skiptime3 = block_rss_client::CLIENT_MAX_SKIPTIME;
-        $message3 = 'http://example.com/rss3 Error: could not load/find the RSS feed - skipping for ' . $skiptime3 . ' seconds.';
-        $this->assertContains($message3, $cronoutput);
-        $this->assertContains('0 feeds refreshed (took ', $cronoutput);
-
-        // Test that the records have been correctly updated.
-        $newrecord = $DB->get_record('block_rss_client', array('id' => $record->id));
-        $this->assertAttributeEquals($skiptime1, 'skiptime', $newrecord);
-        $this->assertAttributeGreaterThanOrEqual($time + $skiptime1, 'skipuntil', $newrecord);
-        $newrecord2 = $DB->get_record('block_rss_client', array('id' => $record2->id));
-        $this->assertAttributeEquals($skiptime2, 'skiptime', $newrecord2);
-        $this->assertAttributeGreaterThanOrEqual($time + $skiptime2, 'skipuntil', $newrecord2);
-        $newrecord3 = $DB->get_record('block_rss_client', array('id' => $record3->id));
-        $this->assertAttributeEquals($skiptime3, 'skiptime', $newrecord3);
-        $this->assertAttributeGreaterThanOrEqual($time + $skiptime3, 'skipuntil', $newrecord3);
+        // Run the cron and capture its output.
+        $this->expectOutputRegex("/.*Error: could not load\/find the RSS feed - skipping for {$newvalue} seconds.*/");
+        $task->execute();
     }
 }