diff --git a/app/Console/Commands/SendAcceptanceReminder.php b/app/Console/Commands/SendAcceptanceReminder.php index 89a5b6167ade..250b08abf9eb 100644 --- a/app/Console/Commands/SendAcceptanceReminder.php +++ b/app/Console/Commands/SendAcceptanceReminder.php @@ -70,23 +70,26 @@ public function handle() // 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.'); diff --git a/app/Http/Controllers/ReportsController.php b/app/Http/Controllers/ReportsController.php index 77df328b4bce..ae2896e6e89d 100644 --- a/app/Http/Controllers/ReportsController.php +++ b/app/Http/Controllers/ReportsController.php @@ -1175,18 +1175,13 @@ public function sentAssetAcceptanceReminder(Request $request) : RedirectResponse } $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) { - Mail::to($email)->send((new CheckoutAssetMail($assetItem, $assetItem->assignedTo, $logItem->user, $acceptance, $logItem->note))); - } - - 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')); } diff --git a/app/Mail/CheckoutAssetMail.php b/app/Mail/CheckoutAssetMail.php index 680014dcd189..1447dfd6f568 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 @@ public function __construct(Asset $asset, $checkedOutTo, User $checkedOutBy, $a $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 @@ public function envelope(): Envelope return new Envelope( from: $from, - subject: trans('mail.Asset_Checkout_Notification'), + subject: $this->getSubject(), ); } @@ -107,4 +111,13 @@ public function attachments(): array { return []; } + + private function getSubject(): string + { + if ($this->firstTimeSending) { + return trans('mail.Asset_Checkout_Notification'); + } + + return trans('mail.unaccepted_asset_reminder'); + } } diff --git a/app/Mail/UnacceptedAssetReminderMail.php b/app/Mail/UnacceptedAssetReminderMail.php index 1436bbc84e08..0e4473aaadf2 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; } /** diff --git a/database/factories/CheckoutAcceptanceFactory.php b/database/factories/CheckoutAcceptanceFactory.php index 6bb87d18e11d..a15e942a8473 100644 --- a/database/factories/CheckoutAcceptanceFactory.php +++ b/database/factories/CheckoutAcceptanceFactory.php @@ -27,6 +27,10 @@ public function definition() 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 @@ public function pending() '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, + ]); + } } diff --git a/resources/lang/en-US/admin/reports/general.php b/resources/lang/en-US/admin/reports/general.php index ea22b07dfebf..ed8bf1a15685 100644 --- a/resources/lang/en-US/admin/reports/general.php +++ b/resources/lang/en-US/admin/reports/general.php @@ -4,6 +4,7 @@ '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 f640b6747da2..69512537e800 100644 --- a/resources/views/reports/unaccepted_assets.blade.php +++ b/resources/views/reports/unaccepted_assets.blade.php @@ -79,13 +79,18 @@ class="table table-striped snipe-table" @if(!$item['acceptance']->trashed())
- @if ($item['acceptance']->assignedTo) + @if (($item['acceptance']->assignedTo) && ($item['acceptance']->assignedTo->email)) @csrf - + @else + + + + + @endif
diff --git a/resources/views/settings/branding.blade.php b/resources/views/settings/branding.blade.php index f37d6685b22a..a31d6e42ff2a 100644 --- a/resources/views/settings/branding.blade.php +++ b/resources/views/settings/branding.blade.php @@ -135,7 +135,7 @@ class="form-horizontal"

@@ -152,7 +152,7 @@ class="form-horizontal"

@@ -172,7 +172,7 @@ class="form-horizontal"
@@ -187,7 +187,7 @@ class="form-horizontal"

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

@@ -228,7 +228,7 @@ class="form-horizontal"

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

diff --git a/resources/views/settings/general.blade.php b/resources/views/settings/general.blade.php index fb85908761d3..d2e51d434145 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') }}
diff --git a/resources/views/settings/labels.blade.php b/resources/views/settings/labels.blade.php index 489c6ac584bb..e4fee660a4e7 100644 --- a/resources/views/settings/labels.blade.php +++ b/resources/views/settings/labels.blade.php @@ -144,7 +144,7 @@ class="table table-striped snipe-table"
@@ -189,7 +189,7 @@ class="table table-striped snipe-table"
@@ -480,23 +480,23 @@ class="form-control"
diff --git a/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php new file mode 100644 index 000000000000..deb3e07d2c01 --- /dev/null +++ b/tests/Feature/Notifications/Email/AssetAcceptanceReminderTest.php @@ -0,0 +1,108 @@ +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, + ]); + } +} diff --git a/tests/Unit/NotificationTest.php b/tests/Unit/NotificationTest.php index 3d5b3c5a7651..5b420a675385 100644 --- a/tests/Unit/NotificationTest.php +++ b/tests/Unit/NotificationTest.php @@ -7,9 +7,7 @@ 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 @@ public function testAUserIsEmailedIfTheyCheckoutAnAssetWithEULA() 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()