From fdcb891cbbdb42e8999cb27e1dcaa960ded995ba Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 15:20:56 -0800 Subject: [PATCH 01/24] Improve test case --- tests/Unit/NotificationTest.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/Unit/NotificationTest.php b/tests/Unit/NotificationTest.php index 3d5b3c5a76..5b420a6753 100644 --- a/tests/Unit/NotificationTest.php +++ b/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) { - return $mail->hasTo($user->email); + Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) use ($user) { + return $mail->hasTo($user->email) && $mail->hasSubject(trans('mail.Asset_Checkout_Notification')); }); } public function testDefaultEulaIsSentWhenSetInCategory() From d254a40e0a90ef080b74fb39745c5b24d0993a33 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 15:21:10 -0800 Subject: [PATCH 02/24] Scaffold tests --- .../factories/CheckoutAcceptanceFactory.php | 19 ++++ .../Email/AssetAcceptanceReminderTest.php | 90 +++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php diff --git a/database/factories/CheckoutAcceptanceFactory.php b/database/factories/CheckoutAcceptanceFactory.php index 6bb87d18e1..8dbb6712c6 100644 --- a/database/factories/CheckoutAcceptanceFactory.php +++ b/database/factories/CheckoutAcceptanceFactory.php @@ -27,6 +27,17 @@ class CheckoutAcceptanceFactory extends Factory public function configure(): static { return $this->afterCreating(function (CheckoutAcceptance $acceptance) { + // @todo: add comment + if ($acceptance->checkoutable instanceof Asset) { + $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, + ]); + } + if ($acceptance->checkoutable instanceof Asset && $acceptance->assignedTo instanceof User) { $acceptance->checkoutable->update([ 'assigned_to' => $acceptance->assigned_to_id, @@ -51,4 +62,12 @@ class CheckoutAcceptanceFactory extends Factory 'declined_at' => null, ]); } + + public function accepted() + { + return $this->state([ + 'accepted_at' => now()->subDay(), + 'declined_at' => null, + ]); + } } diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php new file mode 100644 index 0000000000..3809b826e6 --- /dev/null +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -0,0 +1,90 @@ +actor = User::factory()->canViewReports()->create(); + $this->checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); + } + + public function testMustHavePermissionToSendReminder() + { + $userWithoutPermission = User::factory()->create(); + + $this->actingAs($userWithoutPermission) + ->post($this->routeFor($this->checkoutAcceptance)) + ->assertForbidden(); + } + + public function testReminderNotSentIfAcceptanceDoesNotExist() + { + $this->actingAs($this->actor) + ->post(route('reports/unaccepted_assets_sent_reminder', [ + 'acceptance_id' => 999999, + ])); + + Mail::assertNotSent(CheckoutAssetMail::class); + } + + public function testReminderNotSentIfAcceptanceAlreadyAccepted() + { + $checkoutAcceptanceAlreadyAccepted = CheckoutAcceptance::factory()->accepted()->create(); + + $this->actingAs($this->actor) + ->post($this->routeFor($checkoutAcceptanceAlreadyAccepted)); + + Mail::assertNotSent(CheckoutAssetMail::class); + } + + public function testUserWithoutEmailAddressHandledGracefully() + { + $userWithoutEmailAddress = User::factory()->create(['email' => null]); + + $this->checkoutAcceptance->assigned_to_id = $userWithoutEmailAddress->id; + $this->checkoutAcceptance->save(); + + $this->actingAs($this->actor) + ->post($this->routeFor($this->checkoutAcceptance)) + // check we didn't crash... + ->assertRedirect(); + + Mail::assertNotSent(CheckoutAssetMail::class); + } + + public function testReminderIsSentToUser() + { + $this->actingAs($this->actor) + ->post($this->routeFor($this->checkoutAcceptance)) + ->assertRedirect(route('reports/unaccepted_assets')); + + Mail::assertSent(CheckoutAssetMail::class, 1); + Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) { + return $mail->hasTo($this->checkoutAcceptance->assignedTo->email) + // @todo: + && $mail->hasSubject('Reminder: ' . trans('mail.Asset_Checkout_Notification')); + }); + } + + private function routeFor(CheckoutAcceptance $checkoutAcceptance): string + { + return route('reports/unaccepted_assets_sent_reminder', [ + 'acceptance_id' => $checkoutAcceptance->id, + ]); + } +} From e2805f40332afce449c72695c0f4fbca232d718e Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 15:36:45 -0800 Subject: [PATCH 03/24] Add "Reminder" to subject line --- app/Http/Controllers/ReportsController.php | 5 ++- app/Mail/CheckoutAssetMail.php | 15 +++++++-- .../Email/AssetAcceptanceReminderTest.php | 31 ++++++++++++++++--- 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/app/Http/Controllers/ReportsController.php b/app/Http/Controllers/ReportsController.php index f0bbfb74a2..33188d0c01 100644 --- a/app/Http/Controllers/ReportsController.php +++ b/app/Http/Controllers/ReportsController.php @@ -1177,10 +1177,9 @@ class ReportsController extends Controller $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)); - + Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))->locale($locale)); } elseif ($email) { - Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note))); + Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))); } if ($email == ''){ diff --git a/app/Mail/CheckoutAssetMail.php b/app/Mail/CheckoutAssetMail.php index 680014dcd1..fbdaa4fcc0 100644 --- a/app/Mail/CheckoutAssetMail.php +++ b/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); @@ -54,9 +58,16 @@ class CheckoutAssetMail extends Mailable { $from = new Address(config('mail.from.address'), config('mail.from.name')); + $subject = trans('mail.Asset_Checkout_Notification'); + + if (!$this->firstTimeSending) { + // @todo: translate + $subject = 'Reminder: ' . $subject; + } + return new Envelope( from: $from, - subject: trans('mail.Asset_Checkout_Notification'), + subject: $subject, ); } diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index 3809b826e6..d84b2f7120 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -6,6 +6,7 @@ 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 @@ -67,16 +68,36 @@ class AssetAcceptanceReminderTest extends TestCase Mail::assertNotSent(CheckoutAssetMail::class); } - public function testReminderIsSentToUser() + public static function users() { + yield 'User with locale set' => [ + function () { + return CheckoutAcceptance::factory()->pending()->create(); + } + ]; + + yield 'User without locale set' => [ + function () { + $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); + $checkoutAcceptance->assignedTo->update(['locale' => null]); + + return $checkoutAcceptance; + } + ]; + } + + #[DataProvider('users')] + public function testReminderIsSentToUser($data) + { + $checkoutAcceptance = $data(); + $this->actingAs($this->actor) - ->post($this->routeFor($this->checkoutAcceptance)) + ->post($this->routeFor($checkoutAcceptance)) ->assertRedirect(route('reports/unaccepted_assets')); Mail::assertSent(CheckoutAssetMail::class, 1); - Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) { - return $mail->hasTo($this->checkoutAcceptance->assignedTo->email) - // @todo: + Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) use ($checkoutAcceptance) { + return $mail->hasTo($checkoutAcceptance->assignedTo->email) && $mail->hasSubject('Reminder: ' . trans('mail.Asset_Checkout_Notification')); }); } From 70aed45bfe6c5d4b5ee4ed964c58b414d2d35718 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 15:56:20 -0800 Subject: [PATCH 04/24] Improve naming --- .../Notifications/Email/AssetAcceptanceReminderTest.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index d84b2f7120..7411cfbd3b 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -68,7 +68,7 @@ class AssetAcceptanceReminderTest extends TestCase Mail::assertNotSent(CheckoutAssetMail::class); } - public static function users() + public static function acceptances() { yield 'User with locale set' => [ function () { @@ -86,10 +86,10 @@ class AssetAcceptanceReminderTest extends TestCase ]; } - #[DataProvider('users')] - public function testReminderIsSentToUser($data) + #[DataProvider('acceptances')] + public function testReminderIsSentToUser($callback) { - $checkoutAcceptance = $data(); + $checkoutAcceptance = $callback(); $this->actingAs($this->actor) ->post($this->routeFor($checkoutAcceptance)) From 4e7c6bd2cfd87901fec2c71cde855f4c63f77c0c Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:14:09 -0800 Subject: [PATCH 05/24] Fix relationship --- .../Email/AssetAcceptanceReminderTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index 7411cfbd3b..4898234e10 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -55,13 +55,13 @@ class AssetAcceptanceReminderTest extends TestCase public function testUserWithoutEmailAddressHandledGracefully() { - $userWithoutEmailAddress = User::factory()->create(['email' => null]); - - $this->checkoutAcceptance->assigned_to_id = $userWithoutEmailAddress->id; - $this->checkoutAcceptance->save(); + $checkoutAcceptance = CheckoutAcceptance::factory() + ->pending() + ->forAssignedTo(['email' => null]) + ->create(); $this->actingAs($this->actor) - ->post($this->routeFor($this->checkoutAcceptance)) + ->post($this->routeFor($checkoutAcceptance)) // check we didn't crash... ->assertRedirect(); From 78f92925553d29c6149fcd7c45b1a75dd03b1b17 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:15:27 -0800 Subject: [PATCH 06/24] Inline variable --- .../Notifications/Email/AssetAcceptanceReminderTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index 4898234e10..34bc935bb7 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -11,7 +11,6 @@ use Tests\TestCase; class AssetAcceptanceReminderTest extends TestCase { - private CheckoutAcceptance $checkoutAcceptance; private User $actor; protected function setUp(): void @@ -21,15 +20,15 @@ class AssetAcceptanceReminderTest extends TestCase Mail::fake(); $this->actor = User::factory()->canViewReports()->create(); - $this->checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); } public function testMustHavePermissionToSendReminder() { + $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); $userWithoutPermission = User::factory()->create(); $this->actingAs($userWithoutPermission) - ->post($this->routeFor($this->checkoutAcceptance)) + ->post($this->routeFor($checkoutAcceptance)) ->assertForbidden(); } From ce31ce477e046ecec0cc6d56a5df7c44ab863c87 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:16:47 -0800 Subject: [PATCH 07/24] Inline additional variables --- .../Email/AssetAcceptanceReminderTest.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index 34bc935bb7..f163225d1b 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -11,15 +11,11 @@ use Tests\TestCase; class AssetAcceptanceReminderTest extends TestCase { - private User $actor; - protected function setUp(): void { parent::setUp(); Mail::fake(); - - $this->actor = User::factory()->canViewReports()->create(); } public function testMustHavePermissionToSendReminder() @@ -34,7 +30,7 @@ class AssetAcceptanceReminderTest extends TestCase public function testReminderNotSentIfAcceptanceDoesNotExist() { - $this->actingAs($this->actor) + $this->actingAs(User::factory()->canViewReports()->create()) ->post(route('reports/unaccepted_assets_sent_reminder', [ 'acceptance_id' => 999999, ])); @@ -46,7 +42,7 @@ class AssetAcceptanceReminderTest extends TestCase { $checkoutAcceptanceAlreadyAccepted = CheckoutAcceptance::factory()->accepted()->create(); - $this->actingAs($this->actor) + $this->actingAs(User::factory()->canViewReports()->create()) ->post($this->routeFor($checkoutAcceptanceAlreadyAccepted)); Mail::assertNotSent(CheckoutAssetMail::class); @@ -59,7 +55,7 @@ class AssetAcceptanceReminderTest extends TestCase ->forAssignedTo(['email' => null]) ->create(); - $this->actingAs($this->actor) + $this->actingAs(User::factory()->canViewReports()->create()) ->post($this->routeFor($checkoutAcceptance)) // check we didn't crash... ->assertRedirect(); @@ -90,7 +86,7 @@ class AssetAcceptanceReminderTest extends TestCase { $checkoutAcceptance = $callback(); - $this->actingAs($this->actor) + $this->actingAs(User::factory()->canViewReports()->create()) ->post($this->routeFor($checkoutAcceptance)) ->assertRedirect(route('reports/unaccepted_assets')); From d1197d015c22758bbc49f9f6046f76db9e95a844 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:24:43 -0800 Subject: [PATCH 08/24] Add another case scenario --- .../Email/AssetAcceptanceReminderTest.php | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index f163225d1b..d7c89e041e 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -48,12 +48,31 @@ class AssetAcceptanceReminderTest extends TestCase Mail::assertNotSent(CheckoutAssetMail::class); } - public function testUserWithoutEmailAddressHandledGracefully() + public static function CheckoutAcceptancesToUsersWithoutEmailAddresses() { - $checkoutAcceptance = CheckoutAcceptance::factory() - ->pending() - ->forAssignedTo(['email' => null]) - ->create(); + 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)) From c15c338ffd29beed180d352897599226f70d10bb Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:25:37 -0800 Subject: [PATCH 09/24] Merge if/else --- app/Http/Controllers/ReportsController.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/ReportsController.php b/app/Http/Controllers/ReportsController.php index 33188d0c01..5ce23138e2 100644 --- a/app/Http/Controllers/ReportsController.php +++ b/app/Http/Controllers/ReportsController.php @@ -1176,11 +1176,9 @@ class ReportsController extends Controller $email = $assetItem->assignedTo?->email; $locale = $assetItem->assignedTo?->locale; // Only send notification if assigned - if ($locale && $email) { + if ($email) { Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))->locale($locale)); - } elseif ($email) { - Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))); - } + } if ($email == ''){ return redirect()->route('reports/unaccepted_assets')->with('error', trans('general.no_email')); From ab9e9b66d257d91ca35351ee423901223d7afc1b Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 29 Jan 2025 16:27:18 -0800 Subject: [PATCH 10/24] Reduce complexity --- app/Http/Controllers/ReportsController.php | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/Http/Controllers/ReportsController.php b/app/Http/Controllers/ReportsController.php index 5ce23138e2..fa437ed20e 100644 --- a/app/Http/Controllers/ReportsController.php +++ b/app/Http/Controllers/ReportsController.php @@ -1175,15 +1175,13 @@ class ReportsController extends Controller } $email = $assetItem->assignedTo?->email; $locale = $assetItem->assignedTo?->locale; - // Only send notification if assigned - if ($email) { - Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note, firstTimeSending: false))->locale($locale)); - } - if ($email == ''){ + if (is_null($email) || $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')); } From 6a4a5d13806fead9cc5f19588dad4227956f2231 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 30 Jan 2025 10:35:31 -0800 Subject: [PATCH 11/24] Add translation --- app/Mail/CheckoutAssetMail.php | 3 +-- resources/lang/en-US/mail.php | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Mail/CheckoutAssetMail.php b/app/Mail/CheckoutAssetMail.php index fbdaa4fcc0..d6ce123d40 100644 --- a/app/Mail/CheckoutAssetMail.php +++ b/app/Mail/CheckoutAssetMail.php @@ -61,8 +61,7 @@ class CheckoutAssetMail extends Mailable $subject = trans('mail.Asset_Checkout_Notification'); if (!$this->firstTimeSending) { - // @todo: translate - $subject = 'Reminder: ' . $subject; + $subject = trans('mail.Asset_Checkout_Reminder_Notification'); } return new Envelope( diff --git a/resources/lang/en-US/mail.php b/resources/lang/en-US/mail.php index 7663a0167b..f7d3794dc3 100644 --- a/resources/lang/en-US/mail.php +++ b/resources/lang/en-US/mail.php @@ -6,6 +6,7 @@ return [ 'Accessory_Checkout_Notification' => 'Accessory checked out', 'Asset_Checkin_Notification' => 'Asset checked in', 'Asset_Checkout_Notification' => 'Asset checked out', + 'Asset_Checkout_Reminder_Notification' => 'Reminder: Asset checked out', 'Confirm_Accessory_Checkin' => 'Accessory checkin confirmation', 'Confirm_Asset_Checkin' => 'Asset checkin confirmation', 'Confirm_accessory_delivery' => 'Accessory delivery confirmation', From e94ee48f741846cd8ee4d51987fdb9848ac744f9 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 30 Jan 2025 10:37:11 -0800 Subject: [PATCH 12/24] Extract helper --- app/Mail/CheckoutAssetMail.php | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/app/Mail/CheckoutAssetMail.php b/app/Mail/CheckoutAssetMail.php index d6ce123d40..eb9619217f 100644 --- a/app/Mail/CheckoutAssetMail.php +++ b/app/Mail/CheckoutAssetMail.php @@ -58,15 +58,9 @@ class CheckoutAssetMail extends Mailable { $from = new Address(config('mail.from.address'), config('mail.from.name')); - $subject = trans('mail.Asset_Checkout_Notification'); - - if (!$this->firstTimeSending) { - $subject = trans('mail.Asset_Checkout_Reminder_Notification'); - } - return new Envelope( from: $from, - subject: $subject, + subject: $this->getSubject(), ); } @@ -117,4 +111,13 @@ class CheckoutAssetMail extends Mailable { return []; } + + private function getSubject(): string + { + if ($this->firstTimeSending) { + return trans('mail.Asset_Checkout_Notification'); + } + + return trans('mail.Asset_Checkout_Reminder_Notification'); + } } From fc88b2487ff524d6515852b9fa8224263cdba411 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 30 Jan 2025 11:44:37 -0800 Subject: [PATCH 13/24] Extract method --- .../factories/CheckoutAcceptanceFactory.php | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/database/factories/CheckoutAcceptanceFactory.php b/database/factories/CheckoutAcceptanceFactory.php index 8dbb6712c6..a15e942a84 100644 --- a/database/factories/CheckoutAcceptanceFactory.php +++ b/database/factories/CheckoutAcceptanceFactory.php @@ -27,15 +27,8 @@ class CheckoutAcceptanceFactory extends Factory public function configure(): static { return $this->afterCreating(function (CheckoutAcceptance $acceptance) { - // @todo: add comment if ($acceptance->checkoutable instanceof Asset) { - $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, - ]); + $this->createdAssociatedActionLogEntry($acceptance); } if ($acceptance->checkoutable instanceof Asset && $acceptance->assignedTo instanceof User) { @@ -70,4 +63,15 @@ class CheckoutAcceptanceFactory extends Factory '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, + ]); + } } From 7e9c564d0b1398eafaff1fedfc16773e11b5163d Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 30 Jan 2025 11:47:43 -0800 Subject: [PATCH 14/24] Simplify test --- .../Email/AssetAcceptanceReminderTest.php | 23 ++----------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index d7c89e041e..db3de3c54c 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -82,28 +82,9 @@ class AssetAcceptanceReminderTest extends TestCase Mail::assertNotSent(CheckoutAssetMail::class); } - public static function acceptances() + public function testReminderIsSentToUser() { - yield 'User with locale set' => [ - function () { - return CheckoutAcceptance::factory()->pending()->create(); - } - ]; - - yield 'User without locale set' => [ - function () { - $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); - $checkoutAcceptance->assignedTo->update(['locale' => null]); - - return $checkoutAcceptance; - } - ]; - } - - #[DataProvider('acceptances')] - public function testReminderIsSentToUser($callback) - { - $checkoutAcceptance = $callback(); + $checkoutAcceptance = CheckoutAcceptance::factory()->pending()->create(); $this->actingAs(User::factory()->canViewReports()->create()) ->post($this->routeFor($checkoutAcceptance)) From 7cbb3f7e07f7820f665211e0e0e32a75644281fd Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Thu, 30 Jan 2025 11:48:55 -0800 Subject: [PATCH 15/24] Add assertion --- .../Feature/Notifications/Email/AssetAcceptanceReminderTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index db3de3c54c..85915352b7 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -26,6 +26,8 @@ class AssetAcceptanceReminderTest extends TestCase $this->actingAs($userWithoutPermission) ->post($this->routeFor($checkoutAcceptance)) ->assertForbidden(); + + Mail::assertNotSent(CheckoutAssetMail::class); } public function testReminderNotSentIfAcceptanceDoesNotExist() From 027c2b36276d5da0a15334c9da11ac9c8cdb9cfb Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 24 Feb 2025 11:45:23 -0800 Subject: [PATCH 16/24] Change subject to "You have Unaccepted Assets." --- app/Mail/CheckoutAssetMail.php | 2 +- .../Feature/Notifications/Email/AssetAcceptanceReminderTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Mail/CheckoutAssetMail.php b/app/Mail/CheckoutAssetMail.php index eb9619217f..1447dfd6f5 100644 --- a/app/Mail/CheckoutAssetMail.php +++ b/app/Mail/CheckoutAssetMail.php @@ -118,6 +118,6 @@ class CheckoutAssetMail extends Mailable return trans('mail.Asset_Checkout_Notification'); } - return trans('mail.Asset_Checkout_Reminder_Notification'); + return trans('mail.unaccepted_asset_reminder'); } } diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php index 85915352b7..deb3e07d2c 100644 --- a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -95,7 +95,7 @@ class AssetAcceptanceReminderTest extends TestCase Mail::assertSent(CheckoutAssetMail::class, 1); Mail::assertSent(CheckoutAssetMail::class, function (CheckoutAssetMail $mail) use ($checkoutAcceptance) { return $mail->hasTo($checkoutAcceptance->assignedTo->email) - && $mail->hasSubject('Reminder: ' . trans('mail.Asset_Checkout_Notification')); + && $mail->hasSubject(trans('mail.unaccepted_asset_reminder')); }); } From f97211f6cd0d312cc83c11d5ffad360a59517e36 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Mon, 24 Feb 2025 11:45:58 -0800 Subject: [PATCH 17/24] Remove unused language string --- resources/lang/en-US/mail.php | 1 - 1 file changed, 1 deletion(-) diff --git a/resources/lang/en-US/mail.php b/resources/lang/en-US/mail.php index f7d3794dc3..7663a0167b 100644 --- a/resources/lang/en-US/mail.php +++ b/resources/lang/en-US/mail.php @@ -6,7 +6,6 @@ return [ 'Accessory_Checkout_Notification' => 'Accessory checked out', 'Asset_Checkin_Notification' => 'Asset checked in', 'Asset_Checkout_Notification' => 'Asset checked out', - 'Asset_Checkout_Reminder_Notification' => 'Reminder: Asset checked out', 'Confirm_Accessory_Checkin' => 'Accessory checkin confirmation', 'Confirm_Asset_Checkin' => 'Asset checkin confirmation', 'Confirm_accessory_delivery' => 'Accessory delivery confirmation', From de3c1d159f7e3b5313e50579c3c5a27c5200d5f9 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 26 Feb 2025 15:35:17 -0800 Subject: [PATCH 18/24] Replace Form::checkbox on branding settings page --- resources/views/settings/branding.blade.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/resources/views/settings/branding.blade.php b/resources/views/settings/branding.blade.php index f37d6685b2..8c59a02fba 100644 --- a/resources/views/settings/branding.blade.php +++ b/resources/views/settings/branding.blade.php @@ -135,7 +135,7 @@

@@ -152,7 +152,7 @@

@@ -172,7 +172,7 @@
From 245a16c377bbd1b381ba57556ebdd69bc78ebd24 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 26 Feb 2025 15:37:07 -0800 Subject: [PATCH 19/24] Replace Form::checkbox on branding settings page --- resources/views/settings/branding.blade.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/resources/views/settings/branding.blade.php b/resources/views/settings/branding.blade.php index 8c59a02fba..a31d6e42ff 100644 --- a/resources/views/settings/branding.blade.php +++ b/resources/views/settings/branding.blade.php @@ -187,7 +187,7 @@

{{ trans('admin/settings/general.show_url_in_emails_help_text') }}

@@ -228,7 +228,7 @@

{{ trans('admin/settings/general.allow_user_skin_help_text') }}

From 27aeb518ffd263b0b0ff7b21b4e6d2faae9ac7f7 Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 26 Feb 2025 16:18:00 -0800 Subject: [PATCH 20/24] Replace Form::checkbox on general settings page --- resources/views/settings/general.blade.php | 24 +++++++++++++--------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/resources/views/settings/general.blade.php b/resources/views/settings/general.blade.php index fb85908761..d2e51d4341 100644 --- a/resources/views/settings/general.blade.php +++ b/resources/views/settings/general.blade.php @@ -45,7 +45,7 @@
{!! $errors->first('full_multiple_companies_support', '') !!} @@ -64,7 +64,7 @@
{!! $errors->first('require_accept_signature', '') !!} @@ -136,7 +136,7 @@
@@ -152,7 +152,7 @@
@@ -311,7 +311,7 @@
{!! $errors->first('show_archived_in_list', '') !!} @@ -325,7 +325,7 @@

{{ trans('admin/settings/general.show_assigned_assets_help') }}

@@ -340,15 +340,19 @@
+ modellistCheckedValue('manufacturer'))) aria-label="show_in_model_list"/> + {{ trans('general.manufacturer') }}
From a0dc056da851622b3445ae8493942a319828614c Mon Sep 17 00:00:00 2001 From: Marcus Moore Date: Wed, 26 Feb 2025 16:33:07 -0800 Subject: [PATCH 21/24] Replace Form::checkbox on label settings page --- resources/views/settings/labels.blade.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/resources/views/settings/labels.blade.php b/resources/views/settings/labels.blade.php index 489c6ac584..e4fee660a4 100644 --- a/resources/views/settings/labels.blade.php +++ b/resources/views/settings/labels.blade.php @@ -144,7 +144,7 @@
@@ -189,7 +189,7 @@
@@ -480,23 +480,23 @@
From cd1d1b2d3ef2d3fb5af0b1fa3f211a40ca4b3df4 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 27 Feb 2025 14:22:40 +0000 Subject: [PATCH 22/24] Fixed count Signed-off-by: snipe --- app/Console/Commands/SendAcceptanceReminder.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/Console/Commands/SendAcceptanceReminder.php b/app/Console/Commands/SendAcceptanceReminder.php index 89a5b6167a..250b08abf9 100644 --- a/app/Console/Commands/SendAcceptanceReminder.php +++ b/app/Console/Commands/SendAcceptanceReminder.php @@ -70,23 +70,26 @@ class SendAcceptanceReminder extends Command // The [0] is weird, but it allows for the item_count to work and grabs the appropriate info for each user. // Collapsing and flattening the collection doesn't work above. $acceptance = $unacceptedAssetGroup[0]['acceptance']; + $locale = $acceptance->assignedTo?->locale; $email = $acceptance->assignedTo?->email; + if(!$email){ $no_email_list[] = [ - 'id' => $acceptance->assignedTo->id, - 'name' => $acceptance->assignedTo->present()->fullName(), + 'id' => $acceptance->assignedTo?->id, + 'name' => $acceptance->assignedTo?->present()->fullName(), ]; + } else { + $count++; } $item_count = $unacceptedAssetGroup->count(); if ($locale && $email) { Mail::to($email)->send((new UnacceptedAssetReminderMail($acceptance, $item_count))->locale($locale)); - } elseif ($email) { Mail::to($email)->send((new UnacceptedAssetReminderMail($acceptance, $item_count))); } - $count++; + } $this->info($count.' users notified.'); From 25807cc62f9ef77501421b0cb8d525e0fba71a4b Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 27 Feb 2025 14:22:48 +0000 Subject: [PATCH 23/24] Fixed constructor Signed-off-by: snipe --- app/Mail/UnacceptedAssetReminderMail.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/Mail/UnacceptedAssetReminderMail.php b/app/Mail/UnacceptedAssetReminderMail.php index 1436bbc84e..0e4473aaad 100644 --- a/app/Mail/UnacceptedAssetReminderMail.php +++ b/app/Mail/UnacceptedAssetReminderMail.php @@ -19,9 +19,10 @@ class UnacceptedAssetReminderMail extends Mailable */ public function __construct($checkout_info, $count) { + $this->count = $count; - $this->target = $checkout_info['acceptance']?->assignedTo; - $this->acceptance = $checkout_info['acceptance']; + $this->target = $checkout_info?->assignedTo; + $this->acceptance = $checkout_info; } /** From 27fc30a8810de256ff014bf2d49f0e33663795d5 Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 27 Feb 2025 15:43:53 +0000 Subject: [PATCH 24/24] Nicer button layout on unaccepted assets Signed-off-by: snipe --- resources/lang/en-US/admin/reports/general.php | 1 + resources/views/reports/unaccepted_assets.blade.php | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/resources/lang/en-US/admin/reports/general.php b/resources/lang/en-US/admin/reports/general.php index ea22b07dfe..ed8bf1a156 100644 --- a/resources/lang/en-US/admin/reports/general.php +++ b/resources/lang/en-US/admin/reports/general.php @@ -4,6 +4,7 @@ return [ 'info' => 'Select the options you want for your asset report.', 'deleted_user' => 'Deleted user', 'send_reminder' => 'Send reminder', + 'cannot_send_reminder' => 'User has been deleted or does not have an email address so cannot receive a reminder', 'reminder_sent' => 'Reminder sent', 'acceptance_deleted' => 'Acceptance request deleted', 'acceptance_request' => 'Acceptance request', diff --git a/resources/views/reports/unaccepted_assets.blade.php b/resources/views/reports/unaccepted_assets.blade.php index f640b6747d..69512537e8 100644 --- a/resources/views/reports/unaccepted_assets.blade.php +++ b/resources/views/reports/unaccepted_assets.blade.php @@ -79,13 +79,18 @@ @if(!$item['acceptance']->trashed())
- @if ($item['acceptance']->assignedTo) + @if (($item['acceptance']->assignedTo) && ($item['acceptance']->assignedTo->email)) @csrf - + @else + + + + + @endif