diff --git a/.travis.yml b/.travis.yml index 555faa6..a6b0b63 100644 --- a/.travis.yml +++ b/.travis.yml @@ -14,6 +14,7 @@ matrix: env: - DB=MYSQL - PHPCS_TEST=1 + - PHPUNIT_TEST=1 - php: 7.0 env: - DB=PGSQL @@ -23,9 +24,6 @@ matrix: - PDO=1 before_script: - # - # Silverstripe 4 - # - phpenv rehash - phpenv config-rm xdebug.ini - composer validate @@ -34,8 +32,4 @@ before_script: - composer install --prefer-dist --no-interaction --no-progress --no-suggest --optimize-autoloader --verbose --profile script: - - # - # Silverstripe 4 - # - if [[ $PHPUNIT_TEST ]]; then vendor/bin/phpunit; fi diff --git a/docs/en/quick-start.md b/docs/en/quick-start.md index 1730686..e4797d8 100644 --- a/docs/en/quick-start.md +++ b/docs/en/quick-start.md @@ -8,17 +8,17 @@ MyDataObject: - Symbiote\DataChange\Extension\ChangeRecordable ``` -If you are applying the extension to the SiteTree, use the SiteTreeChangeRecordable +If you are applying the extension to the SiteTree, use the SiteTreeChangeRecordable extension to record publish/unpublish actions. -To track changes to many\_many relationships, you must use a custom -ManyManyRelationship class, as well as indicate which relationships need +To track changes to many\_many relationships, you must use a custom +ManyManyRelationship class, as well as indicate which relationships need tracking. This can be directly configured via the Injector For example ``` -Injector: +SilverStripe\Core\Injector\Injector: SilverStripe\ORM\ManyManyList: class: Symbiote\DataChange\Model\TrackedManyManyList properties: @@ -38,7 +38,7 @@ private static $many_many = array( ## Capturing URL parameters -Set the `save_request_vars` option to 1, and GET and POST vars will be recorded too. +Set the `save_request_vars` option to 1, and GET and POST vars will be recorded too. ``` Symbiote\DataChange\Model\DataChangeRecord: @@ -68,7 +68,7 @@ Symbiote\DataChange\Model\DataChangeRecord: ``` -Also, you may wish to blacklist some request variables from being stored +Also, you may wish to blacklist some request variables from being stored ``` Symbiote\DataChange\Model\DataChangeRecord: @@ -102,6 +102,6 @@ TeamMember: Over time, the data recorded will become overwhelming in size. May not be a problem for you, but if it is you can regularly prune it to retain just (N) months of data at a time. Simply create the `PruneChangesBeforeJob` -from the QueuedJob admin section of the CMS, using a constructor param of something like "-6 months". +from the QueuedJob admin section of the CMS, using a constructor param of something like "-6 months". -The job will restart itself to run each night, to consistently remove anything older than six months. +The job will restart itself to run each night, to consistently remove anything older than six months. diff --git a/src/Admin/DataChangeAdmin.php b/src/Admin/DataChangeAdmin.php index a1974a6..3b22e1d 100644 --- a/src/Admin/DataChangeAdmin.php +++ b/src/Admin/DataChangeAdmin.php @@ -3,6 +3,7 @@ namespace Symbiote\DataChange\Admin; use SilverStripe\Admin\ModelAdmin; +use Symbiote\DataChange\Model\DataChangeRecord; /** * @author marcus@symbiote.com.au @@ -10,9 +11,9 @@ */ class DataChangeAdmin extends ModelAdmin { - private static $managed_models = array( - 'Symbiote\DataChange\Model\DataChangeRecord', - ); + private static $managed_models = [ + DataChangeRecord::class, + ]; private static $url_segment = 'datachanges'; private static $menu_title = 'Data Changes'; diff --git a/src/Extension/ChangeRecordable.php b/src/Extension/ChangeRecordable.php index 3ef5a9d..ac69c48 100644 --- a/src/Extension/ChangeRecordable.php +++ b/src/Extension/ChangeRecordable.php @@ -2,8 +2,8 @@ namespace Symbiote\DataChange\Extension; +use Symbiote\DataChange\Service\DataChangeTrackService; use Symbiote\DataChange\Model\DataChangeRecord; - use SilverStripe\ORM\DataExtension; use SilverStripe\Core\Config\Config; @@ -21,10 +21,11 @@ class ChangeRecordable extends DataExtension * @var DataChangeTrackService */ public $dataChangeTrackService; - + private static $ignored_fields = array(); - + protected $isNewObject = false; + protected $changeType = 'Change'; public function __construct() @@ -66,7 +67,7 @@ public function getIgnoredFields() return array_combine($ignored[$class], $ignored[$class]); } } - + public function onBeforeVersionedPublish($from, $to) { if ($this->owner->isInDB()) { diff --git a/src/Model/DataChangeRecord.php b/src/Model/DataChangeRecord.php index ab9e8e3..a7b75d6 100644 --- a/src/Model/DataChangeRecord.php +++ b/src/Model/DataChangeRecord.php @@ -100,8 +100,8 @@ public function getCMSFields($params = null) ); if (strlen($this->Before) && strlen($this->ChangeRecordClass) && class_exists($this->ChangeRecordClass)) { - $before = Injector::inst()->create($this->ChangeRecordClass, json_decode($this->Before, true), true); - $after = Injector::inst()->create($this->ChangeRecordClass, json_decode($this->After, true), true); + $before = Injector::inst()->create($this->ChangeRecordClass, $this->prepareForDataDifferencer($this->Before), true); + $after = Injector::inst()->create($this->ChangeRecordClass, $this->prepareForDataDifferencer($this->After), true); $diff = DataDifferencer::create($before, $after); // The solr search service injector dependency causes issues with comparison, since it has public variables that are stored in an array. @@ -294,4 +294,19 @@ public function getMemberDetails() return $name; } } + + private function prepareForDataDifferencer($jsonData) + { + // NOTE(Jake): 2018-06-21 + // + // Data Differencer cannot handle arrays within an array, + // + // So JSON data that comes from MultiValueField / Text DB fields + // causes errors to be thrown. + // + // So solve this, we simply only decode to a depth of 1. (rather than the 512 default) + // + $resultJsonData = json_decode($jsonData, true, 1); + return $resultJsonData; + } } diff --git a/src/Model/TrackedManyManyList.php b/src/Model/TrackedManyManyList.php index a4f65d1..2c8300e 100644 --- a/src/Model/TrackedManyManyList.php +++ b/src/Model/TrackedManyManyList.php @@ -13,19 +13,21 @@ class TrackedManyManyList extends ManyManyList { public $trackedRelationships = array(); - + public function add($item, $extraFields = array()) { $this->recordManyManyChange(__FUNCTION__, $item); - return parent::add($item, $extraFields); + $result = parent::add($item, $extraFields); + return $result; } - + public function remove($item) { $this->recordManyManyChange(__FUNCTION__, $item); - return parent::remove($item); + $result = parent::remove($item); + return $result; } - + protected function recordManyManyChange($type, $item) { $joinName = $this->getJoinTable(); @@ -34,27 +36,50 @@ protected function recordManyManyChange($type, $item) } $parts = explode('_', $joinName); if (isset($parts[0]) && count($parts) > 1) { - // table name could be sometihng like Symbiote_DataChange_Tests_TestObject_Kids // which is ClassName_RelName, with - array_pop($parts); - - $addingToClass = implode('\\', $parts); - $addingTo = $this->getForeignID(); - - if (class_exists($addingToClass)) { - $onItem = $addingToClass::get()->byID($addingTo); - if ($onItem) { - if ($item && !($item instanceof DataObject)) { - $class = $this->dataClass; - $item = $class::get()->byID($item); - } - $join = $type == 'add' ? ' to ' : ' from '; - $type = ucfirst($type) . ' "' . $item->Title . '"' . $join . $parts[1]; - $onItem->RelatedItem = $item->ClassName . ' #' . $item->ID; - singleton('DataChangeTrackService')->track($onItem, $type); - } + $tableName = $parts; + $relationName = array_pop($tableName); + $tableName = implode('_', $tableName); + + $addingToClass = $this->tableClass($tableName); + if (!$addingToClass) { + return; + } + if (!class_exists($addingToClass)) { + return; } + $onItem = $addingToClass::get()->byID($this->getForeignID()); + if (!$onItem) { + return; + } + if ($item && !($item instanceof DataObject)) { + $class = $this->dataClass(); + $item = $class::get()->byID($item); + } + $join = $type === 'add' ? ' to ' : ' from '; + $type = ucfirst($type) . ' "' . $item->Title . '"' . $join . $relationName; + $onItem->RelatedItem = $item->ClassName . ' #' . $item->ID; + singleton('DataChangeTrackService')->track($onItem, $type); + } + } + + /** + * Find the class for the given table. + * + * Stripped down version from framework that does not attempt to strip _Live and _versions postfixes as + * that throws errors in its preg_match(). (At least it did as of 2018-06-22 on SilverStripe 4.1.1) + * + * @param string $table + * @return string|null The FQN of the class, or null if not found + */ + private function tableClass($table) + { + $tables = DataObject::getSchema()->getTableNames(); + $class = array_search($table, $tables, true); + if ($class) { + return $class; } + return null; } } diff --git a/src/Service/DataChangeTrackService.php b/src/Service/DataChangeTrackService.php index 6dd0098..d52e7f9 100644 --- a/src/Service/DataChangeTrackService.php +++ b/src/Service/DataChangeTrackService.php @@ -13,7 +13,7 @@ class DataChangeTrackService { protected $dcr_cache = array(); - + public $disabled = false; public function track(DataObject $object, $type = 'Change') @@ -26,7 +26,7 @@ public function track(DataObject $object, $type = 'Change') if (!isset($this->dcr_cache["{$object->ID}-{$object->Classname}-$type"])) { $this->dcr_cache["{$object->ID}-{$object->Classname}"] = DataChangeRecord::create(); } - + $this->dcr_cache["{$object->ID}-{$object->Classname}"]->track($object, $type); } diff --git a/tests/DataChangeCMSTest.php b/tests/DataChangeCMSTest.php new file mode 100644 index 0000000..f1abf2d --- /dev/null +++ b/tests/DataChangeCMSTest.php @@ -0,0 +1,78 @@ +TextFieldWithJSON = json_encode([ + 'The Pixies' => [ + 'Bossanova' => [ + 'The Happening' => [ + 'My head was feeling scared', + 'but my heart was feeling free', + ] + ], + ] + ]); + $record->write(); + $record->TextFieldWithJSON = json_encode([ + 'Radiohead' => [ + 'A Moonshaped Pool' => [ + 'Present Tense' => [ + 'Keep it light and', + 'Keep it moving', + 'I am doing', + 'No harm', + ] + ], + ] + ]); + $record->write(); + + // Get the data change tracker record that was written in 'TestTextJSONFieldObject's onAfterWrite() + $dataChangeTrackRecordIds = $record->getDataChangesList()->column('ID'); + $this->assertEquals(2, count($dataChangeTrackRecordIds)); + + // View in the CMS. + $this->logInWithPermission('ADMIN'); + $dataChangeTrackEditID = $dataChangeTrackRecordIds[0]; + $editLink = 'admin/datachanges/Symbiote-DataChange-Model-DataChangeRecord/EditForm/field/Symbiote-DataChange-Model-DataChangeRecord/item/'.$dataChangeTrackEditID.'/edit'; + + // NOTE(Jake): 2018-06-25 + // + // If the test fails, you will get something like: + // - nl2br() expects parameter 1 to be string, array given + // + // This is because the DataDifferencer can't work wtih a 'Text' field that returns an array. + // ie. `TestTextJSONFieldObject` custom getter "getTextFieldWithJSON" + // + $response = $this->get($editLink); + $this->assertEquals(200, $response->getStatusCode()); + + $body = $response->getBody(); + $this->assertTrue( + true, + strpos($body, 'Get Vars') !== false, + 'Cannot find \'Get Vars\' field to prove that we\'re actually on the editing DataChangeRecord page.' + ); + $this->assertTrue( + true, + strpos($body, 'Post Vars') !== false, + 'Cannot find \'Post Vars\' field to prove that we\'re actually on the editing DataChangeRecord page.' + ); + } +} diff --git a/tests/DataChangeTest.php b/tests/DataChangeTest.php index e0d94f7..c686257 100644 --- a/tests/DataChangeTest.php +++ b/tests/DataChangeTest.php @@ -2,18 +2,27 @@ namespace Symbiote\DataChange\Tests; -use \SilverStripe\Core\Injector\Injector; +use SilverStripe\Dev\SapphireTest; +use SilverStripe\Core\Injector\Injector; +use SilverStripe\Core\Config\Config; +use SilverStripe\ORM\ManyManyList; +use Symbiote\DataChange\Service\DataChangeTrackService; +use Symbiote\DataChange\Model\TrackedManyManyList; /** - * + * * * @author marcus */ -class DataChangeTest extends \SilverStripe\Dev\SapphireTest +class DataChangeTest extends SapphireTest { + protected $usesDatabase = true; + protected static $extra_dataobjects = [ - 'Symbiote\DataChange\Tests\TestTrackedObject', - 'Symbiote\DataChange\Tests\TestTrackedChild', + TestTrackedObject::class, + TestTrackedChild::class, + TestTrackedUnderscoreObject::class, + TestTrackedUnderscoreChild::class, ]; public function testTrackChange() @@ -21,7 +30,7 @@ public function testTrackChange() $obj = TestTrackedObject::create(['Title' => 'Object title']); $obj->write(); - singleton('DataChangeTrackService')->resetChangeCache(); + $this->getService()->resetChangeCache(); $obj->Title = 'Changed title'; $obj->write(); @@ -37,45 +46,124 @@ public function testTrackChange() $this->assertEquals('Changed title', $after['Title']); } - public function testManyManyChanges() { - $injectorConfig = \SilverStripe\Core\Config\Config::inst()->get(Injector::class); + public function testManyManyChanges_TableWithNoUnderscores() + { + // + // Setup data + // + $obj = TestTrackedObject::create(['Title' => 'Object title']); + $obj->write(); + $this->getService()->resetChangeCache(); + + $kid = TestTrackedChild::create(['Title' => 'kid object']); + $kid->write(); + $this->getService()->resetChangeCache(); - $injectorConfig['SilverStripe\ORM\ManyManyList'] = [ - 'class' => 'Symbiote\DataChange\Model\TrackedManyManyList', - 'properties' => [ - 'trackedRelationships' => [ - "Symbiote_DataChange_Tests_TestTrackedObject_Kids", + $kid2 = TestTrackedChild::create(['Title' => 'kid2 object']); + $kid2->write(); + $this->getService()->resetChangeCache(); + + // + // Test that we have overriden the injector config + // + $newInjectorConfig = [ + ManyManyList::class => [ + 'class' => TrackedManyManyList::class, + 'properties' => [ + 'trackedRelationships' => [ + "TestTrackedObject_Kids", + ] ] ] ]; + Injector::inst()->load($newInjectorConfig); - \SilverStripe\Core\Config\Config::modify()->set(Injector::class, 'SilverStripe\ORM\ManyManyList', $injectorConfig['SilverStripe\ORM\ManyManyList']); + // We want to check that $obj->Kids() is returning the injected TrackedManyManyList, not ManyManyList + $this->assertEquals(TrackedManyManyList::class, get_class($obj->Kids())); + // We want to make sure the join table looks like how we expect. + $this->assertEquals('TestTrackedObject_Kids', $obj->Kids()->getJoinTable()); - $obj = TestTrackedObject::create(['Title' => 'Object title']); - $obj->write(); + // + // Test Many many changes + // + $obj->Kids()->add($kid); + $this->getService()->resetChangeCache(); - singleton('DataChangeTrackService')->resetChangeCache(); + $obj->Kids()->add($kid2); + $this->getService()->resetChangeCache(); - $kid = TestTrackedChild::create(['Title' => 'kid object']); + $changes = $obj->getDataChangesList(); + + $mapped = $changes->toArray(); + $this->assertEquals( + 3, + count($mapped), + "Mismatching change records. This means the Injector config is most likely broken. Make sure the 'trackedRelationships' logic is set correctly. (This statement was written in 2018-06-22)" + ); + $this->assertEquals('Add "kid2 object" to Kids', $mapped[0]->ChangeType); + } + + public function testAManyManyChanges_TableWithUnderscores() + { + // + // Setup data + // + $obj = TestTrackedUnderscoreObject::create(['Title' => 'Underscore Object title']); + $obj->write(); + $this->getService()->resetChangeCache(); + + $kid = TestTrackedUnderscoreChild::create(['Title' => 'underscore kid object']); $kid->write(); + $this->getService()->resetChangeCache(); - singleton('DataChangeTrackService')->resetChangeCache(); - $kid2 = TestTrackedChild::create(['Title' => 'kid2 object']); + $kid2 = TestTrackedUnderscoreChild::create(['Title' => 'underscore kid2 object']); $kid2->write(); + $this->getService()->resetChangeCache(); + + // + // Test that we have overriden the injector config + // + $newInjectorConfig = [ + ManyManyList::class => [ + 'class' => TrackedManyManyList::class, + 'properties' => [ + 'trackedRelationships' => [ + 'Symbiote_DataChange_Tests_TestTrackedUnderscoreObject_Kids', + ] + ] + ] + ]; + Injector::inst()->load($newInjectorConfig); + + // We want to check that $obj->Kids() is returning the injected TrackedManyManyList, not ManyManyList + $this->assertEquals(TrackedManyManyList::class, get_class($obj->Kids())); - singleton('DataChangeTrackService')->resetChangeCache(); + // We want to make sure the join table looks like how we expect. + $this->assertEquals('Symbiote_DataChange_Tests_TestTrackedUnderscoreObject_Kids', $obj->Kids()->getJoinTable()); + // + // Test Many many changes + // $obj->Kids()->add($kid); - singleton('DataChangeTrackService')->resetChangeCache(); + $this->getService()->resetChangeCache(); $obj->Kids()->add($kid2); - singleton('DataChangeTrackService')->resetChangeCache(); - + $this->getService()->resetChangeCache(); + $changes = $obj->getDataChangesList(); $mapped = $changes->toArray(); - $this->assertEquals(3, count($mapped)); - $this->assertEquals('Add "kid2 object" to DataChange', $mapped[0]->ChangeType); + $this->assertEquals( + 3, + count($mapped), + "Mismatching change records. This means the Injector config is most likely broken. Make sure the 'trackedRelationships' logic is set correctly. (This statement was written in 2018-06-22)" + ); + $this->assertEquals('Add "underscore kid2 object" to Kids', $mapped[0]->ChangeType); + } + + private function getService() + { + return Injector::inst()->get('DataChangeTrackService'); } -} \ No newline at end of file +} diff --git a/tests/TestTextJSONFieldObject.php b/tests/TestTextJSONFieldObject.php new file mode 100644 index 0000000..9176dfa --- /dev/null +++ b/tests/TestTextJSONFieldObject.php @@ -0,0 +1,35 @@ + DBText::class, + ]; + + private static $extensions = [ + ChangeRecordable::class, + ]; + + /** + * This is the getter that can cause the DataDifferencer to fall + * over. + * + * This getter pattern was used in at least 1 internal Symbiote project. + */ + public function getTextFieldWithJSON() { + $value = $this->getField('TextFieldWithJSON'); + if (is_string($value)) { + $value = json_decode($value, true); + } + return $value; + } +} diff --git a/tests/TestTrackedChild.php b/tests/TestTrackedChild.php index eea64c9..e80c0d1 100644 --- a/tests/TestTrackedChild.php +++ b/tests/TestTrackedChild.php @@ -3,15 +3,18 @@ namespace Symbiote\DataChange\Tests; use SilverStripe\ORM\DataObject; +use SilverStripe\Dev\TestOnly; /** - * + * * * @author marcus */ -class TestTrackedChild extends DataObject implements \SilverStripe\Dev\TestOnly +class TestTrackedChild extends DataObject implements TestOnly { + private static $table_name = 'TestTrackedChild'; + private static $db = [ 'Title' => 'Varchar', ]; -} \ No newline at end of file +} diff --git a/tests/TestTrackedObject.php b/tests/TestTrackedObject.php index 431e54e..dcba66d 100644 --- a/tests/TestTrackedObject.php +++ b/tests/TestTrackedObject.php @@ -3,23 +3,27 @@ namespace Symbiote\DataChange\Tests; use SilverStripe\ORM\DataObject; +use Symbiote\DataChange\Extension\ChangeRecordable; +use SilverStripe\Dev\TestOnly; /** - * + * * * @author marcus */ -class TestTrackedObject extends DataObject implements \SilverStripe\Dev\TestOnly +class TestTrackedObject extends DataObject implements TestOnly { + private static $table_name = 'TestTrackedObject'; + private static $db = [ 'Title' => 'Varchar', ]; private static $many_many = [ - 'Kids' => 'Symbiote\DataChange\Tests\TestTrackedChild', + 'Kids' => TestTrackedChild::class, ]; private static $extensions = [ - 'Symbiote\DataChange\Extension\ChangeRecordable' + ChangeRecordable::class, ]; -} \ No newline at end of file +} diff --git a/tests/TestTrackedUnderscoreChild.php b/tests/TestTrackedUnderscoreChild.php new file mode 100644 index 0000000..db550ee --- /dev/null +++ b/tests/TestTrackedUnderscoreChild.php @@ -0,0 +1,20 @@ + 'Varchar', + ]; +} diff --git a/tests/TestTrackedUnderscoreObject.php b/tests/TestTrackedUnderscoreObject.php new file mode 100644 index 0000000..00dbf2b --- /dev/null +++ b/tests/TestTrackedUnderscoreObject.php @@ -0,0 +1,29 @@ + 'Varchar', + ]; + + private static $many_many = [ + 'Kids' => TestTrackedUnderscoreChild::class, + ]; + + private static $extensions = [ + ChangeRecordable::class, + ]; +}