atk4 / data

Data Access PHP Framework for SQL & high-latency databases
https://atk4-data.readthedocs.io
MIT License
273 stars 46 forks source link

Allow to get target model after deep traversal #1052

Open mvorisek opened 2 years ago

mvorisek commented 2 years ago
class Order extends \Atk4\Data\Model {
 protected function init(): void
    {  

        parent::init();
        $this->hasMany('order_doc', ['model' => [OrderDoc::class
        ]]);

    }
}

class OrderDoc extends \Atk4\Data\Model { 

    protected function init(): void
    { 
        parent::init();

        $this->hasOne('order_id', ['model' => [Order::class]]);

        $this->onHook(\Atk4\Data\Model::HOOK_BEFORE_INSERT, function ($model, &$data) {

                // Create order if not existing
                if (!$model->get('order_id') && $model->get('customer_id')) {

                    $model->set('order_id', $model->refLink('order_id')->insert([...]);
                    ]));   }

        });
    }
}

// Use case 1: Create a parent order container, when a new child order doc is created
$newOrderDoc = (new OrderDoc($db))->createEntity();
$newOrderDoc->save([...]);

// Use case 2: Assign an order document to another order
$order = (new Order($db))->load(1);
$orderdoc = $order->ref('order_doc')->load(1);

$orderdoc->saveAndUnload(['order_id' => 2]);

When traversing on null value, we may allow the ID to change (to newly inserted). I will check what can be done and if it will not break other tests.

use case 2 - how should this work? Previously, the save conditions were relaxed, to me, incorrectly. What is the reasoning to relax save conditions when the entity cannot be loaded after?

A solution I can think of would be to implement Model::getModelWithoutRefConditions(). Currently, we do not store traversed reference info, so in $orderdoc, we have no info which condition to remove. Let's discuss this feature request later. If you have (or can put) your project into Github, I can code some helper methods specific to your projects.

mkrecek234 commented 2 years ago

@mvorisek Just some further food for discussion to make uses cases a bit clearer:

  1. Let's say a user has restricted access to only some customer entities - e.g., he is an account manager for business and not end customers: $customerModel->addCondition('customer_group_id', 1); // may not access any other group

  2. He now filters his customer list showing customers which he flagged as important:

    $customerModel->addCondition('customer_group_id', 1); // may not access any other group`
    $grid->setModel($customerModel)
    $grid->model->addCondition('flagged', true);
  3. In this $grid, the customer decides to remove the flag 'important' for a specific customer - naturally not having scope limitations in mind, you would just do:

    $grid->addActionButton(['icon'=>'flag'], function ($v, $id) use ($grid) {
                    $messagemodel = (clone $grid->model);
                    $entity = $messagemodel->load($id);
                    if ($entity->get('flagged')) {
                        $entity->set('flagged', 0);
                    } else {
                        $entity->set('flagged', 1);
                    }
                    $entity->save();
                    return new \Atk4\Ui\JsReload($grid);              
                });

    This would now fire an Exception, as removing the flag would move it out of the model scope of $messagemodel.

Now here is the idea:

If Atk4/Data knew which conditions might be released on scope validation checks, it could then be more flexible. So in this case only the customer_group condition would be checked to always be valid.

OPTIONAL: Nevertheless, I believe there should be an easy way to also release those critical conditions: Take the use case that the account manager has the right to change a customer to be an end customer: the customers thus should be handled by end customer sales team. $entity->dangerouslySaveWithoutScope(['customer_group', 2) should be allowed, if intentionally decided by the developer.

So concrete proposal:

  1. We add an optional parameter for addCondition called e.g., $weak defaulting to false which - if set to true marks to non-checked conditions on validation checks.
  2. OPTIONAL: We add another function called Model::dangerouslySaveWithoutScope which saves in a "fresh" unconditioned model / without scope.

    Naturally speaking all those saves would mean saveAndUnload as after save the entity is no longer in the model scope.

mvorisek commented 2 years ago

skipping (allowing to skip) whole scope is wrong and should not be part of the framework, so no Model::dangerouslySaveWithoutScope, this is not possible also because there are more save methods, and we would need to implement all methods for such feature

however, your can achive that by creating a new model (and loading it) like you posted in the chat or maybe even easier by:

⚠️ this following example is wrong, entity clone would not clone parent model, thus the model scope is not cloned!

$entity = clone $entity;
$entity->scope()->clear();
$entity->set('xxx', 'value_out_of_original_scope');
$entity->save();

the following should work

$model = clone $entity->getModel();
$model->scope()->clear();
$entity = $model->load($entity->getId());
$entity->set('xxx', 'value_out_of_original_scope');
$entity->save();

I can think of some method like Model::cloneEntityModel() which will clone the current entity (/wo reload), clone parent model and replace the entity model, so scope could be cleared/changed /wo affecting the original model + /wo the need to reload. Example:

$entity = $entity->cloneEntityModel();
$entity->scope()->clear(); // same as $entity->getModel()->scope()->clear()
$entity->set('xxx', 'value_out_of_original_scope');
$entity->save();
mkrecek234 commented 2 years ago

@mvorisek That sounds practical to me and very logical. Any other thoughts from the community @DarkSide666 eventually? What about the idea of weak conditions (so we would not completely clear all conditions, but safely only the ones which are not critical)? So not all scope is cleared?

We should definitely then store those four lines of code above in a separate saveWeakly or saveOutOfScope function.