From cb38f2ad90b17ca8f79c48b81189649a40a6d228 Mon Sep 17 00:00:00 2001 From: Nabeel S Date: Tue, 8 Feb 2022 15:07:02 -0500 Subject: [PATCH] Refactor broadcast notifications (#1402) * Refactor broadcast notifications * StyleCI fixes * Fix the planned_distance accidental changes --- app/Contracts/Notification.php | 21 ------ .../Channels/Discord/DiscordWebhook.php | 15 +++- .../Messages/AdminUserRegistered.php | 29 ++------ .../Messages/Broadcast/NewsAdded.php | 57 +++++++++++++++ .../PirepFiled.php} | 43 ++++------- .../{ => Broadcast}/PirepPrefiled.php | 2 +- .../{ => Broadcast}/PirepStatusChanged.php | 11 +-- .../Messages/Broadcast/UserRegistered.php | 73 +++++++++++++++++++ app/Notifications/Messages/PirepFiled.php | 53 ++++++++++++++ .../NotificationEventsHandler.php | 32 +++++--- app/Providers/AppServiceProvider.php | 7 ++ app/Services/PirepService.php | 4 +- config/notifications.php | 4 +- tests/PIREPTest.php | 16 ++-- tests/RegistrationTest.php | 19 ++--- tests/TestCase.php | 3 +- tests/TestData.php | 20 +++++ 17 files changed, 290 insertions(+), 119 deletions(-) create mode 100644 app/Notifications/Messages/Broadcast/NewsAdded.php rename app/Notifications/Messages/{PirepSubmitted.php => Broadcast/PirepFiled.php} (60%) rename app/Notifications/Messages/{ => Broadcast}/PirepPrefiled.php (98%) rename app/Notifications/Messages/{ => Broadcast}/PirepStatusChanged.php (94%) create mode 100644 app/Notifications/Messages/Broadcast/UserRegistered.php create mode 100644 app/Notifications/Messages/PirepFiled.php diff --git a/app/Contracts/Notification.php b/app/Contracts/Notification.php index 0a7a1750..e58932af 100644 --- a/app/Contracts/Notification.php +++ b/app/Contracts/Notification.php @@ -2,7 +2,6 @@ namespace App\Contracts; -use App\Notifications\Channels\Discord\DiscordMessage; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; @@ -26,24 +25,4 @@ class Notification extends \Illuminate\Notifications\Notification implements Sho $this->channels = $notif_config[$klass];*/ } - - /** - * Get the notification's delivery channels. - * - * @param mixed $notifiable - * - * @return array - */ - /*public function via($notifiable) - { - return $this->channels; - }*/ - - /** - * @return DiscordMessage|null - */ - public function toDiscordChannel($notifiable): ?DiscordMessage - { - return null; - } } diff --git a/app/Notifications/Channels/Discord/DiscordWebhook.php b/app/Notifications/Channels/Discord/DiscordWebhook.php index bb0746c2..beb02f3a 100644 --- a/app/Notifications/Channels/Discord/DiscordWebhook.php +++ b/app/Notifications/Channels/Discord/DiscordWebhook.php @@ -20,14 +20,23 @@ class DiscordWebhook public function send($notifiable, Notification $notification) { $message = $notification->toDiscordChannel($notifiable); - if ($message === null || empty($message->webhook_url)) { - Log::debug('Discord notifications not configured, skipping'); + if ($message === null) { + //Log::debug('Discord notifications not configured, skipping'); return; } + $webhook_url = $message->webhook_url; + if (empty($webhook_url)) { + $webhook_url = setting('notifications.discord_private_webhook_url'); + if (empty($webhook_url)) { + //Log::debug('Discord notifications not configured, skipping'); + return; + } + } + try { $data = $message->toArray(); - $this->httpClient->post($message->webhook_url, $data); + $this->httpClient->post($webhook_url, $data); } catch (RequestException $e) { $request = Psr7\Message::toString($e->getRequest()); $response = Psr7\Message::toString($e->getResponse()); diff --git a/app/Notifications/Messages/AdminUserRegistered.php b/app/Notifications/Messages/AdminUserRegistered.php index 827b2064..6a90fe8a 100644 --- a/app/Notifications/Messages/AdminUserRegistered.php +++ b/app/Notifications/Messages/AdminUserRegistered.php @@ -4,8 +4,6 @@ namespace App\Notifications\Messages; use App\Contracts\Notification; use App\Models\User; -use App\Notifications\Channels\Discord\DiscordMessage; -use App\Notifications\Channels\Discord\DiscordWebhook; use App\Notifications\Channels\MailChannel; use Illuminate\Contracts\Queue\ShouldQueue; @@ -32,32 +30,21 @@ class AdminUserRegistered extends Notification implements ShouldQueue ); } + /** + * @param $notifiable + * + * @return string[] + */ public function via($notifiable) { - return ['mail', DiscordWebhook::class]; + return ['mail']; } /** - * Send a Discord notification - * - * @param User $pirep - * @param mixed $user + * @param $notifiable * - * @return DiscordMessage|null + * @return array */ - public function toDiscordChannel($user): ?DiscordMessage - { - if (empty(setting('notifications.discord_private_webhook_url'))) { - return null; - } - - $dm = new DiscordMessage(); - return $dm->webhook(setting('notifications.discord_private_webhook_url')) - ->success() - ->title('New User Registered: '.$user->ident) - ->fields([]); - } - public function toArray($notifiable) { return [ diff --git a/app/Notifications/Messages/Broadcast/NewsAdded.php b/app/Notifications/Messages/Broadcast/NewsAdded.php new file mode 100644 index 00000000..6e22296c --- /dev/null +++ b/app/Notifications/Messages/Broadcast/NewsAdded.php @@ -0,0 +1,57 @@ +news = $news; + } + + public function via($notifiable) + { + return ['discord_webhook']; + } + + /** + * @param News $news + * + * @return DiscordMessage|null + */ + public function toDiscordChannel($news): ?DiscordMessage + { + $dm = new DiscordMessage(); + return $dm->success() + ->title('News: '.$news->subject) + ->author([ + 'name' => $news->user->ident.' - '.$news->user->name_private, + 'url' => '', + 'icon_url' => $news->user->resolveAvatarUrl(), + ]) + ->description($news->body); + } + + /** + * Get the array representation of the notification. + * + * @param mixed $notifiable + * + * @return array + */ + public function toArray($notifiable) + { + return [ + 'news_id' => $this->news->id, + ]; + } +} diff --git a/app/Notifications/Messages/PirepSubmitted.php b/app/Notifications/Messages/Broadcast/PirepFiled.php similarity index 60% rename from app/Notifications/Messages/PirepSubmitted.php rename to app/Notifications/Messages/Broadcast/PirepFiled.php index 9ce8e626..bf4afe76 100644 --- a/app/Notifications/Messages/PirepSubmitted.php +++ b/app/Notifications/Messages/Broadcast/PirepFiled.php @@ -1,22 +1,18 @@ pirep = $pirep; - - $this->setMailable( - 'New PIREP Submitted', - 'notifications.mail.admin.pirep.submitted', - ['pirep' => $this->pirep] - ); } public function via($notifiable) { - return ['mail', DiscordWebhook::class]; + return ['discord_webhook']; } /** @@ -51,35 +41,30 @@ class PirepSubmitted extends Notification implements ShouldQueue */ public function toDiscordChannel($pirep): ?DiscordMessage { - if (empty(setting('notifications.discord_public_webhook_url'))) { - return null; - } - $title = 'Flight '.$pirep->ident.' Filed'; $fields = [ - 'Flight' => $pirep->ident, - 'Departure Airport' => $pirep->dpt_airport_id, - 'Arrival Airport' => $pirep->arr_airport_id, - 'Equipment' => $pirep->aircraft->ident, - 'Flight Time (Planned)' => Time::minutesToTimeString($pirep->planned_flight_time), + 'Flight' => $pirep->ident, + 'Departure Airport' => $pirep->dpt_airport_id, + 'Arrival Airport' => $pirep->arr_airport_id, + 'Equipment' => $pirep->aircraft->ident, + 'Flight Time' => Time::minutesToTimeString($pirep->flight_time), ]; - if ($pirep->planned_distance) { + if ($pirep->distance) { try { - $planned_distance = new Distance( - $pirep->planned_distance, + $distance = new Distance( + $pirep->distance, config('phpvms.internal_units.distance') ); - $pd = $planned_distance[$planned_distance->unit].' '.$planned_distance->unit; - $fields['Distance (Planned)'] = $pd; + $pd = $distance[$distance->unit].' '.$distance->unit; + $fields['Distance'] = $pd; } catch (NonNumericValue|NonStringUnitName $e) { } } $dm = new DiscordMessage(); - return $dm->webhook(setting('notifications.discord_public_webhook_url')) - ->success() + return $dm->success() ->title($title) ->description($pirep->user->discord_id ? 'Flight by <@'.$pirep->user->discord_id.'>' : '') ->url(route('frontend.pireps.show', [$pirep->id])) diff --git a/app/Notifications/Messages/PirepPrefiled.php b/app/Notifications/Messages/Broadcast/PirepPrefiled.php similarity index 98% rename from app/Notifications/Messages/PirepPrefiled.php rename to app/Notifications/Messages/Broadcast/PirepPrefiled.php index 83f98c54..6f1b27eb 100644 --- a/app/Notifications/Messages/PirepPrefiled.php +++ b/app/Notifications/Messages/Broadcast/PirepPrefiled.php @@ -1,6 +1,6 @@ ident.' '.self::$verbs[$pirep->status]; $fields = [ @@ -112,8 +110,7 @@ class PirepStatusChanged extends Notification implements ShouldQueue } $dm = new DiscordMessage(); - return $dm->webhook(setting('notifications.discord_public_webhook_url')) - ->success() + return $dm->success() ->title($title) ->description($pirep->user->discord_id ? 'Flight by <@'.$pirep->user->discord_id.'>' : '') ->url(route('frontend.pireps.show', [$pirep->id])) diff --git a/app/Notifications/Messages/Broadcast/UserRegistered.php b/app/Notifications/Messages/Broadcast/UserRegistered.php new file mode 100644 index 00000000..cea0bada --- /dev/null +++ b/app/Notifications/Messages/Broadcast/UserRegistered.php @@ -0,0 +1,73 @@ +user = $user; + } + + /** + * @param $notifiable + * + * @return string[] + */ + public function via($notifiable) + { + return ['discord_webhook']; + } + + /** + * Send a Discord notification + * + * @param $notifiable + * + * @return DiscordMessage|null + */ + public function toDiscordChannel($notifiable): ?DiscordMessage + { + $dm = new DiscordMessage(); + return $dm->success() + ->title('New User Registered: '.$this->user->ident) + ->author([ + 'name' => $this->user->ident.' - '.$this->user->name_private, + 'url' => '', + 'icon_url' => $this->user->resolveAvatarUrl(), + ]) + ->fields([]); + } + + /** + * @param $notifiable + * + * @return array + */ + public function toArray($notifiable) + { + return [ + 'user_id' => $this->user->id, + ]; + } +} diff --git a/app/Notifications/Messages/PirepFiled.php b/app/Notifications/Messages/PirepFiled.php new file mode 100644 index 00000000..874461bd --- /dev/null +++ b/app/Notifications/Messages/PirepFiled.php @@ -0,0 +1,53 @@ +pirep = $pirep; + + $this->setMailable( + 'New PIREP Submitted', + 'notifications.mail.admin.pirep.submitted', + ['pirep' => $this->pirep] + ); + } + + public function via($notifiable) + { + return ['mail']; + } + + /** + * Get the array representation of the notification. + * + * @param mixed $notifiable + * + * @return array + */ + public function toArray($notifiable) + { + return [ + 'pirep_id' => $this->pirep->id, + 'user_id' => $this->pirep->user_id, + ]; + } +} diff --git a/app/Notifications/NotificationEventsHandler.php b/app/Notifications/NotificationEventsHandler.php index 86b9d453..bd0199e9 100644 --- a/app/Notifications/NotificationEventsHandler.php +++ b/app/Notifications/NotificationEventsHandler.php @@ -134,10 +134,10 @@ class NotificationEventsHandler extends Listener */ $this->notifyAdmins(new Messages\AdminUserRegistered($event->user)); - /** - * Discord and other notifications + /* + * Broadcast notifications */ - Notification::send([$event->user], new Messages\AdminUserRegistered($event->user)); + Notification::send([$event->user], new Messages\Broadcast\UserRegistered($event->user)); } /** @@ -166,16 +166,20 @@ class NotificationEventsHandler extends Listener public function onPirepPrefile(PirepPrefiled $event): void { Log::info('NotificationEvents::onPirepPrefile: '.$event->pirep->id.' prefiled'); - Notification::send([$event->pirep], new Messages\PirepPrefiled($event->pirep)); + + /* + * Broadcast notifications + */ + Notification::send([$event->pirep], new Messages\Broadcast\PirepPrefiled($event->pirep)); } /** - * Status Change notification + * Status Change notification. Disabled for now, too noisy */ public function onPirepStatusChange(PirepStatusChange $event): void { - Log::info('NotificationEvents::onPirepStatusChange: '.$event->pirep->id.' prefiled'); - Notification::send([$event->pirep], new Messages\PirepStatusChanged($event->pirep)); + // Log::info('NotificationEvents::onPirepStatusChange: '.$event->pirep->id.' prefiled'); + // Notification::send([$event->pirep], new Messages\Discord\PirepStatusChanged($event->pirep)); } /** @@ -186,8 +190,12 @@ class NotificationEventsHandler extends Listener public function onPirepFile(PirepFiled $event): void { Log::info('NotificationEvents::onPirepFile: '.$event->pirep->id.' filed'); - $this->notifyAdmins(new Messages\PirepSubmitted($event->pirep)); - Notification::send([$event->pirep], new Messages\PirepSubmitted($event->pirep)); + $this->notifyAdmins(new Messages\PirepFiled($event->pirep)); + + /* + * Broadcast notifications + */ + Notification::send([$event->pirep], new Messages\Broadcast\PirepFiled($event->pirep)); } /** @@ -221,6 +229,10 @@ class NotificationEventsHandler extends Listener { Log::info('NotificationEvents::onNewsAdded'); $this->notifyAllUsers(new Messages\NewsAdded($event->news)); - Notification::send([$event->news], new Messages\NewsAdded($event->news)); + + /* + * Broadcast notifications + */ + Notification::send([$event->news], new Messages\Broadcast\NewsAdded($event->news)); } } diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index fcc6e1bf..28a71608 100755 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -2,10 +2,12 @@ namespace App\Providers; +use App\Notifications\Channels\Discord\DiscordWebhook; use App\Services\ModuleService; use App\Support\ThemeViewFinder; use App\Support\Utils; use Illuminate\Pagination\Paginator; +use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\Schema; use Illuminate\Support\Facades\View; use Illuminate\Support\ServiceProvider; @@ -15,8 +17,13 @@ class AppServiceProvider extends ServiceProvider public function boot(): void { Schema::defaultStringLength(191); + Paginator::useBootstrap(); View::share('moduleSvc', app(ModuleService::class)); + + Notification::extend('discord_webhook', function ($app) { + return app(DiscordWebhook::class); + }); } /** diff --git a/app/Services/PirepService.php b/app/Services/PirepService.php index 66fc927c..3d4c4d1a 100644 --- a/app/Services/PirepService.php +++ b/app/Services/PirepService.php @@ -243,7 +243,7 @@ class PirepService extends Service // Copy some fields over from Flight if we have it if ($pirep->flight) { - $pirep->planned_distance = $pirep->flight->distance; + $pirep->planned_distance = $pirep->flight->planned_distance; $pirep->planned_flight_time = $pirep->flight->flight_time; } @@ -336,7 +336,7 @@ class PirepService extends Service // Copy some fields over from Flight if we have it if ($pirep->flight) { - $pirep->planned_distance = $pirep->flight->distance; + $pirep->distance = $pirep->flight->distance; $pirep->planned_flight_time = $pirep->flight->flight_time; } diff --git a/config/notifications.php b/config/notifications.php index 069d225b..f2d1fd5a 100644 --- a/config/notifications.php +++ b/config/notifications.php @@ -3,8 +3,8 @@ use App\Notifications\Messages\AdminUserRegistered; use App\Notifications\Messages\NewsAdded; use App\Notifications\Messages\PirepAccepted; +use App\Notifications\Messages\PirepFiled; use App\Notifications\Messages\PirepRejected; -use App\Notifications\Messages\PirepSubmitted; use App\Notifications\Messages\UserPending; use App\Notifications\Messages\UserRegistered; use App\Notifications\Messages\UserRejected; @@ -18,7 +18,7 @@ return [ NewsAdded::class => ['mail'], PirepAccepted::class => ['mail'], PirepRejected::class => ['mail'], - PirepSubmitted::class => ['mail'], + PirepFiled::class => ['mail'], UserPending::class => ['mail'], UserRegistered::class => ['mail'], UserRejected::class => ['mail'], diff --git a/tests/PIREPTest.php b/tests/PIREPTest.php index 2f52d0df..866fa704 100644 --- a/tests/PIREPTest.php +++ b/tests/PIREPTest.php @@ -14,13 +14,12 @@ use App\Models\Pirep; use App\Models\Rank; use App\Models\User; use App\Notifications\Messages\PirepAccepted; -use App\Notifications\Messages\PirepSubmitted; +use App\Notifications\Messages\PirepFiled; use App\Repositories\SettingRepository; use App\Services\AircraftService; use App\Services\BidService; use App\Services\FlightService; use App\Services\PirepService; -use App\Services\UserService; use App\Support\Units\Fuel; use Carbon\Carbon; use Illuminate\Support\Facades\Notification; @@ -156,6 +155,7 @@ class PIREPTest extends TestCase */ public function testUnitFields() { + /** @var Pirep $pirep */ $pirep = $this->createPirep(); $pirep->save(); @@ -251,16 +251,13 @@ class PIREPTest extends TestCase { /** @var User $user */ $user = factory(User::class)->create([ + 'name' => 'testPirepNotifications user', 'flights' => 0, 'flight_time' => 0, 'rank_id' => 1, ]); - $admin = factory(User::class)->create(); - - /** @var UserService $userSvc */ - $userSvc = app(UserService::class); - $userSvc->addUserToRole($admin, 'admin'); + $admin = $this->createAdminUser(['name' => 'testPirepNotifications Admin']); $pirep = factory(Pirep::class)->create([ 'airline_id' => 1, @@ -271,7 +268,8 @@ class PIREPTest extends TestCase $this->pirepSvc->submit($pirep); // Make sure a notification was sent out to the admin - Notification::assertSentTo([$admin], PirepSubmitted::class); + Notification::assertSentTo([$admin], PirepFiled::class); + Notification::assertNotSentTo([$user], PirepFiled::class); } /** @@ -281,6 +279,7 @@ class PIREPTest extends TestCase { $this->updateSetting('pilots.count_transfer_hours', false); + /** @var User $user */ $user = factory(User::class)->create([ 'flights' => 0, 'flight_time' => 0, @@ -304,6 +303,7 @@ class PIREPTest extends TestCase $this->pirepSvc->accept($pirep); } + /** @var User $pilot */ $pilot = User::find($user->id); $last_pirep = Pirep::where('id', $pilot->last_pirep_id)->first(); diff --git a/tests/RegistrationTest.php b/tests/RegistrationTest.php index f539000e..eb471c0b 100644 --- a/tests/RegistrationTest.php +++ b/tests/RegistrationTest.php @@ -2,11 +2,10 @@ namespace Tests; -use App\Events\UserRegistered; use App\Models\Enums\UserState; use App\Models\User; +use App\Notifications\Messages\AdminUserRegistered; use App\Services\UserService; -use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\Notification; @@ -21,13 +20,12 @@ class RegistrationTest extends TestCase */ public function testRegistration() { - Event::fake(); - Notification::fake(); + $admin = $this->createAdminUser(['name' => 'testRegistration Admin']); /** @var UserService $userSvc */ $userSvc = app(UserService::class); - setting('pilots.auto_accept', true); + $this->updateSetting('pilots.auto_accept', true); $attrs = factory(User::class)->make()->makeVisible(['api_key', 'name', 'email'])->toArray(); $attrs['password'] = Hash::make('secret'); @@ -35,14 +33,7 @@ class RegistrationTest extends TestCase $this->assertEquals(UserState::ACTIVE, $user->state); - Event::assertDispatched(UserRegistered::class, function ($e) use ($user) { - return $e->user->id === $user->id - && $e->user->state === $user->state; - }); - - /*Notification::assertSentTo( - [$user], - \App\Notifications\Messages\UserRegistered::class - );*/ + Notification::assertSentTo([$admin], AdminUserRegistered::class); + Notification::assertNotSentTo([$user], AdminUserRegistered::class); } } diff --git a/tests/TestCase.php b/tests/TestCase.php index f2fdd1fc..5aeb6fab 100755 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -58,7 +58,7 @@ abstract class TestCase extends \Illuminate\Foundation\Testing\TestCase ); Artisan::call('database:create', ['--reset' => true]); - Artisan::call('migrate:refresh', ['--env' => 'testing', '--force' => true]); + Artisan::call('migrate', ['--env' => 'testing', '--force' => true]); Notification::fake(); // $this->disableExceptionHandling(); @@ -71,6 +71,7 @@ abstract class TestCase extends \Illuminate\Foundation\Testing\TestCase protected function disableExceptionHandling() { $this->app->instance(ExceptionHandler::class, new class() extends Handler { + /** @noinspection PhpMissingParentConstructorInspection */ public function __construct() { } diff --git a/tests/TestData.php b/tests/TestData.php index 4c71ba92..033cf9f8 100644 --- a/tests/TestData.php +++ b/tests/TestData.php @@ -9,6 +9,7 @@ use App\Models\Pirep; use App\Models\Role; use App\Models\Subfleet; use App\Models\User; +use App\Services\UserService; use Exception; trait TestData @@ -32,6 +33,25 @@ trait TestData ], $attrs)); } + /** + * Create an admin user + * + * @param array $attrs + * + * @return User + */ + public function createAdminUser(array $attrs = []): User + { + /** @var User $admin */ + $admin = factory(User::class)->create($attrs); + + /** @var UserService $userSvc */ + $userSvc = app(UserService::class); + $userSvc->addUserToRole($admin, 'admin'); + + return $admin; + } + /** * Create a new PIREP with a proper subfleet/rank/user and an * aircraft that the user is allowed to fly