From 188f6bdeb721602db07297200c21fd1f394e2d57 Mon Sep 17 00:00:00 2001 From: Marco Hermo Date: Thu, 28 Sep 2023 21:21:17 +1300 Subject: [PATCH] FIX Unpublising parent pages should include child pages Issue [#88](https://github.com/silverstripe/silverstripe-search-service/issues/88) Uses `SiteTree::enforce_strict_hierarchy` config in getting the dependent documents for indexing child pages. --- src/DataObject/DataObjectDocument.php | 18 ++++++ src/Jobs/RemoveDataObjectJob.php | 38 ++++++++++--- tests/DataObject/DataObjectDocumentTest.php | 57 +++++++++++++++++++ tests/Jobs/RemoveDataObjectJobTest.php | 61 ++++++++++++++++++++- tests/pages.yml | 8 +++ 5 files changed, 172 insertions(+), 10 deletions(-) diff --git a/src/DataObject/DataObjectDocument.php b/src/DataObject/DataObjectDocument.php index 31106aa..7ba0fca 100644 --- a/src/DataObject/DataObjectDocument.php +++ b/src/DataObject/DataObjectDocument.php @@ -5,6 +5,7 @@ use Exception; use InvalidArgumentException; use LogicException; +use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injectable; @@ -455,6 +456,10 @@ public function getDependentDocuments(): array } } + if ($ownedDataObject instanceof SiteTree && SiteTree::config()->get('enforce_strict_hierarchy')) { + $docs = array_merge($docs, $this->getChildDocuments($ownedDataObject)); + } + $dependentDocs = array_values($docs); $this->getDataObject()->invokeWithExtensions('updateSearchDependentDocuments', $dependentDocs); @@ -636,4 +641,17 @@ public function onRemoveFromSearchIndexes(string $event): void } } + public function getChildDocuments(SiteTree $page): array + { + $docs = []; + + foreach ($page->AllChildren() as $record) { + $document = DataObjectDocument::create($record); + $docs[$document->getIdentifier()] = $document; + $docs = array_merge($docs, $document->getDependentDocuments()); + } + + return $docs; + } + } diff --git a/src/Jobs/RemoveDataObjectJob.php b/src/Jobs/RemoveDataObjectJob.php index 58a7dae..5314339 100644 --- a/src/Jobs/RemoveDataObjectJob.php +++ b/src/Jobs/RemoveDataObjectJob.php @@ -52,20 +52,40 @@ public function setup(): void $documents = Versioned::withVersionedMode(function () use ($archiveDate) { Versioned::reading_archived_date($archiveDate); + $currentDocument = $this->getDocument(); // Go back in time to find out what the owners were before unpublish - $dependentDocs = $this->document->getDependentDocuments(); + $dependentDocs = $currentDocument->getDependentDocuments(); // refetch everything on the live stage Versioned::set_stage(Versioned::LIVE); - return array_map(function (DataObjectDocument $doc) { - return DataObjectDocument::create( - DataObject::get_by_id( - $doc->getSourceClass(), - $doc->getDataObject()->ID - ) - ); - }, $dependentDocs); + return array_reduce( + $dependentDocs, + function (array $carry, DataObjectDocument $doc) { + $record = DataObject::get_by_id($doc->getSourceClass(), $doc->getDataObject()->ID); + + // Since SiteTree::onBeforeDelete recursively deletes the child pages, + // they end up not found on a live environment which breaks DataObjectDocument::_constructor + if ($record) { + $document = DataObjectDocument::create($record); + $carry[$document->getIdentifier()] = $document; + + return $carry; + } + + // Taking into account that this queued job has a reference of existing child pages + // We need to make sure that we are able to send these pages to ElasticSearch etc. for removal + $oldRecord = $doc->getDataObject(); + + if ($oldRecord->isArchived() || $oldRecord->isOnDraft()) { + $document = DataObjectDocument::create($oldRecord); + $carry[$document->getIdentifier()] = $document; + } + + return $carry; + }, + [] + ); }); $this->setDocuments($documents); diff --git a/tests/DataObject/DataObjectDocumentTest.php b/tests/DataObject/DataObjectDocumentTest.php index 8f9b5ed..980e83b 100644 --- a/tests/DataObject/DataObjectDocumentTest.php +++ b/tests/DataObject/DataObjectDocumentTest.php @@ -3,6 +3,7 @@ namespace SilverStripe\SearchService\Tests\DataObject; use Page; +use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\RelationList; @@ -531,6 +532,7 @@ public function testGetDependentDocuments(): void $config->set('getSearchableClasses', [ DataObjectFake::class, ImageFake::class, + Page::class, ]); $config->set('getFieldsForClass', [ DataObjectFake::class => [ @@ -543,6 +545,10 @@ public function testGetDependentDocuments(): void ImageFake::class => [ new Field('tagtitles', 'Tags.Title'), ], + Page::class => [ + new Field('title'), + new Field('content'), + ], ]); $dataobject = $this->objFromFixture(TagFake::class, 'one'); @@ -576,6 +582,33 @@ public function testGetDependentDocuments(): void } $this->assertEqualsCanonicalizing($expectedDocuments, $resultDocuments); + + $pageOne = $this->objFromFixture(Page::class, 'page1'); + $pageDoc = DataObjectDocument::create($pageOne); + /** @var DataObjectDocument[] $dependentPages */ + $dependentPages = $pageDoc->getDependentDocuments(); + + // Grab all the expected pages + $pageTwo = $this->objFromFixture(Page::class, 'page2'); + $pageThree = $this->objFromFixture(Page::class, 'page3'); + $pageSeven = $this->objFromFixture(Page::class, 'page7'); + $pageEight = $this->objFromFixture(Page::class, 'page8'); + + $expectedPages = [ + sprintf('%s-%s', Page::class, $pageTwo->ID), + sprintf('%s-%s', Page::class, $pageThree->ID), + sprintf('%s-%s', Page::class, $pageSeven->ID), + sprintf('%s-%s', Page::class, $pageEight->ID), + ]; + + $resultPages = []; + + // Now let's check that each Document represents the Pages that we expected + foreach ($dependentPages as $document) { + $resultPages[] = sprintf('%s-%s', $document->getSourceClass(), $document->getDataObject()?->ID); + } + + $this->assertEqualsCanonicalizing($expectedPages, $resultPages); } public function testExtensionRequired(): void @@ -626,4 +659,28 @@ public function testDeletedDataObject(): void unserialize(serialize($doc)); } + public function testGetChildDocuments(): void + { + $pageOne = $this->objFromFixture(Page::class, 'page1'); + $pageTwo = $this->objFromFixture(Page::class, 'page2'); + $pageThree = $this->objFromFixture(Page::class, 'page3'); + $pageSeven = $this->objFromFixture(Page::class, 'page7'); + $pageEight = $this->objFromFixture(Page::class, 'page8'); + + $parentDocument = DataObjectDocument::create($pageOne); + $identifierPrefix = preg_replace('/\d$/', '', $parentDocument->getIdentifier()); + $childDocs = $parentDocument->getChildDocuments($pageOne); + + $expectedIdentifiers = [ + $identifierPrefix . $pageTwo->ID, + $identifierPrefix . $pageThree->ID, + $identifierPrefix . $pageSeven->ID, + $identifierPrefix . $pageEight->ID, + ]; + + $resultIdentifiers = ArrayList::create($childDocs)->column('getIdentifier'); + + $this->assertEqualsCanonicalizing($expectedIdentifiers, $resultIdentifiers); + } + } diff --git a/tests/Jobs/RemoveDataObjectJobTest.php b/tests/Jobs/RemoveDataObjectJobTest.php index 0cd12b2..3f98d0a 100644 --- a/tests/Jobs/RemoveDataObjectJobTest.php +++ b/tests/Jobs/RemoveDataObjectJobTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\SearchService\Tests\Jobs; +use Page; use SilverStripe\SearchService\DataObject\DataObjectDocument; use SilverStripe\SearchService\Jobs\RemoveDataObjectJob; use SilverStripe\SearchService\Schema\Field; @@ -16,7 +17,10 @@ class RemoveDataObjectJobTest extends SearchServiceTest { - protected static $fixture_file = '../fixtures.yml'; // phpcs:ignore + protected static $fixture_file = [ // @phpcs:ignore + '../fixtures.yml', + '../pages.yml', + ]; /** * @phpcsSuppress SlevomatCodingStandard.TypeHints.PropertyTypeHint.MissingNativeTypeHint @@ -83,4 +87,59 @@ public function testJob(): void $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); } + public function testUnpublishedParentPage(): void + { + $config = $this->mockConfig(); + + $config->set( + 'getSearchableClasses', + [ + Page::class, + ] + ); + + $config->set( + 'getFieldsForClass', + [ + Page::class => [ + new Field('title'), + new Field('content'), + ], + ] + ); + + // Publish all pages in fixtures since the internal dependency checks looks for live version + for ($i=1; $i<=8; $i++) { + $this->objFromFixture(Page::class, 'page' . $i)->publishRecursive(); + } + + // Queue up a job to remove a page with child pages are added as related documents + $pageOne = $this->objFromFixture(Page::class, 'page1'); + $pageDoc = DataObjectDocument::create($pageOne); + $job = RemoveDataObjectJob::create($pageDoc); + $job->setup(); + + // Grab what Documents the Job determined it needed to action + /** @var DataObjectDocument[] $documents */ + $documents = $job->getDocuments(); + + // There should be two Pages with this Tag assigned + $this->assertCount(4, $documents); + + $expectedTitles = [ + 'Child Page 1', + 'Child Page 2', + 'Grandchild Page 1', + 'Grandchild Page 2', + ]; + + $resultTitles = []; + + foreach ($documents as $document) { + $resultTitles[] = $document->getDataObject()?->Title; + } + + $this->assertEqualsCanonicalizing($expectedTitles, $resultTitles); + } + } diff --git a/tests/pages.yml b/tests/pages.yml index 83999f5..616ff25 100644 --- a/tests/pages.yml +++ b/tests/pages.yml @@ -21,3 +21,11 @@ Page: Title: Subsite Page 1 Subsite: =>SilverStripe\Subsites\Model\Subsite.subsite1 ShowInSearch: 1 + page7: + Title: Grandchild Page 1 + Parent: =>Page.page2 + ShowInSearch: 1 + page8: + Title: Grandchild Page 2 + Parent: =>Page.page2 + ShowInSearch: 1