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

Incoherent batch behavior when performing batch all_elements and specifying id's at the same time #8168

Closed
pietaj opened this issue Mar 18, 2024 · 2 comments · Fixed by #8177

Comments

@pietaj
Copy link
Contributor

pietaj commented Mar 18, 2024

Subject

When performing a batch action with a confirmation page, there is an inconsistency what the user will get in the confirm vs what sonata will really do.

Steps to reproduce

Take any admin list with a couple of records, select one of them, then select "all records" checkbox by the batch actions and click any batch action with a confirmation page (batch delete will do). The confirmation page will show 'you are about to delete all elements. Proceed?'

This is not surprising, as the template admin-bundle/src/Resources/views/CRUD/batch_confirmation.html.twig states:

<div class="box-body">
                {% if data.all_elements %}
                    {{ 'message_batch_all_confirmation'|trans({}, 'SonataAdminBundle') }}
                {% else %}
                    {% trans with {'%count%': data.idx|length} from 'SonataAdminBundle' %}message_batch_confirmation{% endtrans %}
                {% endif %}
            </div>

so check if the 'all elements' checkbox is checked, if yes - message_batch_all_confirmation

but in the controller admin-bundle/src/Controller/CRUDController.php

we have

    public function batchAction(Request $request): Response
    ...
  
        if (\count($idx) > 0) {
            $this->admin->getModelManager()->addIdentifiersToQuery($this->admin->getClass(), $query, $idx);
        } elseif (!$allElements) {
            $this->addFlash(
                'sonata_flash_info',
                $this->trans('flash_batch_no_elements_processed', [], 'SonataAdminBundle')
            );

            return $this->redirectToList();
        }

        return \call_user_func($batchActionExecutable, $query, $forwardedRequest);
    }

so the controller adds the identifiers even if all elements is selected, and this implies that a sequence of AND and OR statements is added to the query.

Expected results

I would expect, that sonata will delete all elements, as the confirmation page stated that it will do so

Actual results

Sonata will delete only the one selected element

The question is - should Sonata prioritize all elements and the controller should be corrected, or should idx be prioritized and the confirmation page should be addjusted

I'm happy to help with the fix, but a decision needs to be made

@VincentLanglet
Copy link
Member

Take any admin list with a couple of records, select one of them, then select "all records" checkbox by the batch actions and click any batch action with a confirmation page (batch delete will do). The confirmation page will show 'you are about to delete all elements. Proceed?'

I consider it should delete all elements then, so controller should be corrected.

Do you mind a PR @pietaj ?

@pietaj
Copy link
Contributor Author

pietaj commented Apr 11, 2024

Hello @VincentLanglet. Thanks for the feedback. I'll do the PR, no problem

pietaj pushed a commit to pietaj/SonataAdminBundle that referenced this issue Apr 23, 2024
This commit fixes inconsistent behavior observed when both
'all_elements' and individual list rows are selected
simultaneously before executing a batch action. The resolved
behavior is detailed in sonata-project#8168.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants