ENHANCEMENT: Implemented a basic suite of unit tests. MINOR: fixed indentation in GoogleSiteMapDecorator. BUGFIX: fixed potential corrupt XML from invalid Priority fields. Fixed #4

code/GoogleSitemap.php

@@ -26,11 +26,6 @@ class GoogleSitemap extends Controller {
 	 */
 	protected static $enabled = true;
 	
-	/**
-	 * @var DataObjectSet
-	 */
-	protected $Pages;
-	
 	/**
 	 * @var boolean
 	 */
@@ -148,12 +143,12 @@ class GoogleSitemap extends Controller {
 			$filter = "{$bt}ShowInSearch{$bt} = 1";
 		}
 
-		$this->Pages = Versioned::get_by_stage('SiteTree', 'Live', $filter);
+		$pages = Versioned::get_by_stage('SiteTree', 'Live', $filter);
 		
 		$newPages = new DataObjectSet();
 		
-		if($this->Pages) {
+		if($pages) {
-			foreach($this->Pages as $page) {
+			foreach($pages as $page) {
 				// Only include pages from this host and pages which are not an 
 				// instance of ErrorPage. We prefix $_SERVER['HTTP_HOST'] with 
 				// 'http://' so that parse_url to help parse_url identify the 
@@ -182,9 +177,13 @@ class GoogleSitemap extends Controller {
 	
 	/**
 	 * Notifies Google about changes to your sitemap.
+	 *
 	 * Triggered automatically on every publish/unpublish of a page.
 	 * This behaviour is disabled by default, enable with:
+	 *
+	 * <code>
 	 * GoogleSitemap::enable_google_notificaton();
+	 * </code>
 	 *
 	 * If the site is in "dev-mode", no ping will be sent regardless wether
 	 * the Google notification is enabled.
@@ -241,7 +240,7 @@ class GoogleSitemap extends Controller {
 			// But we want to still render.
 			return array();
 		} else {
-			return new SS_HTTPResponse('Not allowed', 405);
+			return new SS_HTTPResponse('Page not found', 404);
 		}
 	}
 	

code/GoogleSitemapDecorator.php

@@ -19,12 +19,12 @@ class GoogleSitemapSiteTreeDecorator extends SiteTreeDecorator {
 		return array(
 			'db' => array(
 				"Priority" => "Varchar(5)",
-            )
+			),
 		);
 	}
 
 	function updateCMSFields(&$fields) {
-        $pagePriorities = array(
+		$prorities = array(
 			'' => _t('SiteTree.PRIORITYAUTOSET', 'Auto-set based on page depth'),
 			'-1' => _t('SiteTree.PRIORITYNOTINDEXED', "Not indexed"), // We set this to -ve one because a blank value implies auto-generation of Priority
 			'1.0' => '1 - ' . _t('SiteTree.PRIORITYMOSTIMPORTANT', "Most important"),
@@ -40,23 +40,17 @@ class GoogleSitemapSiteTreeDecorator extends SiteTreeDecorator {
 		);
 
 		$tabset = $fields->findOrMakeTab('Root.Content');
-        $tabset->push(
+		
-                $addTab = new Tab(
+		$message = "<p>";
-                        'GoogleSitemap',
+		$message .= sprintf(_t('SiteTree.METANOTEPRIORITY', "Manually specify a Google Sitemaps priority for this page (%s)"), 
-                        _t('SiteTree.TABGOOGLESITEMAP', 'Google Sitemap'),
+			'<a href="http://www.google.com/support/webmasters/bin/answer.py?hl=en&answer=71936#prioritize" target="_blank">?</a>'
-                        new LiteralField(
-                                "GoogleSitemapIntro",
-                                "<p>" .
-                                sprintf(
-                                        _t(
-                                                'SiteTree.METANOTEPRIORITY', "Manually specify a Google Sitemaps priority for this page (%s)"
-                                        ), '<a href="http://www.google.com/support/webmasters/bin/answer.py?hl=en&answer=71936#prioritize" target="_blank">?</a>'
-                                ) .
-                                "</p>"
-                        ),
-                        new DropdownField("Priority", $this->owner->fieldLabel('Priority'), $pagePriorities)
-                )
 		);
+		$message .=  "</p>";
+		
+		$tabset->push(new Tab('GoogleSitemap', _t('SiteTree.TABGOOGLESITEMAP', 'Google Sitemap'),
+			new LiteralField("GoogleSitemapIntro", $message),
+			new DropdownField("Priority", $this->owner->fieldLabel('Priority'), $prorities)
+		));
 	}
 
 	function updateFieldLabels(&$labels) {
@@ -76,16 +70,23 @@ class GoogleSitemapSiteTreeDecorator extends SiteTreeDecorator {
 	/**
 	 * The default value of the priority field depends on the depth of the page in
 	 * the site tree, so it must be calculated dynamically.
+	 *
+	 * @return float
 	 */
 	function getPriority() {
-        if (!$this->owner->getField('Priority')) {
+		if(!$this->owner->getField('Priority')) {
 			$parentStack = $this->owner->parentStack();
 			$numParents = is_array($parentStack) ? count($parentStack) - 1 : 0;
+			
 			return max(0.1, 1.0 - ($numParents / 10));
-        } elseif ($this->owner->getField('Priority') == -1) {
+		} 
+		elseif ($this->owner->getField('Priority') == -1) {
 			return 0;
-        } else {
+		} 
-            return $this->owner->getField('Priority');
+		else {
+			$priority = abs($this->owner->getField('Priority'));
+			
+			return (is_float($priority) && $priority <= 1.0) ? $priority : 0.5;
 		}
 	}
 
@@ -98,7 +99,6 @@ class GoogleSitemapSiteTreeDecorator extends SiteTreeDecorator {
 	public function setChangeFrequency() {
 		// The one field that isn't easy to deal with in the template is
 		// Change frequency, so we set that here.
-
 		$date = date('Y-m-d H:i:s');
 
 		$prop = $this->owner->toMap();
@@ -116,23 +116,17 @@ class GoogleSitemapSiteTreeDecorator extends SiteTreeDecorator {
 		$period = $timediff / ($versions + 1);
 
 		if ($period > 60 * 60 * 24 * 365) {
-            // > 1 year
 			$this->owner->ChangeFreq = 'yearly';
 		} elseif ($period > 60 * 60 * 24 * 30) {
 			$this->owner->ChangeFreq = 'monthly';
 		} elseif ($period > 60 * 60 * 24 * 7) {
-            // > 1 week
 			$this->owner->ChangeFreq = 'weekly';
 		} elseif ($period > 60 * 60 * 24) {
-            // > 1 day
 			$this->owner->ChangeFreq = 'daily';
 		} elseif ($period > 60 * 60) {
-            // > 1 hour
 			$this->owner->ChangeFreq = 'hourly';
 		} else {
-            // < 1 hour
 			$this->owner->ChangeFreq = 'always';
 		}
 	}
-
 }

tests/GoogleSitemapTest.php

@@ -1,20 +1,148 @@
 <?php
 
 /**
- * Unit test for GoogleSitemap
+ * @package googlesitemaps
- *
- * @author Roland Lehmann <rlehmann@pixeltricks.de>
- * @since 11.06.2011
  */
-class GoogleSitemapTest extends SapphireTest {
+class GoogleSitemapTest extends FunctionalTest {
 
-    public function testItems() {
+	public static $fixture_file = 'googlesitemaps/tests/GoogleSitemapTest.yml';
-        //Publish a page and check if it returns
+
-        $obj = $this->objFromFixture("Page", "Page1");
+	protected $extraDataObjects = array(
-        $page = DataObject::get_by_id("Page", $obj->ID);
+		'GoogleSitemapTest_DataObject',
-        #$page->publish();
+		'GoogleSitemapTest_OtherDataObject',
-        #$sitemap = new GoogleSitemap();
+		'GoogleSitemapTest_UnviewableDataObject'
-        #$this->assertEquals(1, $sitemap->Items()->Count());
+	);
+
+	function testItems() {
+		$page = $this->objFromFixture('Page', 'Page1');
+		$page->publish('Stage', 'Live');
+		$page->flushCache();
+		
+		$page2 = $this->objFromFixture('Page', 'Page2');
+		$page2->publish('Stage', 'Live');
+		$page2->flushCache();
+
+		$sitemap = new GoogleSitemap();
+		$this->assertDOSEquals(array(
+			array('Title' => 'Testpage1'),
+			array('Title' => 'Testpage2')
+		), $sitemap->Items(), "There should be 2 pages in the sitemap after publishing");
+		
+		// check if we make a page readonly that it is hidden
+		$page2->CanViewType = 'LoggedInUsers';
+		$page2->write();	
+		$page2->publish('Stage', 'Live');
+		
+		$this->session()->inst_set('loggedInAs', null);
+		
+		$this->assertDOSEquals(array(
+			array('Title' => 'Testpage1')
+		), $sitemap->Items(), "There should be only 1 page, other is logged in only");
+		
+		// register a DataObject and see if its aded to the sitemap
+		GoogleSitemap::register_dataobject("GoogleSitemapTest_DataObject", '');
+		
+		// check to see if we have the GoogleSitemapTest_DataObject objects
+		$this->assertEquals(3, $sitemap->Items()->Count());
+
+		// register another dataobject
+		GoogleSitemap::register_dataobject("GoogleSitemapTest_OtherDataObject");
+		$this->assertEquals(4, $sitemap->Items()->Count());
+
+		// check if we register objects that are unreadable they don't end up
+		// in the sitemap
+		GoogleSitemap::register_dataobject("GoogleSitemapTest_UnviewableDataObject");
+		$this->assertEquals(4, $sitemap->Items()->Count());
+	}
+	
+	function testAccess() {
+		GoogleSitemap::enable();
+		
+		$response = $this->get('sitemap.xml');
+		$this->assertEquals(200, $response->getStatusCode(), 'Sitemap returns a 200 success when enabled');
+		$this->assertEquals('application/xml; charset="utf-8"', $response->getHeader('Content-Type'));
+		
+		GoogleSitemap::disable();
+		
+		$response = $this->get('sitemap.xml');
+		$this->assertEquals(404, $response->getStatusCode(), 'Sitemap returns a 404 when disabled');
+	}
+	
+	function testDecoratorAddsFields() {
+		$page = $this->objFromFixture('Page', 'Page1');
+		
+		$fields = $page->getCMSFields();
+		
+		$tab = $fields->fieldByName('Root')->fieldByName('Content')->fieldByName('GoogleSitemap');
+		
+		$this->assertInstanceOf('Tab', $tab);
+		$this->assertInstanceOf('DropdownField', $tab->fieldByName('Priority'));
+		$this->assertInstanceOf('LiteralField', $tab->fieldByName('GoogleSitemapIntro'));
+	}
+	
+	function testGetPriority() {
+		$page = $this->objFromFixture('Page', 'Page1');
+		
+		// invalid field doesn't break google
+		$page->Priority = 'foo';
+		$this->assertEquals(0.5, $page->getPriority());
+		
+		// google doesn't like -1 but we use it to indicate the minimum
+		$page->Priority = -1;
+		$this->assertEquals(0, $page->getPriority());
 	}
 }
 
+/**
+ * @package googlesitemaps
+ */
+class GoogleSitemapTest_DataObject extends DataObject implements TestOnly {
+	
+	public static $db = array(
+		'Priority' => 'Varchar(10)'
+	);
+
+	public function canView($member = null) {
+		return true;
+	}
+
+	public function AbsoluteLink() {
+		return Director::baseURL();
+	}
+}
+
+/**
+ * @package googlesitemaps
+ */
+class GoogleSitemapTest_OtherDataObject extends DataObject implements TestOnly {
+
+	public static $db = array(
+		'Priority' => 'Varchar(10)'
+	);
+
+	public function canView($member = null) {
+		return true;
+	}
+
+	public function AbsoluteLink() {
+		return Director::baseURL();
+	}
+}
+
+/**
+ * @package googlesitemaps
+ */
+class GoogleSitemapTest_UnviewableDataObject extends DataObject implements TestOnly {
+
+	public static $db = array(
+		'Priority' => 'Varchar(10)'
+	);
+
+	public function canView($member = null) {
+		return false;
+	}
+
+	public function AbsoluteLink() {
+		return Director::baseURL();
+	}
+}

tests/GoogleSitemapTest.yml

@@ -1,9 +1,20 @@
 Page:
     Page1:
+        Title: Testpage1
+        Priority: 0.2
     Page2:
-    Page3:
+        Title: Testpage2
         
-DataObject:
+GoogleSitemapTest_DataObject:
-    DataObject1:
+    DataObjectTest1:
-    DataObject2:
+        Priority: 0.4
-    DataObject3:
+    DataObjectTest2:
+        Priority: 0.2
+
+GoogleSitemapTest_OtherDataObject:
+    OtherDataObjectTest2:
+        Priority: 0.3
+    
+GoogleSitemapTest_UnviewableDataObject:
+    Unviewable1:
+        Priority: 0.4