Add support for Slack notifications #730

hanifn · 2024-06-11T11:10:00Z

Description

This PR adds support for Slack notifications on top of EditFlow's existing email notifications. Currently we're only planning to support Slack's incoming webhook API.

Steps to Test

You would need to create a Slack app on your own workspace and enable Incoming Webhooks for that app.

Check out PR.
Go to Notifications.
Enable Send to Webhook and add in the webhook URL.
Save settings
Change a post status or add editorial comments.
Verify that Slack messages were sent to the right channel on your Slack workspace

modules/notifications/notifications.php

alecgeatches · 2024-06-11T16:49:17Z

modules/notifications/notifications.php

+		}
+
+		// Set up the payload
+		$payload = array(


Suuuper minor nit, but can we use array literal notation [/] in new code instead of the array() syntax?

As I've mentioned in another comment, I've opted to stick to current conventions in existing code to make it more consistent.

Def will echo what Alec said - our goal should not be consistency, but modernizing the code that we touch to the new 2024 standards. It's going to mean discrepancy between files until we finish our work, but the end result will be a slow march to modernization.

Hmm looks like our phpcs settings are not allowing the array literal notation: Short array syntax is not allowed

You might need to update that to be similar to the governance plugin here

Are you saying we should override the existing WordPress-VIP-Go ruleset to support short array syntax?

Yeah, that's what I'm saying. Adding that exclusion rule that I linked will allow you to use the short array syntax

Added in 34906df

modules/notifications/notifications.php

hanifn · 2024-06-13T14:23:56Z

So it turns out I had to refactor the entire file anyway because my initial changes triggered some indentation errors in the phpcs diff checks. Simply fixing the indentation on a few select lines obviously wouldn't work so I had to fix the indent of the entire class which then increased the size of the diff so I had to fix everything. Now the only things remaining are some Processing form data without nonce verification. errors which I'm unsure how to fix. Either because the nonce check was already done earlier in the method or the endpoints weren't receiving a nonce in the first place. 🤔

ingeniumed

None of the below points have to be done in this PR, they are for follow up PRs.

Some notes I noted while testing the UI:

The UI is tuned towards sending email notifications to a user or group. If none of those are to checked then there is a default message of "no one will be notified" that's to be displayed. In the follow up PRs this is something we should address if send to webhook is enabled.
For cleaning up the email UI, I'm thinking we introduce a checkbox that says email enabled whose default value is true. If you want, you can flip it to false and that will hide that meta box entirely.

We can make the webhook functionality a bit more robust with the following:

We should have some sort of disabling functionality for the webhook, in the event that it fails more than once. That way, we don't need to retry it and they'd need to go in and fix the URL or the problem instead.
We should also introduce a retry functionality
Right now it's a synchronous call if I'm correct. Could we consider switching to an async call so it's not blocking?

blocks/dist/custom-status.build.js

modules/notifications/lib/notifications.js

ingeniumed · 2024-06-17T00:39:12Z

modules/notifications/notifications.php

+				$user = get_userdata( $user )->user_login;
+			}
+
+			$post_args = array(


Hitting save on VSCode for me changes the formatting slightly for this block of code. Running PHPCS doesn't bring up anything though, so I think this might be prettify doing that?

ingeniumed · 2024-06-17T00:40:49Z

modules/notifications/notifications.php

+			$args             = array(
+				'title'                 => __( 'Notifications', 'edit-flow' ),
+				'short_description'     => __( 'Update your team of important changes to your content.', 'edit-flow' ),
+				'extended_description'  => __( 'With email notifications, you can keep everyone updated about what’s happening with a given content. Each status change or editorial comment sends out an email notification to users subscribed to a post. User groups can be used to manage who receives notifications on what. With webhook notifications, all notifications will also be sent to the specified webhook URL(i.e.: Slack incoming webhooks).', 'edit-flow' ),


This description should be tweaked a touch to reflect that notifications is the purpose of this module and that's done in two ways:

Email to a group or a batch of users

Webhook which isn't piped to a specific group or a batch

Right now that's slightly confusing, since webhook flow doesn't support users/groups.

Changed in 69676fd

ingeniumed · 2024-06-17T01:19:39Z

modules/notifications/notifications.php

-			// Calendar and Story Budget
-			add_filter( 'ef_calendar_item_actions', array( $this, 'filter_post_row_actions' ), 10, 2 );
-			add_filter( 'ef_story_budget_item_actions', array( $this, 'filter_post_row_actions' ), 10, 2 );
+				'settings_help_sidebar' => __( '<p><strong>For more information:</strong></p><p><a href="http://editflow.org/features/notifications/">Notifications Documentation</a></p><p><a href="http://wordpress.org/tags/edit-flow?forum_id=10">Edit Flow Forum</a></p><p><a href="https://github.com/danielbachhuber/Edit-Flow">Edit Flow on Github</a></p>', 'edit-flow' ),


Minor: We should change the danielbachhuber to automattic here.

Changed in 69676fd

ingeniumed · 2024-06-17T01:46:53Z

modules/notifications/notifications.php

+					'headers' => array( 'Content-Type' => 'application/json' ),
+				)
+			);
+			if ( is_wp_error( $response ) ) {


I tried to test the error pathway by giving an invalid webhook url, and the commenting system just hangs as a result. It seems like it's not able to parse the ajax response, and as a result it just gets stuck on an endless spinner. This seems like it's happening in the editorial-comments.js.

In addition, should we print the ajax response? Wondering if that would reveal some sensitive info, and instead if we should just say that "Sending to webhook failed" and instead send some extra debugging info to the error logs

I've fixed the error handling to not hang the UI in 57a6806 but the error display is kinda terrible. I've tried to style it as best as I can but I'm still no happy with it.

What if we ran a check when the webhook was saved, so we could show the error message then and there? That way, it's easy to prevent a save if the webhook is invalid and we could change this to something like "Unable to send notification to webhook provided"

I'm not sure what you mean by that @ingeniumed, can you give an example?

So what I'm suggesting is two fold:

When the slack webhook url is provided, and the save button is pressed we run a test on that url. We post a test message that says "Integrated successfully with EditFlow". If this doesn't happen, the save fails then and there. This will reduce the chances of failures happening down the line (doesn't eliminate it but sure helps).

When a notification fails to be sent in the block editor, we change the error that's shown to be more specific i.e. "Unable to send notification to webhook provided".

Hope that clears up what I mean.

Gotcha. I get it now 👍

Added and additional filter to filter URL if necessary and added some contextual objects to make the filters more useful.

Changed to be more inline with the VIP governance plugin

ingeniumed · 2024-06-24T00:06:55Z

modules/notifications/notifications.php

+			<div id="ef-post_following_box">
+				<a name="subscriptions"></a>
+
+				<p><?php _e( 'Select the users and user groups that should receive notifications when the status of this post is updated or when an editorial comment is added.', 'edit-flow' ); ?></p>


We should add email notifications here so it's clear that webhook doesn't count.

Changed in 902bc04

This reverts commit 81fa03c.

Specifically mention that users and user groups are for email notfications

ingeniumed

🚢

Add slack notifications

ec5e133

hanifn added Enhancement Module: Notifications labels Jun 11, 2024

hanifn requested review from ingeniumed and alecgeatches June 11, 2024 11:10

hanifn self-assigned this Jun 11, 2024

hanifn changed the base branch from master to develop June 11, 2024 11:10

alecgeatches reviewed Jun 11, 2024

View reviewed changes

modules/notifications/notifications.php Outdated Show resolved Hide resolved

alecgeatches reviewed Jun 11, 2024

View reviewed changes

modules/notifications/notifications.php Outdated Show resolved Hide resolved

alecgeatches reviewed Jun 11, 2024

View reviewed changes

