Merge pull request #16156 from marcusmoore/acceptance-reminder-subject

Added "Reminder" to subject line of follow up asset checkout emails

app/Http/Controllers/ReportsController.php

@@ -1175,18 +1175,13 @@ class ReportsController extends Controller
         }
         $email = $assetItem->assignedTo?->email;
         $locale = $assetItem->assignedTo?->locale;
-        // Only send notification if assigned
-        if ($locale && $email) {
-            Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note))->locale($locale));
 
-            } elseif ($email) {
+        if (is_null($email) || $email === '') {
-            Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note)));
-            }
-
-        if ($email == ''){
             return redirect()->route('reports/unaccepted_assets')->with('error', trans('general.no_email'));
         }
 
+        Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))->locale($locale));
+
         return redirect()->route('reports/unaccepted_assets')->with('success', trans('admin/reports/general.reminder_sent'));
     }
 

app/Mail/CheckoutAssetMail.php

@@ -20,10 +20,12 @@ class CheckoutAssetMail extends Mailable
 {
     use Queueable, SerializesModels;
 
+    private bool $firstTimeSending;
+
     /**
      * Create a new message instance.
      */
-    public function __construct(Asset $asset, $checkedOutTo, User $checkedOutBy,  $acceptance, $note)
+    public function __construct(Asset $asset, $checkedOutTo, User $checkedOutBy, $acceptance, $note, bool $firstTimeSending = true)
     {
         $this->item = $asset;
         $this->admin = $checkedOutBy;
@@ -36,6 +38,8 @@ class CheckoutAssetMail extends Mailable
         $this->last_checkout = '';
         $this->expected_checkin = '';
 
+        $this->firstTimeSending = $firstTimeSending;
+
         if ($this->item->last_checkout) {
             $this->last_checkout = Helper::getFormattedDateObject($this->item->last_checkout, 'date',
                 false);
@@ -56,7 +60,7 @@ class CheckoutAssetMail extends Mailable
 
         return new Envelope(
             from: $from,
-            subject: trans('mail.Asset_Checkout_Notification'),
+            subject: $this->getSubject(),
         );
     }
 
@@ -107,4 +111,13 @@ class CheckoutAssetMail extends Mailable
     {
         return [];
     }
+
+    private function getSubject(): string
+    {
+        if ($this->firstTimeSending) {
+            return trans('mail.Asset_Checkout_Notification');
+        }
+
+        return trans('mail.unaccepted_asset_reminder');
+    }
 }

database/factories/CheckoutAcceptanceFactory.php

@@ -27,6 +27,10 @@ class CheckoutAcceptanceFactory extends Factory
     public function configure(): static
     {
         return $this->afterCreating(function (CheckoutAcceptance $acceptance) {
+            if ($acceptance->checkoutable instanceof Asset) {
+                $this->createdAssociatedActionLogEntry($acceptance);
+            }
+
             if ($acceptance->checkoutable instanceof Asset && $acceptance->assignedTo instanceof User) {
                 $acceptance->checkoutable->update([
                     'assigned_to' => $acceptance->assigned_to_id,
@@ -51,4 +55,23 @@ class CheckoutAcceptanceFactory extends Factory
             'declined_at' => null,
         ]);
     }
+
+    public function accepted()
+    {
+        return $this->state([
+            'accepted_at' => now()->subDay(),
+            'declined_at' => null,
+        ]);
+    }
+
+    private function createdAssociatedActionLogEntry(CheckoutAcceptance $acceptance): void
+    {
+        $acceptance->checkoutable->assetlog()->create([
+            'action_type' => 'checkout',
+            'target_id' => $acceptance->assigned_to_id,
+            'target_type' => get_class($acceptance->assignedTo),
+            'item_id' => $acceptance->checkoutable_id,
+            'item_type' => $acceptance->checkoutable_type,
+        ]);
+    }
 }

tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php

@@ -0,0 +1,108 @@
+<?php
+
+namespace Tests\Feature\Notifications\Email;
+
+use App\Mail\CheckoutAssetMail;
+use App\Models\CheckoutAcceptance;
+use App\Models\User;
+use Illuminate\Support\Facades\Mail;
+use PHPUnit\Framework\Attributes\DataProvider;
+use Tests\TestCase;
+
+class AssetAcceptanceReminderTest extends TestCase
+{
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        Mail::fake();
+    }
+
+    public function testMustHavePermissionToSendReminder()
+    {
+        $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create();
+        $userWithoutPermission = User::factory()->create();
+
+        $this->actingAs($userWithoutPermission)
+            ->post($this->routeFor($checkoutAcceptance))
+            ->assertForbidden();
+
+        Mail::assertNotSent(CheckoutAssetMail::class);
+    }
+
+    public function testReminderNotSentIfAcceptanceDoesNotExist()
+    {
+        $this->actingAs(User::factory()->canViewReports()->create())
+            ->post(route('reports/unaccepted_assets_sent_reminder', [
+                'acceptance_id' => 999999,
+            ]));
+
+        Mail::assertNotSent(CheckoutAssetMail::class);
+    }
+
+    public function testReminderNotSentIfAcceptanceAlreadyAccepted()
+    {
+        $checkoutAcceptanceAlreadyAccepted = CheckoutAcceptance::factory()->accepted()->create();
+
+        $this->actingAs(User::factory()->canViewReports()->create())
+            ->post($this->routeFor($checkoutAcceptanceAlreadyAccepted));
+
+        Mail::assertNotSent(CheckoutAssetMail::class);
+    }
+
+    public static function CheckoutAcceptancesToUsersWithoutEmailAddresses()
+    {
+        yield 'User with null email address' => [
+            function () {
+                return CheckoutAcceptance::factory()
+                    ->pending()
+                    ->forAssignedTo(['email' => null])
+                    ->create();
+            }
+        ];
+
+        yield 'User with empty string email address' => [
+            function () {
+                return CheckoutAcceptance::factory()
+                    ->pending()
+                    ->forAssignedTo(['email' => ''])
+                    ->create();
+            }
+        ];
+    }
+
+    #[DataProvider('CheckoutAcceptancesToUsersWithoutEmailAddresses')]
+    public function testUserWithoutEmailAddressHandledGracefully($callback)
+    {
+        $checkoutAcceptance = $callback();
+
+        $this->actingAs(User::factory()->canViewReports()->create())
+            ->post($this->routeFor($checkoutAcceptance))
+            // check we didn't crash...
+            ->assertRedirect();
+
+        Mail::assertNotSent(CheckoutAssetMail::class);
+    }
+
+    public function testReminderIsSentToUser()
+    {
+        $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create();
+
+        $this->actingAs(User::factory()->canViewReports()->create())
+            ->post($this->routeFor($checkoutAcceptance))
+            ->assertRedirect(route('reports/unaccepted_assets'));
+
+        Mail::assertSent(CheckoutAssetMail::class, 1);
+        Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) use ($checkoutAcceptance) {
+            return $mail->hasTo($checkoutAcceptance->assignedTo->email)
+                && $mail->hasSubject(trans('mail.unaccepted_asset_reminder'));
+        });
+    }
+
+    private function routeFor(CheckoutAcceptance $checkoutAcceptance): string
+    {
+        return route('reports/unaccepted_assets_sent_reminder', [
+            'acceptance_id' => $checkoutAcceptance->id,
+        ]);
+    }
+}

tests/Unit/NotificationTest.php

@@ -7,9 +7,7 @@ use App\Models\Asset;
 use App\Models\AssetModel;
 use App\Models\Category;
 use Carbon\Carbon;
-use App\Notifications\CheckoutAssetNotification;
 use Illuminate\Support\Facades\Mail;
-use Illuminate\Support\Facades\Notification;
 use Tests\TestCase;
 
 class NotificationTest extends TestCase
@@ -33,8 +31,8 @@ class NotificationTest extends TestCase
 
         Mail::fake();
         $asset->checkOut($user, $admin->id);
-        Mail::assertSent(CheckoutAssetMail::class, function ($mail) use ($user) {
+        Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) use ($user) {
-            return $mail->hasTo($user->email);
+            return $mail->hasTo($user->email) && $mail->hasSubject(trans('mail.Asset_Checkout_Notification'));
         });
     }
     public function testDefaultEulaIsSentWhenSetInCategory()