NEW Nested gridfield

[forsdahl]

Description

Provides a GridFieldComponent for displaying a nested GridField, e.g. for has_many relations of the main record.

Manual testing steps

Can be tested in a standard Silverstripe 5.1 installation in SecurityAdmin for editing groups, by adding the following extension to the project and adding it to SecurityAdmin via yaml config:

<?php

namespace Creamarketing\NestedGridField;

use SilverStripe\Core\Extension;
use SilverStripe\Security\Group;
use Symbiote\GridFieldExtensions\GridFieldNestedForm;

class SecurityAdminExtension extends Extension
{
    public function updateGridFieldConfig($config)
    {
        if ($this->owner->getModelClass() === Group::class) {
            // shows nested groups
            $config->addComponent(GridFieldNestedForm::create());
            // Shows members in groups
            // $config->addComponent(GridFieldNestedForm::create()->setRelationName('Members'));
        }
    }
}

SilverStripe\Admin\SecurityAdmin:
  extensions:
    - Creamarketing\NestedGridField\SecurityAdminExtension

Issues

• Nested gridfield #383

Pull request checklist

• The target branch is correct
  • See picking the right version
• All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
  • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
• The commit messages follow our commit message guidelines
• The PR follows our contribution guidelines
• Code changes follow our coding conventions
• This change is covered with tests (or tests aren't necessary for this change)
• Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
• CI is green

[GuySartorelli]

Thanks for this!

I've added the pull request template to the PR description (did you remove that, or was it just not there when you started the PR?) and filled in some of it for you.
Can you please look at the rest of the checklist items and validate whether your PR meets those requirements? If it does, check the box. If it doesn't, update the PR until it does meet them.

I haven't looked at the code itself yet but I've added this to our review column.

[GuySartorelli]

I haven't done a comprehensive review of this, but here are some changes or questions I have so far:

• I think breadcrumbs in the GridField should include the parent record
  • In this case Groups > Content Authors > Smith
    
• Search/filter functionality in nested gridfield doesn't seem to work
• Nested GridField state is not retained - e.g. if I apply a sort to the nested gridfield, click to edit a record, and then click back - the sort order is back to its default state.
• Seems to be no way to edit the nested GridField except through extensions
  • The acceptance criterion "the configuration for the nested GridField can be customised by a developer if needed" it a bit vague but IMO it is important to be able to customise it for each GridField independently, not just as a global extension
• There's an acceptance criterion in the issue about an upper limit for recursion, but I can't find where that's implemented - has that been implemented?
• "When a record doesn't have any nested relations, nested gridfield can be configured not to show."
  • Has this been implemented? If so please update the documentation to mention this.
  • Similarly if there are other options that haven't been documented, please document them so that I can test them and other developers can make use of them.
• The approach of subclassing GridFieldDetailForm seems a bit unusual, it's definitely not what I was expecting. Can you please briefly explain the reasons behind that decision?
• Please update the component list in the README.

I have been testing this by adding a nested gridfield to groups in SecurityAdmin with this code:

<?php

namespace App\NestedExtensions;

use SilverStripe\Core\Extension;
use SilverStripe\Forms\GridField\GridFieldConfig;
use Symbiote\GridFieldExtensions\GridFieldNestedForm;

class NestedGroups extends Extension
{
    public function updateGridFieldConfig(GridFieldConfig $config)
    {
        $config->addComponent(GridFieldNestedForm::create()->setRelationName('Members'));
    }
}

SilverStripe\Admin\SecurityAdmin:
  extensions:
    - App\NestedExtensions\NestedGroups

[forsdahl]

I now changed how Breadcrumbs work in nested GridFields. It now shows all the parent records, but the back link goes to the top-level instead of the parent record, since having to click back through all parent records to get back to the place you where is a bit confusing.

The reason GridFieldNestedForm extended GridFieldDetailForm are some legacy stuff, which isn't really true anymore in Silverstripe 5, so I now changed the base class to AbstractGridFieldComponent instead. GridFieldNestedFormItemRequest still extends GridFieldDetailForm_ItemRequest though, since those two actually share a lot of functionality.

The reason the search component doesn't work is probably due to the fact that the name of the nested GridFields contain "[" and "]" characters. The reason for this is so that GridFieldEditableColumns will work in a non-hacky way for nested GridFields. I'll see if I can come up with a solution that works for both components, or if I can find a fix for the search component working event with "["-characters in the GridField names.

I will implement some more UnitTests still, and look at the missing things for the acceptance criterias.

[forsdahl]

I now changed the naming schema of the nested GridFields, to not include any "[" or "]" characters, since I got this schema to also work with GridFieldEditableColumns. This naming schema is safer, there might be other components also which do not like the brackets in the name.
GridFieldOrderableRows now also works with nested GridFields, both with immediate update, or without. For records with the Hierarchy extension, items can also be dragged between GridFields to move them. This can be tested with the following extension applied to SecurityAdmin:

<?php

namespace App\Extensions;

use SilverStripe\Core\Extension;
use SilverStripe\Security\Group;
use Symbiote\GridFieldExtensions\GridFieldNestedForm;
use Symbiote\GridFieldExtensions\GridFieldOrderableRows;

class SecurityAdminExtension extends Extension
{
    public function updateGridFieldConfig($config)
    {
        if ($this->owner->getModelClass() == Group::class) {
            $config->addComponent($nestedForm = new GridFieldNestedForm());
            $config->addComponent($orderableRows = new GridFieldOrderableRows());
            $orderableRows->setImmediateUpdate(true);
        }
    }
}

Also fixed some linting issues, so CI should now hopefully be green.

[GuySartorelli]

I haven't done a full thorough code review but I have taken a look and highlighted some areas that don't meet our coding standards or which seemed like they could be improved.

I've also done another in-browser test and these are the problems I encountered:

• There's some unexpected state conflict between the nested gridfield and the main gridfield. Steps to reproduce:
  1. Have at least two items in the main gridfield, both with nested gridfields
  2. expand both nested gridfields
  3. Filter the main gridfield (e.g. search for "author") so that there's only one item in the main gridfield.
  4. Filter the nested gridfield.
  5. Click on a record in the nested gridfield, then click the back arrow in the breadcrumbs
  6. Remove the filter on the main gridfield
  7. Click on a record in the nested gridfield, then click the back arrow in the breadcrumbs
  8. Notice that the filter (which you cleared just before) is back again.
• Calling setMaxNestingLevel(3) on the nested gridfield component doesn't seem to work - though changing the configuration does.
  • My guess is we need to pass the nesting level down recursively to all child nested gridfield components. We should also disallow developers setting the value in nested components directly, so we don't get a parent with a max nesting level of 3 and a child with a max nesting level of 10 (as a random example).
  • I would have expected the nested gridfield settings to be inherited automatically, since this was tested with a Hierarchy object (using the example in the PR description). It looks like the reason this didn't happen is because the nested gridfield in this case is set up in GridFieldNestedFormItemRequest as a fresh instance of GridFieldNestedForm() which doesn't look at the settings defined in getCMSFields() at all.
• When trying to use sort to move items between nested gridfields, I moved a child group to be a sibling to what was its parent group. This resulted in a popup that said "An error occured while fetching data from the server Please try again later." - and an error in the network tab of the developer console which said ERROR [UNKNOWN TYPE, ERRNO 400]. The full error with stack trace was:

  ERROR [UNKNOWN TYPE, ERRNO 400]: 
  IN POST /admin/security/groups/EditForm/field/groups/nested/1/ItemEditForm/field/groups-GridFieldNestedForm-1/nested/3/ItemEditForm/field/groups-GridFieldNestedForm-1-GridFieldNestedForm-3/nested/5/ItemEditForm/field/groups-GridFieldNestedForm-1-GridFieldNestedForm-3-GridFieldNestedForm-5/reorder?gridState-groups-0=%7B%22GridFieldOrderableRows%22%3A%7B%22enabled%22%3Atrue%7D%2C%22GridFieldNestedForm%22%3A%7B%22SilverStripe-Security-Group-2-Children%22%3Anull%2C%22SilverStripe-Security-Group-1-Children%22%3A1%7D%7D&gridState-groups-GridFieldNestedForm-1-1=%7B%22GridFieldOrderableRows%22%3A%7B%22enabled%22%3Atrue%7D%2C%22GridFieldNestedForm%22%3A%7B%22SilverStripe-Security-Group-3-Children%22%3A1%7D%7D&gridState-groups-GridFieldNestedForm-1-GridFieldNestedForm-3-2=%7B%22GridFieldOrderableRows%22%3A%7B%22enabled%22%3Atrue%7D%2C%22GridFieldNestedForm%22%3A%7B%22SilverStripe-Security-Group-4-Children%22%3Anull%2C%22SilverStripe-Security-Group-5-Children%22%3A1%7D%7D
  Line  in 
  
  Trace
  =====
  SilverStripe\Logging\DetailedErrorFormatter->output(400, , , , )
  DetailedErrorFormatter.php:55
  
  SilverStripe\Logging\DetailedErrorFormatter->format(Array)
  HTTPResponse.php:417
  
  SilverStripe\Control\HTTPResponse->outputBody()
  HTTPResponse.php:346
  
  SilverStripe\Control\HTTPResponse->output()
  index.php:25
  

  • It's worth noting that the group did move to where it was meant to move to, despite the error.

Additionally, please do these actions I previously requested in #384 (review)

• Please add additional documentation so I know what needs to be manually tested and so that developers know what functionality this thing has.
• Please update the component list in the readme

[GuySartorelli]

Thank you for your patience and diligent effort on this so far, we're very close now.

Please avoid marking comments as "resolved" - I use them to know what changes I requested last time (and have verified changes were made) vs which changes have already been dealt with in previous reviews.

There's also a minor PHP linting issue to resolve.

[GuySartorelli]

In addition to the above, the following from #384 (review) haven't been resolved:

• There's some unexpected state conflict between the nested gridfield and the main gridfield (steps to reproduce in the original comment)
• Please add additional documentation so I know what needs to be manually tested and so that developers know what functionality this thing has.
• Please update the component list in the readme

[GuySartorelli]

I've discovered that if you try to add two GridFieldNestedForm components, it doesn't work as expected (e.g. uncomment the second component in the PR description).
Probably we should just throw an exception if trying to add a GridFieldNestedForm when there's already one there. We can look at having multiple nested sibling gridfields working nicely together as a follow-up, if there's appetite for that.

[forsdahl]

I looked in to the state handling issues you described, and it does seem to me to be the default state handling logic which isn't really up to handling this. The GridFieldNestedForm component doesn't do any custom logic for the state handling, neither in php nor in javascript, it relies on the gridfield state manager to add the correct state parameters for all links.
I can reproduce the scenario which you describe, and I can see that when having both a base grid field and a nested grid field with the GridFieldFilterHeader component, the state-part of the url is not updated properly when filtering.
I think the solution here is to update the general gridfield state handling to support this, with the GridFieldStateManager implementation we have which stores individual gridfield states in session, and only adds a state key to the url.
In the mean time, I think we could at least disable the GridFieldFilterHeader in the default nested gridfield config. In all the cases we have used nested gridfields, we have yet to see a real-world use case where we really feel the filtering in the nested gridfield to be really necessary and I suspect the state handling works ok if only the base gridfield has the GridFieldFilterHeader-component.

[GuySartorelli]

There are still outstanding items I've requested above so I'll assume you're still working on this.
I've let CI run so you can see if that's green in the meantime.

[GuySartorelli]

To find it easier to find the requested changes that haven't been made yet:

• NEW Nested gridfield #384 (comment) regarding documentation and the readme
• NEW Nested gridfield #384 (comment)

[forsdahl]

Documentation and readme now updated.
GridFieldNestedForm now also throws an exception if it detects more than one instance of itself on the same GridField.

[GuySartorelli]

LGTM, nothing stands out as being obvious wrong in the code and it works great locally.
Thanks for your efforts on this, it's a really fantastic contribution!

This feature will be included in the October minor release.