From 95a1848b64c0f13d6d15551397ade8bd396a2765 Mon Sep 17 00:00:00 2001 From: will Date: Fri, 26 Feb 2021 11:27:16 +0000 Subject: [PATCH 1/5] Delete failed messages on subscriber deletion --- src/Models/Subscriber.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Models/Subscriber.php b/src/Models/Subscriber.php index 60d77af1..9ad7d856 100644 --- a/src/Models/Subscriber.php +++ b/src/Models/Subscriber.php @@ -91,6 +91,11 @@ function ($model) { static::deleting( function (self $subscriber) { $subscriber->tags()->detach(); + + $subscriber->messages()->each(function (Message $message) { + $message->failures()->delete(); + }); + $subscriber->messages()->delete(); } ); From 51840518dec72002ad83ea7e31abd429f420e2dd Mon Sep 17 00:00:00 2001 From: will Date: Fri, 5 Mar 2021 10:30:30 +0000 Subject: [PATCH 2/5] Move failed deletion to message class --- src/Models/Message.php | 4 ++++ src/Models/Subscriber.php | 5 ----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Models/Message.php b/src/Models/Message.php index 580b74a7..3df11659 100644 --- a/src/Models/Message.php +++ b/src/Models/Message.php @@ -95,6 +95,10 @@ protected static function boot(): void static::creating(function ($model) { $model->hash = $model->hash ?: Uuid::uuid4()->toString(); }); + + static::deleting(function (self $message) { + $message->failures()->delete(); + }); } public function failures(): HasMany diff --git a/src/Models/Subscriber.php b/src/Models/Subscriber.php index 9ad7d856..60d77af1 100644 --- a/src/Models/Subscriber.php +++ b/src/Models/Subscriber.php @@ -91,11 +91,6 @@ function ($model) { static::deleting( function (self $subscriber) { $subscriber->tags()->detach(); - - $subscriber->messages()->each(function (Message $message) { - $message->failures()->delete(); - }); - $subscriber->messages()->delete(); } ); From e0cf3606bdd87b0b1d6583407883d081f244e49b Mon Sep 17 00:00:00 2001 From: Joshua Franks Date: Fri, 5 Mar 2021 11:06:29 +0000 Subject: [PATCH 3/5] Add tests and add loop for message deletion This adds a test for the deletion of a subscriber's messages and any associated message failures. In adding this test, I realised that - because we were performing a mass deletion - eloquent events were not being fired. This meant that the failures were not being deleted and the operation would bomb out due to a constraint error. I'm in two minds as to whether the loop and delete per-message is better than a mass deletion with a loop that just handles the messages in our subscriber class. I think the deletion of a message's failures should be done at the Message level to ensure consistent behaviour and in practice, this should be a relatively inexpensive operation. --- src/Models/Subscriber.php | 5 +++-- tests/Unit/Models/MessageTest.php | 30 ++++++++++++++++++++++++++++ tests/Unit/Models/SubscriberTest.php | 20 +++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 tests/Unit/Models/MessageTest.php diff --git a/src/Models/Subscriber.php b/src/Models/Subscriber.php index 60d77af1..cb83ae56 100644 --- a/src/Models/Subscriber.php +++ b/src/Models/Subscriber.php @@ -87,11 +87,12 @@ function ($model) { $model->hash = Uuid::uuid4()->toString(); } ); - static::deleting( function (self $subscriber) { $subscriber->tags()->detach(); - $subscriber->messages()->delete(); + $subscriber->messages->each(static function (Message $message) { + $message->delete(); + }); } ); } diff --git a/tests/Unit/Models/MessageTest.php b/tests/Unit/Models/MessageTest.php new file mode 100644 index 00000000..0099d6e0 --- /dev/null +++ b/tests/Unit/Models/MessageTest.php @@ -0,0 +1,30 @@ +create(); + $message->failures()->create(); + + // when + $message->delete(); + + // then + static::assertCount(0, Message::all()); + static::assertCount(0, MessageFailure::all()); + } +} diff --git a/tests/Unit/Models/SubscriberTest.php b/tests/Unit/Models/SubscriberTest.php index 97e6a69a..2370076c 100644 --- a/tests/Unit/Models/SubscriberTest.php +++ b/tests/Unit/Models/SubscriberTest.php @@ -8,6 +8,7 @@ use Illuminate\Foundation\Testing\RefreshDatabase; use Sendportal\Base\Models\Campaign; use Sendportal\Base\Models\Message; +use Sendportal\Base\Models\MessageFailure; use Sendportal\Base\Models\Subscriber; use Tests\TestCase; @@ -39,6 +40,25 @@ public function it_has_many_messages() static::assertEquals($campaignTwo->id, $messageTwo->source_id); } + /** @test */ + public function deleting_a_subscriber_also_deletes_its_messages_and_any_failures_associated_to_them() + { + // given + $subscriber = Subscriber::factory()->create(); + $message = Message::factory()->create([ + 'subscriber_id' => $subscriber->id, + ]); + $message->failures()->create(); + + // when + $subscriber->delete(); + + // then + static::assertCount(0, Subscriber::all()); + static::assertCount(0, Message::all()); + static::assertCount(0, MessageFailure::all()); + } + /** * @param Subscriber $subscriber * From 9a2639a397643bf52530d9d13852b48f9d57d0eb Mon Sep 17 00:00:00 2001 From: Joshua Franks Date: Fri, 5 Mar 2021 11:16:07 +0000 Subject: [PATCH 4/5] Favour mass deletion Although it adds some duplication, the performance benefits are simply more valuable. --- src/Models/Subscriber.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Models/Subscriber.php b/src/Models/Subscriber.php index cb83ae56..9da45649 100644 --- a/src/Models/Subscriber.php +++ b/src/Models/Subscriber.php @@ -91,8 +91,9 @@ function ($model) { function (self $subscriber) { $subscriber->tags()->detach(); $subscriber->messages->each(static function (Message $message) { - $message->delete(); + $message->failures()->delete(); }); + $subscriber->messages()->delete(); } ); } From 960b56cb2a811621a9130015acfdb78f2e298851 Mon Sep 17 00:00:00 2001 From: Joshua Franks Date: Fri, 5 Mar 2021 11:18:01 +0000 Subject: [PATCH 5/5] Access relationship instance instead of collection --- src/Models/Subscriber.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Models/Subscriber.php b/src/Models/Subscriber.php index 9da45649..1063bff1 100644 --- a/src/Models/Subscriber.php +++ b/src/Models/Subscriber.php @@ -90,7 +90,7 @@ function ($model) { static::deleting( function (self $subscriber) { $subscriber->tags()->detach(); - $subscriber->messages->each(static function (Message $message) { + $subscriber->messages()->each(static function (Message $message) { $message->failures()->delete(); }); $subscriber->messages()->delete();