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

Handle soft-deletes as regular deletes for Turbo Stream #16

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

tortuetorche
Copy link
Contributor

@tortuetorche tortuetorche commented Mar 18, 2021

Hi @tonysm,

I don't think soft-deletes are handle as regular deletes right now for default Turbo Stream.

According to this line: https://github.com/tonysm/turbo-laravel/blob/0.3.2/src/TurboStreamResponseMacro.php#L19

You check if the model exists, but a soft deleted model response to true on this exists attribute.

In this pull request I add the fix below and a corresponding test:

        if (! $model->exists ||
             (method_exists($model, 'trashed') && $model->trashed())
        ) {
            return $this->renderModelDeletedStream($model);
        }

If you remove my fix, the new test will failed with this error:

There was 1 failure:

1) Tonysm\TurboLaravel\Tests\Http\ResponseMacrosTest::streams_model_on_soft_delete
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<turbo-stream target="test_model_soft_delete_1" action="remove"></turbo-stream>'
+'<turbo-stream target="test_model_soft_deletes" action="append">\n
+    <template>\n
+        <div id="test_model_soft_delete_1">hello</div>\n
+    </template>\n
+</turbo-stream>'

/home/username/turbo-laravel/tests/Http/ResponseMacrosTest.php:126

Reference:
#5 (comment)

Have a good day,
Tortue Torche

@tonysm
Copy link
Collaborator

tonysm commented Mar 19, 2021

Oh, nice catch!

@tonysm tonysm merged commit efd8889 into hotwired-laravel:main Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants