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

Improve automatic detaching / deletion of relations #156

Merged
merged 31 commits into from
Oct 18, 2023

Conversation

mjauvin
Copy link
Member

@mjauvin mjauvin commented Sep 9, 2023

First step in Fixing wintercms/winter#386

Related to wintercms/winter#972

When deleting/soft-deleting & restoring a model relations, we need a separate case for relations using a pivot table.

@mjauvin
Copy link
Member Author

mjauvin commented Sep 9, 2023

@xyzqtc @arvislacis welcome to join the discussion

@mjauvin
Copy link
Member Author

mjauvin commented Sep 9, 2023

I tested this with success on a model with a belongsToMany relation, both deleting and restoring the model soft-deleted and restored the related models in the pivot table.

@mjauvin mjauvin requested a review from LukeTowers September 9, 2023 17:55
@mjauvin mjauvin changed the title Handle softDelete for relations with pivot table Handle soft-delete/restore for relations with pivot table Sep 9, 2023
@mjauvin mjauvin added this to the v1.2.4 milestone Sep 9, 2023
@mjauvin
Copy link
Member Author

mjauvin commented Sep 9, 2023

@arvislacis do you mind creating a PR in the main winter repo to address the issue I mentioned in this PR description?

Thanks!

@arvislacis
Copy link
Contributor

@mjauvin I added wintercms/winter#972 PR but it's not working for me - delete_at column is not being updated when deleting User and columns are fully deleted from backend_users_groups pilot table but maybe I am missing some critical part here. Feel free to comment on my PR.

@mjauvin
Copy link
Member Author

mjauvin commented Sep 9, 2023

@arvislacis did you properly copy the SoftDelete trait from my PR to your local storm folder?

@arvislacis
Copy link
Contributor

@mjauvin I am pretty sure I did, I copied full RAW file of this PR to my local copy. But I will re-check this tomorrow.

@mjauvin
Copy link
Member Author

mjauvin commented Sep 9, 2023

And how did you verify it's not working exactly ?

@arvislacis
Copy link
Contributor

@mjauvin Some updates on this:

  1. Yes, I applied the patch correctly (to be clear);
  2. For testing purposes I added break-point at the end of updatePivotDeletedAtColumn method - so far process executed as expected - the deleted_at column of pivot table was correctly updated with timestamp but if I removed the break-point then somewhere later in the process the relationship entry was still fully removed - I will try to investigate further.

"How did you verify it's not working exactly" - I verified it as described in wintercms/winter#386 (comment) - create new user or add existing user (non default admin) to group, then delete the user, then restore user with WinterCMS admin UI - result is that restored user entry has no group relationships.

@arvislacis
Copy link
Contributor

arvislacis commented Sep 10, 2023

@mjauvin I did some further debugging but unfortunately didn't find root cause of pivot table relationship being still deleted - it looks like that after the softDelete another delete operation is happening (maybe on User model itself which triggers pivot relationships to be still deleted).

Anyways, I would suggest to repeat user deletion and restoration operations with WinterCMS back-end as described here: wintercms/winter#386 (comment)

@mjauvin
Copy link
Member Author

mjauvin commented Sep 10, 2023

@arvislacis I found the problem, the Backend User model extends Storm's Auth/User model which has a afterDelete method shown below:

    /**
     * Delete the user groups
     * @return void
     */
    public function afterDelete()
    {   
        if ($this->hasRelation('groups')) {
            $this->groups()->detach();
        }
    }

@mjauvin
Copy link
Member Author

mjauvin commented Sep 10, 2023

So just override that method as an empty method in Winter's model and that resolves the issue.

In your PR, add a comment that explains why the method is overriden and empty.

@mjauvin
Copy link
Member Author

mjauvin commented Sep 10, 2023

Actually, we might want to remove that afterDelete method in Storm's model since it's already done by default in Database/Model...

@LukeTowers or @bennothommo can you confirm that we should remove the afterDelete method in Storm\Auth\Models\User ?

@mjauvin mjauvin changed the title Handle soft-delete/restore for relations with pivot table Fix delete / soft-delte and restore operations on relations with pivot table Sep 10, 2023
@arvislacis
Copy link
Contributor

@mjauvin Nice find! I can confirm that applying the latest changes fixes the wintercms/winter#386

For me it looks like afterDelete can be safely removed ad now we are soft deleting group relationships.

@mjauvin
Copy link
Member Author

mjauvin commented Sep 10, 2023

Yeah, I made a fix to the base Model class so that the afterDelete method is not required anymore

@mjauvin mjauvin marked this pull request as ready for review September 10, 2023 16:12
src/Database/Model.php Outdated Show resolved Hide resolved
src/Database/Model.php Outdated Show resolved Hide resolved
src/Database/Model.php Outdated Show resolved Hide resolved
@LukeTowers
Copy link
Member

LGTM! @bennothommo any final thoughts?

@mjauvin
Copy link
Member Author

mjauvin commented Oct 15, 2023

@bennothommo I added the hard delete tests you requested.

@LukeTowers LukeTowers changed the title Fix delete / soft-delete and restore operations on relations with pivot table Improve automatic detaching / deletion of relations Oct 18, 2023
@LukeTowers LukeTowers merged commit 404b199 into develop Oct 18, 2023
8 checks passed
@LukeTowers LukeTowers deleted the wip/softdelete-pivot branch October 18, 2023 01:30
@arvislacis
Copy link
Contributor

@LukeTowers @mjauvin As this is merged now, what about related PR and issue in winter repo?

@mjauvin
Copy link
Member Author

mjauvin commented Oct 18, 2023

What do you mean exactly?

@arvislacis
Copy link
Contributor

I mean merging also wintercms/winter#972 and closing wintercms/winter#386 or it will be done with WinterCMS 1.4 release?

@mjauvin
Copy link
Member Author

mjauvin commented Oct 18, 2023

Yes, theee should be merged and closed soon.

LukeTowers pushed a commit to wintercms/winter that referenced this pull request Oct 19, 2023
bennothommo pushed a commit to wintercms/wn-backend-module that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants