Add support for remote notifications #1170

aaemnnosttv · 2020-02-25T12:06:32Z

Summary

Addresses issue #1110

Relevant technical choices

Proxy API responses are mocked during E2E
Added dompurify to allow for limited HTML in content passed to notification description
Increased bundle size limit by 10KB (see Optimize entrypoint bundle sizes #1180)
~~Updated VRT due to visual changes (see Add support for remote notifications #1170 (comment))~~

Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

aaemnnosttv · 2020-02-25T16:13:04Z

includes/Core/Notifications/Notifications.php

+										'site_id'         => $credentials['oauth2_client_id'],
+										'site_secret'     => $credentials['oauth2_client_secret'],
+										'notification_id' => $data['notificationID'],
+										'notification_state' => $data['notificationState'],


@felixarntz I'm not sure why, but PHPCS says that this alignment is correct.

I think it wants to => to be aligned?

'site_id' => $credentials['oauth2_client_id'], 'site_secret' => $credentials['oauth2_client_secret'], 'notification_id' => $data['notificationID'], 'notification_state' => $data['notificationState'],

It should want that, but it automatically fixes it as highlighted above (which should be wrong)

aaemnnosttv · 2020-02-25T16:14:32Z

includes/Core/Notifications/Notifications.php

+		if ( ! empty( $decoded['error'] ) ) {
+			throw new Exception( $decoded['error'] );
+		}


@felixarntz Should we ever display an error message returned by the remote? Currently it's logged in the console only.

A generic error message (like a 500, if the proxy's notification endpoint was down/erroring) seems wrong to display, because it's not very actionable/helpful to the user. I'd think an allow-list of errors to display would work if wanted, but otherwise it seems best to log them to the console for debugging/support. Otherwise hide 'em?

Yeah I think we should stick to only logging and don't render any user-facing messages. It should just silently error then.

aaemnnosttv · 2020-02-25T18:03:24Z

tests/phpunit/integration/Core/REST_API/REST_RoutesTest.php

+			'/' . REST_Routes::REST_ROOT . '/core/site/data/notifications',
+			'/' . REST_Routes::REST_ROOT . '/core/site/data/mark-notification',


@felixarntz I noticed that without these the test failed because they're hooked in during the main bootstrapping process. We could (and probably should) move the route tests to their respective classes that add them, and only test for the routes that are added in this class specifically. We just need to clear the filter at the beginning like we do in others (i.e. remove_all_filters( 'googlesitekit_rest_routes' )). This was the simpler change to make for now though. Let me know if you'd like me to make that change also here or we could do later in another issue.

Yeah that makes sense. Could you do that in a follow-up PR? If we can get it into this release, we can even just connect it to the same issue. I just don't want that work to block this PR.

aaemnnosttv · 2020-02-25T18:15:28Z

includes/Core/Notifications/Notifications.php

+	private function missing_required_param( $param ) {
+		return new WP_Error(
+			'missing_required_param',
+			/* translators: %s: Missing parameter name */
+			sprintf( __( 'Request parameter is empty: %s.', 'google-site-kit' ), $param ),
+			array( 'status' => 400 )
+		);
+	}


@felixarntz I've thought about refactoring with something like this before since there is quite a bit of duplication for this kind of thing in the datapoint handlers. Ideally we would use core's args definitions and leverage the native validation the API already provides. The problem is that our data requests stuff all the parameters into one data parameter (except for a few outliers like module activation), and the REST API doesn't support nested validation except for collections. This may not be the best place to discuss this but just thought I'd raise it.

Yeah I thought about this initially too, but our support for batch requests would make use of that core feature challenging. I think for now we should keep this internal to the respective handlers (although adding utility methods, potentially even a validation class or trait for common validation features is certainly worth considering).

Yeah I thought about this initially too, but our support for batch requests would make use of that core feature challenging.

It would require that we change the way batch requests are handled to dispatch the individual REST requests (internally-w/o HTTP) in the batch handler rather than the way it is now but I think it's possible. Better to discuss this in a new issue though.

googlebot · 2020-02-26T11:30:13Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tofumatt

These visual changes seem off to me:

Approach here looks good. I know I helped write it, but the HTML sanitizing here is safe (it's the same approach I've used on many projects and trusted on some giant sites) which is good. I left thoughts on some of the code but I don't think any comments are blocking/much to fix.

But those visual changes strike me as unintentional.

tofumatt · 2020-02-26T13:09:52Z

assets/js/components/notifications/dashboard-core-site-alerts.js

+	async componentDidMount() {
+		const notifications = await getNotifications().catch( ( e ) => {
+			// eslint-disable-next-line no-console
+			console.warn( 'Error caught while fetching notifications', JSON.stringify( e ) );


I often use global.console for these. It probably doesn't matter? I dunno!

Also: why stringify the error? console can output objects.

Yeah, I know but it doesn't work when logging from E2E tests.

https://travis-ci.com/google/site-kit-wp/jobs/291425794#L1104

I didn't mean to keep it like that though so I'll update it.

tofumatt · 2020-02-26T13:13:00Z

assets/js/components/notifications/dashboard-core-site-alerts.js

+	}
+
+	async componentDidMount() {
+		const notifications = await getNotifications().catch( ( e ) => {


For async/await code it's usually the norm to do try/catch blocks instead of catch. Any reason to prefer catch here?

Suggested change

const notifications = await getNotifications().catch( ( e ) => {

try {

const notifications = await getNotifications();

this.setState( { notifications } );

} catch ( err ) {

// eslint-disable-next-line no-console

console.warn( 'Error caught while fetching notifications', JSON.stringify( err ) );

}

I dunno what notifications get set to when this fails, but it seems off to set notifications if there was an error fetching them too.

tofumatt · 2020-02-26T13:14:19Z

assets/js/components/notifications/dashboard-core-site-alerts.js

+		}
+
+		return notifications.map(
+			( notification ) => <Notification


Just a nitpick, but I find this a bit tougher to read than:

( notification ) => ( <Notification {…props} /> )

tofumatt · 2020-02-26T13:15:16Z

assets/js/components/notifications/dashboard-core-site-alerts.js

+				ctaLink={ notification.ctaURL || '' }
+				ctaLabel={ notification.ctaLabel || '' }
+				ctaTarget={ notification.ctaTarget || '' }
+				dismiss={ notification.dismissLabel || __( 'OK, Got it!', 'google-site-kit' ) }


Suggested change

dismiss={ notification.dismissLabel || __( 'OK, Got it!', 'google-site-kit' ) }

dismiss={ notification.dismissLabel || __( 'OK, got it!', 'google-site-kit' ) }

The "G" here shouldn't be capitalised if not a new sentence.

There's a bunch of these in the codebase. I've updated them all so let's see how that goes...

tofumatt · 2020-02-26T13:18:34Z

includes/Core/Notifications/Notification.php

+		$this->args = array_merge(
+			array(
+				'title'            => '',
+				'content'          => '',


Since these are always empty-string by default, any reason the component does notification.content || '' when passing props? Seems unneeded. 🤷‍♂ Belt and braces I guess?

This ensures that the keys are always set on the PHP side.

tofumatt · 2020-02-26T13:20:54Z

includes/Core/Notifications/Notifications.php

+										'site_id'         => $credentials['oauth2_client_id'],
+										'site_secret'     => $credentials['oauth2_client_secret'],
+										'notification_id' => $data['notificationID'],
+										'notification_state' => $data['notificationState'],


I think it wants to => to be aligned?

'site_id' => $credentials['oauth2_client_id'], 'site_secret' => $credentials['oauth2_client_secret'], 'notification_id' => $data['notificationID'], 'notification_state' => $data['notificationState'],

tofumatt · 2020-02-26T13:22:32Z

includes/Core/Notifications/Notifications.php

+		if ( ! empty( $decoded['error'] ) ) {
+			throw new Exception( $decoded['error'] );
+		}


A generic error message (like a 500, if the proxy's notification endpoint was down/erroring) seems wrong to display, because it's not very actionable/helpful to the user. I'd think an allow-list of errors to display would work if wanted, but otherwise it seems best to log them to the console for debugging/support. Otherwise hide 'em?

felixarntz

@aaemnnosttv

One byproduct of this change is that we needed to separate the description into its own element

Can you clarify why this is necessary? Wouldn't it be possible to use Fragment dangerouslySetInnerHTML=... instead? It would be great if we could avoid that change, so that it doesn't result in additional paragraphs just for the learn more link.

Otherwise, this looks good to me once @tofumatt's comments are addressed.

aaemnnosttv · 2020-02-26T17:43:27Z

Can you clarify why this is necessary? Wouldn't it be possible to use Fragment dangerouslySetInnerHTML=... instead? It would be great if we could avoid that change, so that it doesn't result in additional paragraphs just for the learn more link.

@felixarntz Fragments don't support using dangerouslySetInnerHTML 😞
See facebook/react#12014

It would be great if we could avoid that change, so that it doesn't result in additional paragraphs just for the learn more link.

I think the UI is a bit cleaner and more consistent with the separation myself, but if we wanted to keep them, we could wrap description and the learn more with spans instead and then wrap that with a <p> tag to let them render inline.

These visual changes seem off to me:

@tofumatt They are different, but not unexpected. Like I said, I think they're an improvement but we can revert that if necessary.
See #1170 (comment)

felixarntz · 2020-02-26T18:02:14Z

@aaemnnosttv

I think the UI is a bit cleaner and more consistent with the separation myself, but if we wanted to keep them, we could wrap description and the learn more with spans instead and then wrap that with a <p> tag to let them render inline.

Yeah I thought about that too, let's use spans then and maintain the original appearance.

This reverts commit e92c9b5.

felixarntz

@aaemnnosttv Testing this again, there's just one more thing that needs to be updated here.

felixarntz · 2020-02-26T20:10:33Z

assets/js/components/notifications/dashboard-core-site-alerts.js

+					dismiss={ notification.dismissLabel || __( 'OK, Got it!', 'google-site-kit' ) }
+					isDismissable={ notification.dismissible }
+					onCTAClick={ async () => {
+						await acceptNotification( notification.id );


We need to make the notice dismiss here, also when the user accepts it. It could either be through manual logic in this callback here, or alternatively we could introduce some kind of dismissOnCTA flag prop in Notification to generally support that?

The thing is right now, it opens the link in a new tab (which is what should happen). The problem is then the notice still remains open in the other window. We need to make sure it dismissed on the JS side.

I just pushed a small update to address this although it needs testing still.

Basically, if no onCTAClick handler is passed, the notification will dismiss on CTA click if dismissible.

If an onCTAClick handler is passed then that callback is responsible for calling dismiss or not, which is passed to the callback as the second argument (first being the event). If the event isn't dismissible, the dismiss function is a no-op.

The callback highlighted above has then been updated to call dismiss after acceptNotification 😄

To do this, I had to separate the handleDismiss event handler from the main dismiss logic to prevent calling dismissNotification after dismissing it from the onCTAClick handler 😄 - onDismiss is only called by the event handler (when clicking "dismiss"), not when called by an onCTAClick callback.

Note this also implies that all existing dismissible notifications with CTA links will be dismissed when clicked, although I think that's what should probably happen anyways?

https://github.com/google/site-kit-wp/pull/1170/files/4c4e0bc4feabb77eb739a9a822db342da08f054a..cd676504fd1ff30843177b17be636ac05e06c2b6

felixarntz · 2020-02-26T23:17:07Z

assets/js/components/notifications/notification.js

+			onCTAClick( e, dismiss );
+		} else {
+			dismiss( e );
+		}


Revisiting this, wouldn't it make more sense to make handleCTAClick async and then await onCTAClick, without passing dismiss to it? Then we would always call dismiss at the end. This would make it more similar to handleDismiss and avoid the extra parameter.

Sure we could do that. My initial approach here assumes that there might be cases where we don't want to dismiss on CTA click, even if the notification is dismissible, but that does sound like a bit of an edge case 😄

This looks cleaner indeed though so assuming the above isn't a concern, let's go with it 👍

felixarntz · 2020-02-26T23:25:08Z

assets/js/components/notifications/notification.js

-		const card = this.cardRef.current;
-
+	async handleDismiss( e ) {
+		e.persist();


What's this for?

Since these events are actually SyntheticEvents from React, we need to persist them using this method to ensure they are not disposed of to make them available to any subsequent callbacks. We don't actually use the events right now, but if we're going to pass it, it should be persisted.

aaemnnosttv · 2020-02-27T10:57:59Z

@felixarntz I've simplified the handleCTAClick as you suggested and testing has been going well.
Ready for another pass 👍

See: #943 (comment)

Whoops! 😅

felixarntz

Awesome!

These changes were reverted anyway.

aaemnnosttv added 17 commits February 24, 2020 19:53

Add Core/Notifications/Notification and test.

bc89955

Add Core\Notifications\Notifications + 1st route.

9e7de51

Rename to_json to prepare_for_js.

be33775

Implement notifications/mark route.

c1c07fc

Add Notifications test.

6bb8adc

Register Notifications.

66445a8

Update Notification test.

40ed764

Add new event handlers to notification component.

0fcacfa

Add site notifications api.

7ff399f

Add DashboardCoreSiteAlerts.

7433a27

Fix destructuring.

44e7007

Update notification mapping.

bdfeca5

Update Notifications routes.

114ee4f

Fix notification mapping during render.

bad10dd

Return null if no notifications.

8effab5

Hook in DashboardCoreSiteAlerts.

6984cb6

Ensure notifications cache is invalidated on mark.

4a9f134

googlebot added the cla: yes label Feb 25, 2020

aaemnnosttv added 4 commits February 25, 2020 16:26

Update REST_Routes test.

c20a4b2

Prevent possible error if WP_Error has no reason.

9517250

Improve error messages for JSON errors.

79baa72

Add error handling to notifications request.

d923774

aaemnnosttv commented Feb 25, 2020

View reviewed changes

aaemnnosttv added 4 commits February 25, 2020 20:08

Add core site notification mu API mock.

e5d7eee

Add testSiteNotification e2e util.

e86765c

Add initial dashboard site notifications e2e test.

f01f676

Document e2e notifications option.

b08c25e

aaemnnosttv commented Feb 25, 2020

View reviewed changes

googlebot added cla: yes and removed cla: no labels Feb 26, 2020

tofumatt previously requested changes Feb 26, 2020

View reviewed changes

felixarntz requested changes Feb 26, 2020

View reviewed changes

aaemnnosttv added 4 commits February 26, 2020 20:29

Revert "Approve visual changes."

4b1e17e

This reverts commit e92c9b5.

Only set notifications if no error during fetch and log full error.

3034b63

Update formatting of notification component mapping.

20465e8

Preserve inline layout of notification description.

95d2f07

aaemnnosttv requested review from tofumatt and felixarntz February 26, 2020 19:04

felixarntz requested changes Feb 26, 2020

View reviewed changes

felixarntz and others added 4 commits February 26, 2020 12:20

Merge branch 'develop' into add/1110-remote-notifications

4c4e0bc

Split dismiss into event handler and state update.

ae8a548

Update onCTAClick with new event handler method.

cf88641

Dismiss on CTA click for site notifications.

cd67650

felixarntz reviewed Feb 26, 2020

View reviewed changes

Simplify dismiss handling in handleCTAClick.

dea9668

aaemnnosttv requested a review from felixarntz February 27, 2020 10:58

tofumatt added 2 commits February 27, 2020 11:56

Allow strings or React components as Notification descriptions.

3812a03

See: #943 (comment)

Remove ErrorComponent.

57da512

Whoops! 😅

felixarntz approved these changes Feb 27, 2020

View reviewed changes

felixarntz merged commit dee2ef6 into develop Feb 27, 2020

felixarntz deleted the add/1110-remote-notifications branch February 27, 2020 15:21

aaemnnosttv mentioned this pull request Feb 27, 2020

Leverage React error boundaries to improve user experience of client side errors #943

Closed

		'/' . REST_Routes::REST_ROOT . '/core/site/data/notifications',
		'/' . REST_Routes::REST_ROOT . '/core/site/data/mark-notification',

-		const notifications = await getNotifications().catch( ( e ) => {
+		try {
+			const notifications = await getNotifications();
+			this.setState( { notifications } );
+		} catch ( err ) {
+			// eslint-disable-next-line no-console
+			console.warn( 'Error caught while fetching notifications', JSON.stringify( err ) );
+		}

	dismiss={ notification.dismissLabel \|\| __( 'OK, Got it!', 'google-site-kit' ) }
	dismiss={ notification.dismissLabel \|\| __( 'OK, got it!', 'google-site-kit' ) }

Add support for remote notifications #1170

Add support for remote notifications #1170

Conversation

aaemnnosttv commented Feb 25, 2020 • edited Loading

Summary

Relevant technical choices

Checklist

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

googlebot commented Feb 26, 2020

tofumatt 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

felixarntz left a comment

Choose a reason for hiding this comment

aaemnnosttv commented Feb 26, 2020

felixarntz commented Feb 26, 2020

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaemnnosttv Feb 26, 2020 • edited Loading

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

aaemnnosttv commented Feb 27, 2020

felixarntz left a comment

Choose a reason for hiding this comment

aaemnnosttv commented Feb 25, 2020 •

edited

Loading

aaemnnosttv Feb 26, 2020 •

edited

Loading