Upload field doesn't work in the asset admin context

[mfendeksilverstripe]

Module version(s) affected

2.1.x-dev

Description

I'm trying to set up a simple has_one relation between files. The data setup part works as expected but when I use UploadField the UI goes through some unexpected interactions (details below) and the selected asset is not saved. This doesn't happen when a different field is used, for example a DropdownField.

Tested this on vanilla install using "silverstripe/recipe-cms": "5.x-dev",

How to reproduce

Data setup

Model

<?php

use SilverStripe\ORM\DataExtension;
use SilverStripe\Assets\File;

/**
 * @property int $ReplacementAssetID
 * @method File ReplacementAsset()
 * @method File|ReplacementAssetDataExtension|$this getOwner()
 */
class ReplacementAssetDataExtension extends DataExtension
{
    private static array $has_one = [
        'ReplacementAsset' => File::class,
    ];
}

Form

<?php

use SilverStripe\AssetAdmin\Forms\AssetFormFactory;
use SilverStripe\AssetAdmin\Forms\UploadField;
use SilverStripe\Assets\File;
use SilverStripe\Control\RequestHandler;
use SilverStripe\Core\Extension;
use SilverStripe\Forms\DropdownField;
use SilverStripe\Forms\FieldList;
use SilverStripe\Forms\ReadonlyField;

/**
 * @method AssetFormFactory|$this getOwner()
 */
class ReplacementAssetFormExtension extends Extension
{
    /**
     * Extension point in @see AssetFormFactory::getFormFields()
     *
     * @param FieldList $fields
     * @param RequestHandler $controller
     * @param string|null $formName
     * @param array|null $context
     * @return void
     */
    public function updateFormFields(
        FieldList $fields,
        RequestHandler $controller,
        ?string $formName,
        ?array $context
    ): void {
        $file = $context['Record'] ?? null;

        if (!$file) {
            // Nothing to do if there's no file
            return;
        }

        if (!$file instanceof File) {
            return;
        }

        $sourceFiles = File::get()->map('ID', 'Title');
        $fields->removeByName([
            'ReplacementAsset',
        ]);

        $fields->addFieldsToTab(
            'Editor.Details',
            [
                ReadOnlyField::create('CurrentReplacementAssetID', $file->ReplacementAssetID),
//                DropdownField::create('ReplacementAsset', 'Replacement Asset', $sourceFiles)
//                    ->setHasEmptyDefault(true), // <--- This works (allows selection of asset and stores the asset ID)
                UploadField::create('ReplacementAsset', 'Replacement Asset')
                    ->setUploadEnabled(false) // <---- This doesn't work (UI goes through unexpected interactions and doesn't store the asset ID)
                ],
            'Name'
        );
    }
}

Config

SilverStripe\Assets\File:
  extensions:
    - ReplacementAssetDataExtension

SilverStripe\AssetAdmin\Forms\AssetFormFactory:
  extensions:
    - ReplacementAssetFormExtension

Test scenario:

• upload two assets (for example image A and image B)

• navigate to the image B details form

• we're aiming to use the upload field to select image A and store its ID against the image B record

Expected behaviour

I can see asset selection modal so I can choose Image A

Clicking insert will put the Image A into the Image B records via the ReplacementAsset relation. I then click save and the record gets updated.

Actual behaviour

The screen transitions into a read only version of the Image B edit form.

I can still interact with the upload field.

Clicking on the upload field will trigger the modal and I can select Image A.

Clicking insert button will navigate back to the edit version of the Image B edit form but the asset selection is lost so no changes are saved to the DDB record of Asset B.

Possible Solution

Managed to create workaround by setting up a separate model admin to allow the use of non-react edit form where the upload field works as expected.

Acceptance criteria

• Upload Field works when rendered in an Asset-Admin area forms.
• Upload Field works when rendered in an InsertMediaModal area forms.

Notes

