PrestaShop / PrestaShop

PrestaShop is the universal open-source software platform to build your e-commerce solution.
https://www.prestashop-project.org/
Other
8.11k stars 4.79k forks source link

[SF Migration] Uniformize all Toggle Status action in Symfony pages #17407

Open matks opened 4 years ago

matks commented 4 years ago

A large number of ObjectModels can be toggled (= toggle the status) from the Grid page.

To implement it, we use Toggle Status Commands however

Some of the handlers that do the job set the status to expected new value, some use native ObjectModel::toggleStatus().

We should always use the same implementation.

tdavidsonas88 commented 4 years ago

If we use objectmodel inside handlers then there is no need to pass the status to the command and handler. For the true/false statuses. Wondering which implementation could be chosen. :thinking:

matks commented 4 years ago

If we use objectmodel inside handlers then there is no need to pass the status to the command and handler. For the true/false statuses. Wondering which implementation could be chosen. 🤔

The best solution is to be as explicit as possible:

Each Command should expect both the ID and the new status ('enabled' or 'disabled'). This ensures that if you fire 5 times the same Command in a row, you will obtain the same result and is an important rule for CQRS. Commands are stateless.

The "new status" should be provided by the HTTP request. We should not tell controller "eyh, toggle this status !" but rather "please set object X status to enabled". The same benefit would be provided: firing the same POST request multiple times would obtain the same result. This makes the app behavior more stateless and consequently easier to manage.

On Handler side, we can do whatever we want however since Command is going to carry the "new status" property, toggleStatus() will not be usable.

So if you follow my idea, the feature will keep the name "toggle status" ... but actually it will rather become a "set status to X" feature.

tdavidsonas88 commented 4 years ago

So it might look like Raimondas implemented it:

    /**
     * Toggles legacy state status
     *
     * @param State $state
     * @param bool $newStatus
     *
     * @return bool
     *
     * @throws StateException
     */
    protected function toggleStateStatus(State $state, bool $newStatus): bool
    {
        $state->active = $newStatus;

        try {
            return $state->save();
        } catch (PrestaShopException $e) {
            throw new StateException(sprintf('An error occurred when updating state status with id "%s"', $state->id));
        }
    }
matks commented 4 years ago

This handler is what we want indeed 👍