From 8883413ba71a7b117b79a237d07f3ed72301fa8a Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 27 Aug 2020 11:18:54 +1200 Subject: [PATCH] NEW: Add GridFieldDetailForm::setRedirectMissingRecords() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This new opt-in setting will let grid field detail forms redirect to the “Correct” URL of a GridField if it’s not found in the current list. This works by: * Looking for the item in the database * If it exists, check for a CMSEditLink() method that returns a value * If so, redirect to that This is useful if you have a number of grid fields that each show a partial list of records, and it’s possible for the user to make changes such the item no longer appears in the list, but does appear in another list. It’s an opt-in feature as I think all changes like this should be opt-in, based on previous experiences improving GridField and in turn breaking SecurityAdmin and slowing versioned-data-browsing down. ;-) --- src/Forms/GridField/GridFieldDetailForm.php | 87 +++++++++++++++++++ .../GridFieldDetailForm_ItemRequest.php | 5 ++ .../GridField/GridFieldDetailFormTest.php | 36 +++++++- .../GridField/GridFieldDetailFormTest.yml | 3 + .../CategoryController.php | 2 +- .../GridFieldDetailFormTest/Person.php | 5 ++ .../TestController.php | 4 +- 7 files changed, 137 insertions(+), 5 deletions(-) diff --git a/src/Forms/GridField/GridFieldDetailForm.php b/src/Forms/GridField/GridFieldDetailForm.php index f9a76d209..a6dae5aba 100644 --- a/src/Forms/GridField/GridFieldDetailForm.php +++ b/src/Forms/GridField/GridFieldDetailForm.php @@ -3,8 +3,11 @@ namespace SilverStripe\Forms\GridField; use Closure; +use SilverStripe\Control\Director; use SilverStripe\Control\HTTPRequest; use SilverStripe\Control\HTTPResponse; +use SilverStripe\Control\HTTPResponse_Exception; +use SilverStripe\Control\HTTPStreamResponse; use SilverStripe\Control\RequestHandler; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Extensible; @@ -12,6 +15,7 @@ use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\FieldList; use SilverStripe\Forms\Validator; +use SilverStripe\ORM\DataList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\Filterable; @@ -68,6 +72,12 @@ class GridFieldDetailForm implements GridField_URLHandler */ protected $itemRequestClass; + /** + * If true, will redirect to missing records if they are found elsewhere + * @var bool + */ + protected $redirectMissingRecords = false; + /** * @var callable With two parameters: $form and $component */ @@ -113,6 +123,17 @@ class GridFieldDetailForm implements GridField_URLHandler $requestHandler = $gridField->getForm()->getController(); $record = $this->getRecordFromRequest($gridField, $request); if (!$record) { + // Look for the record elsewhere in the CMS + $redirectDest = $this->getLostRecordRedirection($gridField, $request); + // Don't allow infinite redirections + if ($redirectDest) { + // Mark the remainder of the URL as parsed to trigger an immediate redirection + while (!$request->allParsed()) { + $request->shift(); + } + return (new HTTPResponse())->redirect($redirectDest); + } + return $requestHandler->httpError(404, 'That record was not found'); } $handler = $this->getItemRequestHandler($gridField, $record, $requestHandler); @@ -148,6 +169,50 @@ class GridFieldDetailForm implements GridField_URLHandler return $record; } + /** + * Try and find another URL at which the given record can be edited. + * If redirectMissingRecords is true and the record has a CMSEditLink method, that value will be returned. + * This only works when the list passed to the GridField is a {@link DataList}. + * + * @param $gridField The current GridField + * @param $id The ID of the DataObject to open + */ + public function getLostRecordRedirection(GridField $gridField, HTTPRequest $request, ?int $id = null): ?string + { + + if (!$this->redirectMissingRecords) { + return null; + } + + // If not supplied, look up the ID from the request + if ($id === null && is_numeric($request->param('ID'))) { + $id = (int)$request->param('ID'); + } + + if (!$id) { + return null; + } + + $list = $gridField->getList(); + if (!$list instanceof DataList) { + throw new \LogicException('List is not of type DataList, cannot determine redirection target'); + } + + $existing = DataObject::get($list->dataClass())->byID($id); + if ($existing && $existing->hasMethod('CMSEditLink')) { + $link = $existing->CMSEditLink(); + } + + if ($link && $link == $request->getURL()) { + throw new \LogicException(sprintf( + 'Infinite redirection to "%s" detected in GridFieldDetailForm->getLostRecordRedirection()', + $link + )); + } + + return $link; + } + /** * Build a request handler for the given record * @@ -209,6 +274,28 @@ class GridFieldDetailForm implements GridField_URLHandler return $this->name; } + /** + * Enable redirection to missing records. + * + * If a GridField shows a filtered list, and the DataObject is not in the list but exists in the + * database, and the DataObject has a CMSEditLink method, then the system will redirect to the + * URL returned by that method. + */ + public function setRedirectMissingRecords(bool $redirectMissingRecords): self + { + $this->redirectMissingRecords = $redirectMissingRecords; + return $this; + } + + /** + * Return the status of redirection to missing records. + * @see setRedirectMissingRecordssetRedirectMissingRecords + */ + public function getRedirectMissingRecords(): bool + { + return $this->redirectMissingRecords; + } + /** * @return bool */ diff --git a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php index 9db76aac0..ee04715d6 100644 --- a/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php +++ b/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php @@ -623,6 +623,11 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler // to the same URL (it assumes that its content is already current, and doesn't reload) return $this->edit($controller->getRequest()); } else { + // We might be able to redirect to open the record in a different view + if ($redirectDest = $this->component->getLostRecordRedirection($this->gridField, $controller->getRequest(), $this->record->ID)) { + return $controller->redirect($redirectDest, 302); + } + // Changes to the record properties might've excluded the record from // a filtered list, so return back to the main view if it can't be found $url = $controller->getRequest()->getURL(); diff --git a/tests/php/Forms/GridField/GridFieldDetailFormTest.php b/tests/php/Forms/GridField/GridFieldDetailFormTest.php index 5cab230ba..d8b551999 100644 --- a/tests/php/Forms/GridField/GridFieldDetailFormTest.php +++ b/tests/php/Forms/GridField/GridFieldDetailFormTest.php @@ -207,7 +207,7 @@ class GridFieldDetailFormTest extends FunctionalTest ); $this->assertFalse($response->isError()); - $person = Person::get()->sort('FirstName')->First(); + $person = $this->objFromFixture(Person::class, 'jane'); $favouriteGroup = $person->FavouriteGroups()->first(); $this->assertInstanceOf(PeopleGroup::class, $favouriteGroup); @@ -248,7 +248,7 @@ class GridFieldDetailFormTest extends FunctionalTest ] ); $this->assertFalse($response->isError()); - $person = Person::get()->sort('FirstName')->First(); + $person = $this->objFromFixture(Person::class, 'jane'); $category = $person->Categories()->filter(['Name' => 'Updated Category'])->First(); $this->assertEquals( [ @@ -271,7 +271,7 @@ class GridFieldDetailFormTest extends FunctionalTest ); $this->assertFalse($response->isError()); - $person = Person::get()->sort('FirstName')->First(); + $person = $this->objFromFixture(Person::class, 'jane'); $category = $person->Categories()->filter(['Name' => 'Updated Category'])->First(); $this->assertEquals( [ @@ -405,4 +405,34 @@ class GridFieldDetailFormTest extends FunctionalTest $this->assertEquals($group->Name, (string) $title[0]); $this->assertEquals($group->ID, (string) $id[0]['value']); } + + public function testRedirectMissingRecords() + { + $origAutoFollow = $this->autoFollowRedirection; + $this->autoFollowRedirection = false; + + // GridField is filtered people in "My Group", which does't include "jack" + $included = $this->objFromFixture(Person::class, 'joe'); + $excluded = $this->objFromFixture(Person::class, 'jack'); + + $response = $this->get(sprintf( + 'GridFieldDetailFormTest_Controller/Form/field/testfield/item/%d/edit', + $included->ID + )); + $this->assertFalse( + $response->isRedirect(), + 'Existing records are not redirected' + ); + + $response = $this->get(sprintf( + 'GridFieldDetailFormTest_Controller/Form/field/testfield/item/%d/edit', + $excluded->ID + )); + $this->assertTrue( + $response->isRedirect(), + 'Non-existing records are redirected' + ); + + $this->autoFollowRedirection = $origAutoFollow; + } } diff --git a/tests/php/Forms/GridField/GridFieldDetailFormTest.yml b/tests/php/Forms/GridField/GridFieldDetailFormTest.yml index db1df984d..2182cc031 100644 --- a/tests/php/Forms/GridField/GridFieldDetailFormTest.yml +++ b/tests/php/Forms/GridField/GridFieldDetailFormTest.yml @@ -5,6 +5,9 @@ SilverStripe\Forms\Tests\GridField\GridFieldDetailFormTest\Person: jane: FirstName: Jane Surname: Doe + jack: + FirstName: Jack + Surname: Doe SilverStripe\Forms\Tests\GridField\GridFieldDetailFormTest\PeopleGroup: group: diff --git a/tests/php/Forms/GridField/GridFieldDetailFormTest/CategoryController.php b/tests/php/Forms/GridField/GridFieldDetailFormTest/CategoryController.php index 16563a9b1..e027704e0 100644 --- a/tests/php/Forms/GridField/GridFieldDetailFormTest/CategoryController.php +++ b/tests/php/Forms/GridField/GridFieldDetailFormTest/CategoryController.php @@ -31,7 +31,7 @@ class CategoryController extends Controller implements TestOnly public function Form() { // GridField lists categories for a specific person - $person = Person::get()->sort('FirstName')->First(); + $person = Person::get()->filter('FirstName', 'Jane')->First(); $detailFields = singleton(Category::class)->getCMSFields(); $detailFields->addFieldsToTab( 'Root.Main', diff --git a/tests/php/Forms/GridField/GridFieldDetailFormTest/Person.php b/tests/php/Forms/GridField/GridFieldDetailFormTest/Person.php index 1e6659869..0caf52d1c 100644 --- a/tests/php/Forms/GridField/GridFieldDetailFormTest/Person.php +++ b/tests/php/Forms/GridField/GridFieldDetailFormTest/Person.php @@ -69,4 +69,9 @@ class Person extends DataObject implements TestOnly ] ); } + + public function CMSEditLink() + { + return sprintf('my-admin/%d', $this->ID); + } } diff --git a/tests/php/Forms/GridField/GridFieldDetailFormTest/TestController.php b/tests/php/Forms/GridField/GridFieldDetailFormTest/TestController.php index d4204ed8a..1f81cfebe 100644 --- a/tests/php/Forms/GridField/GridFieldDetailFormTest/TestController.php +++ b/tests/php/Forms/GridField/GridFieldDetailFormTest/TestController.php @@ -48,7 +48,9 @@ class TestController extends Controller implements TestOnly $field->getConfig()->addComponent(new GridFieldViewButton()); $field->getConfig()->addComponent(new GridFieldEditButton()); /** @skipUpgrade */ - $field->getConfig()->addComponent($gridFieldForm = new GridFieldDetailForm($this, 'Form')); + $gridFieldForm = new GridFieldDetailForm($this, 'Form'); + $gridFieldForm->setRedirectMissingRecords(true); + $field->getConfig()->addComponent($gridFieldForm); $field->getConfig()->addComponent(new GridFieldEditButton()); /** @skipUpgrade */ return new Form($this, 'Form', new FieldList($field), new FieldList());