Screen notifications settings page

[r-a-y]

We currently have a way for users to disable email notifications via a user's "Settings > Email" page.

We need a way for users to be able to disable screen notifications as well.

This presents a few things we need to tackle:

• Add a new "Settings > Notifications" page.
  • The "Settings > Email" page already uses the slug 'notifications'. Perhaps this new page will use a slug like 'screen-notifications'?
• Plugins need a way to register settings fields for inclusion on a "Settings > Notifications" page.
  • If no plugins have registered any settings, offer some generic fields based on the 'component_name' so users can disable all notifications for a component all at once.
• Add some form of user meta check into bp_notifications_add_notification() so the notification will not be recorded if a user has chosen to disable it for a given 'component_action'.

FYI, Twitter calls their email notifications page - Email Notifications. And their screen notifications page - Web Notifications. Something to think about when we decide to build this out.

[r-a-y]

01.patch implements the "Settings > Notifications" page with the slug screen-notifications.

For devs, to register component notification actions, you need to add a 'notification_action_callback' key similar to the existing 'notification_callback' key for formatting notifications. During the callback, you can use bp_notifications_set_action() to register each notification action.

When a component does not have any notification actions registered (this can be expected for plugins), we fallback to a generic 'Receive screen notifications if I select "Yes" for this option' row. See comment:8.

This option also serves the purpose of disabling all notifications for a component:

This option only shows up if more than one notification action is registered for a component.

I could some use UX feedback for the 'I select "Yes" for this option' text, as I'm not quite satisfied with it. Some other labels I was mulling around include 'Suspend screen notifications' and 'If I enable them'. See comment:8.

---

Some other dev notes:

• Where possible, I've moved all existing email code from bp-X-notifications.php into bp-X-functions.php as emails are technically not a part of the Notifications component.
• bp-X-notifications.php is now loaded only if the Notifications component is active.
• For components to set the settings component title, I've created a generic, helper function called bp_notifications_set_prop() so devs can inject any custom properties needed. View the patch to see how I'm using this.
• For devs to pull up all notification actions, use the bp_notifications_init_registered_actions() function. This ensures that we fetch notification actions only when needed.

Let me know if you have any questions.

[r-a-y]

Refreshed patch and have updated the UI on the "Settings > Notifications" page.

Here's a new GIF:

There is now a checkbox to toggle screen notifications for each component. The UI in comment:5 was too confusing. (Serves me right for trying to re-use the "Settings > Email" UI!)

In the GIF, the XProfile component does not have any built-in screen notifications, so that is why the "Yes / No" table does not show. This will occur when a component or plugin has not registered any notification actions yet. However, this allows users to disable screen notifications for the entire component until the component or plugin is able to register notification actions with BP.

Let me know what you think of the approach.

[r-a-y]

New patch for 2.6

[DJPaul]

I will look at the patch in detail soon but my first comment is we cannot call components "components" to regular users. That's a developer and site administrator term only, and shouldn't be exposed to regular users. But this is only wording, and trivial to change.

I'm looking forward to testing this soon!

[r-a-y]

Requires 6712.02.notifications-conditional-loading.patch

[r-a-y]

I've refreshed the patch and have split it in two.

• 6712.02.notifications-conditional-loading.patch moves all email-related code out of bp-{$component}-notifications.php and conditionally loads it if the notifications component is active.
• 6712.02.screen-notifications.patch is the actual patch implementing the screen notifications page.

The template is still rough around the edges code-wise, but I'm waiting for feedback on the look and feel before iterating further.

[imath]

That's amazing work wow! I'll look at it more in details soon. But my first comment would be: the src/bp-templates/bp-legacy/buddypress/members/single/settings/screen-notifications.php really needs to be less scary!! There's some inline PHP code and inline Javascript code. Is there a way to avoid it ?

Just noticed your inline comment about moving the Javascript code :)

[hnla]

Going to run this too - looks really good r-a-y

[r-a-y]

But my first comment would be: the src/bp-templates/bp-legacy/buddypress/members/single/settings/screen-notifications.php really needs to be less scary!! There's some inline PHP code and inline Javascript code. Is there a way to avoid it ?

Yeah, I know the template is a little rough with regards to inline PHP at the moment.

See comment:10, specifically:

The template is still rough around the edges code-wise, but I'm waiting for feedback on the look and feel before iterating further.

I'm thinking about moving the table code to a template tag. I'll work on this a bit later on so it isn't so scary :)

[DJPaul]

I've been doing thinking about this recently. I remembered some discussions Boone, John, Rocío, and I, had at the last WCSF: ​https://bpdevel.wordpress.com/2014/11/12/at-wcsf-some-attendees-of/. Please read the "Activity component" section. It broadly covers a concept designed to overhaul how any kind of notification is done internally, and the flexibility that it would provide to the core BuddyPress experience, and for developers. I still believe this is a worthy goal.

