Duplicate Discord Notifications and Exceptions

[FatihKoz]

We get exceptions while publishing news with latest dev build 7.0.0-dev+231224.6b9658

https://github.com/nabeelio/phpvms/blob/6b9658a4e37e193e00e5b1f4b1084130a64aaf1c/app/Notifications/Messages/NewsAdded.php#L51

https://github.com/nabeelio/phpvms/blob/6b9658a4e37e193e00e5b1f4b1084130a64aaf1c/app/Notifications/Messages/NewsAdded.php#L53

production.ERROR: Attempt to read property "ident" on null (app/Notifications/Messages/NewsAdded.php:51)
production.ERROR: Attempt to read property "name_private" on null (app/Notifications/Messages/NewsAdded.php:51)
production.ERROR: Call to a member function resolveAvatarUrl() on null (app/Notifications/Messages/NewsAdded.php:53)

When those calls are changed with fixed strings or dashed out, we get multiple/duplicate discord notifications (matching the count of users who opted in for non administrative mails)

By the mean time, a proper discord notification gets dispatched from Broadcasts (last message in above screenshot)

https://github.com/nabeelio/phpvms/blob/6b9658a4e37e193e00e5b1f4b1084130a64aaf1c/app/Notifications/Messages/Broadcast/NewsAdded.php#L31

So the question is, why do we have the same function in mail channel ?

Should we remove function from Messages/NewsAdded.php completely 'cause it is working properly when called from Messages/Broadcasts/NewsAdded.php ?

[arthurpar06]

Hi,

From my understanding of Laravel notifications, those in the "broadcast" folder are typically meant to be broadcasted to all users at once, while those outside might be specific to each user. Therefore, it seems like disabling the Discord channel for the Messages/NewsAdded.php notification would be a suitable approach. This way, we can continue sending emails to opt-in users through this notification, and broadcast the global notification to the Discord channel via the Messages/Broadcasts/NewsAdded.php notification.

[FatihKoz]

I think so but, still not understood why it is defined there in the first place ;)

Maybe an old implementation before expanding Discord notifications, or using laravel broadcasting and somehow got forgotten.

It is easy to dashout that section completely to deactivate, but not sure about the implementation, thus I did not provided a solution on purpose.

[arthurpar06]

Yeah, that's possible. I'm not quite sure what it does here either. If there were something related to Discord here, it would be something to send direct messages to users, which we don't do. It's worth trying, but for me, the solution is simply to remove the DiscordWebhook::class from here and just leave the 'mail' channel.

https://github.com/nabeelio/phpvms/blob/6b9658a4e37e193e00e5b1f4b1084130a64aaf1c/app/Notifications/Messages/NewsAdded.php#L32

[FatihKoz]

Yeah maybe, but if we intend to send private messages to users via Discord then we need more stuff there 😉

I just dashed out the entire function to open it up to discussion and test, your solution is easier too. Whichever we choose, after @nabeelio 's review we can pick one or both 😄