When deleting and restoring an admin, UserGroups relations are not restored

[xyz1123581321]

Winter CMS Build

1.1

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

I want to prevent User Group deletion when an admin is deleted. The records exists but not the relations. After restoring the admin, groups are not restored.

Tried with changing the core User.php file with:

public $belongsToMany = [
        'groups' => [UserGroup::class, 'table' => 'backend_users_groups', 'softDelete' => true]
];

Doesnt help. I even tried to modify SoftDelete trait and commented out the methods for deleting the relations, doesnt help either.

Steps to replicate

.

Workaround

No response

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days. If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue. If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in the last 60 days. If this issue is still relevant or you would like to see it actioned, please respond and we will re-open this issue. If this issue is critical to your business, consider joining the Premium Support Program where a Service Level Agreement is offered.

[arvislacis]

I can confirm that this issue exists and maybe we also need to check if this problem isn't much bigger - because it looks like when soft deleting main model which has SoftDelete trait (in this case User model and backend_users table) then all related entries in Many To Many table (in this case UserGroup and backend_users_groups) are deleted automatically although if we look at https://wintercms.com/docs/database/relations#detailed-relationships then by default it should be 'delete' => false.

I was also experimenting to manually set delete and/or detach on $belongsToMany in User and UserGroup models (in various variations) but nothing changed this previously described behaviour.

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in this issue for the last 6 months. If this issue is still relevant or you would like to see it actioned, please respond within 3 days. If this issue is critical for your business, please reach out to us at [email protected].

[LukeTowers]

@arvislacis are you able to submit a PR to the test plugin reproducing the issue?

[arvislacis]

@LukeTowers I may try to implement some tests in test plugin although, IMHO, this issue would need back-end related/automated tests more than UI ones...

Overall the problem is that existing pivot table backend_users_groups doesn't have deleted_at column or similar mechanism to implement soft delete state in many-to-many relation. But I see that Laravel has been having problems with this, for example, https://laracasts.com/discuss/channels/general-discussion/soft-deletes-with-pivot-table

[arvislacis]

A step-to-step guide how to reproduce this:

1. Go to the Settings -> Administrators;
2. Create new user (testuser etc.) and add it to group, for example, "Owners" group;

3. Open newly created user (testuser) record and delete it from UI (soft delete on User model will happen);
4. Return to user list, select to "Show deleted" and then restore the deleted user (testuser) account;
5. If you then open testuser record and investigate "Groups" tab then all checkboxes are empty (group relationshios has been deleted with step 3).

[github-actions[bot]]

This issue will be closed and archived in 3 days, as there has been no activity in this issue for the last 6 months. If this issue is still relevant or you would like to see it actioned, please respond within 3 days. If this issue is critical for your business, please reach out to us at [email protected].

[mjauvin]

@arvislacis @xyzqtc The UserGroup model would need to use the SoftDelete trait, and the User model would need to specify 'softDelete' => true in its relatoin definition.

Or am I missing something ?

[arvislacis]

@mjauvin Yes, in general UserGroup model needs SoftDelete trait and DB migration with deleted_at column.

Side note: The problem may also be with the fact that backend_users_groups table is a pivot table, as I have linked before then previously Laravel had issues with them and needed work-arounds but maybe it's fixed now.

[mjauvin]

You're right, the pivot table is probably not supported, unless we need to add a deleted_at column to the pivot table itself ?

[arvislacis]

@mjauvin Yeah, probably... https://laracasts.com/discuss/channels/general-discussion/soft-deletes-with-pivot-table article also gives code examples how to handle these pivot table cases - but still, a bit tricky solution which needs work-around.

[mjauvin]

@arvislacis Winter's SoftDelete trait should handle this case here:

    protected function performSoftDeleteOnRelations()
    {                                                                                                                                            $definitions = $this->getRelationDefinitions();
        foreach ($definitions as $type => $relations) {
            foreach ($relations as $name => $options) {
                if (!array_get($options, 'softDelete', false)) {
                    continue;
                }

                if (!$relation = $this->{$name}) {
                    continue;
                }

                if ($relation instanceof EloquentModel) {
                    $relation->delete();
                }
                elseif ($relation instanceof CollectionBase) {
                    $relation->each(function ($model) {
                        $model->delete();
                    });
                }
            }
        }
    }

And here:

    protected function performRestoreOnRelations()
    {
        $definitions = $this->getRelationDefinitions();
        foreach ($definitions as $type => $relations) {
            foreach ($relations as $name => $options) {
                if (!array_get($options, 'softDelete', false)) {
                    continue;
                }   

                $relation = $this->{$name}()->onlyTrashed()->getResults();
                if (!$relation) {
                    continue;
                }   

                if ($relation instanceof EloquentModel) {
                    $relation->restore();
                }   
                elseif ($relation instanceof CollectionBase) {
                    $relation->each(function ($model) {
                        $model->restore();
                    }); 
                }   
            }
        }
    }

In a belongsToMany relation, the related model should not be deleted, of course, but a special case should be added to this trait's performSoftDeleteOnRelations() / performRestoreOnRelations() methods to handle this (i.e. update the pivot record's deleted_at column instead).

Then, it is the responsibility of the developper marking a belongsToMany relation as softDelete to add a deleted_at column to the pivot table migration.

Any feedback on this @LukeTowers @bennothommo @arvislacis ?

[mjauvin]

We should move the discussion in the associated PR.

[bennothommo]

@arvislacis @mjauvin I appreciate the efforts put into this over the weekend, but let's put a pause on this just for a second.

I do have a minor concern here that we're now unilaterally adding soft deletion to the pivot. Has it been tested and confirmed all working even if people don't enable soft deletion of the Backend users or groups themselves, or will we end up with a whole lot of orphaned pivot records?

I was tempted to suggest that perhaps we should merge @mjauvin's Storm PR (because that does fix a legitimate issue), and maybe in the case of @arvislacis' PR, instead convert that to a plugin so it becomes optional, but I realised that wouldn't work as we can't declare traits programatically, so I just want to make sure we've covered all bases.

[mjauvin]

@bennothommo this is not going to happen because of this:

https://github.com/wintercms/storm/blob/9fac408d370718c91c40db9e797b736b4d3a42b7/src/Database/Traits/SoftDelete.php

[bennothommo]

@mjauvin is there a section you're referencing in that file? I'm not sure what functionality you're specifically referring to that will prevent the orphans from occurring.

[mjauvin]

@mjauvin is there a section you're referencing in that file? I'm not sure what functionality you're specifically referring to that will prevent the orphans from occurring.

Sorry, I'm on mobile and I meant to reference this line:

https://github.com/wintercms/storm/blob/9fac408d370718c91c40db9e797b736b4d3a42b7/src/Database/Traits/SoftDelete.php#L118