laravel / slack-notification-channel

Slack Notification Channel for laravel.
https://laravel.com/docs/notifications#slack-notifications
MIT License
869 stars 61 forks source link

Enable configurable slack webhook URLs #55

Closed ok200paul closed 2 years ago

ok200paul commented 2 years ago

This PR proposes an update to enable configurable slack webhook URLs, defined in the dev-userland.

In the Slack API docs it says here that:

You cannot override the default channel (chosen by the user who installed your app), username, or icon when you're using Incoming Webhooks to post messages. Instead, these values will always inherit from the associated Slack app configuration.

This PR proposes removing the above keys, and a BC update to Channels/SlackWebhookChannel.php that would inspect the incoming notification object for an alternative Slack webhook URL, and if present, route the notification to this channel. It might be a little premature, but the POC seems to work for me. The userland $notification object would ideally use named arguments to ensure correct parameter naming to ensure the $webhookUrl attribute was correctly specified, but I suppose we'd leave that up to the dev.

What I did:

SlackWebhookChannel::buildJsonPayload Removed keys channel, icon_emoji, icon_url, and username, as they are no longer accepted as valid input from the Slack Incoming Webhooks API

SlackWebhookChannel::send After the check to ensure that a default channel is set if (! $url = $notifiable->routeNotificationFor .., the function would check the notification object for an alternative URL to use, and route the notification there if specified.

Usage in Laravel

In the notification, if the user adds a nullable $webhookUrl parameter into their notification, the update to the package will take effect:

class SlackNotificationInLaravelApp extends Notification
{
    use Queueable;

    /**
     * Create a new notification instance.
     *
     * @return void
     */
    public function __construct(public ?string $webhookUrl = null)
    {
        //
    }

.. then to nominate a new webhook URL within the notification:

    $user = User::first();
    $user->notify(new SlackNotificationInLaravelApp(webhookUrl: 'https://hooks.slack.com/services/...'));

.. or to send to the default channel, just don't include the webhookUrl argument:

    $user = User::first();
    $user->notify(new SlackNotificationInLaravelApp());

Potential Alternative

If we implemented a slack config array (config/slack.php) where we entered a key-value array for the $channel key and the webhook url:

return [
    'general' => 'https://hooks.slack.com/services/...',
    'staff-channel' => 'https://hooks.slack.com/services/...',
];

.. there might be some way to re-leverage the to($channel) function in SlackMessage (as it's surplus to requirements now) to map a new webhook URL - if the method I'm proposing here seems a little anti-pattern or weird.

Lastly

Thanks so much for your work to date on this great package. It's served thousands of us really well over the years! I've been developing a long time but not very good at OSS contributions, so please do let me know if there's anything else I can add here. Hello from Melbourne!

taylorotwell commented 2 years ago

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

driesvints commented 2 years ago

Hey @ok200paul, I checked into this and I feel like at some point we definitely need to look for an update for this package. I'll add this PR to my personal todo's to check in at some point. I can't promise when that'll be but hopefully we can get this one and some more updates in at some point in the future.