silverstripe-admin icon indicating copy to clipboard operation
silverstripe-admin copied to clipboard

CMS 6: Adding custom importers in ModelAdmin.model_importers requires adding it twice

Open GuySartorelli opened this issue 3 years ago • 4 comments

If your ModelAdmin uses a custom slug (as per the below example) instead of relying on the class name, any model_importers config for that model needs to be added twice.

    private static $managed_models = [
        'users' => [
            'title' => 'Users',
            'dataClass' => Member::class
        ],
    ];

    private static $model_importers = [
        // Add once for the model tab (slug) and once for the model class
        'users' => MemberCsvBulkLoader::class,
        Member::class => MemberCsvBulkLoader::class,
    ];

ACs

  • [ ] You only need to define EITHER modelClass (e.g. Member::class) OR modelTab (e.g. 'users')
  • [ ] It's possible to have the SAME modelClass on different tabs use different $model_importers - see example below

Example below

  • 'Backend people' tab using Member::class modelClass AND BackendPeopleBulkLoader::class
  • 'Frontend people' tab using Member::class modelClass AND FronendPeopleBulkLoader::class

Notes

  • ModelClass is the "blanket" / "fallback" - ModelTab is the optional specific thing that can be specified

Background

We have to add both the model tab reference and the class name as keys for the importers because ModelAdmin currently checks for $importers[$modelClass] in some places and $importers[$this->modelTab] in others. It should always use either the tab or the class - whichever we decide is the more appropriate canonical reference.

If it uses the class, we have to document the fact that the same importer must be used for all tabs with that class, which is a limitation we perhaps don't want to impose.

If it uses the tab name, we have to document that fact so that it's abundantly clear. This would be my preferred resolution as it allows developers to import models of the same class in different ways depending on what tab they're importing from.

GuySartorelli avatar Sep 15 '22 22:09 GuySartorelli

This is all complicated by the fact that we allow managed models to be defined in several ways — a simple data class name, data class name -> [title], tab name -> [data class name, title], and possibly others.

I agree we should keep support for managing the same model class in multiple tabs as the list of items can be filtered differently in each tab for the same model.

My question/thinking is whether there's a use case to have a different importer for the same model class just because it can be viewed differently in other model admin tabs. I'd say the importer is closer to the class than a view of it, so regardless of the managed models config I would keep the model importer tied to its model class.

michalkleiner avatar Sep 16 '22 00:09 michalkleiner

In 99% of cases I agree with you that the importer implementation should be identical for a class regardless of the tab. But there's that 1% of crazy scientists who will want to, say, have different default values if you're viewing in tab A rather than tab B.

For example, say I have a DataObject called Category. I have two tabs in my ModelAdmin - "product-category" and "client-category". If I am importing Category data from the "product-category" tab I want to have my Type column set to "product", and if I am importing from the "client-category" tab I want it set to "client".

There are arguments to be made about whether that's the "correct" approach etc but ultimately I don't think it's the product's decision whether an approach like that is "correct" - and I don't see any downside to allowing it.

Perhaps a reasonable solution is: If you use the model class as the key in model_importers, it is used across all tabs that manage that class. If you use the model tab as the key, it is exclusively used for that tab.

GuySartorelli avatar Sep 16 '22 01:09 GuySartorelli

But ultimately if it will be too complex to support custom importers for a specific tab, maybe we just make it always the class name so that we can fix this bug.

GuySartorelli avatar Sep 16 '22 01:09 GuySartorelli

I would argue that if a developer is skilled enough to do different tab views for the same model with custom filters, they would be able to support different import logic in some other way, e.g. through a custom import button, subclassing of the model etc. if the support for enabling both tab and model name keys in the importer config proves too difficult/complex to implement.

michalkleiner avatar Sep 16 '22 01:09 michalkleiner