The general idea that any kind of action on a site triggers an event which handles sending notifications to affected user. "Email" or "toolbar" would just become one type of notification (not dissimilar from how WordPress supports post types -- it's still a content type, just handled differently).

I'm not intending to de-rail this ticket by saying "we shouldn't build this until we have that other thing done", but just that consideration is given to this future goal so we can make the best decisions for today without stitching ourselves up in the future.

1) The existing "[user] > Settings > Emails" screen should encompass both Emails and Toolbar notifications.

There should not be a separate screen for each *type* of notification. For example, if we ever wanted to add SMS Text Message support to BuddyPress core (an unlikely example), having to introduce a third notification screen for text messages -- with virtually the same layout as toolbar and emails -- is a very poor user experience.

In terms of what this means for this feature, rather than "yes" and "no" as options, we could have something like a radio button for each notification type for "only email", "only toolbar", "both".

We don't need to build UI support for any kind of custom notification type API at the moment (apart from basic hooks and filters) because the underlying notifications PHP architecture will require a substantial overhaul.

2) The underlying implementation for updating/fetching email or toolbar notification preferences doesn't need to be unified yet.

We can use the new functions you've proposed for toolbar notifications, and the existing functions for email notifications, etc. These can be unified in the future if/when the subscription changes occur.

:)

[hnla]

@r-a-y It's a great addition r-a-y, big patch :)

I haven't read Pauls comments fully above but, testing the patches and visually speaking the new screen on twentyfifteen tended to leap out at me due to a lot of components without actions so just a heading and checkbox/label.

I've added a patch to re-work the screen template to use fieldsets and legends replacing heading elements and wrapping the checkbox/table actions plus adding a intro paragraph similar to the email screen - it's just an attempt to match a little more to other screens.

There probably would need to be a little massaging of styles at the end but minor stuff. maybe border-bottom separators or odd/even light background just to set off the individual sections.

[hnla]

Update screen template markup

[hnla]

Paul Said:

1) The existing "[user] > Settings > Emails" screen should encompass both Emails and Toolbar notifications.

There should not be a separate screen for each *type* of notification

This was bothering me too slightly, but I think where we might have one screen or top level it needs to be labelled 'Notifications' encompassing screen ones, email ones, and future additions

[r-a-y]

Thanks for the feedback everyone.

1) The existing "[user] > Settings > Emails" screen should encompass both Emails and Toolbar notifications.

.

I'm not intending to de-rail this ticket by saying "we shouldn't build this until we have that other thing done", but just that consideration is given to this future goal so we can make the best decisions for today without stitching ourselves up in the future.

I was thinking about merging the Screen Notifications page myself. The problem is our current "Settings > Email" API is quite terrible.

If we believe that there should be just one Notification settings page that encompasses Emails, Screen Notifications (and potentially more), we should only add this functionality once we figure out how to handle outputting and saving existing Email settings and figuring out a system that would work for third-party components as well.

I'm not sure what can be done here.

#6932 actually proposes a schema to handle registering email notification metakeys, but that would only work for core emails. Existing plugins using the 'bp_notification_settings' <table> hook would not be displayed. I'm not sure if it is possible to maintain backward compatibility.

If we want to merge the Email and Screen Notification settings pages, it's most likely that this will have to move to Future Release.

Adding a separate settings page and merging it later will infuriorate many BuddyPress installations.

but, testing the patches and visually speaking the new screen on twentyfifteen tended to leap out at me due to a lot of components without actions so just a heading and checkbox/label.

Yeah, I made the decision to output any component that has registered itself with notifications support. We can decide to only show components that have registered notification actions if that is better for UX.

[DJPaul]

If we believe that there should be just one Notification settings page that... we should only add this functionality once we figure out... a system that would work for third-party components as well.

I disagree. I think you are letting perfect be the enemy of good. As I said in my last comment (here it is again):

The underlying implementation for updating/fetching... (preferences) doesn't need to be unified yet. We can use the new functions you've proposed for toolbar notifications, and the existing functions for email notifications, etc. These can be unified in the future.

[r-a-y]

I don't think I can merge the proposed screen notification settings with the existing "Settings > Email" page in time for 2.6 because I don't know what this is supposed to look like.

I'd need some UI feedback or wireframes to iterate.

I actually wouldn't mind the page staying separate, but would welcome groupthink here :)

[r-a-y]

In 11022:

Load all notification-related code conditionally for each component.

If the notifications component isn't active, there is no need to load all
notification code.

See #6712.

[r-a-y]

I've committed 6712.02.notifications-conditional-loading.patch.

You should now be able to apply ​6712.02.screen-notifications.patch cleanly now.

[mercime]

Thank you @r-a-y. Worked on two mockups in browser for the all-in-one Notifications pages with HTML markup changes

Version 1

<h2 class="bp-screen-reader-text">Notification Settings</h2>

<p>Note: Add any other instructions before the form</p>

<form action="http://localhost/codex/members/admin/settings/screen-notifications" method="post" class="standard-form" id="settings-form">

	<fieldset>
	<legend>Activity</legend>
		<p id="mention-update">A member mentions you in an update using "@admin"</p>
			<label for="activity-notification-new-email" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-email" value="1" checked="checked" type="checkbox">
			Email</label>
			<label for="activity-notification-new-screen" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-screen" value="0" type="checkbox">
			Screen</label>
			<label for="activity-notification-new-sms" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-sms" value="0" type="checkbox">
			SMS</label>

		<p id="activity-reply">A member replies to an update or comment you've posted</p>
			<label for="notification-activity-new-reply-email" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-email" value="1" checked="checked" type="checkbox">
			Email</label>
			<label for="notification-activity-new-reply-screen" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-screen" value="0" type="checkbox">
			Screen</label>
			<label for="notification-activity-new-reply-sms" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-sms" value="0" type="checkbox">
			SMS</label>
	</fieldset>
	. . .  

---

Version 2

<h2 class="bp-screen-reader-text">Notification Settings</h2>

<p>Note: Add any other instructions before the form</p>

<form action="http://localhost/codex/members/admin/settings/screen-notifications" method="post" class="standard-form" id="settings-form">

	<h3>Activity</h3>
	<fieldset>
	<legend>Enable notifications for all activities</legend>
	<button type="button" aria-pressed="true">Hide</button>
		<p id="mention-update">A member mentions you in an update using "@admin"</p>
			<label for="activity-notification-new-email" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-email" value="1" checked="checked" type="checkbox">
			Email</label>
			<label for="activity-notification-new-screen" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-screen" value="0" type="checkbox">
			Screen</label>
			<label for="activity-notification-new-sms" aria-describedby="mention-update">
				<input name="notifications[notification_activity_new_mention]" id="activity-notification-new-sms" value="0" type="checkbox">
			SMS</label>

		<p id="activity-reply">A member replies to an update or comment you've posted</p>
			<label for="notification-activity-new-reply-email" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-email" value="1" checked="checked" type="checkbox">
			Email</label>
			<label for="notification-activity-new-reply-screen" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-screen" value="0" type="checkbox">
			Screen</label>
			<label for="notification-activity-new-reply-sms" aria-describedby="activity-reply">
				<input name="notifications[notification_activity_new_reply]" id="notification-activity-new-reply-sms" value="0" type="checkbox">
			SMS</label>
	</fieldset>
	...

[r-a-y]

Thanks for the mockups, @mercime!

After looking into this some more, the problem I'm seeing is there are some notifications that are applicable only to email and some that are only applicable to screen (or web) notifications.

My remarks in comment:20 still stand:

If we believe that there should be just one Notification settings page that encompasses Emails, Screen Notifications (and potentially more), we should only add this functionality once we figure out how to handle outputting and saving existing Email settings and figuring out a system that would work for third-party components as well.

#6932 actually proposes a schema to handle registering email notification metakeys, but that would only work for core emails. Existing plugins using the 'bp_notification_settings' <table> hook would not be displayed. I'm not sure if it is possible to maintain backward compatibility.

---

That being said, I've implemented this checkbox interface in 04.patch.

Here's what I did:

• To implement this page, I object buffered the entire 'bp_notification_settings' hook and did some DOM parsing to determine what the notification options are (insert :poo: emoji here!). The positives of this is older plugins that are using the 'bp_notification_settings' hook will work. The negative is some plugins are abusing this hook to add additional information that is not a table. I also haven't added a new API for components / plugins to register separate notification sections yet. This is obviously more of a proof-of-concept at the moment, rather than a final iteration.

• Had to map existing email user meta keys to those used by the Notification component's 'component_action' DB column. The better way here is to somehow re-use the email schema outlined in #6932 - ​bp_email_get_type_schema(), but that is mostly tied to the email post type... also plugins cannot add their own schema to that function.

• Introduces a way for plugins to register a new notification type. For example, the Notifications component registers the screen notification type in bp-notification-settings.php.

• At the moment, if a plugin is adding screen notifications and they want a user to have the ability to disable it, plugins will have to register their mapped email meta key to their notification component_action. See bp_notifications_get_mapped_notification_actions(). This is all because I'm piggybacking off the existing email meta keys. This will probably need to be revamped.

• Unlike how each user email notification setting is saved individually as a separate user meta entry, the patch only saves disabled screen notification data into one user meta key - screen_notifications_disabled.

[espellcaste]

Duplicate #8997

[imath]

I wasn’t aware of this discussion, many thanks to @espellcaste for pointing me to it. I believe having the end goal to move the notifications component in an external add-on can help us « rethink » if email / web notifications should be so dependents. Let’s carry on discussing about it during next release.

[espellcaste]

Related to #8774

[espellcaste]

Related to #9078