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

Custom bulk action #176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fedeisas
Copy link

If you have a multi-tenant system and know that the queries will always include a filter condition for a given tenant, you might want to instruct Elasticsearch to group the documents for the same tenant on the same shard.

This PR provides a prepareIndexAction method to add metadata to the Bulk action.

Similar to prepare() I added a new interface JeroenG\Explorer\Application\BeIndexed to hold this new behavior.

@Jeroen-G
Copy link
Owner

What I like: ability to add bulk action settings. What I don't like: separate implementations for each setting.

Given that we just recently added a more general approach to query properties I would like to do the same for these settings. I therefore think a slightly different approach from yours is necessary. @blackshadev do you agree?

@fedeisas in any case, thanks for bringing this idea and PR, it certainly can be a good improvement to Explorer!

@blackshadev
Copy link
Collaborator

blackshadev commented Mar 20, 2023

I agree with @Jeroen-G , I like the new ability. But not the implementation.

Adding multiple if branches with instanceof doesn't seem a natural fit, even for you usecase since you would need to add this to all your models (I think). Moreover I think Explorer will move away from those kind of constructs pretty soon towards a more customizeable fit.

If we have it wrong and it should be implemented with an interface on the model: I'd like to see the interface be called different BeIndexed seems to generic, all scout models BeIndexed. And passing the whole bulk update array to the method to be mutated seems a bit excessive, for your use case, and others. Maybe you do have a usecase for it, which I would like to hear. But otherwise I would like to let prepareIndexAction to only return an array which we merge into the bulkUpdateAction array.

And last but not least, there is one escaped mutation test, which we'd like to see solved in a test.

I hope this doesn't de-motivate you, since we do like to see you and this functionality contributed to Explorer, just not in the way you implemented it. Or we are "missing" some extra information, why this is the best approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants