From e9fe1a470720b29ebb1436b93a6f43e05a783c94 Mon Sep 17 00:00:00 2001 From: Kirk Mayo Date: Thu, 31 Jul 2014 16:49:20 +1200 Subject: [PATCH] NEW: Use DB row for job status and refactor the sql statements --- code/controllers/CMSExternalLinks.php | 71 ++++-------- code/jobs/CheckExternalLinksJob.php | 43 +------- code/model/BrokenExternalLink.php | 12 +- code/reports/BrokenExternalLinksReport.php | 28 ++--- code/tasks/CheckExternalLinks.php | 122 ++++++++++++++++++--- composer.json | 3 + javascript/BrokenExternalLinksReport.js | 15 ++- tests/ExternalLinksTest.php | 18 +-- tests/ExternalLinksTest.yml | 2 +- 9 files changed, 181 insertions(+), 133 deletions(-) diff --git a/code/controllers/CMSExternalLinks.php b/code/controllers/CMSExternalLinks.php index 5c23815..8e3ea1a 100644 --- a/code/controllers/CMSExternalLinks.php +++ b/code/controllers/CMSExternalLinks.php @@ -2,69 +2,44 @@ class CMSExternalLinks_Controller extends Controller { - private static $allowed_actions = array('getJobStatus', 'clear', 'start'); + private static $allowed_actions = array('getJobStatus', 'start'); /* * Respond to Ajax requests for info on a running job - * also calls continueJob and clear depending on the status of the job * * @return string JSON string detailing status of the job */ public function getJobStatus() { - $trackID = Session::get('ExternalLinksTrackID'); - if (!$trackID) return; - $noPages = Versioned::get_by_stage('SiteTree', 'Live')->count(); - $result = BrokenExternalPageTrack::get() - ->filter('TrackID', $trackID) - ->exclude('PageID', 0); - $completedPages = count($result); - + $track = CheckExternalLinks::getLatestTrack(); + if (!$track || !$track->exists()) return null; echo json_encode(array( - 'TrackID' => $trackID, - 'Completed' => $completedPages, - 'Total' => $noPages + 'TrackID' => $track->ID, + 'Status' => $track->Status, + 'Completed' => $track->CompletedPages, + 'Total' => $track->TotalPages )); - - if ($completedPages >= $noPages) { - $this->clear(); - } else { - $this->continueJob(); - } } - /* - * Clears the tracking id and any surplus entries for the BrokenExternalPageTrack model - */ - public function clear() { - // clear any old entries - $trackID = Session::get('ExternalLinksTrackID'); - $oldEntries = BrokenExternalPageTrack::get() - ->exclude('TrackID', $trackID); - foreach ($oldEntries as $entry) { - $entry->delete(); - } - Session::clear('ExternalLinksTrackID'); - } /* * Starts a broken external link check */ public function start() { - $track = BrokenExternalPageTrack::create(); - $track->write(); - $track->TrackID = $track->ID; - $track->write(); - - Session::set('ExternalLinksTrackID', $track->ID); - - $this->continueJob(); - } - - /* - * Continues a broken external link check - */ - public function continueJob() { - $task = new CheckExternalLinks(); - $task->run(null); + $status = checkExternalLinks::getLatestTrackStatus(); + // return if the a job is already running + if ($status == 'Running') { + return; + } + if (class_exists('QueuedJobService')) { + $checkLinks = new CheckExternalLinksJob(); + singleton('QueuedJobService') + ->queueJob($checkLinks, date('Y-m-d H:i:s', time() + 1)); + } else { + //TODO this hangs as it waits for the connection to be released + // should return back and continue processing + // http://us3.php.net/manual/en/features.connection-handling.php + $task = new CheckExternalLinks(); + $task->run(); + } } } diff --git a/code/jobs/CheckExternalLinksJob.php b/code/jobs/CheckExternalLinksJob.php index af76fc2..030f1ea 100644 --- a/code/jobs/CheckExternalLinksJob.php +++ b/code/jobs/CheckExternalLinksJob.php @@ -8,12 +8,6 @@ if(!class_exists('AbstractQueuedJob')) return; */ class CheckExternalLinksJob extends AbstractQueuedJob implements QueuedJob { - public function __construct() { - $this->pagesToProcess = Versioned::get_by_stage('SiteTree', 'Live')->column(); - $this->currentStep = 0; - $this->totalSteps = count($this->pagesToProcess); - } - public function getTitle() { return _t('CheckExternalLiksJob.TITLE', 'Checking for external broken links'); } @@ -26,47 +20,14 @@ class CheckExternalLinksJob extends AbstractQueuedJob implements QueuedJob { return md5(get_class($this)); } - public function setup() { - parent::setup(); - $restart = $this->currentStep == 0; - if ($restart) { - $this->pagesToProcess = Versioned::get_by_stage('SiteTree', 'Live')->column(); - } - - } - /** * Check a individual page */ public function process() { - $remainingPages = $this->pagesToProcess; - if (!count($remainingPages)) { - $this->isComplete = true; - return; - } - - // lets process our first item - note that we take it off the list of things left to do - $ID = array_shift($remainingPages); - - // get the page - $page = Versioned::get_by_stage('SiteTree', 'Live', 'ID = '.$ID); - - if (!$page || !$page->Count()) { - $this->addMessage("Page ID #$ID could not be found, skipping"); - } - $task = new CheckExternalLinks(); - $task->pageToProcess = $page; $task->run(); - - // and now we store the new list of remaining children - $this->pagesToProcess = $remainingPages; - $this->currentStep++; - - if (!count($remainingPages)) { - $this->isComplete = true; - return; - } + $this->isComplete = true; + return; } } diff --git a/code/model/BrokenExternalLink.php b/code/model/BrokenExternalLink.php index 89aeeb8..a716aea 100644 --- a/code/model/BrokenExternalLink.php +++ b/code/model/BrokenExternalLink.php @@ -32,9 +32,19 @@ class BrokenExternalLink extends DataObject { } } +class BrokenExternalPageTrackStatus extends DataObject { + private static $db = array( + 'Status' => 'Enum("Completed, Running", "Running")', + 'TotalPages' => 'Int', + 'CompletedPages' => 'Int', + 'JobInfo' => 'Varchar(255)' + ); +} + class BrokenExternalPageTrack extends DataObject { private static $db = array( - 'TrackID' => 'Int' + 'TrackID' => 'Int', + 'Processed' => 'Boolean' ); private static $has_one = array( diff --git a/code/reports/BrokenExternalLinksReport.php b/code/reports/BrokenExternalLinksReport.php index 9e54a46..a57e976 100644 --- a/code/reports/BrokenExternalLinksReport.php +++ b/code/reports/BrokenExternalLinksReport.php @@ -68,21 +68,21 @@ class BrokenExternalLinksReport extends SS_Report { public function getCMSFields() { Requirements::javascript('externallinks/javascript/BrokenExternalLinksReport.js'); $fields = parent::getCMSFields(); - if (class_exists('AbstractQueuedJob')) { - $button = ''; - $runReportButton = new LiteralField( - 'runReport', - sprintf( - $button, - _t('ExternalBrokenLinksReport.RUNREPORT', 'Create new report') - ) - ); - $fields->push($runReportButton); - $reportResultSpan = '

'; - $reportResult = new LiteralField('ResultTitle', $reportResultSpan); - $fields->push($reportResult); - } + $reportResultSpan = '

'; + $reportResult = new LiteralField('ResultTitle', $reportResultSpan); + $fields->push($reportResult); + + $button = ''; + $runReportButton = new LiteralField( + 'runReport', + sprintf( + $button, + _t('ExternalBrokenLinksReport.RUNREPORT', 'Create new report') + ) + ); + $fields->push($runReportButton); + return $fields; } } diff --git a/code/tasks/CheckExternalLinks.php b/code/tasks/CheckExternalLinks.php index e63815b..960987f 100644 --- a/code/tasks/CheckExternalLinks.php +++ b/code/tasks/CheckExternalLinks.php @@ -12,23 +12,63 @@ class CheckExternalLinks extends BuildTask { private $totalPages; function run($request) { - $trackID = Session::get('ExternalLinksTrackID'); - if (isset($this->pageToProcess)) { - $pages = $this->pageToProcess; - } else { - if ($trackID) { - $result = BrokenExternalPageTrack::get() - ->filter('TrackID', $trackID); - $pages = Versioned::get_by_stage('SiteTree', 'Live') - ->exclude('ID', $result->column('PageID')) - ->limit(10); - } else { - $pages = Versioned::get_by_stage('SiteTree', 'Live'); + $track = CheckExternalLinks::getLatestTrack(); + + // if the script has already been started + if ($track && $track->Status == 'Running') { + $batch = BrokenExternalPageTrack::get() + ->filter(array( + 'TrackID' => $track->ID, + 'Processed' => 0 + ))->limit(10)->column('PageID'); + $pages = Versioned::get_by_stage('SiteTree', 'Live') + ->filter('ID', $batch) + ->limit(10); + $this->updateJobInfo('Fetching pages to check'); + if ($track->CompletedPages == $track->TotalPages) { + $track->Status = 'Completed'; + $track->write(); + $this->updateJobInfo('Setting to completed'); } + // if the script is to be started + } else { + $pages = Versioned::get_by_stage('SiteTree', 'Live')->column('ID'); + $noPages = count($pages); + + $track = BrokenExternalPageTrackStatus::create(); + $track->TotalPages = $noPages; + $track->write(); + $this->updateJobInfo('Creating new tracking object'); + + foreach ($pages as $page) { + $trackPage = BrokenExternalPageTrack::create(); + $trackPage->PageID = $page; + $trackPage->TrackID = $track->ID; + $trackPage->write(); + } + + $batch = BrokenExternalPageTrack::get() + ->filter(array( + 'TrackID' => $track->ID + ))->limit(10)->column('PageID'); + + $pages = Versioned::get_by_stage('SiteTree', 'Live') + ->filter('ID', $batch); } + $trackID = $track->ID; foreach ($pages as $page) { ++$this->totalPages; + if ($track->ID) { + $trackPage = BrokenExternalPageTrack::get() + ->filter(array( + 'PageID' => $page->ID, + 'TrackID' => $track->ID + ))->first(); + $trackPage->Processed = 1; + $trackPage->write(); + } + $htmlValue = Injector::inst()->create('HTMLValue', $page->Content); if (!$htmlValue->isValid()) { continue; @@ -100,12 +140,27 @@ class CheckExternalLinks extends BuildTask { } } ++$this->completedPages; - if ($trackID) { - $trackPage = new BrokenExternalPageTrack(); - $trackPage->PageID = $page->ID; - $trackPage->TrackID = $trackID; - $trackPage->write(); + } + + // run this outside the foreach loop to stop it locking DB rows + $this->updateJobInfo('Updating completed pages'); + $this->updateCompletedPages($trackID); + + // do we need to carry on running the job + $track = $this->getLatestTrack(); + if ($track->CompletedPages >= $track->TotalPages) { + $track->Status = 'Completed'; + $track->write(); + + // clear any old previous data + $rows = BrokenExternalPageTrack::get() + ->exclude('TrackID', $track->ID); + foreach ($rows as $row) { + $row->delete(); } + } else { + $this->updateJobInfo("Running next batch {$track->CompletedPages}/{$track->TotalPages}"); + $this->run($request); } // run this again if queued jobs exists and is a valid int @@ -115,6 +170,39 @@ class CheckExternalLinks extends BuildTask { singleton('QueuedJobService') ->queueJob($checkLinks, date('Y-m-d H:i:s', time() + $queuedJob)); } + } + public static function getLatestTrack() { + $track = BrokenExternalPageTrackStatus::get()->sort('ID', 'DESC')->first(); + if (!$track || !$track->exists()) return null; + return $track; + } + + public static function getLatestTrackID() { + $track = CheckExternalLinks::getLatestTrack(); + if (!$track || !$track->exists()) return null; + return $track->ID; + } + + public static function getLatestTrackStatus() { + $track = CheckExternalLinks::getLatestTrack(); + if (!$track || !$track->exists()) return null; + return $track->Status; + } + + private function updateCompletedPages($trackID = 0) { + $noPages = BrokenExternalPageTrack::get() + ->filter(array('TrackID' => $trackID, 'Processed' => 1))->count(); + $track = $this->getLatestTrack($trackID); + $track->CompletedPages = $noPages; + $track->write(); + return $noPages; + } + + private function updateJobInfo($message) { + $track = CheckExternalLinks::getLatestTrack(); + if (!$track || !$track->exists()) return null; + $track->JobInfo = $message; + $track->write(); } } diff --git a/composer.json b/composer.json index 7f73048..36506f7 100644 --- a/composer.json +++ b/composer.json @@ -13,5 +13,8 @@ { "silverstripe/framework": ">=3.0", "silverstripe/cms": ">=3.0" + }, + "suggest": { + "silverstripe/queuedjobs": "Speeds up running the job for Content Editors fropm the report" } } diff --git a/javascript/BrokenExternalLinksReport.js b/javascript/BrokenExternalLinksReport.js index 9cf4a32..5dd7046 100644 --- a/javascript/BrokenExternalLinksReport.js +++ b/javascript/BrokenExternalLinksReport.js @@ -4,12 +4,16 @@ $(this).start(); $(this).poll(); }, + onmatch: function() { + $(this).poll(); + }, start: function() { // initiate a new job $('#ReportHolder').empty(); $('#ReportHolder').text('Running report 0%'); $('#ReportHolder').append(''); - $.ajax({url: "admin/externallinks/start", async: true, timeout: 1000 }); + $.ajax({url: "admin/externallinks/start", async: true, timeout: 3000 }); + $(this).poll(); }, poll: function() { // poll the current job and update the front end status @@ -18,13 +22,16 @@ async: true, success: function(data) { var obj = $.parseJSON(data); - if (!obj) return; + if (!obj) { + setTimeout(function() { $('#externalLinksReport').poll(); }, 1000); + } var completed = obj.Completed ? obj.Completed : 0; var total = obj.Total ? obj.Total : 0; - if (total > 0 && completed == total) { + var jobStatus = obj.Status ? obj.Status : 'Running'; + if (jobStatus == 'Completed') { $('#ReportHolder').text('Report Finished ' + completed + '/' + total); } else { - setTimeout(function() { $('#externalLinksReport').poll(); }, 1); + setTimeout(function() { $('#externalLinksReport').poll(); }, 1000); } if (total && completed) { if (completed < total) { diff --git a/tests/ExternalLinksTest.php b/tests/ExternalLinksTest.php index ae9d898..a9b813f 100644 --- a/tests/ExternalLinksTest.php +++ b/tests/ExternalLinksTest.php @@ -4,25 +4,28 @@ class ExternalLinks extends FunctionalTest { protected static $fixture_file = 'ExternalLinksTest.yml'; - public function testWorkingLink() { + public function testLinks() { // uses http://127.0.0.1 to test a working link - $page = $this->objFromFixture('Page', 'working'); + $working = $this->objFromFixture('SiteTree', 'working'); + $working->publish('Stage', 'Live'); $task = new CheckExternalLinks(); - $task->run($page); - $brokenLinks = BrokenExternalLinks::get(); + $task->run(null); + $brokenLinks = BrokenExternalLink::get(); $this->assertEquals(0, $brokenLinks->count()); } public function testBrokenLink() { // uses http://192.0.2.1 for a broken link - $page = $this->objFromFixture('Page', 'broken'); + $broken = $this->objFromFixture('SiteTree', 'broken'); + $broken->publish('Stage', 'Live'); $task = new CheckExternalLinks(); - $task->run($page); - $brokenLinks = BrokenExternalLinks::get(); + $task->run(null); + $brokenLinks = BrokenExternalLink::get(); $this->assertEquals(1, $brokenLinks->count()); } public function testReportExists() { + $mock = $this->objFromFixture('SiteTree', 'broken'); $reports = SS_Report::get_reports(); $reportNames = array(); foreach($reports as $report) { @@ -32,3 +35,4 @@ class ExternalLinks extends FunctionalTest { 'BrokenExternalLinksReport is in reports list'); } } + diff --git a/tests/ExternalLinksTest.yml b/tests/ExternalLinksTest.yml index 8a25106..46eb0a8 100644 --- a/tests/ExternalLinksTest.yml +++ b/tests/ExternalLinksTest.yml @@ -1,4 +1,4 @@ -Page: +SiteTree: working: Title: Working Link Content: 'Localhost'