Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks
GuySartorelli opened this issue Sep 15, 2022 · 4 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Sep 15, 2022

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.

@michalkleiner
Copy link
Contributor

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.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Sep 16, 2022

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
Copy link
Member Author

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.

@michalkleiner
Copy link
Contributor

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.

@GuySartorelli GuySartorelli changed the title Adding custom importers in ModelAdmin.model_importers requires adding it twice CMS 6: Adding custom importers in ModelAdmin.model_importers requires adding it twice May 9, 2023
@GuySartorelli GuySartorelli added this to the Silverstripe CMS 6 milestone Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants