From 0d707a18512aa8e01c242d6d5486b675754ed5ed Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Mon, 4 Nov 2024 15:13:10 -0800 Subject: [PATCH 1/3] fixes notifications for licenses and asset to asset checkouables --- app/Listeners/CheckoutableListener.php | 71 ++++++++++++++----- app/Mail/CheckinLicenseMail.php | 5 +- app/Mail/CheckoutLicenseMail.php | 6 +- .../CheckinAssetNotification.php | 3 +- .../mail/markdown/checkin-license.blade.php | 12 ++-- .../mail/markdown/checkout-license.blade.php | 14 ++-- .../SlackNotificationsUponCheckinTest.php | 4 +- 7 files changed, 75 insertions(+), 40 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index f8c6c8baa9..0a0c586dae 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -16,6 +16,7 @@ use App\Models\CheckoutAcceptance; use App\Models\Component; use App\Models\Consumable; use App\Models\LicenseSeat; +use App\Models\Location; use App\Models\Setting; use App\Models\User; use App\Notifications\CheckinAccessoryNotification; @@ -60,14 +61,26 @@ class CheckoutableListener $adminCcEmailsArray = array_map('trim', explode(',', $adminCcEmail)); } $ccEmails = array_filter($adminCcEmailsArray); - $notifiable = $event->checkedOutTo; + $mailable = $this->getCheckoutMailType($event, $acceptance); + + if($event->checkedOutTo instanceof Asset){ + $event->checkedOutTo->load('assignedTo'); + $notifiable = $event->checkedOutTo->assignedto?->email ?? ''; + } + else if($event->checkedOutTo instanceof Location) { + $notifiable = $event->checkedOutTo->manager?->email ?? ''; + } + else{ + $notifiable = $event->checkedOutTo->email; + } + + if (!$event->checkedOutTo->locale){ + $mailable->locale($event->checkedOutTo->locale); + } + // Send email notifications try { - if (!$event->checkedOutTo->locale){ - $mailable->locale($event->checkedOutTo->locale); - } - /** * Send an email if any of the following conditions are met: * 1. The asset requires acceptance @@ -77,15 +90,20 @@ class CheckoutableListener if ($event->checkoutable->requireAcceptance() || $event->checkoutable->getEula() || (method_exists($event->checkoutable, 'checkin_email') && $event->checkoutable->checkin_email())) { - if (!empty($notifiable->email)) { + if (!empty($notifiable)) { Mail::to($notifiable)->cc($ccEmails)->send($mailable); - } else { + } elseif (!empty($ccEmails)) { Mail::cc($ccEmails)->send($mailable); } - Log::info('Sending email, Locale: ' . ($event->checkedOutTo->locale ?? 'default')); - } - + Log::info('Sending email, Locale: ' . ($event->checkedOutTo->locale ?? 'default')); + } + } catch (ClientException $e) { + Log::debug("Exception caught during checkout email: " . $e->getMessage()); + } catch (Exception $e) { + Log::debug("Exception caught during checkout email: " . $e->getMessage()); + } // Send Webhook notification + try{ if ($this->shouldSendWebhookNotification()) { if (Setting::getSettings()->webhook_selected === 'microsoft') { $message = $this->getCheckoutNotification($event)->toMicrosoftTeams(); @@ -137,38 +155,53 @@ class CheckoutableListener $adminCcEmailsArray = array_map('trim', explode(',', $adminCcEmail)); } $ccEmails = array_filter($adminCcEmailsArray); - $notifiable = $event->checkedOutTo; $mailable = $this->getCheckinMailType($event); + + if($event->checkedOutTo instanceof Asset){ + $event->checkedOutTo->load('assignedTo'); + $notifiable = $event->checkedOutTo->assignedto?->email ?? ''; + } + else if($event->checkedOutTo instanceof Location) { + $notifiable = $event->checkedOutTo->manager?->email ?? ''; + } + else{ + $notifiable = $event->checkedOutTo->email; + } + if (!$event->checkedOutTo->locale){ + $mailable->locale($event->checkedOutTo->locale); + } // Send email notifications try { - if (!$event->checkedOutTo->locale){ - $mailable->locale($event->checkedOutTo->locale); - } /** * Send an email if any of the following conditions are met: * 1. The asset requires acceptance * 2. The item has a EULA * 3. The item should send an email at check-in/check-out */ - if ($event->checkoutable->requireAcceptance() || $event->checkoutable->getEula() || (method_exists($event->checkoutable, 'checkin_email') && $event->checkoutable->checkin_email())) { - if (!empty($notifiable->email)) { + if (!empty($notifiable)) { Mail::to($notifiable)->cc($ccEmails)->send($mailable); - } else { + } elseif (!empty($ccEmails)){ Mail::cc($ccEmails)->send($mailable); } Log::info('Sending email, Locale: ' . $event->checkedOutTo->locale); } + } catch (ClientException $e) { + Log::debug("Exception caught during checkin email: " . $e->getMessage()); + } catch (Exception $e) { + Log::debug("Exception caught during checkin email: " . $e->getMessage()); + } - // Send Webhook notification + // Send Webhook notification + try { if ($this->shouldSendWebhookNotification()) { Notification::route(Setting::getSettings()->webhook_selected, Setting::getSettings()->webhook_endpoint) ->notify($this->getCheckinNotification($event)); } } catch (ClientException $e) { - Log::warning("Exception caught during checkout notification: " . $e->getMessage()); + Log::warning("Exception caught during checkin notification: " . $e->getMessage()); } catch (Exception $e) { Log::warning("Exception caught during checkin notification: " . $e->getMessage()); } diff --git a/app/Mail/CheckinLicenseMail.php b/app/Mail/CheckinLicenseMail.php index 97ce4dd3c3..8957f367ef 100644 --- a/app/Mail/CheckinLicenseMail.php +++ b/app/Mail/CheckinLicenseMail.php @@ -23,7 +23,7 @@ class CheckinLicenseMail extends Mailable public function __construct(LicenseSeat $licenseSeat, $checkedOutTo, User $checkedInBy, $note) { $this->target = $checkedOutTo; - $this->item = $licenseSeat->license; + $this->item = $licenseSeat; $this->admin = $checkedInBy; $this->note = $note; $this->settings = Setting::getSettings(); @@ -50,7 +50,8 @@ class CheckinLicenseMail extends Mailable return new Content( markdown: 'mail.markdown.checkin-license', with: [ - 'item' => $this->item, + 'license_seat' => $this->item, + 'license' => $this->item->license, 'admin' => $this->admin, 'note' => $this->note, 'target' => $this->target, diff --git a/app/Mail/CheckoutLicenseMail.php b/app/Mail/CheckoutLicenseMail.php index ecd0d7390d..7377ad340e 100644 --- a/app/Mail/CheckoutLicenseMail.php +++ b/app/Mail/CheckoutLicenseMail.php @@ -22,7 +22,7 @@ class CheckoutLicenseMail extends Mailable */ public function __construct(LicenseSeat $licenseSeat, $checkedOutTo, User $checkedOutBy, $acceptance, $note) { - $this->item = $licenseSeat->license; + $this->item = $licenseSeat; $this->admin = $checkedOutBy; $this->note = $note; $this->target = $checkedOutTo; @@ -53,11 +53,11 @@ class CheckoutLicenseMail extends Mailable $req_accept = method_exists($this->item, 'requireAcceptance') ? $this->item->requireAcceptance() : 0; $accept_url = is_null($this->acceptance) ? null : route('account.accept.item', $this->acceptance); - return new Content( markdown: 'mail.markdown.checkout-license', with: [ - 'item' => $this->item, + 'license_seat' => $this->item, + 'license' => $this->item->license, 'admin' => $this->admin, 'note' => $this->note, 'target' => $this->target, diff --git a/app/Notifications/CheckinAssetNotification.php b/app/Notifications/CheckinAssetNotification.php index 75077aeb46..fa4780c1fd 100644 --- a/app/Notifications/CheckinAssetNotification.php +++ b/app/Notifications/CheckinAssetNotification.php @@ -7,6 +7,7 @@ use App\Models\Asset; use App\Models\Setting; use App\Models\User; use Illuminate\Bus\Queueable; +use Illuminate\Notifications\Channels\SlackWebhookChannel; use Illuminate\Notifications\Messages\MailMessage; use Illuminate\Notifications\Messages\SlackMessage; use Illuminate\Notifications\Notification; @@ -62,7 +63,7 @@ class CheckinAssetNotification extends Notification } if (Setting::getSettings()->webhook_selected == 'slack' || Setting::getSettings()->webhook_selected == 'general' ) { Log::debug('use webhook'); - $notifyBy[] = 'slack'; + $notifyBy[] = SlackWebhookChannel::class; } return $notifyBy; diff --git a/resources/views/mail/markdown/checkin-license.blade.php b/resources/views/mail/markdown/checkin-license.blade.php index 0f14c4bfda..d6e146a962 100644 --- a/resources/views/mail/markdown/checkin-license.blade.php +++ b/resources/views/mail/markdown/checkin-license.blade.php @@ -6,15 +6,15 @@ @component('mail::table') | | | | ------------- | ------------- | -| **{{ trans('mail.asset_name') }}** | {{ $item->name }} | -@if (isset($item->manufacturer)) -| **{{ trans('general.manufacturer') }}** | {{ $item->manufacturer->name }} | +| **{{ trans('mail.asset_name') }}** | {{ $license->name }} | +@if (isset($license->manufacturer)) +| **{{ trans('general.manufacturer') }}** | {{ $license->manufacturer->name }} | @endif -@if ($target->can('update', $item)) -| **Key** | {{ $item->serial }} | +@if (($target instanceof \App\Models\User && $target->can('view', $license)) ||($target instanceof \App\Models\Asset && $license_seat->user->can('view', $license))) +| **Key** | {{ $license->serial }} | @endif @if (isset($item->category)) -| **{{ trans('general.category') }}** | {{ $item->category->name }} | +| **{{ trans('general.category') }}** | {{ $license->category->name }} | @endif @if ($admin) | **{{ trans('general.administrator') }}** | {{ $admin->present()->fullName() }} | diff --git a/resources/views/mail/markdown/checkout-license.blade.php b/resources/views/mail/markdown/checkout-license.blade.php index 08e1e5c763..2c4cd39cc4 100644 --- a/resources/views/mail/markdown/checkout-license.blade.php +++ b/resources/views/mail/markdown/checkout-license.blade.php @@ -9,15 +9,15 @@ @if (isset($checkout_date)) | **{{ trans('mail.checkout_date') }}** | {{ $checkout_date }} | @endif -| **{{ trans('general.license') }}** | {{ $item->name }} | -@if (isset($item->manufacturer)) -| **{{ trans('general.manufacturer') }}** | {{ $item->manufacturer->name }} | +| **{{ trans('general.license') }}** | {{ $license->name}} | +@if (isset($license->manufacturer)) +| **{{ trans('general.manufacturer') }}** | {{ $license->manufacturer->name }} | @endif -@if (isset($item->category)) -| **{{ trans('general.category') }}** | {{ $item->category->name }} | +@if (isset($license->category)) +| **{{ trans('general.category') }}** | {{ $license->category->name }} | @endif -@if ($target->can('view', $item)) -| **Key** | {{ $item->serial }} | +@if (($target instanceof \App\Models\User && $target->can('view', $license)) || ($target instanceof \App\Models\Asset && $license_seat->user->can('view', $license))) +| **Key** | {{ $license->serial }} | @endif @if ($note) | **{{ trans('mail.additional_notes') }}** | {{ $note }} | diff --git a/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php b/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php index adaede07ec..1de852597a 100644 --- a/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php +++ b/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php @@ -31,8 +31,8 @@ class SlackNotificationsUponCheckinTest extends TestCase public static function assetCheckInTargets(): array { return [ - 'Asset checked out to user' => [fn() => User::factory()->create()], - 'Asset checked out to asset' => [fn() => Asset::factory()->laptopMbp()->create()], +// 'Asset checked out to user' => [fn() => User::factory()->create()], +// 'Asset checked out to asset' => [fn() => Asset::factory()->laptopMbp()->create()], 'Asset checked out to location' => [fn() => Location::factory()->create()], ]; } From 93494ac55447b062b80918d84a80e38c5dbd2c3d Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Mon, 4 Nov 2024 15:17:58 -0800 Subject: [PATCH 2/3] put dataprovider back in for tests --- .../Webhooks/SlackNotificationsUponCheckinTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php b/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php index 1de852597a..adaede07ec 100644 --- a/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php +++ b/tests/Feature/Notifications/Webhooks/SlackNotificationsUponCheckinTest.php @@ -31,8 +31,8 @@ class SlackNotificationsUponCheckinTest extends TestCase public static function assetCheckInTargets(): array { return [ -// 'Asset checked out to user' => [fn() => User::factory()->create()], -// 'Asset checked out to asset' => [fn() => Asset::factory()->laptopMbp()->create()], + 'Asset checked out to user' => [fn() => User::factory()->create()], + 'Asset checked out to asset' => [fn() => Asset::factory()->laptopMbp()->create()], 'Asset checked out to location' => [fn() => Location::factory()->create()], ]; } From 2362cb5e5221ecae78c6aaf28bf8380f5ae20de2 Mon Sep 17 00:00:00 2001 From: Godfrey M Date: Mon, 4 Nov 2024 16:32:46 -0800 Subject: [PATCH 3/3] refactor to getNotifiables --- app/Listeners/CheckoutableListener.php | 39 ++++++++++---------------- 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/app/Listeners/CheckoutableListener.php b/app/Listeners/CheckoutableListener.php index 0a0c586dae..8fa96acb6a 100644 --- a/app/Listeners/CheckoutableListener.php +++ b/app/Listeners/CheckoutableListener.php @@ -61,24 +61,12 @@ class CheckoutableListener $adminCcEmailsArray = array_map('trim', explode(',', $adminCcEmail)); } $ccEmails = array_filter($adminCcEmailsArray); - $mailable = $this->getCheckoutMailType($event, $acceptance); - - if($event->checkedOutTo instanceof Asset){ - $event->checkedOutTo->load('assignedTo'); - $notifiable = $event->checkedOutTo->assignedto?->email ?? ''; - } - else if($event->checkedOutTo instanceof Location) { - $notifiable = $event->checkedOutTo->manager?->email ?? ''; - } - else{ - $notifiable = $event->checkedOutTo->email; - } + $notifiable = $this->getNotifiables($event); if (!$event->checkedOutTo->locale){ $mailable->locale($event->checkedOutTo->locale); } - // Send email notifications try { /** @@ -156,18 +144,8 @@ class CheckoutableListener } $ccEmails = array_filter($adminCcEmailsArray); $mailable = $this->getCheckinMailType($event); + $notifiable = $this->getNotifiables($event); - - if($event->checkedOutTo instanceof Asset){ - $event->checkedOutTo->load('assignedTo'); - $notifiable = $event->checkedOutTo->assignedto?->email ?? ''; - } - else if($event->checkedOutTo instanceof Location) { - $notifiable = $event->checkedOutTo->manager?->email ?? ''; - } - else{ - $notifiable = $event->checkedOutTo->email; - } if (!$event->checkedOutTo->locale){ $mailable->locale($event->checkedOutTo->locale); } @@ -311,6 +289,19 @@ class CheckoutableListener return new $mailable($event->checkoutable, $event->checkedOutTo, $event->checkedInBy, $event->note); } + private function getNotifiables($event){ + + if($event->checkedOutTo instanceof Asset){ + $event->checkedOutTo->load('assignedTo'); + return $event->checkedOutTo->assignedto?->email ?? ''; + } + else if($event->checkedOutTo instanceof Location) { + return $event->checkedOutTo->manager?->email ?? ''; + } + else{ + return $event->checkedOutTo->email; + } + } /** * Register the listeners for the subscriber.