• This maybe a very complex problem that can not be fixed in a minor. When trying to identify a fix, consider coordinating with colleague and validating whether this should be in 5.3 or 6.
• Try it for 3 days and if there's no obvious way forward, come back to the team.

[emteknetnz]

Have you tried this on CMS 5.2? There's a chance that https://github.com/silverstripe/silverstripe-admin/pull/1694 will fix this - would require you changing to silverstripe/admin 2.2 to get the fix

[mfendeksilverstripe]

Hey @emteknetnz , I've retested this issue on "silverstripe/recipe-cms": "5.2.x-dev", which pulls in silverstripe/admin 2.2.x-dev 6925ecb.

The issue is still present.

[maxime-rainville]

I've added same ACs to the cards. FYI I suspect this will not be a trivial fix.

And we have to ask ourselves how to handle the recursive scenario where you try to edit a file inside an InserMediaModal and interact with a nested UploadField. It's highly likely this particular scenario will blow up in our face. So it might be that we say "no nested UploadField".

Anyway, I've added this to our Monday refinement session. Maybe we need to do a Spike first before attempting a fix. Not sure.

[GuySartorelli]

This is happening because the UploadField renders InsertMediaModal which relies on the AssetAdmin component.

The AssetAdmin component, as its name suggests, is also the main component used for the asset admin section.

All of the redux state and actions are shared between the AssetAdmin component (and its children) in the asset admin itself, and the AssetAdmin component rendered in the upload field's modal. So when you select a file in the modal, it selects it in the main admin, for example.

To resolve this we'd need to split out the state for the upload field from the state for the main asset admin. I can't see an easy non-hacky way to do that.

Possible ways forward

1. Hackily create a new react root for the upload field's modal so it is a fresh instance of react/redux/etc and therefore can't share anything with the main asset admin, or
2. register a separate namespaced set of redux reducers for the upload field, separate from the current assetAdmin namespaced reducers
   • We'd need to pass the appropriate namespace through AssetAdmin and all its children, telling them which namespace to use for actions and state.
   • We'd need to update mapDispatchToProps() to map the correct actions (may have to map both sets, not sure if we have the real props at this stage)
   • Since we'd still be sharing a redux store we probably also have to alter the actions themselves so the type of the action is appropriately namespaced
3. ??? Maybe someone with more react/redux knowledge has a simple solution I'm not aware of?

[emteknetnz]

1. Hackily create a new react root for the upload field's modal so it is a fresh instance of react/redux/etc

I'm not sure that would even work? Doesn't the redux store live in window.ss.store and it's not 'namespaced' to the react root? i.e. if you create 2x react roots they'll share the store? I'm only making assumptions here, I haven't tested.

e.g. window.ss.store.getState()['assetAdmin'] returns an object with some things in it but it's not namespaced

Maybe we could add an index "namespace" to 'assetAdmin'? to handle multiple versions of it?

This whole seems very hard and messy

[GuySartorelli]

Dunno, haven't investigated it, it was just a possible approach that might maybe possible work. Definitely wouldn't be my go-to. I'd probably prefer just not doing anything over doing that even if it works.

[emteknetnz]

I'd probably prefer just not doing anything

Me too, maybe put a disclaimer somewhere in the docs that using an UploadField in AssetAdmin is known to be buggy?

[maxime-rainville]

I don't think there's any way we can fix this without a substantial re-factor of asset-admin. I'm thinking we bump this up to CMS 6.

My instinct would be to:

• Convert AssetAdmin to use functional components
• Switch from redux to useState hooks to track state. That way multiple AssetAdmin instance could track their own state. This would probably imply a bit more work to pass relevant state to child components.

Most of that stuff should probably be done anyway to have a more solid architecture. Fixing this bug would just be the cherry on top.

[maxime-rainville]

I've re-targeted this issue to the CMS 6 milestone. Our ability to address this issue will be contingent on whether we choose to prioritise an asset-admin refactor.