modules/notifications/notifications.php Outdated Show resolved Hide resolved

alecgeatches reviewed Jun 11, 2024

View reviewed changes

modules/notifications/notifications.php Outdated Show resolved Hide resolved

hanifn added 2 commits June 12, 2024 14:09

Escape form element names

8bf4f9f

Simplify slack alert texts

31ab216

ingeniumed reviewed Jun 13, 2024

View reviewed changes

modules/notifications/notifications.php Outdated Show resolved Hide resolved

modules/notifications/notifications.php Outdated Show resolved Hide resolved

hanifn added 8 commits June 13, 2024 12:04

Switch to generic webhook notifications

2fb73da

Hide webhook url input if it’s disabled

8d2487e

Fix phpcs errors

48bda0c

Fix more phpcs errors

79e565a

Add filters for webhook message and payload

d7962a1

Rename methods and setting names

d3bd8e7

Fix phpcs errors

4df9210

More phpcs fixes

4ce2556

hanifn marked this pull request as ready for review June 13, 2024 13:59

hanifn added 4 commits June 14, 2024 11:03

Fix repeat fetching of POST variables

d757db0

Use single string literal for i18n function

0d6ddf2

Verify nonce before saving ef post subs

8c0adc7

Update module description

5919a69

ingeniumed reviewed Jun 17, 2024

View reviewed changes

ingeniumed and others added 10 commits June 17, 2024 12:24

Run the production build for the js/css files

d410b67

Fix editorial comment error handling

57a6806

Fix phpcs errors

331166c

Refactor send to webhook method for versatility

586e779

Added and additional filter to filter URL if necessary and added some contextual objects to make the filters more useful.

Update module descriptions

69676fd

Add some TODOs

6458d6b

Update PHPCS ruleset

34906df

Changed to be more inline with the VIP governance plugin

Use short array syntax instead

a551fcd

Change webhook error message

653f3d2

Add form validation for webhook settings

81fa03c

ingeniumed reviewed Jun 24, 2024

View reviewed changes

hanifn added 4 commits June 24, 2024 10:45

Revert "Add form validation for webhook settings"

caf723f

This reverts commit 81fa03c.

Update notifications meta box description

902bc04

Specifically mention that users and user groups are for email notfications

Sanitize webhook URL

a540e18

Refactor validation for brevity

157ec5c

ingeniumed approved these changes Jun 24, 2024

View reviewed changes

hanifn merged commit 289cfe6 into develop Jun 24, 2024
2 of 3 checks passed

hanifn deleted the add/slack-notifications branch June 24, 2024 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Slack notifications #730

Add support for Slack notifications #730

hanifn commented Jun 11, 2024 •

edited

Loading

alecgeatches Jun 11, 2024

hanifn Jun 12, 2024

ingeniumed Jun 13, 2024

hanifn Jun 13, 2024

ingeniumed Jun 20, 2024

hanifn Jun 21, 2024

ingeniumed Jun 21, 2024

hanifn Jun 21, 2024

hanifn commented Jun 13, 2024

ingeniumed left a comment

ingeniumed Jun 17, 2024

ingeniumed Jun 17, 2024

hanifn Jun 21, 2024

ingeniumed Jun 17, 2024

hanifn Jun 21, 2024

ingeniumed Jun 17, 2024

ingeniumed Jun 17, 2024

hanifn Jun 19, 2024

ingeniumed Jun 20, 2024

hanifn Jun 21, 2024

ingeniumed Jun 21, 2024

hanifn Jun 21, 2024

ingeniumed Jun 24, 2024

hanifn Jun 24, 2024

ingeniumed left a comment

Add support for Slack notifications #730

Add support for Slack notifications #730

Conversation

hanifn commented Jun 11, 2024 • edited Loading

Description

Steps to Test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanifn commented Jun 13, 2024

ingeniumed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingeniumed left a comment

Choose a reason for hiding this comment

hanifn commented Jun 11, 2024 •

edited

